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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]