From a9577e0808d8432f58a24901eb87ffc88bdc4182 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 28 Jan 2025 10:59:15 +0800 Subject: [PATCH] Fix `GetCommitBranchStart` bug (#33298) Fix #33265 Fix #33370 This PR also fixes some bugs in `TestGitGeneral`. --- modules/git/commit_test.go | 2 +- modules/git/repo_commit.go | 8 ++-- tests/integration/git_general_test.go | 69 +++++++++++++++++---------- 3 files changed, 49 insertions(+), 30 deletions(-) diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 9560c2cd94..9a87dd32d9 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -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) } diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 647894bb21..02d8e163e4 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -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) @@ -533,7 +534,8 @@ 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 { @@ -541,11 +543,9 @@ func (repo *Repository) GetCommitBranchStart(env []string, branch, endCommitID s } for _, b := range branches { if b != branch { - return startCommitID, nil + return string(commitID), nil } } - - startCommitID = string(commitID) } return "", nil diff --git a/tests/integration/git_general_test.go b/tests/integration/git_general_test.go index 6dd29a438a..ab2a948fde 100644 --- a/tests/integration/git_general_test.go +++ b/tests/integration/git_general_test.go @@ -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")) @@ -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)() @@ -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, "user2@example.com", "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, "user2@example.com", "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)() @@ -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, "user2@example.com", "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, "user2@example.com", "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 @@ -364,30 +390,22 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes _, err := generateCommitWithNewData(testFileSizeSmall, dstPath, "user2@example.com", "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 @@ -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, "user2@example.com", "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")) @@ -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) @@ -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 != "" {