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

From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: jonathantanmy@google.com, git@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] config: include file if remote URL matches a glob
Date: Wed, 13 Oct 2021 12:52:45 -0700	[thread overview]
Message-ID: <20211013195245.93145-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqqk0ih24a0.fsf@gitster.g>

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > I marked this as RFC because there are some design points that need to
> > be resolved:
> >
> >  - The existing "include" and "includeIf" instructions are executed
> >    immediately, whereas in order to be useful, the execution of
> >    "includeIf hasremoteurl" needs to be delayed until all config files
> >    are read. Are there better ways to do this?
> 
> An interesting chicken-and-egg problem.  Even if an included
> configuration file does not have further "include", you may discover
> there are more remotes, which may add new includes to fire from the
> top-level configuration file.

That's true. We might need to say that such conditional includes are
based only on what happened during the main config parsing.

> What if we have multiple remotes?  Is it a sufficient match for only
> one of them match what is mentioned in the includeIf condition?
> Should all of them must match the pattern instead?  Majority,
> perhaps?  Something else?

I think at least one remote should match.

> >  - Is the conditionally-included file allowed to have its own
> >    "include{,If}" instructions? I'm thinking that we should forbid it
> >    because, for example, if we had 4 files as follows: A includes B and
> >    C includes D, and we include A and C in our main config (in that
> >    order), it wouldn't be clear whether B (because A was first included)
> >    or C (because we should execute everything at the same depth first)
> >    should be executed first. (In this patch, I didn't do anything about
> >    includes.)
> 
> Interesting.  The order of real inclusion obviously would affect the
> outcome of the "last one wins" rule.  And this does not have to be
> limited to this "hasremote" condition, so we need to design it with
> a bit of care.
> 
> Would it be possible for a newly included file to invalidate an
> earlier condition that was used to decide whether to include another
> file or not?  If not, then you can take a two-pass approach where
> the first pass is used to decide solely to discover which
> conditionally included files are taken, clear the slate and the
> parse these files in the textual order.  In the case of your example
> above, the early part of the primary config would be the first to be
> read, then comes A's early part, then comes B in its entirety, then
> the rest of A, and then the middle part of the primary config, then
> C's early part, then D, and then the rest of C,... you got the idea.
>
> If it is possible for an included file to invalidate a condition we
> have already evaluated to make a decision, it would become messy.
> For example, we might decide to include another file based on the
> value we discovered for a config variable:
> 
>     === .git/config ===
>     [my] variable
>     [your] variable = false
> 
>     [includeIf "configEq:my.variable==true"]
>             file = fileA
> 
> but the included file may override the condition, e.g.
> 
>     === fileA ===
>     [my] variable = false
>     [your] variable = true
> 
> and applying the "last one wins" rule becomes messy.  I do not
> offhand know what these
> 
>     $ git config --bool my.variable
>     $ git config --bool your.variable
> 
> should say, and do not have a good explanation for possible
> outcomes.

In this case, it makes sense to me to think that files are included
entirely or not at all, so my.variable would be false and your.variable
would be true. I guess the tricky part is something like:

  === .git/config ===
  [my] variable = true
  [your] variable = false
  [includeIf "configEq:my.variable==true"]
    file = fileA
  [includeIf "configEq:my.variable==false"]
    file = fileB
  === fileA ===
    my.variable = false
  === fileB ===
    your.variable = true

and what my.variable and your.variable would end up being.

> Maybe the above example can serve as a way to guide us when we
> design the types of conditionals we allow in includeIf.  This
> example tells us that it is probably a terrible idea to allow using
> values of configuration variables as part of "includeIf" condition.

Hmm...well, remote.foo.url is a configuration variable. I think that the
two-pass approach you describe would work if we prohibit subsequent
inclusions.

> There may lead to similar "'hasremoteurl' makes a well-behaved
> condition, because [remote] are additive and not 'last-one-wins',
> but we cannot add 'lacksremoteurl' as a condition, because a file we
> decide to include based on a 'lacks' predicate may invalidate the
> 'lacks' condition by defining such a remote" design decisions you'd
> need to make around the URLs of the remotes defined for the
> repository.

And if we implement two-pass with no subsequent inclusions, "lacks"
would work the same way.