Re: [PATCH] setup: warn if extensions exist on old format

[Taylor Blau <me@ttaylorr.com>]

On Mon, Jul 13, 2020 at 07:20:51PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Prior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions
> for old repositories, 2020-06-05), Git was honoring configured
> extensions, even if core.repositoryFormatVersion was 0 (or unset). This
> was incorrect, and is now fixed.
>
> The issue now is that users who relied on that previously bad behavior
> will upgrade to the next version of Git and suddently be in a bad
> situation. In particular, users of the 'git sparse-checkout' builting
> will rely on the extensions.worktreeConfig for the core.sparseCheckout
> and core.sparseCheckoutCone config options. Without that extension,
> these users will suddenly have repositories stop acting like a sparse
> repo.
>
> What is worse is that a user will be confronted with the following
> error if they try to run 'git sparse-checkout init' again:
>
> 	warning: unable to upgrade repository format from 0 to 1
>
> This is because the logic in 14c7fa269e4 refuses to upgrae repos when

s/upgrae/upgrade

> the version is unset but extensions exist.

I'm not sure that I want to get into a discussion about whether or not
this logic is right while -rc0 is already out, but it seems like
14c7fa269e4 could be tweaked to be a little more tolerant of this case.

On the other hand, I think that the approach here is perfectly sensible,
absent of a more invasive change to the logic in 14c7fa269e4.

> While it is important to correct this improper behavior, create a
> warning so users in this situation can correct themselves without too
> much guesswork. By creating a warning in
> check_repository_format_gently(), we can alert the user if they have a
> ocnfigured extension but not a configured repository version.

s/ocnfigured/configured

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     setup: warn if extensions exist on old format
>
>     This is based on xl/upgrade-repo-format.
>
>     Can this be considered for v2.28.0-rc1? I think this is an important
>     shift in behavior that will disrupt many users if they upgrade without
>     it!

I would be happy to see something like this in -rc1, unless Junio has
reservations about making this change at this point in the release cycle
(I do not have any such concerns).

>     If not this warning or change, then how else can we help users who are
>     in this situation? How can we save those who adopted the sparse-checkout
>     builtin in recent versions from terrible post-upgrade headaches?
>
>     Thanks, -Stolee
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-674%2Fderrickstolee%2Fsparse-checkout-warning-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-674/derrickstolee/sparse-checkout-warning-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/674
>
>  setup.c                            | 5 +++++
>  t/t1091-sparse-checkout-builtin.sh | 9 +++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/setup.c b/setup.c
> index eb066db6d8..6ff6c54d39 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -542,6 +542,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
>  		}
>  	}
>
> +	if (candidate->version == 0 && candidate->has_extensions) {
> +		warning(_("some extensions are enabled, but core.repositoryFormatVersion=0"));
> +		warning(_("if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'"));
> +	}
> +
>  	return 0;
>  }
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 88cdde255c..d249428f44 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -68,6 +68,15 @@ test_expect_success 'git sparse-checkout init' '
>  	check_files repo a
>  '
>
> +test_expect_success 'warning about core.repositoryFormatVersion' '
> +	test_when_finished git -C repo config core.repositoryFormatVersion 1 &&
> +	git -C repo status 2>err &&
> +	test_must_be_empty err &&
> +	git -C repo config --local core.repositoryFormatVersion 0 &&

I don't think it's that difficult to see that this patch is correct, but
it might be worth testing this for the case that
'core.repositoryFormatVersion' is unset, too.

> +	git -C repo status 2>err &&
> +	test_i18ngrep "some extensions are enabled, but core.repositoryFormatVersion=0" err
> +'
> +
>  test_expect_success 'git sparse-checkout list after init' '
>  	git -C repo sparse-checkout list >actual &&
>  	cat >expect <<-\EOF &&
>
> base-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd
> --
> gitgitgadget

Thanks,
Taylor