threads / patch / 17431

patchSupport various HTTP authentication methods

Subject: [PATCH] Support various HTTP authentication methods

## tl;dr

16 messages between Jan 29, 2009 and Feb 5, 2009. Diffs are folded; open one to read it.

replies: 15people: 5as markdown or json

Moriyoshi Koizumi· Jan 29, 2009, 09:32 UTC · lore

Currently there is no way to specify the preferred authentication method for the HTTP backend and it always ends up with the CURL's default settings.

This patch enables it if supported by CURL, adding a couple of new settings and config environment variables listed below (the names within the parentheses indicate the latter.)

- http.auth (GIT_HTTP_AUTH)
  Specifies the preferred authentication method for HTTP.  This can
  be a method name or the combination of those separated by comma. Valid
  values are "basic", "digest", "gss" and "ntlm". You can also specify
  "any" (all of the above), "anysafe" (all of the above except "basic").
  Note that the strings are treated case-insensitive.
- http.proxy_auth (GIT_HTTP_PROXY_AUTH)
  Specifies the preferred authentication method method for HTTP proxy.
  The same thing as above applies to this setting.
Signed-off-by: Moriyoshi Koizumi <mozo@mozo.jp>
---
 http.c |  105
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 105 insertions(+), 0 deletions(-)
Show changes to http.c +105 −0
diff --git a/http.c b/http.c
index ee58799..889135f 100644
--- a/http.c
+++ b/http.c
@@ -25,6 +25,12 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv = 0;
 static const char *curl_http_proxy = NULL;
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static const char *curl_http_auth = NULL;
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+static const char *curl_http_proxy_auth = NULL;
+#endif
 
 static struct curl_slist *pragma_header;
 
@@ -153,11 +159,67 @@ static int http_options(const char *var, const
char *value, void *cb)
 			return git_config_string(&curl_http_proxy, var, value);
 		return 0;
 	}
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (!strcmp("http.auth", var)) {
+		if (curl_http_auth == NULL)
+			return git_config_string(&curl_http_auth, var, value);
+		return 0;
+	}
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (!strcmp("http.proxy_auth", var)) {
+		if (curl_http_proxy_auth == NULL)
+			return git_config_string(&curl_http_proxy_auth, var, value);
+		return 0;
+	}
+#endif
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static long get_curl_auth_bitmask(const char* auth_method)
+{
+	char *buf = xmalloc(strlen(auth_method) + 1);
+	const unsigned char *p = (const unsigned char *)auth_method;
+	long mask = CURLAUTH_NONE;
+
+	for (;;) {
+		char *q = buf;
+		while (*p && isspace(*p))
+			++p;
+
+		while (*p && *p != ',')
+			*q++ = tolower(*p++);
+
+		while (--q >= buf && isspace(*(unsigned char *)q));
+		++q;
+
+		*q = '\0';
+
+		if (strcmp(buf, "basic") == 0)
+			mask |= CURLAUTH_BASIC;
+		else if (strcmp(buf, "digest") == 0)
+			mask |= CURLAUTH_DIGEST;
+		else if (strcmp(buf, "gss") == 0)
+			mask |= CURLAUTH_GSSNEGOTIATE;
+		else if (strcmp(buf, "ntlm") == 0)
+			mask |= CURLAUTH_NTLM;
+		else if (strcmp(buf, "any") == 0)
+			mask |= CURLAUTH_ANY;
+		else if (strcmp(buf, "anysafe") == 0)
+			mask |= CURLAUTH_ANYSAFE;
+
+		if (!*p)
+			break;
+		++p;
+	}
+
+	return mask;
+}
+#endif
+
 static CURL* get_curl_handle(void)
 {
 	CURL* result = curl_easy_init();
@@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 
+	if (curl_http_auth) {
+		long n = get_curl_auth_bitmask(curl_http_auth);
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
+	}
+
+	if (curl_http_proxy) {
+		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+
+		if (curl_http_proxy_auth) {
+			long n = get_curl_auth_bitmask(curl_http_proxy_auth);
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
+		}
+	}
+
 	return result;
 }
 
@@ -258,6 +334,21 @@ void http_init(struct remote *remote)
 	if (low_speed_time != NULL)
 		curl_low_speed_time = strtol(low_speed_time, NULL, 10);
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	{
+		char *http_auth = getenv("GIT_HTTP_AUTH");
+		if (http_auth)
+			curl_http_auth = xstrdup(http_auth);
+	}
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	{
+		char *http_proxy_auth = getenv("GIT_HTTP_PROXY_AUTH");
+		if (http_proxy_auth)
+			curl_http_proxy_auth = xstrdup(http_proxy_auth);
+	}
+#endif
+
 	git_config(http_options, NULL);
 
 	if (curl_ssl_verify == -1)
@@ -309,6 +400,20 @@ void http_cleanup(void)
 		free((void *)curl_http_proxy);
 		curl_http_proxy = NULL;
 	}
+
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth) {
+		free((void *)curl_http_auth);
+		curl_http_auth = NULL;
+	}
+#endif
+
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (curl_http_proxy_auth) {
+		free((void *)curl_http_proxy_auth);
+		curl_http_proxy_auth = NULL;
+	}
+#endif
 }
 
 struct active_request_slot *get_active_slot(void)
-- 
1.5.6.3
Junio C Hamano· Jan 29, 2009, 10:08 UTC · re: Moriyoshi Koizumi · lore

Re: [PATCH] Support various HTTP authentication methods

Moriyoshi Koizumi <mozo@mozo.jp> writes:
Show 6 quoted lines
> Currently there is no way to specify the preferred authentication
> method for the HTTP backend and it always ends up with the CURL's
> default
> settings.
>
> This patch enables it if supported by CURL, adding a couple of new
"it" in "enables it" is a bit unclear...
Show 5 quoted lines
> Signed-off-by: Moriyoshi Koizumi <mozo@mozo.jp>
> ---
>  http.c |  105
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 105 insertions(+), 0 deletions(-)
Linewrapped and whitespace damaged patch that would not apply.
Show 14 quoted lines
> diff --git a/http.c b/http.c
> index ee58799..889135f 100644
> --- a/http.c
> +++ b/http.c
> @@ -25,6 +25,12 @@ static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv = 0;
>  static const char *curl_http_proxy = NULL;
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static const char *curl_http_auth = NULL;
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +static const char *curl_http_proxy_auth = NULL;
> +#endif

