Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`

[Taylor Blau <me@ttaylorr.com>]

On Thu, May 29, 2025 at 01:47:44PM -0700, Junio C Hamano wrote:
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index b9f1d86604..8ddc150778 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> >  	for (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {
> >  		if (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {
> >  			warning(_("could not open pack %s"),
> > -				bitmap_git->midx->pack_names[i]);
> > +				nth_midxed_pack_name(bitmap_git->midx, i));
>
> This loop runs from 0 to (num_packs + num_packs_in_base).  I
> understand if it runs from num_packs_in_base to (num_packs +
> num_packs_in_base), iterating only on this layer, but probably this
> just tries to open everything (i.e. in addition to num_packs we
> have, we know num_packs_in_base packs are there in our base layer(s),
> so we iterate from 0 to that number).
>
> The updated code converts the global 'i', which runs from 0 to
> "everything under us" num_packs + num_packs_in_base, to
> corresponding layer midx plus offset in it, so it looks good, but
> then, is the original reference to bitmap_git->midx->pack_names[i]
> even correct?  If we have a base, i can run larger than
> bitmap_git->midx->num_packs, which is the size of the array
> bitmap_git->midx->pack_names[].
>
> Or, unlike how the proposed log message portrayed this change as
> (i.e. code clean up), does this patch fix real bugs that manifest
> only when midx files are chained?

Right; the original code was buggy if we had a failure opening a MIDX'd
pack outside of the base layer in an incremental MIDX bitmap. Reading
the proposed log message again, I see what you're saying. I am happy to
clarify that this is indeed a bugfix, not just a cleanup.

> > @@ -2469,7 +2469,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
> >  			struct bitmapped_pack pack;
> >  			if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
> >  				warning(_("unable to load pack: '%s', disabling pack-reuse"),
> > -					bitmap_git->midx->pack_names[i]);
> > +					nth_midxed_pack_name(bitmap_git->midx, i));
> >  				free(packs);
> >  				return;
> >  			}
>
> Similar to the above, this is also in a loop that runs from 0 to
> num_packs+num_packs_in_base.  Is the array access to find the name
> for the error message in the original even correct when midx are
> chained?

Right; this spot suffers from the same bug as the previous hunk.

> > diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> > index ac81390899..fbed0f6919 100644
> > --- a/t/helper/test-read-midx.c
> > +++ b/t/helper/test-read-midx.c
> > @@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,
> >  	printf("\nnum_objects: %d\n", m->num_objects);
> >
> >  	printf("packs:\n");
> > -	for (i = 0; i < m->num_packs; i++)
> > -		printf("%s\n", m->pack_names[i]);
> > +	for (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;
> > +	     i++)
> > +		printf("%s\n", nth_midxed_pack_name(m, i));
>
> OK.  This used to iterate from 0 to num_packs using the local
> offset.  Now it iterates from num_packs_in_base to num_packs_in_base+num_packs,
> meaning we iterate over packs in the given midx.  No change in
> behaviour, as accesses to m->pack_names[i] using the local offset in
> the original was correct, and the updated code iterates using the
> global offset.  This is not a bugfix but is a code cleanup.

Right.

> > @@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)
> >  		return 1;
> >  	}
> >
> > -	printf("%s\n", midx->pack_names[preferred_pack]);
> > +	printf("%s\n", nth_midxed_pack_name(midx, preferred_pack));
>
> Again, is the original buggy when midx are chained?

This is also a bugfix.

Thanks,
Taylor