From 90982bffa5fe472d528577409cb77f85a745d1a0 Mon Sep 17 00:00:00 2001
From: Jason Song <i@wolfogre.com>
Date: Tue, 21 Feb 2023 23:42:22 +0800
Subject: [PATCH] Add force_merge to merge request and fix checking mergable
 (#23010) (#23032)

Backport #23010.

Fix #23000.

The bug was introduced in #22633, and it seems that it has been noticed:
https://github.com/go-gitea/gitea/pull/22633#discussion_r1095395359 .

However, #22633 did nothing wrong, the logic should be "check if they is
admin only when `force` is true".

So we should provide the `ForceMerge` when merging from UI.

After this, an admin can also send a normal merge request with
`ForceMerge` false. So it fixes a potential bug: if the admin doesn't
want to do a force merge, they just see the green "Merge" button and
click it. At the same time, the status of the PR changed, and it
shouldn't be merged now, so the admin could send an unexpected force
merge.

In addition, I updated `ForceMerge *bool` to `ForceMerge bool`, I don't
see the reason to use a pointer.

And fixed the logic of CheckPullMergable to handle auto merge and force
merge correctly.
---
 routers/api/v1/repo/pull.go                   | 15 ++++++--
 routers/web/repo/pull.go                      | 16 ++++++--
 services/automerge/automerge.go               |  2 +-
 services/forms/repo_form.go                   |  2 +-
 services/pull/check.go                        | 37 ++++++++++++++-----
 .../js/components/PullRequestMergeForm.vue    |  7 +++-
 6 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index a687eabfba..4f2edc2f2a 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -767,11 +767,18 @@ func MergePullRequest(ctx *context.APIContext) {
 		}
 	}
 
-	manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
-	force := form.ForceMerge != nil && *form.ForceMerge
+	manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
+
+	mergeCheckType := pull_service.MergeCheckTypeGeneral
+	if form.MergeWhenChecksSucceed {
+		mergeCheckType = pull_service.MergeCheckTypeAuto
+	}
+	if manuallyMerged {
+		mergeCheckType = pull_service.MergeCheckTypeManually
+	}
 
 	// start with merging by checking
