At 2024-07-29 16:09-0400, Jeff King <peff@xxxxxxxx> sent:
Looking into the curl support, I did notice two things:
- unix sockets are only supported for socks proxies. Is it meaningful
to have a path on an http proxy? I don't think so (there is no place
to send it in the "GET" line). But perhaps we should limit the new
code only to socks.
- curl says you should put "localhost" in the host field for this,
though obviously it is not otherwise used. Should we likewise
require that to kick in the code?
Sounds good, I've added checks and tests for both of these cases.
@@ -1265,7 +1266,24 @@ static CURL *get_curl_handle(void)
if (!proxy_auth.host)
die("Invalid proxy URL '%s'", curl_http_proxy);
- curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
+ strbuf_addstr(&proxy, proxy_auth.host);
+ if (proxy_auth.path) {
There's one gotcha here I wondered about: we usually throw away the path
element, unless credential.useHTTPPath is set. That only happens after
we load the per-credential config, though, via credential_apply_config(),
which in turn is triggered by a call to credential_fill(), etc.
I think this is OK here, since we have just called credential_from_url()
ourselves, and the earliest credential_fill() we'd hit is from
init_curl_proxy_auth(), which is called right after the code you're
touching.
Yes, good lookout but I don't think this is a problem either.
+ int path_is_supported = 0;
+ /* curl_version_info was added in curl 7.10 */
+#if LIBCURL_VERSION_NUM >= 0x070a00
+ curl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);
+ path_is_supported = ver->version_num >= 0x075400;
+#endif
We've been trying to keep our curl version #ifdefs in git-curl-compat.h,
with human-readable names. But in this case, I think we can ditch it
entirely, as we require curl 7.21.3 or later already.
Ah, okay, that is good to know! I'll remove the #if.
This will be the first time we check curl_version_info() instead of a
build-time option. I think that makes sense here. Most features require
extra information at build-time (e.g., CURLOPT_* constants), but in this
case it is purely a check on the runtime behavior.
We perhaps could get away with just letting an older curl quietly ignore
the path field, but what you have here is probably a bit friendlier for
users.
Yes, curl has a nasty tendency to quietly ignore a lot of things. I didn't
want users to be uncertain about whether they were using the feature
incorrectly if an older curl was the actual issue.
What if we switched to socks4, which drops all of the auth bits and
supports only IPs, not hostnames (that came in socks4a). This is the
shortest I could come up with (only lightly tested):
Ah, many thanks! I like this much better, and I'm not proficient enough
with Perl to have written it myself.
I can see two paths forward:
a. There's been recent discussion about adding an option for Git to
report the run-time curl version. That doesn't exist yet, though
"git version --build-options" will report the build-time version.
That might be good enough for the purposes of testing a build.
b. Write the test such that it expects the request to work _or_ we see
the "version too old" failure.
Got it, I'll go with option b.
If you usually see success, try running the test script with "--stress"
to create extra load, and you should see failures.
Huh. I never saw any race issues on my machine even with --stress, but
your approach is safer so I'm running with it.
Thank you!
R
