BUG: refs.c:1933: reference backend is unknown

BUG: refs.c:1933: reference backend is unknown

[Ronan Pigott]

Hi git,

The following is an abort in git:

  $ git --version
  git version 2.46.1
  $ git --git-dir=notexist archive  
  BUG: refs.c:1933: reference backend is unknown
  zsh: IOT instruction (core dumped)  git --git-dir=notexist archive

Cheers,
Ronan

Re: BUG: refs.c:1933: reference backend is unknown

[shejialuo]

On Fri, Sep 20, 2024 at 05:37:40PM +0000, Ronan Pigott wrote:

[snip]

> The following is an abort in git:
> 
>   $ git --version
>   git version 2.46.1
>   $ git --git-dir=notexist archive  
>   BUG: refs.c:1933: reference backend is unknown
>   zsh: IOT instruction (core dumped)  git --git-dir=notexist archive
> 

The "git-archive(1)" command must accept at least one parameter as
"archive.c::parse_archive_args" shows:

    static int parse_archive_args(...)
    {
        ...
        if (argc < 1)
            usage_with_options(archive_usage, opts);
    }

So, it will print the usage and exit the program. I guess you omit some
things for this bug report. Could you please give us more information to
let us dive into this?

Thanks,
Jialuo

Re: BUG: refs.c:1933: reference backend is unknown

[Ronan Pigott]

September 21, 2024 at 9:09 AM, "shejialuo" <shejialuo@gmail.com> wrote:

> The "git-archive(1)" command must accept at least one parameter as
> 
> "archive.c::parse_archive_args" shows:
> 
>  static int parse_archive_args(...)
> 
>  {
> 
>  ...
> 
>  if (argc < 1)
> 
>  usage_with_options(archive_usage, opts);
> 
>  }
> 
> So, it will print the usage and exit the program. I guess you omit some
> 
> things for this bug report. Could you please give us more information to
> 
> let us dive into this?

The abort is reproducible exactly as written for me, but upon further
examination it may depend on my config. Here is a minimal reproducer:

  $ GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null git --git-dir=notexist -c 'includeif.onbranch:test/**.path=/dev/null' archive

> bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x00007ffff7d78463 in __pthread_kill_internal (threadid=<optimized out>, signo=6) at pthread_kill.c:78
#2  0x00007ffff7d1f120 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff7d064c3 in __GI_abort () at abort.c:79
#4  0x000055555585385a in BUG_vfl (file=<optimized out>, line=<optimized out>, fmt=0x555555905baf "reference backend is unknown", params=0x7fffffffd120) at /usr/src/debug/git/git-2.46.1/usage.c:317
#5  BUG_fl (file=<optimized out>, line=<optimized out>, fmt=0x555555905baf "reference backend is unknown") at /usr/src/debug/git/git-2.46.1/usage.c:334
#6  0x00005555557ca9d2 in ref_store_init (repo=<optimized out>, format=<optimized out>, gitdir=<optimized out>, flags=<optimized out>) at /usr/src/debug/git/git-2.46.1/refs.c:1933
#7  ref_store_init (repo=0x5555559759a0 <the_repo.lto_priv>, format=<optimized out>, gitdir=<optimized out>, flags=15) at /usr/src/debug/git/git-2.46.1/refs.c:1923
#8  get_main_ref_store (r=0x5555559759a0 <the_repo.lto_priv>) at /usr/src/debug/git/git-2.46.1/refs.c:1953
#9  0x00005555556dcdf6 in include_by_branch (cond=0x55555599f503 "test/**.path", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:309
#10 include_condition_is_true (kvi=<optimized out>, inc=0x7fffffffd460, cond=0x55555599f503 "test/**.path", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:409
#11 git_config_include (var=var@entry=0x55555599f4f0 "includeif.onbranch:test/**.path", value=value@entry=0x55555599fa62 "/dev/null", ctx=ctx@entry=0x7fffffffd368, data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:439
#12 0x00005555556d8753 in config_parse_pair (key=<optimized out>, value=0x55555599fa62 "/dev/null", kvi=kvi@entry=0x7fffffffd4a0, fn=fn@entry=0x5555556dcad0 <git_config_include>, data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:617
#13 0x00005555556dc437 in parse_config_env_list (env=0x55555599fa40 "includeif.onbranch:test/**.path", kvi=0x7fffffffd4a0, fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:700
#14 git_config_from_parameters (fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:773
#15 do_git_config_sequence (opts=0x7fffffffd4b0, repo=0x0, fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:2131
#16 config_with_options (fn=0x5555556dcad0 <git_config_include>, fn@entry=0x555555790940 <pager_command_config>, data=0x7fffffffd460, data@entry=0x7fffffffd7a0, config_source=config_source@entry=0x0, repo=repo@entry=0x0, opts=opts@entry=0x7fffffffd660) at /usr/src/debug/git/git-2.46.1/config.c:2174
#17 0x00005555556dd110 in read_early_config (cb=0x555555790940 <pager_command_config>, data=0x7fffffffd7a0) at /usr/src/debug/git/git-2.46.1/config.c:2229
#18 0x000055555555ea58 in check_pager_config (cmd=<optimized out>) at /usr/src/debug/git/git-2.46.1/pager.c:261
#19 run_builtin (p=0x555555966a00 <commands.lto_priv+96>, argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:461
#20 handle_builtin (argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:732
#21 0x000055555555eaf9 in run_argv (argcp=0x7fffffffdb0c, argv=0x7fffffffdb30) at /usr/src/debug/git/git-2.46.1/git.c:796
#22 0x00005555555597eb in cmd_main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.46.1/git.c:931
#23 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.46.1/common-main.c:64

