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

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 20, 2025, 19:57 UTC
Message-ID: <58750d8c-fb1e-4c6d-a356-7cd0e59c621d@gmail.com>
In-Reply-To: <Z9H65fCcv/cybKRa@nand.local>

On 3/12/2025 5:21 PM, Taylor Blau wrote:

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

Show 8 quoted lines

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

Actually, the intention was to make this method less focused on globals, but it in fact uses 'to_pack.objects' when it should use 'list'. Thanks for the careful eye here.

Show 9 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.

Unfortunately, the fact that these are used in the find_deltas() method as an 'unsigned *' reference, these types need to be as they are. I'm hesitant to use 64-bit integers that will eventually cast down to 32-bit in these instances.

Thanks, -Stolee