setup: warn about un-enabled extensions
diff mbox series

Message ID pull.675.git.1594677321039.gitgitgadget@gmail.com
State New
Headers show
Series
  • setup: warn about un-enabled extensions
Related show

Commit Message

Elijah Newren via GitGitGadget July 13, 2020, 9:55 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When any `extensions.*` setting is configured, we newly ignore it unless
`core.repositoryFormatVersion` is set to a positive value.

This might be quite surprising, e.g. when calling `git config --worktree
[...]` elicits a warning that it requires
`extensions.worktreeConfig = true` when that setting _is_ configured
(but ignored because `core.repositoryFormatVersion` is unset).

Let's warn about this situation specifically, especially because there
might be already setups out there that configured a sparse worktree
using Git v2.27.0 (which does set `extensions.worktreeConfig` but not
`core.repositoryFormatVersion`) and users might want to work in those
setups with Git v2.28.0, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Warn when extensions.* is ignored
    
    I did actually run into this today. One of my pipelines is configured to
    clone a bare repository, then set up a sparse secondary worktree. This
    used to work, but all of a sudden, the git config --worktree
    core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-675%2Fdscho%2Frepo-format-version-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-675/dscho/repo-format-version-advice-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/675

 cache.h                    |  2 +-
 setup.c                    | 16 +++++++++++++++-
 t/t2404-worktree-config.sh | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)


base-commit: bd42bbe1a46c0fe486fc33e82969275e27e4dc19

Comments

Junio C Hamano July 13, 2020, 10:48 p.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>     I did actually run into this today. One of my pipelines is configured to
>     clone a bare repository, then set up a sparse secondary worktree. This
>     used to work, but all of a sudden, the git config --worktree
>     core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.

I guess a few people were independently hit and then approached the
same issue from different angles?  If so, can you two compare notes
to help us all come up with a single good solution, preferrably by
-rc1?

Thanks.
Derrick Stolee July 14, 2020, 12:24 a.m. UTC | #2
On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When any `extensions.*` setting is configured, we newly ignore it unless
> `core.repositoryFormatVersion` is set to a positive value.
> 
> This might be quite surprising, e.g. when calling `git config --worktree
> [...]` elicits a warning that it requires
> `extensions.worktreeConfig = true` when that setting _is_ configured
> (but ignored because `core.repositoryFormatVersion` is unset).
> 
> Let's warn about this situation specifically, especially because there
> might be already setups out there that configured a sparse worktree
> using Git v2.27.0 (which does set `extensions.worktreeConfig` but not
> `core.repositoryFormatVersion`) and users might want to work in those
> setups with Git v2.28.0, too.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     Warn when extensions.* is ignored
>     
>     I did actually run into this today. One of my pipelines is configured to
>     clone a bare repository, then set up a sparse secondary worktree. This
>     used to work, but all of a sudden, the git config --worktree
>     core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.

I tried your situation with Junio's patch from earlier [1] [2].

[1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/	

The issue here is that Junio's silent fix for sparse-checkout doesn't
work here for "git config --worktree". However, I think that Johannes
is making the same over-compensating warning message pattern as I was.
That is, this warning happens for all extensions that are enabled when
core.repositoryFormatVersion is less than 1.

To attempt to summarize Junio's opinion, we should keep our situation
isolated to this worktree config extension. Your patch does agree with
the others in that we don't revert the behavior of failing to set the
config, but I think in this instance we can specify the warning more
carefully.

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:

diff --git a/builtin/config.c b/builtin/config.c
index 5e39f618854..b5de7982a93 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
                else if (worktrees[0] && worktrees[1])
                        die(_("--worktree cannot be used with multiple "
                              "working trees unless the config\n"
-                             "extension worktreeConfig is enabled. "
-                             "Please read \"CONFIGURATION FILE\"\n"
+                             "extension worktreeConfig is enabled "
+                             "and core.repositoryFormatVersion is at least\n"
+                             "1. Please read \"CONFIGURATION FILE\""
                              "section in \"git help worktree\" for details"));
                else
                        given_config_source.file = git_pathdup("config");

Thanks,
-Stolee
Johannes Schindelin July 14, 2020, 12:21 p.m. UTC | #3
Hi Stolee,

On Mon, 13 Jul 2020, Derrick Stolee wrote:

> On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When any `extensions.*` setting is configured, we newly ignore it unless
> > `core.repositoryFormatVersion` is set to a positive value.
> >
> > This might be quite surprising, e.g. when calling `git config --worktree
> > [...]` elicits a warning that it requires
> > `extensions.worktreeConfig = true` when that setting _is_ configured
> > (but ignored because `core.repositoryFormatVersion` is unset).
> >
> > Let's warn about this situation specifically, especially because there
> > might be already setups out there that configured a sparse worktree
> > using Git v2.27.0 (which does set `extensions.worktreeConfig` but not
> > `core.repositoryFormatVersion`) and users might want to work in those
> > setups with Git v2.28.0, too.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >     Warn when extensions.* is ignored
> >
> >     I did actually run into this today. One of my pipelines is configured to
> >     clone a bare repository, then set up a sparse secondary worktree. This
> >     used to work, but all of a sudden, the git config --worktree
> >     core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.
>
> I tried your situation with Junio's patch from earlier [1] [2].
>
> [1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/
> [2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/
>
> The issue here is that Junio's silent fix for sparse-checkout doesn't
> work here for "git config --worktree". However, I think that Johannes
> is making the same over-compensating warning message pattern as I was.
> That is, this warning happens for all extensions that are enabled when
> core.repositoryFormatVersion is less than 1.
>
> To attempt to summarize Junio's opinion, we should keep our situation
> isolated to this worktree config extension. Your patch does agree with
> the others in that we don't revert the behavior of failing to set the
> config, but I think in this instance we can specify the warning more
> carefully.

Okay.

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

Thanks,
Dscho

>
> diff --git a/builtin/config.c b/builtin/config.c
> index 5e39f618854..b5de7982a93 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>                 else if (worktrees[0] && worktrees[1])
>                         die(_("--worktree cannot be used with multiple "
>                               "working trees unless the config\n"
> -                             "extension worktreeConfig is enabled. "
> -                             "Please read \"CONFIGURATION FILE\"\n"
> +                             "extension worktreeConfig is enabled "
> +                             "and core.repositoryFormatVersion is at least\n"
> +                             "1. Please read \"CONFIGURATION FILE\""
>                               "section in \"git help worktree\" for details"));
>                 else
>                         given_config_source.file = git_pathdup("config");
>
> Thanks,
> -Stolee
>
>
Junio C Hamano July 14, 2020, 3:27 p.m. UTC | #4
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?  



> Thanks,
> Dscho
>
>>
>> diff --git a/builtin/config.c b/builtin/config.c
>> index 5e39f618854..b5de7982a93 100644
>> --- a/builtin/config.c
>> +++ b/builtin/config.c
>> @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>>                 else if (worktrees[0] && worktrees[1])
>>                         die(_("--worktree cannot be used with multiple "
>>                               "working trees unless the config\n"
>> -                             "extension worktreeConfig is enabled. "
>> -                             "Please read \"CONFIGURATION FILE\"\n"
>> +                             "extension worktreeConfig is enabled "
>> +                             "and core.repositoryFormatVersion is at least\n"
>> +                             "1. Please read \"CONFIGURATION FILE\""
>>                               "section in \"git help worktree\" for details"));
>>                 else
>>                         given_config_source.file = git_pathdup("config");
>>
>> Thanks,
>> -Stolee
>>
>>
Derrick Stolee July 14, 2020, 3:40 p.m. UTC | #5
On 7/14/2020 11:27 AM, Junio C Hamano wrote:
> 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?  

If "v0" includes "core.repositoryFormatVersion is unset" then I
would consider this to be a way to avoid all user pain, which is
positive.

I'd be happy to test and review a patch that accomplishes this
goal.

CC'ing Ed Thomson because this extension stuff affects other tools,
like libgit2.

Thanks,
-Stolee
Johannes Schindelin July 14, 2020, 8:30 p.m. UTC | #6
Hi,

On Tue, 14 Jul 2020, Derrick Stolee wrote:

> On 7/14/2020 11:27 AM, Junio C Hamano wrote:
> > 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?
>
> If "v0" includes "core.repositoryFormatVersion is unset" then I
> would consider this to be a way to avoid all user pain, which is
> positive.

I concur.

> I'd be happy to test and review a patch that accomplishes this
> goal.

Wouldn't that just be a matter of extending your patch to re-set
`has_unhandled_extensions` also for `preciousObjects` and `partialClone`?

Ciao,
Dscho

>
> CC'ing Ed Thomson because this extension stuff affects other tools,
> like libgit2.
>
> Thanks,
> -Stolee
>
>
Junio C Hamano July 14, 2020, 8:47 p.m. UTC | #7
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 14 Jul 2020, Derrick Stolee wrote:
>
>> If "v0" includes "core.repositoryFormatVersion is unset" then I
>> would consider this to be a way to avoid all user pain, which is
>> positive.
>
> I concur.
>
>> I'd be happy to test and review a patch that accomplishes this
>> goal.
>
> Wouldn't that just be a matter of extending your patch to re-set
> `has_unhandled_extensions` also for `preciousObjects` and `partialClone`?

It probably needs a bit more than that.  For example there is this
bit in check_repository_format_gently() that clears the unwanted
extensions that we used to honor by mistake in v0 repository

	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;
	}

and the "different endgame" advocates to keep honoring these (and
only these), the else clause probably needs to go.  There may be
some other tweaks necessary.
Junio C Hamano July 15, 2020, 4:09 p.m. UTC | #8
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?  

It seems that there is no quick concensus to go with your "different
endgame" and worse yet it seems nobody is interested in helping
much.

The current one on the table is NOT
<20200714040616.GA2208896@google.com> but the two patches

<1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>
<e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com>

which we may regret---it is far from a robust and complete solution,
but probably specific to users at Microsoft or something like that.
For example it special cases only the worktreeconfig and nothing
else, even though I suspect that other configuration variables were
also honored by mistake.

So...

Here is my quick attempt to see how far we can go with the
"different endgame" approach, to be applied on top of those two
patches.  It still has two known "breakages" and can use help from
extra eyeballs and real work.

I suspect that an expected test_must_fail not triggering t2404 may
even be a good thing if it is a sign of silent upgrading of the
repository version due to having grandfathered extensions in a v0
repository, but I didn't have time to dig further.

I'll shift my attention to other topics that should be in the
release for the rest of the day, but am pessimistic that I can tag
the -rc1 today, which won't happen until we at least have a
concensus on what to do with the (apparent) regression due to the
"upgrade repository version" topic.

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 &&
Junio C Hamano July 15, 2020, 5:01 p.m. UTC | #9
Junio C Hamano <gitster@pobox.com> writes:

> The current one on the table is NOT
> <20200714040616.GA2208896@google.com> but the two patches
>
> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>
> <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com>
>
> For example it special cases only the worktreeconfig and nothing
> else, even though I suspect that other configuration variables were
> also honored by mistake.

The attached may be a less ambitious and less risky update for the
upcoming release.  It is to be applied on top of the two-patch
series from Derrick, and just marks the other "known and honored
back then by mistake" extensions as OK to be there for upgrading.

Thoughts?  If people are happy with that, then we could apply and
cut an -rc1 with it.  Or if we are OK with the "just special case
worktreeconfig; other extensions may have the same issue but we
haven't heard actual complaints so we will leave them untouched",
then -rc1 can be done with just those two patches.

Now I do need to shift my attention to other topics in flight.

Thanks.


 setup.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/setup.c b/setup.c
index 65270440a9..a072c76d05 100644
--- a/setup.c
+++ b/setup.c
@@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 		data->version = git_config_int(var, value);
 	else if (skip_prefix(var, "extensions.", &ext)) {
 		/*
-		 * record any known extensions here; otherwise,
-		 * we fall through to recording it as unknown, and
-		 * check_repository_format will complain
+		 * Grandfather extensions that were known in 2.27 and
+		 * were honored by mistake even in v0 repositories; it
+		 * shoudn't be an error to upgrade v0 to v1 with them
+		 * in the repository, as they couldn't have been used
+		 * for incompatible purposes by the end user.
 		 */
-		int is_unallowed_extension = 1;
+		int unallowed_in_v0 = 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;
+		} 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);
Derrick Stolee July 15, 2020, 6 p.m. UTC | #10
On 7/15/2020 1:01 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> The current one on the table is NOT
>> <20200714040616.GA2208896@google.com> but the two patches
>>
>> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>
>> <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com>
>>
>> For example it special cases only the worktreeconfig and nothing
>> else, even though I suspect that other configuration variables were
>> also honored by mistake.

Sorry for the delay. I had your previous diff applied and was
playing around with the two "known breakages".

The diff below _does_ fail on t0410-partial-clone.sh, but it's
because you do change the behavior. Here is my diff hunk for that
test:

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be..fc8da56528 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,7 +50,20 @@ 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 &&
-       test_must_fail git -C client fetch --unshallow --filter="blob:none"
+       git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_success '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" 2>err &&
+       test_i18ngrep "unable to upgrade repository format from 0 to 1" err
 '
 
 test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '


> The attached may be a less ambitious and less risky update for the
> upcoming release.  It is to be applied on top of the two-patch
> series from Derrick, and just marks the other "known and honored
> back then by mistake" extensions as OK to be there for upgrading.
> 
> Thoughts?  If people are happy with that, then we could apply and
> cut an -rc1 with it.  Or if we are OK with the "just special case
> worktreeconfig; other extensions may have the same issue but we
> haven't heard actual complaints so we will leave them untouched",
> then -rc1 can be done with just those two patches.

The "special case wortreeConfig" of my submission was maybe based
on an incorrect assumption that this is the only place where Git
itself set an extension.* config without _also_ updating the
core.repositoryFormatVersion config.

The error I made in my v1 is to warn on ALL extensions, not just
the ones that Git knows about. This new approach is a good middle
ground between my v1 and v2.

Of course, the _other_ option is to revert xl/upgrade-repo-format
from v2.28.0 and take our time resolving this issue during the
2.29 cycle. I'm not sure how disruptive that action would be.

> Now I do need to shift my attention to other topics in flight.

I appreciate you spending so much time on this! It's a tough
time to be noticing such a complicated situation that is not
easily testable from a single Git version, but across versions.

> 
>  setup.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 65270440a9..a072c76d05 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
>  		data->version = git_config_int(var, value);
>  	else if (skip_prefix(var, "extensions.", &ext)) {
>  		/*
> -		 * record any known extensions here; otherwise,
> -		 * we fall through to recording it as unknown, and
> -		 * check_repository_format will complain
> +		 * Grandfather extensions that were known in 2.27 and
> +		 * were honored by mistake even in v0 repositories; it
> +		 * shoudn't be an error to upgrade v0 to v1 with them
> +		 * in the repository, as they couldn't have been used
> +		 * for incompatible purposes by the end user.
>  		 */
> -		int is_unallowed_extension = 1;
> +		int unallowed_in_v0 = 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;

Your previous diff had this comment, which I thought to be
helpful: 

+		/*
+		 * Extensions are added by more "} else if (...) {"
+		 * lines here, but do NOT mark them as allowed in v0
+		 * by copy-pasting without thinking.
+		 */

> -		} else
> +			unallowed_in_v0 = 0;
> +		} 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);