I am not a cURL expert, so I'd take your word for these version dependencies.

We do not initialize static scope pointers to "= NULL" nor variables to 0, instead we let BSS take care of that for us. ftp_no_epsv we can see in the context is doing unnecessary initialization that should be fixed.

Show 10 quoted lines
>  static struct curl_slist *pragma_header;
>  
> @@ -153,11 +159,67 @@ static int http_options(const char *var, const
> char *value, void *cb)
>  			return git_config_string(&curl_http_proxy, var, value);
>  		return 0;
>  	}
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +	if (!strcmp("http.auth", var)) {
> +		if (curl_http_auth == NULL)
We tend to say "if (!pointer)".

I see you implemented "the first one wins" rule with this test, but I do not think you want that. We first read $HOME/.gitconfig and then repository specific $GIT_DIR/config, so it is often more useful to use "the last one wins" rule.

Show 22 quoted lines
> +			return git_config_string(&curl_http_auth, var, value);
> +		return 0;
> +	}
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +	if (!strcmp("http.proxy_auth", var)) {
> +		if (curl_http_proxy_auth == NULL)
> +			return git_config_string(&curl_http_proxy_auth, var, value);
> +		return 0;
> +	}
> +#endif
>  
>  	/* Fall back on the default ones */
>  	return git_default_config(var, value, cb);
>  }
>  
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static long get_curl_auth_bitmask(const char* auth_method)
> +{
> +	char *buf = xmalloc(strlen(auth_method) + 1);
> +	const unsigned char *p = (const unsigned char *)auth_method;
> +	long mask = CURLAUTH_NONE;

Our isspace() is a sane_isspace(), so you do not have to play casting games between signed vs unsigned char.

Show 14 quoted lines
> +	for (;;) {
> +		char *q = buf;
> +		while (*p && isspace(*p))
> +			++p;
> +
> +		while (*p && *p != ',')
> +			*q++ = tolower(*p++);
> +
> +		while (--q >= buf && isspace(*(unsigned char *)q));
> +		++q;
> +
> +		*q = '\0';
> +
> +		if (strcmp(buf, "basic") == 0)

Say !strcmp(buf, "literal") like you did in the configuration parsing part earlier.

Show 16 quoted lines
> +			mask |= CURLAUTH_BASIC;
> +		else if (strcmp(buf, "digest") == 0)
> +			mask |= CURLAUTH_DIGEST;
> +		else if (strcmp(buf, "gss") == 0)
> +			mask |= CURLAUTH_GSSNEGOTIATE;
> +		else if (strcmp(buf, "ntlm") == 0)
> +			mask |= CURLAUTH_NTLM;
> +		else if (strcmp(buf, "any") == 0)
> +			mask |= CURLAUTH_ANY;
> +		else if (strcmp(buf, "anysafe") == 0)
> +			mask |= CURLAUTH_ANYSAFE;
> +
> +		if (!*p)
> +			break;
> +		++p;
> +	}

You leak "buf" here you forgot to free. The string you can possibly accept is a known set with some maximum length, so you can use a on-stack buf[] and reject any token longer than that maximum, right?

Show 25 quoted lines
> +	return mask;
> +}
> +#endif
> +
>  static CURL* get_curl_handle(void)
>  {
>  	CURL* result = curl_easy_init();
> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
>  	if (curl_http_proxy)
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  
> +	if (curl_http_auth) {
> +		long n = get_curl_auth_bitmask(curl_http_auth);
> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
> +	}
> +
> +	if (curl_http_proxy) {
> +		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +
> +		if (curl_http_proxy_auth) {
> +			long n = get_curl_auth_bitmask(curl_http_proxy_auth);
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
> +		}
> +	}
> +

This part does not have to be protected with the LIBCURL_VERSION_NUM conditional? I somehow find it unlikely...

Instead of parsing the string every time a curl handle is asked for, how about parsing them once and store the masks in two file scope static longs in http_init() and use that value to easy_setopt() call here?

That way you can free the two strings much early without waiting for http_cleanup(), too, right?

Moriyoshi Koizumi· Jan 29, 2009, 13:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support various HTTP authentication methods

First thank you for the advice. I am not familiar to the code base and definitely doing something wrong.

Junio C Hamano wrote:
> Moriyoshi Koizumi <mozo@mozo.jp> writes:
>> This patch enables it if supported by CURL, adding a couple of new
> 
> "it" in "enables it" is a bit unclear...

I was a bit in a rush and I thought I got to get this done before I went home. This patch dnables various HTTP authentication methods namely basic, digest, GSS and NTLM that are supported by cURL. cURL tries to use basic authentication if the option is not explicitly provided.

> Linewrapped and whitespace damaged patch that would not apply.
Sorry for the crap. I'll try to send the correct one next week.
Show 6 quoted lines
> I am not a cURL expert, so I'd take your word for these version
> dependencies.
> 
> We do not initialize static scope pointers to "= NULL" nor variables to 0,
> instead we let BSS take care of that for us.  ftp_no_epsv we can see in
> the context is doing unnecessary initialization that should be fixed.
Right.
Show 5 quoted lines
>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> +	if (!strcmp("http.auth", var)) {
>> +		if (curl_http_auth == NULL)
> 
> We tend to say "if (!pointer)".
I'll fix this too.
> I see you implemented "the first one wins" rule with this test, but I do
> not think you want that.  We first read $HOME/.gitconfig and then
> repository specific $GIT_DIR/config, so it is often more useful to use
> "the last one wins" rule.

The environment variables win over gitconfigs. I thought that's what I implemented and what we want.

Show 20 quoted lines
> Our isspace() is a sane_isspace(), so you do not have to play casting
> games between signed vs unsigned char.
> 
>> +	for (;;) {
>> +		char *q = buf;
>> +		while (*p && isspace(*p))
>> +			++p;
>> +
>> +		while (*p && *p != ',')
>> +			*q++ = tolower(*p++);
>> +
>> +		while (--q >= buf && isspace(*(unsigned char *)q));
>> +		++q;
>> +
>> +		*q = '\0';
>> +
>> +		if (strcmp(buf, "basic") == 0)
> 
> Say !strcmp(buf, "literal") like you did in the configuration parsing part
> earlier.

I tend to like this way, and the reason for the inconsistency between them is that the earlier one is pasted from the nearby code. I'm willing to fix this as well if I should.

Show 21 quoted lines
> 
>> +			mask |= CURLAUTH_BASIC;
>> +		else if (strcmp(buf, "digest") == 0)
>> +			mask |= CURLAUTH_DIGEST;
>> +		else if (strcmp(buf, "gss") == 0)
>> +			mask |= CURLAUTH_GSSNEGOTIATE;
>> +		else if (strcmp(buf, "ntlm") == 0)
>> +			mask |= CURLAUTH_NTLM;
>> +		else if (strcmp(buf, "any") == 0)
>> +			mask |= CURLAUTH_ANY;
>> +		else if (strcmp(buf, "anysafe") == 0)
>> +			mask |= CURLAUTH_ANYSAFE;
>> +
>> +		if (!*p)
>> +			break;
>> +		++p;
>> +	}
> 
> You leak "buf" here you forgot to free.  The string you can possibly
> accept is a known set with some maximum length, so you can use a on-stack
> buf[] and reject any token longer than that maximum, right?

That one is really silly and shameful :-( I first thought I could go with the on-stack buffer, but I eventually did this because the input might contain an indefinite set of tokens, and thought safer to alloc it.

Show 12 quoted lines
>> +	if (curl_http_proxy) {
>> +		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>> +
>> +		if (curl_http_proxy_auth) {
>> +			long n = get_curl_auth_bitmask(curl_http_proxy_auth);
>> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
>> +		}
>> +	}
>> +
> 
> This part does not have to be protected with the LIBCURL_VERSION_NUM
> conditional?  I somehow find it unlikely...

The block that starts with if (curl_http_proxy_auth) {} should've been enclosed by the conditinals. The leading part had been there in the first place.

> Instead of parsing the string every time a curl handle is asked for, how
> about parsing them once and store the masks in two file scope static longs
> in http_init() and use that value to easy_setopt() call here?

That has to be the way to go, but the question is where I am supposed to parse the strings and store them to the globals?

>
> That way you can free the two strings much early without waiting for
> http_cleanup(), too, right?

Regards, Moriyoshi

Moriyoshi Koizumi· Feb 2, 2009, 04:09 UTC · re: Junio C Hamano · lore

Currently there is no way to specify the preferred authentication method for the HTTP backend and it always falls back to the CURL's default settings.

This patch allows users to specify the authentication method if supported by CURL, adding a couple of new settings and environment variables listed below (the names within the parentheses indicate the environment variables.)

- http.auth (GIT_HTTP_AUTH)
  Specifies the preferred authentication method for HTTP.  This can
  be a method name or the combination of those separated by comma. Valid
  values are "basic", "digest", "gss" and "ntlm". You can also specify
  "any" (all of the above), "anysafe" (all of the above except "basic").
  Note that the strings are treated case-insensitive.
- http.proxy_auth (GIT_HTTP_PROXY_AUTH)
  Specifies the preferred authentication method method for HTTP proxy.
  The same thing as above applies to this setting.
Signed-off-by: Moriyoshi <mozo@mozo.jp>
---
 http.c |  120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 114 insertions(+), 6 deletions(-)
Show changes to http.c +114 −6
diff --git a/http.c b/http.c
index ee58799..41e8e8c 100644
--- a/http.c
+++ b/http.c
@@ -13,18 +13,24 @@ static CURL *curl_default;
 char curl_errorstr[CURL_ERROR_SIZE];
 
 static int curl_ssl_verify = -1;
-static const char *ssl_cert = NULL;
+static const char *ssl_cert;
 #if LIBCURL_VERSION_NUM >= 0x070902
-static const char *ssl_key = NULL;
+static const char *ssl_key;
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-static const char *ssl_capath = NULL;
+static const char *ssl_capath;
 #endif
-static const char *ssl_cainfo = NULL;
+static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv = 0;
-static const char *curl_http_proxy = NULL;
+static const char *curl_http_proxy;
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static const char *curl_http_auth;
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+static const char *curl_http_proxy_auth;
+#endif
 
 static struct curl_slist *pragma_header;
 
@@ -153,11 +159,69 @@ static int http_options(const char *var, const char *value, void *cb)
 			return git_config_string(&curl_http_proxy, var, value);
 		return 0;
 	}
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (!strcmp("http.auth", var)) {
+		if (curl_http_auth == NULL)
+			return git_config_string(&curl_http_auth, var, value);
+		return 0;
+	}
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (!strcmp("http.proxy_auth", var)) {
+		if (curl_http_proxy_auth == NULL)
+			return git_config_string(&curl_http_proxy_auth, var, value);
+		return 0;
+	}
+#endif
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static long get_curl_auth_bitmask(const char* auth_method)
+{
+	char buf[4096];
+	const unsigned char *p = (const unsigned char *)auth_method;
+	long mask = CURLAUTH_NONE;
+
+    strlcpy(buf, auth_method, sizeof(buf));
+
+	for (;;) {
+		char *q = buf;
+		while (*p && isspace(*p))
+			++p;
+
+		while (*p && *p != ',')
+			*q++ = tolower(*p++);
+
+		while (--q >= buf && isspace(*q));
+		++q;
+
+		*q = '\0';
+
+		if (!strcmp(buf, "basic"))
+			mask |= CURLAUTH_BASIC;
+		else if (!strcmp(buf, "digest"))
+			mask |= CURLAUTH_DIGEST;
+		else if (!strcmp(buf, "gss"))
+			mask |= CURLAUTH_GSSNEGOTIATE;
+		else if (!strcmp(buf, "ntlm"))
+			mask |= CURLAUTH_NTLM;
+		else if (!strcmp(buf, "any"))
+			mask |= CURLAUTH_ANY;
+		else if (!strcmp(buf, "anysafe"))
+			mask |= CURLAUTH_ANYSAFE;
+
+		if (!*p)
+			break;
+		++p;
+	}
+
+	return mask;
+}
+#endif
+
 static CURL* get_curl_handle(void)
 {
 	CURL* result = curl_easy_init();
@@ -207,9 +271,24 @@ static CURL* get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-	if (curl_http_proxy)
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth) {
+		long n = get_curl_auth_bitmask(curl_http_auth);
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
+	}
+#endif
+
+	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 
+#if LIBCURL_VERSION_NUM >= 0x070a07
+		if (curl_http_proxy_auth) {
+			long n = get_curl_auth_bitmask(curl_http_proxy_auth);
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
+		}
+#endif
+	}
+
 	return result;
 }
 
