From: Jeff King <peff@peff.net>
Date: Fri, 16 Jan 2026 16:45:49 GMT
Subject: Re: [PATCH] cat-file: only use bitmaps when filtering
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:

> > -	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