setup: warn about un-enabled extensions

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.

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

Quoted 16 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 8 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

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:

builtin/config.c +3 -2

index 5e39f618854..b5de7982a93 100644
@@ -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

Quoted 4 lines

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

Quoted 22 lines

builtin/config.c +3 -2

index 5e39f618854..b5de7982a93 100644
@@ -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

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines

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. <[email protected]>)

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?

Quoted 36 lines · Junio C Hamano

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines

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. <[email protected]>)

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.

Quoted 4 lines

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

Quoted 3 lines · Derrick Stolee

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.

Quoted 2 lines

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?

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines

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. <[email protected]>)

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?

The current one on the table is NOT <[email protected]> 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.

Quoted 9 lines · Junio C Hamano

The current one on the table is NOT <[email protected]> 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.

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

setup.c +9 -7

index 65270440a9..a072c76d05 100644
@@ -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);

Review

Quoted 8 lines · Derrick Stolee

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

(quoted diff) +5 -0

+		/*
+		 * 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:

setup.c +8 -0

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

Quoted 27 lines · Junio C Hamano

Review

Quoted 8 lines · Derrick Stolee

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

(quoted diff) +5 -0

+		/*
+		 * 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:

setup.c +8 -0

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

Quoted 3 lines

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!

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.

Quoted 3 lines · Derrick Stolee

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.

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.

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.

Quoted 5 lines

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

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.

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.

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.

setup.c +4 -0

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

t/t0410-partial-clone.sh +2 -2

index 51d1eba6050..6aa0f313bdd 100755
@@ -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' '

Quoted 4 lines

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.

Quoted 13 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 19 lines · Jeff King

Quoted 13 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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 <[email protected]>

---
 cache.h                 |  2 +
 setup.c                 | 96 ++++++++++++++++++++++++++++++++++-------
 t/t1302-repo-version.sh |  3 ++
 3 files changed, 85 insertions(+), 16 deletions(-)

cache.h +2 -0

index 654426460c..0290849c19 100644
@@ -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, \
 }
 
 /*

setup.c +14 -0

index 3a81307602..c1480b2b60 100644
@@ -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)

setup.c +0 -0

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

setup.c +10 -0

+		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)) {

setup.c +12 -0

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

t/t1302-repo-version.sh +0 -0

index d60c042ce8..0acabb6d11 100755

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.

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

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.

Quoted 5 lines · Jeff King

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.

Quoted 3 lines

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

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.

Quoted 10 lines · Jeff King

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.

Quoted 4 lines

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.

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.

Quoted 2 lines · Jeff King

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

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

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 4 lines

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.

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

Quoted 8 lines

Quoted 4 lines

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

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

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

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.

Quoted 3 lines · Junio C Hamano

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?

Hi Jonathan,

Quoted 9 lines · Jonathan Nieder

Quoted 3 lines · Junio C Hamano

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.