> On Mon, Oct 18, 2021 at 01:48:03PM -0700, Jonathan Tan wrote: > > > (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. > > (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. > > 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. > > > (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.
