Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> "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.
Show 10 quoted lines
> 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
Show 31 quoted lines
>> 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;
>> }