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

[Knut Franke <k.franke@science-computing.de>]

Junio C Hamano wrote:
> > +	if (http_proxy_authmethod) {
> > +		int i;
> > +		for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> > +			if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> > +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> > +						http_proxy_authmethods[i].curlauth_param);
> > +				break;
> > +			}
> > +		}
> > +		if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> > +			warning("unsupported proxy authentication method %s: using default",
> > +			      http_proxy_authmethod);
> > +		}
> > +	}
> > +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> > +	else
> > +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> > +#endif
> > +}
> 
> This patch should take what 1c2dbf20 (http: support curl < 7.10.7,
> 2015-02-03) wanted to do into account.  Having the configuration
> variable or the environment variable defined by itself, while
> running a Git built with old cURL, shouldn't trigger any warning,
> but the entire function should perhaps be ifdefed out or something?

Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do
with LIBCURL_CAN_HANDLE_AUTH_ANY? If so, should this be a separate patch? With
the current (scattered) version dependencies, it took me a while to realize that
if the function is ifdefed out for LIBCURL_VERSION_NUM < 0x070a07, we don't need
to worry about default behavior in case LIBCURL_CAN_HANDLE_AUTH_ANY is not
defined. (on the other hand, looking at other curl-version-ifdefs, the define
for AUTH_ANY is the exception)

> >> +static void copy_from_env(const char **var, const char *envname)
> >> +{
> >> +  const char *val = getenv(envname);
> >> +  if (val)
> >> +      *var = xstrdup(val);
> >> +}
[...]
> 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.

How about env_override? Not perfect, but probably better. I don't think
squeezing in more information (maybe_env_override, override_from_env_var) would
help.

> 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

In fact, if http.proxyAuthMethod (btw, I agree with Eric about capitalization)
is set, I do call it with *var pointing to free-able memory. Will fix this.

FWIW, set_from_env() has the same pre-condition, which doesn't seem to be
satisfied in all cases (namely when overwriting variables previously set by
git_config_string()); not to mention missing free()s in http_cleanup.


Otherwise, I'll make the suggested fixes. Thanks.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196