[do not merge] use a pure node implementation of git ls-remote

[adamburgess]

continued on from #94 (which used a loosely cobbled together npm:js-git and npm:nogit), this one uses a library I just made, npm:git-ls-remote-pure. If you like, I can inline it in this PR instead of using an external library - it's a single file.
implementation is quite small, it fetches https://github.com/user/repo.git/info/refs?service=git-upload-pack and parses the response, which is what actual git does. this means no more git! (though jspm registry still uses git...)

now, onto the problems
the https git endpoint needs a username and password combo for private repositories (where password can/should be a token).
this was broken by #90 - and note, native git will also fail here.

this can be fixed in two ways

• ask, once again, for username and token. Use token specifically for API requests, and use username + token for the locate hook.
• do a one-time lookup /user and then read the login property, which contains the token's username

[guybedford]

Are you saying that #90 has broken the token case for downloading private Git? If so we should file a separate bug for that!!

With the protocol implementation, if you're not using it for anything else it may make sense to check it in here.

My worry with the approach is that it only provides https as the protocol, while I was under the impression we currently use the git: protocol? Perhaps that is where the token-based approach works?

[guybedford]

This is excellent work though - I'd love to get something in along these lines and in other places if it can work out. I guess sharing through a single package makes sense if it is used by the registry as well.

[adamburgess]

Are you saying that #90 has broken the token case for downloading private Git?

yes.

My worry with the approach is that it only provides https as the protocol, while I was under the impression we currently use the git: protocol?

Git has 3 methods: git+ssh (with pub/private keys), git+https (dumb), and git+https (smart).
Github has disabled the dumb protocol. The smart protocol is extremely similar to the ssh protocol but using http authentication (this is what we use currently). It's also just as fast.

This PR implements one function of the smart protocol in javascript.
(sorry if you already knew about git, explaining it anyway is easy)

I'll inline my code, will make bugs easier to fix if they pop up.

[adamburgess]

I think this should be all working now, I've tested with private repos (using username + token).

And yeah, you do need username. there are a few articles on github, e.g. https://help.github.com/articles/git-automation-with-oauth-tokens/ which specifically include username

[tamird]

Yeah, looks like this is a limitation of the git protocol. Note that this isn't Github's API, this is a Git operation.

Note that per discussion in #97 this has absolutely nothing to do API limits, private repos, or anything else. The only reason private repos are relevant here is because they're the only ones that need any authentication at all for doing git operations.

Now, all that said, this use of git ls-remote can be replaced with a single Github API call (which should not require a username, only a token): https://developer.github.com/v3/git/refs/#get-all-references

here's the result for this repo: https://api.github.com/repos/jspm/github/git/refs

Perhaps that's worth considering.

[adamburgess]

Now, all that said, this use of git ls-remote can be replaced with a single Github API call

I believe that the reason the git protocol was used is because that it is faster than the API, at least that is what I thought this comment means: #66 (comment)

[guybedford]

Thanks @tamird the idea here is to move away from API calls entirely as they get rate limited. git remote-ls doesn't seem to be rate limited at all from what I can tell.

[adamburgess]

Ah yes - if the API was used, by default that would be 60 requests per hour unauthenticated. That would pretty much force everyone to use a token.

[guybedford]

So yes, I guess we will need to bring back the username in the authentication process... #99.

[guybedford]

Actually what if we used remote-ls for unauthenticated, and then the API for when authenticated??

[tamird]

I'm not sure what that comment means, but if we only want tags, there is also https://api.github.com/repos/jspm/github/git/refs/tags.

Git operations aren't rate limited? Seems hard to believe, but may it is so.

@guybedford since we can make a single API call to get the user name, why can't we just do it for the user instead of having them enter it (which is unusual for tools using oauth)?

[tamird]

@guybedford yeah, that'd work too.

[guybedford]

@adamburgess am I right in thinking merging this would be blocked by #99 due to the API needing a username, but apart from that we'd be good to merge?

[adamburgess]

Yep, I think this would be ok to merge, apart from it completely failing if
an auth token is set. I'd like to work on getting a fix for that, though. I
should have some time tomorrow.

