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

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

[Patryk Obara]

Compared to v1:

Implemented code suggestions from Duy Nguyễn (string for translation and
strbuf instead of char array). I also added an annotation in
repository-version.txt, clarifying, that this option is useful only for
development purpose for now.

Patryk Obara (1):
  setup: recognise extensions.objectFormat

 Documentation/technical/repository-version.txt | 12 ++++++++++++
 setup.c                                        | 27 ++++++++++++++++++++++++++
 t/t1302-repo-version.sh                        | 15 ++++++++++++++
 3 files changed, 54 insertions(+)


base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
-- 
2.14.3

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

[Patryk Obara]

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.

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

Add tests and documentation note about new extension.

Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
---
 Documentation/technical/repository-version.txt | 12 ++++++++++++
 setup.c                                        | 27 ++++++++++++++++++++++++++
 t/t1302-repo-version.sh                        | 15 ++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt
index 00ad37986e..7e2b832603 100644
--- a/Documentation/technical/repository-version.txt
+++ b/Documentation/technical/repository-version.txt
@@ -86,3 +86,15 @@ for testing format-1 compatibility.
 When the config key `extensions.preciousObjects` is set to `true`,
 objects in the repository MUST NOT be deleted (e.g., by `git-prune` or
 `git repack -d`).
+
+`objectFormat`
+~~~~~~~~~~~~~~
+
+This extension instructs Git to use a specific algorithm for addressing
+and interpreting objects in the object store. Currently, the only
+supported object format is `sha-1`. At the moment, the primary purpose
+of this option is to enable Git developers to experiment with different
+hashing algorithms without re-compilation of git client.
+
+See `hash-function-transition.txt` document for more detailed explanation.
+
diff --git a/setup.c b/setup.c
index 8cc34186ce..9b9993a14e 100644
--- a/setup.c
+++ b/setup.c
@@ -405,6 +405,31 @@ void setup_work_tree(void)
 	initialized = 1;
 }
 
+static int find_object_format(const char *value)
+{
+	int i;
+	for (i = GIT_HASH_SHA1; i < GIT_HASH_NALGOS; ++i) {
+		if (strcmp(value, hash_algos[i].name) == 0)
+			return i;
+	}
+	return GIT_HASH_UNKNOWN;
+}
+
+static void detect_object_format(struct repository_format *data,
+				 const char *value)
+{
+	if (data->version == 0)
+		die(_("invalid repository format version '%d'"), data->version);
+
+	data->hash_algo = find_object_format(value);
+	if (data->hash_algo == GIT_HASH_UNKNOWN) {
+		struct strbuf object_format = STRBUF_INIT;
+		strbuf_addf(&object_format, "objectformat = %s", value);
+		string_list_append(&data->unknown_extensions, object_format.buf);
+		strbuf_release(&object_format);
+	}
+}
+
 static int check_repo_format(const char *var, const char *value, void *vdata)
 {
 	struct repository_format *data = vdata;
@@ -422,6 +447,8 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 			;
 		else if (!strcmp(ext, "preciousobjects"))
 			data->precious_objects = git_config_bool(var, value);
+		else if (!strcmp(ext, "objectformat"))
+			detect_object_format(data, value);
 		else
 			string_list_append(&data->unknown_extensions, ext);
 	} else if (strcmp(var, "core.bare") == 0) {
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index ce4cff13bb..227b397ff2 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -107,4 +107,19 @@ test_expect_success 'gc runs without complaint' '
 	git gc
 '
 
+test_expect_success 'object-format not allowed in repo version=0' '
+	mkconfig 0 "objectFormat = sha-1" >.git/config &&
+	check_abort
+'
+
+test_expect_success 'object-format=sha-1 allowed' '
+	mkconfig 1 "objectFormat = sha-1" >.git/config &&
+	check_allow
+'
+
+test_expect_success 'object-format=foo unsupported' '
+	mkconfig 1 "objectFormat = foo" >.git/config &&
+	check_abort
+'
+
 test_done
-- 
2.14.3

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

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

I think you want an "it" here: "At the moment *it* supports".

> 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.
> 
> 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).
> 
> Add tests and documentation note about new extension.
> 
> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>

