Re: [PATCH 10/13] pack-objects: refactor path-walk delta phase

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 10, 2025 at 01:50:52AM +0000, Derrick Stolee via GitGitGadget wrote:
> ---
>  builtin/pack-objects.c       | 82 +++++++++++++++++++++++++-----------
>  pack-objects.h               | 12 ++++++
>  t/t5300-pack-object.sh       |  8 +++-
>  t/t5530-upload-pack-error.sh |  6 ---
>  4 files changed, 75 insertions(+), 33 deletions(-)

Ah, nice :-). This is where you implement the idea that you were
mentioning back in

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c756ce50dd7..c5a3129c88e 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3233,6 +3233,51 @@ static int should_attempt_deltas(struct object_entry *entry)
>  	return 1;
>  }
>
> +static void find_deltas_for_region(struct object_entry *list UNUSED,

Interesting, it looks like "list" here is UNUSED in this patch. On first
read I figured that you were going to make use of it in later patches,
but it looks like it remains UNUSED at the tip of my local copy of this
series.

Is this a stray change from when you were writing this? Something else?

> +				   struct packing_region *region,
> +				   unsigned int *processed)
> +{
> +	struct object_entry **delta_list;
> +	uint32_t delta_list_nr = 0;

I know that we have a lot of "_nr" and "_alloc" variables in the
pack-objects code that use uint32_t as their type, but we should prefer
size_t for these in the future, even if it breaks the existing pattern.

As much as I can grok of the implementation through the rest of the
patch makes sense to me and looks good.

Thanks,
Taylor