Re: [PATCH] setup: warn about un-enabled extensions

From

Junio C Hamano <gitster@pobox.com>

Date

Jul 15, 2020, 16:09 UTC

Message-ID

<xmqqpn8wkben.fsf@gitster.c.googlers.com>

In-Reply-To

<xmqqzh82ktgm.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> If you don't mind, I was already going to squash Junio's commit into
>>> mine (almost completely replacing mine) but I could add a small
>>> commit on top that provides the following improvement to the error
>>> message:
>>
>> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing
>> users in the same was as v2.28.0-rc0 confused me.
>
> In a nearby thread, Jonathan Nieder raised an interesting approach
> to avoid confusing users, which I think (if I am reading him
> correctly) makes sense (cf. <20200714040616.GA2208896@google.com>)
>
> What if we accept the extensions the code before the topic in
> question that was merged in -rc0 introduced the "confusion" accepts
> even in v0?  If we see extensions other than those handpicked and
> grandfathered ones (which are presumably the ones we add later and
> support in v1 and later repository versions) in a v0 repository, we
> keep ignoring.  Also we'd loosen the overly strict code that
> prevents upgrading from v0 to v1 in the presence of any extensions
> in -rc0, so that the grandfathered ones will not prevent the
> upgrading.
>
> The original reasoning behind the strict check was because the users
> could have used extensions.frotz for their own use with their own
> meaning, trusting that Git would simply ignore it, and an upgrade to
> later version in which Git uses extensions.frotz for a purpose that
> is unrelated to the reason why these users used would just break the
> repository.  
>
> But the ones that were (accidentally) honored in v0 couldn't have
> been used by the users for the purposes other than how Git would use
> them anyway, so there is no point to make them prevent the upgrade
> of the repository version from v0 to v1.
>
> At least, that is how I understood the world would look like in
> Jonathan's "different endgame".
>
> What do you three (Dscho, Derrick and Jonathan) think?  

So...

Thanks.

 setup.c                    | 52 +++++++++++++++++++++++++++-------------------
 t/t0410-partial-clone.sh   | 14 ++++++++++++-
 t/t2404-worktree-config.sh |  2 +-
 3 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/setup.c b/setup.c
index 65270440a9..fe4e1ec066 100644
--- a/setup.c
+++ b/setup.c
@@ -455,28 +455,37 @@ 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)) {
+		int unallowed_in_v0 = 1;
+
 		/*
-		 * record any known extensions here; otherwise,
-		 * we fall through to recording it as unknown, and
-		 * check_repository_format will complain
+		 * The early ones are grandfathered---they existed in
+		 * 2.27 which mistakenly honored even in repositories
+		 * whose version is before v1 (where extensions are
+		 * officially introduced).
 		 */
-		int is_unallowed_extension = 1;
-
-		if (!strcmp(ext, "noop"))
-			;
-		else if (!strcmp(ext, "preciousobjects"))
+		if (!strcmp(ext, "noop")) {
+			unallowed_in_v0 = 0;
+		} else if (!strcmp(ext, "preciousobjects")) {
 			data->precious_objects = git_config_bool(var, value);
-		else if (!strcmp(ext, "partialclone")) {
+			unallowed_in_v0 = 0;
+		} else if (!strcmp(ext, "partialclone")) {
 			if (!value)
 				return config_error_nonbool(var);
 			data->partial_clone = xstrdup(value);
+			unallowed_in_v0 = 0;
 		} else if (!strcmp(ext, "worktreeconfig")) {
 			data->worktree_config = git_config_bool(var, value);
-			is_unallowed_extension = 0;
-		} else
+			unallowed_in_v0 = 0;
+		/*
+		 * Extensions are added by more "} else if (...) {"
+		 * lines here, but do NOT mark them as allowed in v0
+		 * by copy-pasting without thinking.
+		 */
+		} else {
 			string_list_append(&data->unknown_extensions, ext);
+		}
 
-		data->has_unallowed_extensions |= is_unallowed_extension;
+		data->has_unallowed_extensions |= unallowed_in_v0;
 	}
 
 	return read_worktree_config(var, value, vdata);
@@ -511,15 +520,16 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 		die("%s", err.buf);
 	}
 
-	if (candidate->version >= 1) {
-		repository_format_precious_objects = candidate->precious_objects;
-		set_repository_format_partial_clone(candidate->partial_clone);
-		repository_format_worktree_config = candidate->worktree_config;
-	} else {
-		repository_format_precious_objects = 0;
-		set_repository_format_partial_clone(NULL);
-		repository_format_worktree_config = 0;
-	}
+	/*
+	 * Now we know the extensions in "candidate" repository are
+	 * OK, let's copy them to the final place.  Note that this is
+	 * done even in v0 repositories, as long as the extensions are
+	 * the grandfathered ones that used to be honored by mistake.
+	 */
+	repository_format_precious_objects = candidate->precious_objects;
+	set_repository_format_partial_clone(candidate->partial_clone);
+	repository_format_worktree_config = candidate->worktree_config;
+
 	string_list_clear(&candidate->unknown_extensions, 0);
 
 	if (repository_format_worktree_config) {
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be..2fc2d0bbfc 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '
 	test_cmp_config -C client 1 core.repositoryformatversion
 '
 
-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '
+test_expect_success 'convert shallow clone to partial clone succeeds with grandfathered extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
@@ -50,6 +50,18 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e
 	git clone --depth=1 "file://$(pwd)/server" client &&
 	test_cmp_config -C client 0 core.repositoryformatversion &&
 	git -C client config extensions.partialclone origin &&
+	git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_failure 'convert shallow clone to partial clone must fail with unknown extension' '
+	rm -fr server client &&
+	test_create_repo server &&
+	test_commit -C server my_commit 1 &&
+	test_commit -C server my_commit2 1 &&
+	git clone --depth=1 "file://$(pwd)/server" client &&
+	test_cmp_config -C client 0 core.repositoryformatversion &&
+	git -C client config extensions.unknownExtension true &&
+	git -C client config extensions.partialclone origin &&
 	test_must_fail git -C client fetch --unshallow --filter="blob:none"
 '
 
diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh
index 303a2644bd..b8c12df534 100755
--- a/t/t2404-worktree-config.sh
+++ b/t/t2404-worktree-config.sh
@@ -77,7 +77,7 @@ test_expect_success 'config.worktree no longer read without extension' '
 	test_cmp_config -C wt1 shared this.is &&
 	test_cmp_config -C wt2 shared this.is
 '
-test_expect_success 'show advice when extensions.* are not enabled' '
+test_expect_failure 'show advice when extensions.* are not enabled' '
 	test_config core.repositoryformatversion 1 &&
 	test_config extensions.worktreeConfig true &&
 	git config --worktree test.one true &&