RFC: Added documentation and tests for Pkg.setprotocol #13879
Conversation
kshyatt
added
docs
|
If anyone has any comments, I would be greatly appreciative. If I've not committed any major stupidities then I'm happy for it to be merged as-is. |
| @@ -11559,6 +11559,13 @@ Checkout the `Pkg.dir(pkg)` repo to the branch `branch`. Defaults to checking ou | |||
| Pkg.checkout(pkg) | |||
|
|
|||
| doc""" | |||
| setprotocol!(proto) | |||
There was a problem hiding this comment.
it would be better to put this docstring at the function definition rather than helpdb (we're trying to empty out this file).
|
Fixed as requested. Any other requests? |
|
lgtm, though using git config is potentially confusing since pkg no longer requires having command line git installed |
|
Ok, I'll have a look then to see if the same thing can be done with libgit2. |
|
cc @wildart |
|
I agree with @tkelman, reference to git command can be confusing. BTW, why is global constant used instead of ENV variable? |
|
this doesn't influence anything other than julia's use of libgit2 and it isn't needed immediately at startup so i think an env var would be less useful here |
|
Hmm, so if command line Git won't be relevant, then perhaps the solution might be to remove insteadOf from the documentation for the moment, and then add that functionality to setprotocol!? |
|
depends whether libgit2 respects global git config settings |
|
Does it matter if the user has no way to set them? We could just add a configuration option to make protocol changes happen on all domains, then there's no risk of confusion. |
|
The real problem is that we can't just unilaterally force all git URLs to change protocol – because in general not all git repos support that. GitHub repos all do, but we don't (and shouldn't) require hosting packages on GitHub. |
|
And can't since we already have registered packages that aren't hosted on GitHub. |
|
Sure, but as an option, not by default. We'd just add a second argument to setprotocol! that tells it whether to apply it globally or just to a list of domains, defaulting to ["github.com"]. Edit: Probably a second function might be better. |
|
It feels kind of weird that we'd be reproducing the functionality of Git here in a slightly different and probably weaker form. |
|
True, but we have to do so anyway for the GitHub-specific bit that has already been merged, and it seems silly to do the same thing in two places. Plus we don't want to tread over the global config if we can avoid it since people may be perfectly happy to use git:// on internal-network repositories. |
|
Reproducing a fair amount of functionality that exists in command line git but not at a high level in libgit2 is inevitable if we want to finish moving away from Pkg depending on command line git. |
|
I guess we should allow |
|
What about git:// urls? Can we rewrite them by default? |
|
Only if necessary, and only for packages installed with |
|
Clone no, that was removed during review before. So do we do a test of git:// accessibility on first run? The user is going to run into problems if they move between networks then. |
|
It is not that |
|
Ah, ok. I'm not sure automatic fallback is a safe idea, though, since you risk being man-in-the-middled by someone who blocks the SSH connection. They need to be able to defeat HTTPS, but lots of companies already do that. Maybe I'm being overly paranoid though, I should probably stop talking and start coding. |
|
git protocol is not secure so i think we should move away from it. can this be merged? |
|
👍 |
|
Ok, should I pull the git-config stuff first though? |
|
Did we determine whether libgit2 uses global git config? |
|
It can, I'm not sure whether it does by default, but if not it would be an easy fix I expect. |
|
Actually, since git is yet to be removed I'm going to go ahead and change this to RFC, probably best to leave the documentation consistent anyway. |
LachlanGunn
changed the title
|
Right but we already aren't using it, so it's worth determining whether the command we say to run actually does anything. |
|
Ah, got you. I just tried and yes, it does still work on my ten-day-old copy of Julia. |
RFC: Added documentation and tests for Pkg.setprotocol
As a followup to #11312, I have added documentation and a test for Pkg.setprotocol!()
The test is performed by setting the protocol to "notarealprotocol" before testing Pkg.add(), and trapping the error. This seems the most straightforward way to see what is actually being passed to Git.
The protocol is then manually reset to "https" in order to continue testing Pkg,add().