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

From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Date: Aug 1, 2024, 03:44 UTC
Message-ID: <9ffa9bf5-f799-400f-5e45-ccbc5316ee42@alum.mit.edu>
In-Reply-To: <xmqqle1hyzcu.fsf@gitster.g>

At 2024-07-31 15:24-0700, Junio C Hamano <gitster@pobox.com> sent:

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

Will do.

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

I don't see the wisdom of being more restrictive than curl is in this respect. The documentation makes the claim that curl's syntax is what this option supports, and while that is not literally true due to how protocols are handled, I don't see a reason to intentionally deviate further.

I've tested that curl does the right thing with any casing of localhost, both on its own and via Git. Host names are generally case insensitive; the error message should be understood in that context. And if it is misunderstood, there's no negative impact on the user who writes "localhost" (while there is a negative impact on the user who quite reasonably expects "LOCALHOST" to work if we don't follow curl's lead).

Show 11 quoted lines

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

This existing test file is definitely not written with running individual tests in mind; the first test is a prerequisite for all that comes after, and the second test seems to be required for tests 3–5 as well.

But sure, I'll use that pattern if you like.