-	if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
+	if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil {
 		if errors.Is(err, pull_service.ErrIsClosed) {
 			ctx.NotFound()
 		} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
@@ -793,7 +800,7 @@ func MergePullRequest(ctx *context.APIContext) {
 	}
 
 	// handle manually-merged mark
-	if manuallMerge {
+	if manuallyMerged {
 		if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
 			if models.IsErrInvalidMergeStyle(err) {
 				ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index 387853677c..b2566eaaab 100644
--- a/routers/web/repo/pull.go
+++ b/routers/web/repo/pull.go
@@ -912,11 +912,19 @@ func MergePullRequest(ctx *context.Context) {
 	pr := issue.PullRequest
 	pr.Issue = issue
 	pr.Issue.Repo = ctx.Repo.Repository
-	manualMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
-	forceMerge := form.ForceMerge != nil && *form.ForceMerge
+
+	manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
+
+	mergeCheckType := pull_service.MergeCheckTypeGeneral
+	if form.MergeWhenChecksSucceed {
+		mergeCheckType = pull_service.MergeCheckTypeAuto
+	}
+	if manuallyMerged {
+		mergeCheckType = pull_service.MergeCheckTypeManually
+	}
 
 	// start with merging by checking
-	if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manualMerge, forceMerge); err != nil {
+	if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil {
 		switch {
 		case errors.Is(err, pull_service.ErrIsClosed):
 			if issue.IsPull {
@@ -948,7 +956,7 @@ func MergePullRequest(ctx *context.Context) {
 	}
 
 	// handle manually-merged mark
-	if manualMerge {
+	if manuallyMerged {
 		if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
 			switch {
 
diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go
index 49bd1d0c02..8fec2239a2 100644
--- a/services/automerge/automerge.go
+++ b/services/automerge/automerge.go
@@ -231,7 +231,7 @@ func handlePull(pullID int64, sha string) {
 		return
 	}
 
-	if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, false, false); err != nil {
+	if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false); err != nil {
 		if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) {
 			log.Info("%-v was scheduled to automerge by an unauthorized user", pr)
 			return
diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go
index c1e9cb3197..1e54e7cae3 100644
--- a/services/forms/repo_form.go
+++ b/services/forms/repo_form.go
@@ -597,7 +597,7 @@ type MergePullRequestForm struct {
 	MergeMessageField      string
 	MergeCommitID          string // only used for manually-merged
 	HeadCommitID           string `json:"head_commit_id,omitempty"`
-	ForceMerge             *bool  `json:"force_merge,omitempty"`
+	ForceMerge             bool   `json:"force_merge,omitempty"`
 	MergeWhenChecksSucceed bool   `json:"merge_when_checks_succeed,omitempty"`
 	DeleteBranchAfterMerge bool   `json:"delete_branch_after_merge,omitempty"`
 }
diff --git a/services/pull/check.go b/services/pull/check.go
index d828ee8cd7..f7e9e4a269 100644
--- a/services/pull/check.go
+++ b/services/pull/check.go
@@ -59,8 +59,16 @@ func AddToTaskQueue(pr *issues_model.PullRequest) {
 	}
 }
 
+type MergeCheckType int
+
+const (
+	MergeCheckTypeGeneral  MergeCheckType = iota // general merge checks for "merge", "rebase", "squash", etc
+	MergeCheckTypeManually                       // Manually Merged button (mark a PR as merged manually)
+	MergeCheckTypeAuto                           // Auto Merge (Scheduled Merge) After Checks Succeed
+)
+
 // CheckPullMergable check if the pull mergable based on all conditions (branch protection, merge options, ...)
-func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, manuallMerge, force bool) error {
+func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool) error {
 	return db.WithTx(func(ctx context.Context) error {
 		if pr.HasMerged {
 			return ErrHasMerged
@@ -80,8 +88,8 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
 			return ErrUserNotAllowedToMerge
 		}
 
-		if manuallMerge {
-			// don't check rules to "auto merge", doer is going to mark this pull as merged manually
+		if mergeCheckType == MergeCheckTypeManually {
+			// if doer is doing "manually merge" (mark as merged manually), do not check anything
 			return nil
 		}
 
@@ -103,14 +111,25 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
 				return err
 			}
 
-			if !force {
-				return err
+			// Now the branch protection check failed, check whether the failure could be skipped (skip by setting err = nil)
+
+			// * when doing Auto Merge (Scheduled Merge After Checks Succeed), skip the branch protection check
+			if mergeCheckType == MergeCheckTypeAuto {
+				err = nil
 			}
 
-			if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil {
-				log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, err2)
-				return err2
-			} else if !isRepoAdmin {
+			// * if the doer is admin, they could skip the branch protection check
+			if adminSkipProtectionCheck {
+				if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
+					log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
+					return errCheckAdmin
+				} else if isRepoAdmin {
+					err = nil // repo admin can skip the check, so clear the error
+				}
+			}
+
+			// If there is still a branch protection check error, return it
+			if err != nil {
 				return err
 			}
 		}
diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue
index 61c95477e7..7295b16131 100644
--- a/web_src/js/components/PullRequestMergeForm.vue
+++ b/web_src/js/components/PullRequestMergeForm.vue
@@ -18,6 +18,7 @@
         <input type="hidden" name="_csrf" :value="csrfToken">
         <input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
         <input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
+        <input type="hidden" name="force_merge" v-model="forceMerge">
 
         <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
           <div class="field">
@@ -127,6 +128,7 @@ export default {
       textDoMerge: '',
       mergeTitleFieldText: '',
       mergeMessageFieldText: '',
+      hideAutoMerge: false,
     },
     mergeStyleAllowedCount: 0,
 
@@ -138,7 +140,10 @@ export default {
     mergeButtonStyleClass() {
       if (this.mergeForm.allOverridableChecksOk) return 'green';
       return this.autoMergeWhenSucceed ? 'blue' : 'red';
-    }
+    },
+    forceMerge() {
+      return this.mergeForm.canMergeNow && !this.mergeForm.allOverridableChecksOk;
+    },
   },
 
   watch: {