Re: [PATCH] setup: tweak upgrade policy to grandfather worktreeconfig

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 13, 2020, 20:46 UTC
Message-ID: <b8ae18f7-a0af-bd62-2553-92989e1dbe7e@gmail.com>
In-Reply-To: <xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com>

On 7/13/2020 4:18 PM, Junio C Hamano wrote:

Show 14 quoted lines

> The "fail and warn" approach introduced in the previous step may be
> with smaller impact to the codebase, but
> 
>  - it requires the end-user to read, understand and execute the
>    manual upgrade
> 
>  - it encourages to follow the same procedure blindly, making the
>    protection even less useful
> 
> Let's instead keep failing hard without teaching how to bypass the
> repository protection, but allow upgrading even when only the
> worktreeconfig extension exists in an old repository, which is
> likely to be set by a broke version of Git that did not update the
> repository version when setting the extension.

This is a more subtle way to handle the case. In fact, it silently makes extensions.worktreeConfig work as it did in 2.27, which means users will not have any troubles after upgrading.

I also like that you are actually fixing the case where a user in the bad state _can_ get out using "git sparse-checkout init".

This can be verified by adding this test:

test_expect_success 'git sparse-checkout works if repository format is wrong' '
	test_when_finished git -C repo config core.repositoryFormatVersion 1 &&
	git -C repo sparse-checkout init &&
	git -C repo config core.repositoryFormatVersion >actual &&
	echo 1 >expect &&
	test_cmp expect actual &&
	git -C repo config core.repositoryFormatVersion 0 &&
	git -C repo sparse-checkout init &&
	git -C repo config core.repositoryFormatVersion >actual &&
	test_cmp expect actual
'

Show 7 quoted lines

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * This time with a log message to explain and justify the change,
>    with a matching tweak to the test script, designed to be applied
>    on top, but feel free to squash it in if you agree with me that
>    we do not need two separate commits for this.

Since this commit removes all evidence of the previous one, I would recommend just squashing them together.

Thanks for your fast feedback! -Stolee