Re: [PATCH] setup: fix bug with "includeIf.onbranch" when initializing dir
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 23, 2024, 00:41 UTC
- Message-ID
- <xmqq4jap1hs7.fsf@gitster.g>
- In-Reply-To
- <cf182bb9ee7d4a7eb46e5dbf4f3ef5deb198d823.1716374321.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> The bug has been introduced in 173761e21b (setup: start tracking ref > storage format, 2023-12-29), which wired up the ref storage format. The > root cause is in `init_db()`, which tries to read the config before we > have initialized `the_repository` and most importantly its ref storage > format. We eventually end up calling `include_by_branch()` and execute > `refs_resolve_ref_unsafe()`, but because we have not initialized the ref > storage format yet this will trigger the above bug.
So I was trying to see how feasible it would be to backport this fix on top of v2.44.0 which was the first version that was shipped with 173761e2 (setup: start tracking ref storage format, 2023-12-29), which is v2.44.0-rc0~78^2~8 and noticed something curious.
As your addition to t0001-init.sh wants to try converting the ref backends to and from files and reftable, it actually will barf until b4385bf0 (Merge branch 'ps/reftable-backend', 2024-02-26), which is v2.45.0-rc0~176, but even with a reinit with the same backend, the problem should manifest itself, right?
Show 16 quoted lines
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh > index b131d665db..2239bed198 100755 > --- a/t/t0001-init.sh > +++ b/t/t0001-init.sh > @@ -584,14 +584,38 @@ test_expect_success 'init with --ref-format=files' ' > test_cmp expect actual > ' > > -test_expect_success 're-init with same format' ' > - test_when_finished "rm -rf refformat" && > - git init --ref-format=files refformat && > - git init --ref-format=files refformat && > - echo files >expect && > - git -C refformat rev-parse --show-ref-format >actual && > - test_cmp expect actual > -'
So we used to have "files to files" and nothing else. But
Show 17 quoted lines
> +for from_format in files reftable > +do > + for to_format in files reftable > + do > + if test "$from_format" = "$to_format" > + then > + continue > + fi > + > + test_expect_success "re-init with same format ($from_format)" ' > + test_when_finished "rm -rf refformat" && > + git init --ref-format=$from_format refformat && > + git init --ref-format=$from_format refformat && > + echo $from_format >expect && > + git -C refformat rev-parse --show-ref-format >actual && > + test_cmp expect actual > + '
This is very iffy, isn't it? This one wants to do the same format reinit, but it is behind "if from and to are the same, skip the rest" in the loop.
I think the "same format" loop should be lifted outside and immediately before the inner loop and we should be OK.
Show 14 quoted lines
> + test_expect_success "re-init with different format fails ($from_format -> $to_format)" ' > + test_when_finished "rm -rf refformat" && > + git init --ref-format=$from_format refformat && > + cat >expect <<-EOF && > + fatal: attempt to reinitialize repository with different reference storage format > + EOF > + test_must_fail git init --ref-format=$to_format refformat 2>err && > + test_cmp expect err && > + echo $from_format >expect && > + git -C refformat rev-parse --show-ref-format >actual && > + test_cmp expect actual > + ' > + done > +done
In any case, this "reinitialize to a different format is too late" test has nothing to do with the problem we are fixing with this patch, no? It is a valuable set of tests in the post b4385bf0 world where we can choose between the two, but it is somewhat out of scope of the "we need to initialize the ref backend before we can do onbranch config".
I'll send your patch backported to v2.44.0, plus the change needed to review the merge of it into v2.45.0 codebase later.