Skip to content

Retarget branches to next branch in chain, rather than defaultBranch #35464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+77 −36
Open

Retarget branches to next branch in chain, rather than defaultBranch #35464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions services/pull/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these pull requests are not handled, they will because a dead pull request because head branch deleted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lunny, I think my comment is a bit unclear.

What I mean to say is: once we're here, we're not going to give PRs any special treatment due to RetargetChildrenOnMerge. Any branch that still has the toDelete branch as head, will be redirected to the default branch.

My comment was only meant to explain why we're not giving those PRs special treatment here.

I think it's best if I remove the last two lines of this comment. The reason I felt the need to explain here, was because this was the place where we used to handle RetargetChildrenOnMerge. Now that we handle it in the PR-merge code-path, there's no reason for anyone to expect us to handle it on generic branch close.

In summary: I think the code's behavior is what we want (retarget PRs to the default branch when the head branch is deleted), and this comment (line 673 and 674) is confusing and should be deleted. Do you agree?

PS Sorry for the double-post. I was logged in to the wrong account :(

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)
Expand Down Expand Up @@ -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...)
}

Expand Down
9 changes: 9 additions & 0 deletions services/repository/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated with https://github.com/go-gitea/gitea/pull/35464/files#diff-5ebfb9a229f4898ff6b01962f04fbdb5470ed217ab2b24ff5610e607ab42234bR707

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. I think something may have gone wrong in the diff viewer?

This block was moved from
https://github.com/go-gitea/gitea/pull/35464/files#diff-5ebfb9a229f4898ff6b01962f04fbdb5470ed217ab2b24ff5610e607ab42234bL702-L708
to
https://github.com/go-gitea/gitea/pull/35464/files#diff-de304b990c14c1ced400e6d0871f95fd15610dc606c4c85e9ec2d7b808b29956R543-R548

Or am I missing something?

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
Expand Down
80 changes: 58 additions & 22 deletions tests/integration/pull_merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}

Expand Down