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 <aahlenst@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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,
 				}

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 {

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 {

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,
 	)

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)
 

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
 }

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,
 			)

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 {
+			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, commitID, treePath, err))
+				_ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, afterCommitID, treePath, err))
 				return
 			}
 			_ = writer.Close()

templates/repo/diff/box.tmpl

@@ -201,7 +201,7 @@
 										{{end}}
 									</div>
 								{{else}}
-									<table class="chroma" data-new-comment-url="{{$.Issue.Link}}/files/reviews/new_comment?after_commit_id={{$.AfterCommitID}}" data-path="{{$file.Name}}">
+									<table class="chroma" data-new-comment-url="{{$.Issue.Link}}/files/reviews/new_comment?before_commit_id={{$.BeforeCommitID}}&after_commit_id={{$.AfterCommitID}}" data-path="{{$file.Name}}">
 										{{if $.IsSplitStyle}}
 											{{template "repo/diff/section_split" dict "file" . "root" $}}
 										{{else}}

templates/repo/diff/comment_form.tmpl

@@ -1,6 +1,7 @@
 {{if and $.root.SignedUserID (not $.Repository.IsArchived)}}
 	<form class="ui form {{if $.hidden}}tw-hidden comment-form tw-mt-2{{end}}" action="{{$.root.Issue.Link}}/files/reviews/comments" method="post">
 		<input type="hidden" name="origin" value="{{if $.root.PageIsPullFiles}}diff{{else}}timeline{{end}}">
+		<input type="hidden" name="before_commit_id" value="{{$.root.BeforeCommitID}}">
 		<input type="hidden" name="latest_commit_id" value="{{$.root.AfterCommitID}}">
 		<input type="hidden" name="side" value="{{if $.Side}}{{$.Side}}{{end}}">
 		<input type="hidden" name="line" value="{{if $.Line}}{{$.Line}}{{end}}">

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),