Retarget Pulls Based on Pulls on Merge

[kvaster]

Sometimes you need to work on feature which depends on another feature which is in a review process. In this case you may create PR based on that feature insteadof main branch. Currently such PR will be closed without possibility to reopen in case parent feature will be approved, merged and it's branch will be deleted. Automatic target branch change will make life a lot easier in such cases. Github and Bitbucket behaves in such way.

Example:
PR1: main <- feature 1
PR2: feature <- feature2

Currently merging PR1 and deleteing it's branch will lead to PR2 closed without possibility to reopen without loosing it's review histort.

With this change PR2 will change it's target branch to main (PR2: main <- feature2) after PR1 will be merged and it's branch will be deleted.

fixes #18408

[lunny]

The PRs should be rebased when base branch updated but not when merging or deleting the branch.

[kvaster]

I think I've used wrong words. This PR is about changing PR's target branch. I will change names.

[kvaster]

I've updated descritpion and names inside PR. Hopefully it's more clear now...

[kvaster]

Any chances to discuss possibility of such change ? Other git systems are using such approach and it is really usefull and it breaks nothing... And changes are very small.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[spiffytortoise]

Chiming in since a stale label got added.

Having PRs get their base branch updated when a PR for the base branch is merged like this would be great. My team previously used Bitbucket's equivalent feature often for landing complex features in multiple stages.

[lunny]

I think Gitea should not change the target branch automatically but it could be changed manually.

[kvaster]

Currently you're loosing all PRs discussions in such case. Can it be optional setting for gitea behaviour ?

[kvaster]

Usually you and reviewer's don't care about 'child' branches. And 'option' (I will make PR for option) for automatic target branch change may be useful. Also this PR will have no any significant changes to codebase.

[spiffytortoise]

How would the workflow for manually changing the target branch look? I can think of two ways currently that both have problems:

Example PRs:

main <- feature1
feature1 <- feature2

Manual Scenario 1, merge oldest to newest:

1. Merge feature1 into main. Don't delete the feature1 branch, or else the feature2 PR will close.
2. Change the base branch of feature2's PR from feature1 to main.
3. Delete the feature1 branch.
4. Merge feature2 into main.

It's too easy to delete feature1 at the wrong time and permanently close the feature2 PR by accident. (#18478)

Manual Scenario 2, merge newest to oldest:

1. Merge feature2 into feature1.
2. feature1's PR ends up with feature2's changes in its diff.
3. Merge feature1 into main.

The diff change in step 2 adds a bunch of noise to the PR, which makes it more difficult to understand the PR after.

Auto-Update Scenario, merge oldest to newest:

In comparison, this is how Bitbucket Server works:

1. Merge feature1 into main. Check "Delete feature1 and update the base branch of PRs targeting feature1 to main".
2. Bitbucket atomically updates the base branch of feature2 from feature1 to main, without any additional diff being added or displayed in feature2's PR.
3. Merge feature2 into main.

If using a rebase or rebase-and-merge strategy, the history ends up linear with any of these scenarios.

Why should Gitea not update PR base branches automatically?

[kvaster]

I've updated PR according to upstream.

Any suggestion in what section to add flag for enabling/disabling this feature ?

[kvaster]

One more PR update - make an option for retarget.

[6543]

#18478 (comment)

hmm I think we can add some nice integration tests for mentioned cases

example:

gitea/integrations/git_test.go

Lines 487 to 498 in 9efa471

	t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index))
	t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr))
	t.Run("CheckPR", func(t *testing.T) {
	oldMergeBase := pr.MergeBase
	pr2, err := doAPIGetPullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)(t)
	assert.NoError(t, err)
	assert.Equal(t, oldMergeBase, pr2.MergeBase)
	})
	t.Run("EnsurDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength))
	// Then: Delete the head branch & make sure that doesn't break the PR page or change its diff
	t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch))

....

[lafriks]

Does option is really needed for this? I mean is there a use case where re-targeting would be undesirable?

[6543]

I personaly would not mind if it always try retargeting ...
cc @lunny

[lunny]

I personaly would not mind if it always try retargeting ... cc @lunny

Maybe manually allowing to retarget pulls base branch even if base branch(or base repository) was deleted is better or it's the first step of this auto retarget.

[6543]

@kvaster in any case - we need integration tests, if you need help just tell us (here or chat via discord/matrix)

[kvaster]

Will try to do this myself today and will get back tomorrow if will need any help.

[night-gold]

Hello, is this still an ongoing PR or has it been abandoned?

[kvaster]

Actually we're using this patch already for long time, but it was difficult for me to write tests... However I can do necessary changes to PR itself...

[codecov-commenter]

Codecov Report

Merging #18478 (1e270a4) into main (f521e88) will increase coverage by 0.00%.
The diff coverage is 41.40%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff            @@
##             main   #18478    +/-   ##
========================================
  Coverage   47.14%   47.15%            
========================================
  Files        1149     1155     +6     
  Lines      151446   152431   +985     
========================================
+ Hits        71397    71872   +475     
- Misses      71611    72071   +460     
- Partials     8438     8488    +50     

Impacted Files	Coverage Δ
cmd/dump.go	0.66% <0.00%> (-0.01%)	⬇️
cmd/web.go	0.00% <0.00%> (ø)
models/actions/run.go	1.64% <0.00%> (-0.08%)	⬇️
models/actions/runner.go	1.44% <ø> (ø)
models/packages/package.go	45.45% <0.00%> (-1.13%)	⬇️
models/user/search.go	77.50% <0.00%> (-6.29%)	⬇️
modules/actions/workflows.go	0.00% <0.00%> (ø)
modules/context/context.go	64.54% <0.00%> (-3.53%)	⬇️
modules/doctor/storage.go	30.65% <0.00%> (-1.29%)	⬇️
modules/setting/git.go	45.45% <ø> (ø)
... and 37 more

... and 47 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[zeripath]

Thanks for this PR - sorry it's taking so long to get in.

A few thoughts:

1. Could you explain why this retargeting only occurs if the new target is in the same repo? Is this to copy Github's behaviour?
2. I think it might be better to make this on by default.
3. I suspect we would be better off making this setting part of PullRequestsConfig and allowing repos to set this on/off by themselves.

[kvaster]

Telling the truth, I was not aware about retargeting to another repository, but now I think it might be also usefull.
I also think it will not hurt anybody if it will be on by default. And an option inside repository itself will be even better.

[kvaster]

It was started something long ago. I think I've not prepare code for retargeting to new repository cause ChangeTargetBranch function was already implemented while there was no ChangeTargetBranchAndRepo...

[denyskon]

Closing in favor of #28539. If there is a reason to, feel free to reopen.