Re: [PATCH 1/6] t5326: demonstrate potential bitmap corruption

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 22, 2022, 17:57 UTC
Message-ID: <YwPDkW8KemC5Hs/C@nand.local>
In-Reply-To: <3525f8bd-31af-181d-b7a5-6e8a453bbba7@github.com>

On Mon, Aug 22, 2022 at 12:09:55PM -0400, Derrick Stolee wrote:

Show 15 quoted lines

> On 8/19/2022 5:30 PM, Taylor Blau wrote:
>
> > +test_expect_success 'preferred pack change with existing MIDX bitmap' '
> > +	rm -fr repo &&
>
> Does a previous test not delete 'repo' when necessary?
>
> Or, do previous tests re-use 'repo' and now we are in a region
> where we can safely clear that directory? Should we use a
> different name?
>
> > +	git init repo &&
> > +	test_when_finished "rm -fr repo" &&
>
> nit: test_when_finished should be the first line of the test.

The "rm-then-init-then-test_when_finished" is an (unfortunate) pattern extended throughout t5326, mostly that some tests don't clean up "repo" after deleting and recreating it.

But it's easy enough to just use a separate repository, and avoid removing it altogether. Thanks for the suggestion!

Show 13 quoted lines

> > +		# Generate a new MIDX which changes the preferred pack to a pack
> > +		# contained in the existing MIDX, such that not all objects from
> > +		# p2 that appear in the MIDX had their copy selected from p2.
> > +		git multi-pack-index write --bitmap \
> > +			--preferred-pack="pack-$p2.pack" &&
> > +		test_path_is_file $midx &&
> > +		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> > +
> > +		test_must_fail git clone --no-local . clone2
>
> This section is demonstrating the bug. Perhaps we should have
> comments indicating that this is not desired behavior, but is
> being demonstrated so the bug can be fixed by a later change?

Yeah, good call. Thanks again!

Thanks, Taylor