Merge pull request '[v7.0/forgejo] fix: Run full PR checks on agit push' (#4950) from bp-v7.0/forgejo-2d05e92 into v7.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4950
Reviewed-by: Otto <otto@codeberg.org>
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>

services/agit/agit.go

@@ -210,6 +210,8 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 			return nil, fmt.Errorf("failed to update the reference of the pull request: %w", err)
 		}
 
+		// TODO: refactor to unify with `pull_service.AddTestPullRequestTask`
+
 		// Add the pull request to the merge conflicting checker queue.
 		pull_service.AddToTaskQueue(ctx, pr)
 
@@ -217,12 +219,19 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 			return nil, fmt.Errorf("failed to load the issue of the pull request: %w", err)
 		}
 
+		// Validate pull request.
+		pull_service.ValidatePullRequest(ctx, pr, oldCommitID, opts.NewCommitIDs[i], pusher)
+
+		// TODO: call `InvalidateCodeComments`
+
 		// Create and notify about the new commits.
 		comment, err := pull_service.CreatePushPullComment(ctx, pusher, pr, oldCommitID, opts.NewCommitIDs[i])
 		if err == nil && comment != nil {
 			notify_service.PullRequestPushCommits(ctx, pusher, pr, comment)
 		}
 		notify_service.PullRequestSynchronized(ctx, pusher, pr)
+
+		// this always seems to be false
 		isForcePush := comment != nil && comment.IsForcePush
 
 		results = append(results, private.HookProcReceiveRefResult{

services/pull/pull.go

@@ -356,43 +356,7 @@ func TestPullRequest(ctx context.Context, doer *user_model.User, repoID, olderTh
 		}
 		if err == nil {
 			for _, pr := range prs {
-				objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
+				ValidatePullRequest(ctx, pr, newCommitID, oldCommitID, doer)
-				if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() {
-					changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID)
-					if err != nil {
-						log.Error("checkIfPRContentChanged: %v", err)
-					}
-					if changed {
-						// Mark old reviews as stale if diff to mergebase has changed
-						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 := DismissApprovalReviews(ctx, doer, pr); err != nil {
-								log.Error("DismissApprovalReviews: %v", err)
-							}
-						}
-					}
-					if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil {
-						log.Error("MarkReviewsAsNotStale: %v", err)
-					}
-					divergence, err := GetDiverging(ctx, pr)
-					if err != nil {
-						log.Error("GetDiverging: %v", err)
-					} else {
-						err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind)
-						if err != nil {
-							log.Error("UpdateCommitDivergence: %v", err)
-						}
-					}
-				}
-
 				notify_service.PullRequestSynchronized(ctx, doer, pr)
 			}
 		}
@@ -422,6 +386,46 @@ func TestPullRequest(ctx context.Context, doer *user_model.User, repoID, olderTh
 	}
 }
 
+// Mark old reviews as stale if diff to mergebase has changed.
+// Dismiss all approval reviews if protected branch rule item enabled.
+// Update commit divergence.
+func ValidatePullRequest(ctx context.Context, pr *issues_model.PullRequest, newCommitID, oldCommitID string, doer *user_model.User) {
+	objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
+	if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() {
+		changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID)
+		if err != nil {
+			log.Error("checkIfPRContentChanged: %v", err)
+		}
+		if changed {
+			if err := issues_model.MarkReviewsAsStale(ctx, pr.IssueID); err != nil {
+				log.Error("MarkReviewsAsStale: %v", err)
+			}
+
+			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 := DismissApprovalReviews(ctx, doer, pr); err != nil {
+					log.Error("DismissApprovalReviews: %v", err)
+				}
+			}
+		}
+		if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil {
+			log.Error("MarkReviewsAsNotStale: %v", err)
+		}
+		divergence, err := GetDiverging(ctx, pr)
+		if err != nil {
+			log.Error("GetDiverging: %v", err)
+		} else {
+			err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind)
+			if err != nil {
+				log.Error("UpdateCommitDivergence: %v", err)
+			}
+		}
+	}
+}
+
 // checkIfPRContentChanged checks if diff to target branch has changed by push
 // A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged
 func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) {

tests/integration/git_test.go

@@ -825,6 +825,9 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headB
 			if !assert.NotEmpty(t, pr1) {
 				return
 			}
+			assert.Equal(t, 1, pr1.CommitsAhead)
+			assert.Equal(t, 0, pr1.CommitsBehind)
+
 			prMsg, err := doAPIGetPullRequest(*ctx, ctx.Username, ctx.Reponame, pr1.Index)(t)
 			require.NoError(t, err)
 
@@ -845,6 +848,8 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headB
 			if !assert.NotEmpty(t, pr2) {
 				return
 			}
+			assert.Equal(t, 1, pr2.CommitsAhead)
+			assert.Equal(t, 0, pr2.CommitsBehind)
 			prMsg, err = doAPIGetPullRequest(*ctx, ctx.Username, ctx.Reponame, pr2.Index)(t)
 			require.NoError(t, err)
 
@@ -903,6 +908,14 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headB
 			assert.False(t, prMsg.HasMerged)
 			assert.Equal(t, commit, prMsg.Head.Sha)
 
+			pr1 = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
+				HeadRepoID: repo.ID,
+				Flow:       issues_model.PullRequestFlowAGit,
+				Index:      pr1.Index,
+			})
+			assert.Equal(t, 2, pr1.CommitsAhead)
+			assert.Equal(t, 0, pr1.CommitsBehind)
+
 			_, _, err = git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments("HEAD:refs/for/master/test/" + headBranch).RunStdString(&git.RunOpts{Dir: dstPath})
 			require.NoError(t, err)