On Tue, Jul 15, 2025 at 9:21 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > 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.() > This is exactly what came to my mind too while reading Patrick's message. As the global variables which were shifted to `struct repo_settings` were once parsed by repo_config(), we would have no problem calling prepare_repo_settings() inside it as the behaviour would be the same as before, and it checks if the repository is null too. > > 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 >
