Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256

From: Derrick Stolee <stolee@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
Date: Tue, 16 Oct 2018 12:01:12 -0400	[thread overview]
Message-ID: <fbd59390-42ee-8c06-74b7-1e7081680913@gmail.com> (raw)
In-Reply-To: <CACsJy8C6o9PAHsm+t7Rijg6jsz5dXZ5pT79nYGdGbKuK6gGTLQ@mail.gmail.com>

On 10/16/2018 11:35 AM, Duy Nguyen wrote:
> On Mon, Oct 15, 2018 at 4:23 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>> Since the commit-graph code wants to serialize the hash algorithm into
>> the data store, specify a version number for each supported algorithm.
>> Note that we don't use the values of the constants themselves, as they
>> are internal and could change in the future.
>>
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>>   commit-graph.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/commit-graph.c b/commit-graph.c
>> index 7a28fbb03f..e587c21bb6 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -45,7 +45,14 @@ char *get_commit_graph_filename(const char *obj_dir)
>>
>>   static uint8_t oid_version(void)
>>   {
>> -       return 1;
>> +       switch (hash_algo_by_ptr(the_hash_algo)) {
>> +               case GIT_HASH_SHA1:
>> +                       return 1;
>> +               case GIT_HASH_SHA256:
>> +                       return 2;
> Should we just increase this field to uint32_t and store format_id
> instead? That will keep oid version unique in all data formats.
Both the commit-graph and multi-pack-index store a single byte for the 
hash version, so that ship has sailed (without incrementing the full 
file version number in each format).

It may be good to make this method accessible to both formats. I'm not 
sure if Brian's branch is built on top of the multi-pack-index code. 
Probably best to see if ds/multi-pack-verify is in the history.

Thanks,
-Stolee