Improved SSH support: non-bare urls, custom ports, and GIT_SSH/plink by sinbad · Pull Request #404 · git-lfs/git-lfs

sinbad · 2015-06-17T10:23:14Z

Full SSH urls e.g. ssh://user@host/repo
Custom ports in both bare and full urls
GIT_SSH environment for alternate ssh clients
Explicit support for plink.exe and TortoisePlink.exe

Depends on #403 (included in PR)

technoweenie · 2015-06-17T20:49:15Z

Can you merge the latest master now that #403 has been merged? Rebase if you like.

technoweenie · 2015-06-17T20:59:27Z

lfs/config.go

I'm not a fan of this giant method. I'd prefer to have something that calls internal functions:

endpoint := Endpoint{Url: urlstr} u, err := url.Parse(urlstr) if err != nil { // right now just {user}@{host}:{path}, but maybe others one day? processSshRemote(urlstr) } switch u.Scheme { case "http", "https": processHttpUrl(u) case "ssh": processSshUrl(u) default: endpoint.Url = ENDPOINT_URL_UNKNOWN }

Note: feel free to override my suggestions on method or variable names.

Minor nitpick: I tend to use rawurl. I don't like having type related suffixes on variable names. Maybe it's from my experience using hungarian notation in VB/Access code :)

OK - rebased and will refactor a little in next commit.

You did VB/Access too eh? We should commiserate over a beer one day ;)

michael-k · 2015-06-18T10:22:44Z

lfs/config.go

I'd prefer early returns.

if u.Scheme != "" || u.Path == "" { return u } // This might be a bare SSH URL // Turn it into ssh:// for simplicity of extraction later parts := strings.Split(u.Path, ":") if len(parts) < 2 { return u } // Treat presence of ':' as a bare URL var newPath string if len(parts) > 2 { // port included; really should only ever be 3 parts newPath = fmt.Sprintf("%v:%v", parts[0], strings.Join(parts[1:], "/")) } else { newPath = strings.Join(parts, "/") } newrawurl := fmt.Sprintf("ssh://%v", newPath) newu, err := url.Parse(newrawurl) if err != nil { return u } return newu

Another possibility is a switch statement:

