Skip to content

Commit df7f1c2

Browse files
lunnylunny
authored
Fix protected branch files detection on pre_receive hook (#31778)
Fix #31738 When pushing a new branch, the old commit is zero. Most git commands cannot recognize the zero commit id. To get the changed files in the push, we need to get the first diverge commit of this branch. In most situations, we could check commits one by one until one commit is contained by another branch. Then we will think that commit is the diverge point. And in a pre-receive hook, this will be more difficult because all commits haven't been merged and they actually stored in a temporary place by git. So we need to bring some envs to let git know the commit exist.
1 parent de175e3 commit df7f1c2

File tree

6 files changed

+81
-14
lines changed

6 files changed

+81
-14
lines changed

‎modules/git/commit_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
package git
55

66
import (
7+
"context"
8+
"os"
79
"path/filepath"
810
"strings"
911
"testing"
@@ -345,3 +347,18 @@ func TestGetCommitFileStatusMerges(t *testing.T) {
345347
assert.Equal(t, commitFileStatus.Removed, expected.Removed)
346348
assert.Equal(t, commitFileStatus.Modified, expected.Modified)
347349
}
350+
351+
func Test_GetCommitBranchStart(t *testing.T) {
352+
bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
353+
repo, err := OpenRepository(context.Background(), bareRepo1Path)
354+
assert.NoError(t, err)
355+
defer repo.Close()
356+
commit, err := repo.GetBranchCommit("branch1")
357+
assert.NoError(t, err)
358+
assert.EqualValues(t, "2839944139e0de9737a044f78b0e4b40d989a9e3", commit.ID.String())
359+
360+
startCommitID, err := repo.GetCommitBranchStart(os.Environ(), "branch1", commit.ID.String())
361+
assert.NoError(t, err)
362+
assert.NotEmpty(t, startCommitID)
363+
assert.EqualValues(t, "9c9aef8dd84e02bc7ec12641deb4c930a7c30185", startCommitID)
364+
}

‎modules/git/diff.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,17 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
271271
}
272272

273273
// GetAffectedFiles returns the affected files between two commits
274-
func GetAffectedFiles(repo *Repository, oldCommitID, newCommitID string, env []string) ([]string, error) {
274+
func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID string, env []string) ([]string, error) {
275+
if oldCommitID == emptySha1ObjectID.String() || oldCommitID == emptySha256ObjectID.String() {
276+
startCommitID, err := repo.GetCommitBranchStart(env, branchName, newCommitID)
277+
if err != nil {
278+
return nil, err
279+
}
280+
if startCommitID == "" {
281+
return nil, fmt.Errorf("cannot find the start commit of %s", newCommitID)
282+
}
283+
oldCommitID = startCommitID
284+
}
275285
stdoutReader, stdoutWriter, err := os.Pipe()
276286
if err != nil {
277287
log.Error("Unable to create os.Pipe for %s", repo.Path)

‎modules/git/repo_commit.go

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package git
77
import (
88
"bytes"
99
"io"
10+
"os"
1011
"strconv"
1112
"strings"
1213

@@ -414,7 +415,7 @@ func (repo *Repository) commitsBefore(id ObjectID, limit int) ([]*Commit, error)
414415

415416
commits := make([]*Commit, 0, len(formattedLog))
416417
for _, commit := range formattedLog {
417-
branches, err := repo.getBranches(commit, 2)
418+
branches, err := repo.getBranches(os.Environ(), commit.ID.String(), 2)
418419
if err != nil {
419420
return nil, err
420421
}
@@ -437,12 +438,15 @@ func (repo *Repository) getCommitsBeforeLimit(id ObjectID, num int) ([]*Commit,
437438
return repo.commitsBefore(id, num)
438439
}
439440

440-
func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) {
441+
func (repo *Repository) getBranches(env []string, commitID string, limit int) ([]string, error) {
441442
if DefaultFeatures().CheckVersionAtLeast("2.7.0") {
442443
stdout, _, err := NewCommand(repo.Ctx, "for-each-ref", "--format=%(refname:strip=2)").
443444
AddOptionFormat("--count=%d", limit).
444-
AddOptionValues("--contains", commit.ID.String(), BranchPrefix).
445-
RunStdString(&RunOpts{Dir: repo.Path})
445+
AddOptionValues("--contains", commitID, BranchPrefix).
446+
RunStdString(&RunOpts{
447+
Dir: repo.Path,
448+
Env: env,
449+
})
446450
if err != nil {
447451
return nil, err
448452
}
@@ -451,7 +455,10 @@ func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error)
451455
return branches, nil
452456
}
453457

454-
stdout, _, err := NewCommand(repo.Ctx, "branch").AddOptionValues("--contains", commit.ID.String()).RunStdString(&RunOpts{Dir: repo.Path})
458+
stdout, _, err := NewCommand(repo.Ctx, "branch").AddOptionValues("--contains", commitID).RunStdString(&RunOpts{
459+
Dir: repo.Path,
460+
Env: env,
461+
})
455462
if err != nil {
456463
return nil, err
457464
}
@@ -513,3 +520,35 @@ func (repo *Repository) AddLastCommitCache(cacheKey, fullName, sha string) error
513520
}
514521
return nil
515522
}
523+
524+
func (repo *Repository) GetCommitBranchStart(env []string, branch, endCommitID string) (string, error) {
525+
cmd := NewCommand(repo.Ctx, "log", prettyLogFormat)
526+
cmd.AddDynamicArguments(endCommitID)
527+
528+
stdout, _, runErr := cmd.RunStdBytes(&RunOpts{
529+
Dir: repo.Path,
530+
Env: env,
531+
})
532+
if runErr != nil {
533+
return "", runErr
534+
}
535+
536+
parts := bytes.Split(bytes.TrimSpace(stdout), []byte{'\n'})
537+
538+
var startCommitID string
539+
for _, commitID := range parts {
540+
branches, err := repo.getBranches(env, string(commitID), 2)
541+
if err != nil {
542+
return "", err
543+
}
544+
for _, b := range branches {
545+
if b != branch {
546+
return startCommitID, nil
547+
}
548+
}
549+
550+
startCommitID = string(commitID)
551+
}
552+
553+
return "", nil
554+
}

‎modules/git/repo_commit_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package git
55

66
import (
7+
"os"
78
"path/filepath"
89
"testing"
910

@@ -31,7 +32,7 @@ func TestRepository_GetCommitBranches(t *testing.T) {
3132
for _, testCase := range testCases {
3233
commit, err := bareRepo1.GetCommit(testCase.CommitID)
3334
assert.NoError(t, err)
34-
branches, err := bareRepo1.getBranches(commit, 2)
35+
branches, err := bareRepo1.getBranches(os.Environ(), commit.ID.String(), 2)
3536
assert.NoError(t, err)
3637
assert.Equal(t, testCase.ExpectedBranches, branches)
3738
}

‎routers/private/hook_pre_receive.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
235235

236236
globs := protectBranch.GetProtectedFilePatterns()
237237
if len(globs) > 0 {
238-
_, err := pull_service.CheckFileProtection(gitRepo, oldCommitID, newCommitID, globs, 1, ctx.env)
238+
_, err := pull_service.CheckFileProtection(gitRepo, branchName, oldCommitID, newCommitID, globs, 1, ctx.env)
239239
if err != nil {
240240
if !models.IsErrFilePathProtected(err) {
241241
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
@@ -293,7 +293,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
293293
// Allow commits that only touch unprotected files
294294
globs := protectBranch.GetUnprotectedFilePatterns()
295295
if len(globs) > 0 {
296-
unprotectedFilesOnly, err := pull_service.CheckUnprotectedFiles(gitRepo, oldCommitID, newCommitID, globs, ctx.env)
296+
unprotectedFilesOnly, err := pull_service.CheckUnprotectedFiles(gitRepo, branchName, oldCommitID, newCommitID, globs, ctx.env)
297297
if err != nil {
298298
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
299299
ctx.JSON(http.StatusInternalServerError, private.Response{

‎services/pull/patch.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -503,11 +503,11 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
503503
}
504504

505505
// CheckFileProtection check file Protection
506-
func CheckFileProtection(repo *git.Repository, oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string) ([]string, error) {
506+
func CheckFileProtection(repo *git.Repository, branchName, oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string) ([]string, error) {
507507
if len(patterns) == 0 {
508508
return nil, nil
509509
}
510-
affectedFiles, err := git.GetAffectedFiles(repo, oldCommitID, newCommitID, env)
510+
affectedFiles, err := git.GetAffectedFiles(repo, branchName, oldCommitID, newCommitID, env)
511511
if err != nil {
512512
return nil, err
513513
}
@@ -533,11 +533,11 @@ func CheckFileProtection(repo *git.Repository, oldCommitID, newCommitID string,
533533
}
534534

535535
// CheckUnprotectedFiles check if the commit only touches unprotected files
536-
func CheckUnprotectedFiles(repo *git.Repository, oldCommitID, newCommitID string, patterns []glob.Glob, env []string) (bool, error) {
536+
func CheckUnprotectedFiles(repo *git.Repository, branchName, oldCommitID, newCommitID string, patterns []glob.Glob, env []string) (bool, error) {
537537
if len(patterns) == 0 {
538538
return false, nil
539539
}
540-
affectedFiles, err := git.GetAffectedFiles(repo, oldCommitID, newCommitID, env)
540+
affectedFiles, err := git.GetAffectedFiles(repo, branchName, oldCommitID, newCommitID, env)
541541
if err != nil {
542542
return false, err
543543
}
@@ -574,7 +574,7 @@ func checkPullFilesProtection(ctx context.Context, pr *issues_model.PullRequest,
574574
return nil
575575
}
576576

577-
pr.ChangedProtectedFiles, err = CheckFileProtection(gitRepo, pr.MergeBase, "tracking", pb.GetProtectedFilePatterns(), 10, os.Environ())
577+
pr.ChangedProtectedFiles, err = CheckFileProtection(gitRepo, pr.HeadBranch, pr.MergeBase, "tracking", pb.GetProtectedFilePatterns(), 10, os.Environ())
578578
if err != nil && !models.IsErrFilePathProtected(err) {
579579
return err
580580
}

0 commit comments

Comments
 (0)