{"thread":{"id":"61521","subject":"git init BUG when gitconfig has includeIf","startedAt":"2024-05-21T07:54:25Z","lastAt":"2024-05-23T05:26:22Z","messageCount":14,"participants":["Heghedus Razvan","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"495149","messageId":"D1F5SJ52H4Z0.1NR7M8APRFTOB@protonmail.com","threadId":"61521","inReplyTo":null,"subject":"git init BUG when gitconfig has includeIf","fromName":"Heghedus Razvan","fromEmail":"heghedus.razvan@protonmail.com","sentAt":"2024-05-21T07:54:11Z","receivedAt":"2024-05-21T07:54:25Z","isPatch":false,"sender":{"key":"heghedus.razvan@protonmail.com","avatar":null},"body":"Hello everybody\n\nYesterday I stumble upon a bug when doing git init. I didn't\nfind any references to it, so I don't know if is a known problem\nor not.\n\nSteps to reproduce:\n# git init .\nBUG: refs.c:2123: reference backend is unknown\n\nI'm able to reproduce this on multiple systems where the git\nversion is newer than v2.44.0. This happens only when I have\nan \"includeIf\" in the ~/.gitconfig . Bellow [1] I have a\nminimal configuration for reproducing the bug. With same\ngitconfig but versions lower than v2.43.4 the error is\nno longer happening.\n\nSo I went and did a git bisection and the error appeared with\n173761e21b setup: start tracking ref storage format\nPrevious commit (\"0fcc285c5e\") is working fine.\n\nI cc'ed Patrick Steinhardt since the error appeared from his\ncommit.\n\nBest regards,\nRazvan\n\n--\n\n1:\ncontent of ~/.gitconfig\n```\n[user]\n        email = some_email@test.net\n        name = test\n[includeIf \"onbranch:upstream*\"]\n        path=~/.gitconfig_upstream\n```\n\ncontent of ~/.gitconfig_upstream\n```\n[user]\n        name = test test\n```\n\n"},{"id":"495177","messageId":"xmqqbk4zazqd.fsf@gitster.g","threadId":"61521","inReplyTo":"D1F5SJ52H4Z0.1NR7M8APRFTOB@protonmail.com","subject":"Re: git init BUG when gitconfig has includeIf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-21T16:36:42Z","receivedAt":"2024-05-21T16:36:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n\n> Yesterday I stumble upon a bug when doing git init. I didn't\n> find any references to it, so I don't know if is a known problem\n> or not.\n>\n> Steps to reproduce:\n> # git init .\n> BUG: refs.c:2123: reference backend is unknown\n\nPatrick, this looks similar to an earlier one during \"git clone\"\nthat was discussed at\n\nhttps://lore.kernel.org/git/72771da0-a0ef-4fd9-8071-6467cd7b6a6b@kernel-space.org/\n\nthat was fixed with 199f44cb (builtin/clone: allow remote helpers to\ndetect repo, 2024-02-27)?  The fix was about \"git clone\", but the\ncrux of the fix went to setup.c:initialize_repository_version()\nwhich is also called by setup.c:init_db() that is the workhorse of\n\"git init\", so it may already have been fixed (I didn't try).\n\nEven if it is already fixed in the current version by the same\n199f44cb, we may want to follow up 0eab85b9 (t5601: exercise clones\nwith \"includeIf.*.onbranch\", 2024-03-12) with an additional test to\ncover \"git init\".\n\nThanks.\n"},{"id":"495178","messageId":"D1FH40J6URHF.186U29CMRJ0N3@protonmail.com","threadId":"61521","inReplyTo":"xmqqbk4zazqd.fsf@gitster.g","subject":"Re: git init BUG when gitconfig has includeIf","fromName":"Heghedus Razvan","fromEmail":"heghedus.razvan@protonmail.com","sentAt":"2024-05-21T16:46:23Z","receivedAt":"2024-05-21T16:46:38Z","isPatch":false,"sender":{"key":"heghedus.razvan@protonmail.com","avatar":null},"body":"On Tue May 21, 2024 at 7:36 PM EEST, Junio C Hamano wrote:\n> Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n>\n> > Yesterday I stumble upon a bug when doing git init. I didn't\n> > find any references to it, so I don't know if is a known problem\n> > or not.\n> >\n> > Steps to reproduce:\n> > # git init .\n> > BUG: refs.c:2123: reference backend is unknown\n>\n> Patrick, this looks similar to an earlier one during \"git clone\"\n> that was discussed at\n>\n> https://lore.kernel.org/git/72771da0-a0ef-4fd9-8071-6467cd7b6a6b@kernel-space.org/\n>\n> that was fixed with 199f44cb (builtin/clone: allow remote helpers to\n> detect repo, 2024-02-27)?  The fix was about \"git clone\", but the\n> crux of the fix went to setup.c:initialize_repository_version()\n> which is also called by setup.c:init_db() that is the workhorse of\n> \"git init\", so it may already have been fixed (I didn't try).\n\nI guess I forgot to mention, but I tested the current master 4365c6fcf9\nand the issue is still present.\n\n>\n> Even if it is already fixed in the current version by the same\n> 199f44cb, we may want to follow up 0eab85b9 (t5601: exercise clones\n> with \"includeIf.*.onbranch\", 2024-03-12) with an additional test to\n> cover \"git init\".\n>\n> Thanks.\n\n\n"},{"id":"495277","messageId":"Zk2ndVx4gMTV8ZUf@tanuki","threadId":"61521","inReplyTo":"D1FH40J6URHF.186U29CMRJ0N3@protonmail.com","subject":"Re: git init BUG when gitconfig has includeIf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-22T08:06:13Z","receivedAt":"2024-05-22T08:06:19Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 21, 2024 at 04:46:23PM +0000, Heghedus Razvan wrote:\n> On Tue May 21, 2024 at 7:36 PM EEST, Junio C Hamano wrote:\n> > Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n> >\n> > > Yesterday I stumble upon a bug when doing git init. I didn't\n> > > find any references to it, so I don't know if is a known problem\n> > > or not.\n> > >\n> > > Steps to reproduce:\n> > > # git init .\n> > > BUG: refs.c:2123: reference backend is unknown\n> >\n> > Patrick, this looks similar to an earlier one during \"git clone\"\n> > that was discussed at\n> >\n> > https://lore.kernel.org/git/72771da0-a0ef-4fd9-8071-6467cd7b6a6b@kernel-space.org/\n> >\n> > that was fixed with 199f44cb (builtin/clone: allow remote helpers to\n> > detect repo, 2024-02-27)?  The fix was about \"git clone\", but the\n> > crux of the fix went to setup.c:initialize_repository_version()\n> > which is also called by setup.c:init_db() that is the workhorse of\n> > \"git init\", so it may already have been fixed (I didn't try).\n> \n> I guess I forgot to mention, but I tested the current master 4365c6fcf9\n> and the issue is still present.\n\nI cannot reproduce the issue as-is, neither on Git v2.44 nor on the\ncurrent master branch. So clearly, there must be something special to\nyour setup. The following testcase and variants of it do not reproduce:\n\n    test_expect_success 'init with includeIf.onbranch condition' '\n        git config -f ./config foo.bar baz &&\n        include=$(test-tool path-utils absolute_path config) &&\n        test_when_finished \"rm -rf repo\" &&\n        git -c includeIf.onbranch:main.path=\"$(<include)\" init repo\n    '\n\nNow digging into the code, the condition gets evaluated in\n`include_by_branch()`. The call to `refs_resolve_ref_unsafe()` is\nguarded by `the_repository->gitdir`, which is `NULL` the first time it\nis called by git-init(1). It does get called a second time, but at that\npoint we already initialized the refdb and configured the ref storage\nformat as expected.\n\nAha! Seems like this only happens when re-initializing an already\nexistent repository, that's what's missing. In that case we do already\nhave `the_repository->gitdir` set even though we did not yet set up the\nref storage format. I'll investigate and send a patch.\n\nCan you confirm that this is what you see, or do you also see this when\ncreating an entirely new repository?\n\n> > Even if it is already fixed in the current version by the same\n> > 199f44cb, we may want to follow up 0eab85b9 (t5601: exercise clones\n> > with \"includeIf.*.onbranch\", 2024-03-12) with an additional test to\n> > cover \"git init\".\n\nAgreed.\n\nPatrick\n"},{"id":"495281","messageId":"D1G108TQBWK4.2XW34J29NCAHG@protonmail.com","threadId":"61521","inReplyTo":"Zk2ndVx4gMTV8ZUf@tanuki","subject":"Re: git init BUG when gitconfig has includeIf","fromName":"Heghedus Razvan","fromEmail":"heghedus.razvan@protonmail.com","sentAt":"2024-05-22T08:21:50Z","receivedAt":"2024-05-22T08:22:03Z","isPatch":false,"sender":{"key":"heghedus.razvan@protonmail.com","avatar":null},"body":"On Wed May 22, 2024 at 11:06 AM EEST, Patrick Steinhardt wrote:\n> On Tue, May 21, 2024 at 04:46:23PM +0000, Heghedus Razvan wrote:\n> > On Tue May 21, 2024 at 7:36 PM EEST, Junio C Hamano wrote:\n> > > Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n> > >\n> > > > Yesterday I stumble upon a bug when doing git init. I didn't\n> > > > find any references to it, so I don't know if is a known problem\n> > > > or not.\n> > > >\n> > > > Steps to reproduce:\n> > > > # git init .\n> > > > BUG: refs.c:2123: reference backend is unknown\n> > >\n> > > Patrick, this looks similar to an earlier one during \"git clone\"\n> > > that was discussed at\n> > >\n> > > https://lore.kernel.org/git/72771da0-a0ef-4fd9-8071-6467cd7b6a6b@kernel-space.org/\n> > >\n> > > that was fixed with 199f44cb (builtin/clone: allow remote helpers to\n> > > detect repo, 2024-02-27)?  The fix was about \"git clone\", but the\n> > > crux of the fix went to setup.c:initialize_repository_version()\n> > > which is also called by setup.c:init_db() that is the workhorse of\n> > > \"git init\", so it may already have been fixed (I didn't try).\n> > \n> > I guess I forgot to mention, but I tested the current master 4365c6fcf9\n> > and the issue is still present.\n>\n> I cannot reproduce the issue as-is, neither on Git v2.44 nor on the\n> current master branch. So clearly, there must be something special to\n> your setup. The following testcase and variants of it do not reproduce:\n>\n>     test_expect_success 'init with includeIf.onbranch condition' '\n>         git config -f ./config foo.bar baz &&\n>         include=$(test-tool path-utils absolute_path config) &&\n>         test_when_finished \"rm -rf repo\" &&\n>         git -c includeIf.onbranch:main.path=\"$(<include)\" init repo\n>     '\n>\n> Now digging into the code, the condition gets evaluated in\n> `include_by_branch()`. The call to `refs_resolve_ref_unsafe()` is\n> guarded by `the_repository->gitdir`, which is `NULL` the first time it\n> is called by git-init(1). It does get called a second time, but at that\n> point we already initialized the refdb and configured the ref storage\n> format as expected.\n>\n> Aha! Seems like this only happens when re-initializing an already\n> existent repository, that's what's missing. In that case we do already\n> have `the_repository->gitdir` set even though we did not yet set up the\n> ref storage format. I'll investigate and send a patch.\n>\n> Can you confirm that this is what you see, or do you also see this when\n> creating an entirely new repository?\n\nHi Patrick,\n\nThanks for looking into this. It seems that the issue happens only when\nthe folder for the repo exists.\n\nEg:\n$ mkdir new_folder\n$ cd new_folder\n$ git init . \n\nor\n$ mkdir new_folder\n$ git init new_folder\n\nBut directly running `git init new_folder` when there is no `new_folder`\nworks fine.\n\nFYI, I just did these tests on master (4365c6fcf9).\n\nRegards,\nRazvan\n\n>\n> > > Even if it is already fixed in the current version by the same\n> > > 199f44cb, we may want to follow up 0eab85b9 (t5601: exercise clones\n> > > with \"includeIf.*.onbranch\", 2024-03-12) with an additional test to\n> > > cover \"git init\".\n>\n> Agreed.\n>\n> Patrick\n\n\n"},{"id":"495282","messageId":"Zk2sc-649Pz77_G-@tanuki","threadId":"61521","inReplyTo":"D1G108TQBWK4.2XW34J29NCAHG@protonmail.com","subject":"Re: git init BUG when gitconfig has includeIf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-22T08:27:31Z","receivedAt":"2024-05-22T08:27:37Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 22, 2024 at 08:21:50AM +0000, Heghedus Razvan wrote:\n> On Wed May 22, 2024 at 11:06 AM EEST, Patrick Steinhardt wrote:\n> > On Tue, May 21, 2024 at 04:46:23PM +0000, Heghedus Razvan wrote:\n> > > On Tue May 21, 2024 at 7:36 PM EEST, Junio C Hamano wrote:\n> > > > Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n> > Aha! Seems like this only happens when re-initializing an already\n> > existent repository, that's what's missing. In that case we do already\n> > have `the_repository->gitdir` set even though we did not yet set up the\n> > ref storage format. I'll investigate and send a patch.\n> >\n> > Can you confirm that this is what you see, or do you also see this when\n> > creating an entirely new repository?\n> \n> Hi Patrick,\n> \n> Thanks for looking into this. It seems that the issue happens only when\n> the folder for the repo exists.\n> \n> Eg:\n> $ mkdir new_folder\n> $ cd new_folder\n> $ git init . \n> \n> or\n> $ mkdir new_folder\n> $ git init new_folder\n> \n> But directly running `git init new_folder` when there is no `new_folder`\n> works fine.\n> \n> FYI, I just did these tests on master (4365c6fcf9).\n> \n> Regards,\n> Razvan\n\nIndeed, thanks for clarifying. From the following three tests, the\nlatter two fail:\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex b131d665db..c1c7c307d3 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -678,4 +678,21 @@ test_expect_success 'branch -m with the initial branch' '\n    test_cmp expect actual\n '\n \n+test_expect_success 'init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit -c includeIf.onbranch:main.path=something init repo\n+'\n+\n+test_expect_success 'init with includeIf.onbranch condition with existing directory' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tmkdir repo &&\n+\tgit -c includeIf.onbranch:main.path=something init repo\n+'\n+\n+test_expect_success 're-init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:main.path=something init repo\n+'\n+\n test_done\n\nI've got a fix ready that I'll send upstream later today.\n\nPatrick\n"},{"id":"495295","messageId":"cf182bb9ee7d4a7eb46e5dbf4f3ef5deb198d823.1716374321.git.ps@pks.im","threadId":"61521","inReplyTo":"D1F5SJ52H4Z0.1NR7M8APRFTOB@protonmail.com","subject":"[PATCH] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-22T10:38:46Z","receivedAt":"2024-05-22T10:38:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported that git-init(1) can fail when initializing an existing\ndirectory in case the config contains an \"includeIf.onbranch:\"\ncondition:\n\n```shell\n$ mkdir repo\n$ git -c includeIf.onbranch:main.path=nonexistent init repo\nBUG: refs.c:2056: reference backend is unknown\n```\n\nThe same error can also be triggered when re-initializing an already\nexisting repository.\n\nThe bug has been introduced in 173761e21b (setup: start tracking ref\nstorage format, 2023-12-29), which wired up the ref storage format. The\nroot cause is in `init_db()`, which tries to read the config before we\nhave initialized `the_repository` and most importantly its ref storage\nformat. We eventually end up calling `include_by_branch()` and execute\n`refs_resolve_ref_unsafe()`, but because we have not initialized the ref\nstorage format yet this will trigger the above bug.\n\nInterestingly, `include_by_branch()` has a mechanism that will only\ncause us to resolve the ref when `the_repository->gitdir` is set. This\nis also the reason why this only happens when we initialize an already\nexisting directory or repository: `gitdir` is set in those cases, but\nnot when creating a new directory.\n\nNow there are two ways to address the issue:\n\n  - We can adapt `include_by_branch()` to also make the code conditional\n    on whether `the_repository->ref_storage_format` is set.\n\n  - We can shift around code such that we initialize the repository\n    format before we read the config.\n\nWhile the first approach would be safe, it may also cause us to paper\nover issues where a ref store should have been set up. In our case for\nexample, it may be reasonable to expect that re-initializing the repo\nwill cause the \"onbranch:\" condition to trigger, but we would not do\nthat if the ref storage format was not set up yet. This also used to\nwork before the above commit that introduced this bug.\n\nRearrange the code such that we set up the repository format before\nreading the config. This fixes the bug and ensures that \"onbranch:\"\nconditions can trigger.\n\nReported-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n setup.c         |  21 +++++-----\n t/t0001-init.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 104 insertions(+), 17 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 9247cded6a..8c7bc7bdb1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2286,12 +2286,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t}\n \tstartup_info->have_repository = 1;\n \n-\t/* Ensure `core.hidedotfiles` is processed */\n-\tgit_config(platform_core_config, NULL);\n-\n-\tsafe_create_dir(git_dir, 0);\n-\n-\n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n \t * config file, so this will not fail.  What we are catching\n@@ -2302,9 +2296,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \tvalidate_ref_storage_format(&repo_fmt, ref_storage_format);\n \n-\treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, init_shared_repository);\n-\n \t/*\n \t * Now that we have set up both the hash algorithm and the ref storage\n \t * format we can update the repository's settings accordingly.\n@@ -2312,6 +2303,18 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \trepo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);\n \n+\t/*\n+\t * Ensure `core.hidedotfiles` is processed. This must happen after we\n+\t * have set up the repository format such that we can evaluate\n+\t * includeIf conditions correctly in the case of re-initialization.\n+\t */\n+\tgit_config(platform_core_config, NULL);\n+\n+\tsafe_create_dir(git_dir, 0);\n+\n+\treinit = create_default_files(template_dir, original_git_dir,\n+\t\t\t\t      &repo_fmt, init_shared_repository);\n+\n \tif (!(flags & INIT_DB_SKIP_REFDB))\n \t\tcreate_reference_database(repo_fmt.ref_storage_format,\n \t\t\t\t\t  initial_branch, flags & INIT_DB_QUIET);\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex b131d665db..2239bed198 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -584,14 +584,38 @@ test_expect_success 'init with --ref-format=files' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 're-init with same format' '\n-\ttest_when_finished \"rm -rf refformat\" &&\n-\tgit init --ref-format=files refformat &&\n-\tgit init --ref-format=files refformat &&\n-\techo files >expect &&\n-\tgit -C refformat rev-parse --show-ref-format >actual &&\n-\ttest_cmp expect actual\n-'\n+for from_format in files reftable\n+do\n+\tfor to_format in files reftable\n+\tdo\n+\t\tif test \"$from_format\" = \"$to_format\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\ttest_expect_success \"re-init with same format ($from_format)\" '\n+\t\t\ttest_when_finished \"rm -rf refformat\" &&\n+\t\t\tgit init --ref-format=$from_format refformat &&\n+\t\t\tgit init --ref-format=$from_format refformat &&\n+\t\t\techo $from_format >expect &&\n+\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\n+\t\ttest_expect_success \"re-init with different format fails ($from_format -> $to_format)\" '\n+\t\t\ttest_when_finished \"rm -rf refformat\" &&\n+\t\t\tgit init --ref-format=$from_format refformat &&\n+\t\t\tcat >expect <<-EOF &&\n+\t\t\tfatal: attempt to reinitialize repository with different reference storage format\n+\t\t\tEOF\n+\t\t\ttest_must_fail git init --ref-format=$to_format refformat 2>err &&\n+\t\t\ttest_cmp expect err &&\n+\t\t\techo $from_format >expect &&\n+\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n \n test_expect_success 'init with --ref-format=garbage' '\n \ttest_when_finished \"rm -rf refformat\" &&\n@@ -678,4 +702,64 @@ test_expect_success 'branch -m with the initial branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit -c includeIf.onbranch:main.path=nonexistent init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'init with includeIf.onbranch condition with existing directory' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tmkdir repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init skips non-matching includeIf.onbranch' '\n+\ttest_when_finished \"rm -rf repo config\" &&\n+\tcat >config <<-EOF &&\n+\t[\n+\tgarbage\n+\tEOF\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=\"$(test-tool path-utils absolute_path config)\" init repo\n+'\n+\n+test_expect_success 're-init reads matching includeIf.onbranch' '\n+\ttest_when_finished \"rm -rf repo config\" &&\n+\tcat >config <<-EOF &&\n+\t[\n+\tgarbage\n+\tEOF\n+\tpath=\"$(test-tool path-utils absolute_path config)\" &&\n+\tgit init --initial-branch=branch repo &&\n+\tcat >expect <<-EOF &&\n+\tfatal: bad config line 1 in file $path\n+\tEOF\n+\ttest_must_fail git -c includeIf.onbranch:branch.path=\"$path\" init repo 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.45.1.216.g4365c6fcf9.dirty\n\n"},{"id":"495297","messageId":"D1G4BWOMJD3E.2CNXHMRZQ12FW@protonmail.com","threadId":"61521","inReplyTo":"cf182bb9ee7d4a7eb46e5dbf4f3ef5deb198d823.1716374321.git.ps@pks.im","subject":"Re: [PATCH] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Heghedus Razvan","fromEmail":"heghedus.razvan@protonmail.com","sentAt":"2024-05-22T10:58:08Z","receivedAt":"2024-05-22T10:58:15Z","isPatch":true,"sender":{"key":"heghedus.razvan@protonmail.com","avatar":null},"body":"On Wed May 22, 2024 at 1:38 PM EEST, Patrick Steinhardt wrote:\n> It was reported that git-init(1) can fail when initializing an existing\n> directory in case the config contains an \"includeIf.onbranch:\"\n> condition:\n>\n> ```shell\n> $ mkdir repo\n> $ git -c includeIf.onbranch:main.path=nonexistent init repo\n> BUG: refs.c:2056: reference backend is unknown\n> ```\n>\n> The same error can also be triggered when re-initializing an already\n> existing repository.\n>\n> The bug has been introduced in 173761e21b (setup: start tracking ref\n> storage format, 2023-12-29), which wired up the ref storage format. The\n> root cause is in `init_db()`, which tries to read the config before we\n> have initialized `the_repository` and most importantly its ref storage\n> format. We eventually end up calling `include_by_branch()` and execute\n> `refs_resolve_ref_unsafe()`, but because we have not initialized the ref\n> storage format yet this will trigger the above bug.\n>\n> Interestingly, `include_by_branch()` has a mechanism that will only\n> cause us to resolve the ref when `the_repository->gitdir` is set. This\n> is also the reason why this only happens when we initialize an already\n> existing directory or repository: `gitdir` is set in those cases, but\n> not when creating a new directory.\n>\n> Now there are two ways to address the issue:\n>\n>   - We can adapt `include_by_branch()` to also make the code conditional\n>     on whether `the_repository->ref_storage_format` is set.\n>\n>   - We can shift around code such that we initialize the repository\n>     format before we read the config.\n>\n> While the first approach would be safe, it may also cause us to paper\n> over issues where a ref store should have been set up. In our case for\n> example, it may be reasonable to expect that re-initializing the repo\n> will cause the \"onbranch:\" condition to trigger, but we would not do\n> that if the ref storage format was not set up yet. This also used to\n> work before the above commit that introduced this bug.\n>\n> Rearrange the code such that we set up the repository format before\n> reading the config. This fixes the bug and ensures that \"onbranch:\"\n> conditions can trigger.\n>\n> Reported-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nI can confirm it's fixing the issue. Feel free to add:\n\nTested-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\n\n"},{"id":"495317","messageId":"xmqqcypd4qfr.fsf@gitster.g","threadId":"61521","inReplyTo":"D1G4BWOMJD3E.2CNXHMRZQ12FW@protonmail.com","subject":"Re: [PATCH] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T19:06:16Z","receivedAt":"2024-05-22T19:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heghedus Razvan <heghedus.razvan@protonmail.com> writes:\n\n>> Reported-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>\n> I can confirm it's fixing the issue. Feel free to add:\n>\n> Tested-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\n\nThanks, both.\n"},{"id":"495340","messageId":"xmqq4jap1hs7.fsf@gitster.g","threadId":"61521","inReplyTo":"cf182bb9ee7d4a7eb46e5dbf4f3ef5deb198d823.1716374321.git.ps@pks.im","subject":"Re: [PATCH] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T00:41:28Z","receivedAt":"2024-05-23T00:41:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The bug has been introduced in 173761e21b (setup: start tracking ref\n> storage format, 2023-12-29), which wired up the ref storage format. The\n> root cause is in `init_db()`, which tries to read the config before we\n> have initialized `the_repository` and most importantly its ref storage\n> format. We eventually end up calling `include_by_branch()` and execute\n> `refs_resolve_ref_unsafe()`, but because we have not initialized the ref\n> storage format yet this will trigger the above bug.\n\nSo I was trying to see how feasible it would be to backport this fix\non top of v2.44.0 which was the first version that was shipped with\n173761e2 (setup: start tracking ref storage format, 2023-12-29),\nwhich is v2.44.0-rc0~78^2~8 and noticed something curious.\n\nAs your addition to t0001-init.sh wants to try converting the ref\nbackends to and from files and reftable, it actually will barf until\nb4385bf0 (Merge branch 'ps/reftable-backend', 2024-02-26), which is\nv2.45.0-rc0~176, but even with a reinit with the same backend, the\nproblem should manifest itself, right?\n\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index b131d665db..2239bed198 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -584,14 +584,38 @@ test_expect_success 'init with --ref-format=files' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success 're-init with same format' '\n> -\ttest_when_finished \"rm -rf refformat\" &&\n> -\tgit init --ref-format=files refformat &&\n> -\tgit init --ref-format=files refformat &&\n> -\techo files >expect &&\n> -\tgit -C refformat rev-parse --show-ref-format >actual &&\n> -\ttest_cmp expect actual\n> -'\n\nSo we used to have \"files to files\" and nothing else.  But\n\n> +for from_format in files reftable\n> +do\n> +\tfor to_format in files reftable\n> +\tdo\n> +\t\tif test \"$from_format\" = \"$to_format\"\n> +\t\tthen\n> +\t\t\tcontinue\n> +\t\tfi\n> +\n> +\t\ttest_expect_success \"re-init with same format ($from_format)\" '\n> +\t\t\ttest_when_finished \"rm -rf refformat\" &&\n> +\t\t\tgit init --ref-format=$from_format refformat &&\n> +\t\t\tgit init --ref-format=$from_format refformat &&\n> +\t\t\techo $from_format >expect &&\n> +\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t'\n\nThis is very iffy, isn't it?  This one wants to do the same format\nreinit, but it is behind \"if from and to are the same, skip the rest\"\nin the loop.\n\nI think the \"same format\" loop should be lifted outside and\nimmediately before the inner loop and we should be OK.\n\n> +\t\ttest_expect_success \"re-init with different format fails ($from_format -> $to_format)\" '\n> +\t\t\ttest_when_finished \"rm -rf refformat\" &&\n> +\t\t\tgit init --ref-format=$from_format refformat &&\n> +\t\t\tcat >expect <<-EOF &&\n> +\t\t\tfatal: attempt to reinitialize repository with different reference storage format\n> +\t\t\tEOF\n> +\t\t\ttest_must_fail git init --ref-format=$to_format refformat 2>err &&\n> +\t\t\ttest_cmp expect err &&\n> +\t\t\techo $from_format >expect &&\n> +\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t'\n> +\tdone\n> +done\n\nIn any case, this \"reinitialize to a different format is too late\"\ntest has nothing to do with the problem we are fixing with this\npatch, no?  It is a valuable set of tests in the post b4385bf0 world\nwhere we can choose between the two, but it is somewhat out of scope\nof the \"we need to initialize the ref backend before we can do onbranch\nconfig\".\n\nI'll send your patch backported to v2.44.0, plus the change needed\nto review the merge of it into v2.45.0 codebase later.\n"},{"id":"495341","messageId":"xmqqwmnlz7bf.fsf_-_@gitster.g","threadId":"61521","inReplyTo":"cf182bb9ee7d4a7eb46e5dbf4f3ef5deb198d823.1716374321.git.ps@pks.im","subject":"[PATCH v2] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T00:43:32Z","receivedAt":"2024-05-23T00:43:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It was reported that git-init(1) can fail when initializing an existing\ndirectory in case the config contains an \"includeIf.onbranch:\"\ncondition:\n\n    $ mkdir repo\n    $ git -c includeIf.onbranch:main.path=nonexistent init repo\n    BUG: refs.c:2056: reference backend is unknown\n\nThe same error can also be triggered when re-initializing an already\nexisting repository.\n\nThe bug has been introduced in 173761e21b (setup: start tracking ref\nstorage format, 2023-12-29), which wired up the ref storage format. The\nroot cause is in `init_db()`, which tries to read the config before we\nhave initialized `the_repository` and most importantly its ref storage\nformat. We eventually end up calling `include_by_branch()` and execute\n`refs_resolve_ref_unsafe()`, but because we have not initialized the ref\nstorage format yet this will trigger the above bug.\n\nInterestingly, `include_by_branch()` has a mechanism that will only\ncause us to resolve the ref when `the_repository->gitdir` is set. This\nis also the reason why this only happens when we initialize an already\nexisting directory or repository: `gitdir` is set in those cases, but\nnot when creating a new directory.\n\nNow there are two ways to address the issue:\n\n  - We can adapt `include_by_branch()` to also make the code conditional\n    on whether `the_repository->ref_storage_format` is set.\n\n  - We can shift around code such that we initialize the repository\n    format before we read the config.\n\nWhile the first approach would be safe, it may also cause us to paper\nover issues where a ref store should have been set up. In our case for\nexample, it may be reasonable to expect that re-initializing the repo\nwill cause the \"onbranch:\" condition to trigger, but we would not do\nthat if the ref storage format was not set up yet. This also used to\nwork before the above commit that introduced this bug.\n\nRearrange the code such that we set up the repository format before\nreading the config. This fixes the bug and ensures that \"onbranch:\"\nconditions can trigger.\n\nReported-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nTested-by: Heghedus Razvan <heghedus.razvan@protonmail.com>\n[jc: backported to v2.44.0 codebase]\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * So this is how it should look like if we later wanted to merge\n   down the fix to the 2.44 maintenance track.\n\n setup.c         |  26 +++++++------\n t/t0001-init.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 108 insertions(+), 19 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex b69b1cbc2a..c6fffa0164 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2196,13 +2196,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t}\n \tstartup_info->have_repository = 1;\n \n-\t/* Ensure `core.hidedotfiles` is processed */\n-\tgit_config(platform_core_config, NULL);\n-\n-\tsafe_create_dir(git_dir, 0);\n-\n-\tprev_bare_repository = is_bare_repository();\n-\n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n \t * config file, so this will not fail.  What we are catching\n@@ -2213,10 +2206,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \tvalidate_ref_storage_format(&repo_fmt, ref_storage_format);\n \n-\treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, prev_bare_repository,\n-\t\t\t\t      init_shared_repository);\n-\n \t/*\n \t * Now that we have set up both the hash algorithm and the ref storage\n \t * format we can update the repository's settings accordingly.\n@@ -2224,6 +2213,21 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \trepo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);\n \n+\t/*\n+\t * Ensure `core.hidedotfiles` is processed. This must happen after we\n+\t * have set up the repository format such that we can evaluate\n+\t * includeIf conditions correctly in the case of re-initialization.\n+\t */\n+\tgit_config(platform_core_config, NULL);\n+\n+\tsafe_create_dir(git_dir, 0);\n+\n+\tprev_bare_repository = is_bare_repository();\n+\n+\treinit = create_default_files(template_dir, original_git_dir,\n+\t\t\t\t      &repo_fmt, prev_bare_repository,\n+\t\t\t\t      init_shared_repository);\n+\n \tif (!(flags & INIT_DB_SKIP_REFDB))\n \t\tcreate_reference_database(repo_fmt.ref_storage_format,\n \t\t\t\t\t  initial_branch, flags & INIT_DB_QUIET);\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex b131d665db..319ed81631 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -584,14 +584,39 @@ test_expect_success 'init with --ref-format=files' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 're-init with same format' '\n-\ttest_when_finished \"rm -rf refformat\" &&\n-\tgit init --ref-format=files refformat &&\n-\tgit init --ref-format=files refformat &&\n-\techo files >expect &&\n-\tgit -C refformat rev-parse --show-ref-format >actual &&\n-\ttest_cmp expect actual\n-'\n+backends=\"files\"\n+for from_format in $backends\n+do\n+\ttest_expect_success \"re-init with same format ($from_format)\" '\n+\t\ttest_when_finished \"rm -rf refformat\" &&\n+\t\tgit init --ref-format=$from_format refformat &&\n+\t\tgit init --ref-format=$from_format refformat &&\n+\t\techo $from_format >expect &&\n+\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\tfor to_format in $backends\n+\tdo\n+\t\tif test \"$from_format\" = \"$to_format\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\ttest_expect_success \"re-init with different format fails ($from_format -> $to_format)\" '\n+\t\t\ttest_when_finished \"rm -rf refformat\" &&\n+\t\t\tgit init --ref-format=$from_format refformat &&\n+\t\t\tcat >expect <<-EOF &&\n+\t\t\tfatal: attempt to reinitialize repository with different reference storage format\n+\t\t\tEOF\n+\t\t\ttest_must_fail git init --ref-format=$to_format refformat 2>err &&\n+\t\t\ttest_cmp expect err &&\n+\t\t\techo $from_format >expect &&\n+\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n \n test_expect_success 'init with --ref-format=garbage' '\n \ttest_when_finished \"rm -rf refformat\" &&\n@@ -678,4 +703,64 @@ test_expect_success 'branch -m with the initial branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit -c includeIf.onbranch:main.path=nonexistent init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'init with includeIf.onbranch condition with existing directory' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tmkdir repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init with includeIf.onbranch condition' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=/does/not/exist init repo &&\n+\techo $GIT_DEFAULT_REF_FORMAT >expect &&\n+\tgit -C repo rev-parse --show-ref-format >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-init skips non-matching includeIf.onbranch' '\n+\ttest_when_finished \"rm -rf repo config\" &&\n+\tcat >config <<-EOF &&\n+\t[\n+\tgarbage\n+\tEOF\n+\tgit init repo &&\n+\tgit -c includeIf.onbranch:nonexistent.path=\"$(test-tool path-utils absolute_path config)\" init repo\n+'\n+\n+test_expect_success 're-init reads matching includeIf.onbranch' '\n+\ttest_when_finished \"rm -rf repo config\" &&\n+\tcat >config <<-EOF &&\n+\t[\n+\tgarbage\n+\tEOF\n+\tpath=\"$(test-tool path-utils absolute_path config)\" &&\n+\tgit init --initial-branch=branch repo &&\n+\tcat >expect <<-EOF &&\n+\tfatal: bad config line 1 in file $path\n+\tEOF\n+\ttest_must_fail git -c includeIf.onbranch:branch.path=\"$path\" init repo 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.45.1-216-g4365c6fcf9\n\n"},{"id":"495342","messageId":"xmqqjzjlz6p6.fsf_-_@gitster.g","threadId":"61521","inReplyTo":"xmqqwmnlz7bf.fsf_-_@gitster.g","subject":"[rPATCH v2] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T00:56:53Z","receivedAt":"2024-05-23T00:56:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"And this is \"git show --remerge-diff\" output for the merge that\nmerges the result of applying the previous patch to v2.44.0 into\nv2.45.0-rc0~112 (which removed the 'prev_bare_repository' thing).\n\nThe resolution of conflicts in setup.c is trivial.  The change to\nt/t0001-init.sh is an evil one.\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 319ed81631..49e9bf77c6 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -584,7 +584,7 @@ test_expect_success 'init with --ref-format=files' '\n \ttest_cmp expect actual\n '\n \n-backends=\"files\"\n+backends=\"files reftable\"\n for from_format in $backends\n do\n \ttest_expect_success \"re-init with same format ($from_format)\" '\ndiff --git a/setup.c b/setup.c\nremerge CONFLICT (content): Merge conflict in setup.c\nindex 2f23ab6efa..e01462863b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2175,23 +2175,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t}\n \tstartup_info->have_repository = 1;\n \n-<<<<<<< 272fd9125a (Merge branch 'gt/core-bare-in-templates')\n-\t/* Ensure `core.hidedotfiles` is processed */\n-\tgit_config(platform_core_config, NULL);\n-\n-\tsafe_create_dir(git_dir, 0);\n-\n-\n-||||||| 3c2a3fdc38\n-\t/* Ensure `core.hidedotfiles` is processed */\n-\tgit_config(platform_core_config, NULL);\n-\n-\tsafe_create_dir(git_dir, 0);\n-\n-\tprev_bare_repository = is_bare_repository();\n-\n-=======\n->>>>>>> 7a998665dd (setup: fix bug with \"includeIf.onbranch\" when initializing dir)\n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n \t * config file, so this will not fail.  What we are catching\n@@ -2202,17 +2185,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \tvalidate_ref_storage_format(&repo_fmt, ref_storage_format);\n \n-<<<<<<< 272fd9125a (Merge branch 'gt/core-bare-in-templates')\n-\treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, init_shared_repository);\n-\n-||||||| 3c2a3fdc38\n-\treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, prev_bare_repository,\n-\t\t\t\t      init_shared_repository);\n-\n-=======\n->>>>>>> 7a998665dd (setup: fix bug with \"includeIf.onbranch\" when initializing dir)\n \t/*\n \t * Now that we have set up both the hash algorithm and the ref storage\n \t * format we can update the repository's settings accordingly.\n@@ -2229,11 +2201,8 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \n \tsafe_create_dir(git_dir, 0);\n \n-\tprev_bare_repository = is_bare_repository();\n-\n \treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, prev_bare_repository,\n-\t\t\t\t      init_shared_repository);\n+\t\t\t\t      &repo_fmt, init_shared_repository);\n \n \tif (!(flags & INIT_DB_SKIP_REFDB))\n \t\tcreate_reference_database(repo_fmt.ref_storage_format,\n"},{"id":"495351","messageId":"Zk7TdDRFAuwQMBHy@tanuki","threadId":"61521","inReplyTo":"xmqqwmnlz7bf.fsf_-_@gitster.g","subject":"Re: [PATCH v2] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-23T05:26:12Z","receivedAt":"2024-05-23T05:26:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 22, 2024 at 05:43:32PM -0700, Junio C Hamano wrote:\n[snip]\n> diff --git a/setup.c b/setup.c\n> index b69b1cbc2a..c6fffa0164 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -2224,6 +2213,21 @@ int init_db(const char *git_dir, const char *real_git_dir,\n>  \trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n>  \trepo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);\n>  \n> +\t/*\n> +\t * Ensure `core.hidedotfiles` is processed. This must happen after we\n> +\t * have set up the repository format such that we can evaluate\n> +\t * includeIf conditions correctly in the case of re-initialization.\n> +\t */\n> +\tgit_config(platform_core_config, NULL);\n> +\n> +\tsafe_create_dir(git_dir, 0);\n> +\n> +\tprev_bare_repository = is_bare_repository();\n\nOkay, `prev_bare_repository` is another difference between v2.44 and\nmaster.\n\n> +\treinit = create_default_files(template_dir, original_git_dir,\n> +\t\t\t\t      &repo_fmt, prev_bare_repository,\n> +\t\t\t\t      init_shared_repository);\n> +\n>  \tif (!(flags & INIT_DB_SKIP_REFDB))\n>  \t\tcreate_reference_database(repo_fmt.ref_storage_format,\n>  \t\t\t\t\t  initial_branch, flags & INIT_DB_QUIET);\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index b131d665db..319ed81631 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -584,14 +584,39 @@ test_expect_success 'init with --ref-format=files' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success 're-init with same format' '\n> -\ttest_when_finished \"rm -rf refformat\" &&\n> -\tgit init --ref-format=files refformat &&\n> -\tgit init --ref-format=files refformat &&\n> -\techo files >expect &&\n> -\tgit -C refformat rev-parse --show-ref-format >actual &&\n> -\ttest_cmp expect actual\n> -'\n> +backends=\"files\"\n> +for from_format in $backends\n> +do\n> +\ttest_expect_success \"re-init with same format ($from_format)\" '\n> +\t\ttest_when_finished \"rm -rf refformat\" &&\n> +\t\tgit init --ref-format=$from_format refformat &&\n> +\t\tgit init --ref-format=$from_format refformat &&\n> +\t\techo $from_format >expect &&\n> +\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n\nAnd this here is the change to the test setup, which makes sense.\n\nLooks sensible, thanks!\n\nPatrick\n"},{"id":"495352","messageId":"Zk7Tej-AII3Ge6Ge@tanuki","threadId":"61521","inReplyTo":"xmqq4jap1hs7.fsf@gitster.g","subject":"Re: [PATCH] setup: fix bug with \"includeIf.onbranch\" when initializing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-23T05:26:18Z","receivedAt":"2024-05-23T05:26:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 22, 2024 at 05:41:28PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > +for from_format in files reftable\n> > +do\n> > +\tfor to_format in files reftable\n> > +\tdo\n> > +\t\tif test \"$from_format\" = \"$to_format\"\n> > +\t\tthen\n> > +\t\t\tcontinue\n> > +\t\tfi\n> > +\n> > +\t\ttest_expect_success \"re-init with same format ($from_format)\" '\n> > +\t\t\ttest_when_finished \"rm -rf refformat\" &&\n> > +\t\t\tgit init --ref-format=$from_format refformat &&\n> > +\t\t\tgit init --ref-format=$from_format refformat &&\n> > +\t\t\techo $from_format >expect &&\n> > +\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n> > +\t\t\ttest_cmp expect actual\n> > +\t\t'\n> \n> This is very iffy, isn't it?  This one wants to do the same format\n> reinit, but it is behind \"if from and to are the same, skip the rest\"\n> in the loop.\n> \n> I think the \"same format\" loop should be lifted outside and\n> immediately before the inner loop and we should be OK.\n\nYup, we can do that.\n\n> > +\t\ttest_expect_success \"re-init with different format fails ($from_format -> $to_format)\" '\n> > +\t\t\ttest_when_finished \"rm -rf refformat\" &&\n> > +\t\t\tgit init --ref-format=$from_format refformat &&\n> > +\t\t\tcat >expect <<-EOF &&\n> > +\t\t\tfatal: attempt to reinitialize repository with different reference storage format\n> > +\t\t\tEOF\n> > +\t\t\ttest_must_fail git init --ref-format=$to_format refformat 2>err &&\n> > +\t\t\ttest_cmp expect err &&\n> > +\t\t\techo $from_format >expect &&\n> > +\t\t\tgit -C refformat rev-parse --show-ref-format >actual &&\n> > +\t\t\ttest_cmp expect actual\n> > +\t\t'\n> > +\tdone\n> > +done\n> \n> In any case, this \"reinitialize to a different format is too late\"\n> test has nothing to do with the problem we are fixing with this\n> patch, no?  It is a valuable set of tests in the post b4385bf0 world\n> where we can choose between the two, but it is somewhat out of scope\n> of the \"we need to initialize the ref backend before we can do onbranch\n> config\".\n> \n> I'll send your patch backported to v2.44.0, plus the change needed\n> to review the merge of it into v2.45.0 codebase later.\n\nYeah, I wanted to play it safe and add some cases I felt were missing in\nthe existing test coverage so that there ideally aren't any other issues\nand so that the proposed change doesn't regress functionality.\n\nPatrick\n"}]}