Re: [PATCH 17/19] environment: move compression level into repo settings

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 15, 2025, 11:27 UTC
Message-ID: <aHY7LYHqVj-ECf_z@pks.im>
In-Reply-To: <xmqqbjpq1rs0.fsf@gitster.g>

On Fri, Jul 11, 2025 at 11:55:27AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> 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