From 4b8a01c87949ca0614e61693fbf335f0b3bcc2e3 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Mon, 22 Sep 2025 03:28:05 +0200 Subject: [PATCH] [v12.0/forgejo] fix: use correct commit when fetching patch view for review comments (#9374) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/9264 This is effectively a clean revert of e7a77d32cc, which for some reason broke this logic. Yes, always having the latest code shown in the diff is nice, but doing this mismatches the commitID and the line number the comment is being posted on, which very easily and frequently leads to reviews that show a completely nonsensical patch above them, cause the code it's referring too shifted by several lines since the time the review was started and the time it was posted. Co-authored-by: BtbN Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9374 Reviewed-by: Gusted Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- services/pull/review.go | 13 +++---- tests/integration/pull_review_test.go | 54 +++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/services/pull/review.go b/services/pull/review.go index c740328e4c..52a85959b3 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -241,12 +241,11 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // Only fetch diff if comment is review comment if len(patch) == 0 && reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", pr.GetGitRefName(), err) - } if len(commitID) == 0 { - commitID = headCommitID + commitID, err = gitRepo.GetRefCommitID(head) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %w", head, err) + } } reader, writer := io.Pipe() defer func() { @@ -254,8 +253,8 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, _ = writer.Close() }() go func() { - if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, writer); err != nil { - _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, headCommitID, treePath, err)) + if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, writer); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, commitID, treePath, err)) return } _ = writer.Close() diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 39e9cb7007..bd59a13ce2 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -527,6 +527,60 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) { }) } +func TestPullReview_OldLatestCommitId(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user1") + + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: baseRepo.ID, Index: 3}) + + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + require.NoError(t, err) + defer baseGitRepo.Close() + + headCommitSHA, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + require.NoError(t, err) + + headCommit, err := baseGitRepo.GetCommit(headCommitSHA) + require.NoError(t, err) + require.GreaterOrEqual(t, headCommit.ParentCount(), 1) + + parentCommit, err := headCommit.Parent(0) + require.NoError(t, err) + oldCommitSHA := parentCommit.ID.String() + require.NotEqual(t, headCommitSHA, oldCommitSHA) + + req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + + const content = "TestPullReview_OldLatestCommitId" + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{ + "_csrf": doc.GetInputValueByName("_csrf"), + "origin": doc.GetInputValueByName("origin"), + "latest_commit_id": oldCommitSHA, + "side": "proposed", + "line": "2", + "path": "iso-8859-1.txt", + "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), + "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), + "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "content": content, + "single_review": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: pr.IssueID, Content: content}) + require.NotZero(t, comment.ReviewID) + assert.Equal(t, oldCommitSHA, comment.CommitSHA) + assert.NotEqual(t, headCommitSHA, comment.CommitSHA) + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + assert.Equal(t, issues_model.ReviewTypeComment, review.Type) + assert.Equal(t, oldCommitSHA, review.CommitID) + assert.NotEqual(t, headCommitSHA, review.CommitID) +} + func TestPullReviewInArchivedRepo(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { session := loginUser(t, "user2")