On Tue, Nov 28, 2023 at 02:08:24PM -0500, Taylor Blau wrote:
> 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@xxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
