fix: pull request review comment position (#9914)

## Checklist

This PR contains both #9889 and #9912, since it depends on the one, and the other provides a test for it.
The exact reasoning behind its logic is described here: https://codeberg.org/forgejo/forgejo/issues/9473#issuecomment-7976186

This PR should return the behaviour back to how it was before a PR to Gitea changed it.
Only the resulting Database-Entry will reference the line blamed commit, now also with the correct adjusted line.
While the context diff view is pulled from the commit the commenter actually commented on.

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [x] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### 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

- [ ] I do not want this change to show in the release notes.
- [x] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9914
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: BtbN <btbn@btbn.de>
Co-committed-by: BtbN <btbn@btbn.de>

modules/git/repo_blame.go

@@ -7,6 +7,8 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"strconv"
+	"strings"
 )
 
 var (
@@ -17,7 +19,7 @@ var (
 )
 
 // LineBlame returns the latest commit at the given line
-func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, error) {
+func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, uint64, error) {
 	res, _, gitErr := NewCommand(repo.Ctx, "blame").
 		AddOptionFormat("-L %d,%d", line, line).
 		AddOptionValues("-p", revision).
@@ -26,24 +28,40 @@ func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit,
 		stdErr := gitErr.Stderr()
 
 		if stdErr == fmt.Sprintf("fatal: no such path %s in %s\n", file, revision) {
-			return nil, ErrBlameFileDoesNotExist
+			return nil, 0, ErrBlameFileDoesNotExist
 		}
 		if notEnoughLinesRe.MatchString(stdErr) {
-			return nil, ErrBlameFileNotEnoughLines
+			return nil, 0, ErrBlameFileNotEnoughLines
 		}
 
-		return nil, gitErr
+		return nil, 0, gitErr
 	}
 
 	objectFormat, err := repo.GetObjectFormat()
 	if err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 
 	objectIDLen := objectFormat.FullLength()
-	if len(res) < objectIDLen {
-		return nil, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", res)
+
+	if len(res) < objectIDLen+1 {
+		return nil, 0, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", res)
 	}
 
-	return repo.GetCommit(res[:objectIDLen])
+	commit, err := repo.GetCommit(res[:objectIDLen])
+	if err != nil {
+		return nil, 0, fmt.Errorf("GetCommit: %w", err)
+	}
+
+	endIdxOriginalLineNo := strings.IndexRune(res[objectIDLen+1:], ' ')
+	if endIdxOriginalLineNo == -1 {
+		return nil, 0, fmt.Errorf("output of blame is invalid, cannot contain original line number: %s", res)
+	}
+
+	originalLineNo, err := strconv.ParseUint(res[objectIDLen+1:objectIDLen+1+endIdxOriginalLineNo], 10, 64)
+	if err != nil {
+		return nil, 0, fmt.Errorf("strconv.ParseUint: %w", err)
+	}
+
+	return commit, originalLineNo, nil
 }

modules/git/repo_blame_test.go

