Re: [PATCH v3] config: add support for http.<url>.* settings
- From
Kyle J. McKay <mackyle@gmail.com>
- Date
- Jul 12, 2013, 13:07 UTC
- Message-ID
- <F5272E14-188E-4199-9523-D2ED66574D91@gmail.com>
- In-Reply-To
- <20130712095923.GA4695@sigill.intra.peff.net>
On Jul 12, 2013, at 02:59, Jeff King wrote:
Show 11 quoted lines
> 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.
Show 9 quoted lines
>> +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:
Show 9 quoted lines
>> +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. >> +
Show 5 quoted lines
> 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.
Show 8 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 6 quoted lines
>> 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.
Show 11 quoted lines
> 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 *.
Show 6 quoted lines
> 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.