At 2024-07-26 09:29-0700, Junio C Hamano <gitster@xxxxxxxxx> 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
