From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Date: Fri, 26 Jul 2024 17:12:51 GMT
Subject: Re: [PATCH] http: do not ignore proxy path
Message-ID: <6b408a07-f2ff-1e04-5b3a-fd287fb1c151@alum.mit.edu>
In-Reply-To: <xmqqr0bgw1z1.fsf@gitster.g>

At 2024-07-26 09:29-0700, Junio C Hamano <gitster@pobox.com> sent:

> 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?

Correct; I'll add a "[/path]" to that construction.

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

For what it's worth, I was following the precedent set by the if-statement 
starting at line 1256 (a few lines above this patch):

> 		if (strstr(curl_http_proxy, "://"))
> 			credential_from_url(&proxy_auth, curl_http_proxy);
> 		else {
> 			struct strbuf url = STRBUF_INIT;
> 			strbuf_addf(&url, "http://%s", curl_http_proxy);
> 			credential_from_url(&proxy_auth, url.buf);
> 			strbuf_release(&url);
> 		}

<https://github.com/git/git/blob/ad57f148c6b5f8735b62238dda8f571c582e0e54/http.c#L1256>

I have no problem with being inconsistent with surrounding code in these 
style choices; just let me know what I should do in light of that.

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

That is possible, though I have difficulty imagining a scenario in which 
it would be intentional.

What do you recommend I do about that possibility?

R