Cheers,

Ronan

Re: BUG: refs.c:1933: reference backend is unknown

[René Scharfe]

Am 21.09.24 um 20:06 schrieb Ronan Pigott:
>
> The abort is reproducible exactly as written for me, but upon further
> examination it may depend on my config. Here is a minimal reproducer:
>
>   $ GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null git --git-dir=notexist -c 'includeif.onbranch:test/**.path=/dev/null' archive
>
>> bt
> #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
> #1  0x00007ffff7d78463 in __pthread_kill_internal (threadid=<optimized out>, signo=6) at pthread_kill.c:78
> #2  0x00007ffff7d1f120 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
> #3  0x00007ffff7d064c3 in __GI_abort () at abort.c:79
> #4  0x000055555585385a in BUG_vfl (file=<optimized out>, line=<optimized out>, fmt=0x555555905baf "reference backend is unknown", params=0x7fffffffd120) at /usr/src/debug/git/git-2.46.1/usage.c:317
> #5  BUG_fl (file=<optimized out>, line=<optimized out>, fmt=0x555555905baf "reference backend is unknown") at /usr/src/debug/git/git-2.46.1/usage.c:334
> #6  0x00005555557ca9d2 in ref_store_init (repo=<optimized out>, format=<optimized out>, gitdir=<optimized out>, flags=<optimized out>) at /usr/src/debug/git/git-2.46.1/refs.c:1933
> #7  ref_store_init (repo=0x5555559759a0 <the_repo.lto_priv>, format=<optimized out>, gitdir=<optimized out>, flags=15) at /usr/src/debug/git/git-2.46.1/refs.c:1923
> #8  get_main_ref_store (r=0x5555559759a0 <the_repo.lto_priv>) at /usr/src/debug/git/git-2.46.1/refs.c:1953
> #9  0x00005555556dcdf6 in include_by_branch (cond=0x55555599f503 "test/**.path", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:309
> #10 include_condition_is_true (kvi=<optimized out>, inc=0x7fffffffd460, cond=0x55555599f503 "test/**.path", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:409
> #11 git_config_include (var=var@entry=0x55555599f4f0 "includeif.onbranch:test/**.path", value=value@entry=0x55555599fa62 "/dev/null", ctx=ctx@entry=0x7fffffffd368, data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:439
> #12 0x00005555556d8753 in config_parse_pair (key=<optimized out>, value=0x55555599fa62 "/dev/null", kvi=kvi@entry=0x7fffffffd4a0, fn=fn@entry=0x5555556dcad0 <git_config_include>, data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:617
> #13 0x00005555556dc437 in parse_config_env_list (env=0x55555599fa40 "includeif.onbranch:test/**.path", kvi=0x7fffffffd4a0, fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:700
> #14 git_config_from_parameters (fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:773
> #15 do_git_config_sequence (opts=0x7fffffffd4b0, repo=0x0, fn=0x5555556dcad0 <git_config_include>, data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:2131
> #16 config_with_options (fn=0x5555556dcad0 <git_config_include>, fn@entry=0x555555790940 <pager_command_config>, data=0x7fffffffd460, data@entry=0x7fffffffd7a0, config_source=config_source@entry=0x0, repo=repo@entry=0x0, opts=opts@entry=0x7fffffffd660) at /usr/src/debug/git/git-2.46.1/config.c:2174
> #17 0x00005555556dd110 in read_early_config (cb=0x555555790940 <pager_command_config>, data=0x7fffffffd7a0) at /usr/src/debug/git/git-2.46.1/config.c:2229
> #18 0x000055555555ea58 in check_pager_config (cmd=<optimized out>) at /usr/src/debug/git/git-2.46.1/pager.c:261
> #19 run_builtin (p=0x555555966a00 <commands.lto_priv+96>, argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:461
> #20 handle_builtin (argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:732
> #21 0x000055555555eaf9 in run_argv (argcp=0x7fffffffdb0c, argv=0x7fffffffdb30) at /usr/src/debug/git/git-2.46.1/git.c:796
> #22 0x00005555555597eb in cmd_main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.46.1/git.c:931
> #23 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.46.1/common-main.c:64

