Move database operations of merging a pull request to post receive hook and add a transaction (#30805)
Merging PR may fail because of various problems. The pull request may have a dirty state because there is no transaction when merging a pull request. ref https://github.com/go-gitea/gitea/pull/25741#issuecomment-2074126393 This PR moves all database update operations to post-receive handler for merging a pull request and having a database transaction. That means if database operations fail, then the git merging will fail, the git client will get a fail result. There are already many tests for pull request merging, so we don't need to add a new one. --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
parent
6ad77125ca
commit
ebf0c96940
8 changed files with 150 additions and 19 deletions
|
@ -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,17 +177,15 @@ 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)
|
||||
_, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase)
|
||||
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)
|
||||
// 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 {
|
||||
|
@ -245,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) (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 {
|
||||
|
@ -318,11 +309,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
|
|||
pr.BaseRepo.Name,
|
||||
pr.ID,
|
||||
)
|
||||
|
||||
mergeCtx.env = append(mergeCtx.env, repo_module.EnvPushTrigger+"="+string(pushTrigger))
|
||||
pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch)
|
||||
|
||||
// Push back to upstream.
|
||||
// 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{
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue