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

[Patryk Obara <patryk.obara@gmail.com>]

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