Set fork remotes using ssh instead of https

[lukechilds]

Proposed in #540

This allows you to push changes to your fork without being prompted for your GitHub credentials every time.

Previous behaviour:

$ git fork tj/commander.js
Enter your github username
lukechilds
Enter host password for user 'lukechilds':
{
  ...
}
Cloning into 'commander.js'...
remote: Counting objects: 1791, done.
remote: Total 1791 (delta 0), reused 0 (delta 0), pack-reused 1791
Receiving objects: 100% (1791/1791), 381.24 KiB | 677.00 KiB/s, done.
Resolving deltas: 100% (977/977), done.
Checking connectivity... done.
From https://github.com/tj/commander.js
 * [new branch]      gh-pages   -> upstream/gh-pages
 * [new branch]      master     -> upstream/master

$ cd commander.js && git push
Username for 'https://github.com':

New behaviour:

$ git fork tj/commander.js
Enter your github username
lukechilds
Enter host password for user 'lukechilds':
{
  ...
}
Cloning into 'commander.js'...
remote: Counting objects: 1791, done.
remote: Total 1791 (delta 0), reused 0 (delta 0), pack-reused 1791
Receiving objects: 100% (1791/1791), 381.24 KiB | 309.00 KiB/s, done.
Resolving deltas: 100% (977/977), done.
Checking connectivity... done.
From github.com:tj/commander.js
 * [new branch]      gh-pages   -> upstream/gh-pages
 * [new branch]      master     -> upstream/master

$ cd commander.js && git push
Everything up-to-date

[qw3rtman]

I'm good with this.

@nicolaiskogheim, @hemanth, @spacewander?

[nicolaiskogheim]

This will make it less of a hassle for people who have ssh set up, but it will be a problem for those who haven't.

So this is actually a breakage for http users. The forking works, but cloning doesn't.

An alternative would be to use ssh if the argument to git fork is an ssh-url, and default to https. And we could have an option --ssh/-s to force ssh.

Either way, I'm not positive to this particular change.

[lukechilds]

@nicolaiskogheim Fair enough, is this something you want me to work on then or are you not bothered?

I assume most people would use the shorthand git fork user/repo so checking for ssh/https in the argument probably wouldn't solve much.

The flag would work but seems kind of annoying to have to type every time. What do you reckon?

[nicolaiskogheim]

I just got an idea.

We could run something like ssh -T [email protected], and if it works it's safe to assume the ssh protocol.

Be sure to test it in an environment where ssh access isn't configured. I tend to pull out Vagrant for stuff like that.

If the user doesn't use ssh with github, we wouldn't want to add github to .ssh/known_hosts, and we don't want the user to answer any prompts from ssh.

You might get something like

The authenticity of host 'github.com (192.30.252.1)' can't be established.
RSA key fingerprint is 16:27:ac:a5:76:28:2d:36:63:1b:56:4d:eb:df:a6:48.
Are you sure you want to continue connecting (yes/no)?

from ssh, but to avoid that you can do (and redirect all output while we're at it):

yes no | ssh -T [email protected] >/dev/null 2>&1

There probably is a flag you can pass to ssh instead of the yes no | ... stuff.

is this something you want me to work on

Yes :)

[lukechilds]

Nice, I like it.

I'll have a play tomorrow.

[lukechilds]

@qw3rtman @nicolaiskogheim Ok, I've updated this to check if we can login via ssh, if we can, ssh remotes are used, if not it falls back to https.

I've added the exit command to ssh and check for both 0 and 1 exit codes in bash so that if (for whatever reason) GitHub ever allow interactive ssh sessions in the future this will still work and won't hang the terminal.

The only caveat is that if the user has ssh set up for GitHub but they're on a new machine and don't have github.com in known_hosts this will treat it as a fail. This isn't a big deal as it's unlikely to happen and if it did it'll just revert to https so everything will still work ok.

I've done this on purpose, we could actually bypass known_hosts and not save the fingerprint by adding the args -o StrictHostKeyChecking=no -o "UserKnownHostsFile /dev/null", however I don't think we should do this as we could potentially open users up to a MITM attack.

Also updated docs to reflect the new behaviour.

[lukechilds]

Just a thought, I could remove -o BatchMode=yes from ssh and then git fork will prompt them to add github.com to known_hosts if it isn't already in there. All other ssh output will be hidden.

[lukechilds]

Oops, looks like atom removed a load of unnecessary white space when I saved. Happy to split this out into a separate commit (or just remove it) if you want.

[nicolaiskogheim]

This is fine.

[nicolaiskogheim]

Looks great. I gave it a test run with and without ssh configured, and it worked as I expected.
When the man page is updated to reflect the change (especially the example there), this is good to go.

[lukechilds]

Sweet, my lunch break is over now but I'll get that updated when I get home 👍

[nicolaiskogheim]

Just a thought, I could remove -o BatchMode=yes from ssh and then git fork will prompt them to add github.com to known_hosts if it isn't already in there.

My guess is that if github.com isn't in known_hosts, then they probably haven't configured ssh with github.com either, and a silent fail is appropriate.

[lukechilds]

This should be all sorted now 🎉

[qw3rtman]

Maybe change this to:

The ssh remote will be used if you have used an ssh remote with GitHub in the past. Otherwise, the https remote will be used.

[nicolaiskogheim]

if you have used an ssh remote with GitHub in the past

This isn't necessarily accurate.

I'm fine with the way it is.

[qw3rtman]

I think it looks great except for the one stylistic suggestion I made. @nicolaiskogheim, thoughts?

Also, could you regenerate the documentation pages (see this)?

[lukechilds]

I thought I did regenerate the docs in f22d7bc, or is something wrong?

It's late here and I'm tucked up in bed, I'll update the man page tomorrow morning 👍

[qw3rtman]

No worries. It seems like git-fork.html has been deleted so you may need to regenerate that.

[nicolaiskogheim]

The html file is missing :)

Otherwise, LGTM.

EDIT: Forgot to refresh the tab. Didn't see your comment there @qw3rtman :)

EDIT 2: There was more comments that I didn't see. Reading up now.

[nicolaiskogheim]

I wrote that last comment on an old version this page. Apart from the missing git-fork.html this looks good to me.

[lukechilds]

Had to delete git-fork.html for ronn to pick it up ¯_(ツ)_/¯

Should be sorted now ✨

[nicolaiskogheim]

Thanks, @lukechilds!