From: Jeff King <peff@peff•net>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] cat-file: only use bitmaps when filtering
Date: Fri, 16 Jan 2026 11:45:49 -0500 [thread overview]
Message-ID: <20260116164549.GA1636797@coredump.intra.peff.net> (raw)
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
prev parent reply other threads:[~2026-01-16 16:45 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-06 10:25 [PATCH] cat-file: only use bitmaps when filtering Jeff King
2026-01-07 8:20 ` Patrick Steinhardt
2026-01-16 16:45 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260116164549.GA1636797@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox