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

Supports wildcard protected branch #20825

Merged
merged 72 commits into from
Jan 16, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
9f9f488
Make database supports glob branch name protected branch
lunny Aug 17, 2022
1dfa785
Fix bug
lunny Aug 17, 2022
bf27be6
Fix fmt
lunny Aug 17, 2022
963e438
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Aug 17, 2022
6f39f75
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Aug 18, 2022
588c5b7
Most UI changes
lunny Aug 18, 2022
997a01d
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Aug 18, 2022
9740a6d
merge main branch
lunny Sep 6, 2022
b12ac62
Fix test
lunny Sep 6, 2022
5e55875
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Sep 12, 2022
553869a
support protected branch rules
lunny Sep 12, 2022
aba5aad
support * not cross slash
lunny Sep 13, 2022
80d2396
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 18, 2022
e894bdf
Fix lint
lunny Oct 18, 2022
957a20b
Fix matching algorithm
lunny Oct 19, 2022
d87e6bd
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 19, 2022
30bbfd0
Fix import lint
lunny Oct 19, 2022
a82b8d1
Fix tests
lunny Oct 20, 2022
1837aae
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 20, 2022
a5cc6d3
Rule Sort
lunny Oct 20, 2022
ad304ec
Fix tests
lunny Oct 22, 2022
3425685
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 22, 2022
17a7982
Fix lint
lunny Oct 22, 2022
86b0029
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 23, 2022
c0989ed
Fix doc
lunny Oct 23, 2022
562077e
Fix test
lunny Oct 23, 2022
fbb17f3
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 23, 2022
220d385
Fix lint
lunny Oct 23, 2022
0c234ab
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 23, 2022
31d6595
Fix lint
lunny Oct 23, 2022
0c1ef7f
Fix test
lunny Oct 24, 2022
b23a742
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Oct 24, 2022
b428771
merge main branch
lunny Oct 31, 2022
300e623
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Nov 4, 2022
3e0eefc
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Nov 5, 2022
398123b
Fix test
lunny Nov 5, 2022
94b5ae6
fix test
lunny Nov 10, 2022
e392b85
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Nov 10, 2022
4ec2dcb
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 1, 2022
accc65c
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 2, 2022
5b23369
Fix test
lunny Dec 3, 2022
fee37c3
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 3, 2022
e50aad2
Merge main branch
lunny Dec 3, 2022
b0fa0ef
Fix lint
lunny Dec 3, 2022
71310ec
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 3, 2022
d95f214
Fix test
lunny Dec 3, 2022
bdf75cc
Fix test
lunny Dec 3, 2022
7e43b21
Fix bug
lunny Dec 3, 2022
5e8aa8c
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 3, 2022
11fcf57
Fix bug
lunny Dec 4, 2022
841cf18
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 4, 2022
361baca
merge main branch
lunny Dec 4, 2022
6a55815
Some improvements
lunny Dec 5, 2022
40577d7
Add branch name back in API but mark it as deprecated
lunny Dec 5, 2022
39e28c5
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 5, 2022
8fbe8ad
Fix lint
lunny Dec 6, 2022
16ca6c2
merge main branch
lunny Dec 10, 2022
9f18da1
fix lint
lunny Dec 11, 2022
ded5545
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 11, 2022
e40338c
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 11, 2022
8ab38d9
merge main branch
lunny Dec 13, 2022
389dce2
Merge branch 'lunny/glob_protected_branch_rule' of github.com:lunny/g…
lunny Dec 13, 2022
2eeb920
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 14, 2022
9e3a06d
Update models/git/protected_branch.go
lunny Dec 21, 2022
73a589c
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Dec 30, 2022
e378263
Merge branch 'lunny/glob_protected_branch_rule' of github.com:lunny/g…
lunny Dec 30, 2022
59c0714
some improvement
lunny Dec 30, 2022
01c2057
Fix lint
lunny Dec 30, 2022
5d078d4
Merge branch 'main' into lunny/glob_protected_branch_rule
lunny Jan 1, 2023
f5c010b
merge main branch
lunny Jan 10, 2023
c1c1a8b
Merge branch 'lunny/glob_protected_branch_rule' of github.com:lunny/g…
lunny Jan 10, 2023
1bbc980
merge main branch
lunny Jan 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
434 changes: 16 additions & 418 deletions models/git/branches.go

