Retarget branches to next branch in chain, rather than defaultBranch #35464
Conversation
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 go-gitea#35138
GiteaBot
added
the
lgtm/need 2
| // 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 |
There was a problem hiding this comment.
If these pull requests are not handled, they will because a dead pull request because head branch deleted.
There was a problem hiding this comment.
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 :(
| } | ||
|
|
||
| if err := db.WithTx(ctx, func(ctx context.Context) error { | ||
| if pr != nil && setting.Repository.PullRequest.RetargetChildrenOnMerge { |
There was a problem hiding this comment.
There was a problem hiding this comment.
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 setting RETARGET_CHILDREN_ON_MERGE is true, we should retarget a PR that targets a branch we're merging to the target branch of the branch we're merging.
Previously, RETARGET_CHILDREN_ON_MERGE incorrectly retargeted all following PR's in a chain to the defaultBranch when merging a PR in the middle of a chain.
Fixes #35138
I've moved the RETARGET_CHILDREN_ON_MERGE behavior from
pull/AdjustPullsCausedByBranchDeleted()tobranch/DeleteBranch(), because we need to know which PR is being closed to retarget the child branch(es). Inpull/pull.gowe only know that a branch is being deleted, but not why.I'm not very happy with the dependency, though. So if there's a cleaner option, I'd be happy to move this behavior back to
pull/pull.go.