Thanks,
-Stolee
Junio C Hamano July 15, 2020, 6:09 p.m. UTC | #11
Derrick Stolee <stolee@gmail.com> writes:

> Your previous diff had this comment, which I thought to be
> helpful: 
>
> +		/*
> +		 * Extensions are added by more "} else if (...) {"
> +		 * lines here, but do NOT mark them as allowed in v0
> +		 * by copy-pasting without thinking.
> +		 */

Yeah, but it felt somewhat strange to have it at the end of one
entry, like this:

+			unallowed_in_v0 = 0;
 		} else if (!strcmp(ext, "worktreeconfig")) {
 			data->worktree_config = git_config_bool(var, value);
+			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);


In any case, I updated the comment in front of the if/else if/
cascade to essentially say the same thing, and with test updates
this time.

Thanks.

-- >8 --
Subject: [PATCH] setup: grandfather other extensions that used to be honored by mistake

We special cased worktreeconfig extension to be OK to exist when the
repository gets converted from v0 to v1 in an earlier commit, but
other extenions were also honored by mistake in v0.  Mark them the
same way so that they won't interfere with the upgrading from v0 to
v1.

A test in t0410 used to expect that presence of the
extension.partialclone configuration variable to interfere, but that
no longer is true.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                  | 30 +++++++++++++++++++-----------
 t/t0410-partial-clone.sh | 18 ++++++++++++++++--
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/setup.c b/setup.c
index 65270440a9..97292479d6 100644
--- a/setup.c
+++ b/setup.c
@@ -456,27 +456,35 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 		data->version = git_config_int(var, value);
 	else if (skip_prefix(var, "extensions.", &ext)) {
 		/*
-		 * record any known extensions here; otherwise,
-		 * we fall through to recording it as unknown, and
-		 * check_repository_format will complain
+		 * Grandfather extensions that were known in 2.27 and
+		 * were honored by mistake even in v0 repositories; it
+		 * shoudn't be an error to upgrade v0 to v1 with them
+		 * in the repository, as they couldn't have been used
+		 * for incompatible purposes by the end user.
+		 *
+		 * When adding new extensions support in this if/elseif/...
+		 * cascade, do not mark them as allowed in v0!
 		 */
-		int is_unallowed_extension = 1;
+		int unallowed_in_v0 = 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;
+		} 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);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be..e9674fc257 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 is OK with a grandfathered extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
@@ -50,7 +50,21 @@ 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 &&
-	test_must_fail git -C client fetch --unshallow --filter="blob:none"
+	git -C client fetch --unshallow --filter="blob:none" &&
+	test_cmp_config -C client 1 core.repositoryformatversion
+'
+
+test_expect_success 'convert shallow clone to partial clone must fail with an 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.partialclone origin &&
+	git -C client config extensions.unknown true &&
+	test_must_fail git -C client fetch --unshallow --filter="blob:none" &&
+	test_cmp_config -C client 0 core.repositoryformatversion
 '
 
 test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '
Junio C Hamano July 15, 2020, 6:15 p.m. UTC | #12
Derrick Stolee <stolee@gmail.com> writes:

> Of course, the _other_ option is to revert xl/upgrade-repo-format
> from v2.28.0 and take our time resolving this issue during the
> 2.29 cycle. I'm not sure how disruptive that action would be.

Yes, that is becoming very much tempting at this point, isn't it?

In any case, I've pushed out 'seen' with the "these extensions that
used to be honored in v0 won't interfere with repository upgrade"
patch I sent earlier, and I am hoping that it would be a reasonable
middle ground that won't regress things for users while making sure
we do not honor random future extensions by mistake.
Jonathan Nieder July 15, 2020, 6:20 p.m. UTC | #13
Hi,

Junio C Hamano wrote:

> It seems that there is no quick concensus to go with your "different
> endgame" and worse yet it seems nobody is interested in helping
> much.

Sorry for the delay.  I do want to look at this this afternoon.

[...]
> I'll shift my attention to other topics that should be in the
> release for the rest of the day, but am pessimistic that I can tag
> the -rc1 today, which won't happen until we at least have a
> concensus on what to do with the (apparent) regression due to the
> "upgrade repository version" topic.

Ah, I hadn't realized an -rc1 tomorrow instead of today was on the
table. ;-)

