On Fri, Jul 11, 2025 at 11:55:27AM -0700, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
>
> > I do not think adding prepare_repo_settings() calls all over the place
> > is a good way forward as it makes it very easy to introduce
> > regressions like this. Our builtin commands parse the config at
> > startup for good reasons if we're going to move settings out of
> > git_default_core_config() we should ensure that they are still parsed
> > at startup.
>
> I think that is a good guideline that applies not just to this
> series but to other topics that attempt to move globals to a member
> in struct repository (or repository_settings)
So... the only real solution that I can think about right now is to
start parsing the repository configuration whenever we instantiate any
repository. E.g., something like the below patch. This has the effect
that the repo settings would always be populated when we have a
repository at hand. Consequently, we wouldn't need to clutter those
`prepare_repo_settings()` calls everywhere anymore.
But there is a big question: what do we do with invalid configuration
then? Do we want to die immediately when we see such command? The answer
is probably going to be a solid "sometimes":
- Some commands must function even with an invalid configuration. At
the very least git-config(1) needs to handle this alright, as
otherwise it might be impossible to unset/change invalid
configuration. There may be other such examples.
- Not all configuration is equal. It may be perfectly fine to ignore
some configuration, but other configuration may very much be mission
critical. And whether or not configuration is important isn't really
something we can decide, as it will depend on the specific use case.
So I'm afraid that there just isn't a perfect solution here. Does it
make sense to die due to a config key that isn't even used by a specific
command? Maybe. And if not, which config keys _should_ make us die in
case they are invalid?
The overall situation right now is a proper mess: we have config parsing
cluttered everywhere, and the behaviour is just plain inconsistent. Some
parsing is delayed, some isn't. Some is per-repo, some is last-one-wins.
Some config keys will cause us to die in case they are misconfigured,
some will just be ignored.
So where do we want to end up?
My dream would be that all configuration were to be defined in one
central place. The configuration should be typed, there should be
verification for each value configured by the user. All configuration
gets parsed into a structure, and it can be parsed either via a
repository (in which case we take into account its local config), or
only via the global- and system-wide configuration. The whole config
needs to be parsed at startup so that issues like the reported one don't
happen where a subprocess that uses more config keys than the parent
process dies because one of the extra keys is misconfigured.
But I very much feel like this is a pipe dream right now. We already are
working on multiple fronts to modernize the code base, and I don't quite
feel like opening up _another_ large transformation right now.
So I don't quite know what to do while we're not there yet. Without this
large refactoring, all approaches feel like they aren't a perfect fit to
address the bigger issue.
Patrick
dif
