Re: [PATCH 4/4] t5326: test propagating hashcache values

From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH 4/4] t5326: test propagating hashcache values
Date: Tue, 7 Sep 2021 22:30:57 -0400	[thread overview]
Message-ID: <YTggYXtyS/AW4JRF@nand.local> (raw)
In-Reply-To: <87zgsnj0q0.fsf@evledraar.gmail.com>

On Wed, Sep 08, 2021 at 03:46:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Sep 07 2021, Taylor Blau wrote:
>
> > Now that we both can propagate values from the hashcache, and respect
> > the configuration to enable the hashcache at all, test that both of
> > these function correctly by hardening their behavior with a test.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> > index 4ad7c2c969..24148ca35b 100755
> > --- a/t/t5326-multi-pack-bitmaps.sh
> > +++ b/t/t5326-multi-pack-bitmaps.sh
> > @@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '
> >  	)
> >  '
> >
> > +test_expect_success 'hash-cache values are propagated from pack bitmaps' '
> > +	rm -fr repo &&
> > +	git init repo &&
> > +	test_when_finished "rm -fr repo" &&
>
> It seems the need for this "rm -fr repo" dance instead of just relying
> on test_when_finished "rm -rf repo" is because of a 3x tests in a
> function in tb/multi-pack-bitmaps that should probably be combined into
> one, i.e. they share the same logical "repo" setup.

Yeah. There's definitely room for clean-up there if we want to have each
of the tests operate on the same repo. I have always found sharing a
repo between tests difficult to reason about, since we have to assume
that any --run parameters could be passed in.

So I have tended to err on the side of creating and throwing away a new
repo per test, but I understand that's somewhat atypical for Git's
tests.

> > +	(
> > +		cd repo &&
> > +
> > +		git config pack.writeBitmapHashCache true &&
>
> s/git config/test_config/, surely.

No, since this is in a subshell (and we don't care about unsetting the
value of a config in a repo that we're going to throw away, anyways).

(Side-note: since this has a /bin/sh shebang, we can't detect that we're
in a subshell and so test_config actually _would_ work here. But
switching this test to run with /bin/bash where we can detect whether or
not we're in a subshell would cause this test to fail with test_config).

Thanks,
Taylor