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

On Fri, Aug 07, 2026 at 02:11:08PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[email protected]> 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]
> 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