Creating push comments before invoke pull request checking (#35647)

This PR moved the creation of pushing comments before pull request
mergeable checking. So that when the pull request status changed, the
comments should have been created.

---------

Co-authored-by: wxiaoguang <[email protected]>

Author: lunny

services/issue/comments.go

@@ -15,6 +15,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
 	git_service "code.gitea.io/gitea/services/git"
 	notify_service "code.gitea.io/gitea/services/notify"
@@ -151,15 +152,15 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m
 }
 
 // LoadCommentPushCommits Load push commits
-func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err error) {
+func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error {
 	if c.Content == "" || c.Commits != nil || c.Type != issues_model.CommentTypePullRequestPush {
 		return nil
 	}
 
 	var data issues_model.PushActionContent
-	err = json.Unmarshal([]byte(c.Content), &data)
-	if err != nil {
-		return err
+	if err := json.Unmarshal([]byte(c.Content), &data); err != nil {
+		log.Debug("Unmarshal: %v", err) // no need to show 500 error to end user when the JSON is broken
+		return nil
 	}
 
 	c.IsForcePush = data.IsForcePush
@@ -168,9 +169,15 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err e
 		if len(data.CommitIDs) != 2 {
 			return nil
 		}
-		c.OldCommit = data.CommitIDs[0]
-		c.NewCommit = data.CommitIDs[1]
+		c.OldCommit, c.NewCommit = data.CommitIDs[0], data.CommitIDs[1]
 	} else {
+		if err := c.LoadIssue(ctx); err != nil {
+			return err
+		}
+		if err := c.Issue.LoadRepo(ctx); err != nil {
+			return err
+		}
+
 		gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo)
 		if err != nil {
 			return err
@@ -179,10 +186,11 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err e
 
 		c.Commits, err = git_service.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo)
 		if err != nil {
-			return err
+			log.Debug("ConvertFromGitCommit: %v", err) // no need to show 500 error to end user when the commit does not exist
+		} else {
+			c.CommitsNum = int64(len(c.Commits))
 		}
-		c.CommitsNum = int64(len(c.Commits))
 	}
 
-	return err
+	return nil
 }

services/pull/merge.go

@@ -248,6 +248,11 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
 	}
 	defer releaser()
 	defer func() {
+		// This is a duplicated call to AddTestPullRequestTask (it will also be called by the post-receive hook, via a push queue).
+		// This call will do some operations (push to base repo, sync commit divergence, add PR conflict check queue task, etc)
+		// immediately instead of waiting for the "push queue"'s task. The code is from https://github.com/go-gitea/gitea/pull/7082.
+		// But it's really questionable whether it's worth to do it ahead without waiting for the "push queue" task to run.
+		// TODO: DUPLICATE-PR-TASK: maybe can try to remove this in 1.26 to see if there is any issue.
 		go AddTestPullRequestTask(TestPullRequestOptions{
 			RepoID:      pr.BaseRepo.ID,
 			Doer:        doer,

services/pull/pull.go

@@ -379,10 +379,8 @@ type TestPullRequestOptions struct {
 func AddTestPullRequestTask(opts TestPullRequestOptions) {
 	log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch)
 	graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) {
-		// There is no sensible way to shut this down ":-("
-		// If you don't let it run all the way then you will lose data
-		// TODO: graceful: AddTestPullRequestTask needs to become a queue!
-
+		// this function does a lot of operations to various models, if the process gets killed in the middle,
+		// there is no way to recover at the moment. The best workaround is to let end user push again.
 		// GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR.
 		prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch)
 		if err != nil {
@@ -401,11 +399,15 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 				continue
 			}
 
-			StartPullRequestCheckImmediately(ctx, pr)
+			// create push comment before check pull request status,
+			// then when the status is mergeable, the comment is already in database, to make testing easy and stable
 			comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID, opts.IsForcePush)
 			if err == nil && comment != nil {
 				notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment)
 			}
+			// The caller can be in a goroutine or a "push queue", "conflict check" can be time-consuming,
+			// and the concurrency should be limited, so the conflict check will be done in another queue
+			StartPullRequestCheckImmediately(ctx, pr)
 		}
 
 		if opts.IsSync {

services/pull/update.go

@@ -59,6 +59,9 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
 	}
 
 	defer func() {
+		// The code is from https://github.com/go-gitea/gitea/pull/9784,
+		// it seems a simple copy-paste from https://github.com/go-gitea/gitea/pull/7082 without a real reason.
+		// TODO: DUPLICATE-PR-TASK: search and see another TODO comment for more details
 		go AddTestPullRequestTask(TestPullRequestOptions{
 			RepoID:      pr.BaseRepo.ID,
 			Doer:        doer,