[PATCH v4] config: add support for http.<url>.* settings

[PATCH v4] config: add support for http.<url>.* settings

[Kyle J. McKay]

The <url> value is considered a match to a url if the <url> value
is either an exact match or a prefix of the url which ends on a
path component boundary ('/').  So "https://example.com/test" will
match "https://example.com/test" and "https://example.com/test/too"
but not "https://example.com/testextra".

Longer matches take precedence over shorter matches with
environment variable settings taking precedence over all.

With this configuration:

[http]
        useragent = other-agent
        noEPSV = true
[http "https://example.com"]
        useragent = example-agent
        sslVerify = false
[http "https://example.com/path"]
        useragent = path-agent

The "https://other.example.com/" url will have useragent
"other-agent" and sslVerify will be on.

The "https://example.com/" url will have useragent
"example-agent" and sslVerify will be off.

The "https://example.com/path/sub" url will have useragent
"path-agent" and sslVerify will be off.

All three of the examples will have noEPSV enabled.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---

The credentials configuration values already support url-specific
configuration items in the form credential.<url>.*.  This patch
adds similar support for http configuration values.

This version of the patch addresses the following feedback:

  * The url[url_prefix_len-1] test can now never succeed.  It is removed.

  * More text added to check_matched_len to clarify behavior when two
    options have the same key.

 Documentation/config.txt |  11 ++++
 http.c                   | 168 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 162 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b4d4887..3731a3a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1531,6 +1531,17 @@ http.useragent::
 	of common USER_AGENT strings (but not including those like git/1.7.1).
 	Can be overridden by the 'GIT_HTTP_USER_AGENT' environment variable.
 
+http.<url>.*::
+	Any of the http.* options above can be applied selectively to some urls.
+	For example "http.https://example.com.useragent" would set the user
+	agent only for https connections to example.com.  The <url> value
+	matches a url if it is an exact match or a prefix of the url matching
+	at a '/' boundary.  Longer <url> matches take precedence over shorter
+	ones with the environment variable settings taking precedence over all.
+	Note that <url> must match the url exactly (other than possibly being a
+	prefix).  This means any user, password and/or port setting that appears
+	in a url must also appear in <url> to have a successful match.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; Git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/http.c b/http.c
index 2d086ae..feca70a 100644
--- a/http.c
+++ b/http.c
@@ -30,6 +30,34 @@ static CURL *curl_default;
 
 char curl_errorstr[CURL_ERROR_SIZE];
 
+enum http_option_type {
+	opt_post_buffer = 0,
+	opt_min_sessions,
+#ifdef USE_CURL_MULTI
+	opt_max_requests,
+#endif
+	opt_ssl_verify,
+	opt_ssl_try,
+	opt_ssl_cert,
+#if LIBCURL_VERSION_NUM >= 0x070903
+	opt_ssl_key,
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070908
+	opt_ssl_capath,
+#endif
+	opt_ssl_cainfo,
+	opt_low_speed,
+	opt_low_time,
+	opt_no_epsv,
+	opt_http_proxy,
+	opt_cookie_file,
+	opt_user_agent,
+	opt_passwd_req,
+	opt_max
+};
+
+static size_t http_option_max_matched_len[opt_max];
+
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
@@ -141,34 +169,121 @@ static void process_curl_messages(void)
 }
 #endif
 
+static size_t http_options_url_match_prefix(const char *url,
+					    const char *url_prefix,
+					    size_t url_prefix_len)
+{
+	/*
+	 * url_prefix matches url if url_prefix is an exact match for url or it
+	 * is a prefix of url and the match ends on a path component boundary.
+	 * Both url and url_prefix are considered to have an implicit '/' on the
+	 * end for matching purposes if they do not already.
+	 *
+	 * url must be NUL terminated.  url_prefix_len is the length of
+	 * url_prefix which need not be NUL terminated.
+	 *
+	 * The return value is the length of the match in characters (excluding
+	 * any final '/') or 0 for no match.  Passing "/" as url_prefix will
+	 * always cause 0 to be returned.
+	 */
+	size_t url_len;
+	if (url_prefix_len && url_prefix[url_prefix_len - 1] == '/')
+		url_prefix_len--;
+	if (!url_prefix_len || strncmp(url, url_prefix, url_prefix_len))
+		return 0;
+	url_len = strlen(url);
+	if ((url_len == url_prefix_len) || (url[url_prefix_len] == '/'))
+		return url_prefix_len;
+	return 0;
+}
+
+static int check_matched_len(enum http_option_type opt, size_t matchlen)
+{
+	/*
+	 * Check the last matched length of option opt against matchlen and
+	 * return true if the last matched length is larger (meaning the config
+	 * setting should be ignored).  Upon seeing the _same_ key (i.e. new key
+	 * has the same match length which is therefore not larger) the new
+	 * setting will override the previous setting.  Otherwise return false
+	 * and record matchlen as the current last matched length of option opt.
+	 */
+	if (http_option_max_matched_len[opt] > matchlen)
+		return 1;
+	http_option_max_matched_len[opt] = matchlen;
+	return 0;
+}
+
 static int http_options(const char *var, const char *value, void *cb)
 {
-	if (!strcmp("http.sslverify", var)) {
+	const char *url = (const char *)cb;
+	const char *key, *dot;
+	size_t matchlen = 0;
+
+	key = skip_prefix(var, "http.");
+	if (!key)
+		return git_default_config(var, value, cb);
+
+	/*
+	 * If the part following the leading "http." contains a '.' then check
+	 * to see if the part between "http." and the last '.' is a match to
+	 * url.  If it's not then ignore the setting.  Otherwise set key to
+	 * point to the option which is the part after the final '.' and
+	 * use key in subsequent comparisons to determine the option type.
+	 */
+	dot = strrchr(key, '.');
+	if (dot) {
+		matchlen = http_options_url_match_prefix(url, key, dot - key);
+		if (!matchlen)
+			return 0;
+		key = dot + 1;
+	}
+
+	if (!strcmp("sslverify", key)) {
+		if (check_matched_len(opt_ssl_verify, matchlen))
+			return 0;
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp("http.sslcert", var))
+	if (!strcmp("sslcert", key)) {
+		if (check_matched_len(opt_ssl_cert, matchlen))
+			return 0;
 		return git_config_string(&ssl_cert, var, value);
+	}
 #if LIBCURL_VERSION_NUM >= 0x070903
-	if (!strcmp("http.sslkey", var))
+	if (!strcmp("sslkey", key)) {
+		if (check_matched_len(opt_ssl_key, matchlen))
+			return 0;
 		return git_config_string(&ssl_key, var, value);
+	}
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-	if (!strcmp("http.sslcapath", var))
+	if (!strcmp("sslcapath", key)) {
+		if (check_matched_len(opt_ssl_capath, matchlen))
+			return 0;
 		return git_config_string(&ssl_capath, var, value);
+	}
 #endif
-	if (!strcmp("http.sslcainfo", var))
+	if (!strcmp("sslcainfo", key)) {
+		if (check_matched_len(opt_ssl_cainfo, matchlen))
+			return 0;
 		return git_config_string(&ssl_cainfo, var, value);
-	if (!strcmp("http.sslcertpasswordprotected", var)) {
+	}
+	if (!strcmp("sslcertpasswordprotected", key)) {
+		if (check_matched_len(opt_passwd_req, matchlen))
+			return 0;
 		if (git_config_bool(var, value))
 			ssl_cert_password_required = 1;
 		return 0;
 	}
-	if (!strcmp("http.ssltry", var)) {
+	if (!strcmp("ssltry", key)) {
+		if (check_matched_len(opt_ssl_try, matchlen))
+			return 0;
 		curl_ssl_try = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp("http.minsessions", var)) {
+	if (!strcmp("minsessions", key)) {
+		if (check_matched_len(opt_min_sessions, matchlen))
+			return 0;
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
 		if (min_curl_sessions > 1)
@@ -177,39 +292,58 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 #ifdef USE_CURL_MULTI
-	if (!strcmp("http.maxrequests", var)) {
+	if (!strcmp("maxrequests", key)) {
+		if (check_matched_len(opt_max_requests, matchlen))
+			return 0;
 		max_requests = git_config_int(var, value);
 		return 0;
 	}
 #endif
-	if (!strcmp("http.lowspeedlimit", var)) {
+	if (!strcmp("lowspeedlimit", key)) {
+		if (check_matched_len(opt_low_speed, matchlen))
+			return 0;
 		curl_low_speed_limit = (long)git_config_int(var, value);
 		return 0;
 	}
-	if (!strcmp("http.lowspeedtime", var)) {
+	if (!strcmp("lowspeedtime", key)) {
+		if (check_matched_len(opt_low_time, matchlen))
+			return 0;
 		curl_low_speed_time = (long)git_config_int(var, value);
 		return 0;
 	}
 
-	if (!strcmp("http.noepsv", var)) {
+	if (!strcmp("noepsv", key)) {
+		if (check_matched_len(opt_no_epsv, matchlen))
+			return 0;
 		curl_ftp_no_epsv = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp("http.proxy", var))
+	if (!strcmp("proxy", key)) {
+		if (check_matched_len(opt_http_proxy, matchlen))
+			return 0;
 		return git_config_string(&curl_http_proxy, var, value);
+	}
 
-	if (!strcmp("http.cookiefile", var))
+	if (!strcmp("cookiefile", key)) {
+		if (check_matched_len(opt_cookie_file, matchlen))
+			return 0;
 		return git_config_string(&curl_cookie_file, var, value);
+	}
 
-	if (!strcmp("http.postbuffer", var)) {
+	if (!strcmp("postbuffer", key)) {
+		if (check_matched_len(opt_post_buffer, matchlen))
+			return 0;
 		http_post_buffer = git_config_int(var, value);
 		if (http_post_buffer < LARGE_PACKET_MAX)
 			http_post_buffer = LARGE_PACKET_MAX;
 		return 0;
 	}
 
-	if (!strcmp("http.useragent", var))
+	if (!strcmp("useragent", key)) {
+		if (check_matched_len(opt_user_agent, matchlen))
+			return 0;
 		return git_config_string(&user_agent, var, value);
+	}
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
@@ -344,7 +478,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 
 	http_is_verbose = 0;
 
-	git_config(http_options, NULL);
+	git_config(http_options, (void *)url);
 
 	curl_global_init(CURL_GLOBAL_ALL);
 
-- 
1.8.3

Re: [PATCH v4] config: add support for http.<url>.* settings

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> The <url> value is considered a match to a url if the <url> value
> is either an exact match or a prefix of the url which ends on a
> path component boundary ('/').  So "https://example.com/test" will
> match "https://example.com/test" and "https://example.com/test/too"
> but not "https://example.com/testextra".
>
> Longer matches take precedence over shorter matches with
> environment variable settings taking precedence over all.
>
> With this configuration:
>
> [http]
>         useragent = other-agent
>         noEPSV = true
> [http "https://example.com"]
>         useragent = example-agent
>         sslVerify = false
> [http "https://example.com/path"]
>         useragent = path-agent
>
> The "https://other.example.com/" url will have useragent
> "other-agent" and sslVerify will be on.
>
> The "https://example.com/" url will have useragent
> "example-agent" and sslVerify will be off.
>
> The "https://example.com/path/sub" url will have useragent
> "path-agent" and sslVerify will be off.
>
> All three of the examples will have noEPSV enabled.
>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>

I haven't reviewed this version (yet).

> ---
>
> The credentials configuration values already support url-specific
> configuration items in the form credential.<url>.*.  This patch
> adds similar support for http configuration values.

Let's move the above three lines into the proposed log message and
make it its first paragraph.  A log message should say what it is
about (i.e. what does "add support" really mean?  what kind of
functionality the new support brings to us?) upfront and then
explain what it does in detail (i.e. the explanation of matching
semantics you have as the first paragraph).

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b4d4887..3731a3a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1531,6 +1531,17 @@ http.useragent::
>  	of common USER_AGENT strings (but not including those like git/1.7.1).
>  	Can be overridden by the 'GIT_HTTP_USER_AGENT' environment variable.
>  
> +http.<url>.*::
> +	Any of the http.* options above can be applied selectively to some urls.
> +	For example "http.https://example.com.useragent" would set the user
> +	agent only for https connections to example.com.  The <url> value
> +	matches a url if it is an exact match or a prefix of the url matching
> +	at a '/' boundary.

Given Peff's review, I am no longer sure if the "strictly textual
match" is the semantics we want.  At least, it is easy to explain
and understand, but it might be too limiting to be useful.

Let's assume it is what we want, for the rest of the review.

> diff --git a/http.c b/http.c
> index 2d086ae..feca70a 100644
> --- a/http.c
> +++ b/http.c
> @@ -30,6 +30,34 @@ static CURL *curl_default;
>  
>  char curl_errorstr[CURL_ERROR_SIZE];
>  
> +enum http_option_type {
> +	opt_post_buffer = 0,

Do we need to have "= 0" here?

Is this order meant to match some externally defined order
(e.g. alphabetical, or the value of corresponding constants defined
in the cURL library), other than "opt_max is not a real option but
just has to be there at the end to count all of them"?

I am wondering if it would make it easier to read to move all the #ifdef
at the end immediately before opt-max, or something.

> +	opt_min_sessions,
> +#ifdef USE_CURL_MULTI
> +	opt_max_requests,
> +#endif
> +...
> +	opt_user_agent,
> +	opt_passwd_req,
> +	opt_max
> +};

> +static int check_matched_len(enum http_option_type opt, size_t matchlen)
> +{
> +	/*
> +	 * Check the last matched length of option opt against matchlen and
> +	 * return true if the last matched length is larger (meaning the config
> +	 * setting should be ignored).  Upon seeing the _same_ key (i.e. new key
> +	 * has the same match length which is therefore not larger) the new
> +	 * setting will override the previous setting.  Otherwise return false
> +	 * and record matchlen as the current last matched length of option opt.
> +	 */
> +	if (http_option_max_matched_len[opt] > matchlen)
> +		return 1;
> +	http_option_max_matched_len[opt] = matchlen;
> +	return 0;
> +}

A "check_foo" that returns either 0 or 1 usually mean 1 is good and
0 is not.  A "check_foo" that returns either negative or 0 usually
mean negative is an error and 0 is good.

In general, "check_foo" is a bad name for a boolean function.  This
checks "seen_longer_match()", and it is up to the caller if it
considers good or bad to have a matching element that is longer or
shorter what it has already seen.  See below.

>  static int http_options(const char *var, const char *value, void *cb)
>  {
> -	if (!strcmp("http.sslverify", var)) {
> +	const char *url = (const char *)cb;

Cast?

> +	if (!strcmp("sslverify", key)) {
> +		if (check_matched_len(opt_ssl_verify, matchlen))
> +			return 0;
>  		curl_ssl_verify = git_config_bool(var, value);
>  		return 0;
>  	}

At this caller, the name "check_matched_len()" is not immediately
obvious, other than it is checking matchlen, what it does.  "check"
needs "what is checked" (i.e. "matchlen"), but it also needs "how it
judges what is checked" (i.e. is longer the better?), but the name
does not convey that.  If you rename the function, it becomes a lot
easier to understand what is going on.

	if (!strcmp("sslverify", key)) {
		if (!seen_longer_match(opt_ssl_verify, matchlen))
			curl_ssl_verify = git_config_bool(var, value);
		return 0
	}

> @@ -344,7 +478,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  
>  	http_is_verbose = 0;
>  
> -	git_config(http_options, NULL);
> +	git_config(http_options, (void *)url);

Cast?

Re: [PATCH v4] config: add support for http.<url>.* settings

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> +	if (!strcmp("sslcertpasswordprotected", key)) {
> +		if (check_matched_len(opt_passwd_req, matchlen))
> +			return 0;
>  		if (git_config_bool(var, value))
>  			ssl_cert_password_required = 1;
>  		return 0;
>  	}

This is not a new problem, but I think the existing code is wrong.
There is no way to countermand an earlier

	[http]
        	sslcertpasswordprotected

in a more generic configuration file with


	[http]
        	sslcertpasswordprotected = no

in a repository specific configuration file.

Perhaps we should fix it as a preparatory patch (1/2) before the
main "feature addition" patch.

> -	if (!strcmp("http.ssltry", var)) {
> +	if (!strcmp("ssltry", key)) {
> +		if (check_matched_len(opt_ssl_try, matchlen))
> +			return 0;
>  		curl_ssl_try = git_config_bool(var, value);
>  		return 0;
>  	}