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

Author: Zermelo-Ulbe

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...)
 	}
 

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

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)
+		}
 	})
 }