Re: [PATCH v2 01/24] pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 14, 2021, 17:24 UTC
- Message-ID
- <YO8dwitqG/pw2Mqn@nand.local>
- In-Reply-To
- <87k0mizws5.fsf@evledraar.gmail.com>
On Fri, Jun 25, 2021 at 01:02:56AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 26 quoted lines
>
> On Mon, Jun 21 2021, Taylor Blau wrote:
>
> > + enum object_type bitmap_type = OBJ_NONE;
> > + int bitmaps_nr = 0;
> > +
> > + if (bitmap_get(tdata->commits, pos)) {
> > + bitmap_type = OBJ_COMMIT;
> > + bitmaps_nr++;
> > + }
> > + if (bitmap_get(tdata->trees, pos)) {
> > + bitmap_type = OBJ_TREE;
> > + bitmaps_nr++;
> > + }
> > + if (bitmap_get(tdata->blobs, pos)) {
> > + bitmap_type = OBJ_BLOB;
> > + bitmaps_nr++;
> > + }
> > + if (bitmap_get(tdata->tags, pos)) {
> > + bitmap_type = OBJ_TAG;
> > + bitmaps_nr++;
> > + }
>
> This made me wonder if this could be better with something like the
> HAS_MULTI_BITS() macro, but that trick probably can't be applied here in
> any shape or form :)Right; since we're looking at the same bit position in each of the type-level bitmaps, we can't just OR them together, since all of the bits are in the same place.
And really, the object_type enum doesn't have values that tell us the type of an object by looking at just a single bit. So HAS_MULTI_BITS(OBJ_BLOB) would return "true", since OBJ_BLOB is 3.
Show 11 quoted lines
> > +
> > + if (bitmap_type != obj->type)
> > + die("object %s: real type %s, expected: %s",
> > + oid_to_hex(&obj->oid),
> > + type_name(obj->type),
> > + type_name(bitmap_type));
>
> To argue against myself (sort of) about that "== OBJ_NONE" above, if
> we're not assuming that then it's sort of weird not to also assume that
> type_name(type) won't return a NULL in the case of OBJ_NONE, which it
> does (but this code guards against).I tend to agree. To restate what you're saying: by the time we get to the type_name(bitmap_type) we know that bitmap_type is non-zero, so we assume it's OK to call type_name() on it.
Of course, the object_type_strings does handle the zero argument, so this is probably a little academic, but good to think through nonetheless.
Thanks, Taylor