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:

quoted

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

I think this works in terms of having self-consistent rules that make
sense. But deferring things does introduce new complications in terms of
overrides, because we rely on last-one-wins. Emily asked elsewhere about
overriding the inclusion of a file. We don't have a way to do that now,
and I think it would be tricky to add. But what about overriding a
single variable?

Right now this works:

  git config --global foo.bar true
  git config --local foo.bar false

to give you "false". But imagining there was a world of deferred config,
then:

  git config --file ~/.gitconfig-foo foo.bar true
  git config --global deferInclude.path .gitconfig-foo
  git config --local foo.bar false

gives "true". We'd read .gitconfig-foo after everything else, overriding
the repo-level config.

If the deferred includes were processed at the end of each individual
file, that would solve that. You're still left with the slight oddness
that a deferred include may override options within the same file that
come after it, but that's inherent to the "defer" concept, and the
answer is probably "don't do that". It's only when it crosses file
boundaries (which are explicitly ordered by priority) that it really
hurts.

This would indeed solve the issue of the user needing to know the trick
to override variables set by deferred includes. But this wouldn't solve
our primary use case in which a system-level config defines a
conditional include but the repo config defines the URL, I think.

quoted

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

I think doing this as "continue parsing" and "resume" is hard to do.
Because you can't look at other non-remote.*.url entries here (otherwise
you'd see them out of order). So you have to either:

  - complete the parse, stashing all the other variables away, and then
    resolve the include, and then look at all the stashed variables as
    if you were parsing them anew.

  - teach our config parser how to save and restore state, including
    both intra-file state and the progress across the set of files

I am implementing something similar to your first approach (stashing
things). It's almost done so hopefully we'll have something concrete to
discuss soon.

I think it's much easier if you think of it as "start a new config parse
that does not respect hasRemoteURL". And the easiest way to do that is
to just let remote.c's existing git_config() start that parse (probably
by calling git_config_with_options() and telling it "don't respect
hasRemoteURL includes"). You may also need to teach the config parser to
be reentrant. We did some work on that a while ago, pushing the state
int config_source which functions as a stack, but I don't offhand know
if you can call git_config() from within a config callback.

Besides the reentrancy (which may be difficult, as there are some global
variables, but from a glance, some code seems to take care to save and
restore them, so it may already be reentrant or not too difficult to
make reentrant), we would have to bubble down the config (struct
git_config_source and struct config_options) into all the places that
could potentially start the parse and also have a place to store the
URLs we get. If we're already going to stash URLs, it may be easier to
stash the variables instead.

quoted

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

Yeah, I agree this won't work.

quoted

 (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.)

I think this is what I described above, and actually is less
complicated. ;)

-Peff

Well, let me finish up (2), and let's see.