Thanks, that helps.  The BUG occurs during setup, before any
command-specific code is executed.  Affects other commands with the
option RUN_SETUP_GENTLY as well, e.g. git grep.

Bisects to 173761e21b (setup: start tracking ref storage format,
2023-12-29, copying Patrick.

I wonder if the BUG at refs.c:1933 should just be turned into a die().
Or should ref_store_init() return NULL (or some other value indicating
an error) and all callers need to be changed to deal with that?

Simply checking whether --git-dir points to an actual repo earlier is
tempting, but would leave the case of it disappearing after the check
unhandled.

René

Re: BUG: refs.c:1933: reference backend is unknown

[shejialuo]

On Sat, Sep 21, 2024 at 11:22:04PM +0200, René Scharfe wrote:

[snip]

> I wonder if the BUG at refs.c:1933 should just be turned into a die().
> Or should ref_store_init() return NULL (or some other value indicating
> an error) and all callers need to be changed to deal with that?
> 
> Simply checking whether --git-dir points to an actual repo earlier is
> tempting, but would leave the case of it disappearing after the check
> unhandled.

From my perspective, it is not suitable to change the "refs.c". I guess
the reason why Patrick uses the "BUG()" is that it's an internal
implementation and we should tell the developer something is wrong.

In this case, because we use "include.onbranch" configuration, the
"config.c::include_by_branch" will use "refsc::get_main_ref_store" to
get the refname.

However, the repo does not exist, so we cannot find any ref backend. The
main problem is that we never check whether "the_repository->gitdir" is
a git directory in "config.c::include_by_branch". So, we do not need to
check the value of the option "--git-dir".

Thanks,
Jialuo

Re: BUG: refs.c:1933: reference backend is unknown

[shejialuo]

On Sun, Sep 22, 2024 at 02:51:34PM +0800, shejialuo wrote:

[snip]

> However, the repo does not exist, so we cannot find any ref backend. The
> main problem is that we never check whether "the_repository->gitdir" is
> a git directory in "config.c::include_by_branch". So, we do not need to
> check the value of the option "--git-dir".
> 

I have just scanned the recent code here, because Patrick has already
replaced the "the_repository", there is no problem here when set up the
configuration in the TOT.

The code will first check whether the "data->repo" is NULL.

However, there is still a problem after setup:

#5  0x0000555555780442 in ref_store_init (repo=0x555555948b00 <the_repo>,
    format=<optimized out>, gitdir=<optimized out>, flags=15) at refs.c:1938
