From: Phillip Wood Date: Tue, 15 Jul 2025 15:51:32 GMT Subject: Re: [PATCH 17/19] environment: move compression level into repo settings Message-ID: In-Reply-To: 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 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