Re: [PATCH v8 4/4] config: allow http.<url>.* any user matching

"Kyle J. McKay" [off-list ref] writes:

Previously the <url> had to specify an exactly matching user name
and password if those were present in the url being matched against.

Now the password portion is always ignored and omitting the user
name from <url> allows it to match against any user name.

Signed-off-by: Kyle J. McKay <redacted>
---
 Documentation/config.txt |  29 +++--

Thanks.

A solid wall of text is somewhat hard to read, so I'd queue the
equivalent of the following "git diff -w" output on top.  I also was
trying to see if we can clarify the "length comparison" only refers
to the length of the path part, excluding the length of "user@"
(i.e. when comparing "https://user@example.com/path" with
"https://example.com/path", they are of the same length), which you
can see in the first three lines below.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c418adf..635ed5d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1521,9 +1521,11 @@ http.<url>.*::
 	path portion is an exact match or a prefix that matches at a "/"
 	boundary.  If <url> does not include a user name, it will match a url
 	with any username otherwise the user name must match as well (the
-	password part, if present in the url, is always ignored).  Longer <url>
-	path matches take precedence over shorter matches no matter what order
-	they occur in.  For example, if both "https://user@example.com/path" and
+	password part, if present in the url, is always ignored).  A <url>
+	with longer path matches take precedence over shorter matches no matter
+	what order they occur in the configuration file.
++
+For example, if both "https://user@example.com/path" and
 "https://example.com/path/name" are used as a config <url> value and
 then "https://user@example.com/path/name/here" is passed to a git
 command, the settings in the "https://example.com/path/name" section

I am not yet convinced that the precedence rule specified in this
what we want (I do not have an example why it is *not* what we want,
either).  Another definition could be "if user@ is present in the
request, give lower precedence to config entries for the site
without user@ than entries with user@", and I do not have a strong
opinion myself which one between the two is better (and there may be
third and other possible rule).

Comments?

On Jul 22, 2013, at 11:00, Junio C Hamano wrote:

"Kyle J. McKay" [off-list ref] writes:

quoted

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e461f32..c418adf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1517,15 +1517,26 @@ 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 refers to the same scheme, host and port and  
the
+	path portion is an exact match or a prefix that matches at a "/"
+	boundary.  If <url> does not include a user name, it will match a  
url
+	with any username otherwise the user name must match as well (the
+	password part, if present in the url, is always ignored).  Longer  
<url>
+	path matches take precedence over shorter matches no matter what  
order
+	they occur in.  For example, if both "https://user@example.com/ 
path" and
+	"https://example.com/path/name" are used as a config <url> value  
and
+	then "https://user@example.com/path/name/here" is passed to a git
+	command, the settings in the "https://example.com/path/name"  
section
+	will be preferred because that <url> has a longer path length  
match than
+	"https://user@example.com/path" even though the latter did match  
the
+	user.  For same length matches, the last one wins except that a  
same
+	length <url> match that includes a user name will be preferred  
over a
+	same length <url> match that does not.  The urls are normalized  
before
+	matching so that equivalent urls that are simply spelled  
differently
+	will match properly.  Environment variable settings always  
override any
+	matches.  The urls that are matched against are those given  
directly to
+	git commands.  This means any urls visited as a result of a  
redirection
+	do not participate in matching.

A solid wall of text is somewhat hard to read, so I'd queue the
equivalent of the following "git diff -w" output on top.

Can I send out the change as a 'fixup!' patch?  Or do I need to send a  
new v9 patch series with the documentation update?

OK.

I am not yet convinced that the precedence rule specified in this
what we want (I do not have an example why it is *not* what we want,
either).  Another definition could be "if user@ is present in the
request, give lower precedence to config entries for the site
without user@ than entries with user@", and I do not have a strong
opinion myself which one between the two is better (and there may be
third and other possible rule).

Comments?

Consider this site:

example.com/
example.com/dir
example.com/dir/sub
example.com/dir/sub/public

Suppose I want to configure a particular key for example.com/dir/sub  
for a particular user, say "contractor".

I can configure:

[http "https://contractor@example.com/dir/sub"]
   sslkey=contractor_key

But then I want to configure a completely different key for the public  
area example.com/dir/sub/public no matter what user.  I just add this:

[http "https://example.com/dir/sub/public"]
   sslkey=public_key

But if entries with usernames take precedence it won't work.  The only  
way to do it is to list every possible user for "example.com/dir1/sub1/ 
public" in its own new section and then continue to add a new config  
section every time a new user name is encountered.

Conversely, if a special key is supposed to be used for the user  
"contractor" everywhere on the site at or below "example.com/dir/sub",  
then the above configuration doesn't work unless user matches take  
precedence.  With the current code, one additional entry would have to  
be added like so:

[http "https://contractor@example.com/dir/sub/public"]
   sslkey=contractor_key

So my thinking was that having user matches take precedence over path  
length matches can result in endless additions to the config file  
(because you have to list all the other users to override a sub area  
and that could be a large list) whereas having path length matches  
take precedence over user matches will only result in a few, finite  
additions to the config file (the number of already-configured items  
with a longer path).

On Mon, Jul 22, 2013 at 01:24:06PM -0700, Kyle J. McKay wrote:

quoted

I am not yet convinced that the precedence rule specified in this
what we want (I do not have an example why it is *not* what we want,
either).  Another definition could be "if user@ is present in the
request, give lower precedence to config entries for the site
without user@ than entries with user@", and I do not have a strong
opinion myself which one between the two is better (and there may be
third and other possible rule).

Comments?

Consider this site:
[...]

Thanks for explaining, and sorry I missed out on the last few rounds of
review.

I think your scheme (normalization plus special handling of the username
field) addresses my biggest concern, which is matching in the face of
optional usernames. The only two things that make me wary are:

  1. The explanation and special-casing of username is a little
     complicated to explain.

  2. The behavior for resolving the value when faced with multiple
     possibilities is completely unlike the rest of the config system
     (both dropping last-one-wins, and unlike the URL matching for
     credentials).

I think we can decide that (2) is worth it if your semantics are more
flexible in practice. It would be nice to see real-world feedback on how
people use it before setting the behavior in stone, but there's sort of
a chicken and egg problem there.

For (1), I wonder if the explanation would be simpler if the precedences
of each sub-part were simply laid out. That is, would it be correct to
say something like:

  For a config key to match a URL, each element of the config key (if
  present) is compared to that of the URL, in the following order:

    1. Protocol (e.g., `https` in `https://example.com/`). This field
       must match exactly between the config key and the URL.

    2. Host/domain name (e.g., `example.com` in `https://example.com/`).
       This field must match exactly between the config key and the URL.

    3. Path (e.g., `repo.git` in `https://example.com/repo.git`). This
       field is prefix-matched by slash-delimited path elements, so that
       config key `foo/` matches URL `foo/bar`. Longer matches take
       precedence (so `foo/bar`, if it exists, is a better match than
       just `foo/`).

    4. Username (e.g., `user` in `https://user@example.com/repo.git`).

  The list above is ordered by decreasing precedence; a URL that matches
  a config key's path is preferred to one that matches its username.

I don't know if that is more or less clear of an explanation. It makes
more sense to me, but that is probably because I wrote it. I'm also not
100% sure it describes your implementation, but I think it is equivalent
to the prefix matching with normalization.

I have a few other comments on specific patches; I'll send them
separately.

-Peff