#6  ref_store_init (repo=0x555555948b00 <the_repo>, format=<optimized out>,
    gitdir=<optimized out>, flags=15) at refs.c:1928
#7  get_main_ref_store (r=0x555555948b00 <the_repo>) at refs.c:1958
#8  0x00005555556a332d in include_by_branch (data=0x7fffffffd400,
    cond=0x555555953d13 "main.path", cond_len=4) at config.c:308
#9  include_condition_is_true (kvi=0x7fffffffd320, inc=0x7fffffffd400,
    cond=0x555555953d13 "main.path", cond_len=4) at config.c:408
#10 git_config_include (var=var@entry=0x555555953d00 "includeif.onbranch:main.path",
    value=value@entry=0x55555595382f "notexist", ctx=ctx@entry=0x7fffffffd280,
    data=data@entry=0x7fffffffd400) at config.c:438
#11 0x000055555569d3a3 in config_parse_pair (key=<optimized out>,
    value=0x55555595382f "notexist", kvi=kvi@entry=0x7fffffffd320,
    fn=fn@entry=0x5555556a3060 <git_config_include>, data=data@entry=0x7fffffffd400)
    at config.c:616
#12 0x000055555569dafd in parse_config_env_list (
    env=0x555555953810 "includeIf.onbranch:main.path", kvi=0x7fffffffd320,
    fn=0x5555556a3060 <git_config_include>, data=0x7fffffffd400) at config.c:699
#13 git_config_from_parameters (fn=fn@entry=0x5555556a3060 <git_config_include>,
    data=data@entry=0x7fffffffd400) at config.c:772
#14 0x000055555569e383 in do_git_config_sequence (opts=0x7fffffffd550,
    repo=repo@entry=0x555555948b00 <the_repo>,
    fn=fn@entry=0x5555556a3060 <git_config_include>, data=data@entry=0x7fffffffd400)
    at config.c:2131
#15 0x000055555569ed49 in config_with_options (fn=0x5555556a3060 <git_config_include>,
    data=0x7fffffffd400, config_source=<optimized out>, repo=0x555555948b00 <the_repo>,
    opts=<optimized out>) at config.c:2174
#16 0x000055555569eec7 in repo_read_config (repo=0x555555948b00 <the_repo>) at config.c:2544
#17 git_config_check_init (repo=repo@entry=0x555555948b00 <the_repo>) at config.c:2564
#18 0x00005555556a0272 in repo_config (repo=0x555555948b00 <the_repo>,
    fn=fn@entry=0x5555558218a0 <git_tar_config>, data=data@entry=0x0) at config.c:2576
#19 0x0000555555822651 in git_config (fn=0x5555558218a0 <git_tar_config>, data=0x0)
    at /home/shejialuo/Projects/git/config.h:704
#20 init_tar_archiver () at archive-tar.c:544
#21 0x00005555556700c9 in init_archivers () at archive.c:46
#22 0x000055555557f8b8 in cmd_archive (argc=2, argv=0x7fffffffdcc8, prefix=0x0)
    at builtin/archive.c:98
#23 0x00005555555753c9 in run_builtin (p=0x555555916760 <commands+96>, argc=2,
    argv=0x7fffffffdcc8) at git.c:483
#24 handle_builtin (argc=2, argv=0x7fffffffdcc8) at git.c:739
#25 0x0000555555576335 in run_argv (argcp=argcp@entry=0x7ffff

It still uses the old "git_config", so we still have a problem when we
explicitly specify the "--git-dir". However, once Patrick replaces all
the "the_repository", this bug should never exist. So, I won't dive into
this.

Thanks,
Jialuo

