* [PATCH v3] config: add support for http.<url>.* settings
@ 2013-07-11 21:50 Kyle J. McKay
2013-07-11 23:12 ` Junio C Hamano
2013-07-12 9:59 ` Jeff King
0 siblings, 2 replies; 11+ messages in thread
From: Kyle J. McKay @ 2013-07-11 21:50 UTC (permalink / raw)
To: git
Cc: David Aguilar, Petr Baudis, Richard Hartmann, Jeff King,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
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>
---
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 some initial feedback.
Documentation/config.txt | 11 +++
http.c | 169 ++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 163 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..d6d3507 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,122 @@ 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 - 1] == '/') ||
+ (url[url_prefix_len] == '/') )
+ return url[url_prefix_len - 1] == '/'
+ ? url_prefix_len - 1 : 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). Otherwise return false
+ * and record matchlen as the 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 +293,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 +479,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] config: add support for http.<url>.* settings
2013-07-11 21:50 [PATCH v3] config: add support for http.<url>.* settings Kyle J. McKay
@ 2013-07-11 23:12 ` Junio C Hamano
[not found] ` <455666C5-7663-4361-BF34-378D3EAE2891@gmail.com>
2013-07-12 9:59 ` Jeff King
1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2013-07-11 23:12 UTC (permalink / raw)
To: Kyle J. McKay
Cc: git, David Aguilar, Petr Baudis, Richard Hartmann, Jeff King,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
"Kyle J. McKay" <mackyle@gmail.com> writes:
> +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,122 @@ 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=http://a.b/c/" vs "URLprefix=http://a.b/c/" would not return
here, because we only check up to the length of the prefix after
stripping the trailing slash from the prefix..
"URL=http://a.b/cd" vs "URLprefix=http://a.b/c/" would not return
here, because we only check up to the length of the prefix after
stripping the trailing slash from the prefix.
The latter needs to be rejected in the next condition.
> + url_len = strlen(url);
> + if ( (url_len == url_prefix_len) ||
I thought your plan was that you wanted to treat "URL=http://a.b/c/"
and "URL=http://a.b/c" the same way; taking strlen(url) and using it
for comparison without adjusting for the trailing slash makes it
smell somewhat fishy...
> + (url[url_prefix_len - 1] == '/') ||
> + (url[url_prefix_len] == '/') )
The prefix side no longer has slash at the end, and we know url and
the prefix match up to the length of the prefix at this point. Can
url[prefix-len - 1] ever be '/'? The latter (e.g. one past the
prefix length can be '/' so that the prefix "http://a.b/c" can
match against url "http://a.b/c/anything") makes sense, though.
It is not immediately obvious if this function is correct, at
least to me.
> + return url[url_prefix_len - 1] == '/'
> + ? url_prefix_len - 1 : 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). Otherwise return false
> + * and record matchlen as the last matched length of option opt.
> + */
> + if (http_option_max_matched_len[opt] > matchlen)
> + return 1;
If you had
http."http://a.b/c".opt = A
in your ~/.gitconfig and then
http."http://a.b/c".opt = B
to override it for a particular repository's .git/config, then we
read ~/.gitconfig first, remembering "http://a.b/c" has matched, and
then we read .git/config, and encounter the same URLprefix. As the
comparision is done with ">", not ">=", this allows the latter to
override the former.
Which may deserve to be added to the comment, perhaps
... length is larger (meaning the config setting should be
ignored). Upon seeing the _same_ key (i.e. new key is the
same length as the longest match so far) is not ignored, but
overrides the previous settings.
or something. It also would be nice to have a test to make sure
this will not be broken in any future change.
Other than that, overall the change looks nice.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] config: add support for http.<url>.* settings
2013-07-11 21:50 [PATCH v3] config: add support for http.<url>.* settings Kyle J. McKay
2013-07-11 23:12 ` Junio C Hamano
@ 2013-07-12 9:59 ` Jeff King
2013-07-12 13:07 ` Kyle J. McKay
1 sibling, 1 reply; 11+ messages in thread
From: Jeff King @ 2013-07-12 9:59 UTC (permalink / raw)
To: Kyle J. McKay
Cc: git, David Aguilar, Petr Baudis, Richard Hartmann,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:
> 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
I like the general direction you are going, and especially the path
prefix matching is something I had always meant to implement for the
credential code.
A few comments on the implementation:
> +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
> +};
We usually put enum fields in ALL_CAPS to mark them as constants (though
you can find few exceptions in the code).
> +static size_t http_options_url_match_prefix(const char *url,
> + const char *url_prefix,
> + size_t url_prefix_len)
> +{
It looks like you're matching the URLs as raw strings, and I don't see
any canonicalization going on. What happens if I have
"https://example.com/foo+bar" in my config, but then I visit
"https://example.comfoo%20bar"?
Or what about optional components? If I have "https://example.com" in my
config, but I am visiting "https://peff@example.com/", shouldn't it
match? The config spec is more general than my specific URL; you would
not want it to match in the opposite direction, though.
I think you would want to break down the URL into fields, canonicalize
each field by undoing any encoding, and then compare the broken-down
URLs, as credential_match does (adding in your prefix/boundary matching to
the path instead of a straight string comparison).
I think you could mostly reuse the code there by doing:
1. Create a "struct url" that contains the broken-down form; "struct
credential" would contain a url.
2. Pull out credential_from_url into "int url_parse(struct url *,
const char *)".
3. Pull out credential_match into "int url_match(struct url *, struct
url *)".
4. Parse and compare URLs from "http.$URL.*" during the config read
(similar to credential_config_callback).
That does make your by-length ordering impossible, but I don't think you
can do it in the general case. If I have:
[http "http://peff@example.com"] foo = 1
[http "http://example.com:8080"] foo = 2
and I visit "http://peff@example.com:8080", which one is the winner? I
don't think there is an unambiguous definition. I'd suggest instead just
using the usual "last one wins" strategy that our config uses. It has
the unfortunate case that:
[http "http://example.com"]
foo = 1
[http]
foo = 2
will always choose http.foo=2, but I don't think that is a big problem
in practice. You often only set one or the other, and in the cases where
you want to have a non-standard default and then override it with
another host-specific case, the "last one wins" rule makes it simple to
explain that you need to specify keys in most-general to most-specific
order.
> static int http_options(const char *var, const char *value, void *cb)
> {
> - if (!strcmp("http.sslverify", var)) {
> + const char *url = (const char *)cb;
No need to cast from void, this isn't C++. :)
The rest of the http_options() changes look like what I'd expect.
> @@ -344,7 +479,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);
No need to cast again. :)
So this is the URL that we got on the command line. Which means that if
we get a redirect, we will not re-examine the options. I think that's
OK; we do not even _see_ the redirect, as it happens internally within
curl. The credential code has the same problem, but it is not a security
issue because curl clears the credentials on redirect.
However, it may be worth mentioning in the documentation that the config
options operate on the URL you give git, _not_ necessarily on the actual
URL you are hitting at any given moment (the gitcredentials(7) page
should probably make the same distinction).
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] config: add support for http.<url>.* settings
2013-07-12 9:59 ` Jeff King
@ 2013-07-12 13:07 ` Kyle J. McKay
2013-07-12 20:58 ` Aaron Schrab
2013-07-15 4:43 ` Jeff King
0 siblings, 2 replies; 11+ messages in thread
From: Kyle J. McKay @ 2013-07-12 13:07 UTC (permalink / raw)
To: Jeff King
Cc: git, David Aguilar, Petr Baudis, Richard Hartmann,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
On Jul 12, 2013, at 02:59, Jeff King wrote:
> On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:
>
> A few comments on the implementation:
>
>> +enum http_option_type {
>> + opt_post_buffer = 0,
>> + opt_min_sessions,
>
> We usually put enum fields in ALL_CAPS to mark them as constants
> (though
> you can find few exceptions in the code).
OK.
>> +static size_t http_options_url_match_prefix(const char *url,
>> + const char *url_prefix,
>> + size_t url_prefix_len)
>> +{
>
> It looks like you're matching the URLs as raw strings, and I don't see
> any canonicalization going on. What happens if I have
> "https://example.com/foo+bar" in my config, but then I visit
> "https://example.comfoo%20bar"?
The documentation for http.<url>.* says:
>> +http.<url>.*::
> [snip]
>> + 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.
>> +
> Or what about optional components? If I have "https://example.com"
> in my
> config, but I am visiting "https://peff@example.com/", shouldn't it
> match? The config spec is more general than my specific URL; you would
> not want it to match in the opposite direction, though.
They have to be included to match. Any URL-encoding present in the
URL given to git needs to be duplicated in the URL in the config file
exactly or there will not be a match.
> That does make your by-length ordering impossible, but I don't think
> you
> can do it in the general case. If I have:
>
> [http "http://peff@example.com"] foo = 1
> [http "http://example.com:8080"] foo = 2
>
> and I visit "http://peff@example.com:8080", which one is the winner?
If I were to spilt everything out, then I would only have the second
one match. The first config is on a different port, quite possibly an
entirely different service. It shouldn't match. Consider if you were
port forwarding with ssh, services from many different machines would
all be on localhost on different ports. The second one is on the same
port and matches because it's slightly more general (missing the user
name), but it's clearly talking to the same service.
As the patch stands now, neither of them would match since the
documentation requires an exact match (except for possibly being a
prefix).
I don't think it's necessary to split the URL apart though. Whatever
URL the user gave to git on the command line (at some point even if
it's now stored as a remote setting in config) complete with URL-
encoding, user names, port names, etc. is the same url, possibly
shortened, that needs to be used for the http.<url>.option setting.
I think that's simple and very easy to explain and avoids user
confusion and surprise while still allowing a default to be set for a
site but easily overridden for a portion of that site without needing
to worry about the order config files are processed or the order of
the [http "<url>"] sections within them.
> I don't think there is an unambiguous definition. I'd suggest
> instead just
> using the usual "last one wins" strategy that our config uses. It has
> the unfortunate case that:
>
> [http "http://example.com"]
> foo = 1
> [http]
> foo = 2
>
> will always choose http.foo=2, but I don't think that is a big problem
> in practice.
I think that violates the principle of least surprise. In this case:
[http "https://cacert.org"]
sslVerify = false
[http]
sslVerify = true
the intention here is very clear -- for cacert.org only, sslVerify
should be false. To have the [http] setting step on the [http "https://cacert.org
"] setting seems completely surprising and unexpected.
The "last one wins" is still in effect though for the same paths. If
you have:
# System config
[http "https://cacert.org"]
sslVerify = false
# Global config
[http "https://cacert.org"]
sslVerify = true
The setting from the Global config still wins.
> You often only set one or the other, and in the cases where
> you want to have a non-standard default and then override it with
> another host-specific case, the "last one wins" rule makes it simple
> to
> explain that you need to specify keys in most-general to most-specific
> order.
Unless, of course, different config files are involved and that's not
possible without duplicating entries into the later-processed config
file.
>> static int http_options(const char *var, const char *value, void *cb)
>> {
>> - if (!strcmp("http.sslverify", var)) {
>> + const char *url = (const char *)cb;
>
> No need to cast from void, this isn't C++. :)
Indeed.
> The rest of the http_options() changes look like what I'd expect.
>
>> @@ -344,7 +479,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);
>
> No need to cast again. :)
Ah, but there is in order to avoid a warning:
http.c: In function ‘http_init’:
http.c:481: warning: passing argument 2 of ‘git_config’ discards
qualifiers from pointer target type
url is a const char * and git_config takes a void *.
> However, it may be worth mentioning in the documentation that the
> config
> options operate on the URL you give git, _not_ necessarily on the
> actual
> URL you are hitting at any given moment (the gitcredentials(7) page
> should probably make the same distinction).
Can do that. Will have to think about the wording for a while and
mention the %XX escapes as well.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] config: add support for http.<url>.* settings
2013-07-12 13:07 ` Kyle J. McKay
@ 2013-07-12 20:58 ` Aaron Schrab
2013-07-15 4:43 ` Jeff King
1 sibling, 0 replies; 11+ messages in thread
From: Aaron Schrab @ 2013-07-12 20:58 UTC (permalink / raw)
To: Kyle J. McKay
Cc: Jeff King, git, David Aguilar, Petr Baudis, Richard Hartmann,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
At 06:07 -0700 12 Jul 2013, "Kyle J. McKay" <mackyle@gmail.com> wrote:
>I don't think it's necessary to split the URL apart though. Whatever
>URL the user gave to git on the command line (at some point even if
>it's now stored as a remote setting in config) complete with URL-
>encoding, user names, port names, etc. is the same url, possibly
>shortened, that needs to be used for the http.<url>.option setting.
This seems to be assuming that the configuration is done after the URL
is entered and that URLs are always entered manually. I don't think
either of those assumptions is valid. A user may want to specify http
settings for all repositories on a specified host and so add settings
for that host to ~/.gitconfig expecting those settings to be used later.
A URL in a slightly different format may later be copy+pasted without
the user realizing that it won't use that config due to one of the
versions being in a non-canonical form.
>I think that's simple and very easy to explain and avoids user
>confusion and surprise while still allowing a default to be set for a
>site but easily overridden for a portion of that site without needing
>to worry about the order config files are processed or the order of the
>[http "<url>"] sections within them.
I agree that the method is easy to explain, but I think a user may very
well be surprised and confused in a scenario like I described above.
And having the order not matter (in some cases) for these configuration
items deviates from how other configuration values are handled.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] config: add support for http.<url>.* settings
2013-07-12 13:07 ` Kyle J. McKay
2013-07-12 20:58 ` Aaron Schrab
@ 2013-07-15 4:43 ` Jeff King
1 sibling, 0 replies; 11+ messages in thread
From: Jeff King @ 2013-07-15 4:43 UTC (permalink / raw)
To: Kyle J. McKay
Cc: git, David Aguilar, Petr Baudis, Richard Hartmann,
Daniel Knittl-Frank, Jan Krüger, Alejandro Mery
On Fri, Jul 12, 2013 at 06:07:35AM -0700, Kyle J. McKay wrote:
> >It looks like you're matching the URLs as raw strings, and I don't see
> >any canonicalization going on. What happens if I have
> >"https://example.com/foo+bar" in my config, but then I visit
> >"https://example.comfoo%20bar"?
>
> The documentation for http.<url>.* says:
Yes, I know. My question was more rhetorical, as in "what _should_
happen". It seems unfriendly not to do at least some basic
canonicalization with respect to encodings.
> >That does make your by-length ordering impossible, but I don't
> >think you
> >can do it in the general case. If I have:
> >
> > [http "http://peff@example.com"] foo = 1
> > [http "http://example.com:8080"] foo = 2
> >
> >and I visit "http://peff@example.com:8080", which one is the winner?
>
> If I were to spilt everything out, then I would only have the second
> one match. The first config is on a different port, quite possibly
> an entirely different service. It shouldn't match.
Yeah, sorry, that was a bad example, because a missing port is not
"do not care", but rather "default port". A better example is:
[http "http://peff@example.com/"] foo = 1
[http "http://example.com/path/"] foo = 2
If we see the URL "http://peff@example.com/path/foo", I would expect the
second one to match (it does not under a pure-prefix scheme). If we were
to make it match, you cannot say which of the two entries is "more
specific" they are both specific in different ways.
> I don't think it's necessary to split the URL apart though. Whatever
> URL the user gave to git on the command line (at some point even if
> it's now stored as a remote setting in config) complete with URL-
> encoding, user names, port names, etc. is the same url, possibly
> shortened, that needs to be used for the http.<url>.option setting.
I'm not sure I agree with this. The URL handed to git is not always
typed directly by the user. E.g., it might be cut-and-paste from an
email or website, or it may even be fed by a script (e.g., the "repo"
tool will pull URLs from its manifest file).
> I think that's simple and very easy to explain and avoids user
> confusion and surprise while still allowing a default to be set for a
> site but easily overridden for a portion of that site without needing
> to worry about the order config files are processed or the order of
> the [http "<url>"] sections within them.
But the user has to worry about last-one-wins anyway for same-length
prefixes, as you noted below.
> >using the usual "last one wins" strategy that our config uses. It has
> >the unfortunate case that:
> >
> > [http "http://example.com"]
> > foo = 1
> > [http]
> > foo = 2
> >
> >will always choose http.foo=2, but I don't think that is a big problem
> >in practice.
>
> I think that violates the principle of least surprise. In this case:
>
> [http "https://cacert.org"]
> sslVerify = false
> [http]
> sslVerify = true
Sure, I think yours is less surprising in this case. But it is more
surprising in other cases, like ones where URL encoding or optional
components are involved. E.g., imagine your two options above were
flipped (you do not usually verify ssl, but it is very important for you
to do so against cacert.org). An automated tool like repo tries to clone
from https://user@cacert.org, and you might be surprised that SSL
verification is not turned on.
> >>- git_config(http_options, NULL);
> >>+ git_config(http_options, (void *)url);
> >
> >No need to cast again. :)
>
> Ah, but there is in order to avoid a warning:
Ah, you're right. Sorry for the noise.
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-07-15 9:50 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-11 21:50 [PATCH v3] config: add support for http.<url>.* settings Kyle J. McKay
2013-07-11 23:12 ` Junio C Hamano
[not found] ` <455666C5-7663-4361-BF34-378D3EAE2891@gmail.com>
[not found] ` <7vsizjn390.fsf@alter.siamese.dyndns.org>
[not found] ` <7v4nbyic57.fsf@alter.siamese.dyndns.org>
2013-07-13 19:46 ` Kyle J. McKay
2013-07-15 4:02 ` Junio C Hamano
2013-07-15 5:12 ` Jeff King
2013-07-15 9:50 ` Kyle J. McKay
2013-07-15 5:06 ` Jeff King
2013-07-12 9:59 ` Jeff King
2013-07-12 13:07 ` Kyle J. McKay
2013-07-12 20:58 ` Aaron Schrab
2013-07-15 4:43 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).