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
