Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent merge of outdated PRs on protected branches #11012

Merged
merged 13 commits into from
Apr 17, 2020
Merged
6 changes: 6 additions & 0 deletions models/branches.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type ProtectedBranch struct {
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"`
BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"`
DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"`
RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"`
ProtectedFilePatterns string `xorm:"TEXT"`
Expand Down Expand Up @@ -194,6 +195,11 @@ func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullReque
return rejectExist
}

// MergeBlockedByOutdatedBranch returns true if merge is blocked by an outdated head branch
func (protectBranch *ProtectedBranch) MergeBlockedByOutdatedBranch(pr *PullRequest) bool {
return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0
}

// GetProtectedFilePatterns parses a semicolon separated list of protected file patterns and returns a glob.Glob slice
func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob {
extarr := make([]glob.Glob, 0, 10)
Expand Down
6 changes: 4 additions & 2 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,12 @@ var migrations = []Migration{
NewMigration("Add EmailHash Table", addEmailHashTable),
// v134 -> v135
NewMigration("Refix merge base for merged pull requests", refixMergeBase),
// v135 -> 136
// v135 -> v136
NewMigration("Add OrgID column to Labels table", addOrgIDLabelColumn),
// v136 -> 137
// v136 -> v137
NewMigration("Add CommitsAhead and CommitsBehind Column to PullRequest Table", addCommitDivergenceToPulls),
// v137 -> v138
NewMigration("Add Branch Protection Block Outdated Branch", addBlockOnOutdatedBranch),
}

// GetCurrentDBVersion returns the current db version
Expand Down
16 changes: 16 additions & 0 deletions models/migrations/v137.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright 2020 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"xorm.io/xorm"
)

func addBlockOnOutdatedBranch(x *xorm.Engine) error {
type ProtectedBranch struct {
BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"`
}
return x.Sync2(new(ProtectedBranch))
}
1 change: 1 addition & 0 deletions modules/auth/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ type ProtectBranchForm struct {
ApprovalsWhitelistUsers string
ApprovalsWhitelistTeams string
BlockOnRejectedReviews bool
BlockOnOutdatedBranch bool
DismissStaleApprovals bool
RequireSignedCommits bool
ProtectedFilePatterns string
Expand Down
1 change: 1 addition & 0 deletions modules/convert/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func ToBranchProtection(bp *models.ProtectedBranch) *api.BranchProtection {
ApprovalsWhitelistUsernames: approvalsWhitelistUsernames,
ApprovalsWhitelistTeams: approvalsWhitelistTeams,
BlockOnRejectedReviews: bp.BlockOnRejectedReviews,
BlockOnOutdatedBranch: bp.BlockOnOutdatedBranch,
DismissStaleApprovals: bp.DismissStaleApprovals,
RequireSignedCommits: bp.RequireSignedCommits,
ProtectedFilePatterns: bp.ProtectedFilePatterns,
Expand Down
3 changes: 3 additions & 0 deletions modules/structs/repo_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type BranchProtection struct {
ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"`
ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"`
BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"`
BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"`
DismissStaleApprovals bool `json:"dismiss_stale_approvals"`
RequireSignedCommits bool `json:"require_signed_commits"`
ProtectedFilePatterns string `json:"protected_file_patterns"`
Expand Down Expand Up @@ -66,6 +67,7 @@ type CreateBranchProtectionOption struct {
ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"`
ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"`
BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"`
BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"`
DismissStaleApprovals bool `json:"dismiss_stale_approvals"`
RequireSignedCommits bool `json:"require_signed_commits"`
ProtectedFilePatterns string `json:"protected_file_patterns"`
Expand All @@ -88,6 +90,7 @@ type EditBranchProtectionOption struct {
ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"`
ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"`
BlockOnRejectedReviews *bool `json:"block_on_rejected_reviews"`
BlockOnOutdatedBranch *bool `json:"block_on_outdated_branch"`
DismissStaleApprovals *bool `json:"dismiss_stale_approvals"`
RequireSignedCommits *bool `json:"require_signed_commits"`
ProtectedFilePatterns *string `json:"protected_file_patterns"`
Expand Down
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1101,6 +1101,7 @@ pulls.required_status_check_failed = Some required checks were not successful.
pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer."
pulls.blocked_by_outdated_branch = "This Pull Request is blocked by an outdated head branch."
6543 marked this conversation as resolved.
Show resolved Hide resolved
pulls.can_auto_merge_desc = This pull request can be merged automatically.
pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts.
pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
Expand Down Expand Up @@ -1525,6 +1526,8 @@ settings.protected_branch_deletion = Disable Branch Protection
settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue?
settings.block_rejected_reviews = Block merge on rejected reviews
settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, even if there are enough approvals.
settings.block_outdated_branch = Block merge if branch is outdated
6543 marked this conversation as resolved.
Show resolved Hide resolved
settings.block_outdated_branch_desc = Merging will not be possible when head branch is behind base branch.
settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
settings.choose_branch = Choose a branch…
settings.no_protected_branch = There are no protected branches.
Expand Down
5 changes: 5 additions & 0 deletions routers/api/v1/repo/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ func CreateBranchProtection(ctx *context.APIContext, form api.CreateBranchProtec
DismissStaleApprovals: form.DismissStaleApprovals,
RequireSignedCommits: form.RequireSignedCommits,
ProtectedFilePatterns: form.ProtectedFilePatterns,
BlockOnOutdatedBranch: form.BlockOnOutdatedBranch,
}

err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
Expand Down Expand Up @@ -475,6 +476,10 @@ func EditBranchProtection(ctx *context.APIContext, form api.EditBranchProtection
protectBranch.ProtectedFilePatterns = *form.ProtectedFilePatterns
}

if form.BlockOnOutdatedBranch != nil {
protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch
}

var whitelistUsers []int64
if form.PushWhitelistUsernames != nil {
whitelistUsers, err = models.GetUserIDsByNames(form.PushWhitelistUsernames, false)
Expand Down
1 change: 1 addition & 0 deletions routers/private/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
}
}

// Detect Protected file pattern
globs := protectBranch.GetProtectedFilePatterns()
if len(globs) > 0 {
err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env)
Expand Down
1 change: 1 addition & 0 deletions routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,7 @@ func ViewIssue(ctx *context.Context) {
cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull)
ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull)
ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull)
ctx.Data["GrantedApprovals"] = cnt
ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits
}
Expand Down
1 change: 1 addition & 0 deletions routers/repo/setting_protected_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
protectBranch.DismissStaleApprovals = f.DismissStaleApprovals
protectBranch.RequireSignedCommits = f.RequireSignedCommits
protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns
protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch

err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
UserIDs: whitelistUsers,
Expand Down
10 changes: 8 additions & 2 deletions services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -571,16 +571,22 @@ func CheckPRReadyToMerge(pr *models.PullRequest) (err error) {
}
}

