Re: [PATCH 04/31] pack-bitmap: replace sha1_to_hex

From: Jeff King <peff@peff.net>
Date: Feb 14, 2019, 04:41 UTC
Message-ID: <20190214044145.GA19071@sigill.intra.peff.net>
In-Reply-To: <20190213000007.GG684736@genre.crustytoothpaste.net>

On Wed, Feb 13, 2019 at 12:00:07AM +0000, brian m. carlson wrote:

Show 15 quoted lines

> On Tue, Feb 12, 2019 at 01:37:49AM -0500, Jeff King wrote:
> > On Tue, Feb 12, 2019 at 01:22:29AM +0000, brian m. carlson wrote:
> > > -static uint32_t find_object_pos(const unsigned char *sha1)
> > > +static uint32_t find_object_pos(const unsigned char *hash)
> > 
> > Isn't this really just a "struct object_id"? Why don't we want to use
> > that here?
> > 
> > I suspect it may be partially because our khash is storing raw sha1s.
> > But shouldn't we also be converted that to store object_ids?
> 
> I think probably there are some more places that we could convert here.
> There may have been one or two places that weren't convertible because
> we ended up passing data from some sort of buffer around. I'll take
> another look.

Thanks. I don't want to derail you too much if you have a series of other changes on top. And moving to "hash" here is a step in the right direction. But if we can take it all the way to object_id while we're looking at it, I think that's preferable.

Show 9 quoted lines

> > >  	hash_pos = kh_put_sha1(index->bitmaps, stored->oid.hash, &ret);
> > 
> > This last line (which is actually from the previous patch) is going to
> > always truncate the stored data to 20 bytes, isn't it?
> 
> No, I don't think it does. The _sha1 variant stores pointers to unsigned
> char, while the _oid variant stores the entire struct object_id (not
> just a pointer to it). We don't care how much data the pointer points
> to.

Oh, you're right. I was thinking it actually stored the 20-byte sequences, but I was just reading it wrong. Sorry for the confusion.

Show 8 quoted lines

> > I think we need to define a kh_oid. We have the "set" version already in
> > oidset.[ch]; I think we need to make that more public.
> 
> I wrote this quite a bit before that code came in, which is probably why
> I didn't do that originally. I seem to recall last time I looked at this
> that there was some reason that hoisting this didn't work as I expected
> due to header include order, but I'll take a look and see if I can
> figure out a way to do this.

Great, thanks!

-Peff