diff --git a/models/issues/comment.go b/models/issues/comment.go index 624e4352e1..c7b8119377 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -897,8 +897,43 @@ func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repo } anchorResolvedLine := anchorBlame.LineNumber - // A line that no longer resolves is treated as "modified but present" and tolerated; only a - // resolved line landing at an unexpected offset (lines inserted/removed inside the range) is a break. + // Catch a line changed by a later commit by comparing each range line's current content + // to the comment's Patch (its content at creation; lines the PR itself changed already match it). + // The Patch uses the comment's line coordinates, so trust it only + // when the anchor's recorded content equals its current content — otherwise fall back below. + if expected := git.PatchRightSideContent(c.Patch); len(expected) > 0 { + if headLines, ok := c.headFileLines(gitRepo, currentHead, anchorBlame.FilePath); ok { + lineAt := func(n uint64) (string, bool) { + if n >= 1 && n <= uint64(len(headLines)) { + return headLines[n-1], true + } + return "", false + } + anchorExpected, hasAnchor := expected[int64(c.UnsignedLine())] + anchorCurrent, hasCurrent := lineAt(anchorResolvedLine) + if hasAnchor && hasCurrent && anchorExpected == anchorCurrent { + trusted := true + for i := int64(1); i <= c.ExtraLinesCount; i++ { + exp, okExp := expected[int64(c.UnsignedLine())+i] + cur, okCur := lineAt(anchorResolvedLine + uint64(i)) + if !okExp || !okCur { + trusted = false // mapping incomplete -> fall back to the offset-only check + break + } + if exp != cur { + return "invalid", nil // a range line was changed after the comment was made + } + } + if trusted { + return "valid", nil + } + } + } + } + + // Fallback (offset-only): a line that no longer resolves is treated as "modified but present" + // and tolerated; only a resolved line landing at an unexpected offset (lines inserted/removed + // inside the range) is a break. startLine := c.UnsignedLine() for i := int64(1); i <= c.ExtraLinesCount; i++ { blame, err := c.resolveLineAtHead(gitRepo, startLine+uint64(i), currentHead) @@ -919,6 +954,25 @@ func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repo return resultJSON == "valid", nil } +// headFileLines reads the content of treePath at the given commit and returns its lines +// (dropping the trailing empty element produced by a final newline). ok is false when the +// file can't be read at head. +func (c *Comment) headFileLines(gitRepo *git.Repository, head, treePath string) (lines []string, ok bool) { + commit, err := gitRepo.GetCommit(head) + if err != nil { + return nil, false + } + content, err := commit.GetFileContent(treePath, -1) + if err != nil { + return nil, false + } + lines = strings.Split(content, "\n") + if n := len(lines); n > 0 && lines[n-1] == "" { + lines = lines[:n-1] + } + return lines, true +} + // CodeCommentLink returns the url to a comment in code func (c *Comment) CodeCommentLink(ctx context.Context) string { err := c.LoadIssue(ctx) diff --git a/modules/git/diff.go b/modules/git/diff.go index 35368411c1..73091db404 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -278,6 +278,53 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi return strings.Join(newHunk, "\n"), nil } +// PatchRightSideContent parses a unified diff patch (such as a stored code-comment +// Patch) and returns the content of each line on the right (new) side, keyed by its +// new line number. Added ('+') and context (' ') lines are included; removed ('-') +// lines and the "\ No newline at end of file" marker are skipped. It lets callers +// recover the file content captured when a comment was created, to detect whether +// the commented lines were changed afterwards. +func PatchRightSideContent(patch string) map[int64]string { + result := make(map[int64]string) + scanner := bufio.NewScanner(strings.NewReader(patch)) + scanner.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + + var rightLine int64 + inHunk := false + for scanner.Scan() { + line := scanner.Text() + if strings.HasPrefix(line, "@@") { + submatches := hunkRegex.FindStringSubmatch(line) + if submatches == nil { + inHunk = false + continue + } + for i, name := range hunkRegex.SubexpNames() { + if name == "beginNew" { + rightLine, _ = strconv.ParseInt(submatches[i], 10, 64) + } + } + inHunk = rightLine > 0 + continue + } + if !inHunk || len(line) == 0 { + continue + } + switch line[0] { + case '+', ' ': + result[rightLine] = line[1:] + rightLine++ + case '-', '\\': + // '-' is left-only; '\' is the "no newline at end of file" marker + break + default: + // a non-hunk line (e.g. the next "diff --git" header); stop this hunk + inHunk = false + } + } + return result +} + var ErrLineNotFound = errors.New("line not found in diff") type LinePlacement struct { diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 36cfd7fe25..8fa870f13a 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -436,3 +436,47 @@ index 2d203fb..d0cb63f 100644 require.ErrorIs(t, err, ErrLineNotFound) }) } + +func TestPatchRightSideContent(t *testing.T) { + t.Run("empty", func(t *testing.T) { + assert.Empty(t, PatchRightSideContent("")) + }) + + t.Run("single hunk with a replacement", func(t *testing.T) { + patch := "diff --git a/file1.md b/file1.md\n" + + "--- a/file1.md\n" + + "+++ b/file1.md\n" + + "@@ -48,3 +48,3 @@\n" + + " Line 48\n" + + " Line 49\n" + + "-Line 50\n" + + "+Line 50--modified" + assert.Equal(t, map[int64]string{ + 48: "Line 48", + 49: "Line 49", + 50: "Line 50--modified", + }, PatchRightSideContent(patch)) + }) + + t.Run("added lines shift the right side", func(t *testing.T) { + patch := "@@ -10,2 +10,4 @@\n" + + " ctx\n" + + "+added a\n" + + "+added b\n" + + " after" + assert.Equal(t, map[int64]string{ + 10: "ctx", + 11: "added a", + 12: "added b", + 13: "after", + }, PatchRightSideContent(patch)) + }) + + t.Run("removed lines do not consume right-side numbers", func(t *testing.T) { + patch := "@@ -5,3 +5,1 @@\n" + + "-gone 1\n" + + "-gone 2\n" + + " kept" + assert.Equal(t, map[int64]string{5: "kept"}, PatchRightSideContent(patch)) + }) +} diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 85777e78f6..741a916f1a 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -35,6 +35,7 @@ import ( "forgejo.org/modules/test" issue_service "forgejo.org/services/issue" "forgejo.org/services/mailer" + pull_service "forgejo.org/services/pull" repo_service "forgejo.org/services/repository" files_service "forgejo.org/services/repository/files" "forgejo.org/tests" @@ -2016,10 +2017,15 @@ func TestPullRequestCommentPlacement(t *testing.T) { // Push a second commit that changes lines BEFORE the range (removing lines 1-10), // which shifts the range but keeps it contiguous. 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) - tester.changeFile("file1.md", content) + newSHA := tester.changeFile("file1.md", content) + + // Run the invalidation pass synchronously instead of waiting for the async + // goroutine, then check the comment is still valid. + pr, err := issues_model.GetPullRequestByIndex(t.Context(), tester.repo.ID, tester.pr.Index) + require.NoError(t, err) + require.NoError(t, pull_service.InvalidateCodeComments(t.Context(), + issues_model.PullRequestList{pr}, tester.user, tester.repo, newSHA)) - // Wait a bit for async invalidation to run, then check the comment is still valid. - time.Sleep(2 * time.Second) commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) assert.False(t, commentReloaded.Invalidated) @@ -2038,6 +2044,92 @@ func TestPullRequestCommentPlacement(t *testing.T) { } tester.assertFilesChangedDiff(diff) }) + + // Helper: comment on lines 48-50 (anchor 48, middle 49, last 50, all modified by the PR), then a + // later commit applies `secondCommit` to the file and we check the resulting invalidation state. + runRangeInvalidation := func(t *testing.T, secondCommit func(content string) string, wantInvalidated bool) { + tester := newPullRequestCommentPlacementTester(t) + + content := tester.fileContent + content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1) + content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1) + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + tester.changeFile("file1.md", content) + tester.createPR() + + comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2) + assert.EqualValues(t, 48, comment.Line) + assert.EqualValues(t, 2, comment.ExtraLinesCount) + assert.False(t, comment.Invalidated) + + newSHA := tester.changeFile("file1.md", secondCommit(content)) + + if wantInvalidated { + assert.EventuallyWithT(t, func(t *assert.CollectT) { + commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) + assert.True(t, commentReloaded.Invalidated) + }, 5*time.Second, 50*time.Millisecond) + } else { + // Run the invalidation pass synchronously instead of waiting for the async + // goroutine, then assert the comment stayed valid. + pr, err := issues_model.GetPullRequestByIndex(t.Context(), tester.repo.ID, tester.pr.Index) + require.NoError(t, err) + require.NoError(t, pull_service.InvalidateCodeComments(t.Context(), + issues_model.PullRequestList{pr}, tester.user, tester.repo, newSHA)) + + commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) + assert.False(t, commentReloaded.Invalidated) + } + } + + t.Run("multi-line comment invalidated when a middle line is deleted by a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 49--modified\n", "", 1) + }, true) + }) + + t.Run("multi-line comment invalidated when a middle line content changes in a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 49--modified\n", "Line 49--changed-again\n", 1) + }, true) + }) + + t.Run("multi-line comment invalidated when the last line content changes in a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 50--modified\n", "Line 50--changed-again\n", 1) + }, true) + }) + + t.Run("multi-line comment not invalidated when a line outside the range changes", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 60\n", "Line 60--modified\n", 1) + }, false) + }) + + t.Run("multi-line comment invalidated when the last line is deleted by a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 50--modified\n", "", 1) + }, true) + }) + + t.Run("multi-line comment invalidated when the first (anchor) line content changes in a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 48--modified\n", "Line 48--changed-again\n", 1) + }, true) + }) + + t.Run("multi-line comment invalidated when the first (anchor) line is deleted by a later commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + runRangeInvalidation(t, func(content string) string { + return strings.Replace(content, "Line 48--modified\n", "", 1) + }, true) + }) }) }