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

[Patrick Steinhardt <ps@pks.im>]

On Sun, Jun 21, 2026 at 05:12:11PM -0400, Jeff King wrote:
> 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:
> > 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.

Yeah, I thought more about this issue over the weekend and kind of got
to the same conclusion. Sure, the current version where we explicitly
handle the exclusion of "onbranch" conditions is at least less awkward.
But I have to agree that it's still not the right fix, as it doesn't
really solve the root issue.

Taking a step back: all the values that we currently parse are only
relevant when writing new refs. So in theory it should be possible to
lazy-load all of them on the first write. This should be rather easy to
do for the "files" backend. But for the "reftable" backend this will
result in a large refactoring because we require the configuration when
constructing the reftable stack.

That's kind of misdesigned though: the reftable stack shouldn't really
care about write options when being constructed. What it needs to know
about is the expected hash ID, and any optional stuff like the onreload
callback. The write options should then be passed by the caller when we
actually perform a write.

I'll iterate a bit on this idea and will see where I get. I really
shouldn't have opened this can of worms.

Thanks!

Patrick