From 28a95e890eb6d3e47c5090cb8b7fb2e66b037c55 Mon Sep 17 00:00:00 2001 From: Ulbe van der Werf Date: Thu, 11 Sep 2025 17:19:29 +0200 Subject: [PATCH] Retarget branches to next branch in chain, rather than defaultBranch Previously, RETARGET_CHILDREN_ON_MERGE incorrectly retargeted all following PR's to the defaultBranch when merging a PR in the middle of a chain. Fixes #35138 --- services/pull/pull.go | 24 ++++----- services/repository/branch.go | 9 ++++ tests/integration/pull_merge_test.go | 80 ++++++++++++++++++++-------- 3 files changed, 77 insertions(+), 36 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 20d33b53311c7..b1973eab7162c 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -643,10 +643,10 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { return err } -// retargetBranchPulls change target branch for all pull requests whose base branch is the branch -// Both branch and targetBranch must be in the same repo (for security reasons) -func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { - prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) +// RetargetBranchPulls change target branch for all pull requests whose base branch is oldBranch +// Both oldBranch and newTargetBranch must be in the same repo (for security reasons) +func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, oldBranch, newTargetBranch string) error { + prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, oldBranch) if err != nil { return err } @@ -659,7 +659,7 @@ func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 for _, pr := range prs { if err = pr.Issue.LoadRepo(ctx); err != nil { errs = append(errs, err) - } else if err = ChangeTargetBranch(ctx, pr, doer, targetBranch); err != nil && + } else if err = ChangeTargetBranch(ctx, pr, doer, newTargetBranch); err != nil && !issues_model.IsErrIssueIsClosed(err) && !IsErrPullRequestHasMerged(err) && !issues_model.IsErrPullRequestAlreadyExists(err) { errs = append(errs, err) @@ -668,9 +668,10 @@ func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 return errors.Join(errs...) } -// AdjustPullsCausedByBranchDeleted close all the pull requests who's head branch is the branch -// Or Close all the plls who's base branch is the branch if setting.Repository.PullRequest.RetargetChildrenOnMerge is false. -// If it's true, Retarget all these pulls to the default branch. +// AdjustPullsCausedByBranchDeleted close all the pull requests with the deleted branch as head +// Retarget all PR's with the deleted branch as target to the default branch +// We don't handle setting.Repository.PullRequest.RetargetChildrenOnMerge here, +// because we would need the PR that was merged to determine which branch to redirect to func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branch string) error { // branch as head branch prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch) @@ -698,12 +699,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User } } } - - if setting.Repository.PullRequest.RetargetChildrenOnMerge { - if err := retargetBranchPulls(ctx, doer, repo.ID, branch, repo.DefaultBranch); err != nil { - log.Error("retargetBranchPulls failed: %v", err) - errs = append(errs, err) - } + if len(errs) > 0 { return errors.Join(errs...) } diff --git a/services/repository/branch.go b/services/repository/branch.go index df7202227aa70..1af2c20161197 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -27,11 +27,13 @@ import ( "code.gitea.io/gitea/modules/queue" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/reqctx" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" actions_service "code.gitea.io/gitea/services/actions" notify_service "code.gitea.io/gitea/services/notify" + pull_service "code.gitea.io/gitea/services/pull" release_service "code.gitea.io/gitea/services/release" files_service "code.gitea.io/gitea/services/repository/files" @@ -538,6 +540,13 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R } if err := db.WithTx(ctx, func(ctx context.Context) error { + if pr != nil && setting.Repository.PullRequest.RetargetChildrenOnMerge { + if err := pull_service.RetargetBranchPulls(ctx, doer, repo.ID, branchName, pr.BaseBranch); err != nil { + log.Error("retargetBranchPulls failed: %v", err) + return err + } + } + if !notExist { if err := git_model.AddDeletedBranch(ctx, repo.ID, branchName, doer.ID); err != nil { return err diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 56395a5ba245b..69a2e2e695ac0 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -538,35 +538,71 @@ func TestConflictChecking(t *testing.T) { func TestPullRetargetChildOnBranchDelete(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") - testEditFileToNewBranch(t, session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n") - testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") - testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n(Edited - TestPullRetargetOnCleanup - child PR)") + session := loginUser(t, "user2") + testEditFileToNewBranch(t, session, "user2", "repo1", "master", "branch-1", "README.md", "1") + testEditFileToNewBranch(t, session, "user2", "repo1", "branch-1", "branch-2", "README.md", "2") + testEditFileToNewBranch(t, session, "user2", "repo1", "branch-2", "branch-3", "README.md", "3") + testEditFileToNewBranch(t, session, "user2", "repo1", "branch-3", "branch-4", "README.md", "4") - respBasePR := testPullCreate(t, session, "user2", "repo1", true, "master", "base-pr", "Base Pull Request") - elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/") - assert.Equal(t, "pulls", elemBasePR[3]) + respPR1 := testPullCreate(t, session, "user2", "repo1", false, "master", "branch-1", "PR branch-1 > master") + respPR2 := testPullCreate(t, session, "user2", "repo1", false, "branch-1", "branch-2", "PR branch-2 > branch-1") + respPR3 := testPullCreate(t, session, "user2", "repo1", false, "branch-2", "branch-3", "PR branch-3 > branch-2") + respPR4 := testPullCreate(t, session, "user2", "repo1", false, "branch-3", "branch-4", "PR branch-4 > branch-3") - respChildPR := testPullCreate(t, session, "user1", "repo1", false, "base-pr", "child-pr", "Child Pull Request") - 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) + elemPR2 := strings.Split(test.RedirectURL(respPR2), "/") + testPullMerge(t, session, elemPR2[1], elemPR2[2], elemPR2[4], repo_model.MergeStyleMerge, 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"}) - assert.True(t, branchBasePR.IsDeleted) + mergedPR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch-2"}) + assert.True(t, mergedPR.IsDeleted) + { + // Check PR branch-1 > master is unchanged + req := NewRequest(t, "GET", test.RedirectURL(respPR1)) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + + assert.Equal(t, "master", targetBranch) + assert.Equal(t, "Open", prStatus) + } + { + // Check PR branch-2 > branch-1 is merged + req := NewRequest(t, "GET", test.RedirectURL(respPR2)) + resp := session.MakeRequest(t, req, http.StatusOK) - // Check child PR - req := NewRequest(t, "GET", test.RedirectURL(respChildPR)) - resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) - htmlDoc := NewHTMLParser(t, resp.Body) - targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() - prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + assert.Empty(t, targetBranch) + assert.Equal(t, "Merged", prStatus) + } + { + // Check PR branch-3 > branch-2 is rerouted to branch-1 + req := NewRequest(t, "GET", test.RedirectURL(respPR3)) + resp := session.MakeRequest(t, req, http.StatusOK) - assert.Equal(t, "master", targetBranch) - assert.Equal(t, "Open", prStatus) + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + + assert.Equal(t, "branch-1", targetBranch) + assert.Equal(t, "Open", prStatus) + } + { + // Check PR branch-4 > branch-3 is unchanged + req := NewRequest(t, "GET", test.RedirectURL(respPR4)) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + + assert.Equal(t, "branch-3", targetBranch) + assert.Equal(t, "Open", prStatus) + } }) }