Junio C Hamano <gitster@xxxxxxxxx> writes:
>> +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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html