I'll do what I can (in other words, expect a patch from me; but also,
I am very interested in analysis, proposed patches, etc from others so
that we can end up wit ha good fix with the solution space well
explored).

Thanks,
Jonathan
Derrick Stolee July 15, 2020, 6:40 p.m. UTC | #14
On 7/15/2020 2:09 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> Your previous diff had this comment, which I thought to be
>> helpful: 
>>
>> +		/*
>> +		 * Extensions are added by more "} else if (...) {"
>> +		 * lines here, but do NOT mark them as allowed in v0
>> +		 * by copy-pasting without thinking.
>> +		 */
> 
> Yeah, but it felt somewhat strange to have it at the end of one
> entry, like this:
> 
> +			unallowed_in_v0 = 0;
>  		} else if (!strcmp(ext, "worktreeconfig")) {
>  			data->worktree_config = git_config_bool(var, value);
> +			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);
> 
> 
> In any case, I updated the comment in front of the if/else if/
> cascade to essentially say the same thing, and with test updates
> this time.

Thanks. I applied and tested this version. LGTM!

I'll also review Jonathan Nieder's patches when they arrive.

Thanks,
-Stolee
Johannes Schindelin July 15, 2020, 7:16 p.m. UTC | #15
Hi,

On Wed, 15 Jul 2020, Derrick Stolee wrote:

