Fix merge panic (#35606)

To prevent potential bugs, the logic in #35543 makes `gitcmd.Command`
panic when attempting to override stdout or stderr. Instead of using
`PrepareCmd`, this PR now uses the WithXXX methods directly to avoid the
panic.

Fix #35603

services/pull/merge_prepare.go

@@ -32,6 +32,9 @@ type mergeContext struct {
 	env       []string
 }
 
+// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
+// This function can only be called with gitcmd.Run()
+// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
 func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()
@@ -73,7 +76,11 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 	}
 
 	if expectedHeadCommitID != "" {
-		trackingCommitID, _, err := mergeCtx.PrepareGitCmd(gitcmd.NewCommand("show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch)).RunStdString(ctx)
+		trackingCommitID, _, err := gitcmd.NewCommand("show-ref", "--hash").
+			AddDynamicArguments(git.BranchPrefix + trackingBranch).
+			WithEnv(mergeCtx.env).
+			WithDir(mergeCtx.tmpBasePath).
+			RunStdString(ctx)
 		if err != nil {
 			defer cancel()
 			log.Error("failed to get sha of head branch in %-v: show-ref[%s] --hash refs/heads/tracking: %v", mergeCtx.pr, mergeCtx.tmpBasePath, err)

services/pull/temp_repo.go

@@ -37,6 +37,9 @@ type prTmpRepoContext struct {
 	errbuf      *strings.Builder // any use should be preceded by a Reset and preferably after use
 }
 
+// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
+// This function can only be called with gitcmd.Run()
+// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
 func (ctx *prTmpRepoContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()

tests/integration/pull_merge_test.go

@@ -43,7 +43,13 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder {
+type MergeOptions struct {
+	Style        repo_model.MergeStyle
+	HeadCommitID string
+	DeleteBranch bool
+}
+
+func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeOptions MergeOptions) *httptest.ResponseRecorder {
 	req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum))
 	resp := session.MakeRequest(t, req, http.StatusOK)
 
@@ -51,11 +57,12 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
 	link := path.Join(user, repo, "pulls", pullnum, "merge")
 
 	options := map[string]string{
-		"_csrf": htmlDoc.GetCSRF(),
+		"_csrf":          htmlDoc.GetCSRF(),
-		"do":    string(mergeStyle),
+		"do":             string(mergeOptions.Style),
+		"head_commit_id": mergeOptions.HeadCommitID,
 	}
 
-	if deleteBranch {
+	if mergeOptions.DeleteBranch {
 		options["delete_branch_after_merge"] = "on"
 	}
 
@@ -69,6 +76,14 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
 
 	assert.Equal(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect)
 
+	pullnumInt, err := strconv.ParseInt(pullnum, 10, 64)
+	assert.NoError(t, err)
+	repository, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), user, repo)
+	assert.NoError(t, err)
+	pull, err := issues_model.GetPullRequestByIndex(t.Context(), repository.ID, pullnumInt)
+	assert.NoError(t, err)
+	assert.True(t, pull.HasMerged)
+
 	return resp
 }
 
@@ -102,7 +117,10 @@ func TestPullMerge(t *testing.T) {
 
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: false,
+		})
 
 		hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
 		assert.NoError(t, err)
@@ -124,7 +142,10 @@ func TestPullRebase(t *testing.T) {
 
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebase, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleRebase,
+			DeleteBranch: false,
+		})
 
 		hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
 		assert.NoError(t, err)
@@ -146,7 +167,10 @@ func TestPullRebaseMerge(t *testing.T) {
 
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebaseMerge, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleRebaseMerge,
+			DeleteBranch: false,
+		})
 
 		hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
 		assert.NoError(t, err)
@@ -169,7 +193,42 @@ func TestPullSquash(t *testing.T) {
 
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleSquash,
+			DeleteBranch: false,
+		})
+
+		hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
+		assert.NoError(t, err)
+		assert.Len(t, hookTasks, hookTasksLenBefore+1)
+	})
+}
+
+func TestPullSquashWithHeadCommitID(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		hookTasks, err := webhook.HookTasks(t.Context(), 1, 1) // Retrieve previous hook number
+		assert.NoError(t, err)
+		hookTasksLenBefore := len(hookTasks)
+
+		session := loginUser(t, "user1")
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
+		testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
+		testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")
+
+		resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title")
+
+		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
+		headBranch, err := git_model.GetBranch(t.Context(), repo1.ID, "master")
+		assert.NoError(t, err)
+		assert.NotNil(t, headBranch)
+
+		elem := strings.Split(test.RedirectURL(resp), "/")
+		assert.Equal(t, "pulls", elem[3])
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleSquash,
+			DeleteBranch: false,
+			HeadCommitID: headBranch.CommitID,
+		})
 
 		hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
 		assert.NoError(t, err)
