diff --git a/models/issues/comment.go b/models/issues/comment.go index 61d49cf7b6..b42cd8aa40 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -6,11 +6,14 @@ package issues import ( + "bytes" "context" + "errors" "fmt" "html/template" "slices" "strconv" + "strings" "unicode/utf8" "forgejo.org/models/db" @@ -771,9 +774,43 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos } defer closer.Close() - reverseBlame, err := gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead) - if err != nil { - return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err) + var reverseBlame *git.ReverseLineBlame + if c.Line > 0 { + var err error + reverseBlame, err = gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead) + if err != nil { + return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err) + } + } else { + // For comments on removed lines, perform a `git diff` between the last commit that the line of code was + // known to exist (which is recorded as CommitSHA) and the requested head. Then inspect the diff to verify + // that the removed line of code is present in the diff. + buffer := bytes.Buffer{} + err := git.GetRepoRawDiffForFile(gitRepo, c.CommitSHA, currentHead, git.RawDiffNormal, c.TreePath, &buffer) + if err != nil { + return "", fmt.Errorf("failed to get diff: %w", err) + } + + diff := buffer.String() + adjustedLine, err := git.FindAdjustedLineNumber(c.Patch, int64(c.UnsignedLine()), strings.NewReader(diff)) + if err != nil && errors.Is(err, git.ErrLineNotFound) { + // Line not found in the diff. Don't treat this as an error, because that would break the caching -- + // 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 + LineNumber: c.UnsignedLine(), + FilePath: c.TreePath, + } + } else if err != nil { + return "", fmt.Errorf("failed in finding adjusted line number: %w", err) + } else { + reverseBlame = &git.ReverseLineBlame{ + CommitID: currentHead, + LineNumber: uint64(adjustedLine.Left), + FilePath: c.TreePath, + } + } } data, err := json.Marshal(reverseBlame) diff --git a/modules/git/diff.go b/modules/git/diff.go index 507dc5b2f5..c954a933ba 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -7,6 +7,7 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" "io" "os" @@ -277,6 +278,76 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi return strings.Join(newHunk, "\n"), nil } +var ErrLineNotFound = errors.New("line not found in diff") + +type LinePlacement struct { + Left int64 + Right int64 +} + +// Find the line of code where an old line of code from an old patch is, if present, in a new patch. Given a cutDiff +// (from CutDiffAroundLine) and the line of code that it was cut from, and, given a single-file diff from the commit +// where that patch came into a new head, this routine will read through the diff and identify the new line number. It +// will only return successful if the line is exactly the same as the original line, but just placed in a new location +// due to added or removed lines in the diff before the target line of code. +func FindAdjustedLineNumber(cutDiff string, originalLine int64, fullDiff io.Reader) (LinePlacement, error) { + cutDiffSplit := strings.Split(cutDiff, "\n") + if len(cutDiffSplit) == 0 { + return LinePlacement{}, errors.New("cutDiff has no contents") + } + endOfCutDiff := cutDiffSplit[len(cutDiffSplit)-1] + + scanner := bufio.NewScanner(fullDiff) + inHunk := false // used to skip header lines before the first hunk + leftLine := int64(-1) + rightLine := int64(-1) + + for scanner.Scan() { + lineText := scanner.Text() + if strings.HasPrefix(lineText, "@@") { + // A map with named groups of our regex to recognize them later more easily + submatches := hunkRegex.FindStringSubmatch(lineText) + groups := make(map[string]string) + for i, name := range hunkRegex.SubexpNames() { + if i != 0 && name != "" { + groups[name] = submatches[i] + } + } + beginLeft, _ := strconv.ParseInt(groups["beginOld"], 10, 64) + beginRight, _ := strconv.ParseInt(groups["beginNew"], 10, 64) + leftLine = beginLeft + rightLine = beginRight + inHunk = true + } else if inHunk { + if leftLine == originalLine { + if lineText != endOfCutDiff { + return LinePlacement{}, fmt.Errorf( + "line was adjusted from index %d to %d, but contents changed from %q to %q: %w", + originalLine, leftLine, endOfCutDiff, lineText, ErrLineNotFound) + } + return LinePlacement{Left: leftLine, Right: rightLine}, nil + } + switch lineText[0] { + case '+': + rightLine++ + case '-': + leftLine++ + case '\\': + // Should be the end-of-file with "\ No newline at end of file" -- nothing to do here. + break + default: + rightLine++ + leftLine++ + } + } + } + if err := scanner.Err(); err != nil { + return LinePlacement{}, err + } + + return LinePlacement{}, fmt.Errorf("line is no longer in diff: %w", ErrLineNotFound) +} + // GetAffectedFiles returns the affected files between two commits func GetAffectedFiles(repo *Repository, oldCommitID, newCommitID string, env []string) ([]string, error) { objectFormat, err := repo.GetObjectFormat() diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 34e0695fea..e4b7ce6ace 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -167,3 +167,215 @@ func TestParseDiffHunkString(t *testing.T) { assert.Equal(t, 19, rightLine) assert.Equal(t, 5, rightHunk) } + +func TestFindAdjustedLineNumber(t *testing.T) { + commentCutDiff := `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` + + t.Run("no additional changes", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..b21df3f 100644 +--- a/file1.md ++++ b/file1.md +@@ -47,7 +47,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50 + Line 51 + Line 52 + Line 53` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 50, Right: 50}, lineNumber) + }) + + t.Run("removed lines before location", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..c85b903 100644 +--- a/file1.md ++++ b/file1.md +@@ -1,13 +1,3 @@ +-Line 1 +-Line 2 +-Line 3 +-Line 4 +-Line 5 +-Line 6 +-Line 7 +-Line 8 +-Line 9 +-Line 10 + Line 11 + Line 12 + Line 13 +@@ -47,7 +37,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50 + Line 51 + Line 52 + Line 53` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 50, Right: 40}, lineNumber) + }) + + t.Run("added lines before location", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..24b1aa6 100644 +--- a/file1.md ++++ b/file1.md +@@ -8,6 +8,11 @@ Line 7 + Line 8 + Line 9 + Line 10 ++Line 10.1 ++Line 10.2 ++Line 10.3 ++Line 10.4 ++Line 10.5 + Line 11 + Line 12 + Line 13 +@@ -47,7 +52,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50 + Line 51 + Line 52 + Line 53` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 50, Right: 55}, lineNumber) + }) + + t.Run("added and removed in lines before location", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..d0cb63f 100644 +--- a/file1.md ++++ b/file1.md +@@ -5,9 +5,11 @@ Line 4 + Line 5 + Line 6 + Line 7 +-Line 8 +-Line 9 +-Line 10 ++Line 10.1 ++Line 10.2 ++Line 10.3 ++Line 10.4 ++Line 10.5 + Line 11 + Line 12 + Line 13 +@@ -47,7 +49,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50 + Line 51 + Line 52 + Line 53` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 50, Right: 52}, lineNumber) + }) + + t.Run("changes above in the same hunk", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..f35a466 100644 +--- a/file1.md ++++ b/file1.md +@@ -42,12 +42,6 @@ Line 41 + Line 42 + Line 43 + Line 44 +-Line 45 +-Line 46 +-Line 47 +-Line 48 +-Line 49 +-Line 50 + Line 51 + Line 52 + Line 53` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 50, Right: 45}, lineNumber) + }) + + t.Run("first line in diff", func(t *testing.T) { + commentCutDiff := `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -1,4 +1,3 @@ +-Line 1` + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..a490028 100644 +--- a/file1.md ++++ b/file1.md +@@ -1,4 +1,3 @@ +-Line 1 + Line 2 + Line 3 + Line 4` + lineNumber, err := FindAdjustedLineNumber(commentCutDiff, 1, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 1, Right: 1}, lineNumber) + }) + + t.Run("adjusted line not found", func(t *testing.T) { + // "Line 50" is present here but it's no longer "-Line 50", so it should not be identified as present + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..09dd95a 100644 +--- a/file1.md ++++ b/file1.md +@@ -42,10 +42,6 @@ Line 41 + Line 42 + Line 43 + Line 44 +-Line 45 +-Line 46 +-Line 47 +-Line 48 + Line 49 + Line 50 + Line 51` + _, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.ErrorIs(t, err, ErrLineNotFound) + }) + + t.Run("adjusted line hunk not present - not changed anymore", func(t *testing.T) { + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..d0cb63f 100644 +--- a/file1.md ++++ b/file1.md +@@ -5,9 +5,11 @@ Line 4 + Line 5 + Line 6 + Line 7 +-Line 8 +-Line 9 +-Line 10 ++Line 10.1 ++Line 10.2 ++Line 10.3 ++Line 10.4 ++Line 10.5 + Line 11 + Line 12 + Line 13` + _, err := FindAdjustedLineNumber(commentCutDiff, 50, strings.NewReader(diff)) + require.ErrorIs(t, err, ErrLineNotFound) + }) +} diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 7505148978..e2a2852b07 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -12,6 +12,7 @@ import ( access_model "forgejo.org/models/perm/access" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/gitrepo" "forgejo.org/modules/log" "forgejo.org/modules/setting" "forgejo.org/modules/util" @@ -124,7 +125,24 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u return fmt.Errorf("CreateIssueComment failed: %w", err) } case issues_model.CommentTypeCode: - _, err := pull_service.CreateCodeComment( + err := issue.LoadRepo(ctx) + if err != nil { + return fmt.Errorf("LoadRepo failed: %w", err) + } + err = issue.LoadPullRequest(ctx) + if err != nil { + return fmt.Errorf("LoadPullRequest failed: %w", err) + } + repo, err := gitrepo.OpenRepository(ctx, issue.Repo) + defer repo.Close() + if err != nil { + return fmt.Errorf("OpenRepository failed: %w", err) + } + afterCommitID, err := repo.GetRefCommitID(issue.PullRequest.GetGitRefName()) + if err != nil { + return fmt.Errorf("GetRefCommitID failed: %w", err) + } + _, err = pull_service.CreateCodeComment( ctx, doer, nil, @@ -134,7 +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, - "", + afterCommitID, attachmentIDs, ) if err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index 03b7ce531b..617d5d0806 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -263,14 +263,14 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer return nil } -func checkForInvalidation(ctx context.Context, requests issues_model.PullRequestList, repoID int64, doer *user_model.User, branch, newCommitID string) error { +func checkForInvalidation(ctx context.Context, requests issues_model.PullRequestList, repoID int64, doer *user_model.User, newCommitID string) error { repo, err := repo_model.GetRepositoryByID(ctx, repoID) if err != nil { return fmt.Errorf("GetRepositoryByIDCtx: %w", err) } go func() { // FIXME: graceful: We need to tell the manager we're doing something... - err := InvalidateCodeComments(ctx, requests, doer, repo, branch, newCommitID) + err := InvalidateCodeComments(ctx, requests, doer, repo, newCommitID) if err != nil { log.Error("PullRequestList.InvalidateCodeComments: %v", err) } @@ -335,7 +335,7 @@ func TestPullRequest(ctx context.Context, doer *user_model.User, repoID, olderTh if err = requests.LoadAttributes(ctx); err != nil { log.Error("PullRequestList.LoadAttributes: %v", err) } - if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch, newCommitID); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, newCommitID); invalidationErr != nil { log.Error("checkForInvalidation: %v", invalidationErr) } if err == nil { diff --git a/services/pull/review.go b/services/pull/review.go index 5500ce4582..9fc9862823 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -42,18 +42,7 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error { // checkInvalidation checks if the line of code comment got changed by another commit. // If the line got changed the comment is going to be invalidated. -func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, branch, newCommitID string) error { - if c.Line < 0 { - // Comment is on a removed line of code -- the `git blame --reverse` codepath doesn't work for this. Retain the - // originalbehaviour until a revision is done specific to removed lines: - gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo) - if err != nil { - return fmt.Errorf("failed to open repo: %w", err) - } - defer closer.Close() - return obsoleteCheckInvalidation(ctx, c, gitRepo, branch) - } - +func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, newCommitID string) error { reverseBlame, err := c.ResolveCurrentLine(ctx, repo, newCommitID) if err != nil { log.Warn("ResolveCurrentLine failed: %s", err.Error()) @@ -64,25 +53,8 @@ func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_ return nil } -func obsoleteCheckInvalidation(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()) - if err != nil && (errors.Is(err, git.ErrBlameFileDoesNotExist) || errors.Is(err, git.ErrBlameFileNotEnoughLines)) { - c.Invalidated = true - return issues_model.UpdateCommentInvalidate(ctx, c) - } - if err != nil { - return err - } - if c.CommitSHA != "" && c.CommitSHA != commit.ID.String() { - c.Invalidated = true - return issues_model.UpdateCommentInvalidate(ctx, c) - } - return nil -} - // InvalidateCodeComments will lookup the prs for code comments which got invalidated by change -func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, branch, newCommitID string) error { +func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, newCommitID string) error { if len(prs) == 0 { return nil } @@ -98,7 +70,7 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis return fmt.Errorf("find code comments: %v", err) } for _, comment := range codeComments { - if err := checkInvalidation(ctx, comment, repo, branch, newCommitID); err != nil { + if err := checkInvalidation(ctx, comment, repo, newCommitID); err != nil { return err } } @@ -261,6 +233,31 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, } else { blamedCommitID = commitID } + } else { + // 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) + if err != nil { + return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", pr.MergeBase, 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 + // the diff, not a red removed line). In order to record the right information for where to place this + // commit, we'll convert this into a right-hand comment -- using the present line number that the reverse + // blame gave us: + commit, lineres, err := gitRepo.LineBlame(afterCommitID, treePath, blame.LineNumber) + if err == nil { + 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 = blame.CommitID + // retain negative line numbering to identify we're commenting on the "previous" side of the diff + blamedLine = -1 * int64(blame.LineNumber) + } } // Only fetch diff if comment is review comment diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 5ff8bd4adf..0ec229f554 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -112,8 +112,10 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) { DecodeJSON(t, resp, &reviewComment) assert.Equal(t, review.ID, reviewComment.ReviewID) assert.Equal(t, newCommentBody, reviewComment.Body) - assert.EqualValues(t, reviewLine, reviewComment.OldLineNum) - assert.EqualValues(t, 0, reviewComment.LineNum) + // we sent OldLineNum: 1, but that line of code isn't modified in this PR, triggering the PR's logic + // which standardizes comments on non-modified lines of code to be on the right-hand-side of the diff: + assert.EqualValues(t, 0, reviewComment.OldLineNum) + assert.EqualValues(t, reviewLine, reviewComment.LineNum) assert.Equal(t, path, reviewComment.Path) } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 262733f72b..b91fdab6d4 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -1471,6 +1471,265 @@ func TestPullRequestCommentPlacement(t *testing.T) { tester.assertFilesChangedDiff(diff10) tester.assertCommitDiff(commit2, diff10) }) + + t.Run("comment on removed line", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Remove line 50, place a comment on the removed line. + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "", 1) + commit := tester.changeFile("file1.md", content) + tester.createPR() + + comment := tester.commentOnPreviousFromFilesChanged("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`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) // tracked back to the previous commit where it was line num 50 + + diff50 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff50) + tester.assertCommitDiff(commit, diff50) + }) + + t.Run("comment on removed line moves due to a following commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Remove line 50, place a comment on the removed line. + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + comment := tester.commentOnPreviousFromFilesChanged("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`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) // tracked back to the previous commit where it was line num 50 + + // Add a second commit to the PR which removes "Line 1" - "Line 10". + 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) + commit2 := tester.changeFile("file1.md", content) + + diff2 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 9"}, + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowHasCode, code: "Line 11"}, + } + tester.assertFilesChangedDiff(diff2, "checking commit2 contents in full PR diff") + tester.assertCommitDiff(commit2, diff2, "checking commit2 contents in single-commit diff") + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + + // This comment can still be located in the diff, so it should not be marked as Invalidated/Outdated -- + // which is kinda guaranteed by it being loaded in the diff, but for test sanity assert specifically. + commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) + assert.False(t, commentReloaded.Invalidated) + }) + + t.Run("comment on previous side line unchanged by PR appears", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Change line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + 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) + tester.createPR() + + // While viewing the PR, the reviewer made a comment on the previous side on a line of code that wasn't + // actually changed in the PR: + comment := tester.commentOnPreviousFromFilesChanged("file1.md", 49) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -47,7 +37,7 @@ Line 46 + Line 47 + Line 48 + Line 49`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) // will be moved from previous(LHS)->proposed(RHS) because it wasn't a comment on a change in the PR + assert.EqualValues(t, 49, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) + + diff := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff, "checking commit1 contents in single-commit diff") + }) + + t.Run("comment on first line of file removed", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Remove the first line of the file which will then be commented on, as an edge-case + content := tester.fileContent + content = strings.Replace(content, "Line 1\n", "", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + comment := tester.commentOnPreviousFromSpecificCommit(commit1, "file1.md", 1) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -1,4 +1,3 @@ +-Line 1`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -1, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) + + diff := []diffTableRow{ + {rowType: RowDelCode, code: "Line 1"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 2"}, + } + tester.assertFilesChangedDiff(diff, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff, "checking commit1 contents in single-commit diff") + }) + + t.Run("comment on removed line moves due to a following commit, following commit is rewritten and force-push'd", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + // Place a comment on "Line 50" (removed side) + comment := tester.commentOnPreviousFromFilesChanged("file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -47,7 +47,7 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) + assert.False(t, comment.Invalidated) + + // Add a second commit to the PR which removes "Line 1" - "Line 10". + 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) + + // Now amend commit2v1 with an additional change, causing a force push of the branch + tester.withBranchCheckout(func(repoPath string) { + content = strings.Replace(content, "Line 11\n", "", 1) // Remove Line 11 as well + require.NoError(t, os.WriteFile(path.Join(repoPath, "file1.md"), []byte(content), 0o644)) + require.NoError(t, git.NewCommand(t.Context(), "commit", "-a", "--amend", "--no-edit").Run(&git.RunOpts{Dir: repoPath})) + require.NoError(t, git.NewCommand(t.Context(), "push", "--force").Run(&git.RunOpts{Dir: repoPath})) + }) + + diff2 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowDelCode, code: "Line 11"}, + {rowType: RowHasCode, code: "Line 12"}, + } + tester.assertFilesChangedDiff(diff2, "checking commit2 (force push) contents in full PR diff") + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + + // This comment can still be located in the diff, so it should not be marked as Invalidated/Outdated -- + // which is kinda guaranteed by it being loaded in the diff, but for test sanity assert specifically. + commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) + assert.False(t, commentReloaded.Invalidated) + }) + + t.Run("comment on removed line invalidated due to force push", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "", 1) + tester.changeFile("file1.md", content) + tester.createPR() + + // Place a comment on "Line 50" (removed side) + comment := tester.commentOnPreviousFromFilesChanged("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`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) + assert.False(t, comment.Invalidated) + + // Now amend commit1 with an additional change that undoes the earlier change, changes something else instead + tester.withBranchCheckout(func(repoPath string) { + content = strings.Replace(content, "Line 49\n", "Line 49\nLine 50\n", 1) + content = strings.Replace(content, "Line 52\n", "", 1) + require.NoError(t, os.WriteFile(path.Join(repoPath, "file1.md"), []byte(content), 0o644)) + require.NoError(t, git.NewCommand(t.Context(), "commit", "-a", "--amend", "--no-edit").Run(&git.RunOpts{Dir: repoPath})) + require.NoError(t, git.NewCommand(t.Context(), "push", "--force").Run(&git.RunOpts{Dir: repoPath})) + }) + + diff := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowHasCode, code: "Line 50"}, + {rowType: RowHasCode, code: "Line 51"}, + {rowType: RowDelCode, code: "Line 52"}, + {rowType: RowHasCode, code: "Line 53"}, + } + tester.assertFilesChangedDiff(diff, "checking commit2 (force push) contents in full PR diff") + + // The comment on "Line 50" can't be valid anymore since that's not in the diff: + assert.EventuallyWithT(t, func(t *assert.CollectT) { + commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID}) + assert.True(t, commentReloaded.Invalidated) + }, 1*time.Second, 50*time.Millisecond) + }) }) } @@ -1592,17 +1851,32 @@ func (tester *PullRequestCommentPlacementTester) commentFromFilesChanged(filenam // omit after_commit_id -- new_comment form defaults to fetching the PR head fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) - return tester.commentFromNewCommentForm(resp, filename, line) + return tester.commentFromNewCommentForm(resp, filename, line, "proposed") +} + +func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromFilesChanged(filename string, line int) *issues_model.Comment { + req := NewRequest(tester.t, "GET", + // omit after_commit_id -- new_comment form defaults to fetching the PR head + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + return tester.commentFromNewCommentForm(resp, filename, line, "previous") } func (tester *PullRequestCommentPlacementTester) commentFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { 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)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) - return tester.commentFromNewCommentForm(resp, filename, line) + return tester.commentFromNewCommentForm(resp, filename, line, "proposed") } -func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp *httptest.ResponseRecorder, filename string, line int) *issues_model.Comment { +func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + 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)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + return tester.commentFromNewCommentForm(resp, filename, line, "previous") +} + +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) req := NewRequestWithValues(tester.t, "POST", @@ -1610,7 +1884,7 @@ func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp map[string]string{ "origin": doc.GetInputValueByName("origin"), "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), - "side": "proposed", // "proposed" (RHS) or "previous" (LHS) + "side": side, // "proposed" (RHS) or "previous" (LHS) "line": strconv.Itoa(line), "path": filename, "diff_start_cid": doc.GetInputValueByName("diff_start_cid"),