Re: [PATCH 11/24] pack-bitmap: simplify `reuse_partial_packfile_from_bitmap()` signature

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]