Fix GetCommitBranchStart bug (go-gitea#33298) (go-gitea#33421)

Backport go-gitea#33298 by Zettat123

Fix go-gitea#33265
Fix go-gitea#33370

This PR also fixes some bugs in `TestGitGeneral`.

---------

Co-authored-by: Zettat123 <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>

Author: 4 people

modules/git/commit_test.go

@@ -360,5 +360,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)
 }

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,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

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, "[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)()
@@ -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
@@ -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
@@ -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"))
@@ -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 != "" {
@@ -465,7 +484,7 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelistPush, userToWhit
 		// Check if master branch has been locked successfully
 		flashCookie := ctx.Session.GetCookie(gitea_context.CookieNameFlash)
 		assert.NotNil(t, flashCookie)
-		assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Brule%2B%2522"+url.QueryEscape(branch)+"%2522%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value)
+		assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Brule%2B%2522"+url.QueryEscape(url.QueryEscape(branch))+"%2522%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value)
 	}
 }