Re: [PATCH] setup: fix bug with "includeIf.onbranch" when initializing dir

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

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

> 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

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

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