Re: [PATCH v2 1/1] setup: recognise extensions.objectFormat

[Jeff King <peff@peff.net>]

On Sun, Jan 28, 2018 at 01:36:17AM +0100, Patryk Obara wrote:

> This extension selects which hashing algorithm from vtable should be
> used for reading and writing objects in the object store.  At the moment
> supports only single value (sha-1).
> 
> In case value of objectFormat is an unknown hashing algorithm, Git
> command will fail with following message:
> 
>   fatal: unknown repository extensions found:
> 	  objectformat = <value>
> 
> To indicate, that this specific objectFormat value is not recognised.

I don't have a strong opinion on this, but it does feel a little funny
to add this extension now, before we quite know what the code that uses
it is going to look like (or maybe we're farther along there than I
realize).

What do we gain by doing this now as opposed to later? By the design of
the extension code, we should complain on older versions anyway. And by
doing it now we carry a small risk that it might not give us the
interface we want, and it will be slightly harder to paper over this
failed direction.

All that said, if people like brian, who are thinking more about this
transition than I am, are onboard, I'm OK with it.

> The objectFormat extension is not allowed in repository marked as
> version 0 to prevent any possibility of accidentally writing a NewHash
> object in the sha-1 object store. This extension behaviour is different
> than preciousObjects extension (which is allowed in repo version 0).

It wasn't intended that anyone would specify preciousObjects with repo
version 0. It's a dangerous misconfiguration (because versions which
predate the extensions mechanism won't actually respect it at all!).

So we probably ought to complain loudly on having anything in
extensions.* when the repositoryformat is less than 1.

I originally wrote it the other way out of an abundance of
backward-compatibility. After all "extension.*" doesn't mean anything in
format 0, and somebody _could_ have added such a config key for their
own purposes. But that's a pretty weak argument, and if we are going to
start marking some extensions as forbidden there, we might as well do
them all.

Something like this:

diff --git a/cache.h b/cache.h
index d8b975a571..259c4a5361 100644
--- a/cache.h
+++ b/cache.h
@@ -921,6 +921,7 @@ struct repository_format {
 	int is_bare;
 	int hash_algo;
 	char *work_tree;
+	int extensions_seen;
 	struct string_list unknown_extensions;
 };
 
diff --git a/setup.c b/setup.c
index 8cc34186ce..85dfcf330b 100644
--- a/setup.c
+++ b/setup.c
@@ -413,6 +413,7 @@ 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->extensions_seen = 1;
 		/*
 		 * record any known extensions here; otherwise,
 		 * we fall through to recording it as unknown, and
@@ -503,6 +504,11 @@ int verify_repository_format(const struct repository_format *format,
 		return -1;
 	}
 
+	if (format->version < 1 && format->extensions_seen) {
+		strbuf_addstr(err, _("extensions found but repo version is < 1"));
+		return -1;
+	}
+
 	if (format->version >= 1 && format->unknown_extensions.nr) {
 		int i;
 
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index ce4cff13bb..9e9f67d756 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -80,9 +80,10 @@ while read outcome version extensions; do
 done <<\EOF
 allow 0
 allow 1
+abort 0 noop
 allow 1 noop
+abort 0 no-such-extension
 abort 1 no-such-extension
-allow 0 no-such-extension
 EOF
 
 test_expect_success 'precious-objects allowed' '