Re: [PATCH v2 7/8] refs: fix recursing `get_main_ref_store()` with "onbranch" config

[Jeff King <peff@peff.net>]

On Fri, Jun 19, 2026 at 08:25:42AM +0200, Patrick Steinhardt wrote:

> On Thu, Jun 18, 2026 at 12:40:35PM -0400, Jeff King wrote:
> > On Mon, Jun 15, 2026 at 03:56:53PM +0200, Patrick Steinhardt wrote:
> [snip]
> > I'd expect the ref database config (like the ref format) to be read not
> > through the regular config subsystem, but via read_repository_format()
> > and friends. And while that does build on the regular config code, it
> > should never enable includes at all. So includeIf.onbranch:foo.path is
> > just another uninteresting config key to it.
> 
> This feels rather painful though, as we'd now have to do this for every
> single backend that we know about. Also, I think not enabling includes
> is an overly broad fix: there isn't any reason why "includeif.gitdir"
> and all the other conditions shouldn't apply. We really only want to
> disable "onbranch".

Sorry, I should probably gone back and edited my email after finishing
it. I was thinking that you meant not general config, but the specific
extensions.refStorage key. Which is not really config, but repo metadata
we happen to store in the .git/config file. And obviously you cannot
read any refs until you know what's in that key.

And that _is_ read separately while loading the repo config, which I
think is right. Other options, like core.logallrefupdates, are handled
separately. And I realized halfway through my reply that was probably
what you meant.

I agree those are user-facing config options that should generally
respect includes in the normal way. I thinks are a bit funny there,
though. See below.

> I actually tried lazy-loading, but I found it to be quite painful
> overall, as the above setting isn't the only one we use. The reftable
> backend for example has a bunch of additional settings that it reads.
> 
> We could of course start lazy-loading all of these. But that may not
> work for future backends that really _need_ to parse some configuration
> at initiation time.

Yes, obviously there's some true chicken-and-egg issues if there are
config keys that are needed to initialize the backend. But I think there
are many that are not needed immediately (e.g., because they relate only
to writes, not reads) but still block loading.

For example, try this:

  git init
  git config core.logallrefupdates false
  git config includeIf.onbranch:main.path alt-config
  git config -f .git/alt-config core.logallrefupdates true
  git commit --allow-empty -qm foo

  echo "git-config => $(git config core.logallrefupdates)"
  echo "reflog => $(git reflog show)"

git-config will report the value as true, but git-commit will not
respect it. But this used to work! Back when onbranch was added, we'd
create the reflog. Bisecting turns up eafb126456 (environment: stop
storing "core.logAllRefUpdates" globally, 2024-09-12), which makes
sense. That commit pushed the config read down into the ref
initialization function, which created the chicken-and-egg.

Now the config shown above is a bit silly, and I don't expect anybody to
do it in real life. But what worries me is two-fold:

  1. There are some magic variables that just won't work with onbranch
     includes, but the user doesn't necessarily know what they are.

  2. We try to cache the results of config reads. Is it possible for an
     "early" request like this to cache a state that skipped the
     onbranch include, and then we use that state to look up other
     unrelated variables? Or could we see a partially completed state in
     the cache when we lookup a ref variable?

     I'm not sure. The actual backend lookups use the uncached
     repo_config() interface (and in your series here, explicitly
     disables the use of refs during that read). But the
     core.logallrefupdates lookup uses the cached version, and I think
     there are others (some of which happen deep under the hood
     through library calls, like calc_shared_perm()).

I tried to construct a few cases that might tickle this behavior, but
couldn't come up with one. But I have a nagging feeling that we are
mostly getting lucky on some of the ordering, and a seemingly unrelated
change could have bad effects.

Sorry, I know that's kind of vague and hand-wavy.

I'm not sure I have a specific recommendation for a direction. It just
feels like we're piling up hacks to avoid infinite recursion without a
clear model of what config is read when. I guess if I could suggest
anything, it would be that ref backends initialize themselves to do
reads while loading as little config as possible, and then perhaps load
additional config through the non-caching repo_config() path.

-Peff