@@ -258,6 +337,21 @@ void http_init(struct remote *remote)
 	if (low_speed_time != NULL)
 		curl_low_speed_time = strtol(low_speed_time, NULL, 10);
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	{
+		char *http_auth = getenv("GIT_HTTP_AUTH");
+		if (http_auth)
+			curl_http_auth = xstrdup(http_auth);
+	}
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	{
+		char *http_proxy_auth = getenv("GIT_HTTP_PROXY_AUTH");
+		if (http_proxy_auth)
+			curl_http_proxy_auth = xstrdup(http_proxy_auth);
+	}
+#endif
+
 	git_config(http_options, NULL);
 
 	if (curl_ssl_verify == -1)
@@ -309,6 +403,20 @@ void http_cleanup(void)
 		free((void *)curl_http_proxy);
 		curl_http_proxy = NULL;
 	}
+
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth) {
+		free((void *)curl_http_auth);
+		curl_http_auth = NULL;
+	}
+#endif
+
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (curl_http_proxy_auth) {
+		free((void *)curl_http_proxy_auth);
+		curl_http_proxy_auth = NULL;
+	}
+#endif
 }
 
 struct active_request_slot *get_active_slot(void)
-- 
1.5.6.3
Junio C Hamano· Feb 2, 2009, 06:31 UTC · re: Moriyoshi Koizumi · lore

