config: fix evaluating "onbranch" with nonexistent git dir

The `include_by_branch()` function is responsible for evaluating whether
or not a specific include should be pulled in based on the currently
checked out branch. Naturally, his condition can only be evaluated when
we have a properly initialized repository with a ref store in the first
place. This is why the function guards against the case when either
`data->repo` or `data->repo->gitdir` are `NULL` pointers.

But the second check is insufficient: the `gitdir` may be set even
though the repository has not been initialized. Quoting "setup.c":

  NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some
  code paths so we also need to explicitly setup the environment if the
  user has set GIT_DIR.  It may be beneficial to disallow bogus GIT_DIR
  values at some point in the future.

So when either the GIT_DIR environment variable or the `--git-dir`
global option are set by the user then `the_repository` may end up with
an initialized `gitdir` variable. And this happens even when the dir is
invalid, like for example when it doesn't exist. It follows that only
checking for whether or not `gitdir` is `NULL` is not sufficient for us
to determine whether the repository has been properly initialized.

This issue can lead to us triggering a BUG: when using a config with an
"includeIf.onbranch:" condition outside of a repository while using the
`--git-dir` option pointing to an invalid Git directory we may end up
trying to evaluate the condition even though the ref storage format has
not been set up.

This bisects to 173761e (setup: start tracking ref storage format,
2023-12-29), but that commit really only starts to surface the issue
that has already existed beforehand. The code to check for `gitdir` was
introduced via 85fe0e8 (config: work around bug with
includeif:onbranch and early config, 2019-07-31), which tried to fix
similar issues when we didn't yet have a repository set up. But the fix
was incomplete as it missed the described scenario.

As the quoted comment mentions, we'd ideally refactor the code to not
set up `gitdir` with an invalid value in the first place, but that may
be a bigger undertaking. Instead, refactor the code to use the ref
storage format as an indicator of whether or not the ref store has been
set up to fix the bug.

Reported-by: Ronan Pigott <[email protected]>
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pks-t

config.c

@@ -306,13 +306,16 @@ static int include_by_branch(struct config_include_data *data,
 	int flags;
 	int ret;
 	struct strbuf pattern = STRBUF_INIT;
-	const char *refname = (!data->repo || !data->repo->gitdir) ?
-		NULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo),
-					       "HEAD", 0, NULL, &flags);
-	const char *shortname;
+	const char *refname, *shortname;
 
-	if (!refname || !(flags & REF_ISSYMREF)	||
-			!skip_prefix(refname, "refs/heads/", &shortname))
+	if (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
+		return 0;
+
+	refname = refs_resolve_ref_unsafe(get_main_ref_store(data->repo),
+					  "HEAD", 0, NULL, &flags);
+	if (!refname ||
+	    !(flags & REF_ISSYMREF) ||
+	    !skip_prefix(refname, "refs/heads/", &shortname))
 		return 0;
 
 	strbuf_add(&pattern, cond, cond_len);

t/t1305-config-include.sh

@@ -389,7 +389,7 @@ test_expect_success 'onbranch without repository' '
 	test_must_fail nongit git config get foo.bar
 '
 
-test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' '
+test_expect_success 'onbranch without repository but explicit nonexistent Git directory' '
 	test_when_finished "rm -f .gitconfig config.inc" &&
 	git config set -f .gitconfig "includeIf.onbranch:**.path" config.inc &&
 	git config set -f config.inc foo.bar baz &&