Repository navigation
Add support for lowercase proxy environment variables - #4112
ethomson merged 3 commits into
Conversation
Sorry, something went wrong.
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!
Sorry, something went wrong.
| } | ||
|
|
||
| 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().
Sorry, something went wrong.
There was a problem hiding this comment.
Good call, done.
Sorry, something went wrong.
|
|
||
| /* 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Right... Atom loves to automatically fix extra spaces for me. :)
Sorry, something went wrong.
|
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 HTTP_PROXY. From curl(1):
Meaning, it supports https_proxy and HTTPS_PROXY, but for http supports only http_proxy. (This is true as of my curl 7.43.0, anyway.) 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 HTTP_PROXY (all caps) then I don't care about strict compatibility here because this whole mess is quite dumb already. However, I do think that we should honor their preference for http_proxy first and HTTP_PROXY second, if you don't mind flipping the priority in which you look these things up. |
). lynx would look only at the all lower case environment variables http_proxy, ftp_proxy, etc, to everyone's consternation. So if we were going to pick one of those (all lower or all upper case) then we probably chose the wrong one for compatibility.
Sorry, something went wrong.
curl supports HTTPS_PROXY in addition to https_proxy (and their http counterparts). This change ensures parity with curl's behavior.
|
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. 👍 |
Sorry, something went wrong.
| 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
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, right, sorry bout that. Force of habit. :)
Sorry, something went wrong.
There was a problem hiding this comment.
👍
Sorry, something went wrong.
curl supports HTTPS_PROXY in addition to https_proxy (and their http counterparts). This change ensures parity with curl's behavior.