Junio C Hamano <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
