On Wed, Apr 10, 2024 at 11:02:10AM -0400, Taylor Blau wrote:
> On Tue, Apr 09, 2024 at 07:59:25AM +0200, Patrick Steinhardt wrote:
[snip]
> > diff --git a/midx.c b/midx.c
> > index 41521e019c..6903e9dfd2 100644
> > --- a/midx.c
> > +++ b/midx.c
> > @@ -1661,9 +1661,10 @@ static int write_midx_internal(const char *object_dir,
> > add_chunk(cf, MIDX_CHUNKID_REVINDEX,
> > st_mult(ctx.entries_nr, sizeof(uint32_t)),
> > write_midx_revindex);
> > - add_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,
> > - bitmapped_packs_concat_len,
> > - write_midx_bitmapped_packs);
> > + if (git_env_bool("GIT_TEST_MIDX_WRITE_BTMP", 1))
> > + add_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,
> > + bitmapped_packs_concat_len,
> > + write_midx_bitmapped_packs);
>
> I wish that this were possible to exercise without a new
> GIT_TEST_-variable. I think there are a couple of alternatives:
>
> You could introduce a new GIT_TEST_MIDX_READ_BTMP variable, and then set
> that to control whether or not we read the BTMP chunk. This is what we
> did in:
>
> - 28cd730680d (pack-bitmap: prepare to read lookup table extension,
> 2022-08-14), as well as in
>
> - 7f514b7a5e7 (midx: read `RIDX` chunk when present, 2022-01-25)
>
> . I have a vague preference towards controlling whether or not we read
> the BTMP chunk (as opposed to whether or not we write it) as this
> removes a potential footgun for users who might accidentally disable
> writing a BTMP chunk (in which case you have to rewrite the whole MIDX)
> as opposed to reading it (in which case you just change your environment
> variable).
>
> Of course, that is still using a GIT_TEST_-variable, which is less than
> ideal IMHO. The other alternative would be to store a MIDX file as a
> test fixture in the tree (which we do in a couple of places). But with
> the recent xz shenanigans, I'm not sure that's a great idea either ;-).
I'm happy to convert this to use `GIT_TEST_MIDX_READ_BTMP` instead.
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 2baeabacee..f286805724 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -2049,7 +2049,25 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
> >
> > load_reverse_index(r, bitmap_git);
> >
> > - if (bitmap_is_midx(bitmap_git)) {
> > + if (bitmap_is_midx(bitmap_git) &&
> > + (!multi_pack_reuse || !bitmap_git->midx->chunk_bitmapped_packs)) {
> > + uint32_t preferred_pack_pos;
> > + struct packed_git *pack;
> > +
> > + if (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {
> > + warning(_("unable to compute preferred pack, disabling pack-reuse"));
> > + return;
> > + }
> > +
> > + pack = bitmap_git->midx->packs[preferred_pack_pos];
> > +
> > + ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
> > + packs[packs_nr].p = pack;
> > + packs[packs_nr].bitmap_nr = pack->num_objects;
> > + packs[packs_nr].bitmap_pos = 0;
> > +
> > + objects_nr = packs[packs_nr++].bitmap_nr;
> > + } else if (bitmap_is_midx(bitmap_git)) {
> > for (i = 0; i < bitmap_git->midx->num_packs; i++) {
> > struct bitmapped_pack pack;
> > if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
>
> This all makes sense to me. I think we could make the result slightly
> more readable by handling the case where we're doing multi-pack reuse
> separately from the case where we're not.
>
> I tried to make that change locally to see if it was a good idea, and
> I'm reasonably happy with the result. I can't think of a great way to
> talk about it without just showing the resulting patch (as the
> inter-diff is fairly difficult to read IMHO). So here is the resulting
> patch (forging your s-o-b):
Yup, the result indeed looks nicer, thanks!
[snip]
> The way I would structure this series is to first apply the portion of
> the above patch *without* these lines:
>
> - if (bitmap_is_midx(bitmap_git)) {
> + if (!bitmap_is_midx(bitmap_git) ||
> + !bitmap_git->midx->chunk_bitmapped_packs)
> + multi_pack_reuse = 0;
> +
>
> , so we're still able to reproduce the issue. Then, apply the remaining
> portions (the above diff, the test, and the GIT_TEST_MIDX_WRITE_BTMP
> stuff) to demonstrate that the issue is fixed via a separate commit.
>
> I'm happy to write that up, and equally happy to not do it ;-). Sorry
> for the lengthy review, but thank you very much for spotting and fixing
> this issue.
I'd prefer to leave it as a single patch. Junio has expressed at times
that he doesn't see much value in these splits only to demonstrate a
broken test. An interested reader can easily revert the fix and see that
the test would fail.
Patrick
Attachment:
signature.asc
Description: PGP signature
