Move database operations of merging a pull request to post receive hook and add a transaction

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)

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

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

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"
@@ -158,6 +162,14 @@ 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)
+	if ctx.Written() {
+		return
+	}
+
 	isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate)
 	isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate)
 	// Handle Push Options
@@ -172,13 +184,16 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			wasEmpty = repo.IsEmpty
 		}
 
-		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 {
@@ -307,3 +322,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
+		}
+	}
+}

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,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, 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)
+	// 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, isMerge bool) (string, error) {
 	// Clone base repo.
 	mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID)
 	if err != nil {
@@ -318,11 +309,16 @@ 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.
-	// 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{

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, "", "")