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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Jul 13, 2013, 19:46 UTC
Message-ID: <47B58075-3FDD-48E5-9047-8650F7FC5E3B@gmail.com>
In-Reply-To: <7v4nbyic57.fsf@alter.siamese.dyndns.org>

On Jul 12, 2013, at 13:58, Aaron Schrab wrote:

Show 16 quoted lines

> 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.

That seems like a very reasonable expectation to me.

Show 11 quoted lines

>> 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.

On Jul 13, 2013, at 10:48, Junio C Hamano wrote:

> The only remaining issue is if matching strictly at the textual
> level is too limiting.  I personally have no strong opinion myself
> on it, and if we start with a limited form, we can always loosen it
> later, so...

The full on everything is to split the URL into all its pieces, canonizing (according to RFC 1738) the pieces and probably allowing omitted parts to act like wildcards. I'm not opposed to doing this work if that's the consensus.

However, there's probably a shortcut to all that work that will address Aaron's concern.

I expect it will be easier just to normalize the URL without splitting. That is, lowercase the parts that are case-insensitive (scheme and host name) and adjust the URL-escaping to remove URL escaping (%xx) from characters that don't need it but add it to any for which it is required that are not escaped (according to RFC 1738).

Basically a url_normalize function added to url.{h,c}. It can take a const char * and return a char * that needs to be free'd. (Perhaps it should live in http.c until some other file needs to use it and migrate then?)

This should guarantee a match in the scenario Aaron proposes above and still has pretty much the same easy explanation to the user.

Shall I go ahead and add that to the next patch version?

Or proceed with what's there right now (there are a few pending updates from reviewers) and then, as Junio says above, adjust it later if needed?