Other than that, the patch looks good to me.  I like that we reject
invalid values immediately.  Adding documentation is good, too.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

[PATCH v3 0/1] setup: recognise extensions.objectFormat

[Patryk Obara]

Compred to v2:

Fixed commit message.

Patryk Obara (1):
  setup: recognise extensions.objectFormat

 Documentation/technical/repository-version.txt | 12 ++++++++++++
 setup.c                                        | 27 ++++++++++++++++++++++++++
 t/t1302-repo-version.sh                        | 15 ++++++++++++++
 3 files changed, 54 insertions(+)


base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
-- 
2.14.3

[PATCH v3 1/1] setup: recognise extensions.objectFormat

[Patryk Obara]

This extension selects which hashing algorithm from vtable should be
used for reading and writing objects in the object store.  At the moment
it supports only single value (sha-1).

In case value of objectFormat is an unknown hashing algorithm, Git
command will fail with the following message:

  fatal: unknown repository extensions found:
	  objectformat = <value>

To indicate, that this specific objectFormat value is not recognised.

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

Add tests and documentation note about new extension.

Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
---
 Documentation/technical/repository-version.txt | 12 ++++++++++++
 setup.c                                        | 27 ++++++++++++++++++++++++++
 t/t1302-repo-version.sh                        | 15 ++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt
index 00ad37986e..7e2b832603 100644
--- a/Documentation/technical/repository-version.txt
+++ b/Documentation/technical/repository-version.txt
@@ -86,3 +86,15 @@ for testing format-1 compatibility.
 When the config key `extensions.preciousObjects` is set to `true`,
 objects in the repository MUST NOT be deleted (e.g., by `git-prune` or
 `git repack -d`).
+
+`objectFormat`
+~~~~~~~~~~~~~~
+
+This extension instructs Git to use a specific algorithm for addressing
+and interpreting objects in the object store. Currently, the only
+supported object format is `sha-1`. At the moment, the primary purpose
+of this option is to enable Git developers to experiment with different
+hashing algorithms without re-compilation of git client.
+
+See `hash-function-transition.txt` document for more detailed explanation.
+
diff --git a/setup.c b/setup.c
index 8cc34186ce..9b9993a14e 100644
--- a/setup.c
+++ b/setup.c
@@ -405,6 +405,31 @@ void setup_work_tree(void)
 	initialized = 1;
 }
 
+static int find_object_format(const char *value)
+{
+	int i;
+	for (i = GIT_HASH_SHA1; i < GIT_HASH_NALGOS; ++i) {
+		if (strcmp(value, hash_algos[i].name) == 0)
+			return i;
+	}
+	return GIT_HASH_UNKNOWN;
+}
+
+static void detect_object_format(struct repository_format *data,
+				 const char *value)
+{
+	if (data->version == 0)
+		die(_("invalid repository format version '%d'"), data->version);
+
+	data->hash_algo = find_object_format(value);
+	if (data->hash_algo == GIT_HASH_UNKNOWN) {
+		struct strbuf object_format = STRBUF_INIT;
+		strbuf_addf(&object_format, "objectformat = %s", value);
+		string_list_append(&data->unknown_extensions, object_format.buf);
+		strbuf_release(&object_format);
+	}
+}
+
 static int check_repo_format(const char *var, const char *value, void *vdata)
 {
 	struct repository_format *data = vdata;
@@ -422,6 +447,8 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 			;
 		else if (!strcmp(ext, "preciousobjects"))
 			data->precious_objects = git_config_bool(var, value);
+		else if (!strcmp(ext, "objectformat"))
+			detect_object_format(data, value);
 		else
 			string_list_append(&data->unknown_extensions, ext);
 	} else if (strcmp(var, "core.bare") == 0) {
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index ce4cff13bb..227b397ff2 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -107,4 +107,19 @@ test_expect_success 'gc runs without complaint' '
 	git gc
 '
 
+test_expect_success 'object-format not allowed in repo version=0' '
+	mkconfig 0 "objectFormat = sha-1" >.git/config &&
+	check_abort
+'
+
+test_expect_success 'object-format=sha-1 allowed' '
+	mkconfig 1 "objectFormat = sha-1" >.git/config &&
+	check_allow
+'
+
+test_expect_success 'object-format=foo unsupported' '
+	mkconfig 1 "objectFormat = foo" >.git/config &&
+	check_abort
+'
+
 test_done
-- 
2.14.3

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

[Jeff King]

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

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

[Patryk Obara]

On 30/01/2018 02:38, Jeff King wrote:
> 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).

