Re: [PATCH] http: do not ignore proxy path

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 26, 2024, 16:29 UTC
Message-ID: <xmqqr0bgw1z1.fsf@gitster.g>
In-Reply-To: <pull.1767.git.1722009170590.gitgitgadget@gmail.com>

"Ryan Hendrickson via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
>
> 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@alum.mit.edu>
> ---
>     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.

Show 7 quoted lines

> +		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.