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

[Ayush Chandekar <ayu.chandekar@gmail.com>]

On Tue, Jul 15, 2025 at 9:21 PM Phillip Wood <phillip.wood123@gmail.com> 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@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.
>
> >    - 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
>