Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 16, 2024, 04:47 UTC
Message-ID: <Zh4C9xhOC4BVslyX@tanuki>
In-Reply-To: <Zh2vZB/60QlLYjUZ@nand.local>

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]

Show 20 quoted lines

> > 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.

Show 5 quoted lines

> > +			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.

Show 9 quoted lines

> > +			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.

Show 23 quoted lines

> 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