From f8cea67265544ece8d1c9eca37a96eb1d969b2db Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 09:58:28 -0700 Subject: [PATCH] improvements --- modules/git/commit.go | 6 +---- modules/git/diff.go | 5 ++-- modules/git/diff_test.go | 8 +++++- modules/git/repo_commit.go | 52 ++++++++++++++++++++++++++++---------- services/issue/comments.go | 2 ++ services/pull/review.go | 23 +++++++++++++---- 6 files changed, 68 insertions(+), 28 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index aae40c575bc..7382018a347 100644 --- a/modules/git/commit.go +++ b/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 diff --git a/modules/git/diff.go b/modules/git/diff.go index 7f3d57ddbc8..34307f7768e 100644 --- a/modules/git/diff.go +++ b/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,7 +108,7 @@ 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], ",") @@ -117,7 +116,7 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH if len(rightRange) > 1 { rightHunk, _ = strconv.Atoi(rightRange[1]) } else { - rightHunk = rightLine + rightHunk = 1 } } else { log.Debug("Parse line number failed: %v", diffHunk) diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 11bdcd35fb1..8e618d354ad 100644 --- a/modules/git/diff_test.go +++ b/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) } diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 4066a1ca7ba..9ccb13b5a4e 100644 --- a/modules/git/repo_commit.go +++ b/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) + if before != "" { + _, err := repo.GetCommit(before) + if err != nil { + return nil, err + } } - beforeCommit, 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 diff --git a/services/issue/comments.go b/services/issue/comments.go index 10c81198d57..63dd3171c9d 100644 --- a/services/issue/comments.go +++ b/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 diff --git a/services/pull/review.go b/services/pull/review.go index 4e4fb31870e..2586b81b535 100644 --- a/services/pull/review.go +++ b/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: