"Ryan Hendrickson via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Ryan Hendrickson <ryan.hendrickson@xxxxxxxxxxxx>
>
> The documentation for `http.proxy` describes that option, and the
> environment variables it overrides, as supporting "the syntax understood
> by curl". curl allows SOCKS proxies to use a path to a Unix domain
> socket, like `socks5h://localhost/path/to/socket.sock`. Git should
> therefore include, if present, the path part of the proxy URL in what it
> passes to libcurl.
>
> Signed-off-by: Ryan Hendrickson <ryan.hendrickson@xxxxxxxxxxxx>
> ---
> http: do not ignore proxy path
>
> * Documentation: do I need to add anything?
http.proxy documentation says
The syntax thus is [protocol://][user[:password]@]proxyhost[:port].
but the updated code pays attention to what can come after the
"host[:post]" part, does it not?
> diff --git a/http.c b/http.c
> index 623ed234891..0cd75986a6b 100644
> --- a/http.c
> +++ b/http.c
I am unfamiliar with this code path, so let me follow along aloud.
> @@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)
> if (!proxy_auth.host)
> die("Invalid proxy URL '%s'", curl_http_proxy);
We grabbed the value from the configuration variable (or various
environment variables like $http_proxy) in the curl_http_proxy
variable, and then passed it to credential_from_url() to parse parts
out of [protocol://][user[:password]@]proxyhost[:port][/p/a/t/h].
The parsed result is in proxy_auth struct and there is no hope it
can be usable if the .host member is missing.
> - curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
We used to only use the .host member but the curl_http_proxy could
have had a path after it, held in the .path member.
> + if (proxy_auth.path) {
> + struct strbuf proxy = STRBUF_INIT;
> + strbuf_addf(&proxy, "%s/%s", proxy_auth.host, proxy_auth.path);
> + curl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);
> + strbuf_release(&proxy);
> + } else
> + curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
Style. If "if" side needs {braces} because it consists of multiple
statements, the corresponding "else" side should also have {braces}
around its body, even if it only has a single statement.
If you have the proxy strbuf in a bit wider scope, then the above becomes
if (proxy_auth.path)
strbuf_addf(&proxy, "%s/%s", proxy_auth.host, proxy_auth.path);
else
strbuf_addstr(&proxy, proxy_auth.host);
curl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);
strbuf_release(&proxy);
which might (I am not decided) be easier to follow.
Could existing users have been taking advantage of the fact that the
extra /path at the end of http.proxy (and $http_proxy and friends)
are ignored? For them, this change will appear as a regression.
Other than that (and the lack of any documentation and test
updates), looking good.
Thanks.
