Re: [PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2024, 18:54 UTC
Message-ID: <xmqq1q1z9rmt.fsf@gitster.g>
In-Reply-To: <afa25c8806b0e80f1d3ed46f29eb164cab4583ac.1724793201.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 17 quoted lines

> @@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
>  			     struct bitmapped_pack *pack,
>  			     size_t bitmap_pos,
>  			     uint32_t pack_pos,
> +			     off_t offset,
>  			     struct bitmap *reuse,
>  			     struct pack_window **w_curs)
>  {
> -	off_t offset, delta_obj_offset;
> +	off_t delta_obj_offset;
>  	enum object_type type;
>  	unsigned long size;
>  
>  	if (pack_pos >= pack->p->num_objects)
>  		return -1; /* not actually in the pack */
>  
> -	offset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);

We are now passing redundant piece of information "offset" that could be derived from two other parameters, which feels a bit icky,

I suspect that try_partial_reuse() can be taught not to require pack_pos and instead work entirely on offset

 - The caller can pass a very large offset to represent "not
   actually in the pack" early return condition.

 - The logic to punt on a delta pointing backwards can be done by
   comparing the base_offset and offset, instead of comparing the
   base_pos and pack_pos.

but it may not be worth the effort, unless we are going to do similar clean-up globally in all the codepaths that access packfiles.

Thanks.