Re: [PATCH] Support various HTTP authentication methods

Moriyoshi Koizumi <mozo@mozo.jp> writes:
Show 25 quoted lines
> diff --git a/http.c b/http.c
> index ee58799..41e8e8c 100644
> --- a/http.c
> +++ b/http.c
> @@ -13,18 +13,24 @@ static CURL *curl_default;
>  char curl_errorstr[CURL_ERROR_SIZE];
>  
>  static int curl_ssl_verify = -1;
> -static const char *ssl_cert = NULL;
> +static const char *ssl_cert;
>  #if LIBCURL_VERSION_NUM >= 0x070902
> -static const char *ssl_key = NULL;
> +static const char *ssl_key;
>  #endif
>  #if LIBCURL_VERSION_NUM >= 0x070908
> -static const char *ssl_capath = NULL;
> +static const char *ssl_capath;
>  #endif
> -static const char *ssl_cainfo = NULL;
> +static const char *ssl_cainfo;
>  static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv = 0;
> -static const char *curl_http_proxy = NULL;
> +static const char *curl_http_proxy;

Applying style fixes to the existing code is very much appreciated, *but* please make such a clean-up patch a separate one. A two-patch series whose [1/2] is such a pure clean-up without any feature change, with [2/2] that adds code to the cleaned-up state would be much less distracting for people who nitpick your changes.

Show 18 quoted lines
> @@ -153,11 +159,69 @@ static int http_options(const char *var, const char *value, void *cb)
>  			return git_config_string(&curl_http_proxy, var, value);
>  		return 0;
>  	}
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +	if (!strcmp("http.auth", var)) {
> +		if (curl_http_auth == NULL)
> +			return git_config_string(&curl_http_auth, var, value);
> +		return 0;
> +	}
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +	if (!strcmp("http.proxy_auth", var)) {
> +		if (curl_http_proxy_auth == NULL)
> +			return git_config_string(&curl_http_proxy_auth, var, value);
> +		return 0;
> +	}
> +#endif

If you follow config.c::git_config() you will notice that we read from /etc/gitconfig, $HOME/.gitconfig and then finally $GIT_DIR/config. By implementing "if we already have read curl_http_auth already, we will ignore the later setting" like above code does, you break the general expectation that system-wide defaults is overridable by $HOME/.gitconfig and that is in turn overridable by per-repository $GIT_DIR/config.

The preferred order would be:
  - Use the value obtained from command line parameters, if any;
  - Otherwise, if an environment variable is there, use it;
  - Otherwise, the value obtained from git_config(), with "later one wins"
    rule.
  
I think you are not adding any command line option, so favoring
environment and then using configuration is fine, but the configuration
parser must follow the usual "later one wins" rule to avoid dissapointing
the users.
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static long get_curl_auth_bitmask(const char* auth_method)

In git codebase, asterisk that means "a pointer" sticks to the variable name not to type name; "const char *auth_method" (I see this file is already infested with such style violations, but if you are doing a separate clean-up patch it would be appreciated to clean them up).

