From c08bdaacdb9ef4debead340bf0eeab7726fa5c8b Mon Sep 17 00:00:00 2001 From: BtbN Date: Sat, 20 Sep 2025 22:47:52 +0200 Subject: [PATCH] fix: use correct commit when fetching patch view for review comments (#9264) This is effectively a clean revert of e7a77d32cc96692f5dbbd3ed1cfa7ff4a9ba026b, 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. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9264 Reviewed-by: Gusted Co-authored-by: BtbN Co-committed-by: BtbN --- 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 b0ab700fa6..8efe7d6bcb 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -237,12 +237,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() { @@ -250,8 +249,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 b385916f96..131e8537c7 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")