[PATCH] setup: warn about un-enabled extensions

[PATCH] setup: warn about un-enabled extensions

[Johannes Schindelin via GitGitGadget]

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

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

base-commit: bd42bbe1a46c0fe486fc33e82969275e27e4dc19
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

[Derrick Stolee]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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

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

[Derrick Stolee]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

[Derrick Stolee]

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

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

[Junio C Hamano]

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' '
-- 
2.28.0-rc0

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

[Derrick Stolee]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)

[Jonathan Nieder]

Junio C Hamano wrote:

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

Apologies again for the delay.

Here are patches implementing the minimal fix that I'd recommend.
These apply against "master" without requiring any other patches
as prerequisites.  Thoughts?

Jonathan Nieder (2):
  Revert "check_repository_format_gently(): refuse extensions for old
    repositories"
  repository: allow repository format upgrade with extensions

 cache.h                  |  1 -
 setup.c                  | 24 ++++++++++--------------
 t/t0410-partial-clone.sh | 15 +++++++++++++--
 3 files changed, 23 insertions(+), 17 deletions(-)

[PATCH 1/2] Revert "check_repository_format_gently(): refuse extensions for old repositories"

[Jonathan Nieder]

This reverts commit 14c7fa269e42df4133edd9ae7763b678ed6594cd.

The core.repositoryFormatVersion field was introduced in ab9cb76f661
(Repository format version check., 2005-11-25), providing a welcome
bit of forward compatibility, thanks to some welcome analysis by
Martin Atukunda.  The semantics are simple: a repository with
core.repositoryFormatVersion set to 0 should be comprehensible by all
Git implementations in active use; and Git implementations should
error out early instead of trying to act on Git repositories with
higher core.repositoryFormatVersion values representing new formats
that they do not understand.

A new repository format did not need to be defined until 00a09d57eb8
(introduce "extensions" form of core.repositoryformatversion,
2015-06-23).  This provided a finer-grained extension mechanism for
Git repositories.  In a repository with core.repositoryFormatVersion
set to 1, Git implementations can act on "extensions.*" settings that
modify how a repository is interpreted.  In repository format version
1, unrecognized extensions settings cause Git to error out.

What happens if a user sets an extension setting but forgets to
increase the repository format version to 1?  The extension settings
were still recognized in that case; worse, unrecognized extensions
settings do *not* cause Git to error out.  So combining repository
format version 0 with extensions settings produces in some sense the
worst of both worlds.

To improve that situation, since 14c7fa269e4
(check_repository_format_gently(): refuse extensions for old
repositories, 2020-06-05) Git instead ignores extensions in v0 mode.
This way, v0 repositories get the historical (pre-2015) behavior and
maintain compatibility with Git implementations that do not know about
the v1 format.  Unfortunately, users had been using this sort of
configuration and this behavior change came to many as a surprise:

- users of "git config --worktree" that had followed its advice
  to enable extensions.worktreeConfig (without also increasing the
  repository format version) would find their worktree configuration
  no longer taking effect

- tools such as copybara[*] that had set extensions.partialClone in
  existing repositories (without also increasing the repository format
  version) would find that setting no longer taking effect

The behavior introduced in 14c7fa269e4 might be a good behavior if we
were traveling back in time to 2015, but we're far too late.  For some
reason I thought that it was what had been originally implemented and
that it had regressed.  Apologies for not doing my research when
14c7fa269e4 was under development.

Let's return to the behavior we've had since 2015: always act on
extensions.* settings, regardless of repository format version.  While
we're here, include some tests to describe the effect on the "upgrade
repository version" code path.

[*] https://github.com/google/copybara/commit/ca76c0b1e13c4e36448d12c2aba4a5d9d98fb6e7

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c                  | 12 +++---------
 t/t0410-partial-clone.sh | 15 +++++++++++++--
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/setup.c b/setup.c
index dbac2eabe8f..87bf0112cf3 100644
--- a/setup.c
+++ b/setup.c
@@ -507,15 +507,9 @@ 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;
-	}
+	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 463dc3a8be0..51d1eba6050 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -42,14 +42,25 @@ 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 'converting to partial clone fails with noop 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.noop true &&
+	test_must_fail git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_success 'converting to partial clone fails with unrecognized 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.nonsense true &&
 	test_must_fail git -C client fetch --unshallow --filter="blob:none"
 '
 
-- 
2.28.0.rc0.105.gf9edc3c819

Re: [PATCH 1/2] Revert "check_repository_format_gently(): refuse extensions for old repositories"

[Jeff King]

On Wed, Jul 15, 2020 at 11:24:29PM -0700, Jonathan Nieder wrote:

> The behavior introduced in 14c7fa269e4 might be a good behavior if we
> were traveling back in time to 2015, but we're far too late.  For some
> reason I thought that it was what had been originally implemented and
> that it had regressed.  Apologies for not doing my research when
> 14c7fa269e4 was under development.

Thanks for a good summary of the situation. I agree that the current
(well, pre-14c7fa269e4) behavior is a bug (mine) from 2015, and we
probably have to accept that state of affairs to some degree in order to
avoid breaking existing cases.

It is unfortunate that this means that somebody with version=0 and
extensions.preciousObjects is in danger of running a pre-2015 version of
Git and having that extension totally ignored, which could be a
data-safety issue. But the farther we get from 2015 the less likely that
is to be a problem (and the more likely somebody is to be depending on
the current behavior of v0+preciousObjects).

> Let's return to the behavior we've had since 2015: always act on
> extensions.* settings, regardless of repository format version.  While
> we're here, include some tests to describe the effect on the "upgrade
> repository version" code path.

So this makes sense to me as a first step.

-Peff

[PATCH 2/2] repository: allow repository format upgrade with extensions

[Jonathan Nieder]

Now that we officially permit repository extensions in repository
format v0, permit upgrading a repository with extensions from v0 to v1
as well.

For example, this means a repository where the user has set
"extensions.preciousObjects" can use "git fetch --filter=blob:none
origin" to upgrade the repository to use v1 and the partial clone
extension.

To avoid mistakes, continue to forbid repository format upgrades in v0
repositories with an unrecognized extension.  This way, a v0 user
using a misspelled extension field gets a chance to correct the
mistake before updating to the less forgiving v1 format.

