setup: tweak upgrade policy to grandfather worktreeconfig
diff mbox series

Message ID xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com
State New
Headers show
Series
  • setup: tweak upgrade policy to grandfather worktreeconfig
Related show

Commit Message

Junio C Hamano July 13, 2020, 8:18 p.m. UTC
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.

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.

   Thanks.

 cache.h                            |  2 +-
 setup.c                            | 17 ++++++++---------
 t/t1091-sparse-checkout-builtin.sh |  9 ---------
 3 files changed, 9 insertions(+), 19 deletions(-)

Comments

Derrick Stolee July 13, 2020, 8:46 p.m. UTC | #1
On 7/13/2020 4:18 PM, Junio C Hamano wrote:
> 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
'

> 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
Junio C Hamano July 13, 2020, 9:45 p.m. UTC | #2
Derrick Stolee <stolee@gmail.com> writes:

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

Alright, then care to do the honors ;-)?  Let's make sure we have it
in -rc1 to avoid nasty "regression" in the upcoming release.

Thanks for raising the issue and exploring the solution space.
Jonathan Nieder July 14, 2020, 4:06 a.m. UTC | #3
Hi,

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

>> 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'd like to propose a different endgame:

Instead of looking at `extensions.*` settings one by one to see how
various implementations handled them with repositoryFormatVersion=0,
what if we treat repositoryFormatVersion=0 as a synonym of version=1?

That is:

1. in new repositories, set repositoryFormatVersion = 1, since (1) Git
   versions in the wild should all already know how to handle it and
   (2) as we've learned, other Git implementations need to understand
   some of extensions.* anyway

2. whenever setting any extensions.*, automatically upgrade to
   repositoryFormatVersion = 1

3. when in an existing repository with extensions.* set and
   repositoryFormatVersion = 0, act as though repositoryFormatVersion = 1

4. document this new behavior with repositoryFormatVersion = 0 in
   Documentation/technical/repository-version.txt

This way, the result is simpler than where we started.

Unfortunately, we are not even that consistent about what to do with
extensions.* settings when repositoryFormatVersion = 0 today.  So
we'll have to exercise some care and analyze them one by one to make
sure this is safe (and if it isn't, at *that* point we'd come up with
a more complex variant on (2) and (3) above).

It's too late to go that far for 2.28.  It would be tempting to try a
simple revert of 14c7fa269e4 (check_repository_format_gently(): refuse
extensions for old repositories, 2020-06-05) to get back to tried and
true behavior but that does not do enough --- it still produces an
error when trying to upgrade repository format when any extensions are
set.  So how about such a revert plus Junio's patch plus the analogous
change to Junio's patch for

  extensions.preciousObjects
  extensions.partialClone

?

Thanks,
Jonathan
Junio C Hamano July 14, 2020, 4:27 a.m. UTC | #4
Jonathan Nieder <jrnieder@gmail.com> writes:

> It's too late to go that far for 2.28.  It would be tempting to try a
> simple revert of 14c7fa269e4 (check_repository_format_gently(): refuse
> extensions for old repositories, 2020-06-05) to get back to tried and
> true behavior but that does not do enough --- it still produces an
> error when trying to upgrade repository format when any extensions are
> set.  So how about such a revert plus Junio's patch plus the analogous
> change to Junio's patch for
>
>   extensions.preciousObjects
>   extensions.partialClone

My illustration patch was done "assuming that worktreeconfig is the
only thing we wrote by mistake without updating the format version",
and if these two also share the same problem, I obviously is 100%
fine with covering these other ones with the same approach.

I like your "v0 and v1 are the same, but the repository is declared
to be corrupt if the extentions that are not known by today's code
is found in v0 repository", by the way.  Assuming that the two you
listed above plus worktreeconfig are the only ones known by today's
code, that is.  We seem to also know about "noop", so shouldn't it
also be grandfathered in?

Patch
diff mbox series

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 6ff6c54d39..65270440a9 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,11 +546,6 @@  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;
 }
 
@@ -565,7 +564,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);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index d249428f44..88cdde255c 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -68,15 +68,6 @@  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 &&