[PATCH 0/2] config: fix evaluating "onbranch" with nonexistent git dir

[Patrick Steinhardt]

Hi,

this small patch series fixes evaluating "includeIf.onbranch" conditions
when running outside of a repository with an invalid gitdir.

Thanks!

Patrick

Patrick Steinhardt (2):
  t1305: exercise edge cases of "onbranch" includes
  config: fix evaluating "onbranch" with nonexistent git dir

 config.c                  | 15 +++++++++------
 t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 6 deletions(-)

-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 1/2] t1305: exercise edge cases of "onbranch" includes

[Patrick Steinhardt]

Add a couple more tests for "onbranch" includes for several edge cases.
All tests except for the last one pass, so for the most part this change
really only aims to nail down behaviour of include conditionals further.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 5cde79ef8c4..ad08db72308 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -357,4 +357,44 @@ test_expect_success 'include cycles are detected' '
 	grep "exceeded maximum include depth" stderr
 '
 
+test_expect_success 'onbranch with unborn branch' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		git config set includeIf.onbranch:"*".path config.inc &&
+		git config set -f .git/config.inc foo.bar baz &&
+		git config get foo.bar
+	)
+'
+
+test_expect_success 'onbranch with detached HEAD' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		git config set "includeIf.onbranch:*.path" config.inc &&
+		git config set -f .git/config.inc foo.bar baz &&
+		test_commit initial &&
+		git switch --detach HEAD &&
+		test_must_fail git config get foo.bar
+	)
+'
+
+test_expect_success 'onbranch without repository' '
+	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 &&
+	git config get foo.bar &&
+	test_must_fail nongit git config get foo.bar
+'
+
+test_expect_failure '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 &&
+	git config get foo.bar &&
+	test_must_fail nongit git --git-dir=nonexistent config get foo.bar
+'
+
 test_done
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 2/2] config: fix evaluating "onbranch" with nonexistent git dir

[Patrick Steinhardt]

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@rjp.ie>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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

Re: [PATCH 2/2] config: fix evaluating "onbranch" with nonexistent git dir

[shejialuo]

On Tue, Sep 24, 2024 at 12:05:46PM +0200, Patrick Steinhardt wrote:
> 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.
> 

When I dive into this bug report, I feel so wired about this behavior. I
don't mind whether the code sets "gitdir" field. This is not important.
In "setup.c::setup_git_directory_gently", it will set the
"the_repository->gitdir" by checking the environment variable
"GIT_DIR_ENVIRONMENT". And by using `--git-dir` option, the code will
set this environment variable, so these two ways will set the "gitdir"
field in the global variable "the_repository".

We actually check the validation of "--gir-dir" option before we set
this value. Let me give you an example here:

  $ git --git-dir=notexist -c includeIf.onbranch:main.path=any fsck
  fatal: not a git repository: 'notexist'

I am curious here. And I notice the following difference in
"setup.c::setup_explicit_git_dir" function:

    if (!is_git_directory(gitdirenv)) {
        if (nongit_ok) {
            *nongit_ok = 1;
            ...
            return NULL;
        }
        die(_("not a git repository: '%s'"), gitdirenv);
    }

Apparently, the above example will execute the "die". For
"git-archive(1)", it will simply return NULL. This is because we allow
some commands to run outside of the git repo. And we distinguish them by
using the ".option" filed:

    { "apply", cmd_apply, RUN_SETUP_GENTLY },
    { "fsck", cmd_fsck, RUN_SETUP },

As we can see, the code will check whether the "gitdir" (although it is
not set into "the_repository" structure yet) is a valid git repository.
We already have this information.

In f7d61c4135 (config: don't depend on `the_repository` with branch
conditions, 2024-08-13). "config.c::include_by_branch" drops the global
variable "the_repository". This solves the problem reported by Ronan.
Because it happens in the set up process, the "data->repo" will be NULL.

    config_with_options(cb, data, NULL, NULL, &opts);

    int config_with_options(config_fn_t fn, void *data,
                            const struct git_config_source *config_source,
                            struct repository *repo,
                            const struct config_options *opts)
    {
        struct config_include_data inc = CONFIG_INCLUDE_INIT;

        ...

        inc.repo = repo;
    }

