Re: [PATCH v3 0/3] environment: clean up repository config handling

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 10, 2026, 05:50 UTC
Message-ID: <anlmwaEtwcCPse1N@pks.im>
In-Reply-To: <xmqq1pc9eivn.fsf@gitster.g>

On Fri, Aug 07, 2026 at 02:11:08PM -0700, Junio C Hamano wrote:

Show 35 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Fri, Aug 07, 2026 at 04:59:29PM +0800, Tian Yuchen wrote:
> >> Hi all,
> >> 
> >> This series contains several cleanup patches for repository configuration
> >> handling.
> >> 
> >> No functional changes are intended. The patches make the related code
> >> more consistent and easier to maintain by improving documentation,
> >> formatting, and the organization of repo_config_values.
> >> 
> >> RFC:
> >> If there are other small cleanups in this area that would be useful to
> >> include, suggestions are welcome.
> >
> > Somewhat unrelated to this patch series, but I was wondering whether you
> > plan to drop the limitation in `repo_config_values()` that requires that
> > the passed-in repository is `the_repository`. This limitation is
> > starting to create problems as more and more of our infrastructure is
> > migrating into `struct repo_config_values`, so using a different repo
> > than `the_repository` is starting to become harder and harder in our
> > codebase.
> >
> > Thanks!
> >
> > Patrick
> 
> Hmph, that is an interesting point.  What is our plan to really
> enable the use of repository instances other than 'the_repository'
> here?  They of course need to be initialized with repo_init(),
> but is that enough to sensibly use the embedded 'repo_settings'
> and 'repo_config_values' structures?  (By the way, it is not
> entirely clear to me why we need both and how we sift variables
> between them.)

Yeah, this split is adding to the confusion indeed. I think that we should make it a goal to unify those going forward.

[snip]

Show 8 quoted lines

> In any case, all of that has little to do with this series, I
> suspect, unless we are redesigning these configurations and
> settings in such a way that they are not necessarily tied to
> any repository instance.  While I do not know the exact details,
> I can imagine a hierarchical system where system- and
> user-wide sets of setting values are known independently of any
> repository, only to be overridden by per-repository settings
> using a last-one-wins strategy at lookup time.

I've been wondering for a while whether we're operating at the wrong level here. Both `repo_settings` and `repo_config_values` indicates that we're operating in the context of a repository, but as you mention that may not even be the case.

I don't think the approach is inherently flawed though. From my point of view, the best way forward is to merge those two and then generalize them into something like `git_config_values` or `git_settings`, depending on which of both variants we want to retain. We would then have two levels:

  - One on the repository level as we have it today.

  - One truly global variable, because that stuff in fact _is_ global.

We'd then adapt `repo_config_values()` so that it knows to populate either of those variables depending on whether or not the user passes a valid repository, and returns a constant pointer to the respective structure. Callers MUST NOT modify that structure -- if they want to, they'll have to make a copy and pass it down the calling stack.

The last part about not modifying that structure could be quite a bit painful though, as it would mean that we might have to adapt call chains to pass down a `struct git_config_values` instead of a `struct repository`. But arguably, that's the right thing to do anyway for at least some subsystems that are independent of repositories.

As you say though, none of this is really related to this patch series at hand, and I don't think we need to resolve this discussion before we can merge it. I just want to make sure that we have a plan for how to get rid of `the_repository` instead of only shuffling stuff around.

Patrick