Skip to content

Commit

Permalink
Fix GetCommitBranchStart bug (#33298)
Browse files Browse the repository at this point in the history 
Fix #33265
Fix #33370

This PR also fixes some bugs in `TestGitGeneral`.
  • Loading branch information
@Zettat123
Zettat123 authored Jan 28, 2025
1 parent 8f43313 commit a9577e0
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 30 deletions.
2 changes: 1 addition & 1 deletion modules/git/commit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,5 +357,5 @@ func Test_GetCommitBranchStart(t *testing.T) {
startCommitID, err := repo.GetCommitBranchStart(os.Environ(), "branch1", commit.ID.String())
assert.NoError(t, err)
assert.NotEmpty(t, startCommitID)
assert.EqualValues(t, "9c9aef8dd84e02bc7ec12641deb4c930a7c30185", startCommitID)
assert.EqualValues(t, "95bb4d39648ee7e325106df01a621c530863a653", startCommitID)
}
8 changes: 4 additions & 4 deletions modules/git/repo_commit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,7 @@ func (repo *Repository) AddLastCommitCache(cacheKey, fullName, sha string) error
return nil
}

// GetCommitBranchStart returns the commit where the branch diverged
func (repo *Repository) GetCommitBranchStart(env []string, branch, endCommitID string) (string, error) {
cmd := NewCommand(repo.Ctx, "log", prettyLogFormat)
cmd.AddDynamicArguments(endCommitID)
Expand All @@ -533,19 +534,18 @@ func (repo *Repository) GetCommitBranchStart(env []string, branch, endCommitID s

parts := bytes.Split(bytes.TrimSpace(stdout), []byte{'\n'})

var startCommitID string
// check the commits one by one until we find a commit contained by another branch
// and we think this commit is the divergence point
for _, commitID := range parts {
branches, err := repo.getBranches(env, string(commitID), 2)
if err != nil {
return "", err
}
for _, b := range branches {
if b != branch {
return startCommitID, nil
return string(commitID), nil
}
}

startCommitID = string(commitID)
}

return "", nil
Expand Down
69 changes: 44 additions & 25 deletions tests/integration/git_general_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func testGitGeneral(t *testing.T, u *url.URL) {
mediaTest(t, &httpContext, pushedFilesStandard[0], pushedFilesStandard[1], pushedFilesLFS[0], pushedFilesLFS[1])

t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head"))
t.Run("CreateProtectedBranch", doCreateProtectedBranch(&httpContext, dstPath))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath))
t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath))
t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge"))
Expand Down Expand Up @@ -121,6 +122,7 @@ func testGitGeneral(t *testing.T, u *url.URL) {
mediaTest(t, &sshContext, pushedFilesStandard[0], pushedFilesStandard[1], pushedFilesLFS[0], pushedFilesLFS[1])

t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2"))
t.Run("CreateProtectedBranch", doCreateProtectedBranch(&sshContext, dstPath))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath))
t.Run("MergeFork", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
Expand Down Expand Up @@ -325,6 +327,34 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin
return filepath.Base(tmpFile.Name()), err
}

func doCreateProtectedBranch(baseCtx *APITestContext, dstPath string) func(t *testing.T) {
return func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
ctx := NewAPITestContext(t, baseCtx.Username, baseCtx.Reponame, auth_model.AccessTokenScopeWriteRepository)

t.Run("ProtectBranchWithFilePatterns", doProtectBranch(ctx, "release-*", baseCtx.Username, "", "", "config*"))

// push a new branch without any new commits
t.Run("CreateProtectedBranch-NoChanges", doGitCreateBranch(dstPath, "release-v1.0"))
t.Run("PushProtectedBranch-NoChanges", doGitPushTestRepository(dstPath, "origin", "release-v1.0"))
t.Run("CheckoutMaster-NoChanges", doGitCheckoutBranch(dstPath, "master"))

// push a new branch with a new unprotected file
t.Run("CreateProtectedBranch-UnprotectedFile", doGitCreateBranch(dstPath, "release-v2.0"))
_, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "abc.txt")
assert.NoError(t, err)
t.Run("PushProtectedBranch-UnprotectedFile", doGitPushTestRepository(dstPath, "origin", "release-v2.0"))
t.Run("CheckoutMaster-UnprotectedFile", doGitCheckoutBranch(dstPath, "master"))

// push a new branch with a new protected file
t.Run("CreateProtectedBranch-ProtectedFile", doGitCreateBranch(dstPath, "release-v3.0"))
_, err = generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "config")
assert.NoError(t, err)
t.Run("PushProtectedBranch-ProtectedFile", doGitPushTestRepositoryFail(dstPath, "origin", "release-v3.0"))
t.Run("CheckoutMaster-ProtectedFile", doGitCheckoutBranch(dstPath, "master"))
}
}