> +{
> +	char buf[4096];
Do you need that much space?
> +	const unsigned char *p = (const unsigned char *)auth_method;
> +	long mask = CURLAUTH_NONE;
> +
> +    strlcpy(buf, auth_method, sizeof(buf));
A tab is 8-char wide.
What happens when auth_method is longer than 4kB?
Show 5 quoted lines
> +
> +	for (;;) {
> +		char *q = buf;
> +		while (*p && isspace(*p))
> +			++p;

If there is no particular reason to choose one over the other, please use postincrement, p++, as other existing parts of the codebase.

I'll try to demonstrate what (I think) this patch should look like as a pair of follow-up messages to this one, but I am not sure about my rewrite of get_curl_auth_bitmask(). Please consider it as an easter-egg bughunt ;-)

Junio C Hamano· Feb 2, 2009, 06:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support various HTTP authentication methods

Moriyoshi Koizumi <mozo@mozo.jp> writes:
Show 25 quoted lines
> diff --git a/http.c b/http.c
> index ee58799..41e8e8c 100644
> --- a/http.c
> +++ b/http.c
> @@ -13,18 +13,24 @@ static CURL *curl_default;
>  char curl_errorstr[CURL_ERROR_SIZE];
>  
>  static int curl_ssl_verify = -1;
> -static const char *ssl_cert = NULL;
> +static const char *ssl_cert;
>  #if LIBCURL_VERSION_NUM >= 0x070902
> -static const char *ssl_key = NULL;
> +static const char *ssl_key;
>  #endif
>  #if LIBCURL_VERSION_NUM >= 0x070908
> -static const char *ssl_capath = NULL;
> +static const char *ssl_capath;
>  #endif
> -static const char *ssl_cainfo = NULL;
> +static const char *ssl_cainfo;
>  static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv = 0;
> -static const char *curl_http_proxy = NULL;
> +static const char *curl_http_proxy;

Applying style fixes to the existing code is very much appreciated, *but* please make such a clean-up patch a separate one. A two-patch series whose [1/2] is such a pure clean-up without any feature change, with [2/2] that adds code to the cleaned-up state would be much less distracting for people who nitpick your changes.

Show 18 quoted lines
> @@ -153,11 +159,69 @@ static int http_options(const char *var, const char *value, void *cb)
>  			return git_config_string(&curl_http_proxy, var, value);
>  		return 0;
>  	}
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +	if (!strcmp("http.auth", var)) {
> +		if (curl_http_auth == NULL)
> +			return git_config_string(&curl_http_auth, var, value);
> +		return 0;
> +	}
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +	if (!strcmp("http.proxy_auth", var)) {
> +		if (curl_http_proxy_auth == NULL)
> +			return git_config_string(&curl_http_proxy_auth, var, value);
> +		return 0;
> +	}
> +#endif

If you follow config.c::git_config() you will notice that we read from /etc/gitconfig, $HOME/.gitconfig and then finally $GIT_DIR/config. By implementing "if we already have read curl_http_auth already, we will ignore the later setting" like above code does, you break the general expectation that system-wide defaults is overridable by $HOME/.gitconfig and that is in turn overridable by per-repository $GIT_DIR/config.

The preferred order would be:
  - Use the value obtained from command line parameters, if any;
  - Otherwise, if an environment variable is there, use it;
  - Otherwise, the value obtained from git_config(), with "later one wins"
    rule.
  
I think you are not adding any command line option, so favoring
environment and then using configuration is fine, but the configuration
parser must follow the usual "later one wins" rule to avoid dissapointing
the users.
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static long get_curl_auth_bitmask(const char* auth_method)

In git codebase, asterisk that means "a pointer" sticks to the variable name not to type name; "const char *auth_method" (I see this file is already infested with such style violations, but if you are doing a separate clean-up patch it would be appreciated to clean them up).

> +{
> +	char buf[4096];
Do you need that much space?
> +	const unsigned char *p = (const unsigned char *)auth_method;
> +	long mask = CURLAUTH_NONE;
> +
> +    strlcpy(buf, auth_method, sizeof(buf));
A tab is 8-char wide.
What happens when auth_method is longer than 4kB?
Show 5 quoted lines
> +
> +	for (;;) {
> +		char *q = buf;
> +		while (*p && isspace(*p))
> +			++p;

If there is no particular reason to choose one over the other, please use postincrement, p++, as other existing parts of the codebase.

I'll try to demonstrate what (I think) this patch should look like as a pair of follow-up messages to this one, but I am not sure about my rewrite of get_curl_auth_bitmask(). Please consider it as an easter-egg bughunt ;-)

Junio C Hamano· Feb 2, 2009, 06:31 UTC · re: Junio C Hamano · lore

[PATCH 1/2] http.c: fix various style violations

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   46 +++++++++++++++++++++-------------------------
 1 files changed, 21 insertions(+), 25 deletions(-)
Show changes to http.c +21 −25
diff --git a/http.c b/http.c
index ee58799..86be906 100644
--- a/http.c
+++ b/http.c
@@ -1,7 +1,7 @@
 #include "http.h"
 
 int data_received;
-int active_requests = 0;
+int active_requests;
 
 #ifdef USE_CURL_MULTI
 static int max_requests = -1;
@@ -13,22 +13,22 @@ static CURL *curl_default;
 char curl_errorstr[CURL_ERROR_SIZE];
 
 static int curl_ssl_verify = -1;
-static const char *ssl_cert = NULL;
+static const char *ssl_cert;
 #if LIBCURL_VERSION_NUM >= 0x070902
-static const char *ssl_key = NULL;
+static const char *ssl_key;
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-static const char *ssl_capath = NULL;
+static const char *ssl_capath;
 #endif
-static const char *ssl_cainfo = NULL;
+static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
-static int curl_ftp_no_epsv = 0;
-static const char *curl_http_proxy = NULL;
+static int curl_ftp_no_epsv;
+static const char *curl_http_proxy;
 
 static struct curl_slist *pragma_header;
 
-static struct active_request_slot *active_queue_head = NULL;
+static struct active_request_slot *active_queue_head;
 
 size_t fread_buffer(void *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
@@ -94,9 +94,8 @@ static void process_curl_messages(void)
 static int http_options(const char *var, const char *value, void *cb)
 {
 	if (!strcmp("http.sslverify", var)) {
-		if (curl_ssl_verify == -1) {
+		if (curl_ssl_verify == -1)
 			curl_ssl_verify = git_config_bool(var, value);
-		}
 		return 0;
 	}
 
@@ -158,9 +157,9 @@ static int http_options(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static CURL* get_curl_handle(void)
+static CURL *get_curl_handle(void)
 {
-	CURL* result = curl_easy_init();
+	CURL *result = curl_easy_init();
 
 	if (!curl_ssl_verify) {
 		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
@@ -322,15 +321,13 @@ struct active_request_slot *get_active_slot(void)
 	/* Wait for a slot to open up if the queue is full */
 	while (active_requests >= max_requests) {
 		curl_multi_perform(curlm, &num_transfers);
-		if (num_transfers < active_requests) {
+		if (num_transfers < active_requests)
 			process_curl_messages();
-		}
 	}
 #endif
 
-	while (slot != NULL && slot->in_use) {
+	while (slot != NULL && slot->in_use)
 		slot = slot->next;
-	}
 	if (slot == NULL) {
 		newslot = xmalloc(sizeof(*newslot));
 		newslot->curl = NULL;
@@ -341,9 +338,8 @@ struct active_request_slot *get_active_slot(void)
 		if (slot == NULL) {
 			active_queue_head = newslot;
 		} else {
-			while (slot->next != NULL) {
+			while (slot->next != NULL)
 				slot = slot->next;
-			}
 			slot->next = newslot;
 		}
 		slot = newslot;
@@ -404,7 +400,7 @@ struct fill_chain {
 	struct fill_chain *next;
 };
 
-static struct fill_chain *fill_cfg = NULL;
+static struct fill_chain *fill_cfg;
 
 void add_fill_function(void *data, int (*fill)(void *))
 {
@@ -535,9 +531,8 @@ static void finish_active_slot(struct active_request_slot *slot)
 	}
 
 	/* Run callback if appropriate */
-	if (slot->callback_func != NULL) {
+	if (slot->callback_func != NULL)
 		slot->callback_func(slot->callback_data);
-	}
 }
 
 void finish_all_active_slots(void)
@@ -567,8 +562,10 @@ static inline int needs_quote(int ch)
 
 static inline int hex(int v)
 {
-	if (v < 10) return '0' + v;
-	else return 'A' + v - 10;
+	if (v < 10)
+		return '0' + v;
+	else
+		return 'A' + v - 10;
 }
 
 static char *quote_ref_url(const char *base, const char *ref)
@@ -591,8 +588,7 @@ static char *quote_ref_url(const char *base, const char *ref)
 			*dp++ = '%';
 			*dp++ = hex((ch >> 4) & 0xF);
 			*dp++ = hex(ch & 0xF);
-		}
-		else
+		} else
 			*dp++ = ch;
 	}
 	*dp = 0;
-- 
1.6.1.2.333.ged98f
Junio C Hamano· Feb 2, 2009, 06:31 UTC · re: Junio C Hamano · lore

[PATCH 2/2] Support various HTTP authentication methods

From: Moriyoshi Koizumi <mozo@mozo.jp>

Currently there is no way to specify the preferred authentication method for the HTTP backend and it always falls back to the CURL's default settings.

This patch allows users to specify the authentication method if supported by CURL, adding a couple of new settings and environment variables listed below (the names within the parentheses indicate the environment variables.)

- http.auth (GIT_HTTP_AUTH)
  Specifies the preferred authentication method for HTTP.  This can
  be a method name or the combination of those separated by comma. Valid
  values are "basic", "digest", "gss" and "ntlm". You can also specify
  "any" (all of the above), "anysafe" (all of the above except "basic").
  Note that the strings are treated case-insensitive.
- http.proxy_auth (GIT_HTTP_PROXY_AUTH)
  Specifies the preferred authentication method method for HTTP proxy.
  The same thing as above applies to this setting.
Signed-off-by: Moriyoshi Koizumi <mozo@mozo.jp>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |  128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 127 insertions(+), 1 deletions(-)
Show changes to http.c +127 −1
diff --git a/http.c b/http.c
index 86be906..7eb849b 100644
--- a/http.c
+++ b/http.c
@@ -25,6 +25,14 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static const char *curl_http_auth;
+static long http_auth_bitmask;
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+static const char *curl_http_proxy_auth;
+static long http_proxy_auth_bitmask;
+#endif
 
 static struct curl_slist *pragma_header;
 
@@ -152,11 +160,78 @@ static int http_options(const char *var, const char *value, void *cb)
 			return git_config_string(&curl_http_proxy, var, value);
 		return 0;
 	}
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (!strcmp("http.auth", var)) {
+		if (curl_http_auth == NULL)
+			return git_config_string(&curl_http_auth, var, value);
+		return 0;
+	}
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (!strcmp("http.proxy_auth", var)) {
+		if (curl_http_proxy_auth == NULL)
+			return git_config_string(&curl_http_proxy_auth, var, value);
+		return 0;
+	}
+#endif
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+#define is_delim(x) (isspace(x) || x == ',')
+
+static long get_curl_auth_bitmask(const char *auth_method)
+{
+	char buf[20];
+	const unsigned char *p = (const unsigned char *)auth_method;
+	long mask = CURLAUTH_NONE;
+
+	for (;;) {
+		char *q = buf;
+		int toolong = 0;
+
+		while (*p && is_delim(*p))
+			p++;
+		if (!*p)
+			break;
+
+		while (*p && !is_delim(*p)) {
+			if (q < buf + sizeof(buf) - 1)
+				*q++ = tolower(*p);
+			else
+				toolong = 1;
+			p++;
+		}
+		if (toolong)
+			continue;
+
+		while (--q >= buf && is_delim(*q))
+			;
+		q++;
+		*q = '\0';
+
+		if (!strcmp(buf, "basic"))
+			mask |= CURLAUTH_BASIC;
+		else if (!strcmp(buf, "digest"))
+			mask |= CURLAUTH_DIGEST;
+		else if (!strcmp(buf, "gss"))
+			mask |= CURLAUTH_GSSNEGOTIATE;
+		else if (!strcmp(buf, "ntlm"))
+			mask |= CURLAUTH_NTLM;
+		else if (!strcmp(buf, "any"))
+			mask |= CURLAUTH_ANY;
+		else if (!strcmp(buf, "anysafe"))
+			mask |= CURLAUTH_ANYSAFE;
+		p++;
+	}
+
+	return mask;
+}
+#undef is_delim
+#endif
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -206,12 +281,48 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-	if (curl_http_proxy)
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth)
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH,
+				 http_auth_bitmask);
+#endif
+
+	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+#if LIBCURL_VERSION_NUM >= 0x070a07
+		if (curl_http_proxy_auth)
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH,
+					 http_proxy_auth_bitmask);
+#endif
+	}
 
 	return result;
 }
 
