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.

Sign up for GitHub

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

Fix GetCommitBranchStart bug #33298

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −30
Open

Fix GetCommitBranchStart bug #33298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion modules/git/commit_test.go
View file
Open in desktop
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
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use len(branches) >1 here

if b != branch {
return startCommitID, nil
return string(commitID), nil
Zettat123 marked this conversation as resolved.
Show resolved Hide resolved
}
}

startCommitID = string(commitID)
}

return "", nil
Expand Down
69 changes: 44 additions & 25 deletions tests/integration/git_general_test.go
View file
Open in desktop
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
Loading