Subject: Re: [PATCH 03/30] object-names: Support input of oids in any supported hash

From: "Eric W. Biederman" <[email-masked]>
Date: Sun, 01 Oct 2023 20:54:29 -0500

Eric Sunshine <[email-masked]> writes:

> On Wed, Sep 27, 2023 at 3:56 PM Eric W. Biederman <[email-masked]> wrote:
>> Support short oids encoded in any algorithm, while ensuring enough of
>> the oid is specified to disambiguate between all of the oids in the
>> repository encoded in any algorithm.
>>
>> By default have the code continue to only accept oids specified in the
>> storage hash algorithm of the repository, but when something is
>> ambiguous display all of the possible oids from any oid encoding.
>>
>> A new flag is added GET_OID_HASH_ANY that when supplied causes the
>> code to accept oids specified in any hash algorithm, and to return the
>> oids that were resolved.
>>
>> This implements the functionality that allows both SHA-1 and SHA-256
>> object names, from the "Object names on the command line" section of
>> the hash function transition document.
>>
>> Care is taken in get_short_oid so that when the result is ambiguous
>> the output remains the same of GIT_OID_HASH_ANY was not supplied.
>
> s/of/as if/

Actually s/of/if/

Thank you for catching that.  When reviewing this to understand what I
was trying to say I found a bug.  The function repo_for_each_abbrev was
passing algo to init_object_disambiguation to properly initialize
ds.bin_pfx.algo, and then was resetting ds.bin_pfx.algo to GIT_HASH_ANY.
Oops!

>> If GET_OID_HASH_ANY was supplied objects of any hash algorithm
>> that match the prefix are displayed.
>>
>> This required updating repo_for_each_abbrev to give it a parameter
>> so that it knows to look at all hash algorithms.
>>
>> Signed-off-by: "Eric W. Biederman" <[email-masked]>
>> ---
>> diff --git a/object-name.c b/object-name.c
>> @@ -49,6 +50,7 @@ struct disambiguate_state {
>>  static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)
>>  {
>> +       /* The hash algorithm of the current has already been filtered */
>
> Is there a word missing after "current"?

No.  I forgot to remove "the" before current.

>> @@ -503,8 +516,13 @@ static int sort_ambiguous(const void *a, const void *b, void *ctx)
>> -       if (a_type == b_type)
>> -               return oidcmp(a, b);
>> +       if (a_type == b_type) {
>> +               /* Is the hash algorithm the same? */
>> +               if (a->algo == b->algo)
>> +                       return oidcmp(a, b);
>> +               else
>> +                       return a->algo > b->algo ? 1 : -1;
>> +       }
>
> Nit: unnecessary comment ("Is the hash algorithm...") is merely
> repeating what the code itself already says clearly enough

Fair enough.

>
>> @@ -553,6 +575,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
>>         else
>>                 ds.fn = default_disambiguate_hint;
>>
>> +
>>         find_short_object_filename(&ds);
>
> Nit: unnecessary new blank line

Naughty thing how did that sneak in there ;)

Eric