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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 15, 2025, 15:51 UTC
Message-ID: <f6479d6a-32a4-4a49-a75c-589978cb9a57@gmail.com>
In-Reply-To: <aHY7LYHqVj-ECf_z@pks.im>

Hi Patrick

On 15/07/2025 12:27, Patrick Steinhardt wrote:

Show 29 quoted lines

> On Fri, Jul 11, 2025 at 11:55:27AM -0700, Junio C Hamano wrote:
>> 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.

That's a good point.

Show 13 quoted lines

>    - 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.

Show 9 quoted lines

> 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.

Show 11 quoted lines

> 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