While we're here, make the error message for failure to upgrade the
repository format a bit shorter, and present it as an error, not a
warning.

Reported-by: Huan Huan Chen <huanhuanchen@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Apologies again for the trouble, and thanks for your patient help.

 cache.h                  |  1 -
 setup.c                  | 12 +++++++-----
 t/t0410-partial-clone.sh |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 126ec56c7f3..654426460cc 100644
--- a/cache.h
+++ b/cache.h
@@ -1042,7 +1042,6 @@ struct repository_format {
 	int worktree_config;
 	int is_bare;
 	int hash_algo;
-	int has_extensions;
 	char *work_tree;
 	struct string_list unknown_extensions;
 };
diff --git a/setup.c b/setup.c
index 87bf0112cf3..3a81307602e 100644
--- a/setup.c
+++ b/setup.c
@@ -455,7 +455,6 @@ 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
@@ -553,13 +552,16 @@ int upgrade_repository_format(int target_version)
 	if (repo_fmt.version >= target_version)
 		return 0;
 
-	if (verify_repository_format(&repo_fmt, &err) < 0 ||
-	    (!repo_fmt.version && repo_fmt.has_extensions)) {
-		warning("unable to upgrade repository format from %d to %d: %s",
-			repo_fmt.version, target_version, err.buf);
+	if (verify_repository_format(&repo_fmt, &err) < 0) {
+		error("cannot upgrade repository format from %d to %d: %s",
+		      repo_fmt.version, target_version, err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
+	if (!repo_fmt.version && repo_fmt.unknown_extensions.nr)
+		return error("cannot upgrade repository format: "
+			     "unknown extension %s",
+			     repo_fmt.unknown_extensions.items[0].string);
 
 	strbuf_addf(&repo_version, "%d", target_version);
 	git_config_set("core.repositoryformatversion", repo_version.buf);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 51d1eba6050..6aa0f313bdd 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 'converting to partial clone fails with noop extension' '
+test_expect_success 'convert to partial clone with noop extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
@@ -50,7 +50,7 @@ test_expect_success 'converting to partial clone fails with noop extension' '
 	git clone --depth=1 "file://$(pwd)/server" client &&
 	test_cmp_config -C client 0 core.repositoryformatversion &&
 	git -C client config extensions.noop true &&
-	test_must_fail git -C client fetch --unshallow --filter="blob:none"
+	git -C client fetch --unshallow --filter="blob:none"
 '
 
 test_expect_success 'converting to partial clone fails with unrecognized extension' '
-- 
2.28.0.rc0.105.gf9edc3c819

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Now that we officially permit repository extensions in repository
> format v0, permit upgrading a repository with extensions from v0 to v1
> as well.
>
> For example, this means a repository where the user has set
> "extensions.preciousObjects" can use "git fetch --filter=blob:none
> origin" to upgrade the repository to use v1 and the partial clone
> extension.
>
> To avoid mistakes, continue to forbid repository format upgrades in v0
> repositories with an unrecognized extension.  This way, a v0 user
> using a misspelled extension field gets a chance to correct the
> mistake before updating to the less forgiving v1 format.

This needs to be managed carefully.  When the next extension is
added to the codebase, that extension may be "known" to Git, but I
do not think it is a good idea to honor it in v0 repository, or
allow upgrading v0 repository to v1 with such an extension that
weren't "known" to Git.  For example, a topic in flight adds
objectformat extension and I do not think it should be honored in v0
repository.

Having said that, the approach is OK for now at the tip of tonight's
master, but the point is "known" vs "unknown" must be fixed right
with some means.  E.g. tell people to throw the "new" extensions to
the list of "unknown extensions" in check_repo_format() when they
add new ones, or something.

Thanks.

> +	if (!repo_fmt.version && repo_fmt.unknown_extensions.nr)
> +		return error("cannot upgrade repository format: "
> +			     "unknown extension %s",
> +			     repo_fmt.unknown_extensions.items[0].string);
>  
>  	strbuf_addf(&repo_version, "%d", target_version);
>  	git_config_set("core.repositoryformatversion", repo_version.buf);
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 51d1eba6050..6aa0f313bdd 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 'converting to partial clone fails with noop extension' '
> +test_expect_success 'convert to partial clone with noop extension' '
>  	rm -fr server client &&
>  	test_create_repo server &&
>  	test_commit -C server my_commit 1 &&
> @@ -50,7 +50,7 @@ test_expect_success 'converting to partial clone fails with noop extension' '
>  	git clone --depth=1 "file://$(pwd)/server" client &&
>  	test_cmp_config -C client 0 core.repositoryformatversion &&
>  	git -C client config extensions.noop true &&
> -	test_must_fail git -C client fetch --unshallow --filter="blob:none"
> +	git -C client fetch --unshallow --filter="blob:none"
>  '
>  
>  test_expect_success 'converting to partial clone fails with unrecognized extension' '

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 12:01:09AM -0700, Junio C Hamano wrote:

> > To avoid mistakes, continue to forbid repository format upgrades in v0
> > repositories with an unrecognized extension.  This way, a v0 user
> > using a misspelled extension field gets a chance to correct the
> > mistake before updating to the less forgiving v1 format.
> 
> This needs to be managed carefully.  When the next extension is
> added to the codebase, that extension may be "known" to Git, but I
> do not think it is a good idea to honor it in v0 repository, or
> allow upgrading v0 repository to v1 with such an extension that
> weren't "known" to Git.  For example, a topic in flight adds
> objectformat extension and I do not think it should be honored in v0
> repository.
> 
> Having said that, the approach is OK for now at the tip of tonight's
> master, but the point is "known" vs "unknown" must be fixed right
> with some means.  E.g. tell people to throw the "new" extensions to
> the list of "unknown extensions" in check_repo_format() when they
> add new ones, or something.

Yeah, I agree with this line of reasoning. I'd prefer to see it
addressed now, so that we don't have to remember to do anything later.
I.e., for this patch to put the existing known extensions into the
"good" list for v0, locking it into place forever, and leaving the
objectformat topic with nothing particular it needs to do.

But in the name of -rc1 expediency, I'm also OK moving forward with this
for now.

-Peff

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 07:00:08AM -0400, Jeff King wrote:

> > > To avoid mistakes, continue to forbid repository format upgrades in v0
> > > repositories with an unrecognized extension.  This way, a v0 user
> > > using a misspelled extension field gets a chance to correct the
> > > mistake before updating to the less forgiving v1 format.
> > 
> > This needs to be managed carefully.  When the next extension is
> > added to the codebase, that extension may be "known" to Git, but I
> > do not think it is a good idea to honor it in v0 repository, or
> > allow upgrading v0 repository to v1 with such an extension that
> > weren't "known" to Git.  For example, a topic in flight adds
> > objectformat extension and I do not think it should be honored in v0
> > repository.
> > 
> > Having said that, the approach is OK for now at the tip of tonight's
> > master, but the point is "known" vs "unknown" must be fixed right
> > with some means.  E.g. tell people to throw the "new" extensions to
> > the list of "unknown extensions" in check_repo_format() when they
> > add new ones, or something.
> 
> Yeah, I agree with this line of reasoning. I'd prefer to see it
> addressed now, so that we don't have to remember to do anything later.
> I.e., for this patch to put the existing known extensions into the
> "good" list for v0, locking it into place forever, and leaving the
> objectformat topic with nothing particular it needs to do.
> 
> But in the name of -rc1 expediency, I'm also OK moving forward with this
> for now.

Hmm, this is actually a bit trickier than I expected because of the way
the code is written. It's much easier to complain about extensions in a
v0 repository than it is to ignore them. But I'm not sure if that isn't
the right way to go anyway.

The patch I came up with is below (and goes on top of Jonathan's). Even
if we decide this is the right direction, it can definitely happen
post-v2.28.

-- >8 --
Subject: verify_repository_format(): complain about new extensions in v0 repo

We made the mistake in the past of respecting extensions.* even when the
repository format version was set to 0. This is bad because forgetting
to bump the repository version means that older versions of Git (which
do not know about our extensions) won't complain. I.e., it's not a
problem in itself, but it means your repository is in a state which does
not give you the protection you think you're getting from older
versions.

For compatibility reasons, we are stuck with that decision for existing
extensions. However, we'd prefer not to extend the damage further. We
can do that by catching any newly-added extensions and complaining about
the repository format.

Note that this is a pretty heavy hammer: we'll refuse to work with the
repository at all. A lesser option would be to ignore (possibly with a
warning) any new extensions. But because of the way the extensions are
handled, that puts the burden on each new extension that is added to
remember to "undo" itself (because they are handled before we know
for sure whether we are in a v1 repo or not, since we don't insist on a
particular ordering of config entries).

So one option would be to rewrite that handling to record any new
extensions (and their values) during the config parse, and then only
after proceed to handle new ones only if we're in a v1 repository. But
I'm not sure if it's worth the trouble:

  - ignoring extensions is likely to end up with broken results anyway
    (e.g., ignoring a proposed objectformat extension means parsing any
    object data is likely to encounter errors)

  - this is a sign that whatever tool wrote the extension field is
    broken. We may be better off notifying immediately and forcefully so
    that such tools don't even appear to work accidentally.

The only downside is that fixing the istuation is a little tricky,
because programs like "git config" won't want to work with the
repository. But:

  git config --file=.git/config core.repositoryformatversion 1

should still suffice.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h                 |  2 +
 setup.c                 | 96 ++++++++++++++++++++++++++++++++++-------
 t/t1302-repo-version.sh |  3 ++
 3 files changed, 85 insertions(+), 16 deletions(-)

diff --git a/cache.h b/cache.h
index 654426460c..0290849c19 100644
--- a/cache.h
+++ b/cache.h
@@ -1044,6 +1044,7 @@ struct repository_format {
 	int hash_algo;
 	char *work_tree;
 	struct string_list unknown_extensions;
+	struct string_list v1_only_extensions;
 };
 
 /*
@@ -1057,6 +1058,7 @@ struct repository_format {
 	.is_bare = -1, \
 	.hash_algo = GIT_HASH_SHA1, \
 	.unknown_extensions = STRING_LIST_INIT_DUP, \
+	.v1_only_extensions = STRING_LIST_INIT_DUP, \
 }
 
 /*
diff --git a/setup.c b/setup.c
index 3a81307602..c1480b2b60 100644
--- a/setup.c
+++ b/setup.c
@@ -447,6 +447,54 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)
 	return 0;
 }
 
+enum extension_result {
+	EXTENSION_ERROR = -1, /* compatible with error(), etc */
+	EXTENSION_UNKNOWN = 0,
+	EXTENSION_OK = 1
+};
+
+/*
+ * Do not add new extensions to this function. It handles extensions which are
+ * respected even in v0-format repositories for historical compatibility.
+ */
+enum extension_result handle_extension_v0(const char *var,
+					  const char *value,
+					  const char *ext,
+					  struct repository_format *data)
+{
+		if (!strcmp(ext, "noop")) {
+			return EXTENSION_OK;
+		} else if (!strcmp(ext, "preciousobjects")) {
+			data->precious_objects = git_config_bool(var, value);
+			return EXTENSION_OK;
+		} else if (!strcmp(ext, "partialclone")) {
+			if (!value)
+				return config_error_nonbool(var);
+			data->partial_clone = xstrdup(value);
+			return EXTENSION_OK;
+		} else if (!strcmp(ext, "worktreeconfig")) {
+			data->worktree_config = git_config_bool(var, value);
+			return EXTENSION_OK;
+		}
+
+		return EXTENSION_UNKNOWN;
+}
+
+/*
+ * Record any new extensions in this function.
+ */
+enum extension_result handle_extension(const char *var,
+				       const char *value,
+				       const char *ext,
+				       struct repository_format *data)
+{
+	if (!strcmp(ext, "noop-v1")) {
+		return EXTENSION_OK;
+	}
+
+	return EXTENSION_UNKNOWN;
+}
+
 static int check_repo_format(const char *var, const char *value, void *vdata)
 {
 	struct repository_format *data = vdata;
@@ -455,23 +503,25 @@ 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)) {
-		/*
-		 * record any known extensions here; otherwise,
-		 * we fall through to recording it as unknown, and
-		 * check_repository_format will complain
-		 */
-		if (!strcmp(ext, "noop"))
-			;
-		else if (!strcmp(ext, "preciousobjects"))
-			data->precious_objects = git_config_bool(var, value);
-		else if (!strcmp(ext, "partialclone")) {
-			if (!value)
-				return config_error_nonbool(var);
-			data->partial_clone = xstrdup(value);
-		} else if (!strcmp(ext, "worktreeconfig"))
-			data->worktree_config = git_config_bool(var, value);
-		else
+		switch (handle_extension_v0(var, value, ext, data)) {
+		case EXTENSION_ERROR:
+			return -1;
+		case EXTENSION_OK:
+			return 0;
+		case EXTENSION_UNKNOWN:
+			break;
+		}
+
+		switch (handle_extension(var, value, ext, data)) {
+		case EXTENSION_ERROR:
+			return -1;
+		case EXTENSION_OK:
+			string_list_append(&data->v1_only_extensions, ext);
+			return 0;
+		case EXTENSION_UNKNOWN:
 			string_list_append(&data->unknown_extensions, ext);
+			return 0;
+		}
 	}
 
 	return read_worktree_config(var, value, vdata);
@@ -510,6 +560,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	set_repository_format_partial_clone(candidate->partial_clone);
 	repository_format_worktree_config = candidate->worktree_config;
 	string_list_clear(&candidate->unknown_extensions, 0);
+	string_list_clear(&candidate->v1_only_extensions, 0);
 
 	if (repository_format_worktree_config) {
 		/*
@@ -588,6 +639,7 @@ int read_repository_format(struct repository_format *format, const char *path)
 void clear_repository_format(struct repository_format *format)
 {
 	string_list_clear(&format->unknown_extensions, 0);
+	string_list_clear(&format->v1_only_extensions, 0);
 	free(format->work_tree);
 	free(format->partial_clone);
 	init_repository_format(format);
@@ -613,6 +665,18 @@ int verify_repository_format(const struct repository_format *format,
 		return -1;
 	}
 
+	if (format->version == 0 && format->v1_only_extensions.nr) {
+		int i;
+
+		strbuf_addstr(err,
+			      _("repo version is 0, but v1-only extensions found:"));
+
+		for (i = 0; i < format->v1_only_extensions.nr; i++)
+			strbuf_addf(err, "\n\t%s",
+				    format->v1_only_extensions.items[i].string);
+		return -1;
+	}
+
 	return 0;
 }
 
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index d60c042ce8..0acabb6d11 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -87,6 +87,9 @@ allow 1
 allow 1 noop
 abort 1 no-such-extension
 allow 0 no-such-extension
+allow 0 noop
+abort 0 noop-v1
+allow 1 noop-v1
 EOF
 
 test_expect_success 'precious-objects allowed' '
-- 
2.28.0.rc0.424.g7d08728e23

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Derrick Stolee]

On 7/16/2020 8:25 AM, Jeff King wrote:
> On Thu, Jul 16, 2020 at 07:00:08AM -0400, Jeff King wrote:
> 
>>>> To avoid mistakes, continue to forbid repository format upgrades in v0
>>>> repositories with an unrecognized extension.  This way, a v0 user
>>>> using a misspelled extension field gets a chance to correct the
>>>> mistake before updating to the less forgiving v1 format.
>>>
>>> This needs to be managed carefully.  When the next extension is
>>> added to the codebase, that extension may be "known" to Git, but I
>>> do not think it is a good idea to honor it in v0 repository, or
>>> allow upgrading v0 repository to v1 with such an extension that
>>> weren't "known" to Git.  For example, a topic in flight adds
>>> objectformat extension and I do not think it should be honored in v0
>>> repository.
>>>
>>> Having said that, the approach is OK for now at the tip of tonight's
>>> master, but the point is "known" vs "unknown" must be fixed right
>>> with some means.  E.g. tell people to throw the "new" extensions to
>>> the list of "unknown extensions" in check_repo_format() when they
>>> add new ones, or something.
>>
>> Yeah, I agree with this line of reasoning. I'd prefer to see it
>> addressed now, so that we don't have to remember to do anything later.
>> I.e., for this patch to put the existing known extensions into the
>> "good" list for v0, locking it into place forever, and leaving the
>> objectformat topic with nothing particular it needs to do.
>>
>> But in the name of -rc1 expediency, I'm also OK moving forward with this
>> for now.
> 
> Hmm, this is actually a bit trickier than I expected because of the way
> the code is written. It's much easier to complain about extensions in a
> v0 repository than it is to ignore them. But I'm not sure if that isn't
> the right way to go anyway.
> 
> The patch I came up with is below (and goes on top of Jonathan's). Even
> if we decide this is the right direction, it can definitely happen
> post-v2.28.
> 
> -- >8 --
> Subject: verify_repository_format(): complain about new extensions in v0 repo
> 
> We made the mistake in the past of respecting extensions.* even when the
> repository format version was set to 0. This is bad because forgetting
> to bump the repository version means that older versions of Git (which
> do not know about our extensions) won't complain. I.e., it's not a
> problem in itself, but it means your repository is in a state which does
> not give you the protection you think you're getting from older
> versions.
> 
> For compatibility reasons, we are stuck with that decision for existing
> extensions. However, we'd prefer not to extend the damage further. We
> can do that by catching any newly-added extensions and complaining about
> the repository format.
> 
> Note that this is a pretty heavy hammer: we'll refuse to work with the
> repository at all. A lesser option would be to ignore (possibly with a
> warning) any new extensions. But because of the way the extensions are
> handled, that puts the burden on each new extension that is added to
> remember to "undo" itself (because they are handled before we know
> for sure whether we are in a v1 repo or not, since we don't insist on a
> particular ordering of config entries).
> 
> So one option would be to rewrite that handling to record any new
> extensions (and their values) during the config parse, and then only
> after proceed to handle new ones only if we're in a v1 repository. But
> I'm not sure if it's worth the trouble:
> 
>   - ignoring extensions is likely to end up with broken results anyway
>     (e.g., ignoring a proposed objectformat extension means parsing any
>     object data is likely to encounter errors)
> 
>   - this is a sign that whatever tool wrote the extension field is
>     broken. We may be better off notifying immediately and forcefully so
>     that such tools don't even appear to work accidentally.
> 
> The only downside is that fixing the istuation is a little tricky,

s/istuation/situation

> because programs like "git config" won't want to work with the
> repository. But:
> 
>   git config --file=.git/config core.repositoryformatversion 1
> 
> should still suffice.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  cache.h                 |  2 +
>  setup.c                 | 96 ++++++++++++++++++++++++++++++++++-------
>  t/t1302-repo-version.sh |  3 ++
>  3 files changed, 85 insertions(+), 16 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 654426460c..0290849c19 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1044,6 +1044,7 @@ struct repository_format {
>  	int hash_algo;
>  	char *work_tree;
>  	struct string_list unknown_extensions;
> +	struct string_list v1_only_extensions;
>  };
>  
>  /*
> @@ -1057,6 +1058,7 @@ struct repository_format {
>  	.is_bare = -1, \
>  	.hash_algo = GIT_HASH_SHA1, \
>  	.unknown_extensions = STRING_LIST_INIT_DUP, \
> +	.v1_only_extensions = STRING_LIST_INIT_DUP, \
>  }
>  
>  /*
> diff --git a/setup.c b/setup.c
> index 3a81307602..c1480b2b60 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -447,6 +447,54 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)
>  	return 0;
>  }
>  
> +enum extension_result {
> +	EXTENSION_ERROR = -1, /* compatible with error(), etc */
> +	EXTENSION_UNKNOWN = 0,
> +	EXTENSION_OK = 1
> +};
> +
> +/*
> + * Do not add new extensions to this function. It handles extensions which are
> + * respected even in v0-format repositories for historical compatibility.
> + */
> +enum extension_result handle_extension_v0(const char *var,
> +					  const char *value,
> +					  const char *ext,
> +					  struct repository_format *data)
...
> +/*
> + * Record any new extensions in this function.
> + */
> +enum extension_result handle_extension(const char *var,
> +				       const char *value,
> +				       const char *ext,
> +				       struct repository_format *data)

I like the split between these two methods to make it
really clear the difference between "v0" and "v1".

>  	struct repository_format *data = vdata;
> @@ -455,23 +503,25 @@ 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)) {
...
> +		switch (handle_extension_v0(var, value, ext, data)) {
> +		case EXTENSION_ERROR:
> +			return -1;
> +		case EXTENSION_OK:
> +			return 0;
> +		case EXTENSION_UNKNOWN:
> +			break;
> +		}
> +
> +		switch (handle_extension(var, value, ext, data)) {
> +		case EXTENSION_ERROR:
> +			return -1;
> +		case EXTENSION_OK:
> +			string_list_append(&data->v1_only_extensions, ext);
> +			return 0;
> +		case EXTENSION_UNKNOWN:
>  			string_list_append(&data->unknown_extensions, ext);
> +			return 0;
> +		}
>  	}

