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

From: Jeff King <peff@peff.net>
Date: Jul 24, 2013, 06:42 UTC
Message-ID: <20130724064258.GA30074@sigill.intra.peff.net>
In-Reply-To: <DF5B20F8-33C2-4F72-A78B-97EE1FB4A522@gmail.com>

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

Show 12 quoted lines

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