Hi Patrick
On 15/07/2025 12:27, Patrick Steinhardt wrote:
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.
That's a good point.
- 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.
Indeed. My objection here was that we were delaying the parsing when it
wasn't delayed before. Is it feasible to call prepare_repo_settings() in
repo_config()? That would at least avoid the problem that moving config
settings into `struct repo_settings` changes when the settings are
parsed unless the command calls prepare_repo_settings() at start up. As
far as I remember `git config` uses config_with_options() so that would
not be adversely affected by such a change.
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.
Being able to verify config settings when they're set would be a great
improvement but we're a long way from being able to do that.
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.
I agree with this
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.
I agree addressing all the shortcomings you've outlined would require a
lot of refactoring. If we can find a way to avoid introducing anymore
shortcomings as we migrate away from global variables that would be a
good start.
Thanks
Phillip
