diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 3c3eb66d5c..1f82bfce1b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -922,7 +922,8 @@ func MergePullRequest(ctx *context.APIContext) { } } - manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged + mergeStyle := repo_model.MergeStyle(form.Do) + manuallyMerged := mergeStyle == repo_model.MergeStyleManuallyMerged mergeCheckType := pull_service.MergeCheckTypeGeneral if form.MergeWhenChecksSucceed { @@ -933,7 +934,7 @@ func MergePullRequest(ctx *context.APIContext) { } // start with merging by checking - if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil { + if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge, mergeStyle); err != nil { if errors.Is(err, pull_service.ErrIsClosed) { ctx.NotFound() } else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index e9cdfc5ac7..dc36d2de68 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1317,7 +1317,8 @@ func MergePullRequest(ctx *context.Context) { pr.Issue = issue pr.Issue.Repo = ctx.Repo.Repository - manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged + mergeStyle := repo_model.MergeStyle(form.Do) + manuallyMerged := mergeStyle == repo_model.MergeStyleManuallyMerged mergeCheckType := pull_service.MergeCheckTypeGeneral if form.MergeWhenChecksSucceed { @@ -1328,7 +1329,7 @@ func MergePullRequest(ctx *context.Context) { } // start with merging by checking - if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil { + if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge, mergeStyle); err != nil { switch { case errors.Is(err, pull_service.ErrIsClosed): if issue.IsPull { diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 099d048927..4c5459adf6 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -215,7 +215,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if err := pull_service.CheckPullMergeable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false); err != nil { + if err := pull_service.CheckPullMergeable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false, scheduledPRM.MergeStyle); err != nil { if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { log.Info("%-v was scheduled to automerge by an unauthorized user", pr) return diff --git a/services/pull/check.go b/services/pull/check.go index 64bddf497c..0e74973456 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -65,7 +65,7 @@ const ( ) // CheckPullMergeable check if the pull mergeable based on all conditions (branch protection, merge options, ...) -func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool) error { +func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool, mergeStyle repo_model.MergeStyle) error { return db.WithTx(stdCtx, func(ctx context.Context) error { if pr.HasMerged { return ErrHasMerged @@ -136,7 +136,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc } } - if _, err := isSignedIfRequired(ctx, pr, doer); err != nil { + if _, err := isSignedIfRequired(ctx, pr, doer, mergeStyle); err != nil { return err } @@ -151,7 +151,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc } // isSignedIfRequired check if merge will be signed if required -func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User) (bool, error) { +func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle) (bool, error) { pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) if err != nil { return false, err @@ -161,11 +161,22 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer return true, nil } + if !isMergeSigningRequired(mergeStyle) { + return true, nil + } + sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName()) return sign, err } +func isMergeSigningRequired(mergeStyle repo_model.MergeStyle) bool { + // Only fast-forward-only is guaranteed not to create a new commit. Rebase + // rewrites commits when the pull request is behind, and it can also amend + // the tip commit when a REBASE_TEMPLATE is configured. + return mergeStyle != repo_model.MergeStyleFastForwardOnly +} + // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) bool { diff --git a/services/pull/check_test.go b/services/pull/check_test.go index 9b7e1660bc..3115bb7a21 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -11,6 +11,7 @@ import ( "forgejo.org/models/db" issues_model "forgejo.org/models/issues" + repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" "forgejo.org/modules/queue" "forgejo.org/modules/setting" @@ -67,3 +68,53 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { prPatchCheckerQueue.ShutdownWait(5 * time.Second) prPatchCheckerQueue = nil } + +func TestIsMergeSigningRequired(t *testing.T) { + testCases := []struct { + name string + mergeStyle repo_model.MergeStyle + expected bool + }{ + { + name: "fast-forward never requires signing", + mergeStyle: repo_model.MergeStyleFastForwardOnly, + expected: false, + }, + { + name: "rebase requires signing even when up to date", + mergeStyle: repo_model.MergeStyleRebase, + expected: true, + }, + { + name: "rebase-merge requires signing", + mergeStyle: repo_model.MergeStyleRebaseMerge, + expected: true, + }, + { + name: "squash commits require signing", + mergeStyle: repo_model.MergeStyleSquash, + expected: true, + }, + { + name: "merge commits require signing", + mergeStyle: repo_model.MergeStyleMerge, + expected: true, + }, + { + name: "rebase-update style still requires signing", + mergeStyle: repo_model.MergeStyleRebaseUpdate, + expected: true, + }, + { + name: "empty merge style requires signing", + mergeStyle: "", + expected: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + assert.Equal(t, testCase.expected, isMergeSigningRequired(testCase.mergeStyle)) + }) + } +} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index b5cce664a3..ca9b943fa2 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -1,3 +1,9 @@ +{{$prUnit := .Repository.MustGetUnit $.Context $.UnitTypePullRequests}} +{{$prConfig := $prUnit.PullRequestsConfig}} +{{$fastForwardStyleAllowed := and $prConfig.AllowFastForwardOnly (eq .Issue.PullRequest.CommitsBehind 0)}} +{{$manualMergeStyleAllowed := $prConfig.AllowManualMerge}} +{{$hasUnsignedMergeStyle := or $fastForwardStyleAllowed $manualMergeStyleAllowed}} +{{$signingBlocksAllMergeStyles := and .AllowMerge .RequireSigned (not .WillSign) (not $hasUnsignedMergeStyle)}} {{if and .Issue.PullRequest.HasMerged (not .IsPullBranchDeletable)}} {{/* Then the merge box will not be displayed because this page already contains enough information */}} {{else}} @@ -14,7 +20,7 @@ {{- else if .IsBlockedByChangedProtectedFiles}}red {{- else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red {{- else if and .EnableStatusCheck (or (not $.LatestCommitStatus) .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow - {{- else if and .AllowMerge .RequireSigned (not .WillSign)}}red + {{- else if $signingBlocksAllMergeStyles}}red {{- else if .Issue.PullRequest.IsChecking}}yellow {{- else if .Issue.PullRequest.IsEmpty}}grey {{- else if .Issue.PullRequest.CanAutoMerge}}green @@ -148,7 +154,7 @@ {{svg "octicon-x"}} {{ctx.Locale.Tr "repo.pulls.required_status_check_missing"}} - {{else if and .AllowMerge .RequireSigned (not .WillSign)}} + {{else if $signingBlocksAllMergeStyles}}
{{svg "octicon-x"}} {{ctx.Locale.Tr "repo.pulls.require_signed_wont_sign"}} @@ -162,7 +168,7 @@ {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}} {{/* admin can merge without checks, writer can merge when checks succeed */}} - {{$canMergeNow := and (or (and $.IsRepoAdmin (not .ProtectedBranch.ApplyToAdmins)) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}} + {{$canMergeNow := and (or (and $.IsRepoAdmin (not .ProtectedBranch.ApplyToAdmins)) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not $signingBlocksAllMergeStyles))}} {{/* admin and writer both can make an auto merge schedule */}} {{if $canMergeNow}} @@ -201,8 +207,7 @@ {{end}} {{if .AllowMerge}} {{/* user is allowed to merge */}} - {{$prUnit := .Repository.MustGetUnit $.Context $.UnitTypePullRequests}} - {{if or $prUnit.PullRequestsConfig.AllowMerge $prUnit.PullRequestsConfig.AllowRebase $prUnit.PullRequestsConfig.AllowRebaseMerge $prUnit.PullRequestsConfig.AllowSquash $prUnit.PullRequestsConfig.AllowFastForwardOnly $prUnit.PullRequestsConfig.AllowManualMerge}} + {{if or $prConfig.AllowMerge $prConfig.AllowRebase $prConfig.AllowRebaseMerge $prConfig.AllowSquash $prConfig.AllowFastForwardOnly $prConfig.AllowManualMerge}} {{$hasPendingPullRequestMergeTip := ""}} {{if .HasPendingPullRequestMerge}} {{$createdPRMergeStr := DateUtils.TimeSince .PendingPullRequestMerge.CreatedUnix}} @@ -231,7 +236,7 @@ 'pullHeadCommitID': {{.PullHeadCommitID}}, 'isPullBranchDeletable': {{.IsPullBranchDeletable}}, 'defaultMergeStyle': {{.MergeStyle}}, - 'defaultDeleteBranchAfterMerge': {{$prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}, + 'defaultDeleteBranchAfterMerge': {{$prConfig.DefaultDeleteBranchAfterMerge}}, 'mergeMessageFieldPlaceHolder': {{ctx.Locale.Tr "repo.editor.commit_message_desc"}}, 'defaultMergeMessage': defaultMergeMessage, @@ -243,7 +248,7 @@ mergeForm['mergeStyles'] = [ { 'name': 'merge', - 'allowed': {{$prUnit.PullRequestsConfig.AllowMerge}}, + 'allowed': {{and $prConfig.AllowMerge (or (not .RequireSigned) .WillSign)}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.merge_pull_request"}}, 'mergeTitleFieldText': defaultMergeTitle, 'mergeMessageFieldText': defaultMergeMessage, @@ -251,14 +256,14 @@ }, { 'name': 'rebase', - 'allowed': {{$prUnit.PullRequestsConfig.AllowRebase}}, + 'allowed': {{and $prConfig.AllowRebase (or (not .RequireSigned) .WillSign)}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.rebase_merge_pull_request"}}, 'hideMergeMessageTexts': true, 'hideAutoMerge': generalHideAutoMerge, }, { 'name': 'rebase-merge', - 'allowed': {{$prUnit.PullRequestsConfig.AllowRebaseMerge}}, + 'allowed': {{and $prConfig.AllowRebaseMerge (or (not .RequireSigned) .WillSign)}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.rebase_merge_commit_pull_request"}}, 'mergeTitleFieldText': defaultMergeTitle, 'mergeMessageFieldText': defaultMergeMessage, @@ -266,7 +271,7 @@ }, { 'name': 'squash', - 'allowed': {{$prUnit.PullRequestsConfig.AllowSquash}}, + 'allowed': {{and $prConfig.AllowSquash (or (not .RequireSigned) .WillSign)}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.squash_merge_pull_request"}}, 'mergeTitleFieldText': defaultSquashMergeTitle, 'mergeMessageFieldText': {{.GetCommitMessages}} + defaultSquashMergeMessage, @@ -274,14 +279,14 @@ }, { 'name': 'fast-forward-only', - 'allowed': {{and $prUnit.PullRequestsConfig.AllowFastForwardOnly (eq .Issue.PullRequest.CommitsBehind 0)}}, + 'allowed': {{$fastForwardStyleAllowed}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.fast_forward_only_merge_pull_request"}}, 'hideMergeMessageTexts': true, 'hideAutoMerge': generalHideAutoMerge, }, { 'name': 'manually-merged', - 'allowed': {{$prUnit.PullRequestsConfig.AllowManualMerge}}, + 'allowed': {{$manualMergeStyleAllowed}}, 'textDoMerge': {{ctx.Locale.Tr "repo.pulls.merge_manually"}}, 'hideMergeMessageTexts': true, 'hideAutoMerge': true, @@ -293,7 +298,7 @@ {{$showGeneralMergeForm = true}}
{{else}} - {{/* no merge style was set in repo setting: not or ($prUnit.PullRequestsConfig.AllowMerge ...) */}} + {{/* no merge style was set in repo setting: not or ($prConfig.AllowMerge ...) */}}
{{svg "octicon-x"}} @@ -349,7 +354,7 @@ {{svg "octicon-x"}} {{ctx.Locale.Tr "repo.pulls.required_status_check_failed"}}
- {{else if and .RequireSigned (not .WillSign)}} + {{else if $signingBlocksAllMergeStyles}}
{{svg "octicon-x"}} {{ctx.Locale.Tr "repo.pulls.require_signed_wont_sign"}} diff --git a/web_src/js/components/PullRequestMergeForm.test.js b/web_src/js/components/PullRequestMergeForm.test.js index 7b856e0b88..86402eebc6 100644 --- a/web_src/js/components/PullRequestMergeForm.test.js +++ b/web_src/js/components/PullRequestMergeForm.test.js @@ -32,3 +32,35 @@ test('renders escaped branch name', async () => { mergeform = await renderMergeForm(''); expect(mergeform.get('label[for="delete-branch-after-merge"]').text()).toBe('Delete branch ""'); }); + +test('hides merge controls when no merge style is allowed', () => { + window.config.pageData.pullRequestMergeForm = { + textDeleteBranch: 'Delete branch', + textAutoMergeButtonWhenSucceed: 'when checks succeed', + textAutoMergeWhenSucceed: 'Auto merge when checks succeed', + textAutoMergeCancelSchedule: 'Cancel schedule', + textCancel: 'Cancel', + defaultDeleteBranchAfterMerge: false, + defaultMergeMessage: '', + defaultMergeStyle: 'merge', + emptyCommit: false, + hasPendingPullRequestMerge: false, + hasPendingPullRequestMergeTip: '', + isPullBranchDeletable: false, + canMergeNow: true, + allOverridableChecksOk: true, + pullHeadCommitID: 'abc123', + mergeStyles: [{ + name: 'merge', + allowed: false, + textDoMerge: 'Merge', + mergeTitleFieldText: '', + mergeMessageFieldText: '', + hideAutoMerge: false, + }], + }; + + const mergeform = mount(PullRequestMergeForm); + expect(mergeform.find('.merge-button').exists()).toBe(false); + expect(mergeform.find('form.form-fetch-action').exists()).toBe(false); +}); diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue index df123f5e2f..56265b10dd 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -38,7 +38,8 @@ export default { }, watch: { mergeStyle(val) { - this.mergeStyleDetail = this.mergeForm.mergeStyles.find((e) => e.name === val); + const mergeStyleDetail = this.mergeForm.mergeStyles.find((e) => e.name === val); + if (mergeStyleDetail) this.mergeStyleDetail = mergeStyleDetail; for (const elem of document.querySelectorAll('[data-pull-merge-style]')) { toggleElem(elem, elem.getAttribute('data-pull-merge-style') === val); } @@ -49,6 +50,7 @@ export default { let mergeStyle = this.mergeForm.mergeStyles.find((e) => e.allowed && e.name === this.mergeForm.defaultMergeStyle)?.name; if (!mergeStyle) mergeStyle = this.mergeForm.mergeStyles.find((e) => e.allowed)?.name; + if (!mergeStyle) return; this.switchMergeStyle(mergeStyle, !this.mergeForm.canMergeNow); }, mounted() { @@ -62,6 +64,7 @@ export default { this.showMergeStyleMenu = false; }, toggleActionForm(show) { + if (show && this.mergeStyleAllowedCount === 0) return; this.showActionForm = show; if (!show) return; this.deleteBranchAfterMerge = this.mergeForm.defaultDeleteBranchAfterMerge; @@ -134,7 +137,7 @@ export default {
-
+
-
+