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

Junio C Hamano [off-list ref] writes:

quoted

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