From 968b2c5dc12bc7327f7b6135cf484b5cc1e28dc3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 20:54:16 -0700 Subject: [PATCH] Fix bug --- models/issues/comment_test.go | 11 +++-- models/issues/pull.go | 7 ++- routers/web/repo/pull_review.go | 48 ++++++++++++++++---- services/automergequeue/automergequeue.go | 4 +- services/convert/pull_review.go | 4 ++ services/issue/issue.go | 5 +++ services/issue/pull.go | 26 +---------- services/pull/merge.go | 6 +++ services/pull/review.go | 54 +++++++++++++++++------ 9 files changed, 113 insertions(+), 52 deletions(-) diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index 627cc188a66..b5849c6e905 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -71,9 +71,14 @@ func TestFetchCodeComments(t *testing.T) { res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false) assert.NoError(t, err) assert.Contains(t, res, "README.md") - assert.Contains(t, res["README.md"], int64(4)) - assert.Len(t, res["README.md"][4], 1) - assert.Equal(t, int64(4), res["README.md"][0].ID) + fourthLineComments := []*issues_model.Comment{} + for _, comment := range res["README.md"] { + if comment.Line == 4 { + fourthLineComments = append(fourthLineComments, comment) + } + } + assert.Len(t, fourthLineComments, 1) + assert.Equal(t, int64(4), fourthLineComments[0].ID) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false) diff --git a/models/issues/pull.go b/models/issues/pull.go index 6d737076e5f..77af38e0afa 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -392,11 +392,16 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) return committer.Commit() } -// GetGitHeadRefName returns git ref for hidden pull request branch +// GetGitHeadRefName returns git head commit id ref for the pull request's branch func (pr *PullRequest) GetGitHeadRefName() string { return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index) } +// GetGitMergeRefName returns git merged commit id ref for the pull request +func (pr *PullRequest) GetGitMergeRefName() string { + return fmt.Sprintf("%s%d/merge", git.PullPrefix, pr.Index) +} + func (pr *PullRequest) GetGitHeadBranchRefName() string { return fmt.Sprintf("%s%s", git.BranchPrefix, pr.HeadBranch) } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index a8fdd613a7b..29b436e9fc3 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -7,11 +7,13 @@ import ( "errors" "fmt" "net/http" + "slices" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" pull_model "code.gitea.io/gitea/models/pull" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -170,20 +172,50 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori ctx.ServerError("comment.Issue.LoadPullRequest", err) return } - if beforeCommitID == "" { - beforeCommitID = comment.Issue.PullRequest.MergeBase + + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) + if err != nil { + ctx.ServerError("GetRefCommitID", err) + return } - if afterCommitID == "" { - var err error - afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) - if err != nil { - ctx.ServerError("GetRefCommitID", err) + + prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID) + if err != nil { + ctx.ServerError("CommitIDsBetween", err) + return + } + + if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase { + beforeCommitID = comment.Issue.PullRequest.MergeBase + } else { + // beforeCommitID must be one of the pull request commits + if !slices.Contains(prCommitIDs, beforeCommitID) { + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) return } + + beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + + beforeCommit, err = beforeCommit.Parent(0) + if err != nil { + ctx.ServerError("GetParent", err) + return + } + beforeCommitID = beforeCommit.ID.String() + } + if afterCommitID == "" { + afterCommitID = headCommitID + } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)) + return } showOutdatedComments := origin == "timeline" || ctx.Data["ShowOutdatedComments"].(bool) - comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.Repository, comment.IssueID, + comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, comment.IssueID, ctx.Doer, beforeCommitID, afterCommitID, comment.TreePath, comment.Line, showOutdatedComments) if err != nil { ctx.ServerError("FetchCodeCommentsByLine", err) diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index fa9c04da874..aed8cded441 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques return } defer gitRepo.Close() - commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) if err != nil { log.Error("GetRefCommitID: %v", err) return } - AddToQueue(pull, commitID) + AddToQueue(pull, headCommitID) } diff --git a/services/convert/pull_review.go b/services/convert/pull_review.go index 3dcf4dea745..755e416d7d5 100644 --- a/services/convert/pull_review.go +++ b/services/convert/pull_review.go @@ -21,6 +21,10 @@ func ToPullReview(ctx context.Context, r *issues_model.Review, doer *user_model. r.Reviewer = user_model.NewGhostUser() } + if err := r.Issue.LoadRepo(ctx); err != nil { + return nil, err + } + result := &api.PullReview{ ID: r.ID, Reviewer: ToUser(ctx, r.Reviewer, doer), diff --git a/services/issue/issue.go b/services/issue/issue.go index 70a395eb12c..332eb5c674a 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -204,6 +204,11 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil { return err } + if issue.PullRequest.HasMerged { + if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { + return err + } + } } notify_service.DeleteIssue(ctx, doer, issue) diff --git a/services/issue/pull.go b/services/issue/pull.go index 512fdf78e84..9dd818be39d 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -7,33 +7,15 @@ import ( "context" "fmt" "slices" - "time" issues_model "code.gitea.io/gitea/models/issues" org_model "code.gitea.io/gitea/models/organization" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" ) -func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { - // Add a temporary remote - tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) - if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { - return "", fmt.Errorf("AddRemote: %w", err) - } - defer func() { - if err := repo.RemoveRemote(tmpRemote); err != nil { - log.Error("getMergeBase: RemoveRemote: %v", err) - } - }() - - mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) - return mergeBase, err -} - type ReviewRequestNotifier struct { Comment *issues_model.Comment IsAdd bool @@ -96,15 +78,9 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque return nil, nil } - // get the mergebase - mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName()) - if err != nil { - return nil, err - } - // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed // between the merge base and the head commit but not the base branch and the head commit - changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitHeadRefName()) + changedFiles, err := repo.GetFilesChangedBetween(pr.MergeBase, pr.GetGitHeadRefName()) if err != nil { return nil, err } diff --git a/services/pull/merge.go b/services/pull/merge.go index cd9aeb2ad1e..1bb1b56de49 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -726,6 +726,12 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, issues_model.ErrIssueAlreadyChanged } + // update merge ref, this is necessary to ensure pr.MergedCommitID can be used to do diff operations even + // if the repository rebased/force-pushed and the pull request's merge commit is no longer in the history + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil { + return false, fmt.Errorf("UpdateRef: %w", err) + } + if err := committer.Commit(); err != nil { return false, err } diff --git a/services/pull/review.go b/services/pull/review.go index 9f03f60c376..fc3fc5b9179 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -113,7 +113,12 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. defer closer.Close() } - prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID) + headCommitID, err := gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) + } + + prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID) if err != nil { return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err) } @@ -138,10 +143,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. beforeCommitID = beforeCommit.ID.String() } if afterCommitID == "" { - afterCommitID, err = gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) - } + afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) } @@ -520,7 +522,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { } // FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request. -func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { +func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { opts := issues_model.FindCommentsOptions{ Type: issues_model.CommentTypeCode, IssueID: issueID, @@ -538,7 +540,24 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i n := 0 hunksCache := make(map[string][]*git.HunkInfo) for _, comment := range comments { - if comment.CommitSHA == endCommitID { + // Code comment should always have a commit SHA, if not, we need to set it based on the line number + if comment.CommitSHA == "" { + if comment.Line > 0 { + comment.CommitSHA = endCommitID + } else if comment.Line < 0 { + comment.CommitSHA = startCommitID + } else { + // If the comment has no line number, we cannot display it in the diff view + continue + } + } + + dstCommitID := startCommitID + if comment.Line > 0 { + dstCommitID = endCommitID + } + + if comment.CommitSHA == dstCommitID { if comment.Line == line { comments[n] = comment n++ @@ -547,18 +566,27 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i } // If the comment is not for the current commit, we need to recalculate the line number - hunks, ok := hunksCache[comment.CommitSHA] + hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] if !ok { - hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, endCommitID, treePath) + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, treePath) if err != nil { - return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, endCommitID, err) + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err) } - hunksCache[comment.CommitSHA] = hunks + hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks } comment.Line = ReCalculateLineNumber(hunks, comment.Line) - comments[n] = comment - n++ + if comment.Line != 0 { + dstCommit, err := gitRepo.GetCommit(dstCommitID) + if err != nil { + return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) + } + // If the comment is not the first one or the comment created before the current commit + if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { + comments[n] = comment + n++ + } + } } return comments[:n], nil }