Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id

> On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:
>> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
>> >                 ds->hex_pfx[i] = c;
>> >                 if (!(i & 1))
>> >                         val <<= 4;
>> > -               ds->bin_pfx[i >> 1] |= val;
>> > +               ds->bin_pfx.hash[i >> 1] |= val;
>>
>> The indexing makes me a bit nervous, especially since diff context
>> here is too narrow to see. It would be nice if this code (at the
>> beginning of init_object_disambiguation) is converted here too
>>
>>         if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
>>                 return -1;
>
> Well, I think that's the way I would have written that text at the top
> of the function.  I expect that we'll end up turning GIT_SHA1_HEXSZ into
> a global named something like current_hash_len via global
> search-and-replace, so it will always be the right length.
>
> The indexing should be safe because len is guaranteed to be sufficiently
> small, and I feel like it we would have seen it break by now if it had
> had an overflow.  i will always be in the range [0, 40) (for SHA-1), so
> i >> 1 should always be in [0, 20).
>
> Am I understanding you correctly and if so, does that assuage your
> concerns, or did you mean something else?