> On 7/15/2020 2:09 PM, Junio C Hamano wrote:
> > Derrick Stolee <stolee@gmail.com> writes:
> >
> >> Your previous diff had this comment, which I thought to be
> >> helpful:
> >>
> >> +		/*
> >> +		 * Extensions are added by more "} else if (...) {"
> >> +		 * lines here, but do NOT mark them as allowed in v0
> >> +		 * by copy-pasting without thinking.
> >> +		 */
> >
> > Yeah, but it felt somewhat strange to have it at the end of one
> > entry, like this:
> >
> > +			unallowed_in_v0 = 0;
> >  		} else if (!strcmp(ext, "worktreeconfig")) {
> >  			data->worktree_config = git_config_bool(var, value);
> > +			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);
> >
> >
> > In any case, I updated the comment in front of the if/else if/
> > cascade to essentially say the same thing, and with test updates
> > this time.
>
> Thanks. I applied and tested this version. LGTM!

Sorry for being slow at the party; I'd much rather deal with Git issues
than with the paperwork I am fighting with.

Thank you for working on this, I am pretty happy with the state you
whipped it into, but take that with a grain of salt, as my brain is moosh.

Ciao,
Dscho
Johannes Schindelin July 15, 2020, 7:21 p.m. UTC | #16
Hi Junio,

On Wed, 15 Jul 2020, Junio C Hamano wrote:

> Derrick Stolee <stolee@gmail.com> writes:
>
> > Of course, the _other_ option is to revert xl/upgrade-repo-format
> > from v2.28.0 and take our time resolving this issue during the
> > 2.29 cycle. I'm not sure how disruptive that action would be.
>
> Yes, that is becoming very much tempting at this point, isn't it?

It did cross my mind, too.

