improvements

Author: lunny

modules/git/commit.go

@@ -258,11 +258,7 @@ func (c *Commit) CommitsBeforeLimit(num int) ([]*Commit, error) {
 
 // CommitsBeforeUntil returns the commits between commitID to current revision
 func (c *Commit) CommitsBeforeUntil(commitID string) ([]*Commit, error) {
-	endCommit, err := c.repo.GetCommit(commitID)
-	if err != nil {
-		return nil, err
-	}
-	return c.repo.CommitsBetween(c, endCommit)
+	return c.repo.CommitsBetween(c.ID.String(), commitID)
 }
 
 // SearchCommitsOptions specify the parameters for SearchCommits

modules/git/diff.go

@@ -15,7 +15,6 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // RawDiffType type of a raw diff.
@@ -109,15 +108,15 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH
 	if len(leftRange) > 1 {
 		leftHunk, _ = strconv.Atoi(leftRange[1])
 	} else {
-		leftHunk = util.Iif(leftLine > 0, leftLine, -leftLine)
+		leftHunk = 1
 	}
 	if len(ranges) > 1 {
 		rightRange := strings.Split(ranges[1], ",")
 		rightLine, _ = strconv.Atoi(rightRange[0])
 		if len(rightRange) > 1 {
 			rightHunk, _ = strconv.Atoi(rightRange[1])
 		} else {
-			rightHunk = rightLine
+			rightHunk = 1
 		}
 	} else {
 		log.Debug("Parse line number failed: %v", diffHunk)

modules/git/diff_test.go

@@ -185,6 +185,12 @@ func TestParseDiffHunkString(t *testing.T) {
 	leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -1 +0,0 @@")
 	assert.Equal(t, 1, leftLine)
 	assert.Equal(t, 1, leftHunk)
-	assert.Equal(t, 0, rightLine)
+	assert.Equal(t, 1, rightLine)
 	assert.Equal(t, 0, rightHunk)
+
+	leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -2 +2 @@")
+	assert.Equal(t, 2, leftLine)
+	assert.Equal(t, 1, leftHunk)
+	assert.Equal(t, 2, rightLine)
+	assert.Equal(t, 1, rightHunk)
 }

modules/git/repo_commit.go

@@ -6,6 +6,7 @@ package git
 
 import (
 	"bytes"
+	"context"
 	"io"
 	"os"
 	"strconv"
@@ -304,23 +305,47 @@ func (repo *Repository) FilesCountBetween(startCommitID, endCommitID string) (in
 
 // CommitsBetween returns a list that contains commits between [before, last).
 // If before is detached (removed by reset + push) it is not included.
-func (repo *Repository) CommitsBetween(last, before *Commit) ([]*Commit, error) {
+func (repo *Repository) CommitsBetween(lastCommitID, beforeCommitID string) ([]*Commit, error) {
+	commitIDs, err := CommitIDsBetween(repo.Ctx, repo.Path, beforeCommitID, lastCommitID)
+	if err != nil {
+		return nil, err
+	}
+
+	commits := make([]*Commit, 0, len(commitIDs))
+	for _, commitID := range commitIDs {
+		commit, err := repo.GetCommit(commitID)
+		if err != nil {
+			return nil, err
+		}
+		commits = append(commits, commit)
+	}
+	return commits, nil
+}
+
+func CommitIDsBetween(ctx context.Context, repoPath, beforeCommitID, afterCommitID string) ([]string, error) {
 	var stdout []byte
 	var err error
-	if before == nil {
-		stdout, _, err = NewCommand("rev-list").AddDynamicArguments(last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path})
+	if beforeCommitID == "" {
+		stdout, _, err = NewCommand("rev-list").AddDynamicArguments(afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath})
 	} else {
-		stdout, _, err = NewCommand("rev-list").AddDynamicArguments(before.ID.String()+".."+last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand("rev-list").AddDynamicArguments(beforeCommitID+".."+afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath})
 		if err != nil && strings.Contains(err.Error(), "no merge base") {
 			// future versions of git >= 2.28 are likely to return an error if before and last have become unrelated.
 			// previously it would return the results of git rev-list before last so let's try that...
-			stdout, _, err = NewCommand("rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path})
+			stdout, _, err = NewCommand("rev-list").AddDynamicArguments(beforeCommitID, afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath})
 		}
 	}
 	if err != nil {
 		return nil, err
 	}
-	return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout))
+
+	commitIDs := make([]string, 0, 10)
+	for commitID := range bytes.SplitSeq(stdout, []byte{'\n'}) {
+		if len(commitID) > 0 {
+			commitIDs = append(commitIDs, string(commitID))
+		}
+	}
+	return commitIDs, nil
 }
 
 // CommitsBetweenLimit returns a list that contains at most limit commits skipping the first skip commits between [before, last)
@@ -375,18 +400,17 @@ func (repo *Repository) CommitsBetweenNotBase(last, before *Commit, baseBranch s
 
 // CommitsBetweenIDs return commits between twoe commits
 func (repo *Repository) CommitsBetweenIDs(last, before string) ([]*Commit, error) {
-	lastCommit, err := repo.GetCommit(last)
+	_, err := repo.GetCommit(last)
 	if err != nil {
 		return nil, err
 	}
-	if before == "" {
-		return repo.CommitsBetween(lastCommit, nil)
-	}
-	beforeCommit, err := repo.GetCommit(before)
-	if err != nil {
-		return nil, err
+	if before != "" {
+		_, err := repo.GetCommit(before)
+		if err != nil {
+			return nil, err
+		}
 	}
-	return repo.CommitsBetween(lastCommit, beforeCommit)
+	return repo.CommitsBetween(last, before)
 }
 
 // CommitsCountBetween return numbers of commits between two commits

services/issue/comments.go

@@ -134,6 +134,8 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
 func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
 	err := db.WithTx(ctx, func(ctx context.Context) error {
 		return issues_model.DeleteComment(ctx, comment)
+		// TODO: delete review if the comment is the last comment of the review
+		// TODO: delete comment attachments
 	})
 	if err != nil {
 		return err

services/pull/review.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"regexp"
+	"slices"
 	"sort"
 	"strings"
 
@@ -111,14 +112,23 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		defer closer.Close()
 	}
 
-	if beforeCommitID == "" {
+	prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID)
+	if err != nil {
+		return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err)
+	}
+
+	if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase {
 		beforeCommitID = issue.PullRequest.MergeBase
 	} else {
-		beforeCommit, err := gitRepo.GetCommit(beforeCommitID) // Ensure beforeCommitID is valid
+		// beforeCommitID must be one of the pull request commits
+		if !slices.Contains(prCommitIDs, beforeCommitID) {
+			return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)
+		}
+
+		beforeCommit, err := gitRepo.GetCommit(beforeCommitID)
 		if err != nil {
 			return nil, fmt.Errorf("GetCommit[%s]: %w", beforeCommitID, err)
 		}
-		// TODO: beforeCommitID must be one of the pull request commits
 
 		beforeCommit, err = beforeCommit.Parent(0)
 		if err != nil {
@@ -131,8 +141,11 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		if err != nil {
 			return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err)
 		}
-	} else { //nolint
-		// TODO: afterCommitID must be one of the pull request commits
+	} else {
+		// afterCommitID must be one of the pull request commits
+		if !slices.Contains(prCommitIDs, afterCommitID) {
+			return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
+		}
 	}
 
 	// CreateCodeComment() is used for: