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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> 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")"
        '

> +# 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.