- {{$.i18n.Tr "repo.pulls.blocked_by_changed_protected_files"}}
+ {{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
{{range .ChangedProtectedFiles}}
{{end}}
@@ -376,7 +376,7 @@
{{else if .IsBlockedByChangedProtectedFiles}}
{{svg "octicon-x" 16}}
- {{$.i18n.Tr "repo.pulls.blocked_by_changed_protected_files"}}
+ {{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
{{range .ChangedProtectedFiles}}
{{.}}
{{end}}
From 5c1ab251cecfcb70defe0d35283ff22c71a24d20 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sat, 5 Sep 2020 09:24:56 +0800
Subject: [PATCH 05/18] Apply suggestions from code review
---
models/branches.go | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/models/branches.go b/models/branches.go
index fa245f7ea87e3..d82a2ad0c2e01 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -246,12 +246,13 @@ func (protectBranch *ProtectedBranch) MergeBlockedByProtectedFiles(pr *PullReque
}
// GetPrChangedProtectedFiles returns protected files changed by pr
-func (protectBranch *ProtectedBranch) GetPrChangedProtectedFiles(pr *PullRequest) (changs []string, err error) {
+func (protectBranch *ProtectedBranch) GetPrChangedProtectedFiles(pr *PullRequest) ([]string, error) {
glob := protectBranch.GetProtectedFilePatterns()
if len(glob) == 0 {
return nil, nil
}
+ var err error
if err = pr.LoadBaseRepo(); err != nil {
return nil, err
}
@@ -271,12 +272,13 @@ func (protectBranch *ProtectedBranch) GetPrChangedProtectedFiles(pr *PullRequest
return nil, err
}
+ var changs []string
_, changs, err = checkFileProtection(true, pr.MergeBase, headCommitID, glob, gitRepo)
- return
+ return changs, nil
}
// IsProtectedFile return if path is protected
-func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path string) (r bool) {
+func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path string) bool {
if len(patterns) == 0 {
patterns = protectBranch.GetProtectedFilePatterns()
if len(patterns) == 0 {
@@ -286,6 +288,7 @@ func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path
lpath := strings.ToLower(strings.TrimSpace(path))
+ r := false
for _, pat := range patterns {
if pat.Match(lpath) {
r = true
@@ -293,19 +296,21 @@ func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path
}
}
- return
+ return r
}
-func checkFileProtection(needFullResult bool, oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository) (result bool, changedFiles []string, err error) {
+func checkFileProtection(needFullResult bool, oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository) (bool, []string, error) {
stdout, err := git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).RunInDir(repo.Path)
if err != nil {
return false, nil, err
}
+ var changedFiles []string
if needFullResult {
changedFiles = make([]string, 0, 5)
}
+ result := false
for _, path := range strings.Split(stdout, "\n") {
lpath := strings.ToLower(strings.TrimSpace(path))
for _, pat := range patterns {
@@ -323,7 +328,7 @@ func checkFileProtection(needFullResult bool, oldCommitID, newCommitID string, p
}
}
- return
+ return result, changedFiles, nil
}
// GetProtectedBranchByRepoID getting protected branch by repo ID
From 1e491c7ebd92e8d0b3ddbb80bd03f43759078c7c Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sat, 5 Sep 2020 10:16:05 +0800
Subject: [PATCH 06/18] fix lint
---
models/branches.go | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/models/branches.go b/models/branches.go
index d82a2ad0c2e01..4a83a67dc2853 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -274,6 +274,10 @@ func (protectBranch *ProtectedBranch) GetPrChangedProtectedFiles(pr *PullRequest
var changs []string
_, changs, err = checkFileProtection(true, pr.MergeBase, headCommitID, glob, gitRepo)
+ if err != nil {
+ return nil, err
+ }
+
return changs, nil
}
From fcd4ce578efae233f5202fb84fd1a1a33f109260 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com>
Date: Sat, 5 Sep 2020 17:27:33 +0800
Subject: [PATCH 07/18] Update options/locale/locale_en-US.ini
Co-authored-by: zeripath
---
options/locale/locale_en-US.ini | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 792f2617db36a..5a99bea0ea57c 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1195,7 +1195,7 @@ pulls.required_status_check_administrator = As an administrator, you may still m
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 because it's outdated."
-pulls.blocked_by_changed_protected_files_1= "This Pull Request is blocked because it changes protected file."
+pulls.blocked_by_changed_protected_files_1= "This Pull Request is blocked because it changes a protected file."
pulls.blocked_by_changed_protected_files_n= "This Pull Request is blocked because it changes protected files."
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.
From ea6716541a460df58fcfd659a5a31492f591b448 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sun, 6 Sep 2020 13:34:31 +0800
Subject: [PATCH 08/18] Apply suggestions from code review
* move pr proteced files check to TestPatch
* Call TestPatch when protected branches settings changed
---
models/branches.go | 118 +++++++++--------------
models/migrations/migrations.go | 2 +
models/migrations/v149.go | 22 +++++
models/pull.go | 2 +
options/locale/locale_en-US.ini | 4 +-
routers/api/v1/repo/branch.go | 11 +++
routers/repo/issue.go | 13 +--
routers/repo/setting_protected_branch.go | 5 +
services/pull/check.go | 16 ++-
services/pull/patch.go | 21 ++++
services/pull/pull.go | 2 +-
11 files changed, 129 insertions(+), 87 deletions(-)
create mode 100644 models/migrations/v149.go
diff --git a/models/branches.go b/models/branches.go
index 4a83a67dc2853..8fb4fcad74b1a 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -217,68 +217,7 @@ func (protectBranch *ProtectedBranch) MergeBlockedByProtectedFiles(pr *PullReque
return false
}
- var err error
-
- if err = pr.LoadBaseRepo(); err != nil {
- log.Error("pr.loadBaseRepo: %v", err)
- return true
- }
-
- gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
- if err != nil {
- log.Error("git.OpenRepository: %v", err)
- return true
- }
-
- headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
- if err != nil {
- log.Error("git.GetRefCommitID: %v", err)
- return true
- }
-
- result, _, err := checkFileProtection(false, pr.MergeBase, headCommitID, glob, gitRepo)
- if err != nil {
- log.Error("checkFileProtection: %v", err)
- return true
- }
-
- return result
-}
-
-// GetPrChangedProtectedFiles returns protected files changed by pr
-func (protectBranch *ProtectedBranch) GetPrChangedProtectedFiles(pr *PullRequest) ([]string, error) {
- glob := protectBranch.GetProtectedFilePatterns()
- if len(glob) == 0 {
- return nil, nil
- }
-
- var err error
- if err = pr.LoadBaseRepo(); err != nil {
- return nil, err
- }
-
- var (
- gitRepo *git.Repository
- headCommitID string
- )
-
- gitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
- if err != nil {
- return nil, err
- }
-
- headCommitID, err = gitRepo.GetRefCommitID(pr.GetGitRefName())
- if err != nil {
- return nil, err
- }
-
- var changs []string
- _, changs, err = checkFileProtection(true, pr.MergeBase, headCommitID, glob, gitRepo)
- if err != nil {
- return nil, err
- }
-
- return changs, nil
+ return len(pr.ChangedProtectedFiles) > 0
}
// IsProtectedFile return if path is protected
@@ -303,36 +242,69 @@ func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path
return r
}
-func checkFileProtection(needFullResult bool, oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository) (bool, []string, error) {
+// CheckPullFilesProtection check if pr changed protected files and save results
+func (pr *PullRequest) CheckPullFilesProtection() (err error) {
+ if err = pr.LoadProtectedBranch(); err != nil {
+ return
+ }
+
+ if pr.ProtectedBranch == nil {
+ pr.ChangedProtectedFiles = nil
+ return nil
+ }
+
+ if err = pr.LoadBaseRepo(); err != nil {
+ return
+ }
+
+ gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
+ if err != nil {
+ return err
+ }
+ defer gitRepo.Close()
+
+ headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
+ if err != nil {
+ return err
+ }
+
+ pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, gitRepo)
+ return
+}
+
+// CheckFileProtection check file Protection
+func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, repo *git.Repository) ([]string, error) {
stdout, err := git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).RunInDir(repo.Path)
if err != nil {
- return false, nil, err
+ return nil, err
+ }
+
+ if len(patterns) == 0 {
+ return nil, nil
}
var changedFiles []string
- if needFullResult {
- changedFiles = make([]string, 0, 5)
+ if limit <= 10 {
+ changedFiles = make([]string, 0, limit)
+ } else {
+ changedFiles = make([]string, 0, 10)
}
- result := false
for _, path := range strings.Split(stdout, "\n") {
lpath := strings.ToLower(strings.TrimSpace(path))
for _, pat := range patterns {
if pat.Match(lpath) {
- result = true
- if needFullResult {
- changedFiles = append(changedFiles, path)
- }
+ changedFiles = append(changedFiles, path)
break
}
}
- if result && !needFullResult {
+ if len(changedFiles) >= limit {
break
}
}
- return result, changedFiles, nil
+ return changedFiles, nil
}
// GetProtectedBranchByRepoID getting protected branch by repo ID
diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 6d27934f6db70..4898c297d386d 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -230,6 +230,8 @@ var migrations = []Migration{
NewMigration("create review for 0 review id code comments", createReviewsForCodeComments),
// v148 -> v149
NewMigration("remove issue dependency comments who refer to non existing issues", purgeInvalidDependenciesComments),
+ // v149 -> v150
+ NewMigration("add changed_protected_files column for pull_request table", addChangedProtectedFilesPullRequestColumn),
}
// GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v149.go b/models/migrations/v149.go
new file mode 100644
index 0000000000000..58d78b6cfbd90
--- /dev/null
+++ b/models/migrations/v149.go
@@ -0,0 +1,22 @@
+// 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 (
+ "fmt"
+
+ "xorm.io/xorm"
+)
+
+func addChangedProtectedFilesPullRequestColumn(x *xorm.Engine) error {
+ type PullRequest struct {
+ ChangedProtectedFiles []string `xorm:"TEXT JSON"`
+ }
+
+ if err := x.Sync2(new(PullRequest)); err != nil {
+ return fmt.Errorf("Sync2: %v", err)
+ }
+ return nil
+}
diff --git a/models/pull.go b/models/pull.go
index 9f1f485266a59..9b6f0830d7df7 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -45,6 +45,8 @@ type PullRequest struct {
CommitsAhead int
CommitsBehind int
+ ChangedProtectedFiles []string `xorm:"TEXT JSON"`
+
IssueID int64 `xorm:"INDEX"`
Issue *Issue `xorm:"-"`
Index int64
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 5a99bea0ea57c..a16b3ee055d48 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1195,8 +1195,8 @@ pulls.required_status_check_administrator = As an administrator, you may still m
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 because it's outdated."
-pulls.blocked_by_changed_protected_files_1= "This Pull Request is blocked because it changes a protected file."
-pulls.blocked_by_changed_protected_files_n= "This Pull Request is blocked because it changes protected files."
+pulls.blocked_by_changed_protected_files_1= "This Pull Request is blocked because it changes a protected file:"
+pulls.blocked_by_changed_protected_files_n= "This Pull Request is blocked because it changes protected files:"
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.
diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go
index 90db597ef7cb7..77d0f995f9cd7 100644
--- a/routers/api/v1/repo/branch.go
+++ b/routers/api/v1/repo/branch.go
@@ -17,6 +17,7 @@ import (
"code.gitea.io/gitea/modules/repofiles"
repo_module "code.gitea.io/gitea/modules/repository"
api "code.gitea.io/gitea/modules/structs"
+ pull_service "code.gitea.io/gitea/services/pull"
)
// GetBranch get a branch of a repository
@@ -547,6 +548,11 @@ func CreateBranchProtection(ctx *context.APIContext, form api.CreateBranchProtec
return
}
+ if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+ ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err)
+ return
+ }
+
// Reload from db to get all whitelists
bp, err := models.GetProtectedBranchBy(ctx.Repo.Repository.ID, form.BranchName)
if err != nil {
@@ -770,6 +776,11 @@ func EditBranchProtection(ctx *context.APIContext, form api.EditBranchProtection
return
}
+ if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+ ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err)
+ return
+ }
+
// Reload from db to ensure get all whitelists
bp, err := models.GetProtectedBranchBy(repo.ID, bpName)
if err != nil {
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 19b5c63421cd0..d2922bc59e08c 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1200,16 +1200,9 @@ func ViewIssue(ctx *context.Context) {
ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull)
ctx.Data["GrantedApprovals"] = cnt
ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits
-
- var changedProtectedFiles []string
-
- if changedProtectedFiles, err = pull.ProtectedBranch.GetPrChangedProtectedFiles(pull); err != nil {
- ctx.ServerError("ProtectedBranch.GetPrChangedProtectedFiles", err)
- return
- }
- ctx.Data["ChangedProtectedFiles"] = changedProtectedFiles
- ctx.Data["IsBlockedByChangedProtectedFiles"] = len(changedProtectedFiles) != 0
- ctx.Data["ChangedProtectedFilesNum"] = len(changedProtectedFiles)
+ ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles
+ ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0
+ ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)
}
ctx.Data["WillSign"] = false
if ctx.User != nil {
diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go
index ab0fd77eee250..f864e8a75c1f2 100644
--- a/routers/repo/setting_protected_branch.go
+++ b/routers/repo/setting_protected_branch.go
@@ -16,6 +16,7 @@ import (
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
+ pull_service "code.gitea.io/gitea/services/pull"
)
// ProtectedBranch render the page to protect the repository
@@ -262,6 +263,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
ctx.ServerError("UpdateProtectBranch", err)
return
}
+ if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+ ctx.ServerError("CheckPrsForBaseBranch", err)
+ return
+ }
ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", branch))
ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, branch))
} else {
diff --git a/services/pull/check.go b/services/pull/check.go
index d6817bc81b5e0..8665b3e7dfc78 100644
--- a/services/pull/check.go
+++ b/services/pull/check.go
@@ -62,7 +62,7 @@ func checkAndUpdateStatus(pr *models.PullRequest) {
}
if !has {
- if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files"); err != nil {
+ if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil {
log.Error("Update[%d]: %v", pr.ID, err)
}
}
@@ -228,6 +228,20 @@ func handle(data ...queue.Data) {
}
}
+// CheckPrsForBaseBranch check all pulls with bseBrannch
+func CheckPrsForBaseBranch(baseRepo *models.Repository, baseBranchName string) error {
+ prs, err := models.GetUnmergedPullRequestsByBaseInfo(baseRepo.ID, baseBranchName)
+ if err != nil {
+ return err
+ }
+
+ for _, pr := range prs {
+ AddToTaskQueue(pr)
+ }
+
+ return nil
+}
+
// Init runs the task queue to test all the checking status pull requests
func Init() error {
prQueue = queue.CreateUniqueQueue("pr_patch_checker", handle, "").(queue.UniqueQueue)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index 9b894b781cc4d..8c3dd413dbbf7 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -184,10 +184,31 @@ func TestPatch(pr *models.PullRequest) error {
if conflict {
pr.Status = models.PullRequestStatusConflict
log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
+
+ if pr.Index != 0 {
+ if err = pr.CheckPullFilesProtection(); err != nil {
+ return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
+ }
+ }
+
+ if len(pr.ChangedProtectedFiles) > 0 {
+ log.Trace("Found %d protected files changed")
+ }
return nil
}
return fmt.Errorf("git apply --check: %v", err)
}
+
+ if pr.Index != 0 {
+ if err = pr.CheckPullFilesProtection(); err != nil {
+ return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
+ }
+ }
+
+ if len(pr.ChangedProtectedFiles) > 0 {
+ log.Trace("Found %d protected files changed", len(pr.ChangedProtectedFiles))
+ }
+
pr.Status = models.PullRequestStatusMergeable
return nil
diff --git a/services/pull/pull.go b/services/pull/pull.go
index e624b182aa58b..7dac83bf32fa7 100644
--- a/services/pull/pull.go
+++ b/services/pull/pull.go
@@ -175,7 +175,7 @@ func ChangeTargetBranch(pr *models.PullRequest, doer *models.User, targetBranch
pr.CommitsAhead = divergence.Ahead
pr.CommitsBehind = divergence.Behind
- if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "base_branch", "commits_ahead", "commits_behind"); err != nil {
+ if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch", "commits_ahead", "commits_behind"); err != nil {
return err
}
From ed808be059961a181faf1b1b97f1bc910aeb4f30 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 15 Sep 2020 22:37:33 +0800
Subject: [PATCH 09/18] Apply review suggestion @CirnoT
---
templates/repo/issue/view_content/pull.tmpl | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index 0bb35d3750282..6a6ab80743ce2 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -154,9 +154,11 @@
{{svg "octicon-x" 16}}
{{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
- {{range .ChangedProtectedFiles}}
-
{{.}}
- {{end}}
+
+ {{range .ChangedProtectedFiles}}
+
{{.}}
+ {{end}}
+
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
@@ -377,9 +379,11 @@
{{svg "octicon-x" 16}}
{{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
- {{range .ChangedProtectedFiles}}
-
{{.}}
- {{end}}
+
+ {{range .ChangedProtectedFiles}}
+
{{.}}
+ {{end}}
+
{{else if and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess)}}
From 4c7f37050c855297f37a7d1f2f791227058f5e11 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Mon, 21 Sep 2020 17:09:08 +0800
Subject: [PATCH 10/18] move to service @lunny
---
models/branches.go | 66 -----------------------------------
services/pull/patch.go | 78 ++++++++++++++++++++++++++++++++++++------
2 files changed, 68 insertions(+), 76 deletions(-)
diff --git a/models/branches.go b/models/branches.go
index 8fb4fcad74b1a..420a4b663a4e3 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -11,7 +11,6 @@ import (
"time"
"code.gitea.io/gitea/modules/base"
- "code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
@@ -242,71 +241,6 @@ func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path
return r
}
-// CheckPullFilesProtection check if pr changed protected files and save results
-func (pr *PullRequest) CheckPullFilesProtection() (err error) {
- if err = pr.LoadProtectedBranch(); err != nil {
- return
- }
-
- if pr.ProtectedBranch == nil {
- pr.ChangedProtectedFiles = nil
- return nil
- }
-
- if err = pr.LoadBaseRepo(); err != nil {
- return
- }
-
- gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
- if err != nil {
- return err
- }
- defer gitRepo.Close()
-
- headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
- if err != nil {
- return err
- }
-
- pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, gitRepo)
- return
-}
-
-// CheckFileProtection check file Protection
-func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, repo *git.Repository) ([]string, error) {
- stdout, err := git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).RunInDir(repo.Path)
- if err != nil {
- return nil, err
- }
-
- if len(patterns) == 0 {
- return nil, nil
- }
-
- var changedFiles []string
- if limit <= 10 {
- changedFiles = make([]string, 0, limit)
- } else {
- changedFiles = make([]string, 0, 10)
- }
-
- for _, path := range strings.Split(stdout, "\n") {
- lpath := strings.ToLower(strings.TrimSpace(path))
- for _, pat := range patterns {
- if pat.Match(lpath) {
- changedFiles = append(changedFiles, path)
- break
- }
- }
-
- if len(changedFiles) >= limit {
- break
- }
- }
-
- return changedFiles, nil
-}
-
// GetProtectedBranchByRepoID getting protected branch by repo ID
func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
protectedBranches := make([]*ProtectedBranch, 0)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index 8c3dd413dbbf7..8673ddb4d7b27 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -18,6 +18,8 @@ import (
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"
+
+ "github.com/gobwas/glob"
)
// DownloadDiffOrPatch will write the patch for the pr to the writer
@@ -185,22 +187,13 @@ func TestPatch(pr *models.PullRequest) error {
pr.Status = models.PullRequestStatusConflict
log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
- if pr.Index != 0 {
- if err = pr.CheckPullFilesProtection(); err != nil {
- return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
- }
- }
-
- if len(pr.ChangedProtectedFiles) > 0 {
- log.Trace("Found %d protected files changed")
- }
return nil
}
return fmt.Errorf("git apply --check: %v", err)
}
if pr.Index != 0 {
- if err = pr.CheckPullFilesProtection(); err != nil {
+ if err = CheckPullFilesProtection(pr); err != nil {
return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
}
}
@@ -213,3 +206,68 @@ func TestPatch(pr *models.PullRequest) error {
return nil
}
+
+// CheckFileProtection check file Protection
+func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, repo *git.Repository) ([]string, error) {
+ stdout, err := git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).RunInDir(repo.Path)
+ if err != nil {
+ return nil, err
+ }
+
+ if len(patterns) == 0 {
+ return nil, nil
+ }
+
+ var changedFiles []string
+ if limit <= 10 {
+ changedFiles = make([]string, 0, limit)
+ } else {
+ changedFiles = make([]string, 0, 10)
+ }
+
+ for _, path := range strings.Split(stdout, "\n") {
+ lpath := strings.ToLower(strings.TrimSpace(path))
+ for _, pat := range patterns {
+ if pat.Match(lpath) {
+ changedFiles = append(changedFiles, path)
+ break
+ }
+ }
+
+ if len(changedFiles) >= limit {
+ break
+ }
+ }
+
+ return changedFiles, nil
+}
+
+// CheckPullFilesProtection check if pr changed protected files and save results
+func CheckPullFilesProtection(pr *models.PullRequest) (err error) {
+ if err = pr.LoadProtectedBranch(); err != nil {
+ return
+ }
+
+ if pr.ProtectedBranch == nil {
+ pr.ChangedProtectedFiles = nil
+ return nil
+ }
+
+ if err = pr.LoadBaseRepo(); err != nil {
+ return
+ }
+
+ gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
+ if err != nil {
+ return err
+ }
+ defer gitRepo.Close()
+
+ headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
+ if err != nil {
+ return err
+ }
+
+ pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, gitRepo)
+ return
+}
From 2adda2d9eaba4637630d9fb98827b93c3946e5ee Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 10:28:56 +0100
Subject: [PATCH 11/18] slightly restructure routers/private/hook.go
Adds a lot of comments and simplifies the logic
Signed-off-by: Andrew Thornton
---
routers/private/hook.go | 275 +++++++++++++++++++++++-----------------
1 file changed, 156 insertions(+), 119 deletions(-)
diff --git a/routers/private/hook.go b/routers/private/hook.go
index ca8721814c770..e82f570e668b6 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -202,6 +202,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
private.GitQuarantinePath+"="+opts.GitQuarantinePath)
}
+ // Iterate across the provided old commit IDs
for i := range opts.OldCommitIDs {
oldCommitID := opts.OldCommitIDs[i]
newCommitID := opts.NewCommitIDs[i]
@@ -224,142 +225,102 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
})
return
}
- if protectBranch != nil && protectBranch.IsProtected() {
- // detect and prevent deletion
- if newCommitID == git.EmptySHA {
- log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo)
+
+ // Allow pushes to non-protected branches
+ if protectBranch == nil || !protectBranch.IsProtected() {
+ continue
+ }
+
+ // This ref is a protected branch.
+ //
+ // First of all we need to enforce absolutely:
+ //
+ // 1. Detect and prevent deletion of the branch
+ if newCommitID == git.EmptySHA {
+ log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo)
+ ctx.JSON(http.StatusForbidden, map[string]interface{}{
+ "err": fmt.Sprintf("branch %s is protected from deletion", branchName),
+ })
+ return
+ }
+
+ // 2. Disallow force pushes to protected branches
+ if git.EmptySHA != oldCommitID {
+ output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
+ if err != nil {
+ log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Fail to detect force push: %v", err),
+ })
+ return
+ } else if len(output) > 0 {
+ log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
- "err": fmt.Sprintf("branch %s is protected from deletion", branchName),
+ "err": fmt.Sprintf("branch %s is protected from force push", branchName),
})
return
+
}
+ }
- // detect force push
- if git.EmptySHA != oldCommitID {
- output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
- if err != nil {
- log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
+ // 3. Enforce require signed commits
+ if protectBranch.RequireSignedCommits {
+ err := verifyCommits(oldCommitID, newCommitID, gitRepo, env)
+ if err != nil {
+ if !isErrUnverifiedCommit(err) {
+ log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Fail to detect force push: %v", err),
+ "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
})
return
- } else if len(output) > 0 {
- log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo)
- ctx.JSON(http.StatusForbidden, map[string]interface{}{
- "err": fmt.Sprintf("branch %s is protected from force push", branchName),
- })
- return
-
}
+ unverifiedCommit := err.(*errUnverifiedCommit).sha
+ log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit)
+ ctx.JSON(http.StatusForbidden, map[string]interface{}{
+ "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
+ })
+ return
}
+ }
- // Require signed commits
- if protectBranch.RequireSignedCommits {
- err := verifyCommits(oldCommitID, newCommitID, gitRepo, env)
- if err != nil {
- if !isErrUnverifiedCommit(err) {
- log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
- })
- return
- }
- unverifiedCommit := err.(*errUnverifiedCommit).sha
- log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit)
- ctx.JSON(http.StatusForbidden, map[string]interface{}{
- "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
+ // Now there are several tests which can be overriden:
+ //
+ // 4. Check protected file patterns - this is overridable from the UI
+ changedProtectedfiles := false
+ protectedFilePath := ""
+
+ globs := protectBranch.GetProtectedFilePatterns()
+ if len(globs) > 0 {
+ err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env)
+ if err != nil {
+ if !models.IsErrFilePathProtected(err) {
+ log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
})
return
}
- }
- var (
- changedProtectedfiles bool
- protectedFilePath string
- )
-
- // Check Protected file pattern
- globs := protectBranch.GetProtectedFilePatterns()
- if len(globs) > 0 {
- err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env)
- if err != nil {
- if !models.IsErrFilePathProtected(err) {
- log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
- })
- return
- }
-
- changedProtectedfiles = true
- protectedFilePath = err.(models.ErrFilePathProtected).Path
- }
+ changedProtectedfiles = true
+ protectedFilePath = err.(models.ErrFilePathProtected).Path
}
+ }
- canPush := false
- if opts.IsDeployKey {
- canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) && !changedProtectedfiles
- } else {
- canPush = protectBranch.CanUserPush(opts.UserID) && !changedProtectedfiles
- }
- if !canPush && opts.ProtectedBranchID > 0 {
- // Merge (from UI or API)
- pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
- if err != nil {
- log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err),
- })
- return
- }
- user, err := models.GetUserByID(opts.UserID)
- if err != nil {
- log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
- })
- return
- }
- perm, err := models.GetUserRepoPermission(repo, user)
- if err != nil {
- log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
- })
- return
- }
- allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
- if err != nil {
- log.Error("Error calculating if allowed to merge: %v", err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
- })
- return
- }
- if !allowedMerge {
- log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index)
- ctx.JSON(http.StatusForbidden, map[string]interface{}{
- "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
- })
- return
- }
- // Check all status checks and reviews is ok, unless repo admin which can bypass this.
- if !perm.IsAdmin() {
- if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
- if models.IsErrNotAllowedToMerge(err) {
- log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
- ctx.JSON(http.StatusForbidden, map[string]interface{}{
- "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()),
- })
- return
- }
- log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
- ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
- "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
- })
- }
- }
- } else if !canPush {
+ // 5. Check if the doer is allowed to push
+ canPush := false
+ if opts.IsDeployKey {
+ canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
+ } else {
+ canPush = !changedProtectedfiles && protectBranch.CanUserPush(opts.UserID)
+ }
+
+ // 6. If we're not allowed to push directly
+ if !canPush {
+ // Is this is a merge from the UI/API?
+ if opts.ProtectedBranchID == 0 {
+ // 6a. If we're not merging from the UI/API then there are two ways we got here:
+ //
+ // We are changing a protected file and we're not allowed to do that
if changedProtectedfiles {
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
@@ -368,12 +329,88 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
return
}
+ // Or we're simply not able to push to this protected branch
log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
})
return
}
+ // 6b. Merge (from UI or API)
+
+ // Get the PR, user and permissions for the user in the repository
+ pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
+ if err != nil {
+ log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err),
+ })
+ return
+ }
+ user, err := models.GetUserByID(opts.UserID)
+ if err != nil {
+ log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
+ })
+ return
+ }
+ perm, err := models.GetUserRepoPermission(repo, user)
+ if err != nil {
+ log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
+ })
+ return
+ }
+
+ // Now check if the user is allowed to merge PRs for this repository
+ allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
+ if err != nil {
+ log.Error("Error calculating if allowed to merge: %v", err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
+ })
+ return
+ }
+
+ if !allowedMerge {
+ log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index)
+ ctx.JSON(http.StatusForbidden, map[string]interface{}{
+ "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
+ })
+ return
+ }
+
+ // If we're an admin for the repository we can ignore status checks, reviews and override protected files
+ if perm.IsAdmin() {
+ continue
+ }
+
+ // Now if we're not an admin - we can't overwrite protected files so fail now
+ if changedProtectedfiles {
+ log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
+ ctx.JSON(http.StatusForbidden, map[string]interface{}{
+ "err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
+ })
+ return
+ }
+
+ // Check all status checks and reviews are ok
+ if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+ if models.IsErrNotAllowedToMerge(err) {
+ log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
+ ctx.JSON(http.StatusForbidden, map[string]interface{}{
+ "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()),
+ })
+ return
+ }
+ log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
+ ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+ "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
+ })
+ return
+ }
}
}
From 9a7c9c5d31ca3c6854a1ce987570872775453267 Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 10:46:20 +0100
Subject: [PATCH 12/18] placate lint
Signed-off-by: Andrew Thornton
---
routers/private/hook.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/routers/private/hook.go b/routers/private/hook.go
index e82f570e668b6..ea934c0b46bf5 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -283,7 +283,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
}
}
- // Now there are several tests which can be overriden:
+ // Now there are several tests which can be overridden:
//
// 4. Check protected file patterns - this is overridable from the UI
changedProtectedfiles := false
From 2dc3c6d01743517ce7ccf96e8079b61441381e9a Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 13 Oct 2020 19:06:07 +0800
Subject: [PATCH 13/18] skip duplicate protected files check
---
routers/api/v1/repo/pull.go | 2 +-
routers/private/hook.go | 2 +-
routers/repo/pull.go | 2 +-
services/pull/merge.go | 6 +++++-
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index afbe7ddab0c23..e2cab70adc1e1 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -774,7 +774,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
return
}
- if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+ if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
return
diff --git a/routers/private/hook.go b/routers/private/hook.go
index ea934c0b46bf5..bf03f79785e0e 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -397,7 +397,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
}
// Check all status checks and reviews are ok
- if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+ if err := pull_service.CheckPRReadyToMerge(pr, true); err != nil {
if models.IsErrNotAllowedToMerge(err) {
log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
ctx.JSON(http.StatusForbidden, map[string]interface{}{
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 26d908c252677..54da7b76b7edf 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -786,7 +786,7 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
return
}
- if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+ if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.ServerError("Merge PR status", err)
return
diff --git a/services/pull/merge.go b/services/pull/merge.go
index ee10d8ce96a10..e74b4b6b1e814 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -559,7 +559,7 @@ func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *mod
}
// CheckPRReadyToMerge checks whether the PR is ready to be merged (reviews and status checks)
-func CheckPRReadyToMerge(pr *models.PullRequest) (err error) {
+func CheckPRReadyToMerge(pr *models.PullRequest, skipProtectedFilesCheck bool) (err error) {
if err = pr.LoadBaseRepo(); err != nil {
return fmt.Errorf("LoadBaseRepo: %v", err)
}
@@ -598,6 +598,10 @@ func CheckPRReadyToMerge(pr *models.PullRequest) (err error) {
}
}
+ if skipProtectedFilesCheck {
+ return nil
+ }
+
if pr.ProtectedBranch.MergeBlockedByProtectedFiles(pr) {
return models.ErrNotAllowedToMerge{
Reason: "Changed protected files",
From 350b482a9ac82af9b3f499986a74ce9417667d30 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 13 Oct 2020 19:53:37 +0800
Subject: [PATCH 14/18] fix check logic
---
routers/private/hook.go | 50 +---------------------------
services/pull/patch.go | 74 +++++++++++++++++++++++++++--------------
2 files changed, 50 insertions(+), 74 deletions(-)
diff --git a/routers/private/hook.go b/routers/private/hook.go
index bf03f79785e0e..d39cd7178c88d 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -25,7 +25,6 @@ import (
"gitea.com/macaron/macaron"
"github.com/go-git/go-git/v5/plumbing"
- "github.com/gobwas/glob"
)
func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error {
@@ -59,53 +58,6 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []
return err
}
-func checkFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository, env []string) error {
-
- stdoutReader, stdoutWriter, err := os.Pipe()
- if err != nil {
- log.Error("Unable to create os.Pipe for %s", repo.Path)
- return err
- }
- defer func() {
- _ = stdoutReader.Close()
- _ = stdoutWriter.Close()
- }()
-
- // This use of ... is safe as force-pushes have already been ruled out.
- err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).
- RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
- stdoutWriter, nil, nil,
- func(ctx context.Context, cancel context.CancelFunc) error {
- _ = stdoutWriter.Close()
-
- scanner := bufio.NewScanner(stdoutReader)
- for scanner.Scan() {
- path := strings.TrimSpace(scanner.Text())
- if len(path) == 0 {
- continue
- }
- lpath := strings.ToLower(path)
- for _, pat := range patterns {
- if pat.Match(lpath) {
- cancel()
- return models.ErrFilePathProtected{
- Path: path,
- }
- }
- }
- }
- if err := scanner.Err(); err != nil {
- return err
- }
- _ = stdoutReader.Close()
- return err
- })
- if err != nil && !models.IsErrFilePathProtected(err) {
- log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
- }
- return err
-}
-
func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository, env []string) error {
scanner := bufio.NewScanner(input)
for scanner.Scan() {
@@ -291,7 +243,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
globs := protectBranch.GetProtectedFilePatterns()
if len(globs) > 0 {
- err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env)
+ _, err := pull_service.CheckFileProtection(oldCommitID, newCommitID, globs, 0, env, gitRepo)
if err != nil {
if !models.IsErrFilePathProtected(err) {
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index 8673ddb4d7b27..8b803647eae52 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -208,38 +208,59 @@ func TestPatch(pr *models.PullRequest) error {
}
// CheckFileProtection check file Protection
-func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, repo *git.Repository) ([]string, error) {
- stdout, err := git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).RunInDir(repo.Path)
+func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string, repo *git.Repository) ([]string, error) {
+ stdoutReader, stdoutWriter, err := os.Pipe()
if err != nil {
+ log.Error("Unable to create os.Pipe for %s", repo.Path)
return nil, err
}
+ defer func() {
+ _ = stdoutReader.Close()
+ _ = stdoutWriter.Close()
+ }()
- if len(patterns) == 0 {
- return nil, nil
- }
-
- var changedFiles []string
- if limit <= 10 {
- changedFiles = make([]string, 0, limit)
- } else {
- changedFiles = make([]string, 0, 10)
- }
+ changedProtectedFiles := make([]string, 0, limit)
- for _, path := range strings.Split(stdout, "\n") {
- lpath := strings.ToLower(strings.TrimSpace(path))
- for _, pat := range patterns {
- if pat.Match(lpath) {
- changedFiles = append(changedFiles, path)
- break
- }
- }
+ // This use of ... is safe as force-pushes have already been ruled out.
+ err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).
+ RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
+ stdoutWriter, nil, nil,
+ func(ctx context.Context, cancel context.CancelFunc) error {
+ _ = stdoutWriter.Close()
+ counter := 0
- if len(changedFiles) >= limit {
- break
- }
+ scanner := bufio.NewScanner(stdoutReader)
+ for scanner.Scan() {
+ path := strings.TrimSpace(scanner.Text())
+ if len(path) == 0 {
+ continue
+ }
+ lpath := strings.ToLower(path)
+ for _, pat := range patterns {
+ if pat.Match(lpath) {
+ if counter < limit {
+ counter++
+ changedProtectedFiles = append(changedProtectedFiles, path)
+ continue
+ }
+ cancel()
+ return models.ErrFilePathProtected{
+ Path: path,
+ }
+ }
+ }
+ if counter >= limit {
+ break
+ }
+ }
+ err := scanner.Err()
+ return err
+ })
+ if err != nil && !models.IsErrFilePathProtected(err) {
+ log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
}
- return changedFiles, nil
+ return changedProtectedFiles, err
}
// CheckPullFilesProtection check if pr changed protected files and save results
@@ -268,6 +289,9 @@ func CheckPullFilesProtection(pr *models.PullRequest) (err error) {
return err
}
- pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, gitRepo)
+ pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, os.Environ(), gitRepo)
+ if models.IsErrFilePathProtected(err) {
+ err = nil
+ }
return
}
From 741bf9f83261251945ab058e928165e95cfbcb17 Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 17:09:58 +0100
Subject: [PATCH 15/18] slight refactor of TestPatch
Signed-off-by: Andrew Thornton
---
services/pull/patch.go | 86 +++++++++++++++++++++++++++++++-----------
1 file changed, 63 insertions(+), 23 deletions(-)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index 8b803647eae52..e2ea57aec256a 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -68,6 +68,7 @@ func TestPatch(pr *models.PullRequest) error {
}
defer gitRepo.Close()
+ // 1. update merge base
pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
if err != nil {
var err2 error
@@ -77,10 +78,34 @@ func TestPatch(pr *models.PullRequest) error {
}
}
pr.MergeBase = strings.TrimSpace(pr.MergeBase)
+
+ // 2. Check for conflicts
+ if conflicts, err := checkConflicts(pr, gitRepo, tmpBasePath); err != nil || conflicts {
+ return err
+ }
+
+ // 3. Check for protected files changes
+ if pr.Index != 0 {
+ if err = CheckPullFilesProtection(pr); err != nil {
+ return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
+ }
+ }
+
+ if len(pr.ChangedProtectedFiles) > 0 {
+ log.Trace("Found %d protected files changed", len(pr.ChangedProtectedFiles))
+ }
+
+ pr.Status = models.PullRequestStatusMergeable
+
+ return nil
+}
+
+func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath string) (bool, error) {
+ // 1. Create a plain patch from head to base
tmpPatchFile, err := ioutil.TempFile("", "patch")
if err != nil {
log.Error("Unable to create temporary patch file! Error: %v", err)
- return fmt.Errorf("Unable to create temporary patch file! Error: %v", err)
+ return false, fmt.Errorf("Unable to create temporary patch file! Error: %v", err)
}
defer func() {
_ = util.Remove(tmpPatchFile.Name())
@@ -89,38 +114,43 @@ func TestPatch(pr *models.PullRequest) error {
if err := gitRepo.GetDiff(pr.MergeBase, "tracking", tmpPatchFile); err != nil {
tmpPatchFile.Close()
log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
- return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
+ return false, fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
}
stat, err := tmpPatchFile.Stat()
if err != nil {
tmpPatchFile.Close()
- return fmt.Errorf("Unable to stat patch file: %v", err)
+ return false, fmt.Errorf("Unable to stat patch file: %v", err)
}
patchPath := tmpPatchFile.Name()
tmpPatchFile.Close()
+ // 1a. if the size of that patch is 0 - there can be no conflicts!
if stat.Size() == 0 {
log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID)
pr.Status = models.PullRequestStatusMergeable
pr.ConflictedFiles = []string{}
- return nil
+ return false, nil
}
log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
+ // 2. preset the pr.Status as checking (this is not save at present)
pr.Status = models.PullRequestStatusChecking
+ // 3. Read the base branch in to the index of the temporary repository
_, err = git.NewCommand("read-tree", "base").RunInDir(tmpBasePath)
if err != nil {
- return fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err)
+ return false, fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err)
}
+ // 4. Now get the pull request configuration to check if we need to ignore whitespace
prUnit, err := pr.BaseRepo.GetUnit(models.UnitTypePullRequests)
if err != nil {
- return err
+ return false, err
}
prConfig := prUnit.PullRequestsConfig()
+ // 5. Prepare the arguments to apply the patch against the index
args := []string{"apply", "--check", "--cached"}
if prConfig.IgnoreWhitespaceConflicts {
args = append(args, "--ignore-whitespace")
@@ -128,26 +158,40 @@ func TestPatch(pr *models.PullRequest) error {
args = append(args, patchPath)
pr.ConflictedFiles = make([]string, 0, 5)
+ // 6. Prep the pipe:
+ // - Here we could do the equivalent of:
+ // `git apply --check --cached patch_file > conflicts`
+ // Then iterate through the conflicts. However, that means storing all the conflicts
+ // in memory - which is very wasteful.
+ // - alternatively we can do the equivalent of:
+ // `git apply --check ... | grep ...`
+ // meaning we don't store all of the conflicts unnecessarily.
stderrReader, stderrWriter, err := os.Pipe()
if err != nil {
log.Error("Unable to open stderr pipe: %v", err)
- return fmt.Errorf("Unable to open stderr pipe: %v", err)
+ return false, fmt.Errorf("Unable to open stderr pipe: %v", err)
}
defer func() {
_ = stderrReader.Close()
_ = stderrWriter.Close()
}()
+
+ // 7. Run the check command
conflict := false
err = git.NewCommand(args...).
RunInDirTimeoutEnvFullPipelineFunc(
nil, -1, tmpBasePath,
nil, stderrWriter, nil,
func(ctx context.Context, cancel context.CancelFunc) error {
+ // Close the writer end of the pipe to begin processing
_ = stderrWriter.Close()
+
const prefix = "error: patch failed:"
const errorPrefix = "error: "
+
conflictMap := map[string]bool{}
+ // Now scan the output from the command
scanner := bufio.NewScanner(stderrReader)
for scanner.Scan() {
line := scanner.Text()
@@ -172,43 +216,39 @@ func TestPatch(pr *models.PullRequest) error {
break
}
}
+
if len(conflictMap) > 0 {
pr.ConflictedFiles = make([]string, 0, len(conflictMap))
for key := range conflictMap {
pr.ConflictedFiles = append(pr.ConflictedFiles, key)
}
}
+
+ // Close the reader to terminate the git command if necessary
_ = stderrReader.Close()
return nil
})
+ // 8. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error.
if err != nil {
if conflict {
pr.Status = models.PullRequestStatusConflict
log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
- return nil
- }
- return fmt.Errorf("git apply --check: %v", err)
- }
-
- if pr.Index != 0 {
- if err = CheckPullFilesProtection(pr); err != nil {
- return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
+ return true, nil
}
+ return false, fmt.Errorf("git apply --check: %v", err)
}
-
- if len(pr.ChangedProtectedFiles) > 0 {
- log.Trace("Found %d protected files changed", len(pr.ChangedProtectedFiles))
- }
-
- pr.Status = models.PullRequestStatusMergeable
-
- return nil
+ return false, nil
}
// CheckFileProtection check file Protection
func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string, repo *git.Repository) ([]string, error) {
+ // If there are no patterns short-circuit and just return nil
+ if len(patterns) == 0 {
+ return nil, nil
+ }
+
stdoutReader, stdoutWriter, err := os.Pipe()
if err != nil {
log.Error("Unable to create os.Pipe for %s", repo.Path)
From 0c63499e2ba7883cd7859a8e96f24e559c0b111e Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 18:11:55 +0100
Subject: [PATCH 16/18] When checking for protected files changes in TestPatch
use the temporary repository
Signed-off-by: Andrew Thornton
---
services/pull/patch.go | 74 ++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 42 deletions(-)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index e2ea57aec256a..b58d4a9010353 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -86,7 +86,7 @@ func TestPatch(pr *models.PullRequest) error {
// 3. Check for protected files changes
if pr.Index != 0 {
- if err = CheckPullFilesProtection(pr); err != nil {
+ if err = checkPullFilesProtection(pr, gitRepo); err != nil {
return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
}
}
@@ -185,6 +185,10 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath
func(ctx context.Context, cancel context.CancelFunc) error {
// Close the writer end of the pipe to begin processing
_ = stderrWriter.Close()
+ defer func() {
+ // Close the reader on return to terminate the git command if necessary
+ _ = stderrReader.Close()
+ }()
const prefix = "error: patch failed:"
const errorPrefix = "error: "
@@ -224,8 +228,6 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath
}
}
- // Close the reader to terminate the git command if necessary
- _ = stderrReader.Close()
return nil
})
@@ -244,11 +246,12 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath
// CheckFileProtection check file Protection
func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string, repo *git.Repository) ([]string, error) {
- // If there are no patterns short-circuit and just return nil
+ // 1. If there are no patterns short-circuit and just return nil
if len(patterns) == 0 {
return nil, nil
}
+ // 2. Prep the pipe
stdoutReader, stdoutWriter, err := os.Pipe()
if err != nil {
log.Error("Unable to create os.Pipe for %s", repo.Path)
@@ -261,14 +264,19 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob,
changedProtectedFiles := make([]string, 0, limit)
- // This use of ... is safe as force-pushes have already been ruled out.
- err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).
+ // 3. Run `git diff --name-only` to get the names of the changed files
+ err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID).
RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
stdoutWriter, nil, nil,
func(ctx context.Context, cancel context.CancelFunc) error {
+ // Close the writer end of the pipe to begin processing
_ = stdoutWriter.Close()
- counter := 0
+ defer func() {
+ // Close the reader on return to terminate the git command if necessary
+ _ = stdoutReader.Close()
+ }()
+ // Now scan the output from the command
scanner := bufio.NewScanner(stdoutReader)
for scanner.Scan() {
path := strings.TrimSpace(scanner.Text())
@@ -278,24 +286,20 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob,
lpath := strings.ToLower(path)
for _, pat := range patterns {
if pat.Match(lpath) {
- if counter < limit {
- counter++
- changedProtectedFiles = append(changedProtectedFiles, path)
- continue
- }
- cancel()
- return models.ErrFilePathProtected{
- Path: path,
- }
+ changedProtectedFiles = append(changedProtectedFiles, path)
+ break
}
}
- if counter >= limit {
- break
+ if len(changedProtectedFiles) >= limit {
+ cancel()
+ return models.ErrFilePathProtected{
+ Path: path,
+ }
}
}
- err := scanner.Err()
- return err
+ return scanner.Err()
})
+ // 4. log real errors if there are any...
if err != nil && !models.IsErrFilePathProtected(err) {
log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
}
@@ -303,10 +307,10 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob,
return changedProtectedFiles, err
}
-// CheckPullFilesProtection check if pr changed protected files and save results
-func CheckPullFilesProtection(pr *models.PullRequest) (err error) {
- if err = pr.LoadProtectedBranch(); err != nil {
- return
+// checkPullFilesProtection check if pr changed protected files and save results
+func checkPullFilesProtection(pr *models.PullRequest, gitRepo *git.Repository) error {
+ if err := pr.LoadProtectedBranch(); err != nil {
+ return err
}
if pr.ProtectedBranch == nil {
@@ -314,24 +318,10 @@ func CheckPullFilesProtection(pr *models.PullRequest) (err error) {
return nil
}
- if err = pr.LoadBaseRepo(); err != nil {
- return
- }
-
- gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
- if err != nil {
- return err
- }
- defer gitRepo.Close()
-
- headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
- if err != nil {
+ var err error
+ pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, "tracking", pr.ProtectedBranch.GetProtectedFilePatterns(), 10, os.Environ(), gitRepo)
+ if err != nil && !models.IsErrFilePathProtected(err) {
return err
}
-
- pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, headCommitID, pr.ProtectedBranch.GetProtectedFilePatterns(), 10, os.Environ(), gitRepo)
- if models.IsErrFilePathProtected(err) {
- err = nil
- }
- return
+ return nil
}
From 3e24686d175de14bc2fc8cd03c27043e5a835202 Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 18:23:33 +0100
Subject: [PATCH 17/18] fix introduced issue with hook
Signed-off-by: Andrew Thornton
---
routers/private/hook.go | 2 +-
services/pull/patch.go | 11 +++++++----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/routers/private/hook.go b/routers/private/hook.go
index d39cd7178c88d..a2033fc1dd733 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -243,7 +243,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
globs := protectBranch.GetProtectedFilePatterns()
if len(globs) > 0 {
- _, err := pull_service.CheckFileProtection(oldCommitID, newCommitID, globs, 0, env, gitRepo)
+ _, err := pull_service.CheckFileProtection(oldCommitID, newCommitID, globs, 1, env, gitRepo)
if err != nil {
if !models.IsErrFilePathProtected(err) {
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index b58d4a9010353..bf6625e7b01b8 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -291,10 +291,13 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob,
}
}
if len(changedProtectedFiles) >= limit {
- cancel()
- return models.ErrFilePathProtected{
- Path: path,
- }
+ break
+ }
+ }
+
+ if len(changedProtectedFiles) > 0 {
+ return models.ErrFilePathProtected{
+ Path: changedProtectedFiles[0],
}
}
return scanner.Err()
From 62e55c59b2c1ac36193e708b37db632350a83a6b Mon Sep 17 00:00:00 2001
From: Andrew Thornton
Date: Tue, 13 Oct 2020 19:20:02 +0100
Subject: [PATCH 18/18] Remove the check on PR index being greater than 0 as it
unnecessary
Signed-off-by: Andrew Thornton
---
services/pull/patch.go | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/services/pull/patch.go b/services/pull/patch.go
index bf6625e7b01b8..2692d848c433b 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -85,10 +85,8 @@ func TestPatch(pr *models.PullRequest) error {
}
// 3. Check for protected files changes
- if pr.Index != 0 {
- if err = checkPullFilesProtection(pr, gitRepo); err != nil {
- return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
- }
+ if err = checkPullFilesProtection(pr, gitRepo); err != nil {
+ return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
}
if len(pr.ChangedProtectedFiles) > 0 {