setup: warn if extensions exist on old format
diff mbox series

Message ID pull.674.git.1594668051847.gitgitgadget@gmail.com
State New
Headers show
Series
  • setup: warn if extensions exist on old format
Related show

Commit Message

Junio C Hamano via GitGitGadget July 13, 2020, 7:20 p.m. UTC
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
the version is unset but extensions exist.

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.

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!
    
    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(+)


base-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd

Comments

Taylor Blau July 13, 2020, 7:34 p.m. UTC | #1
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
Junio C Hamano July 13, 2020, 7:36 p.m. UTC | #2
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

s/builting/command/ perhaps???

>
> 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
> the version is unset but extensions exist.
>
> 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/ocn/con/

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---

Thanks for a thoughtful analysis of the situation and coming up with
a plan to remedy.

> +	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'"));
> +	}

That reads well.

An alternative may be to grandfather some extensions that were
enabled by git by mistake without updating the format version, and
we update the repository even if the repository has extensions that
should not exist, but those offending extensions are limited only to
those that we decide to special case.  That would make the end-user
experience even smoother.

Is extenions.worktreeCOnfig the only one that needs this escape
hatch?
Derrick Stolee July 13, 2020, 7:41 p.m. UTC | #3
On 7/13/2020 3:34 PM, Taylor Blau wrote:
> s/upgrae/upgrade
> s/ocnfigured/configured

Oh man, what was going on with me when I was typing this message.

Thanks for the thorough inspection.

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

You were absolutely right to check this, since this diff causes
the test to fail:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index d249428f44..b5de141292 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -74,6 +74,9 @@ test_expect_success 'warning about core.repositoryFormatVersion' '
        test_must_be_empty err &&
        git -C repo config --local core.repositoryFormatVersion 0 &&
        git -C repo status 2>err &&
+       test_i18ngrep "some extensions are enabled, but core.repositoryFormatVersion=0" err &&
+       git -C repo config --local --unset core.repositoryFormatVersion 0 &&
+       git -C repo status 2>err &&
        test_i18ngrep "some extensions are enabled, but core.repositoryFormatVersion=0" err
 '
 

I'll investigate why the "unset" case is different than the "0" case.

Hopefully the "should we do this?" question can continue being discussed
while I work on a v2.

Thanks,
-Stolee
Junio C Hamano July 13, 2020, 8 p.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> An alternative may be to grandfather some extensions that were
> enabled by git by mistake without updating the format version, and
> we update the repository even if the repository has extensions that
> should not exist, but those offending extensions are limited only to
> those that we decide to special case.  That would make the end-user
> experience even smoother.
>
> Is extenions.worktreeCOnfig the only one that needs this escape
> hatch?

Assuming that worktreeconfig is the only thing, the change may look
like this.  With this change, we might want to drop the new warning
in hunk ll.542- to avoid encouraging people to muck with their
repository with random configuration variables that happen to share
extensions.* prefix with us.

 cache.h |  2 +-
 setup.c | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index e5885cc9ea..8ff46857f6 100644
--- a/cache.h
+++ b/cache.h
@@ -1042,8 +1042,8 @@ struct repository_format {
 	int worktree_config;
 	int is_bare;
 	int hash_algo;
-	int has_extensions;
 	char *work_tree;
+	int has_unallowed_extensions;
 	struct string_list unknown_extensions;
 };
 
diff --git a/setup.c b/setup.c
index eb066db6d8..5f4786d3b9 100644
--- a/setup.c
+++ b/setup.c
@@ -455,12 +455,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 	if (strcmp(var, "core.repositoryformatversion") == 0)
 		data->version = git_config_int(var, value);
 	else if (skip_prefix(var, "extensions.", &ext)) {
-		data->has_extensions = 1;
 		/*
 		 * record any known extensions here; otherwise,
 		 * we fall through to recording it as unknown, and
 		 * check_repository_format will complain
 		 */
+		int is_unallowed_extension = 1;
+
 		if (!strcmp(ext, "noop"))
 			;
 		else if (!strcmp(ext, "preciousobjects"))
@@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 			if (!value)
 				return config_error_nonbool(var);
 			data->partial_clone = xstrdup(value);
-		} else if (!strcmp(ext, "worktreeconfig"))
+		} else if (!strcmp(ext, "worktreeconfig")) {
 			data->worktree_config = git_config_bool(var, value);
-		else
+			is_unallowed_extension = 0;
+		} else
 			string_list_append(&data->unknown_extensions, ext);
+
+		data->has_unallowed_extensions |= is_unallowed_extension;
 	}
 
 	return read_worktree_config(var, value, vdata);
@@ -542,6 +546,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 		}
 	}
 
+	if (candidate->version == 0 && candidate->has_unallowed_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;
 }
 
@@ -560,7 +569,7 @@ int upgrade_repository_format(int target_version)
 		return 0;
 
 	if (verify_repository_format(&repo_fmt, &err) < 0 ||
-	    (!repo_fmt.version && repo_fmt.has_extensions)) {
+	    (!repo_fmt.version && repo_fmt.has_unallowed_extensions)) {
 		warning("unable to upgrade repository format from %d to %d: %s",
 			repo_fmt.version, target_version, err.buf);
 		strbuf_release(&err);

Patch
diff mbox series

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