Re: [PATCH v2 01/24] pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 21, 2021, 17:15 UTC
Message-ID: <YPhWPVpiGufLVysG@nand.local>
In-Reply-To: <YPfsqJrCTFQXhltJ@coredump.intra.peff.net>

On Wed, Jul 21, 2021 at 05:45:12AM -0400, Jeff King wrote:

Show 16 quoted lines

> > +{
> > +	enum object_type bitmap_type = OBJ_NONE;
> > [...]
> > +
> > +	if (!bitmap_type)
> > +		die("object %s not found in type bitmaps",
> > +		    oid_to_hex(&obj->oid));
>
> I think the suggestion to do:
>
>   if (bitmap_type == OBJ_NONE)
>
> is reasonable here, as it assumes less about the enum. I do think
> OBJ_BAD and OBJ_NONE were chosen with these kind of numeric comparisons
> in mind, but there is no reason to rely on them in places we don't need
> to.

I had to double check your suggestion, because my first question was "what if bitmap_type is OBJ_BAD?" We can call type_name() on OBJ_BAD, but it will return NULL, and we use the return value in a format string unconditionally.

So that would be a problem, but it's impossible for this to ever be OBJ_BAD, because we only set it based on the type bitmaps; so it's either a commit/tree/blob/tag, or none (but not bad).

I took your suggestion, thanks.

> -Peff

Thanks, Taylor