Skip to content
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.

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

pull new/attach: Improve handling of the base branch #232

Open
leandro-lucarella-sociomantic opened this issue Sep 28, 2017 · 7 comments
Open

Comments

@leandro-lucarella-sociomantic
Copy link
Contributor

leandro-lucarella-sociomantic commented Sep 28, 2017

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:

  1. Find a best pull base candidate:

    1. If the --base argument was passed, used that, otherwise... (DONE)
    2. If the branch has a tracking branch configured and the remote of the tracking branch matches the hub.upstreamremote, use that, otherwise... (pull: when creating a PR and the tracking branch is used, the repo should match hub.upstreamremote #296).
    3. If there is a hub.pullbase defined, use that, otherwise... (DONE)
    4. Query the GitHub API to get the repository default_branch (Maybe? It might be too much guessing)
  2. Validate the pull base candidate by checking if the branch exists via the GitHub API. (Check the tracking branch points to the upstream repo when using it as the base branch for new pull requests #92)

  3. If it doesn't, quit with an error message asking the user use --base explicitly. (DONE pull new: Error if no base branch can be inferred reliably #174)

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.

@mathias-baumann-sociomantic

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.

@jwilk
Copy link
Contributor

jwilk commented Jan 20, 2018

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.

@mihails-strasuns-sociomantic

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.

@leandro-lucarella-sociomantic
Copy link
Contributor Author

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.

@leandro-lucarella-sociomantic
Copy link
Contributor Author

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

@leandro-lucarella-sociomantic
Copy link
Contributor Author

Issue updated.

@llucax
Copy link
Member

llucax commented Mar 19, 2021

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... 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants