On Mon, Apr 15, 2024 at 06:51:16PM -0400, Taylor Blau wrote: > On Mon, Apr 15, 2024 at 08:41:25AM +0200, Patrick Steinhardt wrote: [snip] > > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh > > index 70d1b58709..5d7d321840 100755 > > --- a/t/t5326-multi-pack-bitmaps.sh > > +++ b/t/t5326-multi-pack-bitmaps.sh > > @@ -513,4 +513,21 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' ' > > ) > > ' > > > > +for allow_pack_reuse in single multi > > +do > > + test_expect_success "reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse" ' > > + test_when_finished "rm -rf midx-without-btmp" && > > + git init midx-without-btmp && > > + ( > > + cd midx-without-btmp && > > + test_commit initial && > > + > > + git repack -Adbl --write-bitmap-index --write-midx && > > `-b` is redundant with `--write-bitmap-index`. Oops, right. > > + GIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \ > > + pack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err && > > A small note here, but setting stdin to read from /dev/null is > unnecessary with `--all.` Is it really? Executing `git pack-objects --all --stdout` on my system blocks until stdin is closed. It _seems_ to work in the tests alright, but doesn't work outside of them. Which is puzzling on its own. > > + test_must_be_empty err > > + ) > > + ' > > +done > > + > > This test looks like it's exercising the right thing, but I'm not sure > why it was split into two separate tests. Perhaps to allow the two to > fail separately? Exactly. It makes it easier to see which of both tests fails in case only one does. > Either way, the repository initialization, test_commit, and repacking > could probably be combined into a single step to avoid re-running them > for different values of $allow_pack_reuse. > > I would probably have written: > > git init midx-without-btmp && > ( > cd midx-without-btmp && > > test_commit base && > git repack -adb --write-midx && > > for c in single multi > do > GIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$c pack-objects \ > --all --use-bitmap-index --stdout >/dev/null 2>err && > test_must_be_empty err || return 1 > done > ) > > TBH, I would like to see this test cleaned up before merging this one > down. But otherwise this patch is looking good. So I'm a bit torn here. I think your proposed way to test things is inferior regarding usability, even though it is superior regarding performance. We could move the common setup into a separate test, but that has the issue that tests cannot easily be run as self-contained units. Patrick
Attachment:
signature.asc
Description: PGP signature
