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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Mar 20, 2017, 22:32 UTC
Message-ID: <20170320223250.vyzqyqejxrr4dfp4@genre.crustytoothpaste.net>
In-Reply-To: <CACsJy8ACRgcE0SV2qa4bU+QdT7=WMcBRkV=cr4SR-bcXxTMj9Q@mail.gmail.com>

On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:

Show 15 quoted lines

> 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