var newPath string switch { case len(parts) < 2: return u case len(parts) == 2: newPath = strings.Join(parts, "/") default: // port included; really should only ever be 3 parts newPath = fmt.Sprintf("%v:%v", parts[0], strings.Join(parts[1:], "/")) }

I definitely prefer early returns vs multiple levels of nesting. 👍

bozaro · 2015-06-19T06:44:26Z

Without this change, you can not use SSH-connections multiplexing on Windows.
I think this latest update, which can give a appreciable performance boost without using batch api.
It would be nice if it was included in v0.5.2 (#281).

technoweenie · 2015-06-19T15:35:52Z

This PR doesn't look ready, so it likely won't make it in. We can release v0.5.3 whenever we want though.

sinbad · 2015-06-19T15:41:05Z

AFAIK the only thing that's "not ready" about it is stylistic preferences?

technoweenie · 2015-06-19T15:43:53Z

Yes, and a merge conflict with master.

sinbad · 2015-06-19T15:51:04Z

OK, that must be new in the last day or two. Will come back to this next week.

rubyist · 2015-06-19T15:53:30Z

Are we doing ssh multiplexing in this PR? If so, has it been benchmarked with large numbers of large files? It is my understanding that ssh multiplexing starts to fall over when connections with large amounts of data are involved, though I am admittedly unfamiliar with plink.

technoweenie · 2015-06-19T15:55:12Z

There's also another patch I'm holding back because of insufficient testing. We can roll v0.5.3 when these two are ready. I'm anxious to get v0.5.2 out because it's been way too long since the last release.

@rubyist: We may be able to punt on that concern for now until we're reviewing the SSH API.

sinbad · 2015-06-19T16:00:45Z

Are we doing ssh multiplexing in this PR? If so, has it been benchmarked

All this PR does is support ssh:// url forms, custom ports, and the GIT_SSH env var. Its performance characteristics are identical to master since the ssh calls themselves happen exactly the same way, all it does is support cases that would previously fail.

My other PR on full SSH support opens only Config.ConcurrentTransfers() SSH connections to run the API over and pipes everything through those.

rubyist · 2015-06-19T16:03:00Z

@sinbad 👍 that sounds OK then. My concerns were around using something like ssh's ControlMaster (or equivalents in plink, etc), using ConcurrentTransfers() connections with the transfer queue should be just fine.

sinbad · 2015-06-19T16:07:49Z

My concerns were around using something like ssh's ControlMaster (or equivalents in plink, etc), using ConcurrentTransfers() connections with the transfer queue should be just fine.

Yeah I did consider adding another config parameter that could override ConcurrentTransfers() for SSH connections, using a default lower limit but for simplicity I didn't, and the default 3 should be OK for all but the most totalitarian server admin.

bozaro · 2015-06-19T18:16:18Z

Are we doing ssh multiplexing in this PR? If so, has it been benchmarked

I make little benchmark on Windows/Linux with our project repository #376 (4231 LFS files, working copy size: 3.6G, original repository size: ~50G). Local gigabit network environment.

On Linux:
I create simple config ~/.ssh/config:

Host *
    ControlMaster auto
    ControlPath ~/.ssh/controlmasters/%r@%h:%p
    ControlPersist 10m

This changes enable using single SSH connection for all git-lfs-authenticate requests.
Clone time reduced from 1436 sec to 541 sec (2.65X faster, ~0.216 sec per file).

On Windows:
I try to use similar configurations with differ OpenSSH versions. It looks that OpenSSH this feature is not supported on Windows.

A similar function is in Putty (plink.exe), but its use needs support GIT_SSH environment variable (this PR).

The effect of multiplex under Windows, I did not measure.

* Full SSH urls e.g. ssh://user@host/repo * Custom ports in both bare and full urls * GIT_SSH environment for alternate ssh clients * Explicit support for plink.exe and TortoisePlink.exe

sinbad · 2015-06-22T09:50:35Z

Rebased on master & added early-outs, should be ready for inclusion in 0.5.3 now.

michael-k · 2015-06-22T13:52:00Z

lfs/endpoint.go

Should we really return an empty Endpoint in case of err != nil? Why not Endpoint{Url: rawurl}?

Pretty much anything will go through the default: route, but for consistency with the other routes I've returned EndpointUrlUnknown in that case. You have to throw something pretty whacky at url.Parse for it to return an error (none of the tests with bare strings manage it).

…t EndpointUrlUnknown

Refactor some of the code in #404

Backport #404 for v0.5.x: Improved SSH support: non-bare urls, custom ports, and GIT_SSH/plink

sinbad mentioned this pull request Jun 17, 2015

Pure SSH API implementation #378

Closed

technoweenie reviewed Jun 17, 2015
View reviewed changes

bozaro mentioned this pull request Jun 18, 2015

git-lfs ignores GIT_SSH enviroment variable #380

Closed

sinbad force-pushed the ssh_urls branch from 19c0b22 to 88fc1a7 Compare June 18, 2015 09:07

michael-k reviewed Jun 18, 2015
View reviewed changes

technoweenie mentioned this pull request Jun 19, 2015

Release v0.5.2 #281

Closed

38 tasks

This was referenced Jun 19, 2015

Release v0.5.3 #425

Closed

Release v0.6.0 #426

Closed

Improved SSH support: non-bare urls, custom ports, and GIT_SSH/plinks

72795a3

* Full SSH urls e.g. ssh://user@host/repo * Custom ports in both bare and full urls * GIT_SSH environment for alternate ssh clients * Explicit support for plink.exe and TortoisePlink.exe

sinbad force-pushed the ssh_urls branch from a91e1eb to 72795a3 Compare June 22, 2015 09:47

michael-k reviewed Jun 22, 2015
View reviewed changes

In the rare case that url.Parse() returns an error, return an explici…

2892978

…t EndpointUrlUnknown

technoweenie merged commit 2892978 into git-lfs:master Jul 6, 2015

technoweenie added a commit that referenced this pull request Jul 6, 2015

Refactor some of the code in #404

41f1b02

technoweenie mentioned this pull request Jul 6, 2015

Refactor some of the code in #404 #464

Merged

technoweenie added a commit that referenced this pull request Jul 7, 2015

Merge pull request #464 from github/ssh-refactor

a53565a

Refactor some of the code in #404

technoweenie mentioned this pull request Jul 22, 2015

Backport #404 for v0.5.x: Improved SSH support: non-bare urls, custom ports, and GIT_SSH/plink #494

Merged

technoweenie added a commit that referenced this pull request Jul 22, 2015

Backport ssh_urls from #404 to release-0.5

acad586

technoweenie added a commit that referenced this pull request Jul 22, 2015

Refactor some of the code in #404

fddaeb9

technoweenie added a commit that referenced this pull request Jul 22, 2015

Merge pull request #494 from github/release-0.5-backport-404

0f92713

Backport #404 for v0.5.x: Improved SSH support: non-bare urls, custom ports, and GIT_SSH/plink

sinbad deleted the ssh_urls branch August 6, 2015 16:19

Conversation

sinbad commented Jun 17, 2015

Uh oh!

technoweenie commented Jun 17, 2015

Uh oh!

technoweenie Jun 17, 2015

Choose a reason for hiding this comment

Uh oh!

sinbad Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

michael-k Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

michael-k Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

technoweenie Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

bozaro commented Jun 19, 2015

Uh oh!

technoweenie commented Jun 19, 2015

Uh oh!

sinbad commented Jun 19, 2015

Uh oh!

technoweenie commented Jun 19, 2015

Uh oh!

sinbad commented Jun 19, 2015

Uh oh!

rubyist commented Jun 19, 2015

Uh oh!

technoweenie commented Jun 19, 2015

Uh oh!

sinbad commented Jun 19, 2015

Uh oh!

rubyist commented Jun 19, 2015

Uh oh!

sinbad commented Jun 19, 2015

Uh oh!

bozaro commented Jun 19, 2015

Uh oh!

sinbad commented Jun 22, 2015

Uh oh!

michael-k Jun 22, 2015

Choose a reason for hiding this comment

Uh oh!

sinbad Jun 22, 2015

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants