threads / patch / 34416

v4config: add support for http.<url>.* settings

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

## tl;dr

3 messages between Jul 12, 2013 and Jul 12, 2013. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Kyle J. McKay· Jul 12, 2013, 11:20 UTC · lore

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(-)
Show changes to 2 files +162 −17

Documentation/config.txt, http.c

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
Junio C Hamano· Jul 12, 2013, 18:41 UTC · re: Kyle J. McKay · lore

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

"Kyle J. McKay" <mackyle@gmail.com> writes:
Show 33 quoted lines
> 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).
Show 5 quoted lines
> ---
>
> 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).

Show 14 quoted lines
> 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.
Show 10 quoted lines
> 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.

Show 9 quoted lines
> +	opt_min_sessions,
> +#ifdef USE_CURL_MULTI
> +	opt_max_requests,
> +#endif
> +...
> +	opt_user_agent,
> +	opt_passwd_req,
> +	opt_max
> +};
Show 15 quoted lines
> +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?
Show 6 quoted lines
> +	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
	}
Show 6 quoted lines
> @@ -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?
Junio C Hamano· Jul 12, 2013, 18:48 UTC · re: Kyle J. McKay · lore

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

"Kyle J. McKay" <mackyle@gmail.com> writes:
Show 7 quoted lines
> +	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.

Show 7 quoted lines
> -	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;
>  	}

← back to recent threads