func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *testing.T) {
return func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
Expand All @@ -334,27 +364,23 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
ctx := NewAPITestContext(t, baseCtx.Username, baseCtx.Reponame, auth_model.AccessTokenScopeWriteRepository)

// Protect branch without any whitelisting
t.Run("ProtectBranchNoWhitelist", func(t *testing.T) {
doProtectBranch(ctx, "protected", "", "", "")
})
t.Run("ProtectBranchNoWhitelist", doProtectBranch(ctx, "protected", "", "", "", ""))

// Try to push without permissions, which should fail
t.Run("TryPushWithoutPermissions", func(t *testing.T) {
_, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "branch-data-file-")
assert.NoError(t, err)
doGitPushTestRepositoryFail(dstPath, "origin", "protected")
doGitPushTestRepositoryFail(dstPath, "origin", "protected")(t)
})

// Set up permissions for normal push but not force push
t.Run("SetupNormalPushPermissions", func(t *testing.T) {
doProtectBranch(ctx, "protected", baseCtx.Username, "", "")
})
t.Run("SetupNormalPushPermissions", doProtectBranch(ctx, "protected", baseCtx.Username, "", "", ""))

// Normal push should work
t.Run("NormalPushWithPermissions", func(t *testing.T) {
_, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "branch-data-file-")
assert.NoError(t, err)
doGitPushTestRepository(dstPath, "origin", "protected")
doGitPushTestRepository(dstPath, "origin", "protected")(t)
})

// Try to force push without force push permissions, which should fail
Expand All @@ -364,30 +390,22 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
_, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "branch-data-file-new")
assert.NoError(t, err)
})
doGitPushTestRepositoryFail(dstPath, "-f", "origin", "protected")
doGitPushTestRepositoryFail(dstPath, "-f", "origin", "protected")(t)
})

// Set up permissions for force push but not normal push
t.Run("SetupForcePushPermissions", func(t *testing.T) {
doProtectBranch(ctx, "protected", "", baseCtx.Username, "")
})
t.Run("SetupForcePushPermissions", doProtectBranch(ctx, "protected", "", baseCtx.Username, "", ""))

// Try to force push without normal push permissions, which should fail
t.Run("ForcePushWithoutNormalPermissions", func(t *testing.T) {
doGitPushTestRepositoryFail(dstPath, "-f", "origin", "protected")
})
t.Run("ForcePushWithoutNormalPermissions", doGitPushTestRepositoryFail(dstPath, "-f", "origin", "protected"))

// Set up permissions for normal and force push (both are required to force push)
t.Run("SetupNormalAndForcePushPermissions", func(t *testing.T) {
doProtectBranch(ctx, "protected", baseCtx.Username, baseCtx.Username, "")
})
t.Run("SetupNormalAndForcePushPermissions", doProtectBranch(ctx, "protected", baseCtx.Username, baseCtx.Username, "", ""))

// Force push should now work
t.Run("ForcePushWithPermissions", func(t *testing.T) {
doGitPushTestRepository(dstPath, "-f", "origin", "protected")
})
t.Run("ForcePushWithPermissions", doGitPushTestRepository(dstPath, "-f", "origin", "protected"))

t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected", "", "", ""))
t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected", "", "", "", ""))
t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "protected:unprotected"))
var pr api.PullRequest
var err error
Expand All @@ -409,14 +427,14 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
t.Run("MergePR", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr.Index))
t.Run("PullProtected", doGitPull(dstPath, "origin", "protected"))

t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", "", "", "unprotected-file-*"))
t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", "", "", "unprotected-file-*", ""))
t.Run("GenerateCommit", func(t *testing.T) {
_, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "[email protected]", "User Two", "unprotected-file-")
assert.NoError(t, err)
})
t.Run("PushUnprotectedFilesToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected"))

t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username, "", ""))
t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username, "", "", ""))

t.Run("CheckoutMaster", doGitCheckoutBranch(dstPath, "master"))
t.Run("CreateBranchForced", doGitCreateBranch(dstPath, "toforce"))
Expand All @@ -431,7 +449,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
}
}

func doProtectBranch(ctx APITestContext, branch, userToWhitelistPush, userToWhitelistForcePush, unprotectedFilePatterns string) func(t *testing.T) {
func doProtectBranch(ctx APITestContext, branch, userToWhitelistPush, userToWhitelistForcePush, unprotectedFilePatterns, protectedFilePatterns string) func(t *testing.T) {
// We are going to just use the owner to set the protection.
return func(t *testing.T) {
csrf := GetUserCSRFToken(t, ctx.Session)
Expand All @@ -440,6 +458,7 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelistPush, userToWhit
"_csrf": csrf,
"rule_name": branch,
"unprotected_file_patterns": unprotectedFilePatterns,
"protected_file_patterns": protectedFilePatterns,
}

if userToWhitelistPush != "" {
Expand Down

0 comments on commit a9577e0

Please sign in to comment.