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@xxxxxxxxxxx/";, 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@xxxxxxxxxxx";] foo = 1
[http "http://example.com:8080";] foo = 2
and I visit "http://peff@xxxxxxxxxxx: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.--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html