Add support for lowercase proxy environment variables#4112
Add support for lowercase proxy environment variables#4112ethomson merged 3 commits intolibgit2:masterfrom
Conversation
pks-t
left a comment
There was a problem hiding this comment.
I've got some nits on coding style, would be nice if you could fix them (or argue why your code is better).
The feature itself looks good to me, thanks for this!
| } | ||
|
|
||
| giterr_clear(); | ||
| error = 0; |
There was a problem hiding this comment.
Wouldn't it be easier to just have something like
if (error == GIT_ENOTFOUND)
error = git__getenv(&val, use_ssl ? "https_proxy" : "http_proxy");
This would avoid the duplicated logic around giterr_clear().
|
|
||
| /* In autotag mode, don't overwrite any locally-existing tags */ | ||
| error = git_reference_create(&ref, remote->repo, refname.ptr, &head->oid, !autotag, | ||
| error = git_reference_create(&ref, remote->repo, refname.ptr, &head->oid, !autotag, |
There was a problem hiding this comment.
Given that this is a few hundred lines away from the actual change, I'd avoid unneccessary churn here.
There was a problem hiding this comment.
Right... Atom loves to automatically fix extra spaces for me. :)
|
Oh man. The proxy environment variable mess. So we are obviously completely wrong in our current implementation and we should be ashamed of ourselves. Some interesting notes:
It does not. curl supports every other protocol in both upper and lower case but explicitly not
Meaning, it supports This is for backcompat with lynx and/or libwww (I don't honestly know offhand who loaded the proxy environment variable and I'm certain that I'm not going to go find a copy of the sources But I'm really saying this only to complain. Unless somebody can give me a compelling reason why we shouldn't support However, I do think that we should honor their preference for |
). lynx would look only at the all lower case environment variables curl supports HTTPS_PROXY in addition to https_proxy (and their http counterparts). This change ensures parity with curl's behavior.
069fec7 to
2af282d
Compare
|
I'm not surprised that this stuff is even more complicated than it appears at first glance. 😂 I'm more than happy to flip the preference here. 👍 |
| if (error == GIT_ENOTFOUND) { | ||
| /* try uppercase environment variables */ | ||
| error = git__getenv(&val, use_ssl ? "HTTPS_PROXY" : "HTTP_PROXY"); | ||
| } |
There was a problem hiding this comment.
One minor style nit: we generally avoid braces around one-liners
There was a problem hiding this comment.
Ah, right, sorry bout that. Force of habit. :)
curl supports HTTPS_PROXY in addition to https_proxy (and their http counterparts). This change ensures parity with curl's behavior.