On Mon, Apr 15, 2024 at 08:34:33AM +0200, Patrick Steinhardt wrote:
> > 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.
Thanks, I think that is definitely an improvement.
> > 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.
Yeah, to be clear, I wasn't suggesting adding a test_expect_failure
here. I was suggesting instead that you do a refactoring that doesn't
change the behavior in the first step, and then add the test plus the
fix (after it is made easier to land by the first step) in the second
step.
But I don't feel strongly about it whatsoever, so I think that the new
version you have here is fine. I'll take a closer look at it right
now...
Thanks,
Taylor
