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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 12, 2025, 21:21 UTC
Message-ID: <Z9H65fCcv/cybKRa@nand.local>
In-Reply-To: <c047fbd7f275ef79695b5e1356075705e6fd7bc8.1741571455.git.gitgitgadget@gmail.com>

On Mon, Mar 10, 2025 at 01:50:52AM +0000, Derrick Stolee via GitGitGadget wrote:

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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