@@ -4,6 +4,9 @@
 package git
 
 import (
+	"bytes"
+	"os"
+	"path"
 	"path/filepath"
 	"testing"
 
@@ -17,17 +20,20 @@ func TestLineBlame(t *testing.T) {
 		require.NoError(t, err)
 		defer repo.Close()
 
-		commit, err := repo.LineBlame("HEAD", "foo/link_short", 1)
+		commit, lineno, err := repo.LineBlame("HEAD", "foo/link_short", 1)
 		require.NoError(t, err)
 		assert.Equal(t, "37991dec2c8e592043f47155ce4808d4580f9123", commit.ID.String())
+		assert.EqualValues(t, 1, lineno)
 
-		commit, err = repo.LineBlame("HEAD", "foo/link_short", 512)
+		commit, lineno, err = repo.LineBlame("HEAD", "foo/link_short", 512)
 		require.ErrorIs(t, err, ErrBlameFileNotEnoughLines)
 		assert.Nil(t, commit)
+		assert.Zero(t, lineno)
 
-		commit, err = repo.LineBlame("HEAD", "non-existent/path", 512)
+		commit, lineno, err = repo.LineBlame("HEAD", "non-existent/path", 512)
 		require.ErrorIs(t, err, ErrBlameFileDoesNotExist)
 		assert.Nil(t, commit)
+		assert.Zero(t, lineno)
 	})
 
 	t.Run("SHA256", func(t *testing.T) {
@@ -37,16 +43,68 @@ func TestLineBlame(t *testing.T) {
 		require.NoError(t, err)
 		defer repo.Close()
 
-		commit, err := repo.LineBlame("HEAD", "foo/link_short", 1)
+		commit, lineno, err := repo.LineBlame("HEAD", "foo/link_short", 1)
 		require.NoError(t, err)
 		assert.Equal(t, "6aae864a3d1d0d6a5be0cc64028c1e7021e2632b031fd8eb82afc5a283d1c3d1", commit.ID.String())
+		assert.EqualValues(t, 1, lineno)
 
-		commit, err = repo.LineBlame("HEAD", "foo/link_short", 512)
+		commit, lineno, err = repo.LineBlame("HEAD", "foo/link_short", 512)
 		require.ErrorIs(t, err, ErrBlameFileNotEnoughLines)
 		assert.Nil(t, commit)
+		assert.Zero(t, lineno)
 
-		commit, err = repo.LineBlame("HEAD", "non-existent/path", 512)
+		commit, lineno, err = repo.LineBlame("HEAD", "non-existent/path", 512)
 		require.ErrorIs(t, err, ErrBlameFileDoesNotExist)
 		assert.Nil(t, commit)
+		assert.Zero(t, lineno)
+	})
+
+	t.Run("Moved line", func(t *testing.T) {
+		test := func(t *testing.T, objectFormatName string) {
+			t.Helper()
+			tmpDir := t.TempDir()
+
+			require.NoError(t, InitRepository(t.Context(), tmpDir, false, objectFormatName))
+
+			gitRepo, err := OpenRepository(t.Context(), tmpDir)
+			require.NoError(t, err)
+			defer gitRepo.Close()
+
+			require.NoError(t, os.WriteFile(path.Join(tmpDir, "ANSWER"), []byte("abba\n"), 0o666))
+			require.NoError(t, AddChanges(tmpDir, true))
+			require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "Favourite singer of everyone who follows a automata course"}))
+
+			firstCommit, err := gitRepo.GetRefCommitID("HEAD")
+			require.NoError(t, err)
+
+			require.NoError(t, os.WriteFile(path.Join(tmpDir, "ANSWER"), append(bytes.Repeat([]byte("baba\n"), 9), []byte("abba\n")...), 0o666))
+			require.NoError(t, AddChanges(tmpDir, true))
+			require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "Now there's several of them"}))
+
+			secondCommit, err := gitRepo.GetRefCommitID("HEAD")
+			require.NoError(t, err)
+
+			commit, lineno, err := gitRepo.LineBlame("HEAD", "ANSWER", 10)
+			require.NoError(t, err)
+			assert.Equal(t, firstCommit, commit.ID.String())
+			assert.EqualValues(t, 1, lineno)
+
+			for i := range uint64(9) {
+				commit, lineno, err = gitRepo.LineBlame("HEAD", "ANSWER", i+1)
+				require.NoError(t, err)
+				assert.Equal(t, secondCommit, commit.ID.String())
+				assert.Equal(t, i+1, lineno)
+			}
+		}
+
+		t.Run("SHA1", func(t *testing.T) {
+			test(t, Sha1ObjectFormat.Name())
+		})
+
+		t.Run("SHA256", func(t *testing.T) {
+			skipIfSHA256NotSupported(t)
+
+			test(t, Sha256ObjectFormat.Name())
+		})
 	})
 }

services/pull/review.go