if enoughApprovals := pr.ProtectedBranch.HasEnoughApprovals(pr); !enoughApprovals {
if !pr.ProtectedBranch.HasEnoughApprovals(pr) {
return models.ErrNotAllowedToMerge{
Reason: "Does not have enough approvals",
}
}
if rejected := pr.ProtectedBranch.MergeBlockedByRejectedReview(pr); rejected {
if pr.ProtectedBranch.MergeBlockedByRejectedReview(pr) {
return models.ErrNotAllowedToMerge{
Reason: "There are requested changes",
}
}

if pr.ProtectedBranch.MergeBlockedByOutdatedBranch(pr) {
return models.ErrNotAllowedToMerge{
Reason: fmt.Sprintf("Head branch %d behind base branch", pr.CommitsBehind),
6543 marked this conversation as resolved.
Show resolved Hide resolved
}
}

return nil
}
13 changes: 12 additions & 1 deletion templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
{{- else if .IsPullRequestBroken}}red
{{- else if .IsBlockedByApprovals}}red
{{- else if .IsBlockedByRejection}}red
{{- else if .IsBlockedByOutdatedBranch}}red
{{- else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red
{{- else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow
{{- else if and .RequireSigned (not .WillSign)}}}red
Expand Down Expand Up @@ -138,6 +139,11 @@
<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
{{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
</div>
{{else if .IsBlockedByOutdatedBranch}}
<div class="item text red">
<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
{{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}}
</div>
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
<div class="item text red">
<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
Expand All @@ -153,7 +159,7 @@
{{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
</div>
{{end}}
{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOutdatedBranch (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
{{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .RequireSigned) .WillSign)}}
{{if $notAllOverridableChecksOk}}
<div class="item text yellow">
Expand Down Expand Up @@ -337,6 +343,11 @@
{{svg "octicon-x" 16}}
{{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
</div>
{{else if .IsBlockedByOutdatedBranch}}
<div class="item text red">
<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
{{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}}
</div>
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
<div class="item text red">
{{svg "octicon-x" 16}}
Expand Down
7 changes: 7 additions & 0 deletions templates/repo/settings/protected_branch.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,13 @@
<p class="help">{{.i18n.Tr "repo.settings.require_signed_commits_desc"}}</p>
</div>
</div>
<div class="field">
<div class="ui checkbox">
<input name="block_on_outdated_branch" type="checkbox" {{if .Branch.BlockOnOutdatedBranch}}checked{{end}}>
<label for="block_on_outdated_branch">{{.i18n.Tr "repo.settings.block_outdated_branch"}}</label>
<p class="help">{{.i18n.Tr "repo.settings.block_outdated_branch_desc"}}</p>
</div>
</div>
<div class="field">
<label for="protected_file_patterns">{{.i18n.Tr "repo.settings.protect_protected_file_patterns"}}</label>
<input name="protected_file_patterns" id="protected_file_patterns" type="text" value="{{.Branch.ProtectedFilePatterns}}">
Expand Down
12 changes: 12 additions & 0 deletions templates/swagger/v1_json.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -10054,6 +10054,10 @@
},
"x-go-name": "ApprovalsWhitelistUsernames"
},
"block_on_outdated_branch": {
"type": "boolean",
"x-go-name": "BlockOnOutdatedBranch"
},
"block_on_rejected_reviews": {
"type": "boolean",
"x-go-name": "BlockOnRejectedReviews"
Expand Down Expand Up @@ -10374,6 +10378,10 @@
},
"x-go-name": "ApprovalsWhitelistUsernames"
},
"block_on_outdated_branch": {
"type": "boolean",
"x-go-name": "BlockOnOutdatedBranch"
},
"block_on_rejected_reviews": {
"type": "boolean",
"x-go-name": "BlockOnRejectedReviews"
Expand Down Expand Up @@ -11186,6 +11194,10 @@
},
"x-go-name": "ApprovalsWhitelistUsernames"
},
"block_on_outdated_branch": {
"type": "boolean",
"x-go-name": "BlockOnOutdatedBranch"
},
"block_on_rejected_reviews": {
"type": "boolean",
"x-go-name": "BlockOnRejectedReviews"
Expand Down