From 179fbdb04e90a73f64e1910f8ca157fa6e1f678f Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Tue, 14 Apr 2026 17:18:14 +0200 Subject: [PATCH] fix: when reviewing in PRs, make comments relative to viewed base & head, not just viewed head (#12107) While developing tests for #12092, I came across a case where making a comment on a single-commit doesn't include the correct diff for the comment. This is because code comment placement occurs between the PR's base and the commit being viewed, but, that diff could be different from the commit's parent to the commit, which is what is being viewed on a single-commit diff. Similar to #12055, this PR changes code comments to be more precise in their diff generation by providing the backend with both the base commit (`before_commit_id`) and head commit (`after_commit_id`) currently being viewed. As a result, the diffs attached to comments should exactly match the diffs being viewed by the user when the comment was placed. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12107 Reviewed-by: Andreas Ahlenstorf Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- models/issues/comment.go | 2 +- routers/api/v1/repo/pull_review.go | 2 + routers/web/repo/pull.go | 6 +- routers/web/repo/pull_review.go | 10 +++ routers/web/repo/pull_review_test.go | 2 +- services/forms/repo_form.go | 1 + services/mailer/incoming/incoming_handler.go | 1 + services/pull/review.go | 20 +++-- templates/repo/diff/box.tmpl | 2 +- templates/repo/diff/comment_form.tmpl | 1 + tests/integration/pull_review_test.go | 81 +++++++++++++++++++- 11 files changed, 116 insertions(+), 12 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index b42cd8aa40..e0ea1e111d 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -798,7 +798,7 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos // instead, return a blame where CommitID != headCommitID, which will be an indicator to callers (for // both resolution methods) that the line of code is outdated in the diff. reverseBlame = &git.ReverseLineBlame{ - CommitID: c.CommitSHA, // not currentHead + CommitID: "", // not currentHead LineNumber: c.UnsignedLine(), FilePath: c.TreePath, } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index e478f93655..e28c1edafc 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -337,6 +337,7 @@ func CreatePullReviewComment(ctx *context.APIContext) { pr.Issue, opts.Body, opts.Path, + pr.MergeBase, review.CommitID, line, review.ID, @@ -509,6 +510,7 @@ func CreatePullReview(ctx *context.APIContext) { c.Path, true, // pending review 0, // no reply + pr.MergeBase, opts.CommitID, nil, ); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4a8217c719..2c5c7f94f3 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1027,7 +1027,11 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } endCommitID = commitID - startCommitID = prInfo.MergeBase + if prevCommit != nil { + startCommitID = prevCommit.ID.String() + } else { + startCommitID = prInfo.MergeBase + } ctx.Data["IsShowingAllCommits"] = false } else if willShowSpecifiedCommitRange { if len(specifiedEndCommit) > 0 { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index c2e30770d3..401f100d84 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -53,6 +53,15 @@ func RenderNewCodeCommentForm(ctx *context.Context) { } } ctx.Data["AfterCommitID"] = afterCommitID + beforeCommitID := ctx.FormString("before_commit_id") + if beforeCommitID == "" { + if err := issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("LoadPullRequest", err) + return + } + beforeCommitID = issue.PullRequest.MergeBase + } + ctx.Data["BeforeCommitID"] = beforeCommitID ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled upload.AddUploadContext(ctx, "comment") ctx.HTML(http.StatusOK, tplNewComment) @@ -112,6 +121,7 @@ func CreateCodeComment(ctx *context.Context) { form.TreePath, pendingReview, form.Reply, + form.BeforeCommitID, form.LatestCommitID, attachments, ) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index dd0fa7df29..c068f575dd 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -51,7 +51,7 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, - 1, "content", "", false, 0, + 1, "content", "", false, 0, pr.MergeBase, prHeadCommitID, nil) require.NoError(t, err) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index f9443b8b6c..d1a5b521f8 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -472,6 +472,7 @@ type CodeCommentForm struct { TreePath string `form:"path" binding:"Required"` SingleReview bool `form:"single_review"` Reply int64 `form:"reply"` + BeforeCommitID string LatestCommitID string Files []string } diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index e2a2852b07..6fdf00ca59 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -152,6 +152,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u comment.TreePath, false, // not pending review but a single review comment.ReviewID, + issue.PullRequest.MergeBase, afterCommitID, attachmentIDs, ) diff --git a/services/pull/review.go b/services/pull/review.go index 9fc9862823..d8e04675ea 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -78,7 +78,10 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis } // CreateCodeComment creates a comment on the code line -func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string, attachments []string) (*issues_model.Comment, error) { +func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, + issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, + replyReviewID int64, beforeCommitID, latestCommitID string, attachments []string, +) (*issues_model.Comment, error) { var ( existsReview bool err error @@ -109,6 +112,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + beforeCommitID, latestCommitID, line, replyReviewID, @@ -151,6 +155,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + beforeCommitID, latestCommitID, line, review.ID, @@ -173,7 +178,10 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } // CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path -func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath, afterCommitID string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { +func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, + issue *issues_model.Issue, content, treePath, beforeCommitID, afterCommitID string, + line, reviewID int64, attachments []string, +) (*issues_model.Comment, error) { var commitID, blamedCommitID, patch string blamedLine := line if err := issue.LoadPullRequest(ctx); err != nil { @@ -237,9 +245,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // Commenting on a line that was removed. In this case, what we want to track in the comment is which line of // code was this, in the last commit that the line of code actually existed in. We'll use a reverse git blame to // identify this, from the PR base -> commit being viewed. - blame, err := gitRepo.ReverseLineBlame(pr.MergeBase, treePath, uint64(-1*line), afterCommitID) + blame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line), afterCommitID) if err != nil { - return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", pr.MergeBase, treePath, -1*line, afterCommitID, err) + return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line, afterCommitID, err) } else if blame.CommitID == afterCommitID { // Although this is a comment on the "previous" side of the diff, the reverse blame indicates that the line // of code still exists in the commit being viewed (eg. it was a comment on a white line in the left-side of @@ -277,8 +285,8 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, _ = writer.Close() }() go func() { - 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)) + if err := git.GetRepoRawDiffForFile(gitRepo, beforeCommitID, afterCommitID, git.RawDiffNormal, treePath, writer); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, afterCommitID, treePath, err)) return } _ = writer.Close() diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 5bd9a50b3b..7577ff0921 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -201,7 +201,7 @@ {{end}} {{else}} - +
{{if $.IsSplitStyle}} {{template "repo/diff/section_split" dict "file" . "root" $}} {{else}} diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 922f3f3d73..7b64be71a0 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -1,6 +1,7 @@ {{if and $.root.SignedUserID (not $.Repository.IsArchived)}} + diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index b91fdab6d4..8c7b302c68 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -1730,6 +1730,67 @@ func TestPullRequestCommentPlacement(t *testing.T) { assert.True(t, commentReloaded.Invalidated) }, 1*time.Second, 50*time.Millisecond) }) + + t.Run("comment on removed line in specific commit adjusts to correct location", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // 3 commits: modify line 50, remove line 50, remove some earlier lines + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + t.Logf("commit1 = %q", commit1) + content = strings.Replace(content, "Line 50--modified\n", "", 1) + commit2 := tester.changeFile("file1.md", content) + t.Logf("commit2 = %q", commit2) + content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1) + commit3 := tester.changeFile("file1.md", content) + t.Logf("commit3 = %q", commit3) + tester.createPR() + + comment := tester.commentOnPreviousFromSpecificCommit(commit2, "file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -47,7 +47,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + + diff2 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertCommitDiff(commit2, diff2, "checking commit2 contents in single-commit diff") + + diff3 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + // This is a small bug -- the comment was placed on the code "Line 50--modified" in commit1, which was + // later amended by commit2. This comment should be marked out-of-date and not appear here. But the + // comment's `ResolveCurrentLine` doesn't quite detect this case correctly -- as the comment's CommitSHA + // is commit1, it performs a diff commit1..PR-HEAD, not mergebase..PR-HEAD, and it believes that this + // line of code still exists because it exists in that diff range. It's a rare edge case that is defered + // for future repair. + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff3, "checking overall contents in full PR diff") + }) }) } @@ -1862,16 +1923,29 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromFilesChang return tester.commentFromNewCommentForm(resp, filename, line, "previous") } +func (tester *PullRequestCommentPlacementTester) getCommitParent(commitID string) string { + repo, err := gitrepo.OpenRepository(tester.t.Context(), tester.repo) + require.NoError(tester.t, err) + defer repo.Close() + commit, err := repo.GetCommit(commitID) + require.NoError(tester.t, err) + require.Len(tester.t, commit.Parents, 1) + return commit.Parents[0].String() +} + func (tester *PullRequestCommentPlacementTester) commentFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + beforeCommitID := tester.getCommitParent(commitID) req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitID)) + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?before_commit_id=%s&after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, beforeCommitID, commitID)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) return tester.commentFromNewCommentForm(resp, filename, line, "proposed") } func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + beforeCommitID := tester.getCommitParent(commitID) + tester.t.Logf("beforeCommitID(%q) = %q", commitID, beforeCommitID) req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitID)) + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?before_commit_id=%s&after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, beforeCommitID, commitID)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) return tester.commentFromNewCommentForm(resp, filename, line, "previous") } @@ -1879,10 +1953,13 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCo func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp *httptest.ResponseRecorder, filename string, line int, side string) *issues_model.Comment { commentContent := uuid.New().String() doc := NewHTMLParser(tester.t, resp.Body) + tester.t.Logf("doc.before = %q", doc.GetInputValueByName("before_commit_id")) + tester.t.Logf("doc.latest = %q", doc.GetInputValueByName("latest_commit_id")) req := NewRequestWithValues(tester.t, "POST", fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index), map[string]string{ "origin": doc.GetInputValueByName("origin"), + "before_commit_id": doc.GetInputValueByName("before_commit_id"), "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), "side": side, // "proposed" (RHS) or "previous" (LHS) "line": strconv.Itoa(line),