Re: [PATCH 2/2] http: use credential API to handle proxy authentication

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2015, 19:31 UTC
Message-ID: <xmqq8u6kkvam.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151030182426.GA16389@science-computing.de>

Knut Franke <k.franke@science-computing.de> writes:

Show 13 quoted lines

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