{"thread":{"id":"61852","subject":"[PATCH] http: do not ignore proxy path","startedAt":"2024-07-26T15:52:53Z","lastAt":"2024-08-02T22:14:18Z","messageCount":31,"participants":["Ryan Hendrickson via GitGitGadget","Junio C Hamano","Ryan Hendrickson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499453","messageId":"pull.1767.git.1722009170590.gitgitgadget@gmail.com","threadId":"61852","inReplyTo":null,"subject":"[PATCH] http: do not ignore proxy path","fromName":"Ryan Hendrickson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-26T15:52:50Z","receivedAt":"2024-07-26T15:52:53Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"From: Ryan Hendrickson \n\nThe documentation for `http.proxy` describes that option, and the\nenvironment variables it overrides, as supporting \"the syntax understood\nby curl\". curl allows SOCKS proxies to use a path to a Unix domain\nsocket, like `socks5h://localhost/path/to/socket.sock`. Git should\ntherefore include, if present, the path part of the proxy URL in what it\npasses to libcurl.\n\nSigned-off-by: Ryan Hendrickson \n---\n http: do not ignore proxy path\n \n * Documentation: do I need to add anything?\n * Tests: I could use a pointer on how best to add a test for this.\n Adding a case to t5564-http-proxy.sh seems straightforward but I\n don't think httpd can be configured to listen to domain sockets; can\n I use netcat?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1767%2Frhendric%2Frhendric%2Fhttp-proxy-path-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1767/rhendric/rhendric/http-proxy-path-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1767\n\n http.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 623ed234891..0cd75986a6b 100644\n--- a/http.c\n+++ b/http.c\n@@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)\n \t\tif (!proxy_auth.host)\n \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\tif (proxy_auth.path) {\n+\t\t\tstruct strbuf proxy = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n+\t\t\tstrbuf_release(&proxy);\n+\t\t} else\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\n\nbase-commit: ad57f148c6b5f8735b62238dda8f571c582e0e54\n-- \ngitgitgadget\n"},{"id":"499455","messageId":"xmqqr0bgw1z1.fsf@gitster.g","threadId":"61852","inReplyTo":"pull.1767.git.1722009170590.gitgitgadget@gmail.com","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T16:29:54Z","receivedAt":"2024-07-26T16:29:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ryan Hendrickson via GitGitGadget\" writes:\n\n> From: Ryan Hendrickson \n>\n> The documentation for `http.proxy` describes that option, and the\n> environment variables it overrides, as supporting \"the syntax understood\n> by curl\". curl allows SOCKS proxies to use a path to a Unix domain\n> socket, like `socks5h://localhost/path/to/socket.sock`. Git should\n> therefore include, if present, the path part of the proxy URL in what it\n> passes to libcurl.\n>\n> Signed-off-by: Ryan Hendrickson \n> ---\n> http: do not ignore proxy path\n> \n> * Documentation: do I need to add anything?\n\nhttp.proxy documentation says\n\n The syntax thus is [protocol://][user[:password]@]proxyhost[:port].\n\nbut the updated code pays attention to what can come after the\n\"host[:post]\" part, does it not?\n\n> diff --git a/http.c b/http.c\n> index 623ed234891..0cd75986a6b 100644\n> --- a/http.c\n> +++ b/http.c\n\nI am unfamiliar with this code path, so let me follow along aloud.\n\n> @@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)\n> \t\tif (!proxy_auth.host)\n> \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n\nWe grabbed the value from the configuration variable (or various\nenvironment variables like $http_proxy) in the curl_http_proxy\nvariable, and then passed it to credential_from_url() to parse parts\nout of [protocol://][user[:password]@]proxyhost[:port][/p/a/t/h].\nThe parsed result is in proxy_auth struct and there is no hope it\ncan be usable if the .host member is missing.\n\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n\nWe used to only use the .host member but the curl_http_proxy could\nhave had a path after it, held in the .path member.\n\n> +\t\tif (proxy_auth.path) {\n> +\t\t\tstruct strbuf proxy = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n> +\t\t\tstrbuf_release(&proxy);\n> +\t\t} else\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n\nStyle. If \"if\" side needs {braces} because it consists of multiple\nstatements, the corresponding \"else\" side should also have {braces}\naround its body, even if it only has a single statement.\n\nIf you have the proxy strbuf in a bit wider scope, then the above becomes\n\n\tif (proxy_auth.path)\n\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n\telse\n\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n\tstrbuf_release(&proxy);\n\nwhich might (I am not decided) be easier to follow.\n\nCould existing users have been taking advantage of the fact that the\nextra /path at the end of http.proxy (and $http_proxy and friends)\nare ignored? For them, this change will appear as a regression.\n\nOther than that (and the lack of any documentation and test\nupdates), looking good.\n\nThanks.\n"},{"id":"499456","messageId":"6b408a07-f2ff-1e04-5b3a-fd287fb1c151@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqr0bgw1z1.fsf@gitster.g","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-07-26T17:12:51Z","receivedAt":"2024-07-26T17:13:14Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-07-26 09:29-0700, Junio C Hamano sent:\n\n> http.proxy documentation says\n>\n> The syntax thus is [protocol://][user[:password]@]proxyhost[:port].\n>\n> but the updated code pays attention to what can come after the\n> \"host[:post]\" part, does it not?\n\nCorrect; I'll add a \"[/path]\" to that construction.\n\n>> +\t\tif (proxy_auth.path) {\n>> +\t\t\tstruct strbuf proxy = STRBUF_INIT;\n>> +\t\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n>> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n>> +\t\t\tstrbuf_release(&proxy);\n>> +\t\t} else\n>> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>\n> Style. If \"if\" side needs {braces} because it consists of multiple\n> statements, the corresponding \"else\" side should also have {braces}\n> around its body, even if it only has a single statement.\n>\n> If you have the proxy strbuf in a bit wider scope, then the above becomes\n>\n> \tif (proxy_auth.path)\n> \t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n> \telse\n> \t\tstrbuf_addstr(&proxy, proxy_auth.host);\n> \tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n> \tstrbuf_release(&proxy);\n>\n> which might (I am not decided) be easier to follow.\n\nFor what it's worth, I was following the precedent set by the if-statement \nstarting at line 1256 (a few lines above this patch):\n\n> \t\tif (strstr(curl_http_proxy, \"://\"))\n> \t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> \t\telse {\n> \t\t\tstruct strbuf url = STRBUF_INIT;\n> \t\t\tstrbuf_addf(&url, \"http://%s\", curl_http_proxy);\n> \t\t\tcredential_from_url(&proxy_auth, url.buf);\n> \t\t\tstrbuf_release(&url);\n> \t\t}\n\n\n\nI have no problem with being inconsistent with surrounding code in these \nstyle choices; just let me know what I should do in light of that.\n\n> Could existing users have been taking advantage of the fact that the\n> extra /path at the end of http.proxy (and $http_proxy and friends)\n> are ignored? For them, this change will appear as a regression.\n\nThat is possible, though I have difficulty imagining a scenario in which \nit would be intentional.\n\nWhat do you recommend I do about that possibility?\n\nR\n"},{"id":"499458","messageId":"xmqq5xssvyhi.fsf@gitster.g","threadId":"61852","inReplyTo":"6b408a07-f2ff-1e04-5b3a-fd287fb1c151@alum.mit.edu","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T17:45:13Z","receivedAt":"2024-07-26T17:45:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> For what it's worth, I was following the precedent set by the\n> if-statement starting at line 1256 (a few lines above this patch):\n\nIt is worth nothing. Existing violation does not make it a good\nidea to add more of them. It makes it more work to clean them all\nup later.\n\n>> Could existing users have been taking advantage of the fact that the\n>> extra /path at the end of http.proxy (and $http_proxy and friends)\n>> are ignored? For them, this change will appear as a regression.\n>\n> That is possible, though I have difficulty imagining a scenario in\n> which it would be intentional.\n>\n> What do you recommend I do about that possibility?\n\nI have no idea. I already said that I am not familiar with this\ncode path, and it is likely I am a worse person than you are to find\na potential creative (ab)uses of the mechanism to take advantage of\nthe fact that the current code ignores the path part.\n\nHaving said that, given that http.proxy falls back to environment\nvariables that have been established a lot longer than Git itself,\nlike HTTPS_PROXY etc., if we _know_ that setting HTTPS_PROXY to such\na value with /path part causes curl (or other popular programs) to\ntry using such a value without stripping the path part (and even\nbetter if that causes them to fail), then such an observation would\nmake a very good argument why it is extremely unlikely that existing\nusers used such a setting, hence it is unlikely this patch would\nmake a regression.\n\nThanks.\n"},{"id":"499472","messageId":"20240726211156.GA714986@coredump.intra.peff.net","threadId":"61852","inReplyTo":"pull.1767.git.1722009170590.gitgitgadget@gmail.com","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-26T21:11:56Z","receivedAt":"2024-07-26T21:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2024 at 03:52:50PM +0000, Ryan Hendrickson via GitGitGadget wrote:\n\n> * Tests: I could use a pointer on how best to add a test for this.\n> Adding a case to t5564-http-proxy.sh seems straightforward but I\n> don't think httpd can be configured to listen to domain sockets; can\n> I use netcat?\n\nI don't offhand know of a way to test this without a custom program like\nnetcat. If it's the only option, it's OK to use tools that might not be\navailable everywhere as long as the tests are marked as optional with\nthe appropriate prerequisite. You can find prior art by looking for\ntest_lazy_prereq calls (e.g., the ones for GZIP or ZIPINFO are pretty\nstraight-forward).\n\nI would warn that there are several not-quite-compatible variants of\nnetcat floating around, which can create headaches. You might be better\noff with a short perl script using IO::Socket::UNIX or similar.\n\n> diff --git a/http.c b/http.c\n> index 623ed234891..0cd75986a6b 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)\n> \t\tif (!proxy_auth.host)\n> \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n> \n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n> +\t\tif (proxy_auth.path) {\n> +\t\t\tstruct strbuf proxy = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n> +\t\t\tstrbuf_release(&proxy);\n> +\t\t} else\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n\nThe fields in the proxy_auth struct have been parsed from the url, with\nany url encoding removed. But then we paste them back together into a\npseudo-url without doing any further encoding. Is that correct?\n\nI doubt that the host contains a \"/\", but if you had a path that\ncontained a \"%\", then the URL form of that is going to be %25. Which is\ncurl expecting to get here?\n\nI say \"pseudo-url\" because it is weird to slap a path on the end of the\nhostname but leave off the scheme, etc. Which kind of makes me wonder\nwhy we pass proxy_auth.host in the first place, and not simply the\noriginal curl_http_proxy. It looks like a weird interaction between\n372370f167 (http: use credential API to handle proxy authentication,\n2016-01-26) and 57415089bd (http: honor empty http.proxy option to\nbypass proxy, 2017-04-11). The former added a _second_ CURLOPT_PROXY\ncall, and the latter removed the first one.\n\nI wonder if we could go back to passing the string straight to curl (as\nwe did prior to 2016), and keeping the proxy_auth struct purely as a\nmechanism for gathering credentials. That would presumably fix your use\ncase. And this is also perhaps an interesting data point for Junio's\nquestion about regressions (if people were passing paths, it used to\nwork and was broken in 2016).\n\n-Peff\n"},{"id":"499474","messageId":"4f962eaa-d12b-d250-dd93-a1673af850c6@alum.mit.edu","threadId":"61852","inReplyTo":"20240726211156.GA714986@coredump.intra.peff.net","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-07-26T22:43:29Z","receivedAt":"2024-07-26T22:43:34Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-07-26 17:11-0400, Jeff King sent:\n\n> I would warn that there are several not-quite-compatible variants of\n> netcat floating around, which can create headaches. You might be better\n> off with a short perl script using IO::Socket::UNIX or similar.\n\nAh, okay, thanks for the pointer!\n\n>> diff --git a/http.c b/http.c\n>> index 623ed234891..0cd75986a6b 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)\n>> \t\tif (!proxy_auth.host)\n>> \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n>>\n>> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>> +\t\tif (proxy_auth.path) {\n>> +\t\t\tstruct strbuf proxy = STRBUF_INIT;\n>> +\t\t\tstrbuf_addf(&proxy, \"%s/%s\", proxy_auth.host, proxy_auth.path);\n>> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n>> +\t\t\tstrbuf_release(&proxy);\n>> +\t\t} else\n>> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>\n> The fields in the proxy_auth struct have been parsed from the url, with\n> any url encoding removed. But then we paste them back together into a\n> pseudo-url without doing any further encoding. Is that correct?\n>\n> I doubt that the host contains a \"/\", but if you had a path that\n> contained a \"%\", then the URL form of that is going to be %25. Which is\n> curl expecting to get here?\n\nOh, I see! Yes, this is an issue with my patch: if I create a socket file \nnamed \"%30\", command-line curl wants http_proxy to contain \"%2530\", and \npatched Git wants http_proxy to contain \"%252530\". Good edge case to put \nin a test.\n\n> I wonder if we could go back to passing the string straight to curl (as\n> we did prior to 2016), and keeping the proxy_auth struct purely as a\n> mechanism for gathering credentials.\n\nHmm, that would be nice, but I think curl doesn't deal well with the extra \ncase that Git supports of specifying a username but no password. It causes \none of the existing tests to fail if I pass the string straight through.\n\nOn top of that, all of those starts_with tests for checking the protocol \nare written quite loosely, so in practice Git \"supports\" the protocols \n\"socks://\" and \"socksonmyfeet://\" by mapping them both to SOCKS4, and curl \nwould not like it if it received those strings directly.\n\nSo given that Git wants to handle the protocol and the credentials, it \nmakes sense that only the host and the path are passed to curl. I just \nhave to make sure that they are correctly re-encoded.\n\nR\n"},{"id":"499482","messageId":"pull.1767.v2.git.1722062682858.gitgitgadget@gmail.com","threadId":"61852","inReplyTo":"pull.1767.git.1722009170590.gitgitgadget@gmail.com","subject":"[PATCH v2] http: do not ignore proxy path","fromName":"Ryan Hendrickson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-27T06:44:42Z","receivedAt":"2024-07-27T06:44:47Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"From: Ryan Hendrickson \n\nThe documentation for `http.proxy` describes that option, and the\nenvironment variables it overrides, as supporting \"the syntax understood\nby curl\". curl allows SOCKS proxies to use a path to a Unix domain\nsocket, like `socks5h://localhost/path/to/socket.sock`. Git should\ntherefore include, if present, the path part of the proxy URL in what it\npasses to libcurl.\n\nSigned-off-by: Ryan Hendrickson \n---\n http: do not ignore proxy path\n \n Re earlier discussion about regressions, it turns out that curl has only\n supported this syntax since 2022 [https://curl.se/ch/7.84.0.html].\n Earlier versions of curl, if provided an http_proxy that contained a\n path, would also ignore it. curl didn't seem to consider this a breaking\n change when the feature was introduced\n [https://github.com/curl/curl/pull/8668], though; is that a valid\n argument for Git not to lose sleep over it either?\n \n A test has been added, but unfortunately it is not being run on any of\n GitHub's CI workflows, because the runner images in use all have\n versions of libcurl that are too old. It works on my machine, but\n unknown troubles may await someone else.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1767%2Frhendric%2Frhendric%2Fhttp-proxy-path-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1767/rhendric/rhendric/http-proxy-path-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1767\n\nRange-diff vs v1:\n\n 1: 6b6ef680dce < -: ----------- http: do not ignore proxy path\n -: ----------- > 1: b4715eba5b1 http: do not ignore proxy path\n\n\n Documentation/config/http.txt | 4 +-\n http.c | 20 ++-\n t/socks5-proxy-server/LICENSE | 21 +++\n t/socks5-proxy-server/README.md | 27 +++\n t/socks5-proxy-server/src/socks5.pl | 260 ++++++++++++++++++++++++++++\n t/t5564-http-proxy.sh | 18 ++\n 6 files changed, 347 insertions(+), 3 deletions(-)\n create mode 100644 t/socks5-proxy-server/LICENSE\n create mode 100644 t/socks5-proxy-server/README.md\n create mode 100755 t/socks5-proxy-server/src/socks5.pl\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 162b33fc52f..a14371b5c96 100644\n--- a/Documentation/config/http.txt\n+++ b/Documentation/config/http.txt\n@@ -5,8 +5,8 @@ http.proxy::\n \tproxy string with a user name but no password, in which case git will\n \tattempt to acquire one in the same way it does for other credentials. See\n \tlinkgit:gitcredentials[7] for more information. The syntax thus is\n-\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n-\ton a per-remote basis; see remote..proxy\n+\t'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be\n+\toverridden on a per-remote basis; see remote..proxy\n +\n Any proxy, however configured, must be completely transparent and must not\n modify, transform, or buffer the request or response in any way. Proxies which\ndiff --git a/http.c b/http.c\nindex 623ed234891..a6001296dd0 100644\n--- a/http.c\n+++ b/http.c\n@@ -1227,6 +1227,7 @@ static CURL *get_curl_handle(void)\n \t\t */\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n \t} else if (curl_http_proxy) {\n+\t\tstruct strbuf proxy = STRBUF_INIT;\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n@@ -1265,7 +1266,24 @@ static CURL *get_curl_handle(void)\n \t\tif (!proxy_auth.host)\n \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n+\t\tif (proxy_auth.path) {\n+\t\t\tint path_is_supported = 0;\n+\t\t\t/* curl_version_info was added in curl 7.10 */\n+#if LIBCURL_VERSION_NUM >= 0x070a00\n+\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n+\t\t\tpath_is_supported = ver->version_num >= 0x075400;\n+#endif\n+\t\t\tif (path_is_supported) {\n+\t\t\t\tstrbuf_addch(&proxy, '/');\n+\t\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n+\t\t\t} else {\n+\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n+\t\t\t}\n+\t\t}\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n+\t\tstrbuf_release(&proxy);\n+\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\ndiff --git a/t/socks5-proxy-server/LICENSE b/t/socks5-proxy-server/LICENSE\nnew file mode 100644\nindex 00000000000..94981550c94\n--- /dev/null\n+++ b/t/socks5-proxy-server/LICENSE\n@@ -0,0 +1,21 @@\n+MIT License\n+\n+Copyright (c) 2014 kaimi.io\n+\n+Permission is hereby granted, free of charge, to any person obtaining a copy\n+of this software and associated documentation files (the \"Software\"), to deal\n+in the Software without restriction, including without limitation the rights\n+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n+copies of the Software, and to permit persons to whom the Software is\n+furnished to do so, subject to the following conditions:\n+\n+The above copyright notice and this permission notice shall be included in all\n+copies or substantial portions of the Software.\n+\n+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n+SOFTWARE.\ndiff --git a/t/socks5-proxy-server/README.md b/t/socks5-proxy-server/README.md\nnew file mode 100644\nindex 00000000000..6eed0e5ce7a\n--- /dev/null\n+++ b/t/socks5-proxy-server/README.md\n@@ -0,0 +1,27 @@\n+socks5-proxy-server\n+===================\n+\n+This Perl code implements a SOCKS5 proxy server that listens for incoming connections and processes them in separate threads. The server takes three input parameters: `path`, `login`, and `password`.\n+\n+When a client attempts to connect to the server, the server checks if the client supports any of the available authentication methods (no authentication or login/password authentication). If a suitable method is found, the server establishes a connection with the target server and begins forwarding data between the client and the target server.\n+\n+The code uses the `IO::Select` module for working with sockets and the `threads` module for creating and managing threads. It includes several functions, including:\n+\n+- `main`: the main function that creates threads for processing incoming connections.\n+- `replenish`: a function that creates additional threads if the number of free threads is less than the specified value.\n+- `new_client`: a function that handles incoming client connections and checks if the available authentication methods are supported by the client.\n+- `socks_auth`: a function that performs login and password authentication.\n+- `handle_client`: a function that establishes a connection with the target server and forwards data between the client and the target server.\n+\n+This code includes the use of the following Perl modules: `IO::Select`, `Socket`, `threads`, `threads::shared`.\n+\n+To run this code, enter the following command:\n+`perl socks5.pl path login password`\n+\n+Note that this code is designed for educational purposes and should not be used in production environments without proper modifications and security measures. 😊\n+\n+This code is distributed under the [MIT License](LICENSE), which allows for free use, modification, and distribution of the code as long as the original copyright notice and license are included.\n+\n+---\n+\n+Originally from: https://github.com/kaimi-io/socks5-proxy-server\ndiff --git a/t/socks5-proxy-server/src/socks5.pl b/t/socks5-proxy-server/src/socks5.pl\nnew file mode 100755\nindex 00000000000..3ef66a1a287\n--- /dev/null\n+++ b/t/socks5-proxy-server/src/socks5.pl\n@@ -0,0 +1,260 @@\n+#!/usr/bin/perl\n+#В коде использованы части rdss (.Slip) и sss (hm2k)\n+use strict;\n+use IO::Select;\n+use Socket;\n+use threads;\n+use threads::shared;\n+\n+if(scalar @ARGV < 1)\n+{\n+\tdie \"Usage: serv.pl path login pass\\n\";\n+}\n+\n+my ($path, $login, $passw) = @ARGV;\n+\n+my %config = (thr_init => 10, thr_min => 2, thr_max => 20, conn_limit => SOMAXCONN);\n+\n+$| = 1;\n+\n+my %status : shared;\n+my $accept : shared;\n+\n+my $sock_addr = sockaddr_un $path or die $!;\n+socket my $socket, PF_UNIX, SOCK_STREAM, 0;\n+bind $socket, $sock_addr or die $!;\n+listen $socket, $config{'conn_limit'} or die $!;\n+\n+\n+my $sel = IO::Select->new($socket);\n+\n+print \"Starting...\\n\";\n+\n+replenish($config{'thr_init'});\n+\n+while(1)\n+{\n+\tlock %status;\n+\tcond_wait %status;\n+\n+\tmy @free = sort {$a <=> $b} grep {$status{$_} eq 'free'} keys %status;\n+\n+\tif(scalar @free < $config{'thr_min'})\n+\t{\n+\t\treplenish($#free - $config{'thr_min'});\n+\t}\n+\telsif(scalar @free > $config{'thr_max'})\n+\t{\n+\t\tmy @kill = @free[0..$#free - $config{'thr_max'}];\n+\t\tstatus($_ => 'kill') for @kill;\n+\t}\n+}\n+\n+status($_ => 'kill') for keys %status;\n+\n+sub main\n+{\n+\tmy $sock = shift;\n+\tmy $loop = 50;\n+\n+\tmy $tid = threads->tid;\n+\tmy $conn;\n+\n+\tthreads->self->detach;\n+\tstatus($tid, 'free');\n+\n+\twhile(status($tid) ne 'kill' && $loop > 0)\n+\t{\n+\t\tnext unless $sel->can_read(.1);\n+\t\t{\n+\t\t\tlock $accept;\n+\t\t\tnext unless accept $conn, $sock;\n+\t\t}\n+\t\t$loop--;\n+\t\tstatus($tid => 'work');\n+\t\tnew_client($conn);\n+\t\tclose $conn;\n+\t\tstatus($tid => 'free');\n+\t}\n+\n+\tstatus($tid, 'kill');\n+}\n+\n+sub status\n+{\n+\tmy ($tid, $state) = @_;\n+\tlock %status;\n+\n+\treturn $status{$tid} unless $state;\n+\tif($state)\n+\t{\n+\t\t$status{$tid} = $state unless defined $status{$tid} and $status{$tid} eq 'kill';\n+\t}\n+\telse\n+\t{\n+\t\tdelete $status{$tid};\n+\t}\n+\n+\tcond_broadcast %status;\n+}\n+\n+sub replenish\n+{\n+\tthreads->create(\\&main, $socket) for 1..$_[0];\n+}\n+\n+sub new_client\n+{\n+\tmy $sock = shift;\n+\n+\tsysread $sock, my $buf, 2;\n+\n+\tmy ($ver, $auth_num) = unpack('H2H2', $buf);\n+\t#Версия протокола\n+\treturn unless $ver eq '05';\n+\tsysread $sock, $buf, $auth_num;\n+\n+\tmy $ok = 0;\n+\t#Перечисление методов авторизации\n+\tfor(my ($mode, $i) = (0, 0); $i < $auth_num; $mode = ord substr $buf, ++$i, 1)\n+\t{\n+\t\t#0 - Без авторизации; 2 - Username/Password\n+\t\tif($mode == 0 && !$login)\n+\t\t{\n+\t\t\tsyswrite $sock, \"\\x05\\x00\";\n+\t\t\t$ok++;\n+\t\t\tlast;\n+\t\t}\n+\t\telsif($mode == 2 && $login)\n+\t\t{\n+\t\t\treturn unless socks_auth($sock);\n+\t\t\t$ok++;\n+\t\t\tlast;\n+\t\t}\n+\t}\n+\t#Подходящие методы есть\n+\tif($ok)\n+\t{\n+\t\tsysread $sock, $buf, 3;\n+\t\tmy ($ver, $cmd, $r) = unpack 'H2H2H2', $buf;\n+\n+\t\tif($ver eq '05' && $r eq '00')\n+\t\t{\n+\t\t\tmy ($client_host, $client_host_raw, $client_port, $client_port_raw) = get_conn_info($sock);\n+\t\t\treturn unless ($client_host || $client_port);\n+\n+\t\t\tsyswrite $sock, \"\\x05\\x00\\x00\".$client_host_raw.$client_port_raw;\n+\t\t\thandle_client($sock, $client_host, $client_port, $cmd);\n+\t\t}\n+\t}\n+\telse\n+\t{\n+\t\tsyswrite $sock, \"\\x05\\xFF\";\n+\t}\n+}\n+\n+sub socks_auth\n+{\n+\tmy $sock = shift;\n+\n+\tsyswrite $sock, \"\\x05\\x02\";\n+\tsysread $sock, my $buf, 1;\n+\n+\tif(ord $buf == 1)\n+\t{\n+\t\t#username length : username : password length : password\n+\t\tsysread $sock, $buf, 1;\n+\t\tsysread $sock, my $s_login, ord($buf);\n+\t\tsysread $sock, $buf, 1;\n+\t\tsysread $sock, my $s_passw, ord($buf);\n+\n+\t\t#0x00 = success; any other value = failure\n+\t\tif($login eq $s_login && $passw eq $s_passw)\n+\t\t{\n+\t\t\tsyswrite $sock, \"\\x05\\x00\";\n+\t\t\treturn 1;\n+\t\t}\n+\t\telse\n+\t\t{\n+\t\t\tsyswrite $sock, \"\\x05\\x01\";\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+}\n+\n+sub handle_client\n+{\n+\tmy ($sock, $host, $port, $cmd) = @_;\n+\n+\t#0x01 = establish a TCP/IP stream connection\n+\tif($cmd == 1)\n+\t{\n+\t\tmy $sock_addr = sockaddr_in $port, inet_aton($host) or return;\n+\t\tsocket my $target, PF_INET, SOCK_STREAM, getprotobyname('tcp') or return;\n+\t\tconnect $target, $sock_addr or return;\n+\n+\t\twhile($sock || $target)\n+\t\t{\n+\t\t\tmy ($rin, $cbuf, $tbuf, $rout, $eout) = ('', '', '', '', '');\n+\t\t\tvec($rin, fileno($sock), 1) = 1 if $sock;\n+\t\t\tvec($rin, fileno($target), 1) = 1 if $target;\n+\t\t\tselect($rout = $rin, undef, $eout = $rin, 120);\n+\t\t\treturn if(!$rout && !$eout);\n+\n+\t\t\tif($sock && (vec($eout, fileno($sock), 1) || vec($rout, fileno($sock), 1)))\n+\t\t\t{\n+\t\t\t\tmy $res = sysread $sock, $tbuf, 1024;\n+\t\t\t\treturn if(!defined $res || !$res);\n+\t\t\t}\n+\t\t\tif($target && (vec($eout, fileno($target), 1) || vec($rout, fileno($target), 1)))\n+\t\t\t{\n+\t\t\t\tmy $res = sysread $target, $cbuf, 1024;\n+\t\t\t\treturn if(!defined $res || !$res);\n+\t\t\t}\n+\t\t\twhile(my $len = length($tbuf))\n+\t\t\t{\n+\t\t\t\tmy $r = syswrite $target, $tbuf, $len;\n+\t\t\t\treturn if(!defined $r || $r <= 0);\n+\t\t\t\t$tbuf = substr($tbuf, $r);\n+\t\t\t}\n+\t\t\twhile(my $len = length($cbuf))\n+\t\t\t{\n+\t\t\t\tmy $r = syswrite $sock, $cbuf, $len;\n+\t\t\t\treturn if(!defined $r || $r <= 0);\n+\t\t\t\t$cbuf = substr($cbuf, $r);\n+\t\t\t}\n+\t\t}\n+\t}\n+}\n+\n+sub get_conn_info\n+{\n+\tmy $sock = shift;\n+\n+\tmy ($host, $raw_host) = ('', '');\n+\tsysread $sock, my $buf, 1;\n+\t($raw_host, $buf) = ($buf, ord $buf);\n+\t#0x01 = IPv4 address; 0x03 = Domain name\n+\tif($buf == 1)\n+\t{\n+\t\t#4 bytes for IPv4 address\n+\t\tsysread $sock, $buf, 4;\n+\t\t$raw_host .= $buf;\n+\t\t$host = join '.', map(ord, split //, $buf);\n+\t}\n+\telsif($buf == 3)\n+\t{\n+\t\t#1 byte of name length followed by the name for Domain name\n+\t\tsysread $sock, $buf, 1;\n+\t\tsysread $sock, $host, ord($buf);\n+\t\t$raw_host .= $host;\n+\t}\n+\n+\t#port number in a network byte order, 2 bytes\n+\tmy ($port, $raw_port) = ('', '');\n+\tsysread $sock, $raw_port, 2;\n+\t$port = ord(substr($raw_port, 0, 1)) << 8 | ord(substr($raw_port, 1, 1));\n+\n+\treturn $host, $raw_host, $port, $raw_port;\n+}\ndiff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\nindex bb35b87071d..bafaa31adf8 100755\n--- a/t/t5564-http-proxy.sh\n+++ b/t/t5564-http-proxy.sh\n@@ -39,4 +39,22 @@ test_expect_success 'clone can prompt for proxy password' '\n \texpect_askpass pass proxuser\n '\n \n+start_socks() {\n+\t# The %30 tests that the correct amount of percent-encoding is applied\n+\t# to the proxy string passed to curl.\n+\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks5-proxy-server/src/socks5.pl\" \\\n+\t\t\"$TRASH_DIRECTORY/%30.sock\" proxuser proxpass &\n+\tsocks_pid=$!\n+}\n+\n+test_lazy_prereq LIBCURL_7_84 'expr x$(curl-config --vernum) \\>= x075400'\n+\n+test_expect_success PERL,LIBCURL_7_84 'clone via Unix socket' '\n+\tstart_socks &&\n+\ttest_when_finished \"rm -rf clone && kill $socks_pid\" &&\n+\ttest_config_global http.proxy \"socks5://proxuser:proxpass@localhost$PWD/%2530.sock\" &&\n+\tGIT_TRACE_CURL=$PWD/trace git clone \"$HTTPD_URL/smart/repo.git\" clone &&\n+\tgrep -i \"SOCKS5 request granted.\" trace\n+'\n+\n test_done\n\nbase-commit: ad57f148c6b5f8735b62238dda8f571c582e0e54\n-- \ngitgitgadget\n"},{"id":"499530","messageId":"20240729193113.GA23714@coredump.intra.peff.net","threadId":"61852","inReplyTo":"4f962eaa-d12b-d250-dd93-a1673af850c6@alum.mit.edu","subject":"Re: [PATCH] http: do not ignore proxy path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-29T19:31:13Z","receivedAt":"2024-07-29T19:31:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2024 at 06:43:29PM -0400, Ryan Hendrickson wrote:\n\n> > I wonder if we could go back to passing the string straight to curl (as\n> > we did prior to 2016), and keeping the proxy_auth struct purely as a\n> > mechanism for gathering credentials.\n> \n> Hmm, that would be nice, but I think curl doesn't deal well with the extra\n> case that Git supports of specifying a username but no password. It causes\n> one of the existing tests to fail if I pass the string straight through.\n\nOK, thanks for trying. It would have been nice, but I'm not surprised\nthat there's an unusual interaction.\n\n> On top of that, all of those starts_with tests for checking the protocol are\n> written quite loosely, so in practice Git \"supports\" the protocols\n> \"socks://\" and \"socksonmyfeet://\" by mapping them both to SOCKS4, and curl\n> would not like it if it received those strings directly.\n>\n> So given that Git wants to handle the protocol and the credentials, it makes\n> sense that only the host and the path are passed to curl. I just have to\n> make sure that they are correctly re-encoded.\n\nMakes sense.\n\n-Peff\n"},{"id":"499532","messageId":"20240729200953.GB23714@coredump.intra.peff.net","threadId":"61852","inReplyTo":"pull.1767.v2.git.1722062682858.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] http: do not ignore proxy path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-29T20:09:53Z","receivedAt":"2024-07-29T20:09:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 27, 2024 at 06:44:42AM +0000, Ryan Hendrickson via GitGitGadget wrote:\n\n> Re earlier discussion about regressions, it turns out that curl has only\n> supported this syntax since 2022 [https://curl.se/ch/7.84.0.html].\n> Earlier versions of curl, if provided an http_proxy that contained a\n> path, would also ignore it. curl didn't seem to consider this a breaking\n> change when the feature was introduced\n> [https://github.com/curl/curl/pull/8668], though; is that a valid\n> argument for Git not to lose sleep over it either?\n\nIMHO it is probably OK. The path did not do anything before then, so why\nwould anybody pass it?\n\nLooking into the curl support, I did notice two things:\n\n - unix sockets are only supported for socks proxies. Is it meaningful\n to have a path on an http proxy? I don't think so (there is no place\n to send it in the \"GET\" line). But perhaps we should limit the new\n code only to socks.\n\n - curl says you should put \"localhost\" in the host field for this,\n though obviously it is not otherwise used. Should we likewise\n require that to kick in the code?\n\n> @@ -1265,7 +1266,24 @@ static CURL *get_curl_handle(void)\n> \t\tif (!proxy_auth.host)\n> \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n> \n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n> +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n> +\t\tif (proxy_auth.path) {\n\nThere's one gotcha here I wondered about: we usually throw away the path\nelement, unless credential.useHTTPPath is set. That only happens after\nwe load the per-credential config, though, via credential_apply_config(),\nwhich in turn is triggered by a call to credential_fill(), etc.\n\nI think this is OK here, since we have just called credential_from_url()\nourselves, and the earliest credential_fill() we'd hit is from\ninit_curl_proxy_auth(), which is called right after the code you're\ntouching.\n\n> +\t\t\tint path_is_supported = 0;\n> +\t\t\t/* curl_version_info was added in curl 7.10 */\n> +#if LIBCURL_VERSION_NUM >= 0x070a00\n> +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n> +\t\t\tpath_is_supported = ver->version_num >= 0x075400;\n> +#endif\n\nWe've been trying to keep our curl version #ifdefs in git-curl-compat.h,\nwith human-readable names. But in this case, I think we can ditch it\nentirely, as we require curl 7.21.3 or later already.\n\nThis will be the first time we check curl_version_info() instead of a\nbuild-time option. I think that makes sense here. Most features require\nextra information at build-time (e.g., CURLOPT_* constants), but in this\ncase it is purely a check on the runtime behavior.\n\nWe perhaps could get away with just letting an older curl quietly ignore\nthe path field, but what you have here is probably a bit friendlier for\nusers.\n\n> diff --git a/t/socks5-proxy-server/src/socks5.pl b/t/socks5-proxy-server/src/socks5.pl\n> new file mode 100755\n> index 00000000000..3ef66a1a287\n> --- /dev/null\n> +++ b/t/socks5-proxy-server/src/socks5.pl\n\nThis is a lot more code than I was hoping for. There are definitely\nparts of it we don't care about (e.g, threading, handling multiple\nconnections at once) that could be pared down a bit. I don't think we\nparticularly care about auth either (we just want to make sure we talk\nto the unix-socket proxy at all).\n\nWhat if we switched to socks4, which drops all of the auth bits and\nsupports only IPs, not hostnames (that came in socks4a). This is the\nshortest I could come up with (only lightly tested):\n\n-- >8 --\nuse strict;\nuse IO::Select;\nuse IO::Socket::UNIX;\nuse IO::Socket::INET;\n\nmy $path = shift;\n\nunlink($path);\nmy $server = IO::Socket::UNIX->new(Listen => 1, Local => $path)\n\tor die \"unable to listen on $path: $!\";\n\n$| = 1;\nprint \"ready to rumble\\n\";\n\nwhile (my $client = $server->accept()) {\n\tsysread $client, my $buf, 8;\n\tmy ($version, $cmd, $port, $ip) = unpack 'CCnN', $buf;\n\tnext unless $version == 4; # socks4\n\tnext unless $cmd == 1; # TCP stream connection\n\n\t# skip NUL-terminated id\n\twhile (sysread $client, my $char, 1) {\n\t\tlast unless ord($char);\n\t}\n\n\t# version(0), reply(5a == granted), port (ignored), ip (ignored)\n\tsyswrite $client, \"\\x00\\x5a\\x00\\x00\\x00\\x00\\x00\\x00\";\n\n\tmy $remote = IO::Socket::INET->new(PeerHost => $ip, PeerPort => $port)\n\t\tor die \"unable to connect to $ip/$port: $!\";\n\n\tmy $io = IO::Select->new($client, $remote);\n\twhile ($io->count) {\n\t\tfor my $fh ($io->can_read(0)) {\n\t\t\tfor my $pair ([$client, $remote], [$remote, $client]) {\n\t\t\t\tmy ($from, $to) = @$pair;\n\t\t\t\tnext unless $fh == $from;\n\n\t\t\t\tmy $r = sysread $from, my $buf, 1024;\n\t\t\t\tif (!defined $r || $r <= 0) {\n\t\t\t\t\t$io->remove($from);\n\t\t\t\t\tnext;\n\t\t\t\t}\n\t\t\t\tsyswrite $to, $buf;\n\t\t\t}\n\t\t}\n\t}\n}\n-- >8 --\n\nThat's still more than I'd like, but quite a bit smaller. I dunno.\nProbably the one you found is more battle-tested, but I really think we\nhave pretty simple requirements.\n\n> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\n> index bb35b87071d..bafaa31adf8 100755\n> --- a/t/t5564-http-proxy.sh\n> +++ b/t/t5564-http-proxy.sh\n> @@ -39,4 +39,22 @@ test_expect_success 'clone can prompt for proxy password' '\n> \texpect_askpass pass proxuser\n> '\n> \n> +start_socks() {\n> +\t# The %30 tests that the correct amount of percent-encoding is applied\n> +\t# to the proxy string passed to curl.\n> +\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks5-proxy-server/src/socks5.pl\" \\\n> +\t\t\"$TRASH_DIRECTORY/%30.sock\" proxuser proxpass &\n> +\tsocks_pid=$!\n> +}\n> +\n> +test_lazy_prereq LIBCURL_7_84 'expr x$(curl-config --vernum) \\>= x075400'\n\nThis is the first time we'd be relying on curl-config in the test suite.\nI suspect it would _usually_ work, but I'd worry about a few things:\n\n 1. The Makefile allows for a different path for $(CURL_CONFIG), or\n even skipping it entirely by setting $(CURLDIR).\n\n 2. We'd usually build and test in the same environment, but not\n necessarily. In particular, I know Windows uses separate CI jobs\n for this, and I'm not sure if curl-config will be around in the\n test jobs.\n\nI can see two paths forward:\n\n a. There's been recent discussion about adding an option for Git to\n report the run-time curl version. That doesn't exist yet, though\n \"git version --build-options\" will report the build-time version.\n That might be good enough for the purposes of testing a build.\n\n b. Write the test such that it expects the request to work _or_ we see\n the \"version too old\" failure.\n\n> +test_expect_success PERL,LIBCURL_7_84 'clone via Unix socket' '\n\nI'm not sure if this PERL prereq is sufficient. We also need to know\nthat we can make unix sockets. Probably we need to actually run the\nsocks proxy and make sure it gets to the \"starting...\" line before\naccepting that it works. Which also gets us to...\n\n> +\tstart_socks &&\n> +\ttest_when_finished \"rm -rf clone && kill $socks_pid\" &&\n> +\ttest_config_global http.proxy \"socks5://proxuser:proxpass@localhost$PWD/%2530.sock\" &&\n> +\tGIT_TRACE_CURL=$PWD/trace git clone \"$HTTPD_URL/smart/repo.git\" clone &&\n\nThis is racy. We started the proxy in the background, but we don't know\nthat it's ready to accept connections by the time we run \"git clone\". My\nfirst few runs all failed, but putting a \"sleep 1\" in between fixed it\n(which obviously is not a real fix, but just a debugging aid).\n\nIf you usually see success, try running the test script with \"--stress\"\nto create extra load, and you should see failures.\n\nThe usual trick here is to start the process in the background, and then\nin the foreground wait for some \"I'm ready\" output, which involves ugly\ntricks with fifos. There's prior art in lib-git-daemon.sh (though it's a\nlittle more complicated there, because we want to relay its stderr, too,\nbut with a script under our control we can just put the ready message on\nstdout).\n\nSo coupled with the prereq fix that I mentioned above, you might be able\nto do something like (totally untested):\n\n start_socks_proxy () {\n\tmkfifo socks_output &&\n\t{\n\t\tperl proxy.pl ... >socks_output &\n\t\tsocks_pid=$!\n\t} &&\n\tread line sent:\n\n> Looking into the curl support, I did notice two things:\n>\n> - unix sockets are only supported for socks proxies. Is it meaningful\n> to have a path on an http proxy? I don't think so (there is no place\n> to send it in the \"GET\" line). But perhaps we should limit the new\n> code only to socks.\n>\n> - curl says you should put \"localhost\" in the host field for this,\n> though obviously it is not otherwise used. Should we likewise\n> require that to kick in the code?\n\nSounds good, I've added checks and tests for both of these cases.\n\n>> @@ -1265,7 +1266,24 @@ static CURL *get_curl_handle(void)\n>> \t\tif (!proxy_auth.host)\n>> \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n>>\n>> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>> +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n>> +\t\tif (proxy_auth.path) {\n>\n> There's one gotcha here I wondered about: we usually throw away the path\n> element, unless credential.useHTTPPath is set. That only happens after\n> we load the per-credential config, though, via credential_apply_config(),\n> which in turn is triggered by a call to credential_fill(), etc.\n>\n> I think this is OK here, since we have just called credential_from_url()\n> ourselves, and the earliest credential_fill() we'd hit is from\n> init_curl_proxy_auth(), which is called right after the code you're\n> touching.\n\nYes, good lookout but I don't think this is a problem either.\n\n>> +\t\t\tint path_is_supported = 0;\n>> +\t\t\t/* curl_version_info was added in curl 7.10 */\n>> +#if LIBCURL_VERSION_NUM >= 0x070a00\n>> +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n>> +\t\t\tpath_is_supported = ver->version_num >= 0x075400;\n>> +#endif\n>\n> We've been trying to keep our curl version #ifdefs in git-curl-compat.h,\n> with human-readable names. But in this case, I think we can ditch it\n> entirely, as we require curl 7.21.3 or later already.\n\nAh, okay, that is good to know! I'll remove the #if.\n\n> This will be the first time we check curl_version_info() instead of a\n> build-time option. I think that makes sense here. Most features require\n> extra information at build-time (e.g., CURLOPT_* constants), but in this\n> case it is purely a check on the runtime behavior.\n>\n> We perhaps could get away with just letting an older curl quietly ignore\n> the path field, but what you have here is probably a bit friendlier for\n> users.\n\nYes, curl has a nasty tendency to quietly ignore a lot of things. I didn't \nwant users to be uncertain about whether they were using the feature \nincorrectly if an older curl was the actual issue.\n\n> What if we switched to socks4, which drops all of the auth bits and\n> supports only IPs, not hostnames (that came in socks4a). This is the\n> shortest I could come up with (only lightly tested):\n\nAh, many thanks! I like this much better, and I'm not proficient enough \nwith Perl to have written it myself.\n\n> I can see two paths forward:\n>\n> a. There's been recent discussion about adding an option for Git to\n> report the run-time curl version. That doesn't exist yet, though\n> \"git version --build-options\" will report the build-time version.\n> That might be good enough for the purposes of testing a build.\n>\n> b. Write the test such that it expects the request to work _or_ we see\n> the \"version too old\" failure.\n\nGot it, I'll go with option b.\n\n> If you usually see success, try running the test script with \"--stress\"\n> to create extra load, and you should see failures.\n\nHuh. I never saw any race issues on my machine even with --stress, but \nyour approach is safer so I'm running with it.\n\nThank you!\n\nR\n"},{"id":"499709","messageId":"pull.1767.v3.git.1722441675945.gitgitgadget@gmail.com","threadId":"61852","inReplyTo":"pull.1767.v2.git.1722062682858.gitgitgadget@gmail.com","subject":"[PATCH v3] http: do not ignore proxy path","fromName":"Ryan Hendrickson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-31T16:01:15Z","receivedAt":"2024-07-31T16:01:19Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"From: Ryan Hendrickson \n\nThe documentation for `http.proxy` describes that option, and the\nenvironment variables it overrides, as supporting \"the syntax understood\nby curl\". curl allows SOCKS proxies to use a path to a Unix domain\nsocket, like `socks5h://localhost/path/to/socket.sock`. Git should\ntherefore include, if present, the path part of the proxy URL in what it\npasses to libcurl.\n\nCo-authored-by: Jeff King \nSigned-off-by: Ryan Hendrickson \n---\n http: do not ignore proxy path\n \n Re earlier discussion about regressions, it turns out that curl has only\n supported this syntax since 2022 [https://curl.se/ch/7.84.0.html].\n Earlier versions of curl, if provided an http_proxy that contained a\n path, would also ignore it. curl didn't seem to consider this a breaking\n change when the feature was introduced\n [https://github.com/curl/curl/pull/8668], though; is that a valid\n argument for Git not to lose sleep over it either?\n \n A test has been added, but unfortunately it is not being run on any of\n GitHub's CI workflows, because the runner images in use all have\n versions of libcurl that are too old. It works on my machine, but\n unknown troubles may await someone else.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1767%2Frhendric%2Frhendric%2Fhttp-proxy-path-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1767/rhendric/rhendric/http-proxy-path-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1767\n\nRange-diff vs v2:\n\n 1: b4715eba5b1 ! 1: 7a58da7102e http: do not ignore proxy path\n @@ Commit message\n therefore include, if present, the path part of the proxy URL in what it\n passes to libcurl.\n \n + Co-authored-by: Jeff King \n Signed-off-by: Ryan Hendrickson \n \n ## Documentation/config/http.txt ##\n @@ http.c: static CURL *get_curl_handle(void)\n -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n +\t\tif (proxy_auth.path) {\n -+\t\t\tint path_is_supported = 0;\n -+\t\t\t/* curl_version_info was added in curl 7.10 */\n -+#if LIBCURL_VERSION_NUM >= 0x070a00\n +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n -+\t\t\tpath_is_supported = ver->version_num >= 0x075400;\n -+#endif\n -+\t\t\tif (path_is_supported) {\n -+\t\t\t\tstrbuf_addch(&proxy, '/');\n -+\t\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n -+\t\t\t} else {\n ++\t\t\tif (ver->version_num < 0x075400)\n +\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n -+\t\t\t}\n ++\n ++\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n ++\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n ++\t\t\t\t curl_http_proxy);\n ++\n ++\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n ++\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n ++\t\t\t\t curl_http_proxy);\n ++\n ++\t\t\tstrbuf_addch(&proxy, '/');\n ++\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n +\t\t}\n +\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n +\t\tstrbuf_release(&proxy);\n @@ http.c: static CURL *get_curl_handle(void)\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\n \n - ## t/socks5-proxy-server/LICENSE (new) ##\n -@@\n -+MIT License\n -+\n -+Copyright (c) 2014 kaimi.io\n -+\n -+Permission is hereby granted, free of charge, to any person obtaining a copy\n -+of this software and associated documentation files (the \"Software\"), to deal\n -+in the Software without restriction, including without limitation the rights\n -+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n -+copies of the Software, and to permit persons to whom the Software is\n -+furnished to do so, subject to the following conditions:\n -+\n -+The above copyright notice and this permission notice shall be included in all\n -+copies or substantial portions of the Software.\n -+\n -+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n -+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n -+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n -+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n -+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n -+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n -+SOFTWARE.\n -\n - ## t/socks5-proxy-server/README.md (new) ##\n -@@\n -+socks5-proxy-server\n -+===================\n -+\n -+This Perl code implements a SOCKS5 proxy server that listens for incoming connections and processes them in separate threads. The server takes three input parameters: `path`, `login`, and `password`.\n -+\n -+When a client attempts to connect to the server, the server checks if the client supports any of the available authentication methods (no authentication or login/password authentication). If a suitable method is found, the server establishes a connection with the target server and begins forwarding data between the client and the target server.\n -+\n -+The code uses the `IO::Select` module for working with sockets and the `threads` module for creating and managing threads. It includes several functions, including:\n -+\n -+- `main`: the main function that creates threads for processing incoming connections.\n -+- `replenish`: a function that creates additional threads if the number of free threads is less than the specified value.\n -+- `new_client`: a function that handles incoming client connections and checks if the available authentication methods are supported by the client.\n -+- `socks_auth`: a function that performs login and password authentication.\n -+- `handle_client`: a function that establishes a connection with the target server and forwards data between the client and the target server.\n -+\n -+This code includes the use of the following Perl modules: `IO::Select`, `Socket`, `threads`, `threads::shared`.\n -+\n -+To run this code, enter the following command:\n -+`perl socks5.pl path login password`\n -+\n -+Note that this code is designed for educational purposes and should not be used in production environments without proper modifications and security measures. 😊\n -+\n -+This code is distributed under the [MIT License](LICENSE), which allows for free use, modification, and distribution of the code as long as the original copyright notice and license are included.\n -+\n -+---\n -+\n -+Originally from: https://github.com/kaimi-io/socks5-proxy-server\n -\n - ## t/socks5-proxy-server/src/socks5.pl (new) ##\n + ## t/socks4-proxy.pl (new) ##\n @@\n -+#!/usr/bin/perl\n -+#В коде использованы части rdss (.Slip) и sss (hm2k)\n +use strict;\n +use IO::Select;\n -+use Socket;\n -+use threads;\n -+use threads::shared;\n ++use IO::Socket::UNIX;\n ++use IO::Socket::INET;\n +\n -+if(scalar @ARGV < 1)\n -+{\n -+\tdie \"Usage: serv.pl path login pass\\n\";\n -+}\n ++my $path = shift;\n +\n -+my ($path, $login, $passw) = @ARGV;\n -+\n -+my %config = (thr_init => 10, thr_min => 2, thr_max => 20, conn_limit => SOMAXCONN);\n ++unlink($path);\n ++my $server = IO::Socket::UNIX->new(Listen => 1, Local => $path)\n ++\tor die \"unable to listen on $path: $!\";\n +\n +$| = 1;\n ++print \"ready\\n\";\n +\n -+my %status : shared;\n -+my $accept : shared;\n -+\n -+my $sock_addr = sockaddr_un $path or die $!;\n -+socket my $socket, PF_UNIX, SOCK_STREAM, 0;\n -+bind $socket, $sock_addr or die $!;\n -+listen $socket, $config{'conn_limit'} or die $!;\n -+\n -+\n -+my $sel = IO::Select->new($socket);\n -+\n -+print \"Starting...\\n\";\n -+\n -+replenish($config{'thr_init'});\n -+\n -+while(1)\n -+{\n -+\tlock %status;\n -+\tcond_wait %status;\n ++while (my $client = $server->accept()) {\n ++\tsysread $client, my $buf, 8;\n ++\tmy ($version, $cmd, $port, $ip) = unpack 'CCnN', $buf;\n ++\tnext unless $version == 4; # socks4\n ++\tnext unless $cmd == 1; # TCP stream connection\n +\n -+\tmy @free = sort {$a <=> $b} grep {$status{$_} eq 'free'} keys %status;\n -+\n -+\tif(scalar @free < $config{'thr_min'})\n -+\t{\n -+\t\treplenish($#free - $config{'thr_min'});\n -+\t}\n -+\telsif(scalar @free > $config{'thr_max'})\n -+\t{\n -+\t\tmy @kill = @free[0..$#free - $config{'thr_max'}];\n -+\t\tstatus($_ => 'kill') for @kill;\n ++\t# skip NUL-terminated id\n ++\twhile (sysread $client, my $char, 1) {\n ++\t\tlast unless ord($char);\n +\t}\n -+}\n -+\n -+status($_ => 'kill') for keys %status;\n -+\n -+sub main\n -+{\n -+\tmy $sock = shift;\n -+\tmy $loop = 50;\n -+\n -+\tmy $tid = threads->tid;\n -+\tmy $conn;\n -+\n -+\tthreads->self->detach;\n -+\tstatus($tid, 'free');\n -+\n -+\twhile(status($tid) ne 'kill' && $loop > 0)\n -+\t{\n -+\t\tnext unless $sel->can_read(.1);\n -+\t\t{\n -+\t\t\tlock $accept;\n -+\t\t\tnext unless accept $conn, $sock;\n -+\t\t}\n -+\t\t$loop--;\n -+\t\tstatus($tid => 'work');\n -+\t\tnew_client($conn);\n -+\t\tclose $conn;\n -+\t\tstatus($tid => 'free');\n -+\t}\n -+\n -+\tstatus($tid, 'kill');\n -+}\n -+\n -+sub status\n -+{\n -+\tmy ($tid, $state) = @_;\n -+\tlock %status;\n -+\n -+\treturn $status{$tid} unless $state;\n -+\tif($state)\n -+\t{\n -+\t\t$status{$tid} = $state unless defined $status{$tid} and $status{$tid} eq 'kill';\n -+\t}\n -+\telse\n -+\t{\n -+\t\tdelete $status{$tid};\n -+\t}\n -+\n -+\tcond_broadcast %status;\n -+}\n -+\n -+sub replenish\n -+{\n -+\tthreads->create(\\&main, $socket) for 1..$_[0];\n -+}\n -+\n -+sub new_client\n -+{\n -+\tmy $sock = shift;\n -+\n -+\tsysread $sock, my $buf, 2;\n -+\n -+\tmy ($ver, $auth_num) = unpack('H2H2', $buf);\n -+\t#Версия протокола\n -+\treturn unless $ver eq '05';\n -+\tsysread $sock, $buf, $auth_num;\n +\n -+\tmy $ok = 0;\n -+\t#Перечисление методов авторизации\n -+\tfor(my ($mode, $i) = (0, 0); $i < $auth_num; $mode = ord substr $buf, ++$i, 1)\n -+\t{\n -+\t\t#0 - Без авторизации; 2 - Username/Password\n -+\t\tif($mode == 0 && !$login)\n -+\t\t{\n -+\t\t\tsyswrite $sock, \"\\x05\\x00\";\n -+\t\t\t$ok++;\n -+\t\t\tlast;\n -+\t\t}\n -+\t\telsif($mode == 2 && $login)\n -+\t\t{\n -+\t\t\treturn unless socks_auth($sock);\n -+\t\t\t$ok++;\n -+\t\t\tlast;\n -+\t\t}\n -+\t}\n -+\t#Подходящие методы есть\n -+\tif($ok)\n -+\t{\n -+\t\tsysread $sock, $buf, 3;\n -+\t\tmy ($ver, $cmd, $r) = unpack 'H2H2H2', $buf;\n -+\n -+\t\tif($ver eq '05' && $r eq '00')\n -+\t\t{\n -+\t\t\tmy ($client_host, $client_host_raw, $client_port, $client_port_raw) = get_conn_info($sock);\n -+\t\t\treturn unless ($client_host || $client_port);\n -+\n -+\t\t\tsyswrite $sock, \"\\x05\\x00\\x00\".$client_host_raw.$client_port_raw;\n -+\t\t\thandle_client($sock, $client_host, $client_port, $cmd);\n -+\t\t}\n -+\t}\n -+\telse\n -+\t{\n -+\t\tsyswrite $sock, \"\\x05\\xFF\";\n -+\t}\n -+}\n -+\n -+sub socks_auth\n -+{\n -+\tmy $sock = shift;\n -+\n -+\tsyswrite $sock, \"\\x05\\x02\";\n -+\tsysread $sock, my $buf, 1;\n -+\n -+\tif(ord $buf == 1)\n -+\t{\n -+\t\t#username length : username : password length : password\n -+\t\tsysread $sock, $buf, 1;\n -+\t\tsysread $sock, my $s_login, ord($buf);\n -+\t\tsysread $sock, $buf, 1;\n -+\t\tsysread $sock, my $s_passw, ord($buf);\n -+\n -+\t\t#0x00 = success; any other value = failure\n -+\t\tif($login eq $s_login && $passw eq $s_passw)\n -+\t\t{\n -+\t\t\tsyswrite $sock, \"\\x05\\x00\";\n -+\t\t\treturn 1;\n -+\t\t}\n -+\t\telse\n -+\t\t{\n -+\t\t\tsyswrite $sock, \"\\x05\\x01\";\n -+\t\t}\n -+\t}\n -+\n -+\treturn 0;\n -+}\n -+\n -+sub handle_client\n -+{\n -+\tmy ($sock, $host, $port, $cmd) = @_;\n -+\n -+\t#0x01 = establish a TCP/IP stream connection\n -+\tif($cmd == 1)\n -+\t{\n -+\t\tmy $sock_addr = sockaddr_in $port, inet_aton($host) or return;\n -+\t\tsocket my $target, PF_INET, SOCK_STREAM, getprotobyname('tcp') or return;\n -+\t\tconnect $target, $sock_addr or return;\n -+\n -+\t\twhile($sock || $target)\n -+\t\t{\n -+\t\t\tmy ($rin, $cbuf, $tbuf, $rout, $eout) = ('', '', '', '', '');\n -+\t\t\tvec($rin, fileno($sock), 1) = 1 if $sock;\n -+\t\t\tvec($rin, fileno($target), 1) = 1 if $target;\n -+\t\t\tselect($rout = $rin, undef, $eout = $rin, 120);\n -+\t\t\treturn if(!$rout && !$eout);\n -+\n -+\t\t\tif($sock && (vec($eout, fileno($sock), 1) || vec($rout, fileno($sock), 1)))\n -+\t\t\t{\n -+\t\t\t\tmy $res = sysread $sock, $tbuf, 1024;\n -+\t\t\t\treturn if(!defined $res || !$res);\n -+\t\t\t}\n -+\t\t\tif($target && (vec($eout, fileno($target), 1) || vec($rout, fileno($target), 1)))\n -+\t\t\t{\n -+\t\t\t\tmy $res = sysread $target, $cbuf, 1024;\n -+\t\t\t\treturn if(!defined $res || !$res);\n -+\t\t\t}\n -+\t\t\twhile(my $len = length($tbuf))\n -+\t\t\t{\n -+\t\t\t\tmy $r = syswrite $target, $tbuf, $len;\n -+\t\t\t\treturn if(!defined $r || $r <= 0);\n -+\t\t\t\t$tbuf = substr($tbuf, $r);\n -+\t\t\t}\n -+\t\t\twhile(my $len = length($cbuf))\n -+\t\t\t{\n -+\t\t\t\tmy $r = syswrite $sock, $cbuf, $len;\n -+\t\t\t\treturn if(!defined $r || $r <= 0);\n -+\t\t\t\t$cbuf = substr($cbuf, $r);\n ++\t# version(0), reply(5a == granted), port (ignored), ip (ignored)\n ++\tsyswrite $client, \"\\x00\\x5a\\x00\\x00\\x00\\x00\\x00\\x00\";\n ++\n ++\tmy $remote = IO::Socket::INET->new(PeerHost => $ip, PeerPort => $port)\n ++\t\tor die \"unable to connect to $ip/$port: $!\";\n ++\n ++\tmy $io = IO::Select->new($client, $remote);\n ++\twhile ($io->count) {\n ++\t\tfor my $fh ($io->can_read(0)) {\n ++\t\t\tfor my $pair ([$client, $remote], [$remote, $client]) {\n ++\t\t\t\tmy ($from, $to) = @$pair;\n ++\t\t\t\tnext unless $fh == $from;\n ++\n ++\t\t\t\tmy $r = sysread $from, my $buf, 1024;\n ++\t\t\t\tif (!defined $r || $r <= 0) {\n ++\t\t\t\t\t$io->remove($from);\n ++\t\t\t\t\tnext;\n ++\t\t\t\t}\n ++\t\t\t\tsyswrite $to, $buf;\n +\t\t\t}\n +\t\t}\n +\t}\n -+}\n -+\n -+sub get_conn_info\n -+{\n -+\tmy $sock = shift;\n -+\n -+\tmy ($host, $raw_host) = ('', '');\n -+\tsysread $sock, my $buf, 1;\n -+\t($raw_host, $buf) = ($buf, ord $buf);\n -+\t#0x01 = IPv4 address; 0x03 = Domain name\n -+\tif($buf == 1)\n -+\t{\n -+\t\t#4 bytes for IPv4 address\n -+\t\tsysread $sock, $buf, 4;\n -+\t\t$raw_host .= $buf;\n -+\t\t$host = join '.', map(ord, split //, $buf);\n -+\t}\n -+\telsif($buf == 3)\n -+\t{\n -+\t\t#1 byte of name length followed by the name for Domain name\n -+\t\tsysread $sock, $buf, 1;\n -+\t\tsysread $sock, $host, ord($buf);\n -+\t\t$raw_host .= $host;\n -+\t}\n -+\n -+\t#port number in a network byte order, 2 bytes\n -+\tmy ($port, $raw_port) = ('', '');\n -+\tsysread $sock, $raw_port, 2;\n -+\t$port = ord(substr($raw_port, 0, 1)) << 8 | ord(substr($raw_port, 1, 1));\n -+\n -+\treturn $host, $raw_host, $port, $raw_port;\n +}\n \n ## t/t5564-http-proxy.sh ##\n @@ t/t5564-http-proxy.sh: test_expect_success 'clone can prompt for proxy password'\n '\n \n +start_socks() {\n ++\tmkfifo socks_output &&\n ++\t{\n ++\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n ++\t\tsocks_pid=$!\n ++\t} &&\n ++\tread line err &&\n ++\t\t\tgrep -i \"SOCKS4 request granted.\" trace\n ++\t\t} ||\n ++\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t}\n ++'\n +\n -+test_lazy_prereq LIBCURL_7_84 'expr x$(curl-config --vernum) \\>= x075400'\n ++test_expect_success SOCKS_PROXY 'stop SOCKS proxy' 'kill \"$socks_pid\"'\n +\n -+test_expect_success PERL,LIBCURL_7_84 'clone via Unix socket' '\n -+\tstart_socks &&\n -+\ttest_when_finished \"rm -rf clone && kill $socks_pid\" &&\n -+\ttest_config_global http.proxy \"socks5://proxuser:proxpass@localhost$PWD/%2530.sock\" &&\n -+\tGIT_TRACE_CURL=$PWD/trace git clone \"$HTTPD_URL/smart/repo.git\" clone &&\n -+\tgrep -i \"SOCKS5 request granted.\" trace\n ++test_expect_success 'Unix socket requires socks*:' '\n ++\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n ++\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n ++\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t}\n ++'\n ++\n ++test_expect_success 'Unix socket requires localhost' '\n ++\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n ++\t\tgrep \"^fatal: Invalid proxy URL '\\''socks4://127\\.0\\.0\\.1/path'\\'': host must be localhost if a path is present\" err ||\n ++\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t}\n +'\n +\n test_done\n\n\n Documentation/config/http.txt | 4 +--\n http.c | 22 +++++++++++++++-\n t/socks4-proxy.pl | 48 +++++++++++++++++++++++++++++++++++\n t/t5564-http-proxy.sh | 46 +++++++++++++++++++++++++++++++++\n 4 files changed, 117 insertions(+), 3 deletions(-)\n create mode 100644 t/socks4-proxy.pl\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 162b33fc52f..a14371b5c96 100644\n--- a/Documentation/config/http.txt\n+++ b/Documentation/config/http.txt\n@@ -5,8 +5,8 @@ http.proxy::\n \tproxy string with a user name but no password, in which case git will\n \tattempt to acquire one in the same way it does for other credentials. See\n \tlinkgit:gitcredentials[7] for more information. The syntax thus is\n-\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n-\ton a per-remote basis; see remote..proxy\n+\t'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be\n+\toverridden on a per-remote basis; see remote..proxy\n +\n Any proxy, however configured, must be completely transparent and must not\n modify, transform, or buffer the request or response in any way. Proxies which\ndiff --git a/http.c b/http.c\nindex 623ed234891..a50ba095889 100644\n--- a/http.c\n+++ b/http.c\n@@ -1227,6 +1227,7 @@ static CURL *get_curl_handle(void)\n \t\t */\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n \t} else if (curl_http_proxy) {\n+\t\tstruct strbuf proxy = STRBUF_INIT;\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n@@ -1265,7 +1266,26 @@ static CURL *get_curl_handle(void)\n \t\tif (!proxy_auth.host)\n \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n+\t\tif (proxy_auth.path) {\n+\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n+\t\t\tif (ver->version_num < 0x075400)\n+\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n+\n+\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tstrbuf_addch(&proxy, '/');\n+\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n+\t\t}\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n+\t\tstrbuf_release(&proxy);\n+\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\ndiff --git a/t/socks4-proxy.pl b/t/socks4-proxy.pl\nnew file mode 100644\nindex 00000000000..4c3a35c0083\n--- /dev/null\n+++ b/t/socks4-proxy.pl\n@@ -0,0 +1,48 @@\n+use strict;\n+use IO::Select;\n+use IO::Socket::UNIX;\n+use IO::Socket::INET;\n+\n+my $path = shift;\n+\n+unlink($path);\n+my $server = IO::Socket::UNIX->new(Listen => 1, Local => $path)\n+\tor die \"unable to listen on $path: $!\";\n+\n+$| = 1;\n+print \"ready\\n\";\n+\n+while (my $client = $server->accept()) {\n+\tsysread $client, my $buf, 8;\n+\tmy ($version, $cmd, $port, $ip) = unpack 'CCnN', $buf;\n+\tnext unless $version == 4; # socks4\n+\tnext unless $cmd == 1; # TCP stream connection\n+\n+\t# skip NUL-terminated id\n+\twhile (sysread $client, my $char, 1) {\n+\t\tlast unless ord($char);\n+\t}\n+\n+\t# version(0), reply(5a == granted), port (ignored), ip (ignored)\n+\tsyswrite $client, \"\\x00\\x5a\\x00\\x00\\x00\\x00\\x00\\x00\";\n+\n+\tmy $remote = IO::Socket::INET->new(PeerHost => $ip, PeerPort => $port)\n+\t\tor die \"unable to connect to $ip/$port: $!\";\n+\n+\tmy $io = IO::Select->new($client, $remote);\n+\twhile ($io->count) {\n+\t\tfor my $fh ($io->can_read(0)) {\n+\t\t\tfor my $pair ([$client, $remote], [$remote, $client]) {\n+\t\t\t\tmy ($from, $to) = @$pair;\n+\t\t\t\tnext unless $fh == $from;\n+\n+\t\t\t\tmy $r = sysread $from, my $buf, 1024;\n+\t\t\t\tif (!defined $r || $r <= 0) {\n+\t\t\t\t\t$io->remove($from);\n+\t\t\t\t\tnext;\n+\t\t\t\t}\n+\t\t\t\tsyswrite $to, $buf;\n+\t\t\t}\n+\t\t}\n+\t}\n+}\ndiff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\nindex bb35b87071d..7fcffba67a2 100755\n--- a/t/t5564-http-proxy.sh\n+++ b/t/t5564-http-proxy.sh\n@@ -39,4 +39,50 @@ test_expect_success 'clone can prompt for proxy password' '\n \texpect_askpass pass proxuser\n '\n \n+start_socks() {\n+\tmkfifo socks_output &&\n+\t{\n+\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n+\t\tsocks_pid=$!\n+\t} &&\n+\tread line err &&\n+\t\t\tgrep -i \"SOCKS4 request granted.\" trace\n+\t\t} ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n+test_expect_success SOCKS_PROXY 'stop SOCKS proxy' 'kill \"$socks_pid\"'\n+\n+test_expect_success 'Unix socket requires socks*:' '\n+\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n+\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n+test_expect_success 'Unix socket requires localhost' '\n+\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n+\t\tgrep \"^fatal: Invalid proxy URL '\\''socks4://127\\.0\\.0\\.1/path'\\'': host must be localhost if a path is present\" err ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n test_done\n\nbase-commit: 39bf06adf96da25b87c9aa7d35a32ef3683eb4a4\n-- \ngitgitgadget\n"},{"id":"499752","messageId":"xmqqle1hyzcu.fsf@gitster.g","threadId":"61852","inReplyTo":"pull.1767.v3.git.1722441675945.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-31T22:24:01Z","receivedAt":"2024-07-31T22:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ryan Hendrickson via GitGitGadget\" writes:\n\n> @@ -5,8 +5,8 @@ http.proxy::\n> \tproxy string with a user name but no password, in which case git will\n> \tattempt to acquire one in the same way it does for other credentials. See\n> \tlinkgit:gitcredentials[7] for more information. The syntax thus is\n> -\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n> -\ton a per-remote basis; see remote..proxy\n> +\t'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be\n> +\toverridden on a per-remote basis; see remote..proxy\n\nOK.\n\n> diff --git a/http.c b/http.c\n> index 623ed234891..a50ba095889 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1227,6 +1227,7 @@ static CURL *get_curl_handle(void)\n> \t\t */\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n> \t} else if (curl_http_proxy) {\n> +\t\tstruct strbuf proxy = STRBUF_INIT;\n> \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n> \t\t\tcurl_easy_setopt(result,\n> \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n\nIn a block with local variable decl, be more friendly to readers by\nhaving a blank line between the end of declarations and the first\nstatement.\n\n> +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n> +\t\tif (proxy_auth.path) {\n> +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n> +\t\t\tif (ver->version_num < 0x075400)\n> +\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n> +\n> +\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n> +\t\t\t\t curl_http_proxy);\n> +\n> +\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n> +\t\t\t\t curl_http_proxy);\n\nWe insist that it must be \"localhost\", so let's not do strcasecmp()\nbut just do strcmp().\n\n> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\n> index bb35b87071d..7fcffba67a2 100755\n> --- a/t/t5564-http-proxy.sh\n> +++ b/t/t5564-http-proxy.sh\n> @@ -39,4 +39,50 @@ test_expect_success 'clone can prompt for proxy password' '\n> \texpect_askpass pass proxuser\n> '\n> \n> +start_socks() {\n> +\tmkfifo socks_output &&\n> +\t{\n> +\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n> +\t\tsocks_pid=$!\n> +\t} &&\n> +\tread line +\ttest \"$line\" = ready\n> +}\n> +\n> +test_expect_success PERL 'try to start SOCKS proxy' '\n> +\t# The %30 tests that the correct amount of percent-encoding is applied\n> +\t# to the proxy string passed to curl.\n> +\tif start_socks %30.sock\n> +\tthen\n> +\t\ttest_set_prereq SOCKS_PROXY\n> +\tfi\n> +'\n\nMaking it a regular test_expect_success would mean GIT_SKIP_TEST\nmechansim can be used to skip it, which is probably not what you\nwant. Can't this be a more common test_lazy_prereq, perhaps like\n\n\ttest_lazy_prereq SOCKS_PROXY '\n\t\t# useful comment about 30% here ...\n\t\ttest_have_prereq PERL &&\n\t\tstart_socks %30.sock\n\t'\n\nor something?\n\nThanks.\n"},{"id":"499761","messageId":"9ffa9bf5-f799-400f-5e45-ccbc5316ee42@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqle1hyzcu.fsf@gitster.g","subject":"Re: [PATCH v3] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-01T03:44:08Z","receivedAt":"2024-08-01T03:44:15Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-07-31 15:24-0700, Junio C Hamano sent:\n\n> In a block with local variable decl, be more friendly to readers by\n> having a blank line between the end of declarations and the first\n> statement.\n\nWill do.\n\n> We insist that it must be \"localhost\", so let's not do strcasecmp()\n> but just do strcmp().\n\nI don't see the wisdom of being more restrictive than curl is in this \nrespect. The documentation makes the claim that curl's syntax is what this \noption supports, and while that is not literally true due to how protocols \nare handled, I don't see a reason to intentionally deviate further.\n\nI've tested that curl does the right thing with any casing of localhost, \nboth on its own and via Git. Host names are generally case insensitive; \nthe error message should be understood in that context. And if it is \nmisunderstood, there's no negative impact on the user who writes \n\"localhost\" (while there is a negative impact on the user who quite \nreasonably expects \"LOCALHOST\" to work if we don't follow curl's lead).\n\n> Making it a regular test_expect_success would mean GIT_SKIP_TEST\n> mechansim can be used to skip it, which is probably not what you\n> want. Can't this be a more common test_lazy_prereq, perhaps like\n>\n> \ttest_lazy_prereq SOCKS_PROXY '\n> \t\t# useful comment about 30% here ...\n> \t\ttest_have_prereq PERL &&\n> \t\tstart_socks %30.sock\n> \t'\n>\n> or something?\n\nThis existing test file is definitely not written with running individual \ntests in mind; the first test is a prerequisite for all that comes after, \nand the second test seems to be required for tests 3–5 as well.\n\nBut sure, I'll use that pattern if you like.\n\nR"},{"id":"499762","messageId":"xmqqplqsyg1i.fsf@gitster.g","threadId":"61852","inReplyTo":"9ffa9bf5-f799-400f-5e45-ccbc5316ee42@alum.mit.edu","subject":"Re: [PATCH v3] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T05:21:13Z","receivedAt":"2024-08-01T05:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n>> We insist that it must be \"localhost\", so let's not do strcasecmp()\n>> but just do strcmp().\n>\n> I don't see the wisdom of being more restrictive than curl is in this\n> respect.\n\nAh, if curl is doing case insensitivity, then matching its behaviour\nis perfectly fine. I was just reacting to this message ...\n\n> +\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n\n... that results when strcasecmp() does not like it.\n\nThanks.\n"},{"id":"499763","messageId":"pull.1767.v4.git.1722489776279.gitgitgadget@gmail.com","threadId":"61852","inReplyTo":"pull.1767.v3.git.1722441675945.gitgitgadget@gmail.com","subject":"[PATCH v4] http: do not ignore proxy path","fromName":"Ryan Hendrickson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-01T05:22:56Z","receivedAt":"2024-08-01T05:23:00Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"From: Ryan Hendrickson \n\nThe documentation for `http.proxy` describes that option, and the\nenvironment variables it overrides, as supporting \"the syntax understood\nby curl\". curl allows SOCKS proxies to use a path to a Unix domain\nsocket, like `socks5h://localhost/path/to/socket.sock`. Git should\ntherefore include, if present, the path part of the proxy URL in what it\npasses to libcurl.\n\nCo-authored-by: Jeff King \nSigned-off-by: Ryan Hendrickson \n---\n http: do not ignore proxy path\n \n Re earlier discussion about regressions, it turns out that curl has only\n supported this syntax since 2022 [https://curl.se/ch/7.84.0.html].\n Earlier versions of curl, if provided an http_proxy that contained a\n path, would also ignore it. curl didn't seem to consider this a breaking\n change when the feature was introduced\n [https://github.com/curl/curl/pull/8668], though; is that a valid\n argument for Git not to lose sleep over it either?\n \n A test has been added, but unfortunately it is not being run on any of\n GitHub's CI workflows, because the runner images in use all have\n versions of libcurl that are too old. It works on my machine, but\n unknown troubles may await someone else.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1767%2Frhendric%2Frhendric%2Fhttp-proxy-path-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1767/rhendric/rhendric/http-proxy-path-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1767\n\nRange-diff vs v3:\n\n 1: 7a58da7102e ! 1: 507fb75c1a6 http: do not ignore proxy path\n @@ http.c: static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n \t} else if (curl_http_proxy) {\n +\t\tstruct strbuf proxy = STRBUF_INIT;\n ++\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n @@ http.c: static CURL *get_curl_handle(void)\n +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n +\t\tif (proxy_auth.path) {\n +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n ++\n +\t\t\tif (ver->version_num < 0x075400)\n +\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n +\n @@ t/t5564-http-proxy.sh: test_expect_success 'clone can prompt for proxy password'\n +\tmkfifo socks_output &&\n +\t{\n +\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n -+\t\tsocks_pid=$!\n ++\t\techo $! > \"$TRASH_DIRECTORY/socks.pid\"\n +\t} &&\n +\tread line err && {\n +\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n\n\n Documentation/config/http.txt | 4 +--\n http.c | 24 +++++++++++++++++-\n t/socks4-proxy.pl | 48 +++++++++++++++++++++++++++++++++++\n t/t5564-http-proxy.sh | 41 ++++++++++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 3 deletions(-)\n create mode 100644 t/socks4-proxy.pl\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 162b33fc52f..a14371b5c96 100644\n--- a/Documentation/config/http.txt\n+++ b/Documentation/config/http.txt\n@@ -5,8 +5,8 @@ http.proxy::\n \tproxy string with a user name but no password, in which case git will\n \tattempt to acquire one in the same way it does for other credentials. See\n \tlinkgit:gitcredentials[7] for more information. The syntax thus is\n-\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n-\ton a per-remote basis; see remote..proxy\n+\t'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be\n+\toverridden on a per-remote basis; see remote..proxy\n +\n Any proxy, however configured, must be completely transparent and must not\n modify, transform, or buffer the request or response in any way. Proxies which\ndiff --git a/http.c b/http.c\nindex 623ed234891..6c6cc5c822a 100644\n--- a/http.c\n+++ b/http.c\n@@ -1227,6 +1227,8 @@ static CURL *get_curl_handle(void)\n \t\t */\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n \t} else if (curl_http_proxy) {\n+\t\tstruct strbuf proxy = STRBUF_INIT;\n+\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n@@ -1265,7 +1267,27 @@ static CURL *get_curl_handle(void)\n \t\tif (!proxy_auth.host)\n \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n+\t\tif (proxy_auth.path) {\n+\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n+\n+\t\t\tif (ver->version_num < 0x075400)\n+\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n+\n+\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tstrbuf_addch(&proxy, '/');\n+\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n+\t\t}\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n+\t\tstrbuf_release(&proxy);\n+\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\ndiff --git a/t/socks4-proxy.pl b/t/socks4-proxy.pl\nnew file mode 100644\nindex 00000000000..4c3a35c0083\n--- /dev/null\n+++ b/t/socks4-proxy.pl\n@@ -0,0 +1,48 @@\n+use strict;\n+use IO::Select;\n+use IO::Socket::UNIX;\n+use IO::Socket::INET;\n+\n+my $path = shift;\n+\n+unlink($path);\n+my $server = IO::Socket::UNIX->new(Listen => 1, Local => $path)\n+\tor die \"unable to listen on $path: $!\";\n+\n+$| = 1;\n+print \"ready\\n\";\n+\n+while (my $client = $server->accept()) {\n+\tsysread $client, my $buf, 8;\n+\tmy ($version, $cmd, $port, $ip) = unpack 'CCnN', $buf;\n+\tnext unless $version == 4; # socks4\n+\tnext unless $cmd == 1; # TCP stream connection\n+\n+\t# skip NUL-terminated id\n+\twhile (sysread $client, my $char, 1) {\n+\t\tlast unless ord($char);\n+\t}\n+\n+\t# version(0), reply(5a == granted), port (ignored), ip (ignored)\n+\tsyswrite $client, \"\\x00\\x5a\\x00\\x00\\x00\\x00\\x00\\x00\";\n+\n+\tmy $remote = IO::Socket::INET->new(PeerHost => $ip, PeerPort => $port)\n+\t\tor die \"unable to connect to $ip/$port: $!\";\n+\n+\tmy $io = IO::Select->new($client, $remote);\n+\twhile ($io->count) {\n+\t\tfor my $fh ($io->can_read(0)) {\n+\t\t\tfor my $pair ([$client, $remote], [$remote, $client]) {\n+\t\t\t\tmy ($from, $to) = @$pair;\n+\t\t\t\tnext unless $fh == $from;\n+\n+\t\t\t\tmy $r = sysread $from, my $buf, 1024;\n+\t\t\t\tif (!defined $r || $r <= 0) {\n+\t\t\t\t\t$io->remove($from);\n+\t\t\t\t\tnext;\n+\t\t\t\t}\n+\t\t\t\tsyswrite $to, $buf;\n+\t\t\t}\n+\t\t}\n+\t}\n+}\ndiff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\nindex bb35b87071d..039d7fc748e 100755\n--- a/t/t5564-http-proxy.sh\n+++ b/t/t5564-http-proxy.sh\n@@ -39,4 +39,45 @@ test_expect_success 'clone can prompt for proxy password' '\n \texpect_askpass pass proxuser\n '\n \n+start_socks() {\n+\tmkfifo socks_output &&\n+\t{\n+\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n+\t\techo $! > \"$TRASH_DIRECTORY/socks.pid\"\n+\t} &&\n+\tread line err &&\n+\t\t\tgrep -i \"SOCKS4 request granted.\" trace\n+\t\t} ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n+test_expect_success 'Unix socket requires socks*:' '\n+\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n+\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n+test_expect_success 'Unix socket requires localhost' '\n+\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n+\t\tgrep \"^fatal: Invalid proxy URL '\\''socks4://127\\.0\\.0\\.1/path'\\'': host must be localhost if a path is present\" err ||\n+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n+\t}\n+'\n+\n test_done\n\nbase-commit: 39bf06adf96da25b87c9aa7d35a32ef3683eb4a4\n-- \ngitgitgadget\n"},{"id":"499764","messageId":"20240801054523.GA621899@coredump.intra.peff.net","threadId":"61852","inReplyTo":"xmqqle1hyzcu.fsf@gitster.g","subject":"Re: [PATCH v3] http: do not ignore proxy path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T05:45:23Z","receivedAt":"2024-08-01T05:45:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2024 at 03:24:01PM -0700, Junio C Hamano wrote:\n\n> > +start_socks() {\n> > +\tmkfifo socks_output &&\n> > +\t{\n> > +\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n> > +\t\tsocks_pid=$!\n> > +\t} &&\n> > +\tread line > +\ttest \"$line\" = ready\n> > +}\n> > +\n> > +test_expect_success PERL 'try to start SOCKS proxy' '\n> > +\t# The %30 tests that the correct amount of percent-encoding is applied\n> > +\t# to the proxy string passed to curl.\n> > +\tif start_socks %30.sock\n> > +\tthen\n> > +\t\ttest_set_prereq SOCKS_PROXY\n> > +\tfi\n> > +'\n> \n> Making it a regular test_expect_success would mean GIT_SKIP_TEST\n> mechansim can be used to skip it, which is probably not what you\n> want. Can't this be a more common test_lazy_prereq, perhaps like\n> \n> \ttest_lazy_prereq SOCKS_PROXY '\n> \t\t# useful comment about 30% here ...\n> \t\ttest_have_prereq PERL &&\n> \t\tstart_socks %30.sock\n> \t'\n> \n> or something?\n\nI think Ryan picked up this approach from my earlier mail. And the\nreason I suggested doing it this way is that the prereq test has an\nimportant side effect: it creates the socket and starts the proxy\nserver!\n\nI think lazy prereqs only make sense when their only output is the\nyes/no of whether we match the prereq. And then we don't care when or\nhow often they are evaluated. I do think things would work, assuming the\nproxy server then can serve multiple tests. It just feels weird, and\ndoing it more like the git-daemon/http tests made more sense to me\n(though those ones do not even do their work inside an expect block).\n\nIf we did it in a lazy prereq I think you'd need to munge the path, as\nthe lazy prereq operates in a throwaway directory (so the %30.sock would\nget removed before we can use it in the next test).\n\n-Peff\n"},{"id":"499765","messageId":"20240801060418.GB621899@coredump.intra.peff.net","threadId":"61852","inReplyTo":"pull.1767.v4.git.1722489776279.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] http: do not ignore proxy path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T06:04:18Z","receivedAt":"2024-08-01T06:04:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2024 at 05:22:56AM +0000, Ryan Hendrickson via GitGitGadget wrote:\n\n> From: Ryan Hendrickson \n> \n> The documentation for `http.proxy` describes that option, and the\n> environment variables it overrides, as supporting \"the syntax understood\n> by curl\". curl allows SOCKS proxies to use a path to a Unix domain\n> socket, like `socks5h://localhost/path/to/socket.sock`. Git should\n> therefore include, if present, the path part of the proxy URL in what it\n> passes to libcurl.\n> \n> Co-authored-by: Jeff King \n> Signed-off-by: Ryan Hendrickson \n\nThanks for crediting me. I'll add my:\n\n Signed-off-by: Jeff King \n\nto be explicit that the proxy script is under the DCO.\n\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n> +\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n> +\t\tif (proxy_auth.path) {\n> +\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n> +\n> +\t\t\tif (ver->version_num < 0x075400)\n> +\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n> +\n> +\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n> +\t\t\t\t curl_http_proxy);\n> +\n> +\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n> +\t\t\t\t curl_http_proxy);\n> +\n> +\t\t\tstrbuf_addch(&proxy, '/');\n> +\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n\nThis all looks good to me. I wondered briefly whether\nproxy_auth.protocol could ever be NULL, but I think we refuse to parse\nsuch a URL.\n\n> +start_socks() {\n> +\tmkfifo socks_output &&\n> +\t{\n> +\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n> +\t\techo $! > \"$TRASH_DIRECTORY/socks.pid\"\n> +\t} &&\n> +\tread line +\ttest \"$line\" = ready\n> +}\n> +\n> +# The %30 tests that the correct amount of percent-encoding is applied to the\n> +# proxy string passed to curl.\n> +test_lazy_prereq SOCKS_PROXY 'test_have_prereq PERL && start_socks \"$TRASH_DIRECTORY/%30.sock\"'\n\nOK, I see you figured out that the lazy prereq requires giving the full\npath to the socket. :) I had forgotten that we also run the prereq in a\nsubshell to avoid side effects, but you caught that, as well.\n\nAll of this to me is good evidence that the non-lazy version you had\noriginally is a better approach. But I don't think it's worth spending\ntime fighting over, so I'm OK either way.\n\n> +test_atexit 'test ! -e \"$TRASH_DIRECTORY/socks.pid\" || kill \"$(cat \"$TRASH_DIRECTORY/socks.pid\")\"'\n\nAh, good. I had meant to mention cleaning up at the end (as we do for\ngit-daemon and apache), but forgot. I'm glad you included it here.\n\n> +test_expect_success SOCKS_PROXY 'clone via Unix socket' '\n> +\ttest_when_finished \"rm -rf clone\" &&\n> +\ttest_config_global http.proxy \"socks4://localhost$PWD/%2530.sock\" && {\n> +\t\t{\n> +\t\t\tGIT_TRACE_CURL=$PWD/trace git clone \"$HTTPD_URL/smart/repo.git\" clone 2>err &&\n> +\t\t\tgrep -i \"SOCKS4 request granted.\" trace\n> +\t\t} ||\n> +\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n> +\t}\n> +'\n\nLooks good. Usually I do not bother with escaping \".\" in grep messages,\nas it is already a loose match and it keeps the test easier to read. But\nit is OK to do so.\n\nWe do have a test_grep wrapper which gives nicer output when the match\nfails, but maybe that is a bad choice here, since we accept either of\nthe two messages. (Likewise for using test_cmp, I suppose).\n\nThe use of \"||\" in the command-chaining is unusual enough that it's\nprobably worth calling out either in a comment or in the commit message.\n\n> +test_expect_success 'Unix socket requires socks*:' '\n> +\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n> +\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n> +\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n> +\t}\n> +'\n> +\n> +test_expect_success 'Unix socket requires localhost' '\n> +\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n> +\t\tgrep \"^fatal: Invalid proxy URL '\\''socks4://127\\.0\\.0\\.1/path'\\'': host must be localhost if a path is present\" err ||\n> +\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n> +\t}\n> +'\n> +\n\nLikewise here, I'd probably just match the single-quotes with \".\" for\nreadability (but it's OK to write it as you did). You can (since a week\nor so ago) also use a here-doc body like:\n\n test_expect_success 'Unix socket requires socks*:' - <<\\EOT\n\t...\n\ttest_grep \"^fatal: Invalid proxy URL 'localhost/path': only SOCKS proxies support paths\" err ||\n\t...\n EOT\n\nbut I'm OK with it either way. The same \"||\" comment applies.\n\n-Peff\n"},{"id":"499850","messageId":"xmqqfrroxq5c.fsf@gitster.g","threadId":"61852","inReplyTo":"20240801054523.GA621899@coredump.intra.peff.net","subject":"Re: [PATCH v3] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T14:40:31Z","receivedAt":"2024-08-01T14:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think Ryan picked up this approach from my earlier mail. And the\n> reason I suggested doing it this way is that the prereq test has an\n> important side effect: it creates the socket and starts the proxy\n> server!\n\nAh, OK.\n\n> I think lazy prereqs only make sense when their only output is the\n> yes/no of whether we match the prereq. And then we don't care when or\n> how often they are evaluated.\n\nOnce the prereq test is attempted the result is cached (that's the\nwhole point of lazy prereq mechanism) so we won't see multiple sock\nproxies spawned.\n\n> I do think things would work, assuming the\n> proxy server then can serve multiple tests. It just feels weird, and\n> doing it more like the git-daemon/http tests made more sense to me\n> (though those ones do not even do their work inside an expect block).\n\nOK. That's fine. It is probably not a good fit for the pattern.\n\n> If we did it in a lazy prereq I think you'd need to munge the path, as\n> the lazy prereq operates in a throwaway directory (so the %30.sock would\n> get removed before we can use it in the next test).\n\nTrue.\n"},{"id":"499877","messageId":"xmqqo76cuqc9.fsf@gitster.g","threadId":"61852","inReplyTo":"20240801060418.GB621899@coredump.intra.peff.net","subject":"Re: [PATCH v4] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T17:04:38Z","receivedAt":"2024-08-01T17:04:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 01, 2024 at 05:22:56AM +0000, Ryan Hendrickson via GitGitGadget wrote:\n>\n>> From: Ryan Hendrickson \n>> \n>> The documentation for `http.proxy` describes that option, and the\n>> environment variables it overrides, as supporting \"the syntax understood\n>> by curl\". curl allows SOCKS proxies to use a path to a Unix domain\n>> socket, like `socks5h://localhost/path/to/socket.sock`. Git should\n>> therefore include, if present, the path part of the proxy URL in what it\n>> passes to libcurl.\n>> \n>> Co-authored-by: Jeff King \n>> Signed-off-by: Ryan Hendrickson \n>\n> Thanks for crediting me. I'll add my:\n>\n> Signed-off-by: Jeff King \n>\n> to be explicit that the proxy script is under the DCO.\n\nOK, I'll amend it while queuing this v4.\n\nThanks.\n\n>> +# The %30 tests that the correct amount of percent-encoding is applied to the\n>> +# proxy string passed to curl.\n>> +test_lazy_prereq SOCKS_PROXY 'test_have_prereq PERL && start_socks \"$TRASH_DIRECTORY/%30.sock\"'\n>\n> OK, I see you figured out that the lazy prereq requires giving the full\n> path to the socket. :) I had forgotten that we also run the prereq in a\n> subshell to avoid side effects, but you caught that, as well.\n\n;-)\n\n> All of this to me is good evidence that the non-lazy version you had\n> originally is a better approach. But I don't think it's worth spending\n> time fighting over, so I'm OK either way.\n\nI'd be OK either way, too.\n\nThanks, both.\n"},{"id":"499931","messageId":"pull.1767.v5.git.1722576007398.gitgitgadget@gmail.com","threadId":"61852","inReplyTo":"pull.1767.v4.git.1722489776279.gitgitgadget@gmail.com","subject":"[PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T05:20:07Z","receivedAt":"2024-08-02T05:20:10Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"From: Ryan Hendrickson \n\nThe documentation for `http.proxy` describes that option, and the\nenvironment variables it overrides, as supporting \"the syntax understood\nby curl\". curl allows SOCKS proxies to use a path to a Unix domain\nsocket, like `socks5h://localhost/path/to/socket.sock`. Git should\ntherefore include, if present, the path part of the proxy URL in what it\npasses to libcurl.\n\nCo-authored-by: Jeff King \nSigned-off-by: Ryan Hendrickson \nSigned-off-by: Jeff King \n---\n http: do not ignore proxy path\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1767%2Frhendric%2Frhendric%2Fhttp-proxy-path-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1767/rhendric/rhendric/http-proxy-path-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1767\n\nRange-diff vs v4:\n\n 1: 507fb75c1a6 ! 1: fa101a3b264 http: do not ignore proxy path\n @@ Commit message\n \n Co-authored-by: Jeff King \n Signed-off-by: Ryan Hendrickson \n + Signed-off-by: Jeff King \n \n ## Documentation/config/http.txt ##\n @@ Documentation/config/http.txt: http.proxy::\n @@ t/t5564-http-proxy.sh: test_expect_success 'clone can prompt for proxy password'\n +\n +test_atexit 'test ! -e \"$TRASH_DIRECTORY/socks.pid\" || kill \"$(cat \"$TRASH_DIRECTORY/socks.pid\")\"'\n +\n ++# The below tests morally ought to be gated on a prerequisite that Git is\n ++# linked with a libcurl that supports Unix socket paths for proxies (7.84 or\n ++# later), but this is not easy to test right now. Instead, we || the tests with\n ++# this function.\n ++old_libcurl_error() {\n ++\tgrep -Fx \"fatal: libcurl 7.84 or later is required to support paths in proxy URLs\" \"$1\"\n ++}\n ++\n +test_expect_success SOCKS_PROXY 'clone via Unix socket' '\n +\ttest_when_finished \"rm -rf clone\" &&\n +\ttest_config_global http.proxy \"socks4://localhost$PWD/%2530.sock\" && {\n +\t\t{\n +\t\t\tGIT_TRACE_CURL=$PWD/trace git clone \"$HTTPD_URL/smart/repo.git\" clone 2>err &&\n -+\t\t\tgrep -i \"SOCKS4 request granted.\" trace\n ++\t\t\tgrep -i \"SOCKS4 request granted\" trace\n +\t\t} ||\n -+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t\told_libcurl_error err\n +\t}\n +'\n +\n -+test_expect_success 'Unix socket requires socks*:' '\n ++test_expect_success 'Unix socket requires socks*:' - <<\\EOT\n +\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n -+\t\tgrep \"^fatal: Invalid proxy URL '\\''localhost/path'\\'': only SOCKS proxies support paths\" err ||\n -+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t\tgrep -Fx \"fatal: Invalid proxy URL 'localhost/path': only SOCKS proxies support paths\" err ||\n ++\t\told_libcurl_error err\n +\t}\n -+'\n ++EOT\n +\n -+test_expect_success 'Unix socket requires localhost' '\n ++test_expect_success 'Unix socket requires localhost' - <<\\EOT\n +\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n -+\t\tgrep \"^fatal: Invalid proxy URL '\\''socks4://127\\.0\\.0\\.1/path'\\'': host must be localhost if a path is present\" err ||\n -+\t\tgrep \"^fatal: libcurl 7\\.84 or later\" err\n ++\t\tgrep -Fx \"fatal: Invalid proxy URL 'socks4://127.0.0.1/path': host must be localhost if a path is present\" err ||\n ++\t\told_libcurl_error err\n +\t}\n -+'\n ++EOT\n +\n test_done\n\n\n Documentation/config/http.txt | 4 +--\n http.c | 24 ++++++++++++++++-\n t/socks4-proxy.pl | 48 ++++++++++++++++++++++++++++++++++\n t/t5564-http-proxy.sh | 49 +++++++++++++++++++++++++++++++++++\n 4 files changed, 122 insertions(+), 3 deletions(-)\n create mode 100644 t/socks4-proxy.pl\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 162b33fc52f..a14371b5c96 100644\n--- a/Documentation/config/http.txt\n+++ b/Documentation/config/http.txt\n@@ -5,8 +5,8 @@ http.proxy::\n \tproxy string with a user name but no password, in which case git will\n \tattempt to acquire one in the same way it does for other credentials. See\n \tlinkgit:gitcredentials[7] for more information. The syntax thus is\n-\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n-\ton a per-remote basis; see remote..proxy\n+\t'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be\n+\toverridden on a per-remote basis; see remote..proxy\n +\n Any proxy, however configured, must be completely transparent and must not\n modify, transform, or buffer the request or response in any way. Proxies which\ndiff --git a/http.c b/http.c\nindex 623ed234891..6c6cc5c822a 100644\n--- a/http.c\n+++ b/http.c\n@@ -1227,6 +1227,8 @@ static CURL *get_curl_handle(void)\n \t\t */\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n \t} else if (curl_http_proxy) {\n+\t\tstruct strbuf proxy = STRBUF_INIT;\n+\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);\n@@ -1265,7 +1267,27 @@ static CURL *get_curl_handle(void)\n \t\tif (!proxy_auth.host)\n \t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\tstrbuf_addstr(&proxy, proxy_auth.host);\n+\t\tif (proxy_auth.path) {\n+\t\t\tcurl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);\n+\n+\t\t\tif (ver->version_num < 0x075400)\n+\t\t\t\tdie(\"libcurl 7.84 or later is required to support paths in proxy URLs\");\n+\n+\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n+\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n+\t\t\t\t curl_http_proxy);\n+\n+\t\t\tstrbuf_addch(&proxy, '/');\n+\t\t\tstrbuf_add_percentencode(&proxy, proxy_auth.path, 0);\n+\t\t}\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);\n+\t\tstrbuf_release(&proxy);\n+\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n \t\tcurl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);\ndiff --git a/t/socks4-proxy.pl b/t/socks4-proxy.pl\nnew file mode 100644\nindex 00000000000..4c3a35c0083\n--- /dev/null\n+++ b/t/socks4-proxy.pl\n@@ -0,0 +1,48 @@\n+use strict;\n+use IO::Select;\n+use IO::Socket::UNIX;\n+use IO::Socket::INET;\n+\n+my $path = shift;\n+\n+unlink($path);\n+my $server = IO::Socket::UNIX->new(Listen => 1, Local => $path)\n+\tor die \"unable to listen on $path: $!\";\n+\n+$| = 1;\n+print \"ready\\n\";\n+\n+while (my $client = $server->accept()) {\n+\tsysread $client, my $buf, 8;\n+\tmy ($version, $cmd, $port, $ip) = unpack 'CCnN', $buf;\n+\tnext unless $version == 4; # socks4\n+\tnext unless $cmd == 1; # TCP stream connection\n+\n+\t# skip NUL-terminated id\n+\twhile (sysread $client, my $char, 1) {\n+\t\tlast unless ord($char);\n+\t}\n+\n+\t# version(0), reply(5a == granted), port (ignored), ip (ignored)\n+\tsyswrite $client, \"\\x00\\x5a\\x00\\x00\\x00\\x00\\x00\\x00\";\n+\n+\tmy $remote = IO::Socket::INET->new(PeerHost => $ip, PeerPort => $port)\n+\t\tor die \"unable to connect to $ip/$port: $!\";\n+\n+\tmy $io = IO::Select->new($client, $remote);\n+\twhile ($io->count) {\n+\t\tfor my $fh ($io->can_read(0)) {\n+\t\t\tfor my $pair ([$client, $remote], [$remote, $client]) {\n+\t\t\t\tmy ($from, $to) = @$pair;\n+\t\t\t\tnext unless $fh == $from;\n+\n+\t\t\t\tmy $r = sysread $from, my $buf, 1024;\n+\t\t\t\tif (!defined $r || $r <= 0) {\n+\t\t\t\t\t$io->remove($from);\n+\t\t\t\t\tnext;\n+\t\t\t\t}\n+\t\t\t\tsyswrite $to, $buf;\n+\t\t\t}\n+\t\t}\n+\t}\n+}\ndiff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\nindex bb35b87071d..0d6cfebbfab 100755\n--- a/t/t5564-http-proxy.sh\n+++ b/t/t5564-http-proxy.sh\n@@ -39,4 +39,53 @@ test_expect_success 'clone can prompt for proxy password' '\n \texpect_askpass pass proxuser\n '\n \n+start_socks() {\n+\tmkfifo socks_output &&\n+\t{\n+\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n+\t\techo $! > \"$TRASH_DIRECTORY/socks.pid\"\n+\t} &&\n+\tread line err &&\n+\t\t\tgrep -i \"SOCKS4 request granted\" trace\n+\t\t} ||\n+\t\told_libcurl_error err\n+\t}\n+'\n+\n+test_expect_success 'Unix socket requires socks*:' - <<\\EOT\n+\t! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {\n+\t\tgrep -Fx \"fatal: Invalid proxy URL 'localhost/path': only SOCKS proxies support paths\" err ||\n+\t\told_libcurl_error err\n+\t}\n+EOT\n+\n+test_expect_success 'Unix socket requires localhost' - <<\\EOT\n+\t! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {\n+\t\tgrep -Fx \"fatal: Invalid proxy URL 'socks4://127.0.0.1/path': host must be localhost if a path is present\" err ||\n+\t\told_libcurl_error err\n+\t}\n+EOT\n+\n test_done\n\nbase-commit: 406f326d271e0bacecdb00425422c5fa3f314930\n-- \ngitgitgadget\n"},{"id":"499954","messageId":"xmqq7ccygbx6.fsf@gitster.g","threadId":"61852","inReplyTo":"pull.1767.v5.git.1722576007398.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T15:52:05Z","receivedAt":"2024-08-02T15:52:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ryan Hendrickson via GitGitGadget\" writes:\n\n> From: Ryan Hendrickson \n>\n> The documentation for `http.proxy` describes that option, and the\n> environment variables it overrides, as supporting \"the syntax understood\n> by curl\". curl allows SOCKS proxies to use a path to a Unix domain\n> socket, like `socks5h://localhost/path/to/socket.sock`. Git should\n> therefore include, if present, the path part of the proxy URL in what it\n> passes to libcurl.\n>\n> Co-authored-by: Jeff King \n> Signed-off-by: Ryan Hendrickson \n> Signed-off-by: Jeff King \n> ---\n\nThe trailer lines should be ordered in chronological order to record\nthe provenance of the patch. The last two entries make it look as\nif what you assembled and signed-off was relayed by peff, with or\nwithout further modification, with his sign-off to me, to become the\nfinal version, but that is not the story you want to tell.\n\nI'd swap the two lines (i.e., sign-offs) while queuing.\n\n> +\t\t\tif (!starts_with(proxy_auth.protocol, \"socks\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': only SOCKS proxies support paths\",\n> +\t\t\t\t curl_http_proxy);\n\nOur error messages that are prefixed with \"fatal:\" do not typically\nbegin with a capital letter.\n\nBut I'll let it pass, as this copies an existing message in this\nfile. #leftoverbits clean-up needs to correct the ones added by\nthis patch and existing \"Invalid proxy URL '%s'\" by downcasing\n\"Invalid\", possibly enclose the message in _() for i18n, and also\ndowncase \"C\" in two \"Could not set SSL ...\" messages.\n\n> +\t\t\tif (strcasecmp(proxy_auth.host, \"localhost\"))\n> +\t\t\t\tdie(\"Invalid proxy URL '%s': host must be localhost if a path is present\",\n> +\t\t\t\t curl_http_proxy);\n\nDitto.\n\nOr instead of leaving this for a later clean-up after the dust\nsettles, we could have a separate \"preliminary clean-up\" patch to\naddress existing ones first. Either is fine, but taking \"clean-up\nafter the dust settles\" and leaving it a problem for other people is\nprobably easier.\n\n> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh\n> index bb35b87071d..0d6cfebbfab 100755\n> --- a/t/t5564-http-proxy.sh\n> +++ b/t/t5564-http-proxy.sh\n> @@ -39,4 +39,53 @@ test_expect_success 'clone can prompt for proxy password' '\n> \texpect_askpass pass proxuser\n> '\n> \n> +start_socks() {\n> +\tmkfifo socks_output &&\n> +\t{\n> +\t\t\"$PERL_PATH\" \"$TEST_DIRECTORY/socks4-proxy.pl\" \"$1\" >socks_output &\n> +\t\techo $! > \"$TRASH_DIRECTORY/socks.pid\"\n> +\t} &&\n> +\tread line +\ttest \"$line\" = ready\n> +}\n> +\n> +# The %30 tests that the correct amount of percent-encoding is applied to the\n> +# proxy string passed to curl.\n> +test_lazy_prereq SOCKS_PROXY 'test_have_prereq PERL && start_socks \"$TRASH_DIRECTORY/%30.sock\"'\n> +\n> +test_atexit 'test ! -e \"$TRASH_DIRECTORY/socks.pid\" || kill \"$(cat \"$TRASH_DIRECTORY/socks.pid\")\"'\n\nLet's line-wrap these overly long ones.\n\n # The %30 tests that the correct amount of percent-encoding is\n # applied to the proxy string passed to curl.\n test_lazy_prereq SOCKS_PROXY '\n test_have_prereq PERL &&\n start_socks \"$TRASH_DIRECTORY/%30.sock\"\n '\n\n test_atexit '\n test ! -e \"$TRASH_DIRECTORY/socks.pid\" ||\n kill \"$(cat \"$TRASH_DIRECTORY/socks.pid\")\"\n '\n\n> +# The below tests morally ought to be gated on a prerequisite that Git is\n> +# linked with a libcurl that supports Unix socket paths for proxies (7.84 or\n> +# later), but this is not easy to test right now. Instead, we || the tests with\n> +# this function.\n> +old_libcurl_error() {\n> +\tgrep -Fx \"fatal: libcurl 7.84 or later is required to support paths in proxy URLs\" \"$1\"\n> +}\n\nCute. \n\nThis fixes the polarity of the result from the whole \"{ test } ||\nthis helper\" construct. Even if the test proper fails, we will\nallow such a failure only when the reason of the failure is due to\nthis message.\n\nIf I were writing this, I would shorten to look for a bit fuzzier\npattern like\n\n grep \"^fatal: .* is required to support paths in proxy URLs\" \"$1\"\n\nas that would allow us to fix the code later without needing to\nupdate the pattern, if we discover reasons, other than being older\nthan libcURL 7.84, why paths in proxy URLs cannot be supported.\n\nOther than that, very nicely done.\n\nThanks.\n"},{"id":"499959","messageId":"2ba77de5-f103-c2f0-c009-71700c8a020d@alum.mit.edu","threadId":"61852","inReplyTo":"xmqq7ccygbx6.fsf@gitster.g","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-02T16:43:07Z","receivedAt":"2024-08-02T16:43:16Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-08-02 08:52-0700, Junio C Hamano sent:\n\n> I'd swap the two lines (i.e., sign-offs) while queuing.\n\nOkay.\n\n> Our error messages that are prefixed with \"fatal:\" do not typically\n> begin with a capital letter.\n>\n> But I'll let it pass, as this copies an existing message in this\n> file. #leftoverbits clean-up needs to correct the ones added by\n> this patch and existing \"Invalid proxy URL '%s'\" by downcasing\n> \"Invalid\", possibly enclose the message in _() for i18n, and also\n> downcase \"C\" in two \"Could not set SSL ...\" messages.\n>\n> [ . . . ]\n>\n> Or instead of leaving this for a later clean-up after the dust\n> settles, we could have a separate \"preliminary clean-up\" patch to\n> address existing ones first. Either is fine, but taking \"clean-up\n> after the dust settles\" and leaving it a problem for other people is\n> probably easier.\n\nHmm. I'd be inclined to take the preliminary clean-up approach, but some \nof the existing strings (there are also two \"Unsupported ...\"/\"Supported \n...\" strings near the \"Could not set...\"s) are going through gettext, and \nI'm reluctant to interfere with the l10n process.\n\nWould a partial clean-up that downcased only the \"Invalid proxy URL\" \nmessage be acceptable, or worse than leaving this as #leftoverbits?\n\n> Let's line-wrap these overly long ones.\n\nOkay.\n\n> If I were writing this, I would shorten to look for a bit fuzzier\n> pattern like\n>\n> grep \"^fatal: .* is required to support paths in proxy URLs\" \"$1\"\n>\n> as that would allow us to fix the code later without needing to\n> update the pattern, if we discover reasons, other than being older\n> than libcURL 7.84, why paths in proxy URLs cannot be supported.\n\nIs this blocking feedback? This strikes me as speculative \nover-engineering; it would not be difficult to generalize the message, and \npossibly then choose a new, more appropriate function name and update the \nexplanatory comment, when and if such reasons arise.\n\nR\n"},{"id":"499962","messageId":"xmqqv80idf52.fsf@gitster.g","threadId":"61852","inReplyTo":"2ba77de5-f103-c2f0-c009-71700c8a020d@alum.mit.edu","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T17:10:49Z","receivedAt":"2024-08-02T17:10:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> Hmm. I'd be inclined to take the preliminary clean-up approach, but\n> some of the existing strings (there are also two \"Unsupported\n> ...\"/\"Supported ...\" strings near the \"Could not set...\"s) are going\n> through gettext, and I'm reluctant to interfere with the l10n process.\n\nI do not see what you mean by interfering with the localization.\n\nIf we are updating text to be translated anyway, giving translators\nthe strings that need to be translated _earlier_ rather than later\nwould be more helpful to them, no?\n\n>> If I were writing this, I would shorten to look for a bit fuzzier\n>> pattern like\n>>\n>> grep \"^fatal: .* is required to support paths in proxy URLs\" \"$1\"\n>>\n>> as that would allow us to fix the code later without needing to\n>> update the pattern, if we discover reasons, other than being older\n>> than libcURL 7.84, why paths in proxy URLs cannot be supported.\n>\n> Is this blocking feedback? This strikes me as speculative\n> over-engineering\n\nNo, it is loosening a pattern that is overly tight and as a side\neffect shortening the line to more readable length ;-).\n"},{"id":"499965","messageId":"a0b916a4-8941-4c06-263d-0ae92dcaf29e@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqv80idf52.fsf@gitster.g","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-02T18:03:08Z","receivedAt":"2024-08-02T18:03:13Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-08-02 10:10-0700, Junio C Hamano sent:\n\n> Ryan Hendrickson writes:\n>\n>> Hmm. I'd be inclined to take the preliminary clean-up approach, but\n>> some of the existing strings (there are also two \"Unsupported\n>> ...\"/\"Supported ...\" strings near the \"Could not set...\"s) are going\n>> through gettext, and I'm reluctant to interfere with the l10n process.\n>\n> I do not see what you mean by interfering with the localization.\n>\n> If we are updating text to be translated anyway, giving translators\n> the strings that need to be translated _earlier_ rather than later\n> would be more helpful to them, no?\n\nProbably true, but as a new contributor I don't know whether changing \nmsgids means more people need to review the patch, more files need to be \nchanged, a translation team needs to be notified, the change needs to be \npushed a different branch... whatever your process is. Localized strings \nare generally more of a headache for drive-by contributors, in my \nexperience across different projects.\n\n>> Is this blocking feedback? This strikes me as speculative\n>> over-engineering\n>\n> No, it is loosening a pattern that is overly tight and as a side\n> effect shortening the line to more readable length ;-).\n\nBlocking or not?\n\nR\n"},{"id":"499967","messageId":"xmqqh6c2d8qt.fsf@gitster.g","threadId":"61852","inReplyTo":"a0b916a4-8941-4c06-263d-0ae92dcaf29e@alum.mit.edu","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T19:28:58Z","receivedAt":"2024-08-02T19:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> At 2024-08-02 10:10-0700, Junio C Hamano sent:\n>\n>> Ryan Hendrickson writes:\n>>\n>>> Hmm. I'd be inclined to take the preliminary clean-up approach, but\n>>> some of the existing strings (there are also two \"Unsupported\n>>> ...\"/\"Supported ...\" strings near the \"Could not set...\"s) are going\n>>> through gettext, and I'm reluctant to interfere with the l10n process.\n>>\n>> I do not see what you mean by interfering with the localization.\n>>\n>> If we are updating text to be translated anyway, giving translators\n>> the strings that need to be translated _earlier_ rather than later\n>> would be more helpful to them, no?\n>\n> Probably true, but as a new contributor I don't know whether changing\n> msgids means more people need to review the patch, more files need to\n> be changed, a translation team needs to be notified, the change needs\n> to be pushed a different branch... whatever your process is. Localized\n> strings are generally more of a headache for drive-by contributors, in\n> my experience across different projects.\n>\n>>> Is this blocking feedback? This strikes me as speculative\n>>> over-engineering\n>>\n>> No, it is loosening a pattern that is overly tight and as a side\n>> effect shortening the line to more readable length ;-).\n>\n> Blocking or not?\n\nIf we are updating anyway, that question is irrelevant, no? This\nversion may hit 'seen' but until the next version comes it will not\nadvance to 'next'.\n"},{"id":"499969","messageId":"30733887-33d8-4049-7dc9-8bc9d0b106da@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqh6c2d8qt.fsf@gitster.g","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-02T19:39:48Z","receivedAt":"2024-08-02T19:39:53Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-08-02 12:28-0700, Junio C Hamano sent:\n\n>>>> Is this blocking feedback? This strikes me as speculative\n>>>> over-engineering\n>>>\n>>> No, it is loosening a pattern that is overly tight and as a side\n>>> effect shortening the line to more readable length ;-).\n>>\n>> Blocking or not?\n>\n> If we are updating anyway, that question is irrelevant, no? This\n> version may hit 'seen' but until the next version comes it will not\n> advance to 'next'.\n\nI can't figure out what you mean by this so I am going to proceed as if \nyou had simply said ‘non-blocking’.\n\nR"},{"id":"499979","messageId":"xmqqfrrmbpbv.fsf@gitster.g","threadId":"61852","inReplyTo":"30733887-33d8-4049-7dc9-8bc9d0b106da@alum.mit.edu","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:13:40Z","receivedAt":"2024-08-02T21:13:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> At 2024-08-02 12:28-0700, Junio C Hamano sent:\n>\n>>>>> Is this blocking feedback? This strikes me as speculative\n>>>>> over-engineering\n>>>>\n>>>> No, it is loosening a pattern that is overly tight and as a side\n>>>> effect shortening the line to more readable length ;-).\n>>>\n>>> Blocking or not?\n>>\n>> If we are updating anyway, that question is irrelevant, no? This\n>> version may hit 'seen' but until the next version comes it will not\n>> advance to 'next'.\n>\n> I can't figure out what you mean by this so I am going to proceed as\n> if you had simply said ‘non-blocking’.\n\nIt does not make much sense to ask if a suggestion is \"blocking\" or\n\"non-blocking\". If you respond with a reasonable explanation why\nyou do not want to take a suggestion, I may (or may not) say that\nyour reasoning makes sense. IOW, making me say \"it is blocking\"\nmeans you want to me to say that I won't listen to you no matter\nwhat you say. That is rarely be the case.\n\nIn this case, I do not think it makes sense to insist with -Fx that\nthe error message has the exact message. And I do not think your\n\"strikes me as\" qualifies as a \"reasonable explanation\".\n\n"},{"id":"499981","messageId":"c7b3383b-ebbf-6ec4-00b8-41c9430a99f4@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqfrrmbpbv.fsf@gitster.g","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-02T21:26:32Z","receivedAt":"2024-08-02T21:26:37Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-08-02 14:13-0700, Junio C Hamano sent:\n\n>>>>>> Is this blocking feedback? This strikes me as speculative\n>>>>>> over-engineering\n>>>>>\n>>>>> No, it is loosening a pattern that is overly tight and as a side\n>>>>> effect shortening the line to more readable length ;-).\n>>>>\n>>>> Blocking or not?\n>>>\n>>> If we are updating anyway, that question is irrelevant, no? This\n>>> version may hit 'seen' but until the next version comes it will not\n>>> advance to 'next'.\n>>\n>> I can't figure out what you mean by this so I am going to proceed as\n>> if you had simply said ‘non-blocking’.\n>\n> It does not make much sense to ask if a suggestion is \"blocking\" or\n> \"non-blocking\". If you respond with a reasonable explanation why\n> you do not want to take a suggestion, I may (or may not) say that\n> your reasoning makes sense. IOW, making me say \"it is blocking\"\n> means you want to me to say that I won't listen to you no matter\n> what you say. That is rarely be the case.\n\nI want you to do what Documentation/ReviewingGuidelines.txt says reviewers \nshould do:\n\n- When providing a recommendation, be as clear as possible about whether you\n consider it \"blocking\" (the code would be broken or otherwise made worse if an\n issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n the recommendation, but acceptance of the series does not require it).\n Non-blocking recommendations can be particularly ambiguous when they are\n related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n they represent only stylistic differences between the author and reviewer.\n\nBecause I would rather not have what is likely to be a highly subjective \nargument about this particular choice in a test script if we don't have \nto have one.\n\nI would also rather not put time into figuring out how best to rename the \nfunction \"old_curl_version\" if it no longer checks for the particular \nerror produced when the curl version is too old, nor would I want to think \nahead about whether it is correct for these tests that use this function \nto continue to pass if different variations on this error are raised. \nThere is one qualifying error currently, and that's what the test matches \nagainst. Matching against hypothetical future errors is speculative \noverengineering if it is not obvious how the errors will vary and what it \nmay mean if they do.\n\nR"},{"id":"499984","messageId":"xmqqmslua9ee.fsf@gitster.g","threadId":"61852","inReplyTo":"c7b3383b-ebbf-6ec4-00b8-41c9430a99f4@alum.mit.edu","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:43:05Z","receivedAt":"2024-08-02T21:43:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> I would also rather not put time into figuring out how best to rename\n> the function \"old_curl_version\" if it no longer checks for the\n> particular error produced when the curl version is too old,\n\nNow, that is a good argument to make sure the \"libcurl 7.84 or later\"\nI suggested to omit for the sake of brevity is in the pattern.\n\nThanks.\n\n\n"},{"id":"499985","messageId":"xmqqikwia97c.fsf@gitster.g","threadId":"61852","inReplyTo":"c7b3383b-ebbf-6ec4-00b8-41c9430a99f4@alum.mit.edu","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:47:19Z","receivedAt":"2024-08-02T21:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Hendrickson writes:\n\n> what the test matches against. Matching against hypothetical future\n> errors is speculative overengineering if it is not obvious how the\n> errors will vary and what it may mean if they do.\n\nThe easiest you can imagine is that it turns out the cut-off version\nof cURL for the feature turned out to be not 7.84, or versions of\ncURL shipped by some distros have backports of the feature to an\nolder version that explicitly naming 7.84 causes more confusion than\nnaming the exact feature we rely on, and we end up rephrasing the\nerror message. We have done such changes in the past, so it is not\nreally speculating \"hypothetical future errors\", but applying common\nsense gained over years working on this project.\n"},{"id":"499988","messageId":"7bb4bc3b-2280-3bda-c104-1dcb9e12db11@alum.mit.edu","threadId":"61852","inReplyTo":"xmqqikwia97c.fsf@gitster.g","subject":"Re: [PATCH v5] http: do not ignore proxy path","fromName":"Ryan Hendrickson","fromEmail":"ryan.hendrickson@alum.mit.edu","sentAt":"2024-08-02T22:14:13Z","receivedAt":"2024-08-02T22:14:18Z","isPatch":true,"sender":{"key":"ryan.hendrickson@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/1570964?v=4"},"body":"At 2024-08-02 14:47-0700, Junio C Hamano sent:\n\n> Ryan Hendrickson writes:\n>\n>> what the test matches against. Matching against hypothetical future\n>> errors is speculative overengineering if it is not obvious how the\n>> errors will vary and what it may mean if they do.\n>\n> The easiest you can imagine is that it turns out the cut-off version\n> of cURL for the feature turned out to be not 7.84, or versions of\n> cURL shipped by some distros have backports of the feature to an\n> older version that explicitly naming 7.84 causes more confusion than\n> naming the exact feature we rely on, and we end up rephrasing the\n> error message. We have done such changes in the past, so it is not\n> really speculating \"hypothetical future errors\", but applying common\n> sense gained over years working on this project.\n\nOkay, so is it a problem to also change the message in the test if that \nhappens? My concern is that if I pick some fragment of the message to \nelide in the test, the message could still get reworded in a way that \nrequires the test to be changed anyway. Even if not, the comment above it \nwould likely need revision too; and if the test doesn't fail, whoever \namends the message is unlikely to notice this.\n\nThe way the test is written in the current version of the patch, there is \nno ambiguity about what is being tested and what would need to change if \nthe message changes. Future contributors can grep the code for references \nto that error message and find this test regardless of which part and how \nmuch of the message they choose to grep for. Shaving a dozen or so \ncharacters out of line is not more important than those considerations, is \nit?\n\nR\n"}]}