From 6fdab1a911ffcb73d2cd6ffdfb41ff932f41216a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 8 Oct 2025 09:43:16 -0700 Subject: [PATCH 1/5] Fix merge panic --- services/pull/merge_prepare.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 7dedf0d2a017a..4a3b14bdecde3 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -73,7 +73,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) From 195afb93b3e8305a65b6bf6ad1b89e650f5aa08b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 8 Oct 2025 09:45:17 -0700 Subject: [PATCH 2/5] add comment --- services/pull/merge_prepare.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 4a3b14bdecde3..07935ac16d498 100644 --- a/services/pull/merge_prepare.go +++ b/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() From 1183b33a7f3bbfe4e14700a06aebc39b98a8cf58 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 8 Oct 2025 09:53:06 -0700 Subject: [PATCH 3/5] add comment --- services/pull/temp_repo.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 597a4aa48cf5a..4f7a504b11d2c 100644 --- a/services/pull/temp_repo.go +++ b/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() From 50d3d948c5333695445761656cf39f584bfb9c98 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 8 Oct 2025 22:02:39 -0700 Subject: [PATCH 4/5] Fix merge test --- tests/integration/pull_merge_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 7670aebab5349..441f4f07781f0 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -69,6 +69,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 } From 763d8c882ed1554d2782d394e0366c0d58fb74f5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 9 Oct 2025 18:11:24 -0700 Subject: [PATCH 5/5] Add test for merging with a special head commit id --- tests/integration/pull_merge_test.go | 92 +++++++++++++++++++++---- tests/integration/pull_review_test.go | 5 +- tests/integration/repo_activity_test.go | 5 +- tests/integration/repo_branch_test.go | 10 ++- 4 files changed, 95 insertions(+), 17 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 441f4f07781f0..3345216838987 100644 --- a/tests/integration/pull_merge_test.go +++ b/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(), - "do": string(mergeStyle), + "_csrf": htmlDoc.GetCSRF(), + "do": string(mergeOptions.Style), + "head_commit_id": mergeOptions.HeadCommitID, } - if deleteBranch { + if mergeOptions.DeleteBranch { options["delete_branch_after_merge"] = "on" } @@ -110,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) @@ -132,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) @@ -154,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) @@ -177,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) @@ -195,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]) @@ -564,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"}) @@ -600,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"}) @@ -632,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"}) @@ -680,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{ diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 73a40c9440649..67dd023a8aa1a 100644 --- a/tests/integration/pull_review_test.go +++ b/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])) diff --git a/tests/integration/repo_activity_test.go b/tests/integration/repo_activity_test.go index d5025decba078..7781fd0511ced 100644 --- a/tests/integration/repo_activity_test.go +++ b/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") diff --git a/tests/integration/repo_branch_test.go b/tests/integration/repo_branch_test.go index 379cf568028cb..666ae44c08347 100644 --- a/tests/integration/repo_branch_test.go +++ b/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) {