Skip to content

Commit 662a44d

Browse files
lunnylunny
authored
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
1 parent 24a595c commit 662a44d

File tree

6 files changed

+114
-18
lines changed

6 files changed

+114
-18
lines changed

‎services/pull/merge_prepare.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ type mergeContext struct {
3232
env []string
3333
}
3434

35+
// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
36+
// This function can only be called with gitcmd.Run()
37+
// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
3538
func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
3639
ctx.outbuf.Reset()
3740
ctx.errbuf.Reset()
@@ -73,7 +76,11 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
7376
}
7477

7578
if expectedHeadCommitID != "" {
76-
trackingCommitID, _, err := mergeCtx.PrepareGitCmd(gitcmd.NewCommand("show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch)).RunStdString(ctx)
79+
trackingCommitID, _, err := gitcmd.NewCommand("show-ref", "--hash").
80+
AddDynamicArguments(git.BranchPrefix + trackingBranch).
81+
WithEnv(mergeCtx.env).
82+
WithDir(mergeCtx.tmpBasePath).
83+
RunStdString(ctx)
7784
if err != nil {
7885
defer cancel()
7986
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‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ type prTmpRepoContext struct {
3737
errbuf *strings.Builder // any use should be preceded by a Reset and preferably after use
3838
}
3939

40+
// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
41+
// This function can only be called with gitcmd.Run()
42+
// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
4043
func (ctx *prTmpRepoContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
4144
ctx.outbuf.Reset()
4245
ctx.errbuf.Reset()

‎tests/integration/pull_merge_test.go‎

Lines changed: 87 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,26 @@ import (
4343
"github.com/stretchr/testify/assert"
4444
)
4545

46-
func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder {
46+
type MergeOptions struct {
47+
Style repo_model.MergeStyle
48+
HeadCommitID string
49+
DeleteBranch bool
50+
}
51+
52+
func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeOptions MergeOptions) *httptest.ResponseRecorder {
4753
req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum))
4854
resp := session.MakeRequest(t, req, http.StatusOK)
4955

5056
htmlDoc := NewHTMLParser(t, resp.Body)
5157
link := path.Join(user, repo, "pulls", pullnum, "merge")
5258

5359
options := map[string]string{
54-
"_csrf": htmlDoc.GetCSRF(),
55-
"do": string(mergeStyle),
60+
"_csrf": htmlDoc.GetCSRF(),
61+
"do": string(mergeOptions.Style),
62+
"head_commit_id": mergeOptions.HeadCommitID,
5663
}
5764

58-
if deleteBranch {
65+
if mergeOptions.DeleteBranch {
5966
options["delete_branch_after_merge"] = "on"
6067
}
6168

@@ -69,6 +76,14 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
6976

7077
assert.Equal(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect)
7178

79+
pullnumInt, err := strconv.ParseInt(pullnum, 10, 64)
80+
assert.NoError(t, err)
81+
repository, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), user, repo)
82+
assert.NoError(t, err)
83+
pull, err := issues_model.GetPullRequestByIndex(t.Context(), repository.ID, pullnumInt)
84+
assert.NoError(t, err)
85+
assert.True(t, pull.HasMerged)
86+
7287
return resp
7388
}
7489

@@ -102,7 +117,10 @@ func TestPullMerge(t *testing.T) {
102117

103118
elem := strings.Split(test.RedirectURL(resp), "/")
104119
assert.Equal(t, "pulls", elem[3])
105-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
120+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
121+
Style: repo_model.MergeStyleMerge,
122+
DeleteBranch: false,
123+
})
106124

107125
hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
108126
assert.NoError(t, err)
@@ -124,7 +142,10 @@ func TestPullRebase(t *testing.T) {
124142

125143
elem := strings.Split(test.RedirectURL(resp), "/")
126144
assert.Equal(t, "pulls", elem[3])
127-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebase, false)
145+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
146+
Style: repo_model.MergeStyleRebase,
147+
DeleteBranch: false,
148+
})
128149

129150
hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
130151
assert.NoError(t, err)
@@ -146,7 +167,10 @@ func TestPullRebaseMerge(t *testing.T) {
146167

147168
elem := strings.Split(test.RedirectURL(resp), "/")
148169
assert.Equal(t, "pulls", elem[3])
149-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebaseMerge, false)
170+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
171+
Style: repo_model.MergeStyleRebaseMerge,
172+
DeleteBranch: false,
173+
})
150174

151175
hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
152176
assert.NoError(t, err)
@@ -169,7 +193,42 @@ func TestPullSquash(t *testing.T) {
169193

170194
elem := strings.Split(test.RedirectURL(resp), "/")
171195
assert.Equal(t, "pulls", elem[3])
172-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false)
196+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
197+
Style: repo_model.MergeStyleSquash,
198+
DeleteBranch: false,
199+
})
200+
201+
hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
202+
assert.NoError(t, err)
203+
assert.Len(t, hookTasks, hookTasksLenBefore+1)
204+
})
205+
}
206+
207+
func TestPullSquashWithHeadCommitID(t *testing.T) {
208+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
209+
hookTasks, err := webhook.HookTasks(t.Context(), 1, 1) // Retrieve previous hook number
210+
assert.NoError(t, err)
211+
hookTasksLenBefore := len(hookTasks)
212+
213+
session := loginUser(t, "user1")
214+
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
215+
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
216+
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")
217+
218+
resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title")
219+
220+
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
221+
headBranch, err := git_model.GetBranch(t.Context(), repo1.ID, "master")
222+
assert.NoError(t, err)
223+
assert.NotNil(t, headBranch)
224+
225+
elem := strings.Split(test.RedirectURL(resp), "/")
226+
assert.Equal(t, "pulls", elem[3])
227+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
228+
Style: repo_model.MergeStyleSquash,
229+
DeleteBranch: false,
230+
HeadCommitID: headBranch.CommitID,
231+
})
173232

174233
hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
175234
assert.NoError(t, err)
@@ -187,7 +246,10 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
187246

188247
elem := strings.Split(test.RedirectURL(resp), "/")
189248
assert.Equal(t, "pulls", elem[3])
190-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
249+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
250+
Style: repo_model.MergeStyleMerge,
251+
DeleteBranch: false,
252+
})
191253

192254
// Check PR branch deletion
193255
resp = testPullCleanUp(t, session, elem[1], elem[2], elem[4])
@@ -556,7 +618,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
556618
elemChildPR := strings.Split(test.RedirectURL(respChildPR), "/")
557619
assert.Equal(t, "pulls", elemChildPR[3])
558620

559-
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
621+
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
622+
Style: repo_model.MergeStyleMerge,
623+
DeleteBranch: true,
624+
})
560625

561626
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
562627
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -592,7 +657,10 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
592657

593658
defer test.MockVariableValue(&setting.Repository.PullRequest.RetargetChildrenOnMerge, false)()
594659

595-
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
660+
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
661+
Style: repo_model.MergeStyleMerge,
662+
DeleteBranch: true,
663+
})
596664

597665
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
598666
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -624,7 +692,10 @@ func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
624692

625693
// user2 has no permission to delete branch of repo user1/repo1
626694
session2 := loginUser(t, "user2")
627-
testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
695+
testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], MergeOptions{
696+
Style: repo_model.MergeStyleMerge,
697+
DeleteBranch: true,
698+
})
628699

629700
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: "repo1"})
630701
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
@@ -672,7 +743,10 @@ func TestPullMergeIndexerNotifier(t *testing.T) {
672743
// merge the pull request
673744
elem := strings.Split(test.RedirectURL(createPullResp), "/")
674745
assert.Equal(t, "pulls", elem[3])
675-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
746+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
747+
Style: repo_model.MergeStyleMerge,
748+
DeleteBranch: false,
749+
})
676750

677751
// check if the issue is closed
678752
issue = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{

‎tests/integration/pull_review_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,10 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
212212
resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "master", "This is a pull title")
213213
elem := strings.Split(test.RedirectURL(resp), "/")
214214
assert.Equal(t, "pulls", elem[3])
215-
testPullMerge(t, user1Session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
215+
testPullMerge(t, user1Session, elem[1], elem[2], elem[4], MergeOptions{
216+
Style: repo_model.MergeStyleMerge,
217+
DeleteBranch: false,
218+
})
216219

217220
// Grab the CSRF token.
218221
req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))

‎tests/integration/repo_activity_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ func TestRepoActivity(t *testing.T) {
2727
resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title")
2828
elem := strings.Split(test.RedirectURL(resp), "/")
2929
assert.Equal(t, "pulls", elem[3])
30-
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
30+
testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
31+
Style: repo_model.MergeStyleMerge,
32+
DeleteBranch: false,
33+
})
3134

3235
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feat/better_readme", "README.md", "Hello, World (Edited Again)\n")
3336
testPullCreate(t, session, "user1", "repo1", false, "master", "feat/better_readme", "This is a pull title")

‎tests/integration/repo_branch_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,13 +218,19 @@ func prepareRepoPR(t *testing.T, baseSession, headSession *TestSession, baseRepo
218218
testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "merged-pr", http.StatusSeeOther)
219219
prID = testCreatePullToDefaultBranch(t, baseSession, baseRepo, headRepo, "merged-pr", "merged pr")
220220
testAPINewFile(t, headSession, headRepo.OwnerName, headRepo.Name, "merged-pr", fmt.Sprintf("new-commit-%s.txt", headRepo.Name), "new-commit")
221-
testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, repo_model.MergeStyleRebaseMerge, false)
221+
testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, MergeOptions{
222+
Style: repo_model.MergeStyleRebaseMerge,
223+
DeleteBranch: false,
224+
})
222225

223226
// create merged PR with deleted branch
224227
testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "merged-pr-deleted", http.StatusSeeOther)
225228
prID = testCreatePullToDefaultBranch(t, baseSession, baseRepo, headRepo, "merged-pr-deleted", "merged pr with deleted branch")
226229
testAPINewFile(t, headSession, headRepo.OwnerName, headRepo.Name, "merged-pr-deleted", fmt.Sprintf("new-commit-%s-2.txt", headRepo.Name), "new-commit")
227-
testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, repo_model.MergeStyleRebaseMerge, true)
230+
testPullMerge(t, baseSession, baseRepo.OwnerName, baseRepo.Name, prID, MergeOptions{
231+
Style: repo_model.MergeStyleRebaseMerge,
232+
DeleteBranch: true,
233+
})
228234
}
229235

230236
func checkRecentlyPushedNewBranches(t *testing.T, session *TestSession, repoPath string, expected []string) {

0 commit comments

Comments
 (0)