Large diffs are not rendered by default.

434 changes: 434 additions & 0 deletions models/git/protected_branch.go

Large diffs are not rendered by default.

87 changes: 87 additions & 0 deletions models/git/protected_branch_list.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2022 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 git

import (
"context"
"sort"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/modules/git"

"github.com/gobwas/glob"
)

type ProtectedBranchRules []*ProtectedBranch

func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedBranch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedBranch {
// GetFirstMatched returns the first rule matching the branch name, or nil if the branch is not protected
func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedBranch {

lunny marked this conversation as resolved.
Show resolved Hide resolved
for _, rule := range rules {
if rule.Match(branchName) {
return rule
}
}
return nil
}

func (rules ProtectedBranchRules) Sort() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (rules ProtectedBranchRules) Sort() {
// Sort sorts first by "rule literalness" and then by creation date
func (rules ProtectedBranchRules) Sort() {

lunny marked this conversation as resolved.
Show resolved Hide resolved
sort.Slice(rules, func(i, j int) bool {
rules[i].loadGlob()
rules[j].loadGlob()
if rules[i].isPlainName {
if !rules[j].isPlainName {
return true
}
} else if rules[j].isPlainName {
return true
}
return rules[i].CreatedUnix < rules[j].CreatedUnix
lunny marked this conversation as resolved.
Show resolved Hide resolved
})
}

// FindRepoProtectedBranchRules load all repository's protected rules
func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedBranchRules, error) {
var rules ProtectedBranchRules
err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Asc("created_unix").Find(&rules)
if err != nil {
return nil, err
}
rules.Sort()
return rules, nil
}

// FindAllMatchedBranches find all matched branches
func FindAllMatchedBranches(ctx context.Context, gitRepo *git.Repository, ruleName string) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this function?
When would you need that information?
Shouldn't you simply ask for a specific branch if it is matched?

// FIXME: how many should we get?
branches, _, err := gitRepo.GetBranchNames(0, 9999999)
if err != nil {
return nil, err
}
rule := glob.MustCompile(ruleName)
results := make([]string, 0, len(branches))
for _, branch := range branches {
if rule.Match(branch) {
results = append(results, branch)
}
}
return results, nil
}

// GetFirstMatchProtectedBranchRule returns the first matched rules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetFirstMatchProtectedBranchRule returns the first matched rules
// GetFirstMatchProtectedBranchRule returns the first matched rules or nil if none matches

func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, should we perhaps omit the Match?
I think that's implied, and shorter names are better for readability…

rules, err := FindRepoProtectedBranchRules(ctx, repoID)
if err != nil {
return nil, err
}
return rules.GetFirstMatched(branchName), nil
}

// IsBranchProtected checks if branch is protected
func IsBranchProtected(repoID int64, branchName string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, again I'm wondering: When do you need this information without requiring the underlying branch protection?

rule, err := GetFirstMatchProtectedBranchRule(db.DefaultContext, repoID, branchName)
if err != nil {
return false, err
}
return rule != nil, nil
Comment on lines +82 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err != nil {
return false, err
}
return rule != nil, nil
return err == nil && rule != nil, err

}
69 changes: 69 additions & 0 deletions models/git/protected_branch_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright 2022 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 git

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
)

