Hub: feature request: command to interact with a PR's fork branch

[ljharb]

I'd love a command like hub fork-add 123 remoteName that would look up PR 123, add its upstream under "remoteName" (defaulting to "fork", ideally), run git fetch remoteName --prune, and then check out the PR branch's remote ref.

Similarly, hub fork-push 123 that would take the current HEAD and force push it to the fork's branch for PR 123 - even with only that, I could also start out with just git checkout origin/pr/123, make my changes, and hub fork-push 123 -f :-D

fork-push would obviously only work if "allow edits" was checked or I had permissions, but the normal git checks could handle that.

(cc @kytrinyx, from a convo today at sustainOSS)

[kytrinyx]

I've definitely felt the pain of the multiple steps to push up to a contributor's PR. I want to think about what the commands should be named, as fork-add and fork-push seem like they don't fit (philosophically speaking) with the other hub commands. I need to think about this a bit to see if I can come up with something I like.

[mislav]

@ljharb These are all great ideas. Hub already tries to support the "triangular" flow for contributing to OSS projects, but it isn't completely there yet.

Do you feel that these need to be dedicated commands built into hub, or that hub should expose a scripting interface that would allow you to make a simple bash script that does this?

[ljharb]

@mislav I think that either would work for my purposes, but it'll be extremely easier to teach co-collaborators this flow if its all built-in.

[mislav]

Right now there is a feature in master that tries to help with flow of pushing back to a modifiable fork that has sent a PR:

1. Suppose that kytrinyx has forked mislav/example, created a feature branch, and sent a PR #123;

2. When I do hub checkout http://github.com/mislav/example/pull/123 (or hub pr checkout 123), this will create a new local branch for me called kytrinyx-feature and its upstream configuration will be made to push and pull from feature branch on http://github.com/kytrinyx/example.git;

3. When I'm on kytrinyx-feature now, I can create commits and attempt to git push. However, this won't work with the default value for git config push.default being "simple" since git 2.0, which refuses to push to upstream branches named differently than the local branch (in this case, feature vs. kytrinyx-feature).

It seems like all that we have to do to "fix" this workflow is to make git checkout <pr-url> name the local branch identically to the one in the fork. Does that sound sane to everybody else?

The identical naming won't always be possible, though. Somebody could have sent a PR from their master branch in their fork, for example, but there would already be a master branch locally.

[kytrinyx]

It seems like all that we have to do to "fix" this workflow is to make git checkout name the local branch identically to the one in the fork. Does that sound sane to everybody else?

That sounds like a good plan.

The identical naming won't always be possible, though. Somebody could have sent a PR from their master branch in their fork, for example, but there would already be a master branch locally.

I get this a lot. Could we make a great error message that tells them exactly what the command will be to push back up in this case?

[mislav]

@kytrinyx Git already tells you this if branch names don't match:

hub (rouzier-reset-console) $ git push
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push https://github.com/rouzier/hub.git HEAD:reset-console

To push to the branch of the same name on the remote, use

    git push https://github.com/rouzier/hub.git rouzier-reset-console

Therefore, all we would have to do in hub is to match the name if possible, and if not possible then generate a new name for the branch, and have people get this message from git.

[kytrinyx]

@mislav this is excellent. I think this would be a significant improvement. I'd be happy to put some open source Friday hours towards fixing this, if your plate is full.

[ljharb]

Can i use hub checkout and provide a PR number without providing the full URL? ie, in a repo, hub checkout pr/123 or similar? if so, then I think the fix you're providing definitely helps my use case :-)

edit: I see you have hub pr checkout 123 but it seems like i'd need hub checkout pr/1 to make it work; which then doesn't create the upstream. Is hub pr checkout unreleased?

[njsmith]

@mislav just directed me here after I filed #1500, asking for less name matching :-).

I also want pushing back to PRs to work, it's pretty much the whole point of pr checkout for me personally. But I just don't see how using the fork's branch name would work. It's very common to have multiple PRs open at the same time with patch-1 as their branch name, and even if you don't have an actual collision the branch names are often meaningless to anyone else. So I'm imagining that I'd end up with lost in a bunch of local branches I don't recognize, and a significant fraction of the time it still wouldn't work to push back to the pr. This doesn't feel like the basis for a top-notch workflow.

What I really do is use magit for pushes, and for magit the problem isn't the branch name or push.default, all that is fine, but then it gets confused by remote = <raw url>: https://github.com/magit/magit/issues/3116
Hopefully they'll fix that... I guess until then it might be useful if there were an option for pr checkout to configure a named remote for each PR, but this is really only a workaround.

On the command line, it seems like configuring push.default = upstream might be a possible short term workaround? I haven't tried it yet.

