Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once

["Eric W. Biederman" <ebiederm@gmail.com>]

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

> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> Today there is no sanity checking of what happens when
>> extensions.objectFormat is specified multiple times.  Catch confused git
>> configurations by only allowing this option to be specified once.
>
> Hmph.  I am not sure if this is worth doing, and especially only for
> "objectformat".  Do we intend to apply different rules other than
> "you can give it only once" to other extensions, and if so where
> will these rules be catalogued?  I do not see particular harm to let
> them follow the usual "last one wins".
>
> If the patch were about trying to make sure that extensions, which
> are inherentaly per-repository, appear only in $GIT_DIR/config and
> complain if the code gets confused and tried to read them from the
> system or global configuration files, I would understand, and
> strongly support such an effort, ithough.

Unless I misread something, the code only checks for [extensions]
in $GIT_DIR/config.

> The real sanity we want to enforce is that what is reported by
> running "git config extensions.objectformat" must match the object
> format that is used in refs and object database.

Agreed.  Allowing git config extensions.objectformat to change the
existing value is allowing the repository to be corrupted.

> Manually futzing
> the configuration file and adding an entry with a contradictory
> value certainly is one way to break that sanity, and this patch may
> catch such a breakage, but once we start worrying about manually
> futzing the configuration file, the check added here would easily
> miss if the futzing is done by replacing instead of adding, so I am
> not sure if this extra code is worth its bits.
>
> But perhaps I am missing something and not seeing why it is worth
> insisting on "last one is the first one" for this particular one.

I somewhat have blinders on.  There are 3 configuration options I am
concerned with:

extensions.objectFormat
extensions.compatObjectFormat
core.historicObjectFormat (or whatever name we settle on).

One key concern I heard expressed in earlier reviews is that however we
handle these options we handle them in such a way as to give ourselves
room to rise to challenges in the future.

Whatever we do with parsing we have the following logical
constraints:

For extensions.objectFormat: There can only be a single storage hash.

For extensions.compatObjectFormat: There can be no compatibility hash,
there can be a single compatibility hash, and depending how things go
between now and the next hash function transition we might want multiple
compatibility hashes.

For core.historicObjectFormat: There can be no historic hash function, there
can be a single historic hash function, there can be multiple historic
hash functions.


For the compatibility hash I think it is unlikely we will want to
support more than one compatibility hash in practice but I can imagine
a scenario where we just get into the transition from SHA-1 to SHA-256
and a serious break is discovered that requires switching to FutureHash
ASAP.

For historic object formats like SHA-1 will become post transition there
are references embedded in commit comments, email messages, bug
trackers.  All kinds of places that we can not update so there is
fundamentally a need to be able to find which current objects correspond
to the historic names.  For a project each hash function transition will
create more such objects.


When I looked I saw two ways within current git to specify a list of
values for a single configuration option.
- Give that option multiple times.
- Parse the option value in such a way as to generate a list.

It is my sense just specifying the compatObjectFormat multiple times to
specify multiple compatibility object formats makes the most sense.
Especially as all is needed today is to only allow a single value.

After I had implemented the only allow once logic for compatObjectFormat
I saw that objectFormat had nothing similar, and knowing it is a bug
for multiple objectFormat wrote a patch to enforce only appear once
for objectFormat as well.

For objectFormat I don't care very much.  For compatObjectFormat I truly
care, and for even more for the option that allows finding the current
object from a historic oid (even a truncated one) I care very much.

For me the fundamental question is if we allow multiples compatibility
hashes or historical hashes how do we specify them?  Have the option
appear more than once?  A comma separated list?

Whatever we decided I want to enforce that doesn't appear in current
configurations so we can support for multiples later.

Eric

>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  setup.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/setup.c b/setup.c
>> index 18927a847b86..ef9f79b8885e 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,
>>  	if (!strcmp(ext, "noop-v1")) {
>>  		return EXTENSION_OK;
>>  	} else if (!strcmp(ext, "objectformat")) {
>> +		struct string_list_item *item;
>>  		int format;
>>  
>>  		if (!value)
>> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,
>>  		if (format == GIT_HASH_UNKNOWN)
>>  			return error(_("invalid value for '%s': '%s'"),
>>  				     "extensions.objectformat", value);
>> +		/* Only support objectFormat being specified once. */
>> +		for_each_string_list_item(item, &data->v1_only_extensions) {
>> +			if (!strcmp(item->string, "objectformat"))
>> +				return error(_("'%s' already specified as '%s'"),
>> +					"extensions.objectformat",
>> +					hash_algos[data->hash_algo].name);
>> +		}
>>  		data->hash_algo = format;
>>  		return EXTENSION_OK;
>>  	}