Use for a repo action one database transaction (#19576)

... more context

(part of #9307)

6 files changed, 135 insertions, 113 deletions

diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go
index 876cb7c205..6b17c017fc 100644
--- a/services/asymkey/sign.go
+++ b/services/asymkey/sign.go
@@ -317,7 +317,7 @@ Loop:
 			if protectedBranch == nil {
 				return false, "", nil, &ErrWontSign{approved}
 			}
-			if protectedBranch.GetGrantedApprovalsCount(pr) < 1 {
+			if protectedBranch.GetGrantedApprovalsCount(ctx, pr) < 1 {
 				return false, "", nil, &ErrWontSign{approved}
 			}
 		case baseSigned:
diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go
index f40e99a044..5c3adc1cd3 100644
--- a/services/forms/repo_form.go
+++ b/services/forms/repo_form.go
@@ -6,6 +6,7 @@
 package forms
 
 import (
+	stdContext "context"
 	"net/http"
 	"net/url"
 	"strings"
@@ -601,7 +602,7 @@ func (f *MergePullRequestForm) Validate(req *http.Request, errs binding.Errors)
 }
 
 // SetDefaults if not provided for mergestyle and commit message
-func (f *MergePullRequestForm) SetDefaults(pr *models.PullRequest) (err error) {
+func (f *MergePullRequestForm) SetDefaults(ctx stdContext.Context, pr *models.PullRequest) (err error) {
 	if f.Do == "" {
 		f.Do = "merge"
 	}
@@ -610,9 +611,9 @@ func (f *MergePullRequestForm) SetDefaults(pr *models.PullRequest) (err error) {
 	if len(f.MergeTitleField) == 0 {
 		switch f.Do {
 		case "merge", "rebase-merge":
-			f.MergeTitleField, err = pr.GetDefaultMergeMessage()
+			f.MergeTitleField, err = pr.GetDefaultMergeMessage(ctx)
 		case "squash":
-			f.MergeTitleField, err = pr.GetDefaultSquashMessage()
+			f.MergeTitleField, err = pr.GetDefaultSquashMessage(ctx)
 		}
 	}
 
diff --git a/services/issue/status.go b/services/issue/status.go
index 1af4508b09..d2b4fc303e 100644
--- a/services/issue/status.go
+++ b/services/issue/status.go
@@ -5,6 +5,8 @@
 package issue
 
 import (
+	"context"
+
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/models/db"
 	user_model "code.gitea.io/gitea/models/user"
@@ -14,10 +16,16 @@ import (
 
 // ChangeStatus changes issue status to open or closed.
 func ChangeStatus(issue *models.Issue, doer *user_model.User, closed bool) error {
-	comment, err := models.ChangeIssueStatus(issue, doer, closed)
+	return changeStatusCtx(db.DefaultContext, issue, doer, closed)
+}
+
+// changeStatusCtx changes issue status to open or closed.
+// TODO: if context is not db.DefaultContext we get a deadlock!!!
+func changeStatusCtx(ctx context.Context, issue *models.Issue, doer *user_model.User, closed bool) error {
+	comment, err := models.ChangeIssueStatus(ctx, issue, doer, closed)
 	if err != nil {
 		if models.IsErrDependenciesLeft(err) && closed {
-			if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil {
+			if err := models.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
 				log.Error("Unable to stop stopwatch for issue[%d]#%d: %v", issue.ID, issue.Index, err)
 			}
 		}
@@ -25,7 +33,7 @@ func ChangeStatus(issue *models.Issue, doer *user_model.User, closed bool) error
 	}
 
 	if closed {
-		if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil {
+		if err := models.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
 			return err
 		}
 	}
diff --git a/services/pull/check.go b/services/pull/check.go
index eb3b17d555..64eb93104e 100644
--- a/services/pull/check.go
+++ b/services/pull/check.go
@@ -14,6 +14,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -59,70 +60,72 @@ func AddToTaskQueue(pr *models.PullRequest) {
 }
 
 // CheckPullMergable check if the pull mergable based on all conditions (branch protection, merge options, ...)
-func CheckPullMergable(ctx context.Context, doer *user_model.User, perm *models.Permission, pr *models.PullRequest, manuallMerge, force bool) error {
-	if pr.HasMerged {
-		return ErrHasMerged
-	}
+func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *models.Permission, pr *models.PullRequest, manuallMerge, force bool) error {
+	return db.WithTx(func(ctx context.Context) error {
+		if pr.HasMerged {
+			return ErrHasMerged
+		}
 
-	if err := pr.LoadIssue(); err != nil {
-		return err
-	} else if pr.Issue.IsClosed {
-		return ErrIsClosed
-	}
+		if err := pr.LoadIssueCtx(ctx); err != nil {
+			return err
+		} else if pr.Issue.IsClosed {
+			return ErrIsClosed
+		}
 
-	if allowedMerge, err := IsUserAllowedToMerge(pr, *perm, doer); err != nil {
-		return err
-	} else if !allowedMerge {
-		return ErrUserNotAllowedToMerge
-	}
+		if allowedMerge, err := IsUserAllowedToMerge(ctx, pr, *perm, doer); err != nil {
+			return err
+		} else if !allowedMerge {
+			return ErrUserNotAllowedToMerge
+		}
 
-	if manuallMerge {
-		// don't check rules to "auto merge", doer is going to mark this pull as merged manually
-		return nil
-	}
+		if manuallMerge {
+			// don't check rules to "auto merge", doer is going to mark this pull as merged manually
+			return nil
+		}
 
-	if pr.IsWorkInProgress() {
-		return ErrIsWorkInProgress
-	}
+		if pr.IsWorkInProgress() {
+			return ErrIsWorkInProgress
+		}
 
-	if !pr.CanAutoMerge() {
-		return ErrNotMergableState
-	}
+		if !pr.CanAutoMerge() {
+			return ErrNotMergableState
+		}
 
-	if pr.IsChecking() {
-		return ErrIsChecking
-	}
+		if pr.IsChecking() {
+			return ErrIsChecking
+		}
 
-	if err := CheckPullBranchProtections(ctx, pr, false); err != nil {
-		if models.IsErrDisallowedToMerge(err) {
-			if force {
-				if isRepoAdmin, err2 := models.IsUserRepoAdmin(pr.BaseRepo, doer); err2 != nil {
-					return err2
-				} else if !isRepoAdmin {
-					return err
+		if err := CheckPullBranchProtections(ctx, pr, false); err != nil {
+			if models.IsErrDisallowedToMerge(err) {
+				if force {
+					if isRepoAdmin, err2 := models.IsUserRepoAdminCtx(ctx, pr.BaseRepo, doer); err2 != nil {
+						return err2
+					} else if !isRepoAdmin {
+						return err
+					}
 				}
+			} else {
+				return err
 			}
-		} else {
-			return err
 		}
-	}
 
-	if _, err := isSignedIfRequired(ctx, pr, doer); err != nil {
-		return err
-	}
+		if _, err := isSignedIfRequired(ctx, pr, doer); err != nil {
+			return err
+		}
 
-	if noDeps, err := models.IssueNoDependenciesLeft(pr.Issue); err != nil {
-		return err
-	} else if !noDeps {
-		return ErrDependenciesLeft
-	}
+		if noDeps, err := models.IssueNoDependenciesLeft(ctx, pr.Issue); err != nil {
+			return err
+		} else if !noDeps {
+			return ErrDependenciesLeft
+		}
 
-	return nil
+		return nil
+	}, stdCtx)
 }
 
 // isSignedIfRequired check if merge will be signed if required
 func isSignedIfRequired(ctx context.Context, pr *models.PullRequest, doer *user_model.User) (bool, error) {
-	if err := pr.LoadProtectedBranch(); err != nil {
+	if err := pr.LoadProtectedBranchCtx(ctx); err != nil {
 		return false, err
 	}
 
@@ -266,7 +269,7 @@ func manuallyMerged(ctx context.Context, pr *models.PullRequest) bool {
 		pr.Merger = merger
 		pr.MergerID = merger.ID
 
-		if merged, err := pr.SetMerged(); err != nil {
+		if merged, err := pr.SetMerged(ctx); err != nil {
 			log.Error("PullRequest[%d].setMerged : %v", pr.ID, err)
 			return false
 		} else if !merged {
diff --git a/services/pull/merge.go b/services/pull/merge.go
index 330dffd5c7..ad93b9779f 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -17,6 +17,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -34,11 +35,11 @@ import (
 // Merge merges pull request to base repository.
 // Caller should check PR is ready to be merged (review and status checks)
 // FIXME: add repoWorkingPull make sure two merges does not happen at same time.
-func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (err error) {
-	if err = pr.LoadHeadRepoCtx(ctx); err != nil {
+func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) error {
+	if err := pr.LoadHeadRepo(); err != nil {
 		log.Error("LoadHeadRepo: %v", err)
 		return fmt.Errorf("LoadHeadRepo: %v", err)
-	} else if err = pr.LoadBaseRepoCtx(ctx); err != nil {
+	} else if err := pr.LoadBaseRepo(); err != nil {
 		log.Error("LoadBaseRepo: %v", err)
 		return fmt.Errorf("LoadBaseRepo: %v", err)
 	}
@@ -59,7 +60,9 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b
 		go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
 	}()
 
-	pr.MergedCommitID, err = rawMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message)
+	// TODO: make it able to do this in a database session
+	mergeCtx := context.Background()
+	pr.MergedCommitID, err = rawMerge(mergeCtx, pr, doer, mergeStyle, expectedHeadCommitID, message)
 	if err != nil {
 		return err
 	}
@@ -68,18 +71,18 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b
 	pr.Merger = doer
 	pr.MergerID = doer.ID
 
-	if _, err := pr.SetMerged(); err != nil {
+	if _, err := pr.SetMerged(db.DefaultContext); err != nil {
 		log.Error("setMerged [%d]: %v", pr.ID, err)
 	}
 
-	if err := pr.LoadIssue(); err != nil {
+	if err := pr.LoadIssueCtx(db.DefaultContext); err != nil {
 		log.Error("loadIssue [%d]: %v", pr.ID, err)
 	}
 
-	if err := pr.Issue.LoadRepo(ctx); err != nil {
+	if err := pr.Issue.LoadRepo(db.DefaultContext); err != nil {
 		log.Error("loadRepo for issue [%d]: %v", pr.ID, err)
 	}
-	if err := pr.Issue.Repo.GetOwner(ctx); err != nil {
+	if err := pr.Issue.Repo.GetOwner(db.DefaultContext); err != nil {
 		log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err)
 	}
 
@@ -89,17 +92,17 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b
 	cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true))
 
 	// Resolve cross references
-	refs, err := pr.ResolveCrossReferences()
+	refs, err := pr.ResolveCrossReferences(db.DefaultContext)
 	if err != nil {
 		log.Error("ResolveCrossReferences: %v", err)
 		return nil
 	}
 
 	for _, ref := range refs {
-		if err = ref.LoadIssue(); err != nil {
+		if err = ref.LoadIssueCtx(db.DefaultContext); err != nil {
 			return err
 		}
-		if err = ref.Issue.LoadRepo(ctx); err != nil {
+		if err = ref.Issue.LoadRepo(db.DefaultContext); err != nil {
 			return err
 		}
 		close := ref.RefAction == references.XRefActionCloses
@@ -112,7 +115,6 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b
 			}
 		}
 	}
-
 	return nil
 }
 
@@ -504,6 +506,8 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User
 	}
 
 	// 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
 	if err := pushCmd.Run(&git.RunOpts{
 		Env:    env,
 		Dir:    tmpBasePath,
@@ -645,17 +649,17 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string) (
 }
 
 // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
-func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *user_model.User) (bool, error) {
+func IsUserAllowedToMerge(ctx context.Context, pr *models.PullRequest, p models.Permission, user *user_model.User) (bool, error) {
 	if user == nil {
 		return false, nil
 	}
 
-	err := pr.LoadProtectedBranch()
+	err := pr.LoadProtectedBranchCtx(ctx)
 	if err != nil {
 		return false, err
 	}
 
-	if (p.CanWrite(unit.TypeCode) && pr.ProtectedBranch == nil) || (pr.ProtectedBranch != nil && models.IsUserMergeWhitelisted(pr.ProtectedBranch, user.ID, p)) {
+	if (p.CanWrite(unit.TypeCode) && pr.ProtectedBranch == nil) || (pr.ProtectedBranch != nil && models.IsUserMergeWhitelisted(ctx, pr.ProtectedBranch, user.ID, p)) {
 		return true, nil
 	}
 
@@ -668,7 +672,7 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski
 		return fmt.Errorf("LoadBaseRepo: %v", err)
 	}
 
-	if err = pr.LoadProtectedBranch(); err != nil {
+	if err = pr.LoadProtectedBranchCtx(ctx); err != nil {
 		return fmt.Errorf("LoadProtectedBranch: %v", err)
 	}
 	if pr.ProtectedBranch == nil {
@@ -685,17 +689,17 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski
 		}
 	}
 
-	if !pr.ProtectedBranch.HasEnoughApprovals(pr) {
+	if !pr.ProtectedBranch.HasEnoughApprovals(ctx, pr) {
 		return models.ErrDisallowedToMerge{
 			Reason: "Does not have enough approvals",
 		}
 	}
-	if pr.ProtectedBranch.MergeBlockedByRejectedReview(pr) {
+	if pr.ProtectedBranch.MergeBlockedByRejectedReview(ctx, pr) {
 		return models.ErrDisallowedToMerge{
 			Reason: "There are requested changes",
 		}
 	}
-	if pr.ProtectedBranch.MergeBlockedByOfficialReviewRequests(pr) {
+	if pr.ProtectedBranch.MergeBlockedByOfficialReviewRequests(ctx, pr) {
 		return models.ErrDisallowedToMerge{
 			Reason: "There are official review requests",
 		}
@@ -721,52 +725,58 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski
 }
 
 // MergedManually mark pr as merged manually
-func MergedManually(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) (err error) {
-	prUnit, err := pr.BaseRepo.GetUnit(unit.TypePullRequests)
-	if err != nil {
-		return
-	}
-	prConfig := prUnit.PullRequestsConfig()
-
-	// Check if merge style is correct and allowed
-	if !prConfig.IsMergeStyleAllowed(repo_model.MergeStyleManuallyMerged) {
-		return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged}
-	}
+func MergedManually(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) error {
+	if err := db.WithTx(func(ctx context.Context) error {
+		prUnit, err := pr.BaseRepo.GetUnitCtx(ctx, unit.TypePullRequests)
+		if err != nil {
+			return err
+		}
+		prConfig := prUnit.PullRequestsConfig()
 
-	if len(commitID) < 40 {
-		return fmt.Errorf("Wrong commit ID")
-	}
+		// Check if merge style is correct and allowed
+		if !prConfig.IsMergeStyleAllowed(repo_model.MergeStyleManuallyMerged) {
+			return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged}
+		}
 
-	commit, err := baseGitRepo.GetCommit(commitID)
-	if err != nil {
-		if git.IsErrNotExist(err) {
+		if len(commitID) < 40 {
 			return fmt.Errorf("Wrong commit ID")
 		}
-		return
-	}
 
-	ok, err := baseGitRepo.IsCommitInBranch(commitID, pr.BaseBranch)
-	if err != nil {
-		return
-	}
-	if !ok {
-		return fmt.Errorf("Wrong commit ID")
-	}
+		commit, err := baseGitRepo.GetCommit(commitID)
+		if err != nil {
+			if git.IsErrNotExist(err) {
+				return fmt.Errorf("Wrong commit ID")
+			}
+			return err
+		}
+		commitID = commit.ID.String()
 
-	pr.MergedCommitID = commitID
-	pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
-	pr.Status = models.PullRequestStatusManuallyMerged
-	pr.Merger = doer
-	pr.MergerID = doer.ID
+		ok, err := baseGitRepo.IsCommitInBranch(commitID, pr.BaseBranch)
+		if err != nil {
+			return err
+		}
+		if !ok {
+			return fmt.Errorf("Wrong commit ID")
+		}
 
-	merged := false
-	if merged, err = pr.SetMerged(); err != nil {
-		return
-	} else if !merged {
-		return fmt.Errorf("SetMerged failed")
+		pr.MergedCommitID = commitID
+		pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
+		pr.Status = models.PullRequestStatusManuallyMerged
+		pr.Merger = doer
+		pr.MergerID = doer.ID
+
+		merged := false
+		if merged, err = pr.SetMerged(ctx); err != nil {
+			return err
+		} else if !merged {
+			return fmt.Errorf("SetMerged failed")
+		}
+		return nil
+	}); err != nil {
+		return err
 	}
 
 	notification.NotifyMergePullRequest(pr, doer)
-	log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
+	log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commitID)
 	return nil
 }
diff --git a/services/pull/update.go b/services/pull/update.go
index 2d845e8504..08ff1cedfc 100644
--- a/services/pull/update.go
+++ b/services/pull/update.go
@@ -116,13 +116,13 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *models.PullRequest, user *
 		return false, false, err
 	}
 
-	mergeAllowed, err = IsUserAllowedToMerge(pr, headRepoPerm, user)
+	mergeAllowed, err = IsUserAllowedToMerge(ctx, pr, headRepoPerm, user)
 	if err != nil {
 		return false, false, err
 	}
 
 	if pull.AllowMaintainerEdit {
-		mergeAllowedMaintainer, err := IsUserAllowedToMerge(pr, baseRepoPerm, user)
+		mergeAllowedMaintainer, err := IsUserAllowedToMerge(ctx, pr, baseRepoPerm, user)
 		if err != nil {
 			return false, false, err
 		}