Re: [PATCH 3/4] midx: avoid negative array index

[Taylor Blau <me@ttaylorr.com>]

On Tue, May 20, 2025 at 04:04:26PM +0100, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> nth_midxed_pack_int_id() returns the index of the pack file in the multi
> pack index's list of packfiles that the specified object. The index is
> returned as a uint32_t. Storing this in an int will make the index
> negative if the most significant bit is set. Fix this by using uint32_t
> as the rest of the code does. This is unlikely to be a practical problem
> as it requires the multipack index to reference 2^31 packfiles.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  midx-write.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/midx-write.c b/midx-write.c
> index 2ee381e8fcd..38a458d7322 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1566,7 +1566,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
>  					  _("Counting referenced objects"),
>  					  m->num_objects);
>  	for (i = 0; i < m->num_objects; i++) {
> -		int pack_int_id = nth_midxed_pack_int_id(m, i);
> +		uint32_t pack_int_id = nth_midxed_pack_int_id(m, i);
>  		count[pack_int_id]++;
>  		display_progress(progress, i + 1);
>  	}
> @@ -1697,7 +1697,7 @@ static void fill_included_packs_batch(struct repository *r,
>
>  	total_size = 0;
>  	for (i = 0; total_size < batch_size && i < m->num_packs; i++) {
> -		int pack_int_id = pack_info[i].pack_int_id;
> +		uint32_t pack_int_id = pack_info[i].pack_int_id;
>  		struct packed_git *p = m->packs[pack_int_id];
>  		uint64_t expected_size;
>
> --
> 2.49.0.897.gfad3eb7d210
>

Thanks for catching these. I was going to comment on it as something I
noticed while reading the first patch, but I'm glad that you addressed
it here.

It looks like these two declarations date back to:

 - 19575c7c8e (multi-pack-index: implement 'expire' subcommand, 2019-06-10)
 - ce1e4a105b (midx: implement midx_repack(), 2019-06-10)

(both of which were merged via 4308d81d45 (Merge branch
'ds/midx-expire-repack', 2019-07-19)). But I think these have always had
the wrong type, since the pack_int_id field comes from commit fe1ed56f5e
(midx: sort and deduplicate objects from packfiles, 2018-07-12), where
it was first introduced as a uint32_t.

I actually think these all should size_t's since they're indexing into
an array, but that can be dealt with outside of this series, since this
is an obvious improvement.

Thanks,
Taylor