[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 <btbn@btbn.de>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9374
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>

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

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