From ea91028028d4abb6b5ac3bed4df28655eef35e98 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 27 Sep 2026 01:38:42 +0800 Subject: [PATCH] fix: PR merge (#39442) * Revert the behavior introduced by #30805 * Now the PR status is still managed in Gitea's code where the operation is triggerred but not in post-receive hook * Fix #39254 and many more related bugs. * Fix #39124 ``` // MarkAsMerged sets a pull request to merged and closes the corresponding issue // To make sure the pull request is marked as merged correctly, the caller uses multiple-stage operations: // 1. Create a temp repo from base, merge the head into the temp repo, and get the merged commit ID and timestamp, // 2. The merged commit ID and related information are stored into pull request // 3. Push the merged commit to the base repo // 4. Call MarkAsMerged to mark the pull request as merged and do post-processing (notification, close issues, etc) // // If failure occurs in step 1/2/3: the pull request is still open, the base repo is not changed, the doer can start a new merge. // If failure occurs in step 4: the pull request can be marked as merged by the merged commit ID stored in it later. ``` --- cmd/hook.go | 1 - modelmigration/migrations.go | 1 + modelmigration/v28/v355.go | 28 +++ models/issues/pull.go | 12 +- models/issues/pull_list.go | 8 + models/pull/automerge.go | 14 +- modules/git/repo_commit.go | 3 +- modules/git/repo_commit_test.go | 4 +- modules/private/hook.go | 2 - modules/repository/env.go | 16 +- routers/api/v1/repo/pull.go | 12 +- routers/private/hook_post_receive.go | 132 +++++------- routers/private/hook_post_receive_test.go | 49 ----- routers/private/internal_repo.go | 3 + routers/web/repo/pull.go | 6 +- services/automerge/automerge.go | 9 +- services/pull/check.go | 155 +++++++------- services/pull/merge.go | 245 ++++++++++++++-------- services/pull/update.go | 3 +- tests/integration/pull_merge_test.go | 12 +- 20 files changed, 359 insertions(+), 356 deletions(-) create mode 100644 modelmigration/v28/v355.go delete mode 100644 routers/private/hook_post_receive_test.go diff --git a/cmd/hook.go b/cmd/hook.go index b0065d2af2c..0af4c5e04fa 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -364,7 +364,6 @@ Gitea or set your environment appropriately.`, "") GitPushOptions: pushOptions(), PullRequestID: prID, - PushTrigger: repo_module.PushTrigger(os.Getenv(repo_module.EnvPushTrigger)), UserID: pusherID, UserName: os.Getenv(repo_module.EnvPusherName), diff --git a/modelmigration/migrations.go b/modelmigration/migrations.go index 8cdcddd43df..694cf0257c6 100644 --- a/modelmigration/migrations.go +++ b/modelmigration/migrations.go @@ -428,6 +428,7 @@ func prepareMigrationTasks() []*migration { newMigration(352, "Add token columns to deploy_key", v28.AddTokenToDeployKey), newMigration(353, "Add audit event table", v28.AddAuditEventTable), newMigration(354, "Add Actions job queue indexes", v28.AddActionQueueIndexes), + newMigration(355, "Add AutoMerge merged_commit_id column", v28.AddAutoMergeMergedCommitID), } return preparedMigrations } diff --git a/modelmigration/v28/v355.go b/modelmigration/v28/v355.go new file mode 100644 index 00000000000..295fcc9ecef --- /dev/null +++ b/modelmigration/v28/v355.go @@ -0,0 +1,28 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v28 + +import ( + "context" + + "gitea.dev/modelmigration/base" + + "xorm.io/xorm" +) + +type pullAutoMerge struct { + MergedCommitID string `xorm:"VARCHAR(64)"` +} + +func (pullAutoMerge) TableName() string { + return "pull_auto_merge" +} + +func AddAutoMergeMergedCommitID(_ context.Context, x base.EngineMigration) error { + _, err := x.SyncWithOptions(xorm.SyncOptions{ + IgnoreConstrains: true, + IgnoreDropIndices: true, + }, new(pullAutoMerge)) + return err +} diff --git a/models/issues/pull.go b/models/issues/pull.go index 84a9611df6e..333516ae908 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -216,16 +216,10 @@ func (pr *PullRequest) OptionalHeadUserName(ctx context.Context) string { // LoadAttributes loads pull request attributes from database // Note: don't try to get Issue because will end up recursive querying. func (pr *PullRequest) LoadAttributes(ctx context.Context) (err error) { - if pr.HasMerged && pr.Merger == nil { - pr.Merger, err = user_model.GetUserByID(ctx, pr.MergerID) - if user_model.IsErrUserNotExist(err) { - pr.MergerID = user_model.GhostUserID - pr.Merger = user_model.NewGhostUser() - } else if err != nil { - return fmt.Errorf("getUserByID [%d]: %w", pr.MergerID, err) - } + if pr.Merger == nil && pr.MergerID != 0 { + pr.MergerID, pr.Merger, err = user_model.GetPossibleUserByID(ctx, pr.MergerID) + return err } - return nil } diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index b734c81af9c..cdd5cb54eeb 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -166,6 +166,14 @@ func GetPullRequestIDsByCheckStatus(ctx context.Context, status PullRequestStatu Find(&prs) } +func GetInterruptedPullRequestIDs(ctx context.Context) ([]int64, error) { + prs := make([]int64, 0, 10) + return prs, db.GetEngine(ctx).Table("pull_request"). + Where("merged_commit_id <> '' AND has_merged = ? AND merged_unix >= ?", false, timeutil.TimeStampNow()-24*3600). + Cols("pull_request.id"). + Find(&prs) +} + // PullRequests returns all pull requests for a base Repo by the given conditions func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptions) (PullRequestList, int64, error) { if opts.Page <= 0 { diff --git a/models/pull/automerge.go b/models/pull/automerge.go index 3a6514f6cf0..d06d456e30c 100644 --- a/models/pull/automerge.go +++ b/models/pull/automerge.go @@ -23,6 +23,7 @@ type AutoMerge struct { Message string `xorm:"LONGTEXT"` DeleteBranchAfterMerge bool CreatedUnix timeutil.TimeStamp `xorm:"created"` + MergedCommitID string `xorm:"VARCHAR(64)"` } // TableName return database table name for xorm @@ -86,15 +87,6 @@ func GetScheduledMergePullIDsSince(ctx context.Context, since timeutil.TimeStamp return pullIDs, err } -// DeleteScheduledAutoMerge delete a scheduled pull request -func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) error { - exist, scheduledPRM, err := GetScheduledMergeByPullID(ctx, pullID) - if err != nil { - return err - } else if !exist { - return db.ErrNotExist{Resource: "auto_merge", ID: pullID} - } - - _, err = db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{}) - return err +func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) (int64, error) { + return db.GetEngine(ctx).Where("pull_id = ?", pullID).Delete(&AutoMerge{}) } diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 5532044345c..54f5fb76719 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -12,6 +12,7 @@ import ( "strings" "gitea.dev/modules/git/gitcmd" + "gitea.dev/modules/git/gitrepo" "gitea.dev/modules/setting" ) @@ -415,7 +416,7 @@ func (repo *Repository) GetCommitsFromIDs(ctx context.Context, commitIDs []strin } // IsCommitInBranch check if the commit is on the branch -func (repo *Repository) IsCommitInBranch(ctx context.Context, commitID, branch string) (r bool, err error) { +func IsCommitInBranch(ctx context.Context, repo gitrepo.RepositoryFacade, commitID, branch string) (r bool, err error) { stdout, _, err := gitcmd.NewCommand("branch", "--contains"). AddDynamicArguments(commitID, branch). WithRepo(repo). diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index c2174aa891f..3d0f07f0a3c 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -76,11 +76,11 @@ func TestIsCommitInBranch(t *testing.T) { assert.NoError(t, err) defer bareRepo1.Close() - result, err := bareRepo1.IsCommitInBranch(t.Context(), "2839944139e0de9737a044f78b0e4b40d989a9e3", "branch1") + result, err := IsCommitInBranch(t.Context(), bareRepo1, "2839944139e0de9737a044f78b0e4b40d989a9e3", "branch1") assert.NoError(t, err) assert.True(t, result) - result, err = bareRepo1.IsCommitInBranch(t.Context(), "2839944139e0de9737a044f78b0e4b40d989a9e3", "branch2") + result, err = IsCommitInBranch(t.Context(), bareRepo1, "2839944139e0de9737a044f78b0e4b40d989a9e3", "branch2") assert.NoError(t, err) assert.False(t, result) } diff --git a/modules/private/hook.go b/modules/private/hook.go index 85d9dfa37e0..5eedd135a10 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -10,7 +10,6 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/httplib" - "gitea.dev/modules/repository" "gitea.dev/modules/setting" ) @@ -36,7 +35,6 @@ type HookOptions struct { GitPushOptions GitPushOptions PullRequestID int64 - PushTrigger repository.PushTrigger UserID int64 UserName string diff --git a/modules/repository/env.go b/modules/repository/env.go index d2586eaf54d..1cd442a6b43 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -28,18 +28,10 @@ const ( EnvPusherID = "GITEA_PUSHER_ID" EnvPusherExtDoerData = "GITEA_PUSHER_EXT_DOER_DATA" - EnvPRID = "GITEA_PR_ID" - EnvPRIndex = "GITEA_PR_INDEX" // not used by Gitea at the moment, it is for custom git hooks - EnvPushTrigger = "GITEA_PUSH_TRIGGER" - EnvIsInternal = "GITEA_INTERNAL_PUSH" - EnvAppURL = "GITEA_ROOT_URL" -) - -type PushTrigger string - -const ( - PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" - PushTriggerPRUpdateWithBase PushTrigger = "pr-update-with-base" + EnvPRID = "GITEA_PR_ID" + EnvPRIndex = "GITEA_PR_INDEX" // not used by Gitea at the moment, it is for custom git hooks + EnvIsInternal = "GITEA_INTERNAL_PUSH" + EnvAppURL = "GITEA_ROOT_URL" ) // InternalPushingEnvironment returns an os environment to switch off hooks on push diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index d4849d822be..83bb07aabca 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -985,15 +985,7 @@ func MergePullRequest(ctx *context.APIContext) { // handle manually-merged mark if manuallyMerged { if err := pull_service.MergedManually(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { - if pull_service.IsErrInvalidMergeStyle(err) { - ctx.APIError(http.StatusMethodNotAllowed, fmt.Sprintf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do))) - return - } - if strings.Contains(err.Error(), "Wrong commit ID") { - ctx.APIError(http.StatusConflict, err.Error()) - return - } - ctx.APIErrorInternal(err) + ctx.APIErrorAuto(err) return } ctx.Status(http.StatusOK) @@ -1040,7 +1032,7 @@ func MergePullRequest(ctx *context.APIContext) { } } - if err := pull_service.Merge(pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { + if err := pull_service.Merge(pr.ID, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if pull_service.IsErrInvalidMergeStyle(err) { ctx.APIError(http.StatusMethodNotAllowed, fmt.Sprintf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do))) } else if conflictError, ok := err.(pull_service.ErrMergeConflicts); ok { diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 0d15feaaa48..f59aba063dd 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -17,7 +17,6 @@ import ( "gitea.dev/modules/private" repo_module "gitea.dev/modules/repository" "gitea.dev/modules/setting" - "gitea.dev/modules/timeutil" "gitea.dev/modules/util" "gitea.dev/modules/web" "gitea.dev/services/audit" @@ -51,7 +50,7 @@ func hookPostReceiveCollectPushUpdates(opts *private.HookOptions, repo *repo_mod return updates } -func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, updates []*repo_module.PushUpdateOptions) bool { +func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, updates []*repo_module.PushUpdateOptions) { branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) for _, update := range updates { if !update.RefFullName.IsBranch() { @@ -59,8 +58,8 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts } if update.IsDelRef() { if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { - ctx.PrivateInternalErrorf("failed to mark branch %s as deleted: %v", update.RefFullName, err) - return false + log.Error("failed to mark branch %s as deleted: %v", update.RefFullName, err) + continue } } else { branchesToSync = append(branchesToSync, update) @@ -70,13 +69,13 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts } if len(branchesToSync) == 0 { - return true + return } gitRepo, err := git.RepositoryFromRequestContextOrOpen(ctx, repo) if err != nil { - ctx.PrivateInternalErrorf("failed to open repository: %v", err) - return false + log.Error("failed to open git repo: %v", err) + return } branchNames := make([]string, 0, len(branchesToSync)) @@ -85,12 +84,9 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts branchNames = append(branchNames, update.RefFullName.BranchName()) commitIDs = append(commitIDs, update.NewCommitID) } - if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames, commitIDs); err != nil { - ctx.PrivateInternalErrorf("failed to sync branch to DB: %v", err) - return false + log.Error("failed to sync branch to DB: %v", err) } - return true } // HookPostReceive updates services and users @@ -100,74 +96,63 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { setting.PanicInDevOrTesting("wiki hook-post-receive is not supported") return } - if !loadContextDoerPermission(ctx, opts.UserID, opts.UserExtDoerData) { - return - } + // In the post-receive hook, the git repo has received all changes, + // So even if any error happens, nothing can be stopped or undone, so the errors should be skipped repo := ctx.Repo.Repository - // first, collect updates and sync branches - updates := hookPostReceiveCollectPushUpdates(opts, repo) - if !hookPostReceiveSyncDatabaseBranches(ctx, opts, repo, updates) { - return - } + updates := hookPostReceiveCollectPushUpdates(opts, repo) // first, collect updates and sync branches + hookPostReceiveSyncDatabaseBranches(ctx, opts, repo, updates) hookPostReceiveSyncRepoDefaultBranch(ctx, opts, repo) + hookPostReceiveUpdateRepoByOptions(ctx, opts, repo) - // handle pull request merging, a pull request action should push at least 1 commit - if opts.PushTrigger == repo_module.PushTriggerPRMergeToBase { - if !hookPostReceiveHandlePullRequestMerging(ctx, opts, updates) { - return - } - } - - if !hookPostReceiveUpdateRepoByOptions(ctx, opts, repo) { - return - } - - // push async updates + // handle async updates (e.g.: notification, cache management, repo update time, etc.) if err := repo_service.PushUpdates(updates...); err != nil { - ctx.PrivateInternalErrorf("failed to push updates: %v", err) - return + log.Error("failed to push updates: %v", err) } hookPostReceiveRespondWithTrailer(ctx, opts, repo) } -func hookPostReceiveUpdateRepoByOptions(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository) bool { +func hookPostReceiveUpdateRepoByOptions(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository) { isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate) // Handle Push Options - if isPrivate.Has() || isTemplate.Has() { - if !ctx.Repo.Permission.IsAdmin() { - ctx.PrivateUserErrorf(http.StatusNotFound, "permission denied") - return false - } + if !isPrivate.Has() && !isTemplate.Has() { + return + } - // Only honor these options while the repo is still empty (the push-to-create - // case). On a populated repo a bare "git push -o repo.private=..." would - // silently flip visibility, bypassing the audit log, webhooks and notifications. - if !repo.IsEmpty { - return true - } + if !loadContextDoerPermission(ctx, opts.UserID, opts.UserExtDoerData) { + return + } - // The repo is empty and being initialized by this push, so there is no - // dependent state (webhooks, notifications, visibility fan-out) to reconcile - // yet; setting the flags directly is sufficient in this push-to-create case. - if isPrivate.Has() && repo.IsPrivate != isPrivate.Value() { - repo.IsPrivate = isPrivate.Value() - if err := repo_model.UpdateRepositoryColsNoAutoTime(ctx, repo, "is_private"); err != nil { - log.Error("failed to update repo is_private: %v", err) - } else { - audit.RecordAs(ctx, ctx.Doer, audit_model.RepositoryVisibility, repo, "visibility", repo.IsPrivate) - } - } - if isTemplate.Has() && repo.IsTemplate != isTemplate.Value() { - repo.IsTemplate = isTemplate.Value() - if err := repo_model.UpdateRepositoryColsNoAutoTime(ctx, repo, "is_template"); err != nil { - log.Error("failed to update repo is_template: %v", err) - } + if !ctx.Repo.Permission.IsAdmin() { + return + } + + // Only honor these options while the repo is still empty (the push-to-create + // case). On a populated repo a bare "git push -o repo.private=..." would + // silently flip visibility, bypassing the audit log, webhooks and notifications. + if !repo.IsEmpty { + return + } + + // The repo is empty and being initialized by this push, so there is no + // dependent state (webhooks, notifications, visibility fan-out) to reconcile + // yet; setting the flags directly is sufficient in this push-to-create case. + if isPrivate.Has() && repo.IsPrivate != isPrivate.Value() { + repo.IsPrivate = isPrivate.Value() + if err := repo_model.UpdateRepositoryColsNoAutoTime(ctx, repo, "is_private"); err != nil { + log.Error("failed to update repo is_private: %v", err) + } else { + audit.RecordAs(ctx, ctx.Doer, audit_model.RepositoryVisibility, repo, "visibility", repo.IsPrivate) + } + } + if isTemplate.Has() && repo.IsTemplate != isTemplate.Value() { + repo.IsTemplate = isTemplate.Value() + if err := repo_model.UpdateRepositoryColsNoAutoTime(ctx, repo, "is_template"); err != nil { + log.Error("failed to update repo is_template: %v", err) } } - return true } func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository) { @@ -229,29 +214,6 @@ func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts * ctx.JSON(http.StatusOK, private.HookPostReceiveResult{Results: results}) } -// hookPostReceiveHandlePullRequestMerging handle pull request merging, a pull request action should push at least 1 commit -func hookPostReceiveHandlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, updates []*repo_module.PushUpdateOptions) bool { - if len(updates) == 0 { - ctx.PrivateInternalErrorf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID) - return false - } - - pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) - if err != nil { - ctx.PrivateInternalErrorf("failed to get pull request %d: %v", opts.PullRequestID, err) - return false - } - - // FIXME: Maybe we need a `PullRequestStatusMerged` status for PRs that are merged, currently we use the previous status - // here to keep it as before, that maybe PullRequestStatusMergeable - _, err = pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), ctx.Doer, pr.Status) - if err != nil { - ctx.PrivateInternalErrorf("failed to set pr %d to merged: %v", pr.ID, err) - return false - } - return true -} - func hookPostReceiveSyncRepoDefaultBranch(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository) { hasBranch := false for _, refFullName := range opts.RefFullNames { diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go deleted file mode 100644 index a7099aa58df..00000000000 --- a/routers/private/hook_post_receive_test.go +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package private - -import ( - "testing" - - issues_model "gitea.dev/models/issues" - pull_model "gitea.dev/models/pull" - repo_model "gitea.dev/models/repo" - "gitea.dev/models/unittest" - user_model "gitea.dev/models/user" - "gitea.dev/modules/private" - repo_module "gitea.dev/modules/repository" - "gitea.dev/services/contexttest" - - "github.com/stretchr/testify/assert" -) - -func TestHandlePullRequestMerging(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - pr, err := issues_model.GetUnmergedPullRequest(t.Context(), 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub) - assert.NoError(t, err) - assert.NoError(t, pr.LoadBaseRepo(t.Context())) - - user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - err = pull_model.ScheduleAutoMerge(t.Context(), user1, pr.ID, repo_model.MergeStyleSquash, "squash merge a pr", false) - assert.NoError(t, err) - - autoMerge := unittest.AssertExistsAndLoadBean(t, &pull_model.AutoMerge{PullID: pr.ID}) - - ctx, resp := contexttest.MockPrivateContext(t, "/") - ctx.Doer = user2 - hookPostReceiveHandlePullRequestMerging(ctx, &private.HookOptions{ - PullRequestID: pr.ID, - UserID: 2, - }, []*repo_module.PushUpdateOptions{ - {NewCommitID: "01234567"}, - }) - assert.Empty(t, resp.Body.String()) - pr, err = issues_model.GetPullRequestByID(t.Context(), pr.ID) - assert.NoError(t, err) - assert.True(t, pr.HasMerged) - assert.Equal(t, "01234567", pr.MergedCommitID) - - unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{ID: autoMerge.ID}) -} diff --git a/routers/private/internal_repo.go b/routers/private/internal_repo.go index d68c47325b2..b84b10d9077 100644 --- a/routers/private/internal_repo.go +++ b/routers/private/internal_repo.go @@ -45,6 +45,9 @@ func loadRepository(ctx *gitea_context.PrivateContext, ownerName, repoName strin } func loadContextDoerPermission(ctx *gitea_context.PrivateContext, userID int64, extDoerData string) bool { + if ctx.Doer != nil { + return true + } doer, err := user.GetDoerPermissionUser(ctx, userID, extDoerData) if err != nil { ctx.PrivateInternalErrorf("Failed to get user: %d, error: %v", userID, err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index fff8d107032..0605d5210e0 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1097,7 +1097,7 @@ func MergePullRequest(ctx *context.Context) { switch { case pull_service.IsErrInvalidMergeStyle(err): ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option")) - case strings.Contains(err.Error(), "Wrong commit ID"): + case errors.Is(err, util.ErrInvalidArgument): ctx.JSONError(ctx.Tr("repo.pulls.wrong_commit_id")) default: ctx.ServerError("MergedManually", err) @@ -1131,7 +1131,7 @@ func MergePullRequest(ctx *context.Context) { if form.MergeWhenChecksSucceed { // delete all scheduled auto merges - _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) + _, _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) // schedule auto merge scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { @@ -1145,7 +1145,7 @@ func MergePullRequest(ctx *context.Context) { } } - if err := pull_service.Merge(pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { + if err := pull_service.Merge(pr.ID, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if pull_service.IsErrInvalidMergeStyle(err) { ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option")) } else if conflictError, ok := err.(pull_service.ErrMergeConflicts); ok { diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 8c4a8009742..39b4b9efc9f 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -90,10 +90,11 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_ // RemoveScheduledAutoMerge cancels a previously scheduled pull request func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest) error { return db.WithTx(ctx, func(ctx context.Context) error { - if err := pull_model.DeleteScheduledAutoMerge(ctx, pull.ID); err != nil { + if n, err := pull_model.DeleteScheduledAutoMerge(ctx, pull.ID); err != nil { return err + } else if n == 0 { + return nil } - _, err := issues_model.CreateAutoMergeComment(ctx, issues_model.CommentTypePRUnScheduledToAutoMerge, pull, doer) return err }) @@ -132,7 +133,7 @@ func handlePullRequestAutoMerge(ctx context.Context, pr *issues_model.PullReques _ = pr.LoadIssue(ctx) if (pr.Issue != nil && pr.Issue.IsClosed) || pr.HasMerged { // if the PR has been closed or merged, delete the automerge record and skip - err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) + _, err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) if err != nil { return errors.Join(errSkipAutoMerge, err) } @@ -223,7 +224,7 @@ func handlePullRequestAutoMerge(ctx context.Context, pr *issues_model.PullReques // although expectedHeadCommitID is checked before, we should pass it to the Merge function to // make it be checked again in case the head commit id changed after the previous check. - if err := pull_service.Merge(pr, doer, scheduledPRM.MergeStyle, expectedHeadCommitID, scheduledPRM.Message, true); err != nil { + if err := pull_service.Merge(pr.ID, doer, scheduledPRM.MergeStyle, expectedHeadCommitID, scheduledPRM.Message, true); err != nil { if pull_service.IsErrSHADoesNotMatch(err) { return errors.Join(errSkipAutoMerge, err) } diff --git a/services/pull/check.go b/services/pull/check.go index 3dcd6e67f28..424d464d023 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -19,6 +19,7 @@ import ( repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" user_model "gitea.dev/models/user" + "gitea.dev/modules/container" "gitea.dev/modules/git" "gitea.dev/modules/git/gitcmd" "gitea.dev/modules/globallock" @@ -30,7 +31,6 @@ import ( "gitea.dev/modules/timeutil" asymkey_service "gitea.dev/services/asymkey" "gitea.dev/services/automergequeue" - notify_service "gitea.dev/services/notify" ) // prPatchCheckerQueue represents a queue to handle update pull request tests @@ -268,7 +268,7 @@ func checkSigningRequirements(ctx context.Context, pr *issues_model.PullRequest, // markPullRequestAsMergeable checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullRequest) { +func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullRequest) error { // If the status has not been changed to conflict by the conflict checking functions then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable @@ -282,22 +282,22 @@ func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullReques if has { log.Trace("Not updating status for %-v as it is due to be rechecked", pr) - return + return nil } if _, err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { - log.Error("Update[%-v]: %v", pr, err) + return err } // if there is a scheduled merge for this pull request, start the auto merge check (again) - exist, _, err := pull.GetScheduledMergeByPullID(ctx, pr.ID) + hasScheduledMerge, _, err := pull.GetScheduledMergeByPullID(ctx, pr.ID) if err != nil { - log.Error("GetScheduledMergeByPullID[%-v]: %v", pr, err) - return - } else if !exist { - return + return err } - automergequeue.StartAutoMergeCheckByPullHead(ctx, pr) + if hasScheduledMerge { + automergequeue.StartAutoMergeCheckByPullHead(ctx, pr) + } + return nil } // getMergeCommit checks if a pull request has been merged @@ -384,9 +384,9 @@ func getMergerForManuallyMergedPullRequest(ctx context.Context, pr *issues_model return nil, fmt.Errorf("unable to find merger for manually merged pull request: %w", errors.Join(errs...)) } -// manuallyMerged checks if a pull request got manually merged +// manuallyMergedByPushedCommit checks if a pull request got manually merged // When a pull request got manually merged mark the pull request as merged -func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { +func manuallyMergedByPushedCommit(ctx context.Context, pr *issues_model.PullRequest) bool { if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("%-v LoadBaseRepo: %v", pr, err) return false @@ -419,31 +419,35 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } - if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil { - log.Error("%-v setMerged : %v", pr, err) - return false - } else if !merged { - return false + merged, err := MarkAsMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged) + if err != nil { + log.Error("%-v MarkAsMerged : %v", pr, err) } - - notify_service.MergePullRequest(ctx, merger, pr) - - log.Info("manuallyMerged[%-v]: Marked as manually merged into %s/%s by commit id: %s", pr, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) - return true + return merged } // InitializePullRequests checks and tests untested patches of pull requests. func InitializePullRequests(ctx context.Context) { - // If we prefer to delay the checks, then no need to do any check during startup, there should be not much difference - if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 { - return - } - prs, err := issues_model.GetPullRequestIDsByCheckStatus(ctx, issues_model.PullRequestStatusChecking) + prIdSet := container.Set[int64]{} + + prs, err := issues_model.GetInterruptedPullRequestIDs(ctx) if err != nil { - log.Error("Find Checking PRs: %v", err) - return + log.Error("Failed to query interrupted PRs: %v", err) + } else { + prIdSet.AddMultiple(prs...) } - for _, prID := range prs { + + // If we prefer to delay the checks, then no need to do PR check during startup, there should be not much difference + if setting.Repository.PullRequest.DelayCheckForInactiveDays < 0 { + prs, err := issues_model.GetPullRequestIDsByCheckStatus(ctx, issues_model.PullRequestStatusChecking) + if err != nil { + log.Error("Failed to query PRs that need checking: %v", err) + } else { + prIdSet.AddMultiple(prs...) + } + } + + for _, prID := range prIdSet.Values() { select { case <-ctx.Done(): return @@ -453,51 +457,62 @@ func InitializePullRequests(ctx context.Context) { } } -func checkPullRequestMergeable(id int64) { - ctx := graceful.GetManager().HammerContext() - releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(id)) +func restoreInterruptedMerge(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { + hasCommitBeenMerged, err := hasPullRequestCommitBeenMerged(ctx, pr) if err != nil { - log.Error("lock.Lock(): %v", err) - return + return false, err } - defer releaser() - - ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("Test PR[%d] from patch checking queue", id)) - defer finished() - - pr, err := issues_model.GetPullRequestByID(ctx, id) - if err != nil { - log.Error("Unable to GetPullRequestByID[%d] for checkPullRequestMergeable: %v", id, err) - return + if !hasCommitBeenMerged { + return false, nil } - - log.Trace("Testing %-v", pr) - defer func() { - log.Trace("Done testing %-v (status: %s)", pr, pr.Status) - }() - - if pr.HasMerged { - log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return - } - - if manuallyMerged(ctx, pr) { - log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return - } - - if err := checkPullRequestBranchMergeable(ctx, pr); err != nil { - log.Error("checkPullRequestBranchMergeable[%-v]: %v", pr, err) - pr.Status = issues_model.PullRequestStatusError - if err := pr.UpdateCols(ctx, "status"); err != nil { - log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) - } - return - } - markPullRequestAsMergeable(ctx, pr) + return MarkAsMerged(ctx, pr, pr.MergedCommitID, pr.MergedUnix, pr.Merger, pr.Status) } -// CheckPRsForBaseBranch check all pulls with baseBrannch +func checkPullRequestMergeable(prID int64) { + ctx := graceful.GetManager().HammerContext() + + ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("Test PR[%d] from patch checking queue", prID)) + defer finished() + + err := globallock.LockAndDo(ctx, getPullWorkingLockKey(prID), func(ctx context.Context) error { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + + if pr.HasMerged { + log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) + return nil + } + + if ok, err := restoreInterruptedMerge(ctx, pr); err != nil { + return err + } else if ok { + return nil + } + + if manuallyMergedByPushedCommit(ctx, pr) { + log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) + return nil + } + + if err := checkPullRequestBranchMergeable(ctx, pr); err != nil { + log.Error("checkPullRequestBranchMergeable[%-v]: %v", pr, err) + pr.Status = issues_model.PullRequestStatusError + if err := pr.UpdateCols(ctx, "status"); err != nil { + log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) + } + return err + } + + return markPullRequestAsMergeable(ctx, pr) + }) + if err != nil { + log.Error("Unable to checkPullRequestMergeable[%d]: %v", prID, err) + } +} + +// CheckPRsForBaseBranch check all pulls with base branch func CheckPRsForBaseBranch(ctx context.Context, baseRepo *repo_model.Repository, baseBranchName string) error { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, baseRepo.ID, baseBranchName) if err != nil { diff --git a/services/pull/merge.go b/services/pull/merge.go index 7117bfdcdd5..f091ce4629c 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -6,7 +6,6 @@ package pull import ( "context" - "errors" "fmt" "maps" "os" @@ -242,7 +241,7 @@ func addTestPullRequestTaskAfterWebOperation(pr *issues_model.PullRequest, doer // But it's really questionable whether it's worth to do it ahead without waiting for the "push queue" task to run. // TODO: DUPLICATE-PR-TASK: maybe can try to remove this in 1.26 to see if there is any issue. go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, + RepoID: pr.BaseRepoID, Doer: doer, Branch: pr.BaseBranch, IsSync: false, @@ -252,50 +251,104 @@ func addTestPullRequestTaskAfterWebOperation(pr *issues_model.PullRequest, doer }) } -// Merge merges pull request to base repository. -// Caller should check PR is ready to be merged (review and status checks) -func Merge(pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { - ctx := graceful.GetManager().HammerContext() // don't abort the git operation even if the user's request is canceled - - if err := pr.LoadBaseRepo(ctx); err != nil { - log.Error("Unable to load base repo: %v", err) - return fmt.Errorf("unable to load base repo: %w", err) - } else if err := pr.LoadHeadRepo(ctx); err != nil { - log.Error("Unable to load head repo: %v", err) - return fmt.Errorf("unable to load head repo: %w", err) - } - - prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests) - if err != nil { - log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err) - return err - } - prConfig := prUnit.PullRequestsConfig() - - // Check if merge style is correct and allowed - if !prConfig.IsMergeStyleAllowed(mergeStyle) { - return ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} - } - - err = globallock.LockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { - _, err := doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) - return err +func recordMergeIntent(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, commitID string, wasAutoMerged bool) error { + return db.WithTx(ctx, func(ctx context.Context) error { + pr.MergedUnix = timeutil.TimeStampNow() + pr.MergedCommitID = commitID + pr.MergerID = doer.ID + _, err := db.GetEngine(ctx).ID(pr.ID).Cols("merged_unix", "merged_commit_id", "merger_id").Update(pr) + if err != nil { + return err + } + if wasAutoMerged { + hasScheduledMerge, scheduledMerge, err := pull_model.GetScheduledMergeByPullID(ctx, pr.ID) + if err != nil { + return err + } + if hasScheduledMerge { + scheduledMerge.MergedCommitID = commitID + _, err := db.GetEngine(ctx).ID(scheduledMerge.ID).Cols("merged_commit_id").Update(scheduledMerge) + if err != nil { + return err + } + } + } + return nil }) - defer addTestPullRequestTaskAfterWebOperation(pr, doer) // keep the same behavior as old code: always call AddTestPullRequestTask - // TODO: the "merge" operation has finished, there could still be some edge cases: - // * if the post-process hook isn't executed correctly: - // * the commit has been merged into target branch - // * the PR's status is still "open (unmerged)" - // * something wrong happens (e.g.: out of sync?) - // * maybe this is the reason that why the duplicate AddTestPullRequestTask is called in defer func above - if err != nil { - return err - } - // TODO: it is questionable whether it should return error here, the "merge" operation has succeeded - return handleMergePostProcess(ctx, pr.ID, doer, wasAutoMerged) } -func handleMergePostProcess(ctx context.Context, prID int64, doer *user_model.User, wasAutoMerged bool) error { +func hasPullRequestCommitBeenMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { + if pr.MergedCommitID == "" { + return false, nil + } + if err := pr.LoadBaseRepo(ctx); err != nil { + return false, err + } + return git.IsCommitInBranch(ctx, pr.BaseRepo, pr.MergedCommitID, pr.BaseBranch) +} + +// Merge merges pull request to base repository. +// Caller should check PR is ready to be merged (review and status checks) +func Merge(prID int64, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { + ctx := graceful.GetManager().HammerContext() // don't abort the git operation even if the user's request is canceled + + err := globallock.LockAndDo(ctx, getPullWorkingLockKey(prID), func(ctx context.Context) error { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + if err := pr.LoadBaseRepo(ctx); err != nil { + return fmt.Errorf("unable to load base repo: %w", err) + } else if err := pr.LoadHeadRepo(ctx); err != nil { + return fmt.Errorf("unable to load head repo: %w", err) + } else if err := pr.LoadIssue(ctx); err != nil { + return fmt.Errorf("unable to load issue: %w", err) + } + + prConfig := pr.BaseRepo.MustGetUnit(ctx, unit.TypePullRequests).PullRequestsConfig() + + // Check if merge style is correct and allowed + if !prConfig.IsMergeStyleAllowed(mergeStyle) { + return ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} + } + + hasCommitBeenMerged, err := hasPullRequestCommitBeenMerged(ctx, pr) + if err != nil { + return err + } + + if !hasCommitBeenMerged { + _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, message, mergeAndPushOptions{ + expectedHeadCommitID: expectedHeadCommitID, + onMergedIntoTempBase: func(commitID string) error { + return recordMergeIntent(ctx, pr, doer, commitID, wasAutoMerged) + }, + }) + if err != nil { + return err + } + } + + _, err = MarkAsMerged(ctx, pr, pr.MergedCommitID, pr.MergedUnix, doer, pr.Status) + if err != nil { + return err + } + + return err + }) + if err != nil { + return err + } + + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + addTestPullRequestTaskAfterWebOperation(pr, doer) // keep the same behavior as old code: always call AddTestPullRequestTask + return nil +} + +func markAsMergedPostProcess(ctx context.Context, prID int64, doer *user_model.User, wasAutoMerged bool) error { // reload pull request because it has been updated by post receive hook pr, err := issues_model.GetPullRequestByID(ctx, prID) if err != nil { @@ -325,7 +378,7 @@ func handleMergePostProcess(ctx context.Context, prID int64, doer *user_model.Us } func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User) error { - // Resolve cross references + // Resolve cross-references refs, err := pr.ResolveCrossReferences(ctx) if err != nil { log.Error("ResolveCrossReferences: %v", err) @@ -355,10 +408,15 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques return nil } +type mergeAndPushOptions struct { + expectedHeadCommitID string + onMergedIntoTempBase func(commitID string) error +} + // 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, pushTrigger repo_module.PushTrigger) (string, error) { //nolint:unparam // non-error result is never used +func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, message string, opts mergeAndPushOptions) (string, error) { //nolint:unparam // non-error result is never used // Clone base repo. - mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID) + mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, opts.expectedHeadCommitID) if err != nil { return "", err } @@ -389,15 +447,15 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use // OK we should cache our current head and origin/headbranch mergeHeadSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpRepo, "HEAD") if err != nil { - return "", fmt.Errorf("Failed to get full commit id for HEAD: %w", err) + return "", fmt.Errorf("failed to get full commit id for HEAD: %w", err) } mergeBaseSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpRepo, "original_"+tmpRepoBaseBranch) if err != nil { - return "", fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) + return "", fmt.Errorf("failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) } mergeCommitID, err := git.GetFullCommitID(ctx, mergeCtx.tmpRepo, tmpRepoBaseBranch) if err != nil { - return "", fmt.Errorf("Failed to get full commit id for the new merge: %w", err) + return "", fmt.Errorf("failed to get full commit id for the new merge: %w", err) } // Now it's questionable about where this should go - either after or before the push @@ -422,6 +480,12 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use headUser = pr.HeadRepo.Owner } + if opts.onMergedIntoTempBase != nil { + if err := opts.onMergedIntoTempBase(mergeCommitID); err != nil { + return "", err + } + } + mergeCtx.env = repo_module.FullPushingEnvironment( headUser, doer, @@ -430,8 +494,6 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use pr.ID, pr.Index, ) - - mergeCtx.env = append(mergeCtx.env, repo_module.EnvPushTrigger+"="+string(pushTrigger)) pushCmd := gitcmd.NewCommand("push", "origin").AddDynamicArguments(tmpRepoBaseBranch + ":" + git.BranchPrefix + pr.BaseBranch) // Push back to upstream. @@ -646,15 +708,8 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques } // MergedManually mark pr as merged manually -func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) error { - releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(pr.ID)) - if err != nil { - log.Error("lock.Lock(): %v", err) - return fmt.Errorf("lock.Lock: %w", err) - } - defer releaser() - - err = db.WithTx(ctx, func(ctx context.Context) error { +func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeCommitRef string) error { + return globallock.LockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { if err := pr.LoadBaseRepo(ctx); err != nil { return err } @@ -669,49 +724,41 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use return ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged} } - objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) - if len(commitID) != objectFormat.FullLength() { - return errors.New("Wrong commit ID") - } - - commit, err := baseGitRepo.GetCommit(ctx, commitID) + commit, err := baseGitRepo.GetCommit(ctx, mergeCommitRef) if err != nil { if git.IsErrNotExist(err) { - return errors.New("Wrong commit ID") + return util.NewInvalidArgumentErrorf("commit not found in base repository") } return err } - commitID = commit.ID.String() + mergeCommitRef = commit.ID.String() - ok, err := baseGitRepo.IsCommitInBranch(ctx, commitID, pr.BaseBranch) + ok, err := git.IsCommitInBranch(ctx, baseGitRepo, mergeCommitRef, pr.BaseBranch) if err != nil { return err } if !ok { - return errors.New("Wrong commit ID") + return util.NewInvalidArgumentErrorf("commit not found in base branch") } - var merged bool - if merged, err = SetMerged(ctx, pr, commitID, timeutil.TimeStamp(commit.Author.When.Unix()), doer, issues_model.PullRequestStatusManuallyMerged); err != nil { + _, err = MarkAsMerged(ctx, pr, mergeCommitRef, timeutil.TimeStamp(commit.Author.When.Unix()), doer, issues_model.PullRequestStatusManuallyMerged) + if err != nil { return err - } else if !merged { - return errors.New("SetMerged failed") } return nil }) - releaser() - if err != nil { - return err - } - - notify_service.MergePullRequest(ctx, doer, pr) - log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commitID) - - return handleCloseCrossReferences(ctx, pr, doer) } -// SetMerged sets a pull request to merged and closes the corresponding issue -func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID string, mergedTimeStamp timeutil.TimeStamp, merger *user_model.User, mergeStatus issues_model.PullRequestStatus) (bool, error) { +// MarkAsMerged sets a pull request to merged and closes the corresponding issue +// To make sure the pull request is marked as merged correctly, the caller uses multiple-stage operations: +// 1. Create a temp repo from base, merge the head into the temp repo, and get the merged commit ID and timestamp, +// 2. The merged commit ID and related information are stored into pull request +// 3. Push the merged commit to the base repo +// 4. Call MarkAsMerged to mark the pull request as merged and do post-processing (notification, close issues, etc) +// +// If failure occurs in step 1/2/3: the pull request is still open, the base repo is not changed, the doer can start a new merge. +// If failure occurs in step 4: the pull request can be marked as merged by the merged commit ID stored in it later. +func MarkAsMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID string, mergedTimeStamp timeutil.TimeStamp, merger *user_model.User, mergeStatus issues_model.PullRequestStatus) (bool, error) { if pr.HasMerged { return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) } @@ -729,7 +776,8 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index) } - return db.WithTx2(ctx, func(ctx context.Context) (bool, error) { + wasAutoMerged := false + ok, err := db.WithTx2(ctx, func(ctx context.Context) (bool, error) { pr.Issue = nil if err := pr.LoadIssue(ctx); err != nil { return false, err @@ -743,9 +791,16 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, err } - // Removing an auto merge pull and ignore if not exist - if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { - return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + // Handle the scheduled auto merge + _, scheduledAutoMerge, err := pull_model.GetScheduledMergeByPullID(ctx, pr.ID) + if err != nil { + return false, err + } + if scheduledAutoMerge != nil { + wasAutoMerged = scheduledAutoMerge.MergedCommitID == pr.MergedCommitID + if _, err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil { + return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + } } // Set issue as closed @@ -765,6 +820,18 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return true, nil }) + if err != nil { + return false, err + } else if !ok { + return false, err + } + + err = markAsMergedPostProcess(ctx, pr.ID, merger, wasAutoMerged) + if err != nil { + // the merge has succeeded, so any other errors are not critical (the merge can't be undone), just log them + log.Error("markAsMergedPostProcess: %v", err) + } + return true, nil } func ShouldDeleteBranchAfterMerge(ctx context.Context, userOption *bool, repo *repo_model.Repository, pr *issues_model.PullRequest) (bool, error) { diff --git a/services/pull/update.go b/services/pull/update.go index e4bc5327ad7..2af773d3874 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -18,7 +18,6 @@ import ( "gitea.dev/modules/globallock" "gitea.dev/modules/graceful" "gitea.dev/modules/log" - "gitea.dev/modules/repository" ) // Update updates pull request with base branch. @@ -88,7 +87,7 @@ func Update(pr *issues_model.PullRequest, doer *user_model.User, message string, BaseBranch: pr.HeadBranch, } - _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) + _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, message, mergeAndPushOptions{}) // TODO: the "update" (merge target branch to PR head branch) operation has finished, there could still be some edge cases: // * the database was already out of sync: the target branch was already in head branch: // * so no post-receive hook is really executed, no PR status update diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index c4c99f2e6bd..e6e3f8f08c5 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -378,11 +378,11 @@ func TestCantMergeConflict(t *testing.T) { BaseBranch: "base", }) - err := pull_service.Merge(pr, user1, repo_model.MergeStyleMerge, "", "CONFLICT", false) + err := pull_service.Merge(pr.ID, user1, repo_model.MergeStyleMerge, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, pull_service.IsErrMergeConflicts(err), "Merge error is not a conflict error") - err = pull_service.Merge(pr, user1, repo_model.MergeStyleRebase, "", "CONFLICT", false) + err = pull_service.Merge(pr.ID, user1, repo_model.MergeStyleRebase, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, pull_service.IsErrRebaseConflicts(err), "Merge error is not a conflict error") }) @@ -473,7 +473,7 @@ func TestCantMergeUnrelated(t *testing.T) { BaseBranch: "base", }) - err = pull_service.Merge(pr, user1, repo_model.MergeStyleMerge, "", "UNRELATED", false) + err = pull_service.Merge(pr.ID, user1, repo_model.MergeStyleMerge, "", "UNRELATED", false) assert.Error(t, err, "Merge should return an error due to unrelated") assert.True(t, pull_service.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error") }) @@ -509,7 +509,7 @@ func TestFastForwardOnlyMerge(t *testing.T) { BaseBranch: "master", }) - err := pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false) + err := pull_service.Merge(pr.ID, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false) assert.NoError(t, err) }) } @@ -596,7 +596,7 @@ func TestFastForwardOnlyMergeWithRequiredSignedCommits(t *testing.T) { pb.RequireSignedCommits = false require.NoError(t, git_model.UpdateProtectBranch(t.Context(), repo1, pb, git_model.WhitelistOptions{})) - require.NoError(t, pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false)) + require.NoError(t, pull_service.Merge(pr.ID, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false)) }) } @@ -631,7 +631,7 @@ func TestCantFastForwardOnlyMergeDiverging(t *testing.T) { BaseBranch: "master", }) - err := pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "DIVERGING", false) + err := pull_service.Merge(pr.ID, user1, repo_model.MergeStyleFastForwardOnly, "", "DIVERGING", false) assert.Error(t, err, "Merge should return an error due to being for a diverging branch") assert.True(t, pull_service.IsErrMergeDivergingFastForwardOnly(err), "Merge error is not a diverging fast-forward-only error") })