And it makes this loop much cleaner.
> @@ -613,6 +665,18 @@ int verify_repository_format(const struct repository_format *format,
>  		return -1;
>  	}
>  
> +	if (format->version == 0 && format->v1_only_extensions.nr) {
> +		int i;
> +
> +		strbuf_addstr(err,
> +			      _("repo version is 0, but v1-only extensions found:"));
> +
> +		for (i = 0; i < format->v1_only_extensions.nr; i++)
> +			strbuf_addf(err, "\n\t%s",
> +				    format->v1_only_extensions.items[i].string);
> +		return -1;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index d60c042ce8..0acabb6d11 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -87,6 +87,9 @@ allow 1
>  allow 1 noop
>  abort 1 no-such-extension
>  allow 0 no-such-extension
> +allow 0 noop
> +abort 0 noop-v1
> +allow 1 noop-v1

LGTM.

Thanks,
-Stolee

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm, this is actually a bit trickier than I expected because of the way
> the code is written. It's much easier to complain about extensions in a
> v0 repository than it is to ignore them. But I'm not sure if that isn't
> the right way to go anyway.
>
> The patch I came up with is below (and goes on top of Jonathan's). Even
> if we decide this is the right direction, it can definitely happen
> post-v2.28.

It must happen already in 'seen' if we want to keep bc/sha-2-part-3
with us, though ;-)

> So one option would be to rewrite that handling to record any new
> extensions (and their values) during the config parse, and then only
> after proceed to handle new ones only if we're in a v1 repository.

I do not think it would be too bad for read_repository_format() to
call git_config_from_file() to collect extensions.* in a string list
while looking for core.repositoryformatversion.  Then the function
can iterate over the string list to call check_repo_format() itself.

> I'm not sure if it's worth the trouble:
>
>   - ignoring extensions is likely to end up with broken results anyway
>     (e.g., ignoring a proposed objectformat extension means parsing any
>     object data is likely to encounter errors)

The primary reason why the safety calls for ignore/reject is the
namespace collision.  We may decide to use extensions.objectformat
to record what hash algorithms are used for objects in the
repository, while the end user and their tools may use it for
totally different purpose (perhaps they have a custom script around
"git repack" that reads the variable to learn what command line
options e.g. --window=800 to pass).  A new version of Git that
supports SHA-2 will suddenly break their configuration, when the
users are 100% happy with the current SHA-1 system, with the way
their tool uses extensions.objectformat configuration variable and a
newer version of Git that happens to know how to also work with SHA-2,
using their v0 repository.

And the reasoning 'ignoring would make the problem get noticed
anyway' does not apply to such users at all, does it?

We need to declare that any names under "extensions.*" is off limits
by end users regardless and write it in big flasing red letters if
we haven't already done so.  It is enforced in v1 repositories by
dying upon seeing an unrecognised extension, but not entirely.  When
the users are lucky, a known-but-name-collided extension may stop
with a type error (e.g. "extensions.objectformat=frotz" may say
"frotz is not among the accepted hash algorithms") but that is not
guaranteed.  In v0 repositories, enforcing it after the fact would
cause the same trouble as the tightening caused.

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 09:32:35AM -0700, Junio C Hamano wrote:

> We need to declare that any names under "extensions.*" is off limits
> by end users regardless and write it in big flasing red letters if
> we haven't already done so.

I thought this was already well-understood, and it was definitely part
of the plan since 2015. Are other tools really sticking stuff in
extensions.* that we don't know about?

-Peff

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> Hmm, this is actually a bit trickier than I expected because of the way
>> the code is written. It's much easier to complain about extensions in a
>> v0 repository than it is to ignore them. But I'm not sure if that isn't
>> the right way to go anyway.
>>
>> The patch I came up with is below (and goes on top of Jonathan's). Even
>> if we decide this is the right direction, it can definitely happen
>> post-v2.28.
>
> It must happen already in 'seen' if we want to keep bc/sha-2-part-3
> with us, though ;-)

FWIW, I needed to adjust t0001 while merging the SHA-2 topic.  The
internal use of "git config" via test_config triggers the "this is
not a Git repository as the value of repositoryformatversion and the
defined set of extensions are incompatible".

diff --cc t/t0001-init.sh
index 6d2467995e,34d2064660..ff538c0eed
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
 ...
 -test_expect_success 'extensions.objectFormat is not honored with repo version 0' '
++test_expect_success 'extensions.objectFormat would cause an error in repo version 0' '
+ 	git init --object-format=sha256 explicit-v0 &&
 -	test_config -C explicit-v0 core.repositoryformatversion 0 &&
 -	git -C explicit-v0 rev-parse --show-object-format >actual &&
 -	echo sha1 >expected &&
 -	test_cmp expected actual
++	v=$(git config --file=explicit-v0/.git/config core.repositoryformatversion 0) &&
++	test_when_finished "
++		git config --file=explicit-v0/.git/config core.repositoryformatversion $v
++	" &&
++	git config --file=explicit-v0/.git/config core.repositoryformatversion 0 &&
++	test_must_fail git -C explicit-v0 rev-parse --show-object-format >actual
+ '

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: verify_repository_format(): complain about new extensions in v0 repo
>
> We made the mistake in the past of respecting extensions.* even when the
> repository format version was set to 0. This is bad because forgetting
> to bump the repository version means that older versions of Git (which
> do not know about our extensions) won't complain. I.e., it's not a
> problem in itself, but it means your repository is in a state which does
> not give you the protection you think you're getting from older
> versions.
>
> For compatibility reasons, we are stuck with that decision for existing
> extensions. However, we'd prefer not to extend the damage further. We
> can do that by catching any newly-added extensions and complaining about
> the repository format.

Looking good overall, but I needed this to build from the source.

Thanks.

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

diff --git a/setup.c b/setup.c
index e29659b7b9..e69bd28ed6 100644
--- a/setup.c
+++ b/setup.c
@@ -457,10 +457,10 @@ enum extension_result {
  * Do not add new extensions to this function. It handles extensions which are
  * respected even in v0-format repositories for historical compatibility.
  */
-enum extension_result handle_extension_v0(const char *var,
-					  const char *value,
-					  const char *ext,
-					  struct repository_format *data)
+static enum extension_result handle_extension_v0(const char *var,
+						 const char *value,
+						 const char *ext,
+						 struct repository_format *data)
 {
 		if (!strcmp(ext, "noop")) {
 			return EXTENSION_OK;
@@ -483,10 +483,10 @@ enum extension_result handle_extension_v0(const char *var,
 /*
  * Record any new extensions in this function.
  */
-enum extension_result handle_extension(const char *var,
-				       const char *value,
-				       const char *ext,
-				       struct repository_format *data)
+static enum extension_result handle_extension(const char *var,
+					      const char *value,
+					      const char *ext,
+					      struct repository_format *data)
 {
 	if (!strcmp(ext, "noop-v1")) {
 		return EXTENSION_OK;

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 09:49:14AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Subject: verify_repository_format(): complain about new extensions in v0 repo
> >
> > We made the mistake in the past of respecting extensions.* even when the
> > repository format version was set to 0. This is bad because forgetting
> > to bump the repository version means that older versions of Git (which
> > do not know about our extensions) won't complain. I.e., it's not a
> > problem in itself, but it means your repository is in a state which does
> > not give you the protection you think you're getting from older
> > versions.
> >
> > For compatibility reasons, we are stuck with that decision for existing
> > extensions. However, we'd prefer not to extend the damage further. We
> > can do that by catching any newly-added extensions and complaining about
> > the repository format.
> 
> Looking good overall, but I needed this to build from the source.

Oof, thanks. I did this as a one-off not even on a branch, and my
config.mak magic loosens -Werror in that case (because usually a
detached HEAD means I'm investigating some old commit, and quite a few
of them don't build without warnings these days).

Thankfully it seems I only managed a minor error without the compiler
there to help me. :)

-Peff

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I agree with this line of reasoning. I'd prefer to see it
> addressed now, so that we don't have to remember to do anything later.

Very true.  Also the documentation may need some updating,
e.g. "These 4 extensions are honored without adding
repositoryFormatVersion to your repository (as special cases)" to
avoid further confusion e.g. "why doesn't my objectFormat=SHA-3 does
not take effect by itself?".

> I.e., for this patch to put the existing known extensions into the
> "good" list for v0, locking it into place forever, and leaving the
> objectformat topic with nothing particular it needs to do.
>
> But in the name of -rc1 expediency, I'm also OK moving forward with this
> for now.

I'm OK, too, as I said.

I'd need to kick out bc/sha-2-part-3 topic out of my tree while that
infrastructure is in place on top of these two patches, though.

Thanks.

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jonathan Nieder]

(replying from vacation; back tomorrow)
Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:

>> Yeah, I agree with this line of reasoning. I'd prefer to see it
>> addressed now, so that we don't have to remember to do anything later.
>
> Very true.  Also the documentation may need some updating,
> e.g. "These 4 extensions are honored without adding
> repositoryFormatVersion to your repository (as special cases)" to
> avoid further confusion e.g. "why doesn't my objectFormat=SHA-3 does
> not take effect by itself?".

Yes, I agree, especially about documentation.

For 2.29, I would like to do or see the following:

- defining the list of repository format v0 supported extensions as
  "these and no more", futureproofing along the lines suggested in
  Peff's patch.  I like the general approach taken there since it
  allows parsing the relevant config in a single pass, so I think
  it basically takes the right approach.  (That said, it might be
  possible to simplify a bit with further changes, e.g. by using the
  configset API.)

  When doing this for real, we'd want to document the set of
  supported extensions.  That is especially useful to independent
  implementers wanting to support Git's formats, since it tells
  them "this is the minimum set of extensions that you must
  either handle or error out cleanly on to maintain compatibility
  with Git's repository format v0".
 
- improving the behavior when an extension not supported in v0 is
  encountered in a v0 repository.  For extensions that are supported
  in v1 and not v0, we should presumably error out so the user can
  repair the repository, and we can put the "noop" extension in that
  category for the sake of easy testing.  We can also include a check
  in "git fsck" for repositories that request the undefined behavior
  of v0 repositories with non-v0 extensions, for faster diagnosis.

  What about unrecognized extensions that are potentially extensions
  yet to be defined?  Should these be silently ignored to match the
  historical behavior, or should we error out even in repository
  format v0?  I lean toward the latter; we'll need to be cautious,
  though, e.g. by making this a separate patch so we can easily tweak
  it if this ends up being disruptive in some unanticipated way.

- making "git init" use repository format v1 by default.  It's been
  long enough that users can count on Git implementations supporting
  it.  This way, users are less likely to run into v0+extensions
  confusion, just because users are less likely to be using v0.

Does that sound like a good plan to others?  If so, are there any
steps beyond the two first patches in jn/v0-with-extensions-fix that
we would want in order to prepare for it in 2.28?

My preference would be to move forward in 2.28 with the first two
patches in that topic branch (i.e., *not* the third yet), since they
don't produce any user facing behavior that would create danger for
users or clash with this plan.  Today, the only extensions we
recognize are in that set of extensions that we'll want to continue to
recognize in v0 (except possibly the for-testing extension "noop").
The steps to take with additional extensions are more subtle so it
seems reasonable for them to bake in "next" and then "master" for a
2.29 release.

Thanks,
Jonathan

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> - defining the list of repository format v0 supported extensions as
>   "these and no more", futureproofing along the lines suggested in
>   Peff's patch.  I like the general approach taken there since it
>   allows parsing the relevant config in a single pass, so I think
>   it basically takes the right approach.  (That said, it might be
>   possible to simplify a bit with further changes, e.g. by using the
>   configset API.)
>
>   When doing this for real, we'd want to document the set of
>   supported extensions.  That is especially useful to independent
>   implementers wanting to support Git's formats, since it tells
>   them "this is the minimum set of extensions that you must
>   either handle or error out cleanly on to maintain compatibility
>   with Git's repository format v0".

Good.

> - improving the behavior when an extension not supported in v0 is
>   encountered in a v0 repository.  For extensions that are supported
>   in v1 and not v0, we should presumably error out so the user can
>   repair the repository, and we can put the "noop" extension in that
>   category for the sake of easy testing.  We can also include a check
>   in "git fsck" for repositories that request the undefined behavior
>   of v0 repositories with non-v0 extensions, for faster diagnosis.
>
>   What about unrecognized extensions that are potentially extensions
>   yet to be defined?  Should these be silently ignored to match the
>   historical behavior, or should we error out even in repository
>   format v0?  I lean toward the latter; we'll need to be cautious,
>   though, e.g. by making this a separate patch so we can easily tweak
>   it if this ends up being disruptive in some unanticipated way.

I disagree with your first paragraph.  Those that weren't honored by
mistake back in v0 days, in addition to those that aren't known to us
even now, should just be silently ignored, not causing an error.

> - making "git init" use repository format v1 by default.  It's been
>   long enough that users can count on Git implementations supporting
>   it.  This way, users are less likely to run into v0+extensions
>   confusion, just because users are less likely to be using v0.

Absolutely.  I would think this is a very good move.

> Does that sound like a good plan to others?  If so, are there any
> steps beyond the two first patches in jn/v0-with-extensions-fix that
> we would want in order to prepare for it in 2.28?
>
> My preference would be to move forward in 2.28 with the first two
> patches in that topic branch (i.e., *not* the third yet), since they
> don't produce any user facing behavior that would create danger for
> users or clash with this plan.

Yup, I agree.  I'd give another name to the third commit and then
rewind jn/v0-with-extensions-fix by one to prevent mistakes from
happening.  Thanks.

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 04:50:34PM -0700, Junio C Hamano wrote:

> > - improving the behavior when an extension not supported in v0 is
> >   encountered in a v0 repository.  For extensions that are supported
> >   in v1 and not v0, we should presumably error out so the user can
> >   repair the repository, and we can put the "noop" extension in that
> >   category for the sake of easy testing.  We can also include a check
> >   in "git fsck" for repositories that request the undefined behavior
> >   of v0 repositories with non-v0 extensions, for faster diagnosis.
> >
> >   What about unrecognized extensions that are potentially extensions
> >   yet to be defined?  Should these be silently ignored to match the
> >   historical behavior, or should we error out even in repository
> >   format v0?  I lean toward the latter; we'll need to be cautious,
> >   though, e.g. by making this a separate patch so we can easily tweak
> >   it if this ends up being disruptive in some unanticipated way.
> 
> I disagree with your first paragraph.  Those that weren't honored by
> mistake back in v0 days, in addition to those that aren't known to us
> even now, should just be silently ignored, not causing an error.

That's very much the opposite of my patch.  As we add new extensions,
those "unknowns" will start to die().

I remain unconvinced that there are a bunch of unknown extension.*
config options hanging around in the wild, but maybe I'm being naive.
It seems to me more likely that users will be helped by warning about
extensions that _should_ have had v1 set than that they will be harmed
because they put random crap in their extensions.* config. But maybe you
know of a specific example?

Anyway, if we move to "v1" as the default for "git init" anyway, then
the number of people being helped would become much smaller.

> > My preference would be to move forward in 2.28 with the first two
> > patches in that topic branch (i.e., *not* the third yet), since they
> > don't produce any user facing behavior that would create danger for
> > users or clash with this plan.
> 
> Yup, I agree.  I'd give another name to the third commit and then
> rewind jn/v0-with-extensions-fix by one to prevent mistakes from
> happening.  Thanks.

OK. I was confused to see it still at the tip in the latest What's
Cooking, but I think we're just crossing emails. :)

-Peff

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Anyway, if we move to "v1" as the default for "git init" anyway, then
> the number of people being helped would become much smaller.

Yup. So in that sense I do not think I care too deeply either way.

>> > My preference would be to move forward in 2.28 with the first two
>> > patches in that topic branch (i.e., *not* the third yet), since they
>> > don't produce any user facing behavior that would create danger for
>> > users or clash with this plan.
>> 
>> Yup, I agree.  I'd give another name to the third commit and then
>> rewind jn/v0-with-extensions-fix by one to prevent mistakes from
>> happening.  Thanks.
>
> OK. I was confused to see it still at the tip in the latest What's
> Cooking, but I think we're just crossing emails. :)

Yes.

Re: [PATCH 2/2] repository: allow repository format upgrade with extensions

[Jeff King]

On Thu, Jul 16, 2020 at 03:37:19PM -0700, Jonathan Nieder wrote:

> For 2.29, I would like to do or see the following:
> 
> - defining the list of repository format v0 supported extensions as
>   "these and no more", futureproofing along the lines suggested in
>   Peff's patch.  I like the general approach taken there since it
>   allows parsing the relevant config in a single pass, so I think
>   it basically takes the right approach.  (That said, it might be
>   possible to simplify a bit with further changes, e.g. by using the
>   configset API.)
> 
>   When doing this for real, we'd want to document the set of
>   supported extensions.  That is especially useful to independent
>   implementers wanting to support Git's formats, since it tells
>   them "this is the minimum set of extensions that you must
>   either handle or error out cleanly on to maintain compatibility
>   with Git's repository format v0".

I think we should still consider people setting v0 along with extensions
to be a bug. It was never documented to work that way and we are being
nice by keeping the existing behavior, but it is still wrong (and
pre-extension versions of Git will silently ignore them). I don't mind
making other implementers aware of the special status, but we should be
careful not to endorse the broken setup.

> - making "git init" use repository format v1 by default.  It's been
>   long enough that users can count on Git implementations supporting
>   it.  This way, users are less likely to run into v0+extensions
>   confusion, just because users are less likely to be using v0.

That's probably reasonable. It will be mildly annoying for people like
me who are often testing old versions of Git, but I'm sure I will
survive.

We should make sure that all major implementations handle v1 reasonably
first, though (and that they did so long enough ago that it's not likely
to cause problems).

> My preference would be to move forward in 2.28 with the first two
> patches in that topic branch (i.e., *not* the third yet), since they
> don't produce any user facing behavior that would create danger for
> users or clash with this plan.  Today, the only extensions we
> recognize are in that set of extensions that we'll want to continue to
> recognize in v0 (except possibly the for-testing extension "noop").
> The steps to take with additional extensions are more subtle so it
> seems reasonable for them to bake in "next" and then "master" for a
> 2.29 release.

I'm OK with the plan to ship the first two patches for 2.28, and leave
my patch for later (or even soften it from "die" to "ignore with a
warning").

I think leaving "noop" in that set of special extensions makes sense,
since it lets us test that case easily (and I added a "noop-v1" in my
patch to test the other one; clearly we could also flip it and have
noop-v0).

-Peff

Re: [PATCH 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)

[Johannes Schindelin]

Hi Jonathan,

On Wed, 15 Jul 2020, Jonathan Nieder wrote:

> Junio C Hamano wrote:
>
> > 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.
>
> Here are patches implementing the minimal fix that I'd recommend.
> These apply against "master" without requiring any other patches
> as prerequisites.  Thoughts?

IIUC all of the existing `extensions.*` predate the reverted strict check,
right? And the idea is that future `extensions.*` will only work when
`repositoryFormatVersion` is larger than 1, right?

I would have been fine with Junio's patch on top of Stolee's, and I am
equally fine with this patch series. My main aim is not so much
future-proofing, though, as it is to avoid regressions in existing setups.

Thanks,
Dscho

Re: [PATCH 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)

[Derrick Stolee]

On 7/16/2020 4:13 AM, Johannes Schindelin wrote:
> Hi Jonathan,
> 
> On Wed, 15 Jul 2020, Jonathan Nieder wrote:
> 
>> Junio C Hamano wrote:
>>
>>> 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.
>>
>> Here are patches implementing the minimal fix that I'd recommend.
>> These apply against "master" without requiring any other patches
>> as prerequisites.  Thoughts?
> 
> IIUC all of the existing `extensions.*` predate the reverted strict check,
> right? And the idea is that future `extensions.*` will only work when
> `repositoryFormatVersion` is larger than 1, right?
> 
> I would have been fine with Junio's patch on top of Stolee's, and I am
> equally fine with this patch series. My main aim is not so much
> future-proofing, though, as it is to avoid regressions in existing setups.

I'm fine either way.  I think that Jonathan's patch comes from a
more informed place than my patches, so his are probably safer.

The situation that caught my interest is covered by this test
that was part of my patches:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 7cd45fc1394..6c0b82c3930 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -68,6 +68,18 @@ test_expect_success 'git sparse-checkout init' '
        check_files repo a
 '
 
+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 config --unset core.repositoryFormatVersion &&
+       git -C repo sparse-checkout init &&
+       git -C repo config core.repositoryFormatVersion >actual &&
+       echo 1 >expect &&
+       git -C repo config core.repositoryFormatVersion 0 &&
+       git -C repo sparse-checkout init &&
+       git -C repo config core.repositoryFormatVersion >actual &&
+       test_cmp expect actual
+'
+
 test_expect_success 'git sparse-checkout list after init' '
        git -C repo sparse-checkout list >actual &&
        cat >expect <<-\EOF &&

and this test passes with Jonathan's series. I think this kind
of behavior is covered by his change to the 'converting to partial
clone fails with noop extension' test in t0410-partial-clone.sh,
so a duplicate test in t1091-sparse-checkout-builtin.sh may be
overkill.

Thanks, all.

-Stolee