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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2024, 15:52 UTC
Message-ID: <xmqq7ccygbx6.fsf@gitster.g>
In-Reply-To: <pull.1767.v5.git.1722576007398.gitgitgadget@gmail.com>

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

Show 13 quoted lines

> From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
>
> The documentation for `http.proxy` describes that option, and the
> environment variables it overrides, as supporting "the syntax understood
> by curl". curl allows SOCKS proxies to use a path to a Unix domain
> socket, like `socks5h://localhost/path/to/socket.sock`. Git should
> therefore include, if present, the path part of the proxy URL in what it
> passes to libcurl.
>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

The trailer lines should be ordered in chronological order to record the provenance of the patch. The last two entries make it look as if what you assembled and signed-off was relayed by peff, with or without further modification, with his sign-off to me, to become the final version, but that is not the story you want to tell.

I'd swap the two lines (i.e., sign-offs) while queuing.

> +			if (!starts_with(proxy_auth.protocol, "socks"))
> +				die("Invalid proxy URL '%s': only SOCKS proxies support paths",
> +				    curl_http_proxy);

Our error messages that are prefixed with "fatal:" do not typically begin with a capital letter.

But I'll let it pass, as this copies an existing message in this file. #leftoverbits clean-up needs to correct the ones added by this patch and existing "Invalid proxy URL '%s'" by downcasing "Invalid", possibly enclose the message in _() for i18n, and also downcase "C" in two "Could not set SSL ..." messages.

> +			if (strcasecmp(proxy_auth.host, "localhost"))
> +				die("Invalid proxy URL '%s': host must be localhost if a path is present",
> +				    curl_http_proxy);

Ditto.

Or instead of leaving this for a later clean-up after the dust settles, we could have a separate "preliminary clean-up" patch to address existing ones first. Either is fine, but taking "clean-up after the dust settles" and leaving it a problem for other people is probably easier.

Show 23 quoted lines

> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh
> index bb35b87071d..0d6cfebbfab 100755
> --- a/t/t5564-http-proxy.sh
> +++ b/t/t5564-http-proxy.sh
> @@ -39,4 +39,53 @@ 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 &
> +		echo $! > "$TRASH_DIRECTORY/socks.pid"
> +	} &&
> +	read line <socks_output &&
> +	test "$line" = ready
> +}
> +
> +# The %30 tests that the correct amount of percent-encoding is applied to the
> +# proxy string passed to curl.
> +test_lazy_prereq SOCKS_PROXY 'test_have_prereq PERL && start_socks "$TRASH_DIRECTORY/%30.sock"'
> +
> +test_atexit 'test ! -e "$TRASH_DIRECTORY/socks.pid" || kill "$(cat "$TRASH_DIRECTORY/socks.pid")"'

Let's line-wrap these overly long ones.

        # The %30 tests that the correct amount of percent-encoding is
        # applied to the proxy string passed to curl.
        test_lazy_prereq SOCKS_PROXY '
                test_have_prereq PERL &&
                start_socks "$TRASH_DIRECTORY/%30.sock"
        '

        test_atexit '
                test ! -e "$TRASH_DIRECTORY/socks.pid" ||
                kill "$(cat "$TRASH_DIRECTORY/socks.pid")"
        '

Show 7 quoted lines

> +# The below tests morally ought to be gated on a prerequisite that Git is
> +# linked with a libcurl that supports Unix socket paths for proxies (7.84 or
> +# later), but this is not easy to test right now. Instead, we || the tests with
> +# this function.
> +old_libcurl_error() {
> +	grep -Fx "fatal: libcurl 7.84 or later is required to support paths in proxy URLs" "$1"
> +}

Cute.

This fixes the polarity of the result from the whole "{ test } || this helper" construct. Even if the test proper fails, we will allow such a failure only when the reason of the failure is due to this message.

If I were writing this, I would shorten to look for a bit fuzzier pattern like

    grep "^fatal: .* is required to support paths in proxy URLs" "$1"

as that would allow us to fix the code later without needing to update the pattern, if we discover reasons, other than being older than libcURL 7.84, why paths in proxy URLs cannot be supported.

Other than that, very nicely done.

Thanks.