In the longer run for the command line, it seems like the optimal UX would be to have unique/locally meaningful names (like pr-123-{slugified PR title}, say) and to have a bare git push push changes back to the PR fork. What would we need to do to make that happen? One possibility would be to have some way to override push.default = simple on a per-branch basis ("yes, I know this branch has a different local name than remote name"), and then pr checkout could set configuration when it's setting up the branch. This seems like something that git upstream could potentially be willing to do, and also something that hub could potentially implement itself? I dunno, just throwing an idea out there.

What do you think?

[njsmith]

Oh, and the other issue I ran into with bare git push is that the repository I was testing on had remote.pushDefault = origin, so the first time I tried to push back to a PR, I instead made a new branch in the original repo. From squinting at the git-config docs it looks like what's happening here might be that pr checkout is only configuring branch.*.remote, and maybe it should also configure branch.*.pushRemote. I can open a separate issue for this if you like.

[ljharb]

To be honest, I don't actually even want a local branch created in the first place. When I do this manually, I check out the ref SHA directly, rebase and edit it as desired, and then explicitly push it to the fork's branch.

[kytrinyx]

it just occurred to me that we could name the branch however, as long as we --set-upstream on it to match the correct upstream (fork/feature or whatever). Then pushing should "just work" (famous last words).

I don't actually even want a local branch created in the first place.

My suggestion doesn't take this into account at all, of course. @ljharb would you write down the actual commands that you use in your preferred flow? I want to make sure we're all on the same page here.

[njsmith]

--set-upstream

My impression was that this still runs into the push.default issue? That's just from squinting at the docs though, I find this part of git incredibly confusing.

Incidentally, magit fixed their bug mentioned up thread, so that's now an option for some of us.

[ljharb]

@kytrinyx Sure!

setfork () 
{ 
    echo "existing fork: $(git remote get-url fork)";
    echo git remote remove fork;
    git remote remove fork;
    echo git remote add --no-tags --fetch fork [email protected]:${1}.git;
    git remote add --no-tags --fetch fork [email protected]:${1}.git
}

cd $repo_in_question
git fetch source # "source" is the repo receiving the PR. This includes all the /pr/123 refs, but this is incidental
git checkout source/pr/123 # whatever the PR number is
# rebase, make my changes, etc
setfork forkauthor/reponame # manually look at the PR and read out the fork author's username and repo name
git push fork HEAD:$PR_branch_name -f # manually look at the PR and read out the fork's branch name
git fetch source # update the local source/pr/123 ref

My ideal flow would be:

hub checkout 123
# rebase, make my changes, etc
hub push-pr 123 -f
git checkout master # or whatever

If it must make a local branch, then:

hub checkout -b 123 # sets the upstream
# rebase, make my changes, etc
git push -f
git checkout master # or whatever
git branch -D $local_PR_branch_name # this step would be annoying if it made a local branch

[kytrinyx]

My impression was that this still runs into the push.default issue?

@njsmith ... maybe? I'm pretty confused myself. I think you can set this on a per-branch basis, though.

@ljharb I really like the workflow without the local branch. It seems like we should be able to implement that without adding a separate command to hub, but I think I need to think that through more thoroughly.

[mislav]

it just occurred to me that we could name the branch however, as long as we --set-upstream on it to match the correct upstream

This is what hub does currently in the latest prerelease, but is still not enough, since git's default options are to refuse pushing to a remote branch of a different name.

[kytrinyx]

but is still not enough, since git's default options are to refuse pushing to a remote branch of a different name.

Damn you're right. I was so sure that I had seen --set-upstream used such that you didn't have to have the same name, but I realize that I always push with the specific name mine:upstream.

[ljharb]

For this, couldn't hub push with mine:upstream tho, since it knows which remote the PR is on?

[jorisvandenbossche]

After #1764, the name of the local branch is now by default the branch name the contributor used for his/her PR, so that git push "just works" also for the default setting of push.default == simple.

However, that also results in an often uninformative name local branch (contributors don't always give their branches meaningful names + there is no indication from which fork or PR this is coming).

Is there a possibility to have this optional? (or a way to configure this?)
I personally use push.default upstream, so before when the forkusername-branchname pattern was used for the local branch, this was working nicely for me, and also giving a more meaningful name. Of course not everybody uses push.default upstream, so I understand that it is nice git push also just works for the default settings, but therefore the question whether there could be somehow an option to use/give another branch name.

[mislav]

@jorisvandenbossche There is no way to configure this, except passing your own branch name explicitly:

$ hub pr checkout 123 my-branch
$ hub checkout <URL> my-branch

Ultimately, you're right in your assessment. I did consciously make a call to sacrifice a branch naming scheme that was better suited for push.default upstream to one that was better suited for push.default simple. I'm not sure if there should be a configuration option to flip back and forth; i.e. would it be worth the development+maintenance overhead. Perhaps there could be a --branch-format='{owner}-pr-{number}-{branch} flag to these commands so a person like you can define their own naming scheme. It would be unwieldy to type out in casual use, but when we support per-command default configuration across the board, perhaps a person could set up that flag to always be present by default.