Re: [PATCH v6 2/2] config: include file if remote URL matches a glob

From: Glen Choo <chooglen@google.com>
Date: Dec 9, 2021, 23:33 UTC
Message-ID: <kl6l8rwt9ww2.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20211209223919.513113-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 46 quoted lines

>> > +`hasconfig:remote.*.url:`::
>> > +	The data that follows this keyword is taken to
>> > +	be a pattern with standard globbing wildcards and two
>> > +	additional ones, `**/` and `/**`, that can match multiple
>> > +	components. The first time this keyword is seen, the rest of
>> > +	the config files will be scanned for remote URLs (without
>> > +	applying any values). If there exists at least one remote URL
>> > +	that matches this pattern, the include condition is met.
>> > ++
>> > +Files included by this option (directly or indirectly) are not allowed
>> > +to contain remote URLs.
>> 
>> Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
>> should make it clear to users *why* we choose to forbid remote URLs.
>> 
>> Since this series is setting a precedent for future "hasconfig:"
>> conditions (files included by "hasconfig:foo.*.bar" cannot contain any
>> "foo.*.bar" values), it would be useful to git developers to explain
>> *why* we chose to do this. And if we're documenting it for ourselves,
>> we might as well write it in the public docs. That way, users would know
>> that this is more of a guardrail (because it's simpler to understand
>> this way) than a hard limitation.
>> 
>> [1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
>
> The explanation is rather long, though. It goes something like this:
>
>   If the main config is:
>
>   [remote a]
>     url = bar
>   [includeif hasconfig:remote.*.url:foo]
>     path = foo
>   [includeif hasconfig:remote.*.url:bar]
>     path = bar
>
>   and "bar" contains:
>
>   [remote b]
>     url = foo
>
>   Should "foo" be included? For now, we avoid these situations
>   completely by prohibiting URLs from being configured in "includeif
>   hasconfig".
>
> If you can think of a concise explanation, maybe we can include it.

Yeah, I can't think of a concise-yet-clear way to convey this to users (if I had thought of one, I wouldn't have prefaced my original comment with "Wondering out loud").

Spitballing here...

  `hasconfig:remote.*.url:`::
    The data that follows this keyword is taken to
    be a pattern with standard globbing wildcards and two
    additional ones, `**/` and `/**`, that can match multiple
    components. The first time this keyword is seen, the rest of
    the config files will be scanned for remote URLs (without
    applying any values). If there exists at least one remote URL
    that matches this pattern, the include condition is met.

  - Files included by this option (directly or indirectly) are not allowed
  - to contain remote URLs.
  + Because new remote URLs might affect the correctness of the include
  + condition, files included by this option (directly or indirectly) are
  + not allowed to contain remote URLs.

Although, upon further reflection, I wonder if this approach of banning config variables really gives us the safety we want after all. Reworking your example, say we expand "hasconfig" to include "hasconfig:branch.*.merge" then we can have this in the main config:

   [remote a]
     url = baz
   [branch c]
     merge = bar

   [includeif hasconfig:remote.*.url:foo]
     path = foo
   [includeif hasconfig:branch.*.merge:bar]
     path = bar

and "bar" contains:

   [remote b]
     url = foo

we end up with the exact same question of "Should "foo" be included?". This shows that the rule isn't actually "files included by hasconfig:remote.*.url cannot include remote.*.url", but the much more restrictive "files included by hasconfig:<anything> cannot include any config values that can appear in hasconfig". This sounds pretty unusable to me..

But I think that with the semantics you've defined, we don't really need to forbid config variables. This section describes:

  The first time this keyword is seen, the rest of the config files will
  be scanned for remote URLs (without applying any values). If there
  exists at least one remote URL that matches this pattern, the include
  condition is met.

which, to me, gives us a pass to say "the first time we see a hasconfig, we will do an additional scan without applying values". That doesn't sound _too_ confusing to me, but I don't know how it looks to someone with fresh eyes.

Forgive me if this exact suggestion came up before on-list (I know we've discussed this exact approach off-list).