On Wed, Feb 13, 2019 at 12:00:07AM +0000, brian m. carlson wrote: > 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. > > > 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. > > 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
