diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index 2fa49e443a..411777cb17 100644 --- a/models/unittest/unit_tests.go +++ b/models/unittest/unit_tests.go @@ -67,7 +67,7 @@ func BeanExists(t testing.TB, bean any, conditions ...any) bool { } // AssertExistsAndLoadBean assert that a bean exists and load it from the test database -func AssertExistsAndLoadBean[T any](t testing.TB, bean T, conditions ...any) T { +func AssertExistsAndLoadBean[T any](t require.TestingT, bean T, conditions ...any) T { exists, err := LoadBeanIfExists(bean, conditions...) require.NoError(t, err) assert.True(t, exists, diff --git a/services/pull/pull.go b/services/pull/pull.go index 9cb957d021..03b7ce531b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -263,22 +263,17 @@ 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 string) error { +func checkForInvalidation(ctx context.Context, requests issues_model.PullRequestList, repoID int64, doer *user_model.User, branch, newCommitID string) error { repo, err := repo_model.GetRepositoryByID(ctx, repoID) if err != nil { return fmt.Errorf("GetRepositoryByIDCtx: %w", err) } - gitRepo, err := gitrepo.OpenRepository(ctx, repo) - if err != nil { - return fmt.Errorf("gitrepo.OpenRepository: %w", err) - } go func() { // FIXME: graceful: We need to tell the manager we're doing something... - err := InvalidateCodeComments(ctx, requests, doer, gitRepo, branch) + err := InvalidateCodeComments(ctx, requests, doer, repo, branch, newCommitID) if err != nil { log.Error("PullRequestList.InvalidateCodeComments: %v", err) } - gitRepo.Close() }() return nil } @@ -340,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); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch, newCommitID); invalidationErr != nil { log.Error("checkForInvalidation: %v", invalidationErr) } if err == nil { diff --git a/services/pull/review.go b/services/pull/review.go index 760235325d..b8d4a30be8 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -42,7 +42,29 @@ 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 *git.Repository, branch string) error { +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) + } + + reverseBlame, err := c.ResolveCurrentLine(ctx, repo, newCommitID) + if err != nil { + log.Warn("ResolveCurrentLine failed: %s", err.Error()) + } else if reverseBlame.CommitID != newCommitID { + c.Invalidated = true + return issues_model.UpdateCommentInvalidate(ctx, c) + } + 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)) { @@ -60,7 +82,7 @@ func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *git.R } // 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 *git.Repository, branch string) error { +func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, branch, newCommitID string) error { if len(prs) == 0 { return nil } @@ -76,7 +98,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); err != nil { + if err := checkInvalidation(ctx, comment, repo, branch, newCommitID); err != nil { return err } } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 1a60c1ea11..3c0ea62a9c 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -1242,6 +1242,7 @@ func TestPullRequestCommentPlacement(t *testing.T) { assert.Equal(t, "proposed", comment.DiffSide()) assert.EqualValues(t, 50, comment.Line) assert.Equal(t, commit1, 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) @@ -1271,6 +1272,11 @@ func TestPullRequestCommentPlacement(t *testing.T) { } 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 line commit is rewritten and force-push'd", func(t *testing.T) { @@ -1296,6 +1302,7 @@ func TestPullRequestCommentPlacement(t *testing.T) { assert.Equal(t, "proposed", comment.DiffSide()) assert.EqualValues(t, 50, comment.Line) assert.Equal(t, commit1, 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) @@ -1350,6 +1357,17 @@ func TestPullRequestCommentPlacement(t *testing.T) { {rowType: RowHasCode, code: "Line 51"}, } tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + + // After the force push, the comment we originally left should be marked as invalidated since it can no + // longer be resolved to a code location in the PR head. The above tests validate that it no longer appears + // in the diff, but this will also happen because of the diff-side check for the correct location -- so + // let's check that it's invalidated as well, indicating that it will be shown in the UI as "Outdated". This + // usually passes on the first check but is wrapped in Eventually because the async goroutine used in the + // pull request testing when the branch is pushed may not be immediately complete. + 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) }) t.Run("comment lands on blame with original line number varying from current", func(t *testing.T) { @@ -1551,7 +1569,7 @@ func (tester *PullRequestCommentPlacementTester) withBranchCheckout(action func( func (tester *PullRequestCommentPlacementTester) assertFilesChangedDiff(rowAssertions []diffTableRow, note ...string) { req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/files?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) + fmt.Sprintf("/%s/%s/pulls/%d/files", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) doc := NewHTMLParser(tester.t, resp.Body) var testNote string @@ -1565,7 +1583,7 @@ func (tester *PullRequestCommentPlacementTester) assertFilesChangedDiff(rowAsser func (tester *PullRequestCommentPlacementTester) assertCommitDiff(commitSHA string, rowAssertions []diffTableRow, note ...string) { req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/commits/%s?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitSHA)) + fmt.Sprintf("/%s/%s/pulls/%d/commits/%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitSHA)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) doc := NewHTMLParser(tester.t, resp.Body) var testNote string