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

[Junio C Hamano <gitster@pobox.com>]

Knut Franke <k.franke@science-computing.de> writes:

> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
> to http.proxy and remote.<name>.proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>


> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Please add these only when you are doing the final submission,
sending the same version reviewed by these people after they said
the patch(es) look good.  To credit others for helping you to polish
your patch, Helped-by: would be more appropriate.

> @@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +/* assumes *var is either NULL or free-able */
> +static void env_override(const char **var, const char *envname)
> +{
> +	const char *val = getenv(envname);
> +	if (val) {
> +		if (*var)
> +			free((void*)*var);

Just
		free((void *)*var);

would be more idiomatic (freeing NULL is not a crime but a norm).
Also as you did elsewhere, have a space between void and the
asterisk.

> +static void init_curl_proxy_auth(CURL *result)
> +{
> +	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Shouldn't this also be part of the #if/#endif?

> +
> +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
> +	if (http_proxy_authmethod) {
> +		int i;
> +		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
> +			if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {
> +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +						proxy_authmethods[i].curlauth_param);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(proxy_authmethods)) {
> +			warning("unsupported proxy authentication method %s: using anyauth",
> +					http_proxy_authmethod);
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +		}
> +	}
> +	else
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}
> +
>  static int has_cert_password(void)
>  {
>  	if (ssl_cert == NULL || ssl_cert_password_required != 1)
> @@ -466,9 +523,7 @@ static CURL *get_curl_handle(void)
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  	}
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +	init_curl_proxy_auth(result);
>  
>  	set_curl_keepalive(result);
>  
> @@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  	if (remote && remote->http_proxy)
>  		curl_http_proxy = xstrdup(remote->http_proxy);
>  
> +	if (remote && remote->http_proxy_authmethod) {
> +		if (http_proxy_authmethod)
> +			free((void*)http_proxy_authmethod);

Just
		free((void *)http_proxy_authmethod);

without NULL-ness check.

But this makes me wonder if env_override() was a good abstraction.

That is, with this helper:

        /* existing value in *var must be freeable */
        static void var_override(const char **var, char *value)
        {
                if (value) {
                        free((void *)(*var));
                        var = xstrdup(value);
                }
        }

the beginning of the init_proxy_auth() would become:

        static void init_curl_proxy_auth(CURL *result)
        {
        #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
		...

and this code would be:

	if (remote)
		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);

which might be even cleaner.

> +		http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);
> +	}
> +
>  	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
>  	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
>  
> @@ -607,6 +668,11 @@ void http_cleanup(void)
>  		curl_http_proxy = NULL;
>  	}
>  
> +	if (http_proxy_authmethod) {
> +		free((void *)http_proxy_authmethod);
> +		http_proxy_authmethod = NULL;
> +	}

No need for NULL-ness check here, either.

Other than the above nits, looks cleanly done.

Thanks.