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. > In particular: > > > @@ -181,7 +181,7 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index, > > stored->root = root; > > stored->xor = xor_with; > > stored->flags = flags; > > - oidread(&stored->oid, sha1); > > + oidread(&stored->oid, hash); > > > > 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. > 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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
