From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Date: Fri, 02 Aug 2024 16:43:07 GMT
Subject: Re: [PATCH v5] http: do not ignore proxy path
Message-ID: <2ba77de5-f103-c2f0-c009-71700c8a020d@alum.mit.edu>
In-Reply-To: <xmqq7ccygbx6.fsf@gitster.g>

At 2024-08-02 08:52-0700, Junio C Hamano <gitster@pobox.com> sent:

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

Okay.

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

Hmm. I'd be inclined to take the preliminary clean-up approach, but some 
of the existing strings (there are also two "Unsupported ..."/"Supported 
..." strings near the "Could not set..."s) are going through gettext, and 
I'm reluctant to interfere with the l10n process.

Would a partial clean-up that downcased only the "Invalid proxy URL" 
message be acceptable, or worse than leaving this as #leftoverbits?

> Let's line-wrap these overly long ones.

Okay.

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

Is this blocking feedback? This strikes me as speculative 
over-engineering; it would not be difficult to generalize the message, and 
possibly then choose a new, more appropriate function name and update the 
explanatory comment, when and if such reasons arise.

R