func TestBranchRuleMatch(t *testing.T) {
kases := []struct {
Rule string
BranchName string
ExpectedMatch bool
}{
{
Rule: "release/*",
BranchName: "release/v1.17",
ExpectedMatch: true,
},
{
Rule: "release/**/v1.17",
BranchName: "release/test/v1.17",
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have to say, I find especially the edge case

{
	"Rule":          "release/**/v1.17",
	"BranchName":    "release/v1.17",
	"ExpectedMatch": true,
}

interesting.

ExpectedMatch: true,
},
{
Rule: "release/**/v1.17",
BranchName: "release/test/1/v1.17",
ExpectedMatch: true,
},
{
Rule: "release/*/v1.17",
BranchName: "release/test/1/v1.17",
ExpectedMatch: false,
},
{
Rule: "release/v*",
BranchName: "release/v1.16",
ExpectedMatch: true,
},
{
Rule: "*",
BranchName: "release/v1.16",
ExpectedMatch: false,
},
{
Rule: "**",
BranchName: "release/v1.16",
ExpectedMatch: true,
},
}

for _, kase := range kases {
pb := ProtectedBranch{BranchName: kase.Rule}
var should, infact string
if !kase.ExpectedMatch {
should = " not"
} else {
infact = " not"
}
assert.EqualValues(t, kase.ExpectedMatch, pb.Match(kase.BranchName),
fmt.Sprintf("%s should%s match %s but it is%s", kase.BranchName, should, kase.Rule, infact),
)
}
}
27 changes: 2 additions & 25 deletions models/issues/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,8 @@ type PullRequest struct {
HeadBranch string
HeadCommitID string `xorm:"-"`
BaseBranch string
ProtectedBranch *git_model.ProtectedBranch `xorm:"-"`
MergeBase string `xorm:"VARCHAR(40)"`
AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"`
MergeBase string `xorm:"VARCHAR(40)"`
AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"`

HasMerged bool `xorm:"INDEX"`
MergedCommitID string `xorm:"VARCHAR(40)"`
Expand Down Expand Up @@ -313,28 +312,6 @@ func (pr *PullRequest) LoadIssueCtx(ctx context.Context) (err error) {
return err
}

// LoadProtectedBranch loads the protected branch of the base branch
func (pr *PullRequest) LoadProtectedBranch() (err error) {
return pr.LoadProtectedBranchCtx(db.DefaultContext)
}

// LoadProtectedBranchCtx loads the protected branch of the base branch
func (pr *PullRequest) LoadProtectedBranchCtx(ctx context.Context) (err error) {
if pr.ProtectedBranch == nil {
if pr.BaseRepo == nil {
if pr.BaseRepoID == 0 {
return nil
}
pr.BaseRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.BaseRepoID)
if err != nil {
return
}
}
pr.ProtectedBranch, err = git_model.GetProtectedBranchBy(ctx, pr.BaseRepo.ID, pr.BaseBranch)
}
return err
}

