Re: [WIP v2 2/2] config: include file if remote URL matches a glob

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 9, 2021, 00:22 UTC
Message-ID: <20211109002255.1110653-1-jonathantanmy@google.com>
In-Reply-To: <YYWTCcAljHQRTJQ/@google.com>

> To expand a little more on this:

[snip]

Show 5 quoted lines

> The nice thing about 'hasRemoteUrl' in this case is that we don't need
> to know the location of the user's big-oss-project/ checkout on disk. We
> can set that config globally and they can checkout big-oss-project as
> many times and as many places as they wish. It wouldn't be possible to
> ship configs via a package manager or other automated script without it.

Ah, thanks for the elaboration!

Show 16 quoted lines

> > NEEDSWORK: The way this works is that if we see such an include, we
> > shunt all subsequent configs into a stash (while looking for URLs), then
> > process the stash. In particular, this means that more memory is needed,
> > and the nature of error reporting changes (currently, if a callback
> > returns nonzero for a variable, processing halts immediately, but with
> > this patch, all the config might be read from disk before the callback
> > even sees the variable). I'll need to expand on this and write a
> > documentation section.
> 
> Hm. I'm not so sure about making another structure for storing config
> into memory, because we already do that during the regular config parse
> (to make things like git_config_get_string() fast). Can we not re-walk
> the in-memory config at the end of the normal parse, rather than reading
> from disk twice?
> 
> I think git_config()/repo_config() callback even does that for you for free...?

The main thing is that we wouldn't know if an entry would have been overridden by a value from an includeif.hasremoteurl or not.

> What's the easiest way to "try it and see", to add tooling and find out
> whether the config files would be reopened during the second parse?
> Because I suspect that we won't actually reopen those files, due to the
> config cache.

As Ævar says, strace should work. The hard part is implementing the recursive config parse, but it looks like the way to go, so I'll try it and see how it goes.

[1] https://lore.kernel.org/git/211106.8635o9hogz.gmgdl@evledraar.gmail.com/

Show 17 quoted lines

> So couldn't we do something like....
> 
> pass #1:
>  if (include)
>    if (not hasRemoteUrl)
>      open up path & parse
>  put config into in-memory cache normally
> pass #2: (and this pass would need to be added to repo_config() probably)
>  if (include)
>    if (hasRemoteUrl)
>      open up path & parse
>      insert in-order into in-memory cache
>  don't touch existing configs otherwise
>
> I think it's in practice similar to the approach you're using (getting
> around the weird ordering with a cache in memory), but we could reuse
> the existing config cache rather than creating a new and different one.

What do you mean by "insert in-order"? If you mean figuring out which variables would be overridden (and for multi-valued variables, what order to put all the values in), I think that's the hard part.

Another thing is that at the point where we read the config (config_with_options()), we have a callback, so we would need to make sure that we're writing to the in-memory cache in the first place (as opposed to passing a callback that does something else). That might be doable by changing the API, but in ay case, I'll try the recursive config parse first.