+static void prepare_http_auth_settings(void)
+{
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	char *val;
+
+	val = getenv("GIT_HTTP_AUTH");
+	if (val)
+		curl_http_auth = val;
+
+	if (curl_http_auth) {
+		http_auth_bitmask = get_curl_auth_bitmask(curl_http_auth);
+	}
+
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	val = getenv("GIT_HTTP_PROXY_AUTH");
+	if (val)
+		curl_http_proxy_auth = val;
+	if (curl_http_proxy_auth) {
+		http_proxy_auth_bitmask = get_curl_auth_bitmask(curl_http_proxy_auth);
+	}
+#endif
+#endif
+}
+
 void http_init(struct remote *remote)
 {
 	char *low_speed_limit;
@@ -258,6 +369,7 @@ void http_init(struct remote *remote)
 		curl_low_speed_time = strtol(low_speed_time, NULL, 10);
 
 	git_config(http_options, NULL);
+	prepare_http_auth_settings();
 
 	if (curl_ssl_verify == -1)
 		curl_ssl_verify = 1;
@@ -308,6 +420,20 @@ void http_cleanup(void)
 		free((void *)curl_http_proxy);
 		curl_http_proxy = NULL;
 	}
+
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth) {
+		free((void *)curl_http_auth);
+		curl_http_auth = NULL;
+	}
+#endif
+
+#if LIBCURL_VERSION_NUM >= 0x070a07
+	if (curl_http_proxy_auth) {
+		free((void *)curl_http_proxy_auth);
+		curl_http_proxy_auth = NULL;
+	}
+#endif
 }
 
 struct active_request_slot *get_active_slot(void)