> In any case, I've pushed out 'seen' with the "these extensions that
> used to be honored in v0 won't interfere with repository upgrade"
> patch I sent earlier, and I am hoping that it would be a reasonable
> middle ground that won't regress things for users while making sure
> we do not honor random future extensions by mistake.

Given that we still have -rc1 and -rc2 to make sure that things work, and
given that your patch on top of Stolee's two patches looks like it is
Doing The Best We Can Do, I am optimistic that your reasonable middle
ground is the best way forward.

Thanks,
Dscho
Junio C Hamano July 16, 2020, 2:06 a.m. UTC | #17
Jonathan Nieder <jrnieder@gmail.com> writes:

>> I'll shift my attention to other topics that should be in the
>> release for the rest of the day, but am pessimistic that I can tag
>> the -rc1 today, which won't happen until we at least have a
>> concensus on what to do with the (apparent) regression due to the
>> "upgrade repository version" topic.
>
> Ah, I hadn't realized an -rc1 tomorrow instead of today was on the
> table. ;-)

It could be delayed even more if we do not have a good solution
agreed upon.

> I'll do what I can (in other words, expect a patch from me; but also,
> I am very interested in analysis, proposed patches, etc from others so
> that we can end up wit ha good fix with the solution space well
> explored).

Yup, that's the spirit.  Thanks!

Patch
diff mbox series

diff --git a/cache.h b/cache.h
index 126ec56c7f..da2c71f366 100644
--- a/cache.h
+++ b/cache.h
@@ -1042,7 +1042,7 @@  struct repository_format {
 	int worktree_config;
 	int is_bare;
 	int hash_algo;
-	int has_extensions;
+	int has_extensions, saw_extensions;
 	char *work_tree;
 	struct string_list unknown_extensions;
 };
diff --git a/setup.c b/setup.c
index dbac2eabe8..0f45e2e174 100644
--- a/setup.c
+++ b/setup.c
@@ -489,6 +489,15 @@  static int check_repository_format_gently(const char *gitdir, struct repository_
 	read_repository_format(candidate, sb.buf);
 	strbuf_release(&sb);
 
+	if (candidate->version < 1 &&
+	    (candidate->saw_extensions || candidate->has_extensions))
+		advise(_("extensions.* settings require a positive repository "
+			 "format version greater than zero.\n"
+			 "\n"
+			 "Please use the following call to enable extensions.* "
+			 "config settings:\n"
+			 "\"git config core.repositoryFormatVersion 1\""));
+
 	/*
 	 * For historical use of check_repository_format() in git-init,
 	 * we treat a missing config as a silent "ok", even when nongit_ok
@@ -584,8 +593,13 @@  int read_repository_format(struct repository_format *format, const char *path)
 {
 	clear_repository_format(format);
 	git_config_from_file(check_repo_format, path, format);
-	if (format->version == -1)
+	if (format->version == -1) {
+		int saw_extensions = format->has_extensions;
+
 		clear_repository_format(format);
+
+		format->saw_extensions = saw_extensions;
+	}
 	return format->version;
 }
 
diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh
index 9536d10919..1c08a45177 100755
--- a/t/t2404-worktree-config.sh
+++ b/t/t2404-worktree-config.sh
@@ -78,4 +78,19 @@  test_expect_success 'config.worktree no longer read without extension' '
 	test_cmp_config -C wt2 shared this.is
 '
 
+test_expect_success 'show advice when extensions.* are not enabled' '
+	test_config core.repositoryformatversion 1 &&
+	test_config extensions.worktreeConfig true &&
+	git status 2>err &&
+	test_i18ngrep ! "git config core.repositoryFormatVersion 1" err &&
+
+	test_config core.repositoryformatversion 0 &&
+	git status 2>err &&
+	test_i18ngrep "git config core.repositoryFormatVersion 1" err &&
+
+	git config --unset core.repositoryformatversion &&
+	git status 2>err &&
+	test_i18ngrep "git config core.repositoryFormatVersion 1" err
+'
+
 test_done