Code using this is already in master - in the result of overwriting
data->hash_algo, every piece of code, that was modernised starts using
the selected hash algorithm (through the_hash_algo) instead of hardcoded
sha-1.

As far as I can tell status is following:

Once following topics will land:
- po/object-id (in pu)
- brian's object-id-part-11 (in review now)
- upcoming brian's object-id-part-12 (not sent to mailing list yet)
- few more object-id conversions and uses of the_hash_algo

we'll be in state, where just dropping new entry in hash_algos table
will enable experimental switching of object format.

With changes listed above "git hash-object -w -t blob" and
"git cat-file" work with NewHash (whatever it may be - brian is using 
blake2 in his experiments, I am using openssl sha3-256).

Right now I am looking at updating index structures and functions - 
after which git commit should work. In the transition plan it's 
described as "introducing index v3" (are there any new requirements, 
that constitute "v3" besides longer hash?).

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

Mostly convenience for developers, who want to work on transition. 
There's no need to re-compile only for changing default hashing 
algorithm (which is useful for testing and debugging). I could carry 
this patch around to every NewHash-related branch, that I work on but 
it's annoying me already ;)

As long as hash_algos table contains only sha-1, users effectively see
this extension as noop.

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

What about users, who are using new version of Git, but have it 
misconfigured with preciousObjects and repo format 0? That's why I 
decided to make repo format check specific to objectFormat extension 
(initially I made it generic to all extensions).

At the same time... there's extension.partialclone in pu and it does not 
have check on repo format.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

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

[Jeff King]

On Tue, Jan 30, 2018 at 05:30:04PM +0100, Patryk Obara wrote:

> > 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).
> 
> Code using this is already in master - in the result of overwriting
> data->hash_algo, every piece of code, that was modernised starts using
> the selected hash algorithm (through the_hash_algo) instead of hardcoded
> sha-1.

Right, that part seems pretty simple. But in the long run, is that going
to be enough for the hash transition? My impression is that the
transition document is going to require a more nuanced view than "this
is the hash algorithm for this repo".

Putting code in master is OK; we can always refactor it. But once we
add and document a user-facing config option like this, we have to
support it forever. So that's really the step I was wondering about: are
we sure this is what the user-facing config is going to look like?

> > 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.
> 
> Mostly convenience for developers, who want to work on transition. There's
> no need to re-compile only for changing default hashing algorithm (which is
> useful for testing and debugging). I could carry this patch around to every
> NewHash-related branch, that I work on but it's annoying me already ;)

OK, that makes some sense to me. Even if we may end up with a more
nuanced config later, this is useful for getting the first step done:
just making a standalone NewHash repo without worrying about
interoperation with existing history.

> > 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.
> 
> What about users, who are using new version of Git, but have it
> misconfigured with preciousObjects and repo format 0? That's why I decided
> to make repo format check specific to objectFormat extension (initially I
> made it generic to all extensions).

But that's sort of my point. It appears to be working, but the
prior-version safety they think they have is not there. I think we're
better off erring on the side of caution here, and letting them know
forcefully that their config is bogus.

> At the same time... there's extension.partialclone in pu and it does not
> have check on repo format.

IMHO it should (and we should just do it by enforcing it for all
extensions automatically).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Putting code in master is OK; we can always refactor it. But once we
> add and document a user-facing config option like this, we have to
> support it forever. So that's really the step I was wondering about: are
> we sure this is what the user-facing config is going to look like?

Yup, that is an important distinction.

> But that's sort of my point. It appears to be working, but the
> prior-version safety they think they have is not there. I think we're
> better off erring on the side of caution here, and letting them know
> forcefully that their config is bogus.
>
>> At the same time... there's extension.partialclone in pu and it does not
>> have check on repo format.
>
> IMHO it should (and we should just do it by enforcing it for all
> extensions automatically).

Sounds good.