[PATCH] Support various HTTP authentication methods

[PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi]

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(-)

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

Re: [PATCH] Support various HTTP authentication methods

[Junio C Hamano]

Moriyoshi Koizumi <mozo@mozo.jp> writes:

> 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...

> 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.

> 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.

>  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.

> +			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.

> +	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.

> +			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?

> +	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?

Re: [PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi]

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.

> 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.

>> +#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.

> 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.

> 
>> +			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.

>> +	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

[PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi]

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(-)

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

Re: [PATCH] Support various HTTP authentication methods

[Junio C Hamano]

Moriyoshi Koizumi <mozo@mozo.jp> writes:

> 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.

> @@ -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?

> +
> +	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
;-)

Re: [PATCH] Support various HTTP authentication methods

[Junio C Hamano]

Moriyoshi Koizumi <mozo@mozo.jp> writes:

> 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.

> @@ -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?

> +
> +	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
;-)

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

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   46 +++++++++++++++++++++-------------------------
 1 files changed, 21 insertions(+), 25 deletions(-)

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

[PATCH 2/2] Support various HTTP authentication methods

[Junio C Hamano]

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(-)

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

Re: Support various HTTP authentication methods

[Aristotle Pagaltzis]

* 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/>

Re: Support various HTTP authentication methods

[Daniel Stenberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 377 bytes --]

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

Re: Support various HTTP authentication methods

[Aristotle Pagaltzis]

* Daniel Stenberg <daniel@haxx.se> [2009-02-04 23:15]:
> 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/>

Re: Support various HTTP authentication methods

[Daniel Stenberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 572 bytes --]

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

Re: [PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi]

On Mon, Feb 2, 2009 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> @@ -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.

> 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.

>> +#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.

>> +     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.

> 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

Re: [PATCH] Support various HTTP authentication methods

[Johannes Sixt]

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...

>  
> +	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

Re: [PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi]

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?

Regards,
Moriyoshi

Re: [PATCH] Support various HTTP authentication methods

[Johannes Sixt]

Moriyoshi Koizumi schrieb:
> 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:

> +	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