On 22 Jul 2016 1:17 AM, "Guy Bedford" notifications@github.com wrote:

@adamburgess https://github.com/adamburgess am I right in thinking
merging this would be blocked by #99
#99 due to the API needing a
username, but apart from that we'd be good to merge?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHILKL0P3rG1B45LxBT049SDTXvYkh2mks5qX42hgaJpZM4JDxmV
.

[guybedford]

Sure, thanks for the update. I've invited you as a collaborator so you can merge as you see fit.

[adamburgess]

It's taking a little bit of time to do this, mainly the handling of errors. Also, now that the API is involved, it'd make sense to get caching in as well. Thanks for inviting me! I'll probably get some tests in the PR too in that case, though I'm unsure of how exactly to to test private repos

[guybedford]

That sounds like a great plan. Please just copy me in when you'd like for code review.

[guybedford]

@adamburgess I'm wondering if we should just revert the previous authentication change. Do you think this would make sense?

[tamird]

Reverting the previous authentication change so that we can use a pure-node implementation of the git protocol? That seems backwards.

The previous authentication change allowed users to avoid giving real access to their repos to JSPM. You seem to be suggesting trading security for "cleanliness", which is a bad trade in my mind.

[guybedford]

@tamird this is purely about the fact that we have a regression with private GitHub accounts, as described in the first response in #95 (comment).

[tamird]

Ah, so the other option is to move that operation to the API as described in

#95 (comment)
#95 (comment)

I can work on that if you'd like. I really would like to avoid reverting the authentication changes.

[guybedford]

As discussed, the worry with the API approach is that it means introducing rate limiting for lookups and it will at least triple the bandwidth for lookups, which can already be quite large (eg try seeing what the output is for Bootstrap for all versions...).

[guybedford]

@tamird could we not add the same PR we did to 0.16, to allow username and token or just token both as valid auth options?

[guybedford]

We've actually dropped NodeJS v0 support in jspm 0.17 now.

[adamburgess]

👍

[guybedford]

@adamburgess thanks so much for the update here. I definitely like the suggestion of username and token, with a possible automatic lookup - worth thinking about.

Are these changes compatible / complementary with the PR at #102?

[adamburgess]

username + password doesn't work for API (using plain HTTP authentication), so no. but I think it's a small change to make to ask for username + token

oh and FYI: when this finally gets merged I'll cut the entire thing down to just a few commits in total, not 23 ;)

[guybedford]

Merged! Thanks for your great work here.

[tamird]

hm. sounded like @adamburgess wasn't quite ready for this to be merged.

[adamburgess]

wait--

._.

the title is [do not merge]

[guybedford]

@adamburgess ah.

[guybedford]

@adamburgess you have commit access, so feel free to clean up the branch as you see fit.

[tamird]

@adamburgess since you have commit you should set up travis, too.

On Tue, Aug 2, 2016 at 10:25 AM, Guy Bedford notifications@github.com
wrote:

@adamburgess https://github.com/adamburgess you have commit access, so
feel free to clean up the branch as you see fit.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPEomGAHaTkxgYbPmLbJeD6wLu1owks5qb1NOgaJpZM4JDxmV
.

[adamburgess]

let's just hope no one cloned the 0.17 branch in the interim :)

[tamird]

@adamburgess it's probably not a good idea to force-push. You'll have to
live with the dirty history :(

On Tue, Aug 2, 2016 at 10:30 AM, Adam Burgess notifications@github.com
wrote:

let's just hope no one cloned the 0.17 branch in the interim :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPGiVpEYO1LMOK4jafunuvKUYp0yuks5qb1R0gaJpZM4JDxmV
.

[adamburgess]

sigh, fine

how embarassing...the PR was mostly done, apart from error messages which are mostly unhelpful

[guybedford]

It's only been 10 minutes, a force push is still ok within this window if you want to clean up the history. I don't mind doing it too quickly if you like.

[adamburgess]

ok, I've done that.
I'll open another PR soon. right now it's a bit too late to continue fixing it, 12:42am and all :)

[guybedford]

Thanks. No problem, when you can!

[guybedford]

@adamburgess no pressure here, but I just wanted to check up if you plan to come back to this? If not I can go through and pull in these changes manually.