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

[Jeff King <peff@peff.net>]

On Mon, Jun 15, 2026 at 03:56:53PM +0200, Patrick Steinhardt wrote:

> When we have an "onbranch" condition we need to ask the reference
> database whether HEAD currently points at the configured branch. This
> unfortunately creates a chicken-and-egg problem:
> 
>   - The reference database needs to read the configuration so that it
>     can configure itself.
> 
>   - The configuration needs to construct a reference database to fully
>     parse all of its conditionals.
> 
> The way we handle this is by simply excluding "onbranch" conditionals
> when we haven't yet configured the reference database.

My gut feeling upon reading this is that some part of the config reading
is being done wrong to create this chicken-and-egg situation.

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.

In other words, there should be two passes over the config file: one to
load basic repository information (and not respect includes), and one to
actually load what we think of as user-visible config[1].

And it seems to work. If I do this:

diff --git a/config.c b/config.c
index 45144f73c5..343af2cf9a 100644
--- a/config.c
+++ b/config.c
@@ -303,7 +303,7 @@ static int include_by_branch(struct config_include_data *data,
 	const char *refname, *shortname;
 
 	if (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
-		return 0;
+		BUG("chicken and egg");
 
 	refname = refs_resolve_ref_unsafe(get_main_ref_store(data->repo),
 					  "HEAD", 0, NULL, &flags);

and then:

  git config includeIf.onbranch:main.path alt-config
  git config -f .git/alt-config foo.bar baz
  git config foo.bar

then we correctly read the value without triggering this code path.

Looking back at the last commit that touched include_by_branch(), the
problem does not appear to be about a chicken-and-egg at all, though. It
is about reading config with includes when there is _no_ repository at
all. I.e., this:

  git config -f main-config includeIf.onbranch:main.path alt-config
  git config -f  alt-config foo.bar baz
  GIT_DIR=/does/not/exist git.compile config --include -f main-config foo.bar

will trigger that BUG() marker, and quietly returning "no match" (like
the current code does) is the right thing.

Looking below...

> The consequence is that we have recursion:
> 
>   1. We call `get_main_ref_store()`.
> 
>   2. We don't yet have a reference store, so we call `ref_store_init()`.
> 
>   3. We parse the configuration required for the reference store.
> 
>   4. We eventually end up in `include_by_branch()`.
> 
>   5. We have already configured the reference storage format, so we end
>      up calling `get_main_ref_store()` again.

Ah, the culprit seems to be ref_store_init() calling into the regular
config parser via repo_settings_get_log_all_ref_updates(). But that
feels weird to me. Either:

  1. It is application config that should not be something we need to
     load in order to initialize the backend. We could lazy-load it
     later, or rely on higher level code to set the option.

  2. It is crucial to the ref backend functioning, in which case we
     ought to be reading it alongside core.repositoryFormatVersion, etc.

-Peff