Re: [PATCH v2 01/24] pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-06-24 23:07:59
On Mon, Jun 21 2021, Taylor Blau wrote:
quoted hunk ↗ jump to hunk
+ 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 :)
quoted hunk ↗ jump to hunk
+ + 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....
quoted hunk ↗ jump to hunk
+ + 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...
quoted hunk ↗ jump to hunk
+ + 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).