// ReviewCount represents a count of Reviews
type ReviewCount struct {
IssueID int64
Expand Down
17 changes: 10 additions & 7 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,15 +272,17 @@ func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewers ...*user_mo
if err != nil {
return false, err
}
if err = pr.LoadProtectedBranchCtx(ctx); err != nil {

rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if err != nil {
return false, err
}
Comment on lines +267 to 270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if err != nil {
return false, err
}
rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if rule == nil || err != nil {
return false, err
}

if pr.ProtectedBranch == nil {
if rule == nil {
return false, nil
}
Comment on lines +271 to 273
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if rule == nil {
return false, nil
}


for _, reviewer := range reviewers {
official, err := git_model.IsUserOfficialReviewerCtx(ctx, pr.ProtectedBranch, reviewer)
official, err := git_model.IsUserOfficialReviewer(ctx, rule, reviewer)
if official || err != nil {
return official, err
}
Expand All @@ -295,18 +297,19 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
if err != nil {
return false, err
}
if err = pr.LoadProtectedBranchCtx(ctx); err != nil {
pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if err != nil {
return false, err
}
Comment on lines +292 to 294
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err != nil {
return false, err
}
if pb == nil || err != nil {
return false, err
}

if pr.ProtectedBranch == nil {
if pb == nil {
return false, nil
}
Comment on lines +295 to 297
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if pb == nil {
return false, nil
}


if !pr.ProtectedBranch.EnableApprovalsWhitelist {
if !pb.EnableApprovalsWhitelist {
return team.UnitAccessModeCtx(ctx, unit.TypeCode) >= perm.AccessModeWrite, nil
}

return base.Int64sContains(pr.ProtectedBranch.ApprovalsWhitelistTeamIDs, team.ID), nil
return base.Int64sContains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil
}

// CreateReview creates a new review based on opts
Expand Down
2 changes: 1 addition & 1 deletion modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ type CanCommitToBranchResults struct {
//
// and branch is not protected for push
func (r *Repository) CanCommitToBranch(ctx context.Context, doer *user_model.User) (CanCommitToBranchResults, error) {
protectedBranch, err := git_model.GetProtectedBranchBy(ctx, r.Repository.ID, r.BranchName)
protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, r.Repository.ID, r.BranchName)
if err != nil {
return CanCommitToBranchResults{}, err
}
Expand Down
2 changes: 1 addition & 1 deletion modules/structs/repo_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type BranchProtection struct {

// CreateBranchProtectionOption options for creating a branch protection
type CreateBranchProtectionOption struct {
BranchName string `json:"branch_name"`
RuleName string `json:"branch_name"` // it now in fact stores rule name not only branch name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there perhaps an option to create aliases?
Because if yes, we could make branch_name the default for legacy reasons and fallback to rule_name for newer things.
Otherwise you can ignore this comment.

EnablePush bool `json:"enable_push"`
EnablePushWhitelist bool `json:"enable_push_whitelist"`
PushWhitelistUsernames []string `json:"push_whitelist_usernames"`
Expand Down
10 changes: 7 additions & 3 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1809,6 +1809,7 @@ settings.mirror_sync_in_progress = Mirror synchronization is in progress. Check
settings.site = Website
settings.update_settings = Update Settings
settings.branches.update_default_branch = Update Default Branch
settings.branches.add_new_rule = Add New Rule
settings.advanced_settings = Advanced Settings
settings.wiki_desc = Enable Repository Wiki
settings.use_internal_wiki = Use Built-In Wiki
Expand Down Expand Up @@ -2052,6 +2053,8 @@ settings.deploy_key_deletion_desc = Removing a deploy key will revoke its access
settings.deploy_key_deletion_success = The deploy key has been removed.
settings.branches = Branches
settings.protected_branch = Branch Protection
settings.protected_branch.save_rule = Save Rule
settings.protected_branch.delete_rule = Delete Rule
settings.protected_branch_can_push = Allow push?
settings.protected_branch_can_push_yes = You can push
settings.protected_branch_can_push_no = You can not push
Expand Down Expand Up @@ -2086,15 +2089,16 @@ settings.dismiss_stale_approvals = Dismiss stale approvals
settings.dismiss_stale_approvals_desc = When new commits that change the content of the pull request are pushed to the branch, old approvals will be dismissed.
settings.require_signed_commits = Require Signed Commits
settings.require_signed_commits_desc = Reject pushes to this branch if they are unsigned or unverifiable.
settings.protect_branch_name_pattern = Protected Branch Name Pattern
settings.protect_protected_file_patterns = Protected file patterns (separated using semicolon '\;'):
settings.protect_protected_file_patterns_desc = Protected files that are not allowed to be changed directly even if user has rights to add, edit, or delete files in this branch. Multiple patterns can be separated using semicolon ('\;'). See <a href="https://pkg.go.dev/github.com/gobwas/glob#Compile">github.com/gobwas/glob</a> documentation for pattern syntax. Examples: <code>.drone.yml</code>, <code>/docs/**/*.txt</code>.
settings.protect_unprotected_file_patterns = Unprotected file patterns (separated using semicolon '\;'):
settings.protect_unprotected_file_patterns_desc = Unprotected files that are allowed to be changed directly if user has write access, bypassing push restriction. Multiple patterns can be separated using semicolon ('\;'). See <a href="https://pkg.go.dev/github.com/gobwas/glob#Compile">github.com/gobwas/glob</a> documentation for pattern syntax. Examples: <code>.drone.yml</code>, <code>/docs/**/*.txt</code>.
settings.add_protected_branch = Enable protection
settings.delete_protected_branch = Disable protection
settings.update_protect_branch_success = Branch protection for branch '%s' has been updated.
settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled.
settings.protected_branch_deletion = Disable Branch Protection
settings.update_protect_branch_success = Branch protection for rule '%s' has been updated.
settings.remove_protected_branch_success = Branch protection for rule '%s' has been removed.
settings.protected_branch_deletion = Delete 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.
Expand Down
Loading