But the problem still exists for "git-archive(1)", in "cmd_archive"
function, we will initialize the configurations by using "repo_config".
But we are not inside the repo.

I wonder how we use the global variable "the_repository". I think the
main problem here is that we use "the_repository" structure outside of
the repo where we have already broken the semantics of the
"the_repository" variable.

> 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.
> 

Yes, exactly. Because before 173761e21b, the code will always find the
files backend, it does care about whether we are in the git repo or not.

    unsigned int format = REF_STORAGE_FORMAT_FILES;
    const struct ref_storage_be *be = find_ref_storage_backend(format);

So, it won't complain.

> 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.
> 

Should we? From my above comments, it does no matter whether we set the
"gitdir" in the "the_repository". Because we already check this and we
have this information. I think the main problem is that we use
"the_repository" badly.

We could run git commands inside the repo or outside the repo. If we run
git commands outside the repo, should we use the "the_repository"
variable? I guess we should not.

Because "git_config", "repo_config" and so on use the global variable
"the_repository", so we will encounter the trouble. But if we could use
something like "data->repo". We will make everything OK:

1. When running commands inside the git repo, we set "data->repo" to be
"the_repository".
2. When ruing commands outside the git repo, we set "data->repo" to be
NULL.

> Reported-by: Ronan Pigott <ronan@rjp.ie>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  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

Re: [PATCH 0/2] config: fix evaluating "onbranch" with nonexistent git dir

[shejialuo]

On Tue, Sep 24, 2024 at 12:05:37PM +0200, Patrick Steinhardt wrote:
> Hi,
> 
> this small patch series fixes evaluating "includeIf.onbranch" conditions
> when running outside of a repository with an invalid gitdir.
> 
> Thanks!
> 
> Patrick
> 
> Patrick Steinhardt (2):
>   t1305: exercise edge cases of "onbranch" includes
>   config: fix evaluating "onbranch" with nonexistent git dir
> 
>  config.c                  | 15 +++++++++------
>  t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> -- 
> 2.46.0.551.gc5ee8f2d1c.dirty
> 

Although I comment a lot in [PATCH 2/2], I think this series is good
enough. The reason why I comment on [PATCH 2/2] is I think we should
enhance this behavior in the future.

Thanks,
Jialuo

Re: [PATCH 1/2] t1305: exercise edge cases of "onbranch" includes

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +test_expect_success 'onbranch without repository' '
> +	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 &&

This assumes that the $(pwd) is the $HOME; so .gitconfig is the
per-user configuration that ought to apply everywhere; since
includeIf.<condition>.path that is relative is relative to the
including file, config.inc would be in cluded when the condition
holds in $HOME/.gitconfig.  OK.

> +	git config get foo.bar &&

This assumes that the $(pwd) that is $HOME is a valid repository,
and checks if includeIf.onbranch works from within a repository.
OK.

> +	test_must_fail nongit git config get foo.bar
> +'

> +test_expect_failure '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 &&

The same set-up.

> +	git config get foo.bar &&
> +	test_must_fail nongit git --git-dir=nonexistent config get foo.bar

It has to work when $(pwd) is outside a repository, but is "nongit"
strictly necessary?  IOW, even when we _could_ discover the top
level of a git-controlled working tree, wouldn't presence of --git-dir
that points at elsewhere make $(pwd) and the repository there irrelevant
to the operation?

I am not suggesting to just drop "nongit" from this test.  I am
wondering if this is better split into two tests, with and without
"nongit" to test different situations.

> +'
> +
>  test_done

Other than that, looks like good additions to the test coverage.

Thanks.

Re: [PATCH 2/2] config: fix evaluating "onbranch" with nonexistent git dir

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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@rjp.ie>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ...
> -	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;

OK.  A very direct check to see what we really care about.