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 173761e21b (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 85fe0e800c (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 <ronan@xxxxxx> Signed-off-by: Patrick Steinhardt <ps@xxxxxx> --- config.c | 15 +++++++++------ t/t1305-config-include.sh | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/config.c b/config.c index 1266eab0860..a11bb85da30 100644 --- a/config.c +++ b/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); diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index ad08db72308..517d6c86937 100755 --- a/t/t1305-config-include.sh +++ b/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 && -- 2.46.0.551.gc5ee8f2d1c.dirty
