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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2015, 20:33 UTC
Message-ID: <xmqqk2q9fjyx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445882109-18184-1-git-send-email-k.franke@science-computing.de>

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

Show 6 quoted lines

> 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.proxy-authmethod and remote.<name>.proxy-authmethod (in analogy
> to http.proxy and remote.<name>.proxy).

Please do not name configuration variable with dashes (or underscores). There still may be existing aberrations that are deprecated and need to be removed over time that are misnamed, but the core git variables are named without dashes and underscores.

Call yours "http.proxyAuthmethod" in the documentation, and use strcmp("http.proxyauthmethod", var) in the options callback code.

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

Strange indentation here...

Show 20 quoted lines

> +
> +	if (http_proxy_authmethod) {
> +		if (!strcmp(http_proxy_authmethod, "basic"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_BASIC);
> +		else if (!strcmp(http_proxy_authmethod, "digest"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_DIGEST);
> +		else if (!strcmp(http_proxy_authmethod, "negotiate"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_GSSNEGOTIATE);
> +		else if (!strcmp(http_proxy_authmethod, "ntlm"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_NTLM);
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +		else if (!strcmp(http_proxy_authmethod, "anyauth"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +		// CURLAUTH_DIGEST_IE has no corresponding command-line option in
> +		// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> +		// here, too
> +		else
> +			error("Unknown proxy authentication method: %s",
> +			      http_proxy_authmethod);

Along the same line as how we do sslversions[] instead of a long if/else if/ chain is preferrable.

Thansk.