@@ -187,7 +246,10 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
 
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: false,
+		})
 
 		// Check PR branch deletion
 		resp = testPullCleanUp(t, session, elem[1], elem[2], elem[4])
@@ -556,7 +618,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
 		elemChildPR := strings.Split(test.RedirectURL(respChildPR), "/")
 		assert.Equal(t, "pulls", elemChildPR[3])
 
-		testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+		testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: true,
+		})
 
 		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
 		branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -592,7 +657,10 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
 
 		defer test.MockVariableValue(&setting.Repository.PullRequest.RetargetChildrenOnMerge, false)()
 
-		testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+		testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: true,
+		})
 
 		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
 		branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -624,7 +692,10 @@ func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
 
 		// user2 has no permission to delete branch of repo user1/repo1
 		session2 := loginUser(t, "user2")
-		testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+		testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: true,
+		})
 
 		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: "repo1"})
 		branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -672,7 +743,10 @@ func TestPullMergeIndexerNotifier(t *testing.T) {
 		// merge the pull request
 		elem := strings.Split(test.RedirectURL(createPullResp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: false,
+		})
 
 		// check if the issue is closed
 		issue = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{

tests/integration/pull_review_test.go

@@ -212,7 +212,10 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
 			resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "master", "This is a pull title")
 			elem := strings.Split(test.RedirectURL(resp), "/")
 			assert.Equal(t, "pulls", elem[3])
-			testPullMerge(t, user1Session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+			testPullMerge(t, user1Session, elem[1], elem[2], elem[4], MergeOptions{
+				Style:        repo_model.MergeStyleMerge,
+				DeleteBranch: false,
+			})
 
 			// Grab the CSRF token.
 			req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))

tests/integration/repo_activity_test.go

@@ -27,7 +27,10 @@ func TestRepoActivity(t *testing.T) {
 		resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title")
 		elem := strings.Split(test.RedirectURL(resp), "/")
 		assert.Equal(t, "pulls", elem[3])
-		testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			Style:        repo_model.MergeStyleMerge,
+			DeleteBranch: false,
+		})
 
 		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feat/better_readme", "README.md", "Hello, World (Edited Again)\n")
 		testPullCreate(t, session, "user1", "repo1", false, "master", "feat/better_readme", "This is a pull title")

tests/integration/repo_branch_test.go

@@ -218,13 +218,19 @@ func prepareRepoPR(t *testing.T, baseSession, headSession *TestSession, baseRepo
 	testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "merged-pr", http.StatusSeeOther)
 	prID = testCreatePullToDefaultBranch(t, baseSession, baseRepo, headRepo, "merged-pr", "merged pr")
 	testAPINewFile(t, headSession, headRepo.OwnerName, headRepo.Name, "merged-pr", fmt.Sprintf("new-commit-%s.txt", headRepo.Name), "new-commit")
-	testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, repo_model.MergeStyleRebaseMerge, false)
+	testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, MergeOptions{
+		Style:        repo_model.MergeStyleRebaseMerge,
+		DeleteBranch: false,
+	})
 
 	// create merged PR with deleted branch
 	testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "merged-pr-deleted", http.StatusSeeOther)
 	prID = testCreatePullToDefaultBranch(t, baseSession, baseRepo, headRepo, "merged-pr-deleted", "merged pr with deleted branch")
 	testAPINewFile(t, headSession, headRepo.OwnerName, headRepo.Name, "merged-pr-deleted", fmt.Sprintf("new-commit-%s-2.txt", headRepo.Name), "new-commit")
-	testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, repo_model.MergeStyleRebaseMerge, true)
+	testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, MergeOptions{
+		Style:        repo_model.MergeStyleRebaseMerge,
+		DeleteBranch: true,
+	})
 }
 
 func checkRecentlyPushedNewBranches(t *testing.T, session *TestSession, repoPath string, expected []string) {