-- 
1.6.1.2.333.ged98f
Aristotle Pagaltzis· Feb 4, 2009, 18:51 UTC · re: Junio C Hamano · lore

Re: Support various HTTP authentication methods

* Junio C Hamano <gitster@pobox.com> [2009-02-02 07:35]:
> This patch allows users to specify the authentication method if
> supported by CURL, adding a couple of new settings and
> environment variables listed below (the names within the
> parentheses indicate the environment variables.)

I tried this patch against my Apache WebDAV server for which I use Digest auth, but it didn’t get very far:

    Initialized empty Git repository in /tmp/xxxxxxx/.git/
    error: Couldn't get http://xxx:xxx@example.org:8086/xxxxxxx.git/HEAD for HEAD
    The requested URL returned error: 400
    Getting alternates list for http://xxx:xxx@example.org:8086/xxxxxxx.git
    Getting pack list for http://xxx:xxx@example.org:8086/xxxxxxx.git
    Getting index for pack a9bed5817173acdf0a2dc86ddaf36c4f0c7f9ea3
    error: Unable to get pack index http://xxx:xxx@example.org:8086/xxxxxxx.git/objects/pack/pack-a9bed5817173acdf0a2dc86ddaf36c4f0c7f9ea3.idx
    The requested URL returned error: 400
    error: Unable to find 7ec6910c4d1733d575f955063fd52b2b0ae7ca5b under http://xxx:xxx@example.org:8086/xxxxxxx.git
    Cannot obtain needed object 7ec6910c4d1733d575f955063fd52b2b0ae7ca5b
    fatal: Fetch failed.
The server error log contained the following lines:
    [Wed Feb 04 19:49:14 2009] [error] [client 192.168.1.1] Digest: uri mismatch - </xxxxxxx.git/info/refs> does not match request-uri </xxxxxxx.git/HEAD>
    [Wed Feb 04 19:49:15 2009] [error] [client 192.168.1.1] Digest: uri mismatch - </xxxxxxx.git/objects/info/packs> does not match request-uri </xxxxxxx.git/objects/pack/pack-a9bed5817173acdf0a2dc86ddaf36c4f0c7f9ea3.idx>

Does that have to do with something being missing in the Digest support or is that operator error? If the latter, what might be the culprit – how do I diagnose it?

Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/>
Daniel Stenberg· Feb 4, 2009, 22:09 UTC · re: Aristotle Pagaltzis · lore

Re: Support various HTTP authentication methods

On Wed, 4 Feb 2009, Aristotle Pagaltzis wrote:
> Does that have to do with something being missing in the Digest support or 
> is that operator error? If the latter, what might be the culprit – how do I 
> diagnose it?

Can you use curl the command line tool with Digest successfully against some of those URLs?

What libcurl version are you using?
-- 
  / daniel.haxx.se
Aristotle Pagaltzis· Feb 4, 2009, 23:25 UTC · re: Daniel Stenberg · lore

Re: Support various HTTP authentication methods

* Daniel Stenberg <daniel@haxx.se> [2009-02-04 23:15]:
Show 7 quoted lines
> On Wed, 4 Feb 2009, Aristotle Pagaltzis wrote:
>> Does that have to do with something being missing in the
>> Digest support or is that operator error? If the latter, what
>> might be the culprit – how do I diagnose it?
>
> Can you use curl the command line tool with Digest successfully
> against some of those URLs?
Yes, that works.

I have cross-checked the error and access log and here is what happens – I have marked the requests that are sent with correct credentials with a `@` and those which are sent with mismatched credentials with a `#`:

GET /x.git/info/refs : 401 GET @ /x.git/info/refs : 200 GET # /x.git/HEAD : 400 GET /x.git/objects/7e/c6910c4d1733d575f955063fd52b2b0ae7ca5b : 401 GET @ /x.git/objects/7e/c6910c4d1733d575f955063fd52b2b0ae7ca5b : 404 GET @ /x.git/objects/info/http-alternates : 404 GET @ /x.git/objects/info/alternates : 404 GET /x.git/objects/info/packs : 401 GET @ /x.git/objects/info/packs : 200 GET # /x.git/objects/pack/pack-a9bed5817173acdf0a2dc86ddaf36c4f0c7f9ea3.idx : 400

Repeating the clone always yiels this exact sequence.
If I use `curl` manually to fetch those URIs, they work just fine.
> What libcurl version are you using?
    $ curl --version
    curl 7.16.2 (i686-pc-linux-gnu) libcurl/7.16.2 OpenSSL/0.9.8g
    zlib/1.2.3 libidn/1.5
    Protocols: tftp ftp telnet dict ldap http file https ftps
    Features: IDN IPv6 Largefile NTLM SSL libz
Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/>
Daniel Stenberg· Feb 5, 2009, 08:11 UTC · re: Aristotle Pagaltzis · lore

Re: Support various HTTP authentication methods

On Thu, 5 Feb 2009, Aristotle Pagaltzis wrote:
> I have cross-checked the error and access log and here is what happens – I 
> have marked the requests that are sent with correct credentials with a `@` 
> and those which are sent with mismatched credentials with a `#`:
[...]
>    curl 7.16.2 (i686-pc-linux-gnu) libcurl/7.16.2 OpenSSL/0.9.8g

Among the 279 bugfixes done to libcurl since that release there were a few ones clearly Digest-related, so it's not totally unlikely that this problem will go away if you just update your libcurl.

-- 
  / daniel.haxx.se
