git-hub
git-hub copied to clipboard
pull new/attach: Improve handling of the base branch
We already have a few issues regarding on how the base branch is handled when creating pull requests.
One problem is following the tracking branch blindly. Another is to fallback to hub.pullbase
(a somehow hardcoded value) blindly.
The common problem seems to be not verifying whatever we do with GitHub first.
I think to come up with a holistic solution for this problem we should do the following to find out what the pull base should be:
-
Find a best pull base candidate:
-
If the(DONE)--base
argument was passed, used that, otherwise... - If the branch has a tracking branch configured and the remote of the tracking branch matches the
hub.upstreamremote
, use that, otherwise... (#296). -
If there is a(DONE)hub.pullbase
defined, use that, otherwise... - Query the GitHub API to get the repository
default_branch
(Maybe? It might be too much guessing)
-
-
Validate the pull base candidate by checking if the branch exists via the GitHub API.(#92) -
If it doesn't, quit with an error message asking the user use(DONE #174)--base
explicitly.
Validation should happen before the anything is pushed or the user is asked to edit a message.
@jwilk @mathias-baumann-sociomantic please confirm this covers the issues you created.
I think my ticket was referring to the scenario where the whole upstream repo doesn't exist, not just a branch. But I suspect this would solve it either way.
Sounds good to me; feel free to close #174 in favor of this bug.
There's one corner case this doesn't handle well:
if upstream changed default branch after I cloned the repo, and I didn't notice the change, and I don't have tracking branch set, git-hub would make a PR against the new default branch, which is unexpected.
But maybe I should just set branch.autoSetupMerge=always
, so that the tracking branch is always set.
git-hub would make a PR against the new default branch, which is unexpected
This actually sounds correct to me. If default branch was changed, most likely new PR based from old default one will have to be rebased on the new branch. At least that would be the case for our repos.
I agree, for me it would not be all that unexpected to get PRs done against the default branch if there is no other configuration. The only other sane alternative I can think of is to remove step 1.iv. and just give an error if no branch was specified. Maybe that is a slightly more annoying option for the cases where what you want is to use the repo's default branch, but it at least guarantees no surprises, which is always a good thing.
This actually sounds correct to me. If default branch was changed, most likely new PR based from old default one will have to be rebased on the new branch. At least that would be the case for our repos.
BTW, I agree with this too, but given there is a most likely means that in some cases we'll screw up if we assume this. So I think we should go with the safe option...
Issue updated.
There was some movement towards this. Now there is no hardcoded default branch to use as base (as of #174 - v2.1.0). We are still missing verification that a branch exists against GitHub.
OTOH, people use different workflows, and I'm starting to think that using the tracking branch, or the default branch implicitly can always lead to surprises for certain use cases. Maybe to keep the usability boost of having useful defaults we can just add an option to disable guessing and always requiring --base
for users affected by this.
All that said, creating a PR against a wrong branch is not all that bad as now the base branch can be easily changed... :thinking: