Re: [PATCH v3 18/25] t5326: test multi-pack bitmap behavior

From: Jeff King <peff@peff.net>
Date: Aug 12, 2021, 21:07 UTC
Message-ID: <YRWNm+HlgBE5E0ia@coredump.intra.peff.net>
In-Reply-To: <YRWMYg2rvv7HjGE+@coredump.intra.peff.net>

On Thu, Aug 12, 2021 at 05:02:26PM -0400, Jeff King wrote:

Show 9 quoted lines

> > +# We'll be writing our own midx and bitmaps, so avoid getting confused by the
> > +# automatic ones.
> > +GIT_TEST_MULTI_PACK_INDEX=0
> > +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
> 
> This latter variable doesn't do anything at this point in the series.
> Probably not a big deal (it is simply a noop until then), but if it's
> not hard, it may make sense to bump the "respect ... WRITE_BITMAP" patch
> earlier in the series.

Reading the other patches, I guess you ordering was to "fix" each of the tests preemptively, and then add the knob at the end. That's OK by me. For an alternate test-mode like this, I usually wouldn't worry about bisectability, but it doesn't hurt. Somebody reading the commits later won't have any trouble finding the definition of the WRITE_BITMAP variable added in the subsequent patch.

Show 8 quoted lines

> > +test_expect_success '--no-bitmap is respected when bitmaps exist' '
> > +	git multi-pack-index write --bitmap &&
> > +
> > +	test_commit respect--no-bitmap &&
> > +	GIT_TEST_MULTI_PACK_INDEX=0 git repack -d &&
> 
> Do we need to set this env variable? We've already set it to 0 at the
> top of the script.

By the way, there were a few more of these later in the script that could be cleaned up, too.

-Peff