From 462f3af98a2b5e913f0a65f0f5d7520733be7302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Wed, 2 Jul 2025 14:32:20 +0200 Subject: [PATCH] Mark old reviews as stale on agit pr updates Fixes: https://github.com/go-gitea/gitea/issues/34134 --- services/agit/agit.go | 27 ++++++++ tests/integration/git_misc_test.go | 102 +++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/services/agit/agit.go b/services/agit/agit.go index 0fe28c5d66639..b27dfc8ecd342 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -9,6 +9,7 @@ import ( "os" "strings" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -199,11 +200,37 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } } + // Store old commit ID for review staleness checking + oldHeadCommitID := pr.HeadCommitID + pr.HeadCommitID = opts.NewCommitIDs[i] if err = pull_service.UpdateRef(ctx, pr); err != nil { return nil, fmt.Errorf("failed to update pull ref. Error: %w", err) } + // Mark existing reviews as stale when PR content changes (same as regular GitHub flow) + if oldHeadCommitID != opts.NewCommitIDs[i] { + if err := issues_model.MarkReviewsAsStale(ctx, pr.IssueID); err != nil { + log.Error("MarkReviewsAsStale: %v", err) + } + + // Dismiss all approval reviews if protected branch rule item enabled + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { + log.Error("GetFirstMatchProtectedBranchRule: %v", err) + } + if pb != nil && pb.DismissStaleApprovals { + if err := pull_service.DismissApprovalReviews(ctx, pusher, pr); err != nil { + log.Error("DismissApprovalReviews: %v", err) + } + } + + // Mark reviews for the new commit as not stale + if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitIDs[i]); err != nil { + log.Error("MarkReviewsAsNotStale: %v", err) + } + } + pull_service.StartPullRequestCheckImmediately(ctx, pr) err = pr.LoadIssue(ctx) if err != nil { diff --git a/tests/integration/git_misc_test.go b/tests/integration/git_misc_test.go index bf9e2c02ab38b..a5c53fd6e96f6 100644 --- a/tests/integration/git_misc_test.go +++ b/tests/integration/git_misc_test.go @@ -135,3 +135,105 @@ func TestAgitPullPush(t *testing.T) { assert.NoError(t, err) }) } + +func TestAgitReviewStaleness(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + baseAPITestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + + u.Path = baseAPITestContext.GitPath() + u.User = url.UserPassword("user2", userPassword) + + dstPath := t.TempDir() + doGitClone(dstPath, u)(t) + + gitRepo, err := git.OpenRepository(t.Context(), dstPath) + assert.NoError(t, err) + defer gitRepo.Close() + + doGitCreateBranch(dstPath, "test-agit-review") + + // Create initial commit + _, err = generateCommitWithNewData(testFileSizeSmall, dstPath, "user2@example.com", "User Two", "initial-") + assert.NoError(t, err) + + // create PR via agit + err = git.NewCommand("push", "origin", + "-o", "title=Test agit Review Staleness", "-o", "description=Testing review staleness", + "HEAD:refs/for/master/test-agit-review", + ).Run(git.DefaultContext, &git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ + BaseRepoID: 1, + Flow: issues_model.PullRequestFlowAGit, + HeadBranch: "user2/test-agit-review", + }) + assert.NoError(t, pr.LoadIssue(db.DefaultContext)) + + // Get initial commit ID for the review + initialCommitID := pr.HeadCommitID + t.Logf("Initial commit ID: %s", initialCommitID) + + // Create a review on the PR (as user1 reviewing user2's PR) + reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + review, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Type: issues_model.ReviewTypeApprove, + Reviewer: reviewer, + Issue: pr.Issue, + CommitID: initialCommitID, + Content: "LGTM! Looks good to merge.", + Official: false, + }) + assert.NoError(t, err) + assert.False(t, review.Stale, "New review should not be stale") + + // Verify review exists and is not stale + reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ + IssueID: pr.IssueID, + }) + assert.NoError(t, err) + assert.Len(t, reviews, 1) + assert.Equal(t, initialCommitID, reviews[0].CommitID) + assert.False(t, reviews[0].Stale, "Review should not be stale initially") + + // Create a new commit and update the agit PR + _, err = generateCommitWithNewData(testFileSizeSmall, dstPath, "user2@example.com", "User Two", "updated-") + assert.NoError(t, err) + + err = git.NewCommand("push", "origin", "HEAD:refs/for/master/test-agit-review").Run(git.DefaultContext, &git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + // Reload PR to get updated commit ID + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ + BaseRepoID: 1, + Flow: issues_model.PullRequestFlowAGit, + HeadBranch: "user2/test-agit-review", + }) + assert.NoError(t, pr.LoadIssue(db.DefaultContext)) + + // For AGit PRs, HeadCommitID must be loaded from git references + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + assert.NoError(t, err) + defer baseGitRepo.Close() + + updatedCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + assert.NoError(t, err) + t.Logf("Updated commit ID: %s", updatedCommitID) + + // Verify the PR was updated with new commit + assert.NotEqual(t, initialCommitID, updatedCommitID, "PR should have new commit ID after update") + + // Check that the review is now marked as stale + reviews, err = issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ + IssueID: pr.IssueID, + }) + assert.NoError(t, err) + assert.Len(t, reviews, 1) + + assert.True(t, reviews[0].Stale, "Review should be marked as stale after AGit PR update") + + // The review commit ID should remain the same (pointing to the original commit) + assert.Equal(t, initialCommitID, reviews[0].CommitID, "Review commit ID should remain unchanged and point to original commit") + }) +}