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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2015, 19:19 UTC
Message-ID: <xmqqd1vwkvtz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151030180129.GA6425@science-computing.de>

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

> Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do
> with LIBCURL_CAN_HANDLE_AUTH_ANY?

Quite frankly, my first preference is to have a code that is clear enough so that you do not need such an intermediate macro. By isolating implementation details that have to be version dependent into a helper function whose purpose is well defined, the rest of the code can be #ifdef free; it would become sufficiently clear to switch based on curl version where implementation details matter.

By refraining from littering #ifdef all over the code, we do assign to disable_gssnegotiate even though the value is not even used when compiled for ancient version of libCURL, but the benefit of code clarity outweighs such downside. We may have to use #ifdef/#endif in some places, but we should in general minimize their uses and write code for the more up-to-date API.

For what I mean, see the attached patch outline to show how to get rid of CAN_HANDLE_AUTH_ANY.

> How about env_override? Not perfect, but probably better.

Much better than anything I'd come up with myself (I am bad at naming, even though I may sometimes be good at spotting a bad name).

 http.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/http.c b/http.c
index 7da76ed..d272b02 100644
--- a/http.c
+++ b/http.c
@@ -15,10 +15,6 @@ int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
-#if LIBCURL_VERSION_NUM >= 0x070a06
-#define LIBCURL_CAN_HANDLE_AUTH_ANY
-#endif
-
 static int min_curl_sessions = 1;
 static int curl_session_count;
 #ifdef USE_CURL_MULTI
@@ -79,9 +75,6 @@ static const char *user_agent;
 
 static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-static unsigned long http_auth_methods = CURLAUTH_ANY;
-#endif
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
@@ -90,6 +83,19 @@ static struct active_request_slot *active_queue_head;
 
 static char *cached_accept_language;
 
+static int disable_gssnegotiate;
+
+static void set_httpauth_opt(CURL *curl)
+{
+#if LIBCURL_VERSION_NUM >= 0x070a06  /* Is CURLAUTH_ANY available? */
+	unsigned long auth_methods = CURLAUTH_ANY;
+
+	if (disable_gssnegotiate)
+		auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, auth_methods);
+#endif
+}
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -375,9 +381,7 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
 #endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
-#endif
+	set_httpauth_opt(result);
 
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
@@ -681,9 +685,7 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-#endif
+	set_httpauth_opt(slot->curl);
 	if (http_auth.password)
 		init_curl_http_auth(slot->curl);
 
@@ -943,9 +945,7 @@ static int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-#endif
+			disable_gssnegotiate = 1;
 			return HTTP_REAUTH;
 		}
 	} else {