Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The '--write-midx -b packs non-kept objects' test in t7700-repack.sh
> uses test_subcommand_inexact to check that 'git repack' properly adds
> the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.
> However, the test_subcommand_inexact helper is more flexible than
> initially designed, and this instance is the only one that makes use of
> it: there are additional arguments between 'git pack-objects' and the
> '--honor-pack-keep' flag. In order to make test_subcommand_inexact more
> strict, we need to fix this instance.
>
> This test checks that 'git repack --write-midx -a -b -d' will create a
> new pack-file that does not contain the objects within the kept pack.
> This behavior is possible because of the multi-pack-index bitmap that
> will bitmap objects against multiple packs. Without --write-midx, the
> objects in the kept pack would be duplicated so the resulting pack is
> closed under reachability and bitmaps can be created against it. This is
> discussed in more detail in e4d0c11c0 (repack: respect kept objects with
> '--write-midx -b', 2021-12-20) which also introduced this instance of
> test_subcommand_inexact.
>
> To better verify the intended post-conditions while also removing this
> instance of test_subcommand_inexact, rewrite the test to check the list
> of packed objects in the kept pack and the list of the objects in the
> newly-repacked pack-file _other_ than the kept pack. These lists should
> be disjoint.
>
> Be sure to include a non-kept pack-file and loose objects to be extra
> careful that this is properly behaving with kept packs and not just
> avoiding repacking all pack-files.

Nicely put.

> Co-authored-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

And to be clear, I am totally OK with this usage of my signed-off-by.

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 770d1432046..73452e23896 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
>  	)
>  '
>
> +get_sorted_objects_from_packs () {
> +	git show-index <$(cat) >raw &&

It seems a little odd to me to pass the name of a single file as input
to get_sorted_objects_from_packs over stdin. I probably would have
expected something like `git show-index <"$1" >raw && ...` instead.

We may also want to s/packs/pack, since this function only will handle
one index at a time.

> +	cut -d" " -f2 raw | sort

Having the sort in there is my fault, but after reading this more
carefully it's definitely unnecessary, since show-index will give us
the results in lexical order by object name already.

> +}
> +
>  test_expect_success '--write-midx -b packs non-kept objects' '
> -	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> -		git repack --write-midx -a -b &&
> -	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		# Create a kept pack-file
> +		test_commit base &&
> +		git repack -ad &&
> +		find $objdir/pack -name "*.idx" >before &&

I thought that here it might be easier to say:

    before="$(find $objdir/pack -name "*.idx")"

> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&

...and then replace "$(cat before)" with "$before", along with the
other uses of the before file below. But it gets a little funny when
you want to discover which is the new pack, where it is more natural to
dump the output of comm into a file.

> +		# Get object list from the one non-kept pack-file
> +		comm -13 before after >new-pack &&

You could write "new_pack=$(comm -13 before after)", but debugging this
test would be difficult if the output of comm there contained more than
one line.

> +		get_sorted_objects_from_packs \
> +			<new-pack \

Though we probably want to check that we only get one line anyway here,
since get_sorted_objects_from_packs will barf if we had more than one
line in file new-pack here, too.

Thanks,
Taylor