Re: [PATCH 11/24] pack-bitmap: simplify `reuse_partial_packfile_from_bitmap()` signature
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 7, 2023, 13:13 UTC
- Message-ID
- <ZXHE7_KwukSRBET1@tanuki>
- In-Reply-To
- <432854b27c6731bd6ab1fa739b3a086ec0a90be8.1701198172.git.me@ttaylorr.com>
On Tue, Nov 28, 2023 at 02:08:24PM -0500, Taylor Blau wrote:
Show 48 quoted lines
> The signature of `reuse_partial_packfile_from_bitmap()` currently takes
> in a bitmap, as well as three output parameters (filled through
> pointers, and passed as arguments), and also returns an integer result.
>
> The output parameters are filled out with: (a) the packfile used for
> pack-reuse, (b) the number of objects from that pack that we can reuse,
> and (c) a bitmap indicating which objects we can reuse. The return value
> is either -1 (when there are no objects to reuse), or 0 (when there is
> at least one object to reuse).
>
> Some of these parameters are redundant. Notably, we can infer from the
> bitmap how many objects are reused by calling bitmap_popcount(). And we
> can similar compute the return value based on that number as well.
>
> As such, clean up the signature of this function to drop the "*entries"
> parameter, as well as the int return value, since the single caller of
> this function can infer these values themself.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> builtin/pack-objects.c | 16 +++++++++-------
> pack-bitmap.c | 16 +++++++---------
> pack-bitmap.h | 7 +++----
> 3 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 107154db34..2bb1b64e8f 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3946,13 +3946,15 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
> if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
> return -1;
>
> - if (pack_options_allow_reuse() &&
> - !reuse_partial_packfile_from_bitmap(
> - bitmap_git,
> - &reuse_packfile,
> - &reuse_packfile_objects,
> - &reuse_packfile_bitmap)) {
> - assert(reuse_packfile_objects);
> + if (pack_options_allow_reuse())
> + reuse_partial_packfile_from_bitmap(bitmap_git, &reuse_packfile,
> + &reuse_packfile_bitmap);
> +
> + if (reuse_packfile) {
> + reuse_packfile_objects = bitmap_popcount(reuse_packfile_bitmap);
> + if (!reuse_packfile_objects)
> + BUG("expected non-empty reuse bitmap");We're now re-computing `bitmap_popcount()` for the bitmap a second time. But I really don't think this is ever going to be a problem in practice given that it only does a bunch of math. Any performance regression would thus ultimately be drowned out by everything else.
In other words: this is probably fine.
Patrick