From 276c7a9987099eea3f574137d8b4505cb493e6c0 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 7 Jul 2023 08:00:41 +0200 Subject: [PATCH 1/8] wip yES i Know theres is a cycle --- services/automerge/automerge.go | 13 +++++++++++-- services/repository/commitstatus/commitstatus.go | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index bd427bef9f731..135916247a7ca 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -95,8 +95,8 @@ func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull * }) } -// MergeScheduledPullRequest merges a previously scheduled pull request when all checks succeeded -func MergeScheduledPullRequest(ctx context.Context, sha string, repo *repo_model.Repository) error { +// MergeScheduledPullRequestsBySha merges a previously scheduled pull request(s) when all checks succeeded +func MergeScheduledPullRequestsBySha(ctx context.Context, sha string, repo *repo_model.Repository) error { pulls, err := getPullRequestsByHeadSHA(ctx, sha, repo, func(pr *issues_model.PullRequest) bool { return !pr.HasMerged && pr.CanAutoMerge() }) @@ -111,6 +111,15 @@ func MergeScheduledPullRequest(ctx context.Context, sha string, repo *repo_model return nil } +// MergeScheduledPullRequest merges a previously scheduled pull request when all checks succeeded +func MergeScheduledPullRequest(pull *issues_model.PullRequest) { + if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { + return + } + + addToQueue(pull, pull.HeadCommitID) +} + func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.Repository, filter func(*issues_model.PullRequest) bool) (map[int64]*issues_model.PullRequest, error) { gitRepo, err := gitrepo.OpenRepository(ctx, repo) if err != nil { diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index 8a62a603d4609..503b73e9846c9 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -117,7 +117,7 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato } if status.State.IsSuccess() { - if err := automerge.MergeScheduledPullRequest(ctx, sha, repo); err != nil { + if err := automerge.MergeScheduledPullRequestsBySha(ctx, sha, repo); err != nil { return fmt.Errorf("MergeScheduledPullRequest[repo_id: %d, user_id: %d, sha: %s]: %w", repo.ID, creator.ID, sha, err) } } From 798a194f609255b995114ecf98851f4b39dd73c8 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 2 Mar 2024 08:35:37 +0100 Subject: [PATCH 2/8] fix & finish --- routers/api/v1/repo/pull_review.go | 23 +++++++++++++++++++++-- routers/web/repo/pull_review.go | 14 ++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index b527e90f10b8c..55904eef9a3ad 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -16,6 +16,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" + "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" @@ -376,6 +377,11 @@ func CreatePullReview(ctx *context.APIContext) { return } + // as a missing / blocking reviews could have blocked a pending automerge let's recheck + if reviewType == issues_model.ReviewTypeApprove { + automerge.MergeScheduledPullRequest(pr) + } + // convert response apiReview, err := convert.ToPullReview(ctx, review, ctx.Doer) if err != nil { @@ -464,6 +470,11 @@ func SubmitPullReview(ctx *context.APIContext) { return } + // as a missing / blocking reviews could have blocked a pending automerge let's recheck + if review.Type == issues_model.ReviewTypeApprove { + automerge.MergeScheduledPullRequest(pr) + } + // convert response apiReview, err := convert.ToPullReview(ctx, review, ctx.Doer) if err != nil { @@ -879,7 +890,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors ctx.Error(http.StatusForbidden, "", "Must be repo admin") return } - review, _, isWrong := prepareSingleReview(ctx) + review, pr, isWrong := prepareSingleReview(ctx) if isWrong { return } @@ -889,7 +900,12 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors return } - _, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) + if pr.Issue.IsClosed { + ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed") + return + } + + comm, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) if err != nil { if pull_service.IsErrDismissRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -899,6 +915,9 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors return } + // as reviews could have blocked a pending automerge let's recheck + automerge.MergeScheduledPullRequest(comm.Issue.PullRequest) + if review, err = issues_model.GetReviewByID(ctx, review.ID); err != nil { ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) return diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index a65d4866d0a48..30ddb0dd03df6 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/forms" @@ -269,6 +270,16 @@ func SubmitReview(ctx *context.Context) { } return } + + // as a missing / blocking reviews could have blocked a pending automerge let's recheck + if reviewType == issues_model.ReviewTypeApprove { + if err := issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("GetPullRequest", err) + return + } + automerge.MergeScheduledPullRequest(issue.PullRequest) + } + ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } @@ -285,6 +296,9 @@ func DismissReview(ctx *context.Context) { return } + // as reviews could have blocked a pending automerge let's recheck + automerge.MergeScheduledPullRequest(comm.Issue.PullRequest) + ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag())) } From 7ba4d009c23e258695229d12ca86c99a69f664e7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Apr 2024 17:44:08 +0800 Subject: [PATCH 3/8] Use notify to trigger auto merge check --- models/issues/review.go | 6 ++-- routers/api/v1/repo/pull_review.go | 16 +---------- routers/web/repo/pull_review.go | 13 --------- services/automerge/automerge.go | 3 ++ services/automerge/notify.go | 44 ++++++++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 31 deletions(-) create mode 100644 services/automerge/notify.go diff --git a/models/issues/review.go b/models/issues/review.go index 3c6934b060afc..ca6fd6035b130 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -155,14 +155,14 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) { if r.CodeComments != nil { return err } - if err = r.loadIssue(ctx); err != nil { + if err = r.LoadIssue(ctx); err != nil { return err } r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue, nil, r, false) return err } -func (r *Review) loadIssue(ctx context.Context) (err error) { +func (r *Review) LoadIssue(ctx context.Context) (err error) { if r.Issue != nil { return err } @@ -199,7 +199,7 @@ func (r *Review) LoadReviewerTeam(ctx context.Context) (err error) { // LoadAttributes loads all attributes except CodeComments func (r *Review) LoadAttributes(ctx context.Context) (err error) { - if err = r.loadIssue(ctx); err != nil { + if err = r.LoadIssue(ctx); err != nil { return err } if err = r.LoadCodeComments(ctx); err != nil { diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 55904eef9a3ad..a46b943ec0b86 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -16,7 +16,6 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" - "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" @@ -377,11 +376,6 @@ func CreatePullReview(ctx *context.APIContext) { return } - // as a missing / blocking reviews could have blocked a pending automerge let's recheck - if reviewType == issues_model.ReviewTypeApprove { - automerge.MergeScheduledPullRequest(pr) - } - // convert response apiReview, err := convert.ToPullReview(ctx, review, ctx.Doer) if err != nil { @@ -470,11 +464,6 @@ func SubmitPullReview(ctx *context.APIContext) { return } - // as a missing / blocking reviews could have blocked a pending automerge let's recheck - if review.Type == issues_model.ReviewTypeApprove { - automerge.MergeScheduledPullRequest(pr) - } - // convert response apiReview, err := convert.ToPullReview(ctx, review, ctx.Doer) if err != nil { @@ -905,7 +894,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors return } - comm, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) + _, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) if err != nil { if pull_service.IsErrDismissRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -915,9 +904,6 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors return } - // as reviews could have blocked a pending automerge let's recheck - automerge.MergeScheduledPullRequest(comm.Issue.PullRequest) - if review, err = issues_model.GetReviewByID(ctx, review.ID); err != nil { ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) return diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 30ddb0dd03df6..0feabd86f6e76 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -16,7 +16,6 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web" - "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/forms" @@ -271,15 +270,6 @@ func SubmitReview(ctx *context.Context) { return } - // as a missing / blocking reviews could have blocked a pending automerge let's recheck - if reviewType == issues_model.ReviewTypeApprove { - if err := issue.LoadPullRequest(ctx); err != nil { - ctx.ServerError("GetPullRequest", err) - return - } - automerge.MergeScheduledPullRequest(issue.PullRequest) - } - ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } @@ -296,9 +286,6 @@ func DismissReview(ctx *context.Context) { return } - // as reviews could have blocked a pending automerge let's recheck - automerge.MergeScheduledPullRequest(comm.Issue.PullRequest) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag())) } diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 135916247a7ca..ab82c10b386a2 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/queue" + notify_service "code.gitea.io/gitea/services/notify" pull_service "code.gitea.io/gitea/services/pull" ) @@ -30,6 +31,8 @@ var prAutoMergeQueue *queue.WorkerPoolQueue[string] // Init runs the task queue to that handles auto merges func Init() error { + notify_service.RegisterNotifier(NewNotifier()) + prAutoMergeQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_auto_merge", handler) if prAutoMergeQueue == nil { return fmt.Errorf("unable to create pr_auto_merge queue") diff --git a/services/automerge/notify.go b/services/automerge/notify.go new file mode 100644 index 0000000000000..4d75773565b2a --- /dev/null +++ b/services/automerge/notify.go @@ -0,0 +1,44 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package automerge + +import ( + "context" + + issues_model "code.gitea.io/gitea/models/issues" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" + notify_service "code.gitea.io/gitea/services/notify" +) + +type automergeNotifier struct { + notify_service.NullNotifier +} + +var _ notify_service.Notifier = &automergeNotifier{} + +// NewNotifier create a new automergeNotifier notifier +func NewNotifier() notify_service.Notifier { + return &automergeNotifier{} +} + +func (n *automergeNotifier) PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) { + // as a missing / blocking reviews could have blocked a pending automerge let's recheck + if review.Type == issues_model.ReviewTypeApprove { + MergeScheduledPullRequest(pr) + } +} + +func (n *automergeNotifier) PullReviewDismiss(ctx context.Context, doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) { + if err := review.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + if err := review.Issue.LoadPullRequest(ctx); err != nil { + log.Error("LoadPullRequest: %v", err) + return + } + // as reviews could have blocked a pending automerge let's recheck + MergeScheduledPullRequest(review.Issue.PullRequest) +} From 5ff68886ca5f58f188b12e4baa7159018bf02374 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Apr 2024 17:46:15 +0800 Subject: [PATCH 4/8] Remove unnecessary changes --- routers/api/v1/repo/pull_review.go | 7 +------ routers/web/repo/pull_review.go | 1 - 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index a46b943ec0b86..b527e90f10b8c 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -879,7 +879,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors ctx.Error(http.StatusForbidden, "", "Must be repo admin") return } - review, pr, isWrong := prepareSingleReview(ctx) + review, _, isWrong := prepareSingleReview(ctx) if isWrong { return } @@ -889,11 +889,6 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors return } - if pr.Issue.IsClosed { - ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed") - return - } - _, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) if err != nil { if pull_service.IsErrDismissRequestOnClosedPR(err) { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 0feabd86f6e76..a65d4866d0a48 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -269,7 +269,6 @@ func SubmitReview(ctx *context.Context) { } return } - ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } From 9b16eda215d7c8a392ba92960fa69322487206bf Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 16:50:22 +0800 Subject: [PATCH 5/8] Fix bugs --- services/automerge/automerge.go | 90 ++++++++++++------- services/automerge/notify.go | 4 +- .../repository/commitstatus/commitstatus.go | 2 +- 3 files changed, 62 insertions(+), 34 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index ab82c10b386a2..4cea0fdb8f199 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -50,7 +50,7 @@ func handler(items ...string) []string { log.Error("could not parse data from pr_auto_merge queue (%v): %v", s, err) continue } - handlePull(id, sha) + handlePullRequestAutoMerge(id, sha) } return nil } @@ -98,8 +98,8 @@ func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull * }) } -// MergeScheduledPullRequestsBySha merges a previously scheduled pull request(s) when all checks succeeded -func MergeScheduledPullRequestsBySha(ctx context.Context, sha string, repo *repo_model.Repository) error { +// StartPullRequestAutoMergeCheckBySHA start an automerge check task for repository and SHA +func StartPullRequestAutoMergeCheckBySHA(ctx context.Context, sha string, repo *repo_model.Repository) error { pulls, err := getPullRequestsByHeadSHA(ctx, sha, repo, func(pr *issues_model.PullRequest) bool { return !pr.HasMerged && pr.CanAutoMerge() }) @@ -114,13 +114,31 @@ func MergeScheduledPullRequestsBySha(ctx context.Context, sha string, repo *repo return nil } -// MergeScheduledPullRequest merges a previously scheduled pull request when all checks succeeded -func MergeScheduledPullRequest(pull *issues_model.PullRequest) { +// StartPullRequestAutoMergeCheck start an automerge check task for a pull request +func StartPullRequestAutoMergeCheck(ctx context.Context, pull *issues_model.PullRequest) { if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { return } - addToQueue(pull, pull.HeadCommitID) + if err := pull.LoadBaseRepo(ctx); err != nil { + log.Error("LoadBaseRepo: %v", err) + return + } + + gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) + if err != nil { + log.Error("OpenRepository: %v", err) + return + } + defer gitRepo.Close() + + commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return + } + + addToQueue(pull, commitID) } func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.Repository, filter func(*issues_model.PullRequest) bool) (map[int64]*issues_model.PullRequest, error) { @@ -173,7 +191,8 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. return pulls, nil } -func handlePull(pullID int64, sha string) { +// handlePullRequestAutoMerge merge the pull request if all checks are successful +func handlePullRequestAutoMerge(pullID int64, sha string) { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("Handle AutoMerge of PR[%d] with sha[%s]", pullID, sha)) defer finished() @@ -194,24 +213,50 @@ func handlePull(pullID int64, sha string) { return } + if err = pr.LoadBaseRepo(ctx); err != nil { + log.Error("%-v LoadBaseRepo: %v", pr, err) + return + } + + // check the sha is the same as pull request head commit id + baseGitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) + if err != nil { + log.Error("OpenRepository: %v", err) + return + } + defer baseGitRepo.Close() + + headCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return + } + if headCommitID != sha { + log.Warn("Head commit id of auto merge %-v does not match sha [%s]", pr, sha) + return + } + // Get all checks for this pr // We get the latest sha commit hash again to handle the case where the check of a previous push // did not succeed or was not finished yet. - if err = pr.LoadHeadRepo(ctx); err != nil { log.Error("%-v LoadHeadRepo: %v", pr, err) return } - headGitRepo, err := gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) - return + var headGitRepo *git.Repository + if pr.BaseRepoID == pr.HeadRepoID { + headGitRepo = baseGitRepo + } else { + headGitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) + if err != nil { + log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) + return + } + defer headGitRepo.Close() } - defer headGitRepo.Close() headBranchExist := headGitRepo.IsBranchExist(pr.HeadBranch) - if pr.HeadRepo == nil || !headBranchExist { log.Warn("Head branch of auto merge %-v does not exist [HeadRepoID: %d, Branch: %s]", pr, pr.HeadRepoID, pr.HeadBranch) return @@ -250,23 +295,6 @@ func handlePull(pullID int64, sha string) { return } - var baseGitRepo *git.Repository - if pr.BaseRepoID == pr.HeadRepoID { - baseGitRepo = headGitRepo - } else { - if err = pr.LoadBaseRepo(ctx); err != nil { - log.Error("%-v LoadBaseRepo: %v", pr, err) - return - } - - baseGitRepo, err = gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - log.Error("OpenRepository %-v: %v", pr.BaseRepo, err) - return - } - defer baseGitRepo.Close() - } - if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) return diff --git a/services/automerge/notify.go b/services/automerge/notify.go index 4d75773565b2a..64ce62b01e986 100644 --- a/services/automerge/notify.go +++ b/services/automerge/notify.go @@ -26,7 +26,7 @@ func NewNotifier() notify_service.Notifier { func (n *automergeNotifier) PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) { // as a missing / blocking reviews could have blocked a pending automerge let's recheck if review.Type == issues_model.ReviewTypeApprove { - MergeScheduledPullRequest(pr) + StartPullRequestAutoMergeCheckBySHA(ctx, review.CommitID, pr.BaseRepo) } } @@ -40,5 +40,5 @@ func (n *automergeNotifier) PullReviewDismiss(ctx context.Context, doer *user_mo return } // as reviews could have blocked a pending automerge let's recheck - MergeScheduledPullRequest(review.Issue.PullRequest) + StartPullRequestAutoMergeCheck(ctx, review.Issue.PullRequest) } diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index 503b73e9846c9..fb6d5bd1a2f24 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -117,7 +117,7 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato } if status.State.IsSuccess() { - if err := automerge.MergeScheduledPullRequestsBySha(ctx, sha, repo); err != nil { + if err := automerge.StartPullRequestAutoMergeCheckBySHA(ctx, sha, repo); err != nil { return fmt.Errorf("MergeScheduledPullRequest[repo_id: %d, user_id: %d, sha: %s]: %w", repo.ID, creator.ID, sha, err) } } From acd2a9b9626eddef37aa0310059fdcf75230e137 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 17:11:46 +0800 Subject: [PATCH 6/8] Fix bug --- services/automerge/automerge.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 4cea0fdb8f199..231690b872c92 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -65,16 +65,6 @@ func addToQueue(pr *issues_model.PullRequest, sha string) { // ScheduleAutoMerge if schedule is false and no error, pull can be merged directly func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest, style repo_model.MergeStyle, message string) (scheduled bool, err error) { err = db.WithTx(ctx, func(ctx context.Context) error { - lastCommitStatus, err := pull_service.GetPullRequestCommitStatusState(ctx, pull) - if err != nil { - return err - } - - // we don't need to schedule - if lastCommitStatus.IsSuccess() { - return nil - } - if err := pull_model.ScheduleAutoMerge(ctx, doer, pull.ID, style, message); err != nil { return err } From ec2f717523b3ba40c2bb0635320a83747047c225 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 17:13:54 +0800 Subject: [PATCH 7/8] Fix lint --- services/automerge/notify.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/automerge/notify.go b/services/automerge/notify.go index 64ce62b01e986..bc65a43f2444a 100644 --- a/services/automerge/notify.go +++ b/services/automerge/notify.go @@ -26,7 +26,9 @@ func NewNotifier() notify_service.Notifier { func (n *automergeNotifier) PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) { // as a missing / blocking reviews could have blocked a pending automerge let's recheck if review.Type == issues_model.ReviewTypeApprove { - StartPullRequestAutoMergeCheckBySHA(ctx, review.CommitID, pr.BaseRepo) + if err := StartPullRequestAutoMergeCheckBySHA(ctx, review.CommitID, pr.BaseRepo); err != nil { + log.Error("StartPullRequestAutoMergeCheckBySHA: %v", err) + } } } From db4113e3de467a18360a22f0fd9f06607132b45f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 16:58:58 +0800 Subject: [PATCH 8/8] Add transaction for auto merge --- services/automerge/automerge.go | 1 + services/pull/merge.go | 90 +++++++++++++++++++++------------ services/pull/merge_prepare.go | 13 +++-- 3 files changed, 68 insertions(+), 36 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 231690b872c92..adc782dd67510 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -287,6 +287,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) + // FIXME: if merge failed, we should display some error message to the pull request page. return } } diff --git a/services/pull/merge.go b/services/pull/merge.go index 00f23e1e3ae23..8954d1cc74448 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -162,12 +162,6 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U pullWorkingPool.CheckIn(fmt.Sprint(pr.ID)) defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID)) - // Removing an auto merge pull and ignore if not exist - // FIXME: is this the correct point to do this? Shouldn't this be after IsMergeStyleAllowed? - if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { - return err - } - prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests) if err != nil { log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err) @@ -184,17 +178,31 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") }() - pr.MergedCommitID, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message) + mergeCtx, cancel, err := doMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message) if err != nil { return err } + defer cancel() - pr.MergedUnix = timeutil.TimeStampNow() - pr.Merger = doer - pr.MergerID = doer.ID + if err := db.WithTx(ctx, func(ctx context.Context) error { + // Removing an auto merge pull and ignore if not exist + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + return err + } + + pr.MergedCommitID = mergeCtx.mergeCommitID + pr.MergedUnix = timeutil.TimeStampNow() + pr.Merger = doer + pr.MergerID = doer.ID + if _, err := pr.SetMerged(ctx); err != nil { + log.Error("SetMerged %-v: %v", pr, err) + return err + } - if _, err := pr.SetMerged(ctx); err != nil { - log.Error("SetMerged %-v: %v", pr, err) + _, err := doPush(ctx, mergeCtx, pr, doer) + return err + }); err != nil { + return err } if err := pr.LoadIssue(ctx); err != nil { @@ -244,62 +252,82 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return nil } -// doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository -func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) { +func doMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (*mergeContext, context.CancelFunc, error) { // Clone base repo. mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID) if err != nil { - return "", err + return nil, nil, err } - defer cancel() - // Merge commits. switch mergeStyle { case repo_model.MergeStyleMerge: if err := doMergeStyleMerge(mergeCtx, message); err != nil { - return "", err + cancel() + return nil, nil, err } case repo_model.MergeStyleRebase, repo_model.MergeStyleRebaseMerge: if err := doMergeStyleRebase(mergeCtx, mergeStyle, message); err != nil { - return "", err + cancel() + return nil, nil, err } case repo_model.MergeStyleSquash: if err := doMergeStyleSquash(mergeCtx, message); err != nil { - return "", err + cancel() + return nil, nil, err } case repo_model.MergeStyleFastForwardOnly: if err := doMergeStyleFastForwardOnly(mergeCtx); err != nil { - return "", err + cancel() + return nil, nil, err } default: - return "", models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} + cancel() + return nil, nil, models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} } // OK we should cache our current head and origin/headbranch - mergeHeadSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD") + mergeCtx.mergeHeadSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD") if err != nil { - return "", fmt.Errorf("Failed to get full commit id for HEAD: %w", err) + cancel() + return nil, nil, fmt.Errorf("Failed to get full commit id for HEAD: %w", err) } - mergeBaseSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch) + mergeCtx.mergeBaseSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch) if err != nil { - return "", fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) + cancel() + return nil, nil, fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) } - mergeCommitID, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch) + mergeCtx.mergeCommitID, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch) if err != nil { - return "", fmt.Errorf("Failed to get full commit id for the new merge: %w", err) + cancel() + return nil, nil, fmt.Errorf("Failed to get full commit id for the new merge: %w", err) } + return mergeCtx, cancel, nil +} + +// doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository +func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) { + mergeCtx, cancel, err := doMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message) + if err != nil { + return "", err + } + defer cancel() + + return doPush(ctx, mergeCtx, pr, doer) +} + +func doPush(ctx context.Context, mergeCtx *mergeContext, pr *issues_model.PullRequest, doer *user_model.User) (string, error) { // Now it's questionable about where this should go - either after or before the push // I think in the interests of data safety - failures to push to the lfs should prevent // the merge as you can always remerge. if setting.LFS.StartServer { - if err := LFSPush(ctx, mergeCtx.tmpBasePath, mergeHeadSHA, mergeBaseSHA, pr); err != nil { + if err := LFSPush(ctx, mergeCtx.tmpBasePath, mergeCtx.mergeHeadSHA, mergeCtx.mergeBaseSHA, pr); err != nil { return "", err } } var headUser *user_model.User - err = pr.HeadRepo.LoadOwner(ctx) + err := pr.HeadRepo.LoadOwner(ctx) if err != nil { if !user_model.IsErrUserNotExist(err) { log.Error("Can't find user: %d for head repository in %-v: %v", pr.HeadRepo.OwnerID, pr, err) @@ -344,7 +372,7 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use mergeCtx.outbuf.Reset() mergeCtx.errbuf.Reset() - return mergeCommitID, nil + return mergeCtx.mergeCommitID, nil } func commitAndSignNoAuthor(ctx *mergeContext, message string) error { diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 88f6c037ebc90..883e274396e86 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -25,11 +25,14 @@ import ( type mergeContext struct { *prContext - doer *user_model.User - sig *git.Signature - committer *git.Signature - signKeyID string // empty for no-sign, non-empty to sign - env []string + doer *user_model.User + sig *git.Signature + committer *git.Signature + signKeyID string // empty for no-sign, non-empty to sign + env []string + mergeHeadSHA string + mergeBaseSHA string + mergeCommitID string } func (ctx *mergeContext) RunOpts() *git.RunOpts {