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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2024, 22:24 UTC
Message-ID: <xmqqle1hyzcu.fsf@gitster.g>
In-Reply-To: <pull.1767.v3.git.1722441675945.gitgitgadget@gmail.com>

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

Show 8 quoted lines

> @@ -5,8 +5,8 @@ http.proxy::
>  	proxy string with a user name but no password, in which case git will
>  	attempt to acquire one in the same way it does for other credentials. See
>  	linkgit:gitcredentials[7] for more information. The syntax thus is
> -	'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden
> -	on a per-remote basis; see remote.<name>.proxy
> +	'[protocol://][user[:password]@]proxyhost[:port][/path]'. This can be
> +	overridden on a per-remote basis; see remote.<name>.proxy

OK.

Show 12 quoted lines

> diff --git a/http.c b/http.c
> index 623ed234891..a50ba095889 100644
> --- a/http.c
> +++ b/http.c
> @@ -1227,6 +1227,7 @@ static CURL *get_curl_handle(void)
>  		 */
>  		curl_easy_setopt(result, CURLOPT_PROXY, "");
>  	} else if (curl_http_proxy) {
> +		struct strbuf proxy = STRBUF_INIT;
>  		if (starts_with(curl_http_proxy, "socks5h"))
>  			curl_easy_setopt(result,
>  				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5_HOSTNAME);

In a block with local variable decl, be more friendly to readers by having a blank line between the end of declarations and the first statement.

Show 13 quoted lines

> +		strbuf_addstr(&proxy, proxy_auth.host);
> +		if (proxy_auth.path) {
> +			curl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);
> +			if (ver->version_num < 0x075400)
> +				die("libcurl 7.84 or later is required to support paths in proxy URLs");
> +
> +			if (!starts_with(proxy_auth.protocol, "socks"))
> +				die("Invalid proxy URL '%s': only SOCKS proxies support paths",
> +				    curl_http_proxy);
> +
> +			if (strcasecmp(proxy_auth.host, "localhost"))
> +				die("Invalid proxy URL '%s': host must be localhost if a path is present",
> +				    curl_http_proxy);

We insist that it must be "localhost", so let's not do strcasecmp() but just do strcmp().

Show 26 quoted lines

> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh
> index bb35b87071d..7fcffba67a2 100755
> --- a/t/t5564-http-proxy.sh
> +++ b/t/t5564-http-proxy.sh
> @@ -39,4 +39,50 @@ test_expect_success 'clone can prompt for proxy password' '
>  	expect_askpass pass proxuser
>  '
>  
> +start_socks() {
> +	mkfifo socks_output &&
> +	{
> +		"$PERL_PATH" "$TEST_DIRECTORY/socks4-proxy.pl" "$1" >socks_output &
> +		socks_pid=$!
> +	} &&
> +	read line <socks_output &&
> +	test "$line" = ready
> +}
> +
> +test_expect_success PERL 'try to start SOCKS proxy' '
> +	# The %30 tests that the correct amount of percent-encoding is applied
> +	# to the proxy string passed to curl.
> +	if start_socks %30.sock
> +	then
> +		test_set_prereq SOCKS_PROXY
> +	fi
> +'

Making it a regular test_expect_success would mean GIT_SKIP_TEST mechansim can be used to skip it, which is probably not what you want. Can't this be a more common test_lazy_prereq, perhaps like

	test_lazy_prereq SOCKS_PROXY '
		# useful comment about 30% here ...
		test_have_prereq PERL &&
		start_socks %30.sock
	'

or something?

Thanks.