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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

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?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]