{"thread":{"id":"62157","subject":"BUG: refs.c:1933: reference backend is unknown","startedAt":"2024-09-20T17:37:43Z","lastAt":"2024-09-24T16:17:53Z","messageCount":13,"participants":["Ronan Pigott","shejialuo","René Scharfe","Patrick Steinhardt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"503148","messageId":"1b9fb3f3fde62594b9ac999ffb69e6c4fb9f6fd6@rjp.ie","threadId":"62157","inReplyTo":null,"subject":"BUG: refs.c:1933: reference backend is unknown","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2024-09-20T17:37:40Z","receivedAt":"2024-09-20T17:37:43Z","isPatch":false,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"Hi git,\n\nThe following is an abort in git:\n\n $ git --version\n git version 2.46.1\n $ git --git-dir=notexist archive \n BUG: refs.c:1933: reference backend is unknown\n zsh: IOT instruction (core dumped) git --git-dir=notexist archive\n\nCheers,\nRonan\n"},{"id":"503196","messageId":"Zu7vpPs8fcqlMlNK@ArchLinux","threadId":"62157","inReplyTo":"1b9fb3f3fde62594b9ac999ffb69e6c4fb9f6fd6@rjp.ie","subject":"Re: BUG: refs.c:1933: reference backend is unknown","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-21T16:09:08Z","receivedAt":"2024-09-21T16:07:58Z","isPatch":false,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Sep 20, 2024 at 05:37:40PM +0000, Ronan Pigott wrote:\n\n[snip]\n\n> The following is an abort in git:\n> \n> $ git --version\n> git version 2.46.1\n> $ git --git-dir=notexist archive \n> BUG: refs.c:1933: reference backend is unknown\n> zsh: IOT instruction (core dumped) git --git-dir=notexist archive\n> \n\nThe \"git-archive(1)\" command must accept at least one parameter as\n\"archive.c::parse_archive_args\" shows:\n\n static int parse_archive_args(...)\n {\n ...\n if (argc < 1)\n usage_with_options(archive_usage, opts);\n }\n\nSo, it will print the usage and exit the program. I guess you omit some\nthings for this bug report. Could you please give us more information to\nlet us dive into this?\n\nThanks,\nJialuo\n"},{"id":"503199","messageId":"9fdb9561bb0ad85e55ca3253cc4db9b098641e30@rjp.ie","threadId":"62157","inReplyTo":"Zu7vpPs8fcqlMlNK@ArchLinux","subject":"Re: BUG: refs.c:1933: reference backend is unknown","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2024-09-21T18:06:09Z","receivedAt":"2024-09-21T18:06:12Z","isPatch":false,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"September 21, 2024 at 9:09 AM, \"shejialuo\" wrote:\n\n> The \"git-archive(1)\" command must accept at least one parameter as\n> \n> \"archive.c::parse_archive_args\" shows:\n> \n> static int parse_archive_args(...)\n> \n> {\n> \n> ...\n> \n> if (argc < 1)\n> \n> usage_with_options(archive_usage, opts);\n> \n> }\n> \n> So, it will print the usage and exit the program. I guess you omit some\n> \n> things for this bug report. Could you please give us more information to\n> \n> let us dive into this?\n\nThe abort is reproducible exactly as written for me, but upon further\nexamination it may depend on my config. Here is a minimal reproducer:\n\n $ GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null git --git-dir=notexist -c 'includeif.onbranch:test/**.path=/dev/null' archive\n\n> bt\n#0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44\n#1 0x00007ffff7d78463 in __pthread_kill_internal (threadid=, signo=6) at pthread_kill.c:78\n#2 0x00007ffff7d1f120 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26\n#3 0x00007ffff7d064c3 in __GI_abort () at abort.c:79\n#4 0x000055555585385a in BUG_vfl (file=, line=, fmt=0x555555905baf \"reference backend is unknown\", params=0x7fffffffd120) at /usr/src/debug/git/git-2.46.1/usage.c:317\n#5 BUG_fl (file=, line=, fmt=0x555555905baf \"reference backend is unknown\") at /usr/src/debug/git/git-2.46.1/usage.c:334\n#6 0x00005555557ca9d2 in ref_store_init (repo=, format=, gitdir=, flags=) at /usr/src/debug/git/git-2.46.1/refs.c:1933\n#7 ref_store_init (repo=0x5555559759a0 , format=, gitdir=, flags=15) at /usr/src/debug/git/git-2.46.1/refs.c:1923\n#8 get_main_ref_store (r=0x5555559759a0 ) at /usr/src/debug/git/git-2.46.1/refs.c:1953\n#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\n#10 include_condition_is_true (kvi=, inc=0x7fffffffd460, cond=0x55555599f503 \"test/**.path\", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:409\n#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\n#12 0x00005555556d8753 in config_parse_pair (key=, value=0x55555599fa62 \"/dev/null\", kvi=kvi@entry=0x7fffffffd4a0, fn=fn@entry=0x5555556dcad0 , data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:617\n#13 0x00005555556dc437 in parse_config_env_list (env=0x55555599fa40 \"includeif.onbranch:test/**.path\", kvi=0x7fffffffd4a0, fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:700\n#14 git_config_from_parameters (fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:773\n#15 do_git_config_sequence (opts=0x7fffffffd4b0, repo=0x0, fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:2131\n#16 config_with_options (fn=0x5555556dcad0 , fn@entry=0x555555790940 , 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\n#17 0x00005555556dd110 in read_early_config (cb=0x555555790940 , data=0x7fffffffd7a0) at /usr/src/debug/git/git-2.46.1/config.c:2229\n#18 0x000055555555ea58 in check_pager_config (cmd=) at /usr/src/debug/git/git-2.46.1/pager.c:261\n#19 run_builtin (p=0x555555966a00 , argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:461\n#20 handle_builtin (argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:732\n#21 0x000055555555eaf9 in run_argv (argcp=0x7fffffffdb0c, argv=0x7fffffffdb30) at /usr/src/debug/git/git-2.46.1/git.c:796\n#22 0x00005555555597eb in cmd_main (argc=, argv=) at /usr/src/debug/git/git-2.46.1/git.c:931\n#23 main (argc=, argv=) at /usr/src/debug/git/git-2.46.1/common-main.c:64\n\nCheers,\n\nRonan\n"},{"id":"503206","messageId":"9bf5c254-4d87-4f6f-b278-d9df4d7897b5@web.de","threadId":"62157","inReplyTo":"9fdb9561bb0ad85e55ca3253cc4db9b098641e30@rjp.ie","subject":"Re: BUG: refs.c:1933: reference backend is unknown","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-09-21T21:22:04Z","receivedAt":"2024-09-21T21:22:17Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.09.24 um 20:06 schrieb Ronan Pigott:\n>\n> The abort is reproducible exactly as written for me, but upon further\n> examination it may depend on my config. Here is a minimal reproducer:\n>\n> $ GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null git --git-dir=notexist -c 'includeif.onbranch:test/**.path=/dev/null' archive\n>\n>> bt\n> #0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44\n> #1 0x00007ffff7d78463 in __pthread_kill_internal (threadid=, signo=6) at pthread_kill.c:78\n> #2 0x00007ffff7d1f120 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26\n> #3 0x00007ffff7d064c3 in __GI_abort () at abort.c:79\n> #4 0x000055555585385a in BUG_vfl (file=, line=, fmt=0x555555905baf \"reference backend is unknown\", params=0x7fffffffd120) at /usr/src/debug/git/git-2.46.1/usage.c:317\n> #5 BUG_fl (file=, line=, fmt=0x555555905baf \"reference backend is unknown\") at /usr/src/debug/git/git-2.46.1/usage.c:334\n> #6 0x00005555557ca9d2 in ref_store_init (repo=, format=, gitdir=, flags=) at /usr/src/debug/git/git-2.46.1/refs.c:1933\n> #7 ref_store_init (repo=0x5555559759a0 , format=, gitdir=, flags=15) at /usr/src/debug/git/git-2.46.1/refs.c:1923\n> #8 get_main_ref_store (r=0x5555559759a0 ) at /usr/src/debug/git/git-2.46.1/refs.c:1953\n> #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\n> #10 include_condition_is_true (kvi=, inc=0x7fffffffd460, cond=0x55555599f503 \"test/**.path\", cond_len=7) at /usr/src/debug/git/git-2.46.1/config.c:409\n> #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\n> #12 0x00005555556d8753 in config_parse_pair (key=, value=0x55555599fa62 \"/dev/null\", kvi=kvi@entry=0x7fffffffd4a0, fn=fn@entry=0x5555556dcad0 , data=data@entry=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:617\n> #13 0x00005555556dc437 in parse_config_env_list (env=0x55555599fa40 \"includeif.onbranch:test/**.path\", kvi=0x7fffffffd4a0, fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:700\n> #14 git_config_from_parameters (fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:773\n> #15 do_git_config_sequence (opts=0x7fffffffd4b0, repo=0x0, fn=0x5555556dcad0 , data=0x7fffffffd460) at /usr/src/debug/git/git-2.46.1/config.c:2131\n> #16 config_with_options (fn=0x5555556dcad0 , fn@entry=0x555555790940 , 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\n> #17 0x00005555556dd110 in read_early_config (cb=0x555555790940 , data=0x7fffffffd7a0) at /usr/src/debug/git/git-2.46.1/config.c:2229\n> #18 0x000055555555ea58 in check_pager_config (cmd=) at /usr/src/debug/git/git-2.46.1/pager.c:261\n> #19 run_builtin (p=0x555555966a00 , argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:461\n> #20 handle_builtin (argc=1, argv=0x7fffffffdd78) at /usr/src/debug/git/git-2.46.1/git.c:732\n> #21 0x000055555555eaf9 in run_argv (argcp=0x7fffffffdb0c, argv=0x7fffffffdb30) at /usr/src/debug/git/git-2.46.1/git.c:796\n> #22 0x00005555555597eb in cmd_main (argc=, argv=) at /usr/src/debug/git/git-2.46.1/git.c:931\n> #23 main (argc=, argv=) at /usr/src/debug/git/git-2.46.1/common-main.c:64\n\nThanks, that helps. The BUG occurs during setup, before any\ncommand-specific code is executed. Affects other commands with the\noption RUN_SETUP_GENTLY as well, e.g. git grep.\n\nBisects to 173761e21b (setup: start tracking ref storage format,\n2023-12-29, copying Patrick.\n\nI wonder if the BUG at refs.c:1933 should just be turned into a die().\nOr should ref_store_init() return NULL (or some other value indicating\nan error) and all callers need to be changed to deal with that?\n\nSimply checking whether --git-dir points to an actual repo earlier is\ntempting, but would leave the case of it disappearing after the check\nunhandled.\n\nRené\n\n"},{"id":"503211","messageId":"Zu--coU4qv6Z1SmD@ArchLinux","threadId":"62157","inReplyTo":"9bf5c254-4d87-4f6f-b278-d9df4d7897b5@web.de","subject":"Re: BUG: refs.c:1933: reference backend is unknown","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-22T06:51:30Z","receivedAt":"2024-09-22T06:50:19Z","isPatch":false,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sat, Sep 21, 2024 at 11:22:04PM +0200, René Scharfe wrote:\n\n[snip]\n\n> I wonder if the BUG at refs.c:1933 should just be turned into a die().\n> Or should ref_store_init() return NULL (or some other value indicating\n> an error) and all callers need to be changed to deal with that?\n> \n> Simply checking whether --git-dir points to an actual repo earlier is\n> tempting, but would leave the case of it disappearing after the check\n> unhandled.\n\nFrom my perspective, it is not suitable to change the \"refs.c\". I guess\nthe reason why Patrick uses the \"BUG()\" is that it's an internal\nimplementation and we should tell the developer something is wrong.\n\nIn this case, because we use \"include.onbranch\" configuration, the\n\"config.c::include_by_branch\" will use \"refsc::get_main_ref_store\" to\nget the refname.\n\nHowever, the repo does not exist, so we cannot find any ref backend. The\nmain problem is that we never check whether \"the_repository->gitdir\" is\na git directory in \"config.c::include_by_branch\". So, we do not need to\ncheck the value of the option \"--git-dir\".\n\nThanks,\nJialuo\n"},{"id":"503215","messageId":"ZvAsnyZrVTO-j6XB@ArchLinux","threadId":"62157","inReplyTo":"Zu--coU4qv6Z1SmD@ArchLinux","subject":"Re: BUG: refs.c:1933: reference backend is unknown","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-22T14:41:35Z","receivedAt":"2024-09-22T14:40:24Z","isPatch":false,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sun, Sep 22, 2024 at 02:51:34PM +0800, shejialuo wrote:\n\n[snip]\n\n> However, the repo does not exist, so we cannot find any ref backend. The\n> main problem is that we never check whether \"the_repository->gitdir\" is\n> a git directory in \"config.c::include_by_branch\". So, we do not need to\n> check the value of the option \"--git-dir\".\n> \n\nI have just scanned the recent code here, because Patrick has already\nreplaced the \"the_repository\", there is no problem here when set up the\nconfiguration in the TOT.\n\nThe code will first check whether the \"data->repo\" is NULL.\n\nHowever, there is still a problem after setup:\n\n#5 0x0000555555780442 in ref_store_init (repo=0x555555948b00 ,\n format=, gitdir=, flags=15) at refs.c:1938\n#6 ref_store_init (repo=0x555555948b00 , format=,\n gitdir=, flags=15) at refs.c:1928\n#7 get_main_ref_store (r=0x555555948b00 ) at refs.c:1958\n#8 0x00005555556a332d in include_by_branch (data=0x7fffffffd400,\n cond=0x555555953d13 \"main.path\", cond_len=4) at config.c:308\n#9 include_condition_is_true (kvi=0x7fffffffd320, inc=0x7fffffffd400,\n cond=0x555555953d13 \"main.path\", cond_len=4) at config.c:408\n#10 git_config_include (var=var@entry=0x555555953d00 \"includeif.onbranch:main.path\",\n value=value@entry=0x55555595382f \"notexist\", ctx=ctx@entry=0x7fffffffd280,\n data=data@entry=0x7fffffffd400) at config.c:438\n#11 0x000055555569d3a3 in config_parse_pair (key=,\n value=0x55555595382f \"notexist\", kvi=kvi@entry=0x7fffffffd320,\n fn=fn@entry=0x5555556a3060 , data=data@entry=0x7fffffffd400)\n at config.c:616\n#12 0x000055555569dafd in parse_config_env_list (\n env=0x555555953810 \"includeIf.onbranch:main.path\", kvi=0x7fffffffd320,\n fn=0x5555556a3060 , data=0x7fffffffd400) at config.c:699\n#13 git_config_from_parameters (fn=fn@entry=0x5555556a3060 ,\n data=data@entry=0x7fffffffd400) at config.c:772\n#14 0x000055555569e383 in do_git_config_sequence (opts=0x7fffffffd550,\n repo=repo@entry=0x555555948b00 ,\n fn=fn@entry=0x5555556a3060 , data=data@entry=0x7fffffffd400)\n at config.c:2131\n#15 0x000055555569ed49 in config_with_options (fn=0x5555556a3060 ,\n data=0x7fffffffd400, config_source=, repo=0x555555948b00 ,\n opts=) at config.c:2174\n#16 0x000055555569eec7 in repo_read_config (repo=0x555555948b00 ) at config.c:2544\n#17 git_config_check_init (repo=repo@entry=0x555555948b00 ) at config.c:2564\n#18 0x00005555556a0272 in repo_config (repo=0x555555948b00 ,\n fn=fn@entry=0x5555558218a0 , data=data@entry=0x0) at config.c:2576\n#19 0x0000555555822651 in git_config (fn=0x5555558218a0 , data=0x0)\n at /home/shejialuo/Projects/git/config.h:704\n#20 init_tar_archiver () at archive-tar.c:544\n#21 0x00005555556700c9 in init_archivers () at archive.c:46\n#22 0x000055555557f8b8 in cmd_archive (argc=2, argv=0x7fffffffdcc8, prefix=0x0)\n at builtin/archive.c:98\n#23 0x00005555555753c9 in run_builtin (p=0x555555916760 , argc=2,\n argv=0x7fffffffdcc8) at git.c:483\n#24 handle_builtin (argc=2, argv=0x7fffffffdcc8) at git.c:739\n#25 0x0000555555576335 in run_argv (argcp=argcp@entry=0x7ffff\n\nIt still uses the old \"git_config\", so we still have a problem when we\nexplicitly specify the \"--git-dir\". However, once Patrick replaces all\nthe \"the_repository\", this bug should never exist. So, I won't dive into\nthis.\n\nThanks,\nJialuo\n"},{"id":"503342","messageId":"cover.1727171197.git.ps@pks.im","threadId":"62157","inReplyTo":"1b9fb3f3fde62594b9ac999ffb69e6c4fb9f6fd6@rjp.ie","subject":"[PATCH 0/2] config: fix evaluating \"onbranch\" with nonexistent git dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T10:05:37Z","receivedAt":"2024-09-24T10:05:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes evaluating \"includeIf.onbranch\" conditions\nwhen running outside of a repository with an invalid gitdir.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (2):\n t1305: exercise edge cases of \"onbranch\" includes\n config: fix evaluating \"onbranch\" with nonexistent git dir\n\n config.c | 15 +++++++++------\n t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 6 deletions(-)\n\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503343","messageId":"615df98339e9451bc237decea087716ca15d157d.1727171197.git.ps@pks.im","threadId":"62157","inReplyTo":"cover.1727171197.git.ps@pks.im","subject":"[PATCH 1/2] t1305: exercise edge cases of \"onbranch\" includes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T10:05:42Z","receivedAt":"2024-09-24T10:05:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add a couple more tests for \"onbranch\" includes for several edge cases.\nAll tests except for the last one pass, so for the most part this change\nreally only aims to nail down behaviour of include conditionals further.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 5cde79ef8c4..ad08db72308 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -357,4 +357,44 @@ test_expect_success 'include cycles are detected' '\n \tgrep \"exceeded maximum include depth\" stderr\n '\n \n+test_expect_success 'onbranch with unborn branch' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set includeIf.onbranch:\"*\".path config.inc &&\n+\t\tgit config set -f .git/config.inc foo.bar baz &&\n+\t\tgit config get foo.bar\n+\t)\n+'\n+\n+test_expect_success 'onbranch with detached HEAD' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set \"includeIf.onbranch:*.path\" config.inc &&\n+\t\tgit config set -f .git/config.inc foo.bar baz &&\n+\t\ttest_commit initial &&\n+\t\tgit switch --detach HEAD &&\n+\t\ttest_must_fail git config get foo.bar\n+\t)\n+'\n+\n+test_expect_success 'onbranch without repository' '\n+\ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n+\tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n+\tgit config set -f config.inc foo.bar baz &&\n+\tgit config get foo.bar &&\n+\ttest_must_fail nongit git config get foo.bar\n+'\n+\n+test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' '\n+\ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n+\tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n+\tgit config set -f config.inc foo.bar baz &&\n+\tgit config get foo.bar &&\n+\ttest_must_fail nongit git --git-dir=nonexistent config get foo.bar\n+'\n+\n test_done\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503344","messageId":"535d0d07506e8248e47f90c1a7581679fc297b3d.1727171197.git.ps@pks.im","threadId":"62157","inReplyTo":"cover.1727171197.git.ps@pks.im","subject":"[PATCH 2/2] config: fix evaluating \"onbranch\" with nonexistent git dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T10:05:46Z","receivedAt":"2024-09-24T10:05:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `include_by_branch()` function is responsible for evaluating whether\nor not a specific include should be pulled in based on the currently\nchecked out branch. Naturally, his condition can only be evaluated when\nwe have a properly initialized repository with a ref store in the first\nplace. This is why the function guards against the case when either\n`data->repo` or `data->repo->gitdir` are `NULL` pointers.\n\nBut the second check is insufficient: the `gitdir` may be set even\nthough the repository has not been initialized. Quoting \"setup.c\":\n\n NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some\n code paths so we also need to explicitly setup the environment if the\n user has set GIT_DIR. It may be beneficial to disallow bogus GIT_DIR\n values at some point in the future.\n\nSo when either the GIT_DIR environment variable or the `--git-dir`\nglobal option are set by the user then `the_repository` may end up with\nan initialized `gitdir` variable. And this happens even when the dir is\ninvalid, like for example when it doesn't exist. It follows that only\nchecking for whether or not `gitdir` is `NULL` is not sufficient for us\nto determine whether the repository has been properly initialized.\n\nThis issue can lead to us triggering a BUG: when using a config with an\n\"includeIf.onbranch:\" condition outside of a repository while using the\n`--git-dir` option pointing to an invalid Git directory we may end up\ntrying to evaluate the condition even though the ref storage format has\nnot been set up.\n\nThis bisects to 173761e21b (setup: start tracking ref storage format,\n2023-12-29), but that commit really only starts to surface the issue\nthat has already existed beforehand. The code to check for `gitdir` was\nintroduced via 85fe0e800c (config: work around bug with\nincludeif:onbranch and early config, 2019-07-31), which tried to fix\nsimilar issues when we didn't yet have a repository set up. But the fix\nwas incomplete as it missed the described scenario.\n\nAs the quoted comment mentions, we'd ideally refactor the code to not\nset up `gitdir` with an invalid value in the first place, but that may\nbe a bigger undertaking. Instead, refactor the code to use the ref\nstorage format as an indicator of whether or not the ref store has been\nset up to fix the bug.\n\nReported-by: Ronan Pigott \nSigned-off-by: Patrick Steinhardt \n---\n config.c | 15 +++++++++------\n t/t1305-config-include.sh | 2 +-\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 1266eab0860..a11bb85da30 100644\n--- a/config.c\n+++ b/config.c\n@@ -306,13 +306,16 @@ static int include_by_branch(struct config_include_data *data,\n \tint flags;\n \tint ret;\n \tstruct strbuf pattern = STRBUF_INIT;\n-\tconst char *refname = (!data->repo || !data->repo->gitdir) ?\n-\t\tNULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo),\n-\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n-\tconst char *shortname;\n+\tconst char *refname, *shortname;\n \n-\tif (!refname || !(flags & REF_ISSYMREF)\t||\n-\t\t\t!skip_prefix(refname, \"refs/heads/\", &shortname))\n+\tif (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)\n+\t\treturn 0;\n+\n+\trefname = refs_resolve_ref_unsafe(get_main_ref_store(data->repo),\n+\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n+\tif (!refname ||\n+\t !(flags & REF_ISSYMREF) ||\n+\t !skip_prefix(refname, \"refs/heads/\", &shortname))\n \t\treturn 0;\n \n \tstrbuf_add(&pattern, cond, cond_len);\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex ad08db72308..517d6c86937 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -389,7 +389,7 @@ test_expect_success 'onbranch without repository' '\n \ttest_must_fail nongit git config get foo.bar\n '\n \n-test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' '\n+test_expect_success 'onbranch without repository but explicit nonexistent Git directory' '\n \ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n \tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n \tgit config set -f config.inc foo.bar baz &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503359","messageId":"ZvLInrIQRj8xpSgF@ArchLinux","threadId":"62157","inReplyTo":"535d0d07506e8248e47f90c1a7581679fc297b3d.1727171197.git.ps@pks.im","subject":"Re: [PATCH 2/2] config: fix evaluating \"onbranch\" with nonexistent git dir","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-24T14:11:42Z","receivedAt":"2024-09-24T14:10:29Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Sep 24, 2024 at 12:05:46PM +0200, Patrick Steinhardt wrote:\n> The `include_by_branch()` function is responsible for evaluating whether\n> or not a specific include should be pulled in based on the currently\n> checked out branch. Naturally, his condition can only be evaluated when\n> we have a properly initialized repository with a ref store in the first\n> place. This is why the function guards against the case when either\n> `data->repo` or `data->repo->gitdir` are `NULL` pointers.\n> \n> But the second check is insufficient: the `gitdir` may be set even\n> though the repository has not been initialized. Quoting \"setup.c\":\n> \n> NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some\n> code paths so we also need to explicitly setup the environment if the\n> user has set GIT_DIR. It may be beneficial to disallow bogus GIT_DIR\n> values at some point in the future.\n> \n> So when either the GIT_DIR environment variable or the `--git-dir`\n> global option are set by the user then `the_repository` may end up with\n> an initialized `gitdir` variable. And this happens even when the dir is\n> invalid, like for example when it doesn't exist. It follows that only\n> checking for whether or not `gitdir` is `NULL` is not sufficient for us\n> to determine whether the repository has been properly initialized.\n> \n\nWhen I dive into this bug report, I feel so wired about this behavior. I\ndon't mind whether the code sets \"gitdir\" field. This is not important.\nIn \"setup.c::setup_git_directory_gently\", it will set the\n\"the_repository->gitdir\" by checking the environment variable\n\"GIT_DIR_ENVIRONMENT\". And by using `--git-dir` option, the code will\nset this environment variable, so these two ways will set the \"gitdir\"\nfield in the global variable \"the_repository\".\n\nWe actually check the validation of \"--gir-dir\" option before we set\nthis value. Let me give you an example here:\n\n $ git --git-dir=notexist -c includeIf.onbranch:main.path=any fsck\n fatal: not a git repository: 'notexist'\n\nI am curious here. And I notice the following difference in\n\"setup.c::setup_explicit_git_dir\" function:\n\n if (!is_git_directory(gitdirenv)) {\n if (nongit_ok) {\n *nongit_ok = 1;\n ...\n return NULL;\n }\n die(_(\"not a git repository: '%s'\"), gitdirenv);\n }\n\nApparently, the above example will execute the \"die\". For\n\"git-archive(1)\", it will simply return NULL. This is because we allow\nsome commands to run outside of the git repo. And we distinguish them by\nusing the \".option\" filed:\n\n { \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n { \"fsck\", cmd_fsck, RUN_SETUP },\n\nAs we can see, the code will check whether the \"gitdir\" (although it is\nnot set into \"the_repository\" structure yet) is a valid git repository.\nWe already have this information.\n\nIn f7d61c4135 (config: don't depend on `the_repository` with branch\nconditions, 2024-08-13). \"config.c::include_by_branch\" drops the global\nvariable \"the_repository\". This solves the problem reported by Ronan.\nBecause it happens in the set up process, the \"data->repo\" will be NULL.\n\n config_with_options(cb, data, NULL, NULL, &opts);\n\n int config_with_options(config_fn_t fn, void *data,\n const struct git_config_source *config_source,\n struct repository *repo,\n const struct config_options *opts)\n {\n struct config_include_data inc = CONFIG_INCLUDE_INIT;\n\n ...\n\n inc.repo = repo;\n }\n\nBut the problem still exists for \"git-archive(1)\", in \"cmd_archive\"\nfunction, we will initialize the configurations by using \"repo_config\".\nBut we are not inside the repo.\n\nI wonder how we use the global variable \"the_repository\". I think the\nmain problem here is that we use \"the_repository\" structure outside of\nthe repo where we have already broken the semantics of the\n\"the_repository\" variable.\n\n> This issue can lead to us triggering a BUG: when using a config with an\n> \"includeIf.onbranch:\" condition outside of a repository while using the\n> `--git-dir` option pointing to an invalid Git directory we may end up\n> trying to evaluate the condition even though the ref storage format has\n> not been set up.\n> \n> This bisects to 173761e21b (setup: start tracking ref storage format,\n> 2023-12-29), but that commit really only starts to surface the issue\n> that has already existed beforehand. The code to check for `gitdir` was\n> introduced via 85fe0e800c (config: work around bug with\n> includeif:onbranch and early config, 2019-07-31), which tried to fix\n> similar issues when we didn't yet have a repository set up. But the fix\n> was incomplete as it missed the described scenario.\n> \n\nYes, exactly. Because before 173761e21b, the code will always find the\nfiles backend, it does care about whether we are in the git repo or not.\n\n unsigned int format = REF_STORAGE_FORMAT_FILES;\n const struct ref_storage_be *be = find_ref_storage_backend(format);\n\nSo, it won't complain.\n\n> As the quoted comment mentions, we'd ideally refactor the code to not\n> set up `gitdir` with an invalid value in the first place, but that may\n> be a bigger undertaking. Instead, refactor the code to use the ref\n> storage format as an indicator of whether or not the ref store has been\n> set up to fix the bug.\n> \n\nShould we? From my above comments, it does no matter whether we set the\n\"gitdir\" in the \"the_repository\". Because we already check this and we\nhave this information. I think the main problem is that we use\n\"the_repository\" badly.\n\nWe could run git commands inside the repo or outside the repo. If we run\ngit commands outside the repo, should we use the \"the_repository\"\nvariable? I guess we should not.\n\nBecause \"git_config\", \"repo_config\" and so on use the global variable\n\"the_repository\", so we will encounter the trouble. But if we could use\nsomething like \"data->repo\". We will make everything OK:\n\n1. When running commands inside the git repo, we set \"data->repo\" to be\n\"the_repository\".\n2. When ruing commands outside the git repo, we set \"data->repo\" to be\nNULL.\n\n> Reported-by: Ronan Pigott \n> Signed-off-by: Patrick Steinhardt \n> ---\n> config.c | 15 +++++++++------\n> t/t1305-config-include.sh | 2 +-\n> 2 files changed, 10 insertions(+), 7 deletions(-)\n> \n> diff --git a/config.c b/config.c\n> index 1266eab0860..a11bb85da30 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -306,13 +306,16 @@ static int include_by_branch(struct config_include_data *data,\n> \tint flags;\n> \tint ret;\n> \tstruct strbuf pattern = STRBUF_INIT;\n> -\tconst char *refname = (!data->repo || !data->repo->gitdir) ?\n> -\t\tNULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo),\n> -\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n> -\tconst char *shortname;\n> +\tconst char *refname, *shortname;\n> \n> -\tif (!refname || !(flags & REF_ISSYMREF)\t||\n> -\t\t\t!skip_prefix(refname, \"refs/heads/\", &shortname))\n> +\tif (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)\n> +\t\treturn 0;\n> +\n> +\trefname = refs_resolve_ref_unsafe(get_main_ref_store(data->repo),\n> +\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n> +\tif (!refname ||\n> +\t !(flags & REF_ISSYMREF) ||\n> +\t !skip_prefix(refname, \"refs/heads/\", &shortname))\n> \t\treturn 0;\n> \n> \tstrbuf_add(&pattern, cond, cond_len);\n> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n> index ad08db72308..517d6c86937 100755\n> --- a/t/t1305-config-include.sh\n> +++ b/t/t1305-config-include.sh\n> @@ -389,7 +389,7 @@ test_expect_success 'onbranch without repository' '\n> \ttest_must_fail nongit git config get foo.bar\n> '\n> \n> -test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' '\n> +test_expect_success 'onbranch without repository but explicit nonexistent Git directory' '\n> \ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n> \tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n> \tgit config set -f config.inc foo.bar baz &&\n> -- \n> 2.46.0.551.gc5ee8f2d1c.dirty\n"},{"id":"503360","messageId":"ZvLKqujqfDFxCtBp@ArchLinux","threadId":"62157","inReplyTo":"cover.1727171197.git.ps@pks.im","subject":"Re: [PATCH 0/2] config: fix evaluating \"onbranch\" with nonexistent git dir","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-24T14:20:26Z","receivedAt":"2024-09-24T14:19:14Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Sep 24, 2024 at 12:05:37PM +0200, Patrick Steinhardt wrote:\n> Hi,\n> \n> this small patch series fixes evaluating \"includeIf.onbranch\" conditions\n> when running outside of a repository with an invalid gitdir.\n> \n> Thanks!\n> \n> Patrick\n> \n> Patrick Steinhardt (2):\n> t1305: exercise edge cases of \"onbranch\" includes\n> config: fix evaluating \"onbranch\" with nonexistent git dir\n> \n> config.c | 15 +++++++++------\n> t/t1305-config-include.sh | 40 +++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 49 insertions(+), 6 deletions(-)\n> \n> -- \n> 2.46.0.551.gc5ee8f2d1c.dirty\n> \n\nAlthough I comment a lot in [PATCH 2/2], I think this series is good\nenough. The reason why I comment on [PATCH 2/2] is I think we should\nenhance this behavior in the future.\n\nThanks,\nJialuo\n"},{"id":"503366","messageId":"xmqqed59vxy3.fsf@gitster.g","threadId":"62157","inReplyTo":"615df98339e9451bc237decea087716ca15d157d.1727171197.git.ps@pks.im","subject":"Re: [PATCH 1/2] t1305: exercise edge cases of \"onbranch\" includes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T16:05:56Z","receivedAt":"2024-09-24T16:06:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +test_expect_success 'onbranch without repository' '\n> +\ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n> +\tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n> +\tgit config set -f config.inc foo.bar baz &&\n\nThis assumes that the $(pwd) is the $HOME; so .gitconfig is the\nper-user configuration that ought to apply everywhere; since\nincludeIf..path that is relative is relative to the\nincluding file, config.inc would be in cluded when the condition\nholds in $HOME/.gitconfig. OK.\n\n> +\tgit config get foo.bar &&\n\nThis assumes that the $(pwd) that is $HOME is a valid repository,\nand checks if includeIf.onbranch works from within a repository.\nOK.\n\n> +\ttest_must_fail nongit git config get foo.bar\n> +'\n\n> +test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' '\n> +\ttest_when_finished \"rm -f .gitconfig config.inc\" &&\n> +\tgit config set -f .gitconfig \"includeIf.onbranch:**.path\" config.inc &&\n> +\tgit config set -f config.inc foo.bar baz &&\n\nThe same set-up.\n\n> +\tgit config get foo.bar &&\n> +\ttest_must_fail nongit git --git-dir=nonexistent config get foo.bar\n\nIt has to work when $(pwd) is outside a repository, but is \"nongit\"\nstrictly necessary? IOW, even when we _could_ discover the top\nlevel of a git-controlled working tree, wouldn't presence of --git-dir\nthat points at elsewhere make $(pwd) and the repository there irrelevant\nto the operation?\n\nI am not suggesting to just drop \"nongit\" from this test. I am\nwondering if this is better split into two tests, with and without\n\"nongit\" to test different situations.\n\n> +'\n> +\n> test_done\n\nOther than that, looks like good additions to the test coverage.\n\nThanks.\n"},{"id":"503367","messageId":"xmqq7cb1vxee.fsf@gitster.g","threadId":"62157","inReplyTo":"535d0d07506e8248e47f90c1a7581679fc297b3d.1727171197.git.ps@pks.im","subject":"Re: [PATCH 2/2] config: fix evaluating \"onbranch\" with nonexistent git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T16:17:45Z","receivedAt":"2024-09-24T16:17:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> As the quoted comment mentions, we'd ideally refactor the code to not\n> set up `gitdir` with an invalid value in the first place, but that may\n> be a bigger undertaking. Instead, refactor the code to use the ref\n> storage format as an indicator of whether or not the ref store has been\n> set up to fix the bug.\n>\n> Reported-by: Ronan Pigott \n> Signed-off-by: Patrick Steinhardt \n> ---\n> ...\n> -\tconst char *refname = (!data->repo || !data->repo->gitdir) ?\n> -\t\tNULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo),\n> -\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n> -\tconst char *shortname;\n> +\tconst char *refname, *shortname;\n> \n> -\tif (!refname || !(flags & REF_ISSYMREF)\t||\n> -\t\t\t!skip_prefix(refname, \"refs/heads/\", &shortname))\n> +\tif (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)\n> +\t\treturn 0;\n\nOK. A very direct check to see what we really care about.\n\n"}]}