diff --git a/services/issue/comments.go b/services/issue/comments.go index 9442701029..3ce2e2a5e1 100644 --- a/services/issue/comments.go +++ b/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 } diff --git a/services/pull/merge.go b/services/pull/merge.go index f1ad8fa17d..1e8e9d444b 100644 --- a/services/pull/merge.go +++ b/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, diff --git a/services/pull/pull.go b/services/pull/pull.go index 619347055b..a519a1032f 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -374,10 +374,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. repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) if err != nil { log.Error("GetRepositoryByID: %v", err) @@ -402,11 +400,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 { diff --git a/services/pull/update.go b/services/pull/update.go index cce3937451..436e3b52a6 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -63,6 +63,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,