Moriyoshi Koizumi· Feb 2, 2009, 08:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support various HTTP authentication methods

On Mon, Feb 2, 2009 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Applying style fixes to the existing code is very much appreciated, *but*
> please make such a clean-up patch a separate one.  A two-patch series
> whose [1/2] is such a pure clean-up without any feature change, with [2/2]
> that adds code to the cleaned-up state would be much less distracting for
> people who nitpick your changes.
Okay, I'll try to do so next time.
Show 25 quoted lines
>> @@ -153,11 +159,69 @@ static int http_options(const char *var, const char *value, void *cb)
>>                       return git_config_string(&curl_http_proxy, var, value);
>>               return 0;
>>       }
>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> +     if (!strcmp("http.auth", var)) {
>> +             if (curl_http_auth == NULL)
>> +                     return git_config_string(&curl_http_auth, var, value);
>> +             return 0;
>> +     }
>> +#endif
>> +#if LIBCURL_VERSION_NUM >= 0x070a07
>> +     if (!strcmp("http.proxy_auth", var)) {
>> +             if (curl_http_proxy_auth == NULL)
>> +                     return git_config_string(&curl_http_proxy_auth, var, value);
>> +             return 0;
>> +     }
>> +#endif
>
> If you follow config.c::git_config() you will notice that we read from
> /etc/gitconfig, $HOME/.gitconfig and then finally $GIT_DIR/config.  By
> implementing "if we already have read curl_http_auth already, we will
> ignore the later setting" like above code does, you break the general
> expectation that system-wide defaults is overridable by $HOME/.gitconfig
> and that is in turn overridable by per-repository $GIT_DIR/config.

But aren't the globals supposed to be set just here? I guessed you assume that these are set elsewhere and then it prevents the values provided later from being applied.

Show 13 quoted lines
> The preferred order would be:
>
>  - Use the value obtained from command line parameters, if any;
>
>  - Otherwise, if an environment variable is there, use it;
>
>  - Otherwise, the value obtained from git_config(), with "later one wins"
>    rule.
>
> I think you are not adding any command line option, so favoring
> environment and then using configuration is fine, but the configuration
> parser must follow the usual "later one wins" rule to avoid dissapointing
> the users.

I just followed the way other options behave. I was just not sure how I was supposed to deal with them.

Show 8 quoted lines
>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> +static long get_curl_auth_bitmask(const char* auth_method)
>
> In git codebase, asterisk that means "a pointer" sticks to the variable
> name not to type name; "const char *auth_method" (I see this file is
> already infested with such style violations, but if you are doing a
> separate clean-up patch it would be appreciated to clean them up).
>
I'm not willing to do it this time ;-)
>> +{
>> +     char buf[4096];
>
> Do you need that much space?

I think as long as we use fixed-size buffers, I should get them enough sized. If this is not preferrable, then it'd be better off using heap-allocated buffers.

Show 6 quoted lines
>> +     const unsigned char *p = (const unsigned char *)auth_method;
>> +     long mask = CURLAUTH_NONE;
>> +
>> +    strlcpy(buf, auth_method, sizeof(buf));
>
> A tab is 8-char wide.

Sorry about this. I actually was careful but I just forgot to turn off the tab expansion for the second time.

Show 15 quoted lines
> What happens when auth_method is longer than 4kB?
>
>> +
>> +     for (;;) {
>> +             char *q = buf;
>> +             while (*p && isspace(*p))
>> +                     ++p;
>
> If there is no particular reason to choose one over the other, please use
> postincrement, p++, as other existing parts of the codebase.
>
> I'll try to demonstrate what (I think) this patch should look like as a
> pair of follow-up messages to this one, but I am not sure about my rewrite
> of get_curl_auth_bitmask().  Please consider it as an easter-egg bughunt
> ;-)

I anyway appreciate this kind of knit-picking as I'd do so to newbies. Thanks very much for the advice.

Regards, Moriyoshi

Johannes Sixt· Jan 29, 2009, 10:18 UTC · re: Moriyoshi Koizumi · lore

Re: [PATCH] Support various HTTP authentication methods

Moriyoshi Koizumi schrieb:
> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
>  	if (curl_http_proxy)
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
CURLOPT_PROXY is set here...
Show 8 quoted lines
>  
> +	if (curl_http_auth) {
> +		long n = get_curl_auth_bitmask(curl_http_auth);
> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
> +	}
> +
> +	if (curl_http_proxy) {
> +		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
... and here again. Is that necessary?
-- Hannes
Moriyoshi Koizumi· Jan 29, 2009, 14:02 UTC · re: Johannes Sixt · lore

Re: [PATCH] Support various HTTP authentication methods

Johannes Sixt wrote:
Show 6 quoted lines
> Moriyoshi Koizumi schrieb:
>> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
>>  	if (curl_http_proxy)
>>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> 
> CURLOPT_PROXY is set here...
As I wrote in the previous post, this part was from the original.
Show 6 quoted lines
> 
>>  
>> +	if (curl_http_auth) {
>> +		long n = get_curl_auth_bitmask(curl_http_auth);
>> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
>> +	}
> ... and here again. Is that necessary?
What part do you mean by that?

Regards, Moriyoshi

Johannes Sixt· Jan 29, 2009, 14:08 UTC · re: Moriyoshi Koizumi · lore

Re: [PATCH] Support various HTTP authentication methods

Moriyoshi Koizumi schrieb:
Show 18 quoted lines
> Johannes Sixt wrote:
>> Moriyoshi Koizumi schrieb:
>>> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
>>>  	if (curl_http_proxy)
>>>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>> CURLOPT_PROXY is set here...
> 
> As I wrote in the previous post, this part was from the original.
> 
>>>  
>>> +	if (curl_http_auth) {
>>> +		long n = get_curl_auth_bitmask(curl_http_auth);
>>> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
>>> +	}
> 
>> ... and here again. Is that necessary?
> 
> What part do you mean by that?
Oops, sorry, I cut too much from your patch:
Show 7 quoted lines
> +	if (curl_http_auth) {
> +		long n = get_curl_auth_bitmask(curl_http_auth);
> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
> +	}
> +
> +	if (curl_http_proxy) {
> +		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
Here you set CURLOPT_PROXY again.
-- Hannes

← back to recent threads