From f51af184fb38f1b6e4c4a4bbf4e111c2ba39491d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 7 Jul 2023 08:00:41 +0200 Subject: [PATCH 01/30] 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 444ae04d0c228..faef0512bae69 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -115,7 +115,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 d145c269cdcf2cf668595592fa3550b0fd0a5f3c Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 2 Mar 2024 08:35:37 +0100 Subject: [PATCH 02/30] 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 4b481790fb1a8..109778761b939 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -17,6 +17,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" @@ -381,6 +382,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 { @@ -473,6 +479,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 { @@ -888,7 +899,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 } @@ -898,7 +909,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) @@ -908,6 +924,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 62f6d71c5e5cf..0da67ec623080 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" @@ -271,6 +272,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())) } @@ -287,6 +298,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 0dc95312829b4c5089d0fc39d5554dc962f90a39 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Apr 2024 17:44:08 +0800 Subject: [PATCH 03/30] 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 109778761b939..8234185d8bcb6 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -17,7 +17,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" @@ -382,11 +381,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 { @@ -479,11 +473,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 { @@ -914,7 +903,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) @@ -924,9 +913,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 0da67ec623080..99daf5f7c9442 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" @@ -273,15 +272,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())) } @@ -298,9 +288,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 e4e24d2ce52c2c5118697acdb73cf751a4be72cc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Apr 2024 17:46:15 +0800 Subject: [PATCH 04/30] 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 8234185d8bcb6..4b481790fb1a8 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -888,7 +888,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 } @@ -898,11 +898,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 99daf5f7c9442..62f6d71c5e5cf 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -271,7 +271,6 @@ func SubmitReview(ctx *context.Context) { } return } - ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } From db6e2788718a32e6b339f8a763e0316ce46e7141 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 16:50:22 +0800 Subject: [PATCH 05/30] 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 faef0512bae69..af958aa437991 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -115,7 +115,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 a5d0708fdd7c2aa86a68b18fcb4372413b84be88 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 17:11:46 +0800 Subject: [PATCH 06/30] 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 b208bdec1c0b8ab74ed61063c402ab9897d11335 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Apr 2024 17:13:54 +0800 Subject: [PATCH 07/30] 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 c1eac83e59fe47d3e7149eef2de47513b7b46242 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 16:58:58 +0800 Subject: [PATCH 08/30] 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 { From 3ec275303d8b5112f5a4e49dfc1d746c35f3dc44 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 30 Apr 2024 16:27:14 +0800 Subject: [PATCH 09/30] Close git repo earlier --- services/automerge/automerge.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index adc782dd67510..6836f4ea34506 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -120,9 +120,9 @@ func StartPullRequestAutoMergeCheck(ctx context.Context, pull *issues_model.Pull log.Error("OpenRepository: %v", err) return } - defer gitRepo.Close() commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) + gitRepo.Close() if err != nil { log.Error("GetRefCommitID: %v", err) return @@ -136,9 +136,9 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. if err != nil { return nil, err } - defer gitRepo.Close() refs, err := gitRepo.GetRefsBySha(sha, "") + gitRepo.Close() if err != nil { return nil, err } From ed361d29154cab6e6fa2b70b6371cd33c9076733 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 16:02:31 +0800 Subject: [PATCH 10/30] Revert unnecessary change --- services/automerge/automerge.go | 2 + services/pull/merge.go | 73 +++++++++++++++++++++++++++------ services/pull/merge_prepare.go | 13 +++--- 3 files changed, 68 insertions(+), 20 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 6836f4ea34506..be3d0201b8bf3 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -288,6 +288,8 @@ 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. + // The resolution is add a new column on automerge table named `error_message` to store the error message and displayed + // on the pull request page. But this should not be finished in a bug fix PR which will be backport to release branch. return } } diff --git a/services/pull/merge.go b/services/pull/merge.go index a2d2b369c2d1f..c830f42b86e42 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -247,47 +247,96 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use switch mergeStyle { case repo_model.MergeStyleMerge: if err := doMergeStyleMerge(mergeCtx, message); err != nil { - cancel() return "", err } case repo_model.MergeStyleRebase, repo_model.MergeStyleRebaseMerge: if err := doMergeStyleRebase(mergeCtx, mergeStyle, message); err != nil { - cancel() return "", err } case repo_model.MergeStyleSquash: if err := doMergeStyleSquash(mergeCtx, message); err != nil { - cancel() return "", err } case repo_model.MergeStyleFastForwardOnly: if err := doMergeStyleFastForwardOnly(mergeCtx); err != nil { - cancel() return "", err } default: - cancel() return "", models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} } // OK we should cache our current head and origin/headbranch - mergeCtx.mergeHeadSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD") + mergeHeadSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD") if err != nil { - cancel() return "", fmt.Errorf("Failed to get full commit id for HEAD: %w", err) } - mergeCtx.mergeBaseSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch) + mergeBaseSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch) if err != nil { - cancel() return "", fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) } - mergeCtx.mergeCommitID, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch) + mergeCommitID, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch) if err != nil { - cancel() return "", fmt.Errorf("Failed to get full commit id for the new merge: %w", err) } - return mergeCtx.mergeCommitID, nil + // 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 { + return "", err + } + } + + var headUser *user_model.User + 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) + return "", err + } + log.Warn("Can't find user: %d for head repository in %-v - defaulting to doer: %s - %v", pr.HeadRepo.OwnerID, pr, doer.Name, err) + headUser = doer + } else { + headUser = pr.HeadRepo.Owner + } + + mergeCtx.env = repo_module.FullPushingEnvironment( + headUser, + doer, + pr.BaseRepo, + pr.BaseRepo.Name, + pr.ID, + ) + + mergeCtx.env = append(mergeCtx.env, repo_module.EnvPushTrigger+"="+string(pushTrigger)) + pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch) + + // Push back to upstream. + // This cause an api call to "/api/internal/hook/post-receive/...", + // If it's merge, all db transaction and operations should be there but not here to prevent deadlock. + if err := pushCmd.Run(mergeCtx.RunOpts()); err != nil { + if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") { + return "", &git.ErrPushOutOfDate{ + StdOut: mergeCtx.outbuf.String(), + StdErr: mergeCtx.errbuf.String(), + Err: err, + } + } else if strings.Contains(mergeCtx.errbuf.String(), "! [remote rejected]") { + err := &git.ErrPushRejected{ + StdOut: mergeCtx.outbuf.String(), + StdErr: mergeCtx.errbuf.String(), + Err: err, + } + err.GenerateMessage() + return "", err + } + return "", fmt.Errorf("git push: %s", mergeCtx.errbuf.String()) + } + mergeCtx.outbuf.Reset() + mergeCtx.errbuf.Reset() + + return mergeCommitID, nil } func commitAndSignNoAuthor(ctx *mergeContext, message string) error { diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 883e274396e86..88f6c037ebc90 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -25,14 +25,11 @@ 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 - mergeHeadSHA string - mergeBaseSHA string - mergeCommitID string + doer *user_model.User + sig *git.Signature + committer *git.Signature + signKeyID string // empty for no-sign, non-empty to sign + env []string } func (ctx *mergeContext) RunOpts() *git.RunOpts { From f5e417698543e8c7410ee6a24107aa78b429b161 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 16:11:00 +0800 Subject: [PATCH 11/30] Revert unnecessary change --- services/pull/merge.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/pull/merge.go b/services/pull/merge.go index c830f42b86e42..20be7c5b5a877 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -243,6 +243,7 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use return "", err } defer cancel() + // Merge commits. switch mergeStyle { case repo_model.MergeStyleMerge: From 531e8dbb2562cec4429d22d1113bd39916bb1ac6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 16:13:48 +0800 Subject: [PATCH 12/30] Revert unnecessary change --- services/automerge/automerge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index be3d0201b8bf3..4a8499503b862 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -136,9 +136,9 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. if err != nil { return nil, err } + defer gitRepo.Close() refs, err := gitRepo.GetRefsBySha(sha, "") - gitRepo.Close() if err != nil { return nil, err } From 9331ccdfbb52903e43732d07f981fddd37f7ea05 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 16:24:04 +0800 Subject: [PATCH 13/30] Use better names for the automerge functions --- services/automerge/automerge.go | 8 ++++---- services/automerge/notify.go | 4 ++-- services/repository/commitstatus/commitstatus.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 4a8499503b862..60a6ca80945a5 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -88,8 +88,8 @@ func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull * }) } -// StartPullRequestAutoMergeCheckBySHA start an automerge check task for repository and SHA -func StartPullRequestAutoMergeCheckBySHA(ctx context.Context, sha string, repo *repo_model.Repository) error { +// StartPRCheckAndAutoMergeBySHA start an automerge check and auto merge task for all pull requests of repository and SHA +func StartPRCheckAndAutoMergeBySHA(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() }) @@ -104,8 +104,8 @@ func StartPullRequestAutoMergeCheckBySHA(ctx context.Context, sha string, repo * return nil } -// StartPullRequestAutoMergeCheck start an automerge check task for a pull request -func StartPullRequestAutoMergeCheck(ctx context.Context, pull *issues_model.PullRequest) { +// StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request +func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { return } diff --git a/services/automerge/notify.go b/services/automerge/notify.go index bc65a43f2444a..cb078214f63b7 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 { - if err := StartPullRequestAutoMergeCheckBySHA(ctx, review.CommitID, pr.BaseRepo); err != nil { + if err := StartPRCheckAndAutoMergeBySHA(ctx, review.CommitID, pr.BaseRepo); err != nil { log.Error("StartPullRequestAutoMergeCheckBySHA: %v", err) } } @@ -42,5 +42,5 @@ func (n *automergeNotifier) PullReviewDismiss(ctx context.Context, doer *user_mo return } // as reviews could have blocked a pending automerge let's recheck - StartPullRequestAutoMergeCheck(ctx, review.Issue.PullRequest) + StartPRCheckAndAutoMerge(ctx, review.Issue.PullRequest) } diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index af958aa437991..adc59abed8f47 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -115,7 +115,7 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato } if status.State.IsSuccess() { - if err := automerge.StartPullRequestAutoMergeCheckBySHA(ctx, sha, repo); err != nil { + if err := automerge.StartPRCheckAndAutoMergeBySHA(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 7d14dfcab77b5d0b3c5732b8740d13a211f430b8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 16:44:44 +0800 Subject: [PATCH 14/30] Improve the comments --- services/automerge/automerge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 60a6ca80945a5..abbc02c1fed77 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -222,7 +222,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } if headCommitID != sha { - log.Warn("Head commit id of auto merge %-v does not match sha [%s]", pr, sha) + log.Warn("Head commit id of auto merge %-v does not match sha [%s], it may means the head branch has been updated. Just ignore this request because a new request expected in the queue", pr, sha) return } From 797236ce349cac3a16c34eb0a3b0d08863a05a7d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 10 May 2024 10:40:52 +0800 Subject: [PATCH 15/30] Add tests for pull request automerge when updating or commit status success --- tests/integration/editor_test.go | 35 ++++---- tests/integration/pull_merge_test.go | 124 +++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 15 deletions(-) diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 045567ce77c4d..f510c79bc6b01 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -4,6 +4,7 @@ package integration import ( + "fmt" "net/http" "net/http/httptest" "net/url" @@ -19,25 +20,29 @@ import ( func TestCreateFile(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { session := loginUser(t, "user2") + testCreateFile(t, session, "user2", "repo1", "master", "test.txt", "Content") + }) +} - // Request editor page - req := NewRequest(t, "GET", "/user2/repo1/_new/master/") - resp := session.MakeRequest(t, req, http.StatusOK) +func testCreateFile(t *testing.T, session *TestSession, user, repo, branch, filePath, content string) *httptest.ResponseRecorder { + // Request editor page + newURL := fmt.Sprintf("/%s/%s/_new/%s/", user, repo, branch) + req := NewRequest(t, "GET", newURL) + resp := session.MakeRequest(t, req, http.StatusOK) - doc := NewHTMLParser(t, resp.Body) - lastCommit := doc.GetInputValueByName("last_commit") - assert.NotEmpty(t, lastCommit) + doc := NewHTMLParser(t, resp.Body) + lastCommit := doc.GetInputValueByName("last_commit") + assert.NotEmpty(t, lastCommit) - // Save new file to master branch - req = NewRequestWithValues(t, "POST", "/user2/repo1/_new/master/", map[string]string{ - "_csrf": doc.GetCSRF(), - "last_commit": lastCommit, - "tree_path": "test.txt", - "content": "Content", - "commit_choice": "direct", - }) - session.MakeRequest(t, req, http.StatusSeeOther) + // Save new file to master branch + req = NewRequestWithValues(t, "POST", newURL, map[string]string{ + "_csrf": doc.GetCSRF(), + "last_commit": lastCommit, + "tree_path": filePath, + "content": content, + "commit_choice": "direct", }) + return session.MakeRequest(t, req, http.StatusSeeOther) } func TestCreateFileOnProtectedBranch(t *testing.T) { diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 826568caf2b4f..7abdf9a4c4a84 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/models" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -30,8 +31,11 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/pull" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" + commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" @@ -648,3 +652,123 @@ func TestPullMergeIndexerNotifier(t *testing.T) { } }) } + +func TestPullAutoMergeAfterUpdated(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + // create a pull request + session := loginUser(t, "user1") + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + forkedName := "repo1-1" + testRepoFork(t, session, "user2", "repo1", "user1", forkedName) + defer func() { + testDeleteRepository(t, session, "user1", forkedName) + }() + testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") + testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") + + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) + forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ + BaseRepoID: baseRepo.ID, + BaseBranch: "master", + HeadRepoID: forkedRepo.ID, + HeadBranch: "master", + }) + + testCreateFile(t, session, "user2", "repo1", "master", "README-1.md", "Hello, World (Edited - 2)\n") + + // first time insert automerge record, return true + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + assert.NoError(t, err) + assert.True(t, scheduled) + + // second time insert automerge record, return false because it does exist + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + assert.Error(t, err) + assert.False(t, scheduled) + + // reload pr again + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.False(t, pr.HasMerged) + assert.Empty(t, pr.MergedCommitID) + + // update the pull request, then it should be merged automatically + err = pull_service.Update(db.DefaultContext, pr, user1, "update for auto merge", false) + assert.NoError(t, err) + + // realod pr again + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + assert.True(t, pr.HasMerged) + assert.NotEmpty(t, pr.MergedCommitID) + }) +} + +func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + // create a pull request + session := loginUser(t, "user1") + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + forkedName := "repo1-2" + testRepoFork(t, session, "user2", "repo1", "user1", forkedName) + defer func() { + testDeleteRepository(t, session, "user1", forkedName) + }() + testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") + testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") + + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) + forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ + BaseRepoID: baseRepo.ID, + BaseBranch: "master", + HeadRepoID: forkedRepo.ID, + HeadBranch: "master", + }) + + // add protected branch for commit status + csrf := GetCSRF(t, session, "/user2/repo1/settings/branches") + // Change master branch to protected + req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{ + "_csrf": csrf, + "rule_name": "master", + "enable_push": "true", + "enable_status_check": "true", + "status_check_contexts": `["gitea/actions"]`, + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + // first time insert automerge record, return true + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + assert.NoError(t, err) + assert.True(t, scheduled) + + // second time insert automerge record, return false because it does exist + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + assert.Error(t, err) + assert.False(t, scheduled) + + // reload pr again + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.False(t, pr.HasMerged) + assert.Empty(t, pr.MergedCommitID) + + // update commit status to success, then it should be merged automatically + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + assert.NoError(t, err) + sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + assert.NoError(t, err) + baseGitRepo.Close() + + err = commitstatus_service.CreateCommitStatus(db.DefaultContext, baseRepo, user1, sha, &git_model.CommitStatus{ + State: api.CommitStatusSuccess, + TargetURL: "https://gitea.com", + Context: "gitea/actions", + }) + assert.NoError(t, err) + + // realod pr again + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + assert.True(t, pr.HasMerged) + assert.NotEmpty(t, pr.MergedCommitID) + }) +} From 5c0f00a7821760195460a986ea8061f0f01f694f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 10 May 2024 14:59:58 +0800 Subject: [PATCH 16/30] Fix test --- tests/integration/pull_merge_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 7abdf9a4c4a84..2996e2e3a31a6 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -33,7 +33,6 @@ import ( "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/pull" - pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" files_service "code.gitea.io/gitea/services/repository/files" @@ -693,7 +692,7 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { assert.Empty(t, pr.MergedCommitID) // update the pull request, then it should be merged automatically - err = pull_service.Update(db.DefaultContext, pr, user1, "update for auto merge", false) + err = pull.Update(db.DefaultContext, pr, user1, "update for auto merge", false) assert.NoError(t, err) // realod pr again From 44b306ad6eec46ed3beaef271eb0f7738c0bc17e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 11 May 2024 12:33:47 +0800 Subject: [PATCH 17/30] Fix test --- tests/integration/pull_merge_test.go | 39 +++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 2996e2e3a31a6..900df4e574fc2 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -12,6 +12,7 @@ import ( "net/url" "os" "path" + "path/filepath" "strings" "testing" "time" @@ -21,6 +22,7 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -652,6 +654,21 @@ func TestPullMergeIndexerNotifier(t *testing.T) { }) } +func testResetRepo(t *testing.T, repoPath, branch, commitID string) { + f, err := os.OpenFile(filepath.Join(repoPath, "refs", "heads", branch), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) + assert.NoError(t, err) + defer f.Close() + _, err = f.WriteString(commitID + "\n") + assert.NoError(t, err) + + repo, err := git.OpenRepository(context.Background(), repoPath) + assert.NoError(t, err) + defer repo.Close() + id, err := repo.GetBranchCommitID(branch) + assert.NoError(t, err) + assert.EqualValues(t, commitID, id) +} + func TestPullAutoMergeAfterUpdated(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request @@ -663,7 +680,7 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { testDeleteRepository(t, session, "user1", forkedName) }() testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") - testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") + testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Test Update Auto Merge") baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName}) @@ -673,8 +690,19 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { HeadRepoID: forkedRepo.ID, HeadBranch: "master", }) + assert.NoError(t, pr.LoadIssue(db.DefaultContext)) + assert.EqualValues(t, "Test Update Auto Merge", pr.Issue.Title) + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + assert.NoError(t, err) + previousCommitID, err := baseGitRepo.GetBranchCommitID("master") + baseGitRepo.Close() + assert.NoError(t, err) + // add a new file for base repository, so the pull request needs to be updated testCreateFile(t, session, "user2", "repo1", "master", "README-1.md", "Hello, World (Edited - 2)\n") + defer func() { + testResetRepo(t, baseRepo.RepoPath(), "master", previousCommitID) + }() // first time insert automerge record, return true scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") @@ -695,10 +723,13 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { err = pull.Update(db.DefaultContext, pr, user1, "update for auto merge", false) assert.NoError(t, err) + time.Sleep(5 * time.Second) // realod pr again - pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.True(t, pr.HasMerged) assert.NotEmpty(t, pr.MergedCommitID) + + unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID}) }) } @@ -766,8 +797,10 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { assert.NoError(t, err) // realod pr again - pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.True(t, pr.HasMerged) assert.NotEmpty(t, pr.MergedCommitID) + + unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID}) }) } From 0d4cce72213cb810d7dad3902b925da26ea80bed Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 11 May 2024 17:18:43 +0800 Subject: [PATCH 18/30] Fix tests --- tests/integration/pull_merge_test.go | 74 ++++++++++++++++++--------- tests/integration/pull_review_test.go | 12 ++--- 2 files changed, 57 insertions(+), 29 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 900df4e574fc2..c66246fbaeda2 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -13,6 +13,7 @@ import ( "os" "path" "path/filepath" + "strconv" "strings" "testing" "time" @@ -669,7 +670,7 @@ func testResetRepo(t *testing.T, repoPath, branch, commitID string) { assert.EqualValues(t, commitID, id) } -func TestPullAutoMergeAfterUpdated(t *testing.T) { +func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request session := loginUser(t, "user1") @@ -680,7 +681,7 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { testDeleteRepository(t, session, "user1", forkedName) }() testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") - testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Test Update Auto Merge") + testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName}) @@ -690,27 +691,26 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { HeadRepoID: forkedRepo.ID, HeadBranch: "master", }) - assert.NoError(t, pr.LoadIssue(db.DefaultContext)) - assert.EqualValues(t, "Test Update Auto Merge", pr.Issue.Title) - baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) - assert.NoError(t, err) - previousCommitID, err := baseGitRepo.GetBranchCommitID("master") - baseGitRepo.Close() - assert.NoError(t, err) - // add a new file for base repository, so the pull request needs to be updated - testCreateFile(t, session, "user2", "repo1", "master", "README-1.md", "Hello, World (Edited - 2)\n") - defer func() { - testResetRepo(t, baseRepo.RepoPath(), "master", previousCommitID) - }() + // add protected branch for commit status + csrf := GetCSRF(t, session, "/user2/repo1/settings/branches") + // Change master branch to protected + req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{ + "_csrf": csrf, + "rule_name": "master", + "enable_push": "true", + "enable_status_check": "true", + "status_check_contexts": "gitea/actions", + }) + session.MakeRequest(t, req, http.StatusSeeOther) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") assert.Error(t, err) assert.False(t, scheduled) @@ -719,11 +719,22 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { assert.False(t, pr.HasMerged) assert.Empty(t, pr.MergedCommitID) - // update the pull request, then it should be merged automatically - err = pull.Update(db.DefaultContext, pr, user1, "update for auto merge", false) + // update commit status to success, then it should be merged automatically + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) assert.NoError(t, err) + sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + assert.NoError(t, err) + baseGitRepo.Close() + + err = commitstatus_service.CreateCommitStatus(db.DefaultContext, baseRepo, user1, sha, &git_model.CommitStatus{ + State: api.CommitStatusSuccess, + TargetURL: "https://gitea.com", + Context: "gitea/actions", + }) + assert.NoError(t, err) + + time.Sleep(2 * time.Second) - time.Sleep(5 * time.Second) // realod pr again pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.True(t, pr.HasMerged) @@ -733,7 +744,7 @@ func TestPullAutoMergeAfterUpdated(t *testing.T) { }) } -func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { +func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request session := loginUser(t, "user1") @@ -763,17 +774,18 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { "rule_name": "master", "enable_push": "true", "enable_status_check": "true", - "status_check_contexts": `["gitea/actions"]`, + "status_check_contexts": "gitea/actions", + "required_approvals": "1", }) session.MakeRequest(t, req, http.StatusSeeOther) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "") + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") assert.Error(t, err) assert.False(t, scheduled) @@ -796,6 +808,22 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { }) assert.NoError(t, err) + time.Sleep(2 * time.Second) + + // reload pr again + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.False(t, pr.HasMerged) + assert.Empty(t, pr.MergedCommitID) + + // approve the PR from non-author + approveSession := loginUser(t, "user2") + req = NewRequest(t, "GET", fmt.Sprintf("/user2/repo1/pulls/%d", pr.Index)) + resp := approveSession.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + testSubmitReview(t, approveSession, htmlDoc.GetCSRF(), "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK) + + time.Sleep(2 * time.Second) + // realod pr again pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.True(t, pr.HasMerged) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 273332a36b416..df5d7b38ea49a 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -202,10 +202,10 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) { htmlDoc := NewHTMLParser(t, resp.Body) // Submit an approve review on the PR. - testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity) + testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity) // Submit a reject review on the PR. - testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity) + testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity) }) t.Run("Submit approve/reject review on closed PR", func(t *testing.T) { @@ -222,18 +222,18 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) { htmlDoc := NewHTMLParser(t, resp.Body) // Submit an approve review on the PR. - testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity) + testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity) // Submit a reject review on the PR. - testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity) + testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity) }) }) } -func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder { +func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, commitID, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder { options := map[string]string{ "_csrf": csrf, - "commit_id": "", + "commit_id": commitID, "content": "test", "type": reviewType, } From 438d8804a3011d02040ae78b8955942ab2f43ac2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 11 May 2024 17:29:28 +0800 Subject: [PATCH 19/30] Fix test --- tests/integration/pull_merge_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index c66246fbaeda2..816df25358282 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -724,7 +724,12 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) + masterCommitID, err := baseGitRepo.GetRefCommitID("master") + assert.NoError(t, err) baseGitRepo.Close() + defer func() { + testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) + }() err = commitstatus_service.CreateCommitStatus(db.DefaultContext, baseRepo, user1, sha, &git_model.CommitStatus{ State: api.CommitStatusSuccess, @@ -799,7 +804,12 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) + masterCommitID, err := baseGitRepo.GetRefCommitID("master") + assert.NoError(t, err) baseGitRepo.Close() + defer func() { + testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) + }() err = commitstatus_service.CreateCommitStatus(db.DefaultContext, baseRepo, user1, sha, &git_model.CommitStatus{ State: api.CommitStatusSuccess, From fd8dda36c3a7f703b279f2db0497d0d0283c7e12 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 13 May 2024 15:04:45 +0800 Subject: [PATCH 20/30] improvment --- tests/integration/pull_merge_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 816df25358282..ecc7002a4116f 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -658,9 +658,9 @@ func TestPullMergeIndexerNotifier(t *testing.T) { func testResetRepo(t *testing.T, repoPath, branch, commitID string) { f, err := os.OpenFile(filepath.Join(repoPath, "refs", "heads", branch), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) assert.NoError(t, err) - defer f.Close() _, err = f.WriteString(commitID + "\n") assert.NoError(t, err) + f.Close() repo, err := git.OpenRepository(context.Background(), repoPath) assert.NoError(t, err) From deeea795e409c7e5ce33eb5d93e337e67972c842 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 18 May 2024 13:16:00 +0800 Subject: [PATCH 21/30] add trace for test --- tests/integration/pull_merge_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index ecc7002a4116f..12c9bf17de90c 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -725,7 +725,11 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) masterCommitID, err := baseGitRepo.GetRefCommitID("master") - assert.NoError(t, err) + if !assert.NoError(t, err) { + branches, _, err := baseGitRepo.GetBranchNames(0, 100) + assert.NoError(t, err) + fmt.Println("----", branches) + } baseGitRepo.Close() defer func() { testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) From 8331fa0a08cb6ca04edfaaad28f0de975dbb9cf1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 18 May 2024 21:54:21 +0800 Subject: [PATCH 22/30] Add trace --- tests/integration/pull_merge_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 12c9bf17de90c..f48add316e499 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "fmt" + "log" "net/http" "net/http/httptest" "net/url" @@ -728,7 +729,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { if !assert.NoError(t, err) { branches, _, err := baseGitRepo.GetBranchNames(0, 100) assert.NoError(t, err) - fmt.Println("----", branches) + log.Println("----", branches) } baseGitRepo.Close() defer func() { From d9e8a423d7dc301b70d03f87e3100522bd6c1e2a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 19 May 2024 11:36:43 +0800 Subject: [PATCH 23/30] add trace for test --- tests/integration/pull_merge_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index f48add316e499..edb2b0d0a4a96 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -7,7 +7,6 @@ import ( "bytes" "context" "fmt" - "log" "net/http" "net/http/httptest" "net/url" @@ -31,6 +30,7 @@ import ( "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/queue" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" @@ -726,11 +726,11 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) masterCommitID, err := baseGitRepo.GetRefCommitID("master") - if !assert.NoError(t, err) { - branches, _, err := baseGitRepo.GetBranchNames(0, 100) - assert.NoError(t, err) - log.Println("----", branches) - } + assert.NoError(t, err) + + branches, _, err := baseGitRepo.GetBranchNames(0, 100) + assert.NoError(t, err) + log.Info("Checking---- %v", branches) baseGitRepo.Close() defer func() { testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) From e447ec2ef5354635ee6ed368cc39627cba1e72a5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 19 May 2024 16:57:41 +0800 Subject: [PATCH 24/30] improve test --- tests/integration/pull_merge_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index edb2b0d0a4a96..a2b2d6eabb7af 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -30,7 +30,6 @@ import ( "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/queue" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" @@ -730,7 +729,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { branches, _, err := baseGitRepo.GetBranchNames(0, 100) assert.NoError(t, err) - log.Info("Checking---- %v", branches) + assert.EqualValues(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches) baseGitRepo.Close() defer func() { testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) From a0adfd55d245fd256e50a7d2ad0fcce1ee20e148 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 20 May 2024 17:35:14 +0800 Subject: [PATCH 25/30] Fix test --- tests/integration/pull_merge_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index a2b2d6eabb7af..18db4695bfc87 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -729,7 +729,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { branches, _, err := baseGitRepo.GetBranchNames(0, 100) assert.NoError(t, err) - assert.EqualValues(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches) + assert.ElementsMatch(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches) baseGitRepo.Close() defer func() { testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID) From 1c202fa989cbec224a8d3e61e9a31d82c72a7984 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 12:31:08 +0800 Subject: [PATCH 26/30] Fix test --- tests/integration/pull_merge_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 18db4695bfc87..fd91b7ada419f 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -724,7 +724,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) - masterCommitID, err := baseGitRepo.GetRefCommitID("master") + masterCommitID, err := baseGitRepo.GetBranchCommitID("master") assert.NoError(t, err) branches, _, err := baseGitRepo.GetBranchNames(0, 100) From 56914ed209d821e4b1967c375861b3408e8762be Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 13:30:49 +0800 Subject: [PATCH 27/30] Fix test --- tests/integration/pull_merge_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index fd91b7ada419f..979c408388760 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -808,7 +808,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) assert.NoError(t, err) - masterCommitID, err := baseGitRepo.GetRefCommitID("master") + masterCommitID, err := baseGitRepo.GetBranchCommitID("master") assert.NoError(t, err) baseGitRepo.Close() defer func() { From a5f039777ab1106f7e0c23e1f1cc57aa12212739 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 21:39:24 +0800 Subject: [PATCH 28/30] Use pull request directly when pull request review approved --- services/automerge/notify.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/services/automerge/notify.go b/services/automerge/notify.go index cb078214f63b7..bcdc82a98d251 100644 --- a/services/automerge/notify.go +++ b/services/automerge/notify.go @@ -26,9 +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 { - if err := StartPRCheckAndAutoMergeBySHA(ctx, review.CommitID, pr.BaseRepo); err != nil { - log.Error("StartPullRequestAutoMergeCheckBySHA: %v", err) - } + StartPRCheckAndAutoMerge(ctx, pr) } } From 9680940c21a276413ec62d30bbe8cce43a3a9bf9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 21:59:50 +0800 Subject: [PATCH 29/30] Revert "Use pull request directly when pull request review approved" This reverts commit a5f039777ab1106f7e0c23e1f1cc57aa12212739. --- 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 bcdc82a98d251..cb078214f63b7 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 { - StartPRCheckAndAutoMerge(ctx, pr) + if err := StartPRCheckAndAutoMergeBySHA(ctx, review.CommitID, pr.BaseRepo); err != nil { + log.Error("StartPullRequestAutoMergeCheckBySHA: %v", err) + } } } From 894537c067f5278e74fdac8a1b0029296b9086e8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 22:22:04 +0800 Subject: [PATCH 30/30] Update services/automerge/automerge.go Co-authored-by: wxiaoguang --- services/automerge/automerge.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 5288e48362330..10f3c28d56be4 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -120,9 +120,8 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques log.Error("OpenRepository: %v", err) return } - + defer gitRepo.Close() commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) - gitRepo.Close() if err != nil { log.Error("GetRefCommitID: %v", err) return