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.
This commit is contained in:
Jason Song 2023-02-21 23:42:22 +08:00 committed by GitHub
parent 8fa62be905
commit 90982bffa5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 59 additions and 20 deletions

View file

@ -767,11 +767,18 @@ func MergePullRequest(ctx *context.APIContext) {
} }
} }
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
force := form.ForceMerge != nil && *form.ForceMerge
mergeCheckType := pull_service.MergeCheckTypeGeneral
if form.MergeWhenChecksSucceed {
mergeCheckType = pull_service.MergeCheckTypeAuto
}
if manuallyMerged {
mergeCheckType = pull_service.MergeCheckTypeManually
}
// start with merging by checking // 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) { if errors.Is(err, pull_service.ErrIsClosed) {
ctx.NotFound() ctx.NotFound()
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { } else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
@ -793,7 +800,7 @@ func MergePullRequest(ctx *context.APIContext) {
} }
// handle manually-merged mark // handle manually-merged mark
if manuallMerge { if manuallyMerged {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if models.IsErrInvalidMergeStyle(err) { 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))) 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)))

View file

@ -912,11 +912,19 @@ func MergePullRequest(ctx *context.Context) {
pr := issue.PullRequest pr := issue.PullRequest
pr.Issue = issue pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository 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 // 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 { switch {
case errors.Is(err, pull_service.ErrIsClosed): case errors.Is(err, pull_service.ErrIsClosed):
if issue.IsPull { if issue.IsPull {
@ -948,7 +956,7 @@ func MergePullRequest(ctx *context.Context) {
} }
// handle manually-merged mark // handle manually-merged mark
if manualMerge { if manuallyMerged {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
switch { switch {

View file

@ -231,7 +231,7 @@ func handlePull(pullID int64, sha string) {
return 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) { if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) {
log.Info("%-v was scheduled to automerge by an unauthorized user", pr) log.Info("%-v was scheduled to automerge by an unauthorized user", pr)
return return

View file

@ -597,7 +597,7 @@ type MergePullRequestForm struct {
MergeMessageField string MergeMessageField string
MergeCommitID string // only used for manually-merged MergeCommitID string // only used for manually-merged
HeadCommitID string `json:"head_commit_id,omitempty"` 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"` MergeWhenChecksSucceed bool `json:"merge_when_checks_succeed,omitempty"`
DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"`
} }

View file

@ -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, ...) // 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 { return db.WithTx(func(ctx context.Context) error {
if pr.HasMerged { if pr.HasMerged {
return ErrHasMerged return ErrHasMerged
@ -80,8 +88,8 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
return ErrUserNotAllowedToMerge return ErrUserNotAllowedToMerge
} }
if manuallMerge { if mergeCheckType == MergeCheckTypeManually {
// don't check rules to "auto merge", doer is going to mark this pull as merged manually // if doer is doing "manually merge" (mark as merged manually), do not check anything
return nil return nil
} }
@ -103,14 +111,25 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
return err return err
} }
if !force { // Now the branch protection check failed, check whether the failure could be skipped (skip by setting err = nil)
return err
// * 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 { // * if the doer is admin, they could skip the branch protection check
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, err2) if adminSkipProtectionCheck {
return err2 if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
} else if !isRepoAdmin { 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 return err
} }
} }

View file

@ -18,6 +18,7 @@
<input type="hidden" name="_csrf" :value="csrfToken"> <input type="hidden" name="_csrf" :value="csrfToken">
<input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID"> <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="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
<input type="hidden" name="force_merge" v-model="forceMerge">
<template v-if="!mergeStyleDetail.hideMergeMessageTexts"> <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
<div class="field"> <div class="field">
@ -127,6 +128,7 @@ export default {
textDoMerge: '', textDoMerge: '',
mergeTitleFieldText: '', mergeTitleFieldText: '',
mergeMessageFieldText: '', mergeMessageFieldText: '',
hideAutoMerge: false,
}, },
mergeStyleAllowedCount: 0, mergeStyleAllowedCount: 0,
@ -138,7 +140,10 @@ export default {
mergeButtonStyleClass() { mergeButtonStyleClass() {
if (this.mergeForm.allOverridableChecksOk) return 'green'; if (this.mergeForm.allOverridableChecksOk) return 'green';
return this.autoMergeWhenSucceed ? 'blue' : 'red'; return this.autoMergeWhenSucceed ? 'blue' : 'red';
} },
forceMerge() {
return this.mergeForm.canMergeNow && !this.mergeForm.allOverridableChecksOk;
},
}, },
watch: { watch: {