Re: [PATCH v3 5/5] sha1_name: Minimize OID comparisons during disambiguation

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 3, 2017, 17:05 UTC
Message-ID: <8ddd2da9-2846-a4c8-f80a-55eb61e5925d@gmail.com>
In-Reply-To: <CAGZ79kZz0EPUo=FpndxQMztQ=x-_c_CbvRB2NDmTLQXWapDBqQ@mail.gmail.com>

On 10/3/2017 11:55 AM, Stefan Beller wrote:

Show 15 quoted lines

>> @@ -505,6 +506,65 @@ static int extend_abbrev_len(const struct object_id *oid, void *cb_data)
>>          return 0;
>>   }
>>
>> +static void find_abbrev_len_for_pack(struct packed_git *p,
>> +                                    struct min_abbrev_data *mad)
>> +{
>> +       int match = 0;
>> +       uint32_t num, last, first = 0;
>> +       struct object_id oid;
>> +
>> +       open_pack_index(p);
> coverity complained here with
>      Calling "open_pack_index" without checking return value
>      (as is done elsewhere 13 out of 15 times).

Good catch! This same line is repeated in unique_in_pack() in this same file, so if this is worth fixing then we should probably fix it there, too.

Show 6 quoted lines

> I think the easiest way out is just a
>
>      if (open_pack_index(p))
>          die(_("Cannot open existing pack idx file for '%s'"), p);
>
> or is there another good approach?

You probably intended to have p->pack_name in the die();

Using `cat *.c | grep -A 2 "if (open_pack_index("` and `cat */*.c | grep -A 2 "if (open_pack_index("` I see a few places that return error codes or quietly fail. The cases that use die() are inside builtin/ so I don't think die() is the right choice here.

Since find_abbrev_len_for_pack() is intended to extend the abbreviation length when necessary, I think a silent return is best here:

     if (open_pack_index(p))
         return;

Thanks, -Stolee