Re: [PATCH 1/2] http: allow selection of proxy authentication method

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2015, 16:59 UTC
Message-ID: <xmqqoafjkjy3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

>> +static void copy_from_env(const char **var, const char *envname)
>> +{
>> +	const char *val = getenv(envname);
>> +	if (val)
>> +		*var = xstrdup(val);
>> +}
>> +
>> +static void init_curl_proxy_auth(CURL *result)
>> +{
>> +	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
>
> Unless this helper is used regularly from many other places, is use
> makes it harder to follow the flow of the logic, as it does not
> offer clear and obvious abstraction, especially with the name
> "copy_from_env()".  I was forced to look at the implementation to
> see what happens when the environment variable does not exist to
> make sure the right thing happens (i.e. http_proxy_authmethod is
> unchanged).

I see you use this liberally in 2/2, it is a handy helper to have, and I do _not_ think it is a good idea to open-code this in 1/2 and turn it into a helper in 2/2. IOW, I am OK with this "one helper with a single caller introduced and used in 1/2". I primarily was wishing that its name more clearly conveyed that it sets the variable from the environment _only if_ the environment variable exists, and otherwise it does not clobber.

The implementation of the helper seems to assume that the variable must not be pointing at a free-able piece of memory when it is called (that is why *var is assigned to without freeing the old value). That's another subtle thing the callers need to be aware of (i.e. deserves at least a comment in front of the function, but as always a good name that clearly conveys it would be more preferrable, if we can find one).

Thanks.