From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 30 Oct 2015 19:35:26 GMT
Subject: Re: [PATCH 2/2] http: use credential API to handle proxy authentication
Message-ID: <CAPig+cSzF0B1rP1ns7PqZjnOuAm50t+Wky1S0EUNh2VWAmdjNw@mail.gmail.com>
In-Reply-To: <xmqq8u6kkvam.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 30, 2015 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Knut Franke <k.franke@science-computing.de> writes:
>> On 2015-10-28 14:58, Eric Sunshine wrote:
>>> > +               }
>>> > +               if (!curl_http_proxy) {
>>> > +                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
>>> > +                       copy_from_env(&curl_http_proxy, "all_proxy");
>>> > +               }
>>>
>>> If this sort of upper- and lowercase environment variable name
>>> checking is indeed desirable, I wonder if it would make sense to fold
>>> that functionality into the helper function.
>>
>> It's just for consistency with libcurl here, not generally desirable; so I don't
>> think it makes sense to add it to the helper.
>
> I agree.  Unlike many environment variables, historically these
> proxy environment variables were all lowercase only for quite a
> while (found as early as in CERN libwww 2.15 March '94).
>
> It appears that CURL did not know this and implemented only
> uppercase variants, which was later corrected to take both
> (http://sourceforge.net/p/curl/bugs/40/ shows a fix in Nov 2000).
>
> So both unfortunately are used in user's environment and we need to
> pay attention to both.  As lowercase version is the more kosher one,
> looking at uppercase first and then overriding it with the lowercase
> one is the right order, I think.
>
> It's a mess, but it is limited to these proxy-ish variables and does
> not deserve a helper to promote the pattern.

It would be great to have this explanation in the commit message or
in-code comment. Thanks.