Re: [PATCH v2 01/24] pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 24, 2021, 23:02 UTC
- Message-ID
- <87k0mizws5.fsf@evledraar.gmail.com>
- In-Reply-To
- <a18baeb0b42994ebcb216df5fe69459ba9a33795.1624314293.git.me@ttaylorr.com>
On Mon, Jun 21 2021, Taylor Blau wrote:
Show 19 quoted lines
> + 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 :)
> +
> + if (!bitmap_type)
> + die("object %s not found in type bitmaps",
> + oid_to_hex(&obj->oid));It feels a bit magical to use an enum and then assume to know the enum's values, I know we do "type < 0" all over the place, but I'd think "if (bitmap_type == OBJ_NONE)" would be better here....
> +
> + if (bitmaps_nr > 1)
> + die("object %s does not have a unique type",
> + oid_to_hex(&obj->oid));Or just check the bitmaps_nr instead:
if (!bitmaps_nr)
die("found none");
else if (bitmaps_nr > 1)
...;Just bikeshedding...
Show 6 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).