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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jun 17, 2026 at 01:41:40PM -0500, Justin Tobler wrote:
> On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> [snip]
> > diff --git a/refs.c b/refs.c
> > index d3caa9a633..e69b9b8ac8 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -2351,15 +2351,31 @@ void ref_store_release(struct ref_store *ref_store)
> >  
> >  struct ref_store *get_main_ref_store(struct repository *r)
> >  {
> > +	enum ref_storage_format format;
> > +
> >  	if (r->refs_private)
> >  		return r->refs_private;
> >  
> >  	if (!r->gitdir)
> >  		BUG("attempting to get main_ref_store outside of repository");
> >  
> > -	r->refs_private = ref_store_init(r, r->ref_storage_format,
> > -					 r->gitdir, REF_STORE_ALL_CAPS);
> > +	/*
> > +	 * When constructing the reference backend we'll end up reading the Git
> > +	 * configuration. This means we'll also try to evaluate "onbranch"
> > +	 * conditions.
> > +	 *
> > +	 * We cannot read branches when constructing the refdb, so it is not
> > +	 * possible to evaluate those conditions in the first place. To gate
> > +	 * their evaluation we check whether or not the reference storage
> > +	 * format has been configured -- we thus have to temporarily set it to
> > +	 * UNKNOWN here so that we don't end up recursing.
> > +	 */
> > +	format = r->ref_storage_format;
> > +	r->ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
> 
> Is this really the best signal to indicate that a repository ref store
> has not been initialized? Temporarily setting the storage format to
> REF_STORAGE_FORMAT_UNKNOWN feels rather awkward and suggests to me that
> `include_by_branch()` probably shouldn't be using it to begin with if
> its not reliable.

True, but we don't really have a better signal to the best of my
knowledge. Ideally, we'd be able to use the existence `r->refs_private`
as signal. But that doesn't really work as the reference database is
lazily constructed, and the recursion happens in the exact function that
would construct it in the first place. And there indeed are cases where
reading the configuration is the first caller of `get_main_ref_store()`.

My first internal iteration tried to make this non-lazily constructed so
that we can use it as a proper signal. But that led to a bunch of
problems where we now parsed configuration way earlier than we currently
do, and that in turn led to all kinds of errors. I was able to fix all
of those errors except one: we expect `git config set` to work in a
misconfigured repository so that the user can fix the misconfig without
having to manually edit the Git configuration files. But when
constructing the refdb eagerly we will die early in such cases.

We could again work around that issue, but that unfortunately evolved
into a proper mess that I eventually discarded as unworkable. I think
this is an inherent design flaw: constructing the refdb requires us to
be able to parse the configuration, but constructing the configuration
may require us to construct the refdb. So this awkwardness is built into
Git's design, unfortunately.

So I'd really love to have a better signal, as I fully agree that the
above workaround is nothing more but a hack. But I'm just not sure what
that signal would be. And this version here does exactly what we want:
we honor "onbranch" conditionals in all cases, except when constructing
the main reference store. Even if it's ugly.

Patrick