From 8a3ab7888923b56862af2edddce18c125abb4b9a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 21:29:30 +0800 Subject: [PATCH 01/11] Add transaction for merge a pull request --- cmd/hook.go | 3 ++ modules/private/hook.go | 1 + modules/repository/env.go | 5 +++ routers/private/hook_post_receive.go | 57 +++++++++++++++++++++++++++- services/pull/merge.go | 24 ++++-------- services/pull/update.go | 2 +- 6 files changed, 73 insertions(+), 19 deletions(-) diff --git a/cmd/hook.go b/cmd/hook.go index 2a9c25add5237..68ba19bc0c21d 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -341,6 +341,7 @@ Gitea or set your environment appropriately.`, "") isWiki, _ := strconv.ParseBool(os.Getenv(repo_module.EnvRepoIsWiki)) repoName := os.Getenv(repo_module.EnvRepoName) pusherID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPusherID), 10, 64) + prID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPRID), 10, 64) pusherName := os.Getenv(repo_module.EnvPusherName) hookOptions := private.HookOptions{ @@ -350,6 +351,8 @@ Gitea or set your environment appropriately.`, "") GitObjectDirectory: os.Getenv(private.GitObjectDirectory), GitQuarantinePath: os.Getenv(private.GitQuarantinePath), GitPushOptions: pushOptions(), + PullRequestID: prID, + PullRequestAction: os.Getenv(repo_module.EnvPRAction), } oldCommitIDs := make([]string, hookBatchSize) newCommitIDs := make([]string, hookBatchSize) diff --git a/modules/private/hook.go b/modules/private/hook.go index 79c3d482298cc..61aa3865af24d 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -54,6 +54,7 @@ type HookOptions struct { GitQuarantinePath string GitPushOptions GitPushOptions PullRequestID int64 + PullRequestAction string DeployKeyID int64 // if the pusher is a DeployKey, then UserID is the repo's org user. IsWiki bool ActionPerm int diff --git a/modules/repository/env.go b/modules/repository/env.go index 30edd1c9e326c..65e23b6e5cfec 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -25,11 +25,16 @@ const ( EnvKeyID = "GITEA_KEY_ID" // public key ID EnvDeployKeyID = "GITEA_DEPLOY_KEY_ID" EnvPRID = "GITEA_PR_ID" + EnvPRAction = "GITEA_PR_ACTION" EnvIsInternal = "GITEA_INTERNAL_PUSH" EnvAppURL = "GITEA_ROOT_URL" EnvActionPerm = "GITEA_ACTION_PERM" ) +const ( + PullRequestActionMerge = "merge" +) + // InternalPushingEnvironment returns an os environment to switch off hooks on push // It is recommended to avoid using this unless you are pushing within a transaction // or if you absolutely are sure that post-receive and pre-receive will do nothing diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index adc435b42cbd7..a1d25e017ca3d 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -4,12 +4,15 @@ package private import ( + "context" "fmt" "net/http" + "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" + pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" @@ -18,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/private" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" + timeutil "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" gitea_context "code.gitea.io/gitea/services/context" @@ -160,6 +164,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate) + var pusher *user_model.User // Handle Push Options if isPrivate.Has() || isTemplate.Has() { // load the repository @@ -172,7 +177,8 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - pusher, err := user_model.GetUserByID(ctx, opts.UserID) + var err error + pusher, err = user_model.GetUserByID(ctx, opts.UserID) if err != nil { log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ @@ -218,6 +224,55 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } + // handle pull request merging, a pull request action should only push 1 commit + if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { + // Get the pull request + pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) + if err != nil { + log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), + }) + return + } + + // Removing an auto merge pull and ignore if not exist + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + log.Error("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err), + }) + return + } + + if pusher == nil { + pusher, err = user_model.GetUserByID(ctx, opts.UserID) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } + + pr.MergedCommitID = updates[len(updates)-1].NewCommitID + pr.MergedUnix = timeutil.TimeStampNow() + pr.Merger = pusher + pr.MergerID = opts.UserID + + if err := db.WithTx(ctx, func(ctx context.Context) error { + _, err := pr.SetMerged(ctx) + return err + }); err != nil { + log.Error("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } + results := make([]private.HookPostReceiveBranchResult, 0, len(opts.OldCommitIDs)) // We have to reload the repo in case its state is changed above diff --git a/services/pull/merge.go b/services/pull/merge.go index 00f23e1e3ae23..4891e758c7e6a 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -18,7 +18,6 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" - pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -162,12 +161,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,19 +177,11 @@ 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) + pr.MergedCommitID, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, true) if err != nil { return err } - 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) - } - if err := pr.LoadIssue(ctx); err != nil { log.Error("LoadIssue %-v: %v", pr, err) } @@ -245,7 +230,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } // 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 doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, isMerge bool) (string, error) { // Clone base repo. mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID) if err != nil { @@ -318,6 +303,11 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use pr.BaseRepo.Name, pr.ID, ) + action := "" + if isMerge { + action = repo_module.PullRequestActionMerge + } + mergeCtx.env = append(mergeCtx.env, repo_module.EnvPRAction+"="+action) pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch) // Push back to upstream. diff --git a/services/pull/update.go b/services/pull/update.go index 9b676e13ef941..eedc8ef0cf0f9 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -72,7 +72,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. BaseBranch: pr.HeadBranch, } - _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message) + _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, false) defer func() { go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "") From a5985e43b325f28dcb2cebf769f769824d18d66b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 21:36:20 +0800 Subject: [PATCH 02/11] Move pull request handle position on post receive --- routers/private/hook_post_receive.go | 116 +++++++++++++-------------- 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index a1d25e017ca3d..a9aae80cd561e 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -162,9 +162,56 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } + var pusher *user_model.User + + // handle pull request merging, a pull request action should only push 1 commit + if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { + // Get the pull request + pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) + if err != nil { + log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), + }) + return + } + + pusher, err = user_model.GetUserByID(ctx, opts.UserID) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + + pr.MergedCommitID = updates[len(updates)-1].NewCommitID + pr.MergedUnix = timeutil.TimeStampNow() + pr.Merger = pusher + pr.MergerID = opts.UserID + + 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) + } + + if _, err := pr.SetMerged(ctx); err != nil { + return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + } + return nil + }); err != nil { + log.Error("%v", err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: err.Error(), + }) + return + } + } + isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate) - var pusher *user_model.User + // Handle Push Options if isPrivate.Has() || isTemplate.Has() { // load the repository @@ -177,14 +224,16 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - var err error - pusher, err = user_model.GetUserByID(ctx, opts.UserID) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return + if pusher == nil { + var err error + pusher, err = user_model.GetUserByID(ctx, opts.UserID) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } } perm, err := access_model.GetUserRepoPermission(ctx, repo, pusher) if err != nil { @@ -224,55 +273,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } - // handle pull request merging, a pull request action should only push 1 commit - if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { - // Get the pull request - pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) - if err != nil { - log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), - }) - return - } - - // Removing an auto merge pull and ignore if not exist - if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { - log.Error("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err), - }) - return - } - - if pusher == nil { - pusher, err = user_model.GetUserByID(ctx, opts.UserID) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - } - - pr.MergedCommitID = updates[len(updates)-1].NewCommitID - pr.MergedUnix = timeutil.TimeStampNow() - pr.Merger = pusher - pr.MergerID = opts.UserID - - if err := db.WithTx(ctx, func(ctx context.Context) error { - _, err := pr.SetMerged(ctx) - return err - }); err != nil { - log.Error("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - } - results := make([]private.HookPostReceiveBranchResult, 0, len(opts.OldCommitIDs)) // We have to reload the repo in case its state is changed above From 16adc83852a24c28a00b6bd7bd7e43eae4d86d83 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 21:54:27 +0800 Subject: [PATCH 03/11] Reload pull request after pushing --- services/pull/merge.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 4891e758c7e6a..66c947040e8ec 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -182,6 +182,12 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return err } + // reload pull request because it has been updated by post receive hook + pr, err = issues_model.GetPullRequestByID(ctx, pr.ID) + if err != nil { + return err + } + if err := pr.LoadIssue(ctx); err != nil { log.Error("LoadIssue %-v: %v", pr, err) } @@ -311,8 +317,8 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch) // Push back to upstream. - // TODO: this cause an api call to "/api/internal/hook/post-receive/...", - // that prevents us from doint the whole merge in one db transaction + // 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{ From 15b2cde4a5b2988ae9d10739c553347ed4ef73d9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 22:04:43 +0800 Subject: [PATCH 04/11] some improvements --- routers/private/hook_post_receive.go | 96 +++++++++++++++------------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index a9aae80cd561e..7bcbe599cee93 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -164,49 +164,10 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { var pusher *user_model.User - // handle pull request merging, a pull request action should only push 1 commit - if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { - // Get the pull request - pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) - if err != nil { - log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), - }) - return - } - - pusher, err = user_model.GetUserByID(ctx, opts.UserID) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - - pr.MergedCommitID = updates[len(updates)-1].NewCommitID - pr.MergedUnix = timeutil.TimeStampNow() - pr.Merger = pusher - pr.MergerID = opts.UserID - - 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) - } - - if _, err := pr.SetMerged(ctx); err != nil { - return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) - } - return nil - }); err != nil { - log.Error("%v", err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: err.Error(), - }) - return - } + // handle pull request merging, a pull request action should push at least 1 commit + handlePullRequestMerging(ctx, opts, pusher, ownerName, repoName, updates) + if ctx.Written() { + return } isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) @@ -362,3 +323,52 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { RepoWasEmpty: wasEmpty, }) } + +func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, pusher *user_model.User, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { + // handle pull request merging, a pull request action should only push 1 commit + if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { + // Get the pull request + pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) + if err != nil { + log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), + }) + return + } + + if pusher == nil { + pusher, err = user_model.GetUserByID(ctx, opts.UserID) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } + + pr.MergedCommitID = updates[len(updates)-1].NewCommitID + pr.MergedUnix = timeutil.TimeStampNow() + pr.Merger = pusher + pr.MergerID = opts.UserID + + 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) + } + + if _, err := pr.SetMerged(ctx); err != nil { + return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + } + return nil + }); err != nil { + log.Error("%v", err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: err.Error(), + }) + return + } + } +} From a164a92737ddcc6926ea4242a3293fd3285a438f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 22:06:44 +0800 Subject: [PATCH 05/11] some adjustments --- routers/private/hook_post_receive.go | 1 - services/pull/merge.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 7bcbe599cee93..973b5e1a1ad39 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -172,7 +172,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate) - // Handle Push Options if isPrivate.Has() || isTemplate.Has() { // load the repository diff --git a/services/pull/merge.go b/services/pull/merge.go index 66c947040e8ec..b11d15ae3157b 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -177,7 +177,7 @@ 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, true) + _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, true) if err != nil { return err } From 188e55b0e0e12710bd2b01774e2fbe987b658828 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 May 2024 13:56:31 +0800 Subject: [PATCH 06/11] Use context cache for loading pusher --- routers/private/hook_post_receive.go | 47 ++++++++++++++-------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 973b5e1a1ad39..0f4d28c8324b9 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -15,6 +15,7 @@ import ( pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" @@ -162,10 +163,8 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } - var pusher *user_model.User - // handle pull request merging, a pull request action should push at least 1 commit - handlePullRequestMerging(ctx, opts, pusher, ownerName, repoName, updates) + handlePullRequestMerging(ctx, opts, ownerName, repoName, updates) if ctx.Written() { return } @@ -184,17 +183,17 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - if pusher == nil { - var err error - pusher, err = user_model.GetUserByID(ctx, opts.UserID) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } + pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { + return user_model.GetUserByID(ctx, opts.UserID) + }) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return } + perm, err := access_model.GetUserRepoPermission(ctx, repo, pusher) if err != nil { log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) @@ -323,7 +322,9 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) } -func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, pusher *user_model.User, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { +const contextCachePusherKey = "hook_post_receive_pusher" + +func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { // handle pull request merging, a pull request action should only push 1 commit if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { // Get the pull request @@ -336,15 +337,15 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H return } - if pusher == nil { - pusher, err = user_model.GetUserByID(ctx, opts.UserID) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } + pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { + return user_model.GetUserByID(ctx, opts.UserID) + }) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return } pr.MergedCommitID = updates[len(updates)-1].NewCommitID From 45c544932e6e427e8c315371d8d06947f987199c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 May 2024 13:58:35 +0800 Subject: [PATCH 07/11] Remove unnecessary changes --- routers/private/hook_post_receive.go | 1 - 1 file changed, 1 deletion(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 0f4d28c8324b9..c44c8c07b1eb9 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -193,7 +193,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) return } - perm, err := access_model.GetUserRepoPermission(ctx, repo, pusher) if err != nil { log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) From 3e320f865f1c93e1ce646da74f5cacc6c404e3e1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 May 2024 17:29:09 +0800 Subject: [PATCH 08/11] Follow wxiaoguang's suggestion --- cmd/hook.go | 2 +- modules/private/hook.go | 2 +- modules/repository/env.go | 6 ++- routers/private/hook_post_receive.go | 80 ++++++++++++++-------------- services/pull/merge.go | 11 ++-- services/pull/update.go | 2 +- 6 files changed, 52 insertions(+), 51 deletions(-) diff --git a/cmd/hook.go b/cmd/hook.go index 68ba19bc0c21d..95491c2e467c8 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -352,7 +352,7 @@ Gitea or set your environment appropriately.`, "") GitQuarantinePath: os.Getenv(private.GitQuarantinePath), GitPushOptions: pushOptions(), PullRequestID: prID, - PullRequestAction: os.Getenv(repo_module.EnvPRAction), + PushTrigger: os.Getenv(repo_module.EnvPushTrigger), } oldCommitIDs := make([]string, hookBatchSize) newCommitIDs := make([]string, hookBatchSize) diff --git a/modules/private/hook.go b/modules/private/hook.go index 61aa3865af24d..8f4724c5b0747 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -54,7 +54,7 @@ type HookOptions struct { GitQuarantinePath string GitPushOptions GitPushOptions PullRequestID int64 - PullRequestAction string + PushTrigger string DeployKeyID int64 // if the pusher is a DeployKey, then UserID is the repo's org user. IsWiki bool ActionPerm int diff --git a/modules/repository/env.go b/modules/repository/env.go index 65e23b6e5cfec..4560ea81e7e41 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -25,14 +25,16 @@ const ( EnvKeyID = "GITEA_KEY_ID" // public key ID EnvDeployKeyID = "GITEA_DEPLOY_KEY_ID" EnvPRID = "GITEA_PR_ID" - EnvPRAction = "GITEA_PR_ACTION" + EnvPushTrigger = "GITEA_PUSH_TRIGGER" EnvIsInternal = "GITEA_INTERNAL_PUSH" EnvAppURL = "GITEA_ROOT_URL" EnvActionPerm = "GITEA_ACTION_PERM" ) +type PushTrigger string + const ( - PullRequestActionMerge = "merge" + PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" ) // InternalPushingEnvironment returns an os environment to switch off hooks on push diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index c44c8c07b1eb9..58e5c7b142d2d 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -323,51 +323,53 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { const contextCachePusherKey = "hook_post_receive_pusher" +// handlePullRequestMerging handle pull request merging, a pull request action should only push 1 commit func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { - // handle pull request merging, a pull request action should only push 1 commit - if opts.PullRequestAction == repo_module.PullRequestActionMerge && len(updates) >= 1 { - // Get the pull request - pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) - if err != nil { - log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), - }) - return - } + if opts.PushTrigger != string(repo_module.PushTriggerPRMergeToBase) || len(updates) < 1 { + return + } - pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { - return user_model.GetUserByID(ctx, opts.UserID) + // Get the pull request + pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) + if err != nil { + log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), }) - if err != nil { - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } + return + } - pr.MergedCommitID = updates[len(updates)-1].NewCommitID - pr.MergedUnix = timeutil.TimeStampNow() - pr.Merger = pusher - pr.MergerID = opts.UserID + pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { + return user_model.GetUserByID(ctx, opts.UserID) + }) + if err != nil { + log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } - 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) - } + pr.MergedCommitID = updates[len(updates)-1].NewCommitID + pr.MergedUnix = timeutil.TimeStampNow() + pr.Merger = pusher + pr.MergerID = opts.UserID - if _, err := pr.SetMerged(ctx); err != nil { - return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) - } - return nil - }); err != nil { - log.Error("%v", err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: err.Error(), - }) - return + 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) } + + if _, err := pr.SetMerged(ctx); err != nil { + return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + } + return nil + }); err != nil { + log.Error("%v", err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: err.Error(), + }) + return } } diff --git a/services/pull/merge.go b/services/pull/merge.go index b11d15ae3157b..20be7c5b5a877 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -177,7 +177,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") }() - _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, true) + _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) if err != nil { return err } @@ -236,7 +236,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } // 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, isMerge bool) (string, error) { +func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, pushTrigger repo_module.PushTrigger) (string, error) { // Clone base repo. mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID) if err != nil { @@ -309,11 +309,8 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use pr.BaseRepo.Name, pr.ID, ) - action := "" - if isMerge { - action = repo_module.PullRequestActionMerge - } - mergeCtx.env = append(mergeCtx.env, repo_module.EnvPRAction+"="+action) + + 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. diff --git a/services/pull/update.go b/services/pull/update.go index eedc8ef0cf0f9..d912d5a8d557a 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -72,7 +72,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. BaseBranch: pr.HeadBranch, } - _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, false) + _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, "") defer func() { go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "") From 430d9df43789fcff102e41ccfcdc11ce2b5787f1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 3 May 2024 22:22:55 +0800 Subject: [PATCH 09/11] fix --- cmd/hook.go | 2 +- modules/private/hook.go | 3 +- modules/repository/env.go | 3 +- routers/private/hook_post_receive.go | 58 +++++++++++------------ routers/private/hook_post_receive_test.go | 37 +++++++++++++++ services/contexttest/context_tests.go | 13 +++++ services/pull/update.go | 3 +- 7 files changed, 86 insertions(+), 33 deletions(-) create mode 100644 routers/private/hook_post_receive_test.go diff --git a/cmd/hook.go b/cmd/hook.go index 95491c2e467c8..c2fb910235d4b 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -352,7 +352,7 @@ Gitea or set your environment appropriately.`, "") GitQuarantinePath: os.Getenv(private.GitQuarantinePath), GitPushOptions: pushOptions(), PullRequestID: prID, - PushTrigger: os.Getenv(repo_module.EnvPushTrigger), + PushTrigger: repo_module.PushTrigger(os.Getenv(repo_module.EnvPushTrigger)), } oldCommitIDs := make([]string, hookBatchSize) newCommitIDs := make([]string, hookBatchSize) diff --git a/modules/private/hook.go b/modules/private/hook.go index 8f4724c5b0747..49d92987441c6 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" ) @@ -54,7 +55,7 @@ type HookOptions struct { GitQuarantinePath string GitPushOptions GitPushOptions PullRequestID int64 - PushTrigger string + PushTrigger repository.PushTrigger DeployKeyID int64 // if the pusher is a DeployKey, then UserID is the repo's org user. IsWiki bool ActionPerm int diff --git a/modules/repository/env.go b/modules/repository/env.go index 4560ea81e7e41..e4f32092fc792 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -34,7 +34,8 @@ const ( type PushTrigger string const ( - PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" + PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" + PushTriggerPRUpdateWithBase PushTrigger = "pr-update-with-base" ) // InternalPushingEnvironment returns an os environment to switch off hooks on push diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 58e5c7b142d2d..d69d342f4ecad 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -164,9 +164,11 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } // handle pull request merging, a pull request action should push at least 1 commit - handlePullRequestMerging(ctx, opts, ownerName, repoName, updates) - if ctx.Written() { - return + if opts.PushTrigger == repo_module.PushTriggerPRMergeToBase { + handlePullRequestMerging(ctx, opts, ownerName, repoName, updates) + if ctx.Written() { + return + } } isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) @@ -183,9 +185,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { - return user_model.GetUserByID(ctx, opts.UserID) - }) + pusher, err := loadContextCacheUser(ctx, opts.UserID) if err != nil { log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ @@ -321,55 +321,55 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) } -const contextCachePusherKey = "hook_post_receive_pusher" +func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, error) { + return cache.GetWithContextCache(ctx, "hook_post_receive_user", id, func() (*user_model.User, error) { + return user_model.GetUserByID(ctx, id) + }) +} // handlePullRequestMerging handle pull request merging, a pull request action should only push 1 commit func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { - if opts.PushTrigger != string(repo_module.PushTriggerPRMergeToBase) || len(updates) < 1 { + if len(updates) == 0 { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID), + }) return } + if len(updates) != 1 && !setting.IsProd { + // it shouldn't happen + panic(fmt.Sprintf("Pushing a merged PR (pr:%d) should only push 1 commit, but got %d", opts.PullRequestID, len(updates))) + } - // Get the pull request pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) if err != nil { log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), - }) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "GetPullRequestByID failed"}) return } - pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { - return user_model.GetUserByID(ctx, opts.UserID) - }) + pusher, err := loadContextCacheUser(ctx, opts.UserID) if err != nil { log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), - }) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Load pusher user failed"}) return } pr.MergedCommitID = updates[len(updates)-1].NewCommitID pr.MergedUnix = timeutil.TimeStampNow() pr.Merger = pusher - pr.MergerID = opts.UserID - - if err := db.WithTx(ctx, func(ctx context.Context) error { + pr.MergerID = pusher.ID + 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 fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) } - if _, err := pr.SetMerged(ctx); err != nil { - return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err) } return nil - }); err != nil { - log.Error("%v", err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: err.Error(), - }) - return + }) + if err != nil { + log.Error("Failed to update PR to merged: %v", err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Failed to update PR to merged"}) } } diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go new file mode 100644 index 0000000000000..d4401bc6995a2 --- /dev/null +++ b/routers/private/hook_post_receive_test.go @@ -0,0 +1,37 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package private + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/private" + repo_module "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/services/contexttest" + + "github.com/stretchr/testify/assert" +) + +func TestHandlePullRequestMerging(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + pr, err := issues_model.GetUnmergedPullRequest(db.DefaultContext, 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub) + assert.NoError(t, err) + assert.NoError(t, pr.LoadBaseRepo(db.DefaultContext)) + ctx, resp := contexttest.MockPrivateContext(t, "/") + handlePullRequestMerging(ctx, &private.HookOptions{ + PullRequestID: pr.ID, + UserID: 2, + }, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, []*repo_module.PushUpdateOptions{ + {NewCommitID: "01234567"}, + }) + assert.Equal(t, 0, len(resp.Body.String())) + pr, err = issues_model.GetPullRequestByID(db.DefaultContext, pr.ID) + assert.NoError(t, err) + assert.True(t, pr.HasMerged) + assert.EqualValues(t, "01234567", pr.MergedCommitID) + // TODO: test the removal of auto merge +} diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go index 0c1e5ee54fe2f..5624d24058727 100644 --- a/services/contexttest/context_tests.go +++ b/services/contexttest/context_tests.go @@ -94,6 +94,19 @@ func MockAPIContext(t *testing.T, reqPath string) (*context.APIContext, *httptes return ctx, resp } +func MockPrivateContext(t *testing.T, reqPath string) (*context.PrivateContext, *httptest.ResponseRecorder) { + resp := httptest.NewRecorder() + req := mockRequest(t, reqPath) + base, baseCleanUp := context.NewBaseContext(resp, req) + base.Data = middleware.GetContextData(req.Context()) + base.Locale = &translation.MockLocale{} + ctx := &context.PrivateContext{Base: base} + _ = baseCleanUp // during test, it doesn't need to do clean up. TODO: this can be improved later + chiCtx := chi.NewRouteContext() + ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx) + return ctx, resp +} + // LoadRepo load a repo into a test context. func LoadRepo(t *testing.T, ctx gocontext.Context, repoID int64) { var doer *user_model.User diff --git a/services/pull/update.go b/services/pull/update.go index d912d5a8d557a..d2c0c2df809e5 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -15,6 +15,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/repository" ) // Update updates pull request with base branch. @@ -72,7 +73,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. BaseBranch: pr.HeadBranch, } - _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, "") + _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) defer func() { go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "") From 860121db9f0e9facf31bf7948538df7aec61f336 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 May 2024 23:19:28 +0800 Subject: [PATCH 10/11] Fix comment and bug --- routers/private/hook_post_receive.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index d69d342f4ecad..0c2c1836ed104 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -327,7 +327,7 @@ func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, erro }) } -// handlePullRequestMerging handle pull request merging, a pull request action should only push 1 commit +// handlePullRequestMerging handle pull request merging, a pull request action should push at least 1 commit func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { if len(updates) == 0 { ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ @@ -335,10 +335,6 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H }) return } - if len(updates) != 1 && !setting.IsProd { - // it shouldn't happen - panic(fmt.Sprintf("Pushing a merged PR (pr:%d) should only push 1 commit, but got %d", opts.PullRequestID, len(updates))) - } pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) if err != nil { From 03ab2fa2711c22d55a4ddbfcc08fbeca701950eb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 11:02:35 +0800 Subject: [PATCH 11/11] Add test for automerge deletion --- routers/private/hook_post_receive_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go index d4401bc6995a2..658557d3cfc03 100644 --- a/routers/private/hook_post_receive_test.go +++ b/routers/private/hook_post_receive_test.go @@ -8,7 +8,10 @@ import ( "code.gitea.io/gitea/models/db" 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" "code.gitea.io/gitea/modules/private" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/services/contexttest" @@ -21,6 +24,14 @@ func TestHandlePullRequestMerging(t *testing.T) { pr, err := issues_model.GetUnmergedPullRequest(db.DefaultContext, 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub) assert.NoError(t, err) assert.NoError(t, pr.LoadBaseRepo(db.DefaultContext)) + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + err = pull_model.ScheduleAutoMerge(db.DefaultContext, user1, pr.ID, repo_model.MergeStyleSquash, "squash merge a pr") + assert.NoError(t, err) + + autoMerge := unittest.AssertExistsAndLoadBean(t, &pull_model.AutoMerge{PullID: pr.ID}) + ctx, resp := contexttest.MockPrivateContext(t, "/") handlePullRequestMerging(ctx, &private.HookOptions{ PullRequestID: pr.ID, @@ -33,5 +44,6 @@ func TestHandlePullRequestMerging(t *testing.T) { assert.NoError(t, err) assert.True(t, pr.HasMerged) assert.EqualValues(t, "01234567", pr.MergedCommitID) - // TODO: test the removal of auto merge + + unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{ID: autoMerge.ID}) }