From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 07 Jan 2026 08:20:59 GMT
Subject: Re: [PATCH] cat-file: only use bitmaps when filtering
Message-ID: <aV4Xa9ceY4ahYj2m@pks.im>
In-Reply-To: <20260106102558.GA68914@coredump.intra.peff.net>

On Tue, Jan 06, 2026 at 05:25:58AM -0500, Jeff King wrote:
> There are some timing tests in 8002e8ee18 that claim the non-filter case
> is not regressed, but it's not clear to me exactly which commands were
> run. Given the size of the repo there and the fact that it's more I/O
> bound, I'd guess it is using an output format that requires looking at
> the packed objects.

I honestly can't remember anymore, either. I really should stick with
the actual command run in the Hyperfine benchmark names.

> You can see the mean user CPU does jump by almost 2 seconds in the
> timings given there. So it may be that the problem was there but drowned
> out by I/O noise. At any rate, the new t/perf tests isolate it better
> and reproduce consistently for me.

Could be, yeah.

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 505ddaa12f..3cb725940d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -846,12 +846,14 @@ static void batch_each_object(struct batch_options *opt,
>  		.callback = callback,
>  		.payload = _payload,
>  	};
> -	struct bitmap_index *bitmap = prepare_bitmap_git(the_repository);
> +	struct bitmap_index *bitmap = NULL;
>  
>  	for_each_loose_object(the_repository->objects, batch_one_object_loose, &payload, 0);
>  
> -	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.

Thanks!

Patrick