@@ -44,7 +44,7 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error {
 // If the line got changed the comment is going to be invalidated.
 func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *git.Repository, branch string) error {
 	// FIXME differentiate between previous and proposed line
-	commit, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine())
+	commit, _, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine())
 	if err != nil && (errors.Is(err, git.ErrBlameFileDoesNotExist) || errors.Is(err, git.ErrBlameFileNotEnoughLines)) {
 		c.Invalidated = true
 		return issues_model.UpdateCommentInvalidate(ctx, c)
@@ -178,7 +178,8 @@ 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 string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) {
-	var commitID, patch string
+	var commitID, blamedCommitID, patch string
+	blamedLine := line
 	if err := issue.LoadPullRequest(ctx); err != nil {
 		return nil, fmt.Errorf("LoadPullRequest: %w", err)
 	}
@@ -226,12 +227,15 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
 			// FIXME validate treePath
 			// Get latest commit referencing the commented line
 			// No need for get commit for base branch changes
-			commit, err := gitRepo.LineBlame(head, treePath, uint64(line))
+			commit, lineres, err := gitRepo.LineBlame(head, treePath, uint64(line))
 			if err == nil {
-				commitID = commit.ID.String()
+				blamedCommitID = commit.ID.String()
+				blamedLine = int64(lineres)
 			} else if !errors.Is(err, git.ErrBlameFileDoesNotExist) && !errors.Is(err, git.ErrBlameFileNotEnoughLines) {
 				return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
 			}
+		} else {
+			blamedCommitID = commitID
 		}
 	}
 
@@ -243,6 +247,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
 				return nil, fmt.Errorf("GetRefCommitID[%s]: %w", head, err)
 			}
 		}
+		if len(blamedCommitID) == 0 {
+			blamedCommitID = commitID
+		}
 		reader, writer := io.Pipe()
 		defer func() {
 			_ = reader.Close()
@@ -268,9 +275,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
 		Repo:        repo,
 		Issue:       issue,
 		Content:     content,
-		LineNum:     line,
+		LineNum:     blamedLine,
 		TreePath:    treePath,
-		CommitSHA:   commitID,
+		CommitSHA:   blamedCommitID,
 		ReviewID:    reviewID,
 		Patch:       patch,
 		Invalidated: invalidated,

tests/e2e/declare_repos_test.go

@@ -5,6 +5,7 @@ package e2e
 
 import (
 	"fmt"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -130,6 +131,19 @@ body:
 			postIssue(repo, user, 450, "right issue", "an issue containing word right")
 			postIssue(repo, user, 150, "left issue", "an issue containing word left")
 		}),
+		newRepo(t, 2, "long-diff-test", nil, []FileChanges{{
+			Filename: "test-README.md",
+			Versions: []string{
+				readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/0-README.md"),
+			},
+		}}, func(user *user_model.User, repo *repo_model.Repository) {
+			commit1Sha := addCommitToBranch(t, user, repo, "main", "test-branch", "test-README.md", "",
+				readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/1-README.md"))
+			commit2Sha := addCommitToBranch(t, user, repo, "test-branch", "test-branch", "test-README.md", commit1Sha,
+				readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/2-README.md"))
+			addCommitToBranch(t, user, repo, "test-branch", "test-branch", "test-README.md", commit2Sha,
+				readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/3-README.md"))
+		}),
 		// add your repo declarations here
 	}
 
@@ -140,6 +154,12 @@ body:
 	}
 }
 
+func readStringFile(t *testing.T, fn string) string {
+	c, err := os.ReadFile(fn)
+	require.NoError(t, err)
+	return string(c)
+}
+
 func newRepo(t *testing.T, userID int64, repoName string, initOpts *tests.DeclarativeRepoOptions, fileChanges []FileChanges, setup SetupRepo) func() {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID})
 
@@ -206,3 +226,32 @@ func newRepo(t *testing.T, userID int64, repoName string, initOpts *tests.Declar
 
 	return cleanupFunc
 }
+
+func addCommitToBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, oldBranch, newBranch, filename, lastSha, content string) string {
+	resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, user, &files_service.ChangeRepoFilesOptions{
+		Files: []*files_service.ChangeRepoFile{{
+			Operation:     "update",
+			TreePath:      filename,
+			ContentReader: strings.NewReader(content),
+		}},
+		Message:   "add commit to branch",
+		OldBranch: oldBranch,
+		NewBranch: newBranch,
+		Author: &files_service.IdentityOptions{
+			Name:  user.Name,
+			Email: user.Email,
+		},
+		Committer: &files_service.IdentityOptions{
+			Name:  user.Name,
+			Email: user.Email,
+		},
+		Dates: &files_service.CommitDateOptions{
+			Author:    time.Now(),
+			Committer: time.Now(),
+		},
+		LastCommitID: lastSha,
+	})
+	require.NoError(t, err)
+	assert.NotEmpty(t, resp)
+	return resp.Commit.SHA
+}

tests/e2e/pr-review.test.e2e.ts

@@ -133,3 +133,55 @@ test('PR: Test mentions values', async ({page}) => {
   await page.locator("ul.suggestions li[data-value='@user1']").click();
   await expect(page.locator('.review-box-panel textarea#_combo_markdown_editor_0')).toHaveValue('@user1 ');
 });
+
+test('multi-commit commenting', async ({page, request}) => {
+  const response = await page.goto('/user2/long-diff-test');
+  expect(response?.status()).toBe(200);
+
+  try {
+    await page.getByText('2 branches').click(); // navigate to branch list
+    await page.getByText('New pull request').click(); // load compare view for the branch
+    await page.locator('.show-form-container').getByText('New pull request').click(); // actually open the PR form
+    await page.locator('.primary.button').getByText('Create pull request').click(); // submit PR creation
+
+    // Test situation: adding a comment on a line that was created in the *second* commit, doing it from the "Files changed" view.
+    await page.getByText('Files changed').click();
+    await page.getByText('More  This line was changed in commit 2')
+      .locator('..')
+      .locator('button.add-code-comment')
+      .click();
+    await page.getByPlaceholder('Leave a comment').fill('Comment on line changed in commit 2');
+    await page.getByText('Add single comment').click();
+
+    // Test assertion: when viewing the comment from the 'Conversation' page, it's diff should look correct:
+    await page.getByText('Conversation').click();
+    await expect(page.locator('.pull.menu .item.active')).toContainText('Conversation'); // ensure we navigated back to Conversation page
+    await expect(page.locator('.text.comment-content .render-content.markup')).toHaveText('Comment on line changed in commit 2');
+    await expect(page.locator('.diff-file-box .code-diff')).toContainText('More  This line was changed in commit 2');
+
+    // Test assertion: when viewing the comment from the second commit, it should be placed correctly in the UI:
+    await page.getByRole('link', {name: 'Commits'}).click();
+    await page.getByText('add commit to branch').nth(1).click();
+    // FIXME: The intent of this test is to make sure that the comment box appears in the "right spot", which is *below*
+    // the line of code that it was commented on.  This check uses the elements bounding boxes which... is pretty ugly
+    // and could be done better.  Probably would be better to find the line of code that the box is rendered on,
+    // instead.
+    const codeLine = await page.getByText('More  This line was changed in commit 2').boundingBox();
+    const commentBox = await page.locator('.render-content.markup').getByText('Comment on line changed in commit 2').boundingBox();
+    expect(commentBox.y).toBeGreaterThan(codeLine.y);
+  } finally {
+    // Delete any PRs on the test repo so that this test can be rerun.
+    const issuesResp = await request.get(`/api/v1/repos/user2/long-diff-test/issues`);
+    expect(issuesResp.ok()).toBeTruthy();
+    const issues = await issuesResp.json();
+    for (const issue of issues) {
+      const delResp = await request.delete(`/api/v1/repos/user2/long-diff-test/issues/${issue.number}`, {
+        headers: {
+          'Content-Type': 'application/json',
+          'Authorization': `Basic ${btoa(`user1:password`)}`,
+        },
+      });
+      expect(delResp.ok()).toBeTruthy();
+    }
+  }
+});