Re: [PATCH] cat-file: only use bitmaps when filtering

From: Jeff King <peff@peff.net>
Date: Jan 16, 2026, 16:45 UTC
Message-ID: <20260116164549.GA1636797@coredump.intra.peff.net>
In-Reply-To: <aV4Xa9ceY4ahYj2m@pks.im>

On Wed, Jan 07, 2026 at 09:20:59AM +0100, Patrick Steinhardt wrote:

Show 13 quoted lines

> > -	if (bitmap && !for_each_bitmapped_object(bitmap, &opt->objects_filter,
> > -						 batch_one_object_bitmapped, &payload)) {
> > +	if (opt->objects_filter.choice != LOFC_DISABLED &&
> > +	    (bitmap = prepare_bitmap_git(the_repository)) &&
> > +	    !for_each_bitmapped_object(bitmap, &opt->objects_filter,
> > +				       batch_one_object_bitmapped, &payload)) {
> >  		struct packed_git *pack;
> >  
> >  		repo_for_each_pack(the_repository, pack) {
> 
> Yeah, this seems like a reasonable change to me. I would've preferred to
> avoid the assignment in the conditional, but other than that this looks
> good to me.

Yeah, I tried to rewrite this to avoid the assignment-in-conditional, but the logic gets even more convoluted because we need to get to the "else" clause from multiple places then.

I do think that the for_each_bitmapped_object() interface is making this a bit harder. Before it was added, the main bitmap entry point was always prepare_bitmap_walk(), which opened the bitmap file itself (and only after doing the cheap can_filter_bitmap() check).

But here that doesn't quite work, because we need the bitmap_index to persist after the for_each_bitmapped_object() call so that we can check bitmap_index_contains_pack() on it.

I was tempted to suggest that for_each_bitmapped_object() should return the bitmap_index itself, and then this code would become:

   if (filter.choice != LOFC_DISABLED)
	   bitmap = for_each_bitmapped_object(filter, cb, &payload);
   if (bitmap) {
	   /* we iterated those objects; check for other packs */
   } else {
	   /* we did nothing; look at all packs */
   }

   free_bitmap_index(bitmap);

which is not too bad. Mostly I wanted to make the fix as small as possible, but I was also a little hesitant to tweak the API when we have only one caller (and we don't know what a second caller might want). But we could always revisit it on top.

-Peff