Re: [RFC PATCH 0/2] Conditional config includes based on remote URL

On Mon, Oct 18, 2021 at 01:48:03PM -0700, Jonathan Tan wrote:

After some in-office discussion, here are the alternatives as I see it:

 (1) Introduce a "includeAfterIf" (or "deferIncludeIf", or some other
     name) command that is executed after all config files are read. (If
     there are multiple, they are executed in order of appearance.)
     Files included by this mechanism cannot directly or indirectly
     contain another "includeAfterIf". This is the same as what was
     introduced in this patch set, except for the name of the directive.

 (2) Leave the name as "includeIf", and when it is encountered with a
     remote-URL condition: continue parsing the config files, skipping
     all "includeIf hasRemoteUrl", only looking for remote.*.url. After
     that, resume the reading of config files at the first "includeIf
     hasRemoteUrl", using the prior remote.*.url information gathered to
     determine which files to include when "includeIf hasRemoteUrl" is
     encountered. Files included by this mechanism cannot contain any
     "remote.*.url" variables.

In all cases, the include is executed if at least one remote URL
matches.

There are other ideas including:

 (3) remote.*.url must appear before a "includeIf hasRemoteUrl" that
     wants to match it. (But this doesn't fit our use case, in which a
     repo config has the URL but a system or user config has the
     include.)

 (4) "includeIf hasRemoteUrl" triggers a search of the repo config just
     for remote.*.url. (I think this out-of-order config search is more
     complicated than (2), though.)

For (2), I think that prohibiting "remote.*.url" from any "includeIf
hasRemoteUrl" files sidesteps questions like "what happens when an
included file overrides the URL that made us include this file in the
first place" or "what happens if an included file includes a
remote.*.url that validates or invalidates a prior or subsequent file",
because now that cannot happen at all. My main concern with this
prohibition was that if we were to introduce another similar condition
(say, one based on remote names), what would happen? But I think this is
solvable - make the prohibitions based only on all the conditions that
the actually used, so if the user only uses conditions on remote URLs,
then the user can still set refspecs (for example), even after the
remote-name-condition feature is introduced in Git.

For (1), it is simpler in concept (and also in implementation, I think).
The user just needs to know that certain includes are on-the-spot and
certain includes (the ones with "after" in the name) are deferred - in
particular, if a config variable isn't the value they expect, they'll
need to check that it wasn't introduced in an includeAfterIf file. (And
the user also needs to figure out that if they want to override such a
variable, they'll need to make their own includeAfterIf with an
always-true condition.)

From the prior replies, I think that people will be more interested in
(2) as it preserves the "last config wins" rule, and I'm inclined to go
for (2) too. I'll see if others have any other opinions, and if not I'll
see how the implementation of (2) will look like.

Another concern which came up for me in a private conversation today -

How difficult will it be for users to override this include directive if
it is set somewhere outside of their control? For example:

/etc/gitconfig:
[includeIf hasRemoteUrl.https://example.com/example.git] // or whatever
  path = /etc/some-special-config

Will it be possible for a user to "un-include" /etc/some-special-config
themselves?

I don't think this should change your patch much - if my understanding
is correct, we also don't have a way to "un-include" existing include or
includeIf directives made outside of the user's control. But I wonder if
it'd be useful to think about some way to do that. Maybe we can teach
the config parse how to include a config file in reverse? Maybe we need
a "neverInclude" directive? Food for thought, anyway.

Sorry, but I won't have time to take a look at the rest of this series
til next week.

 - Emily