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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2022, 17:07 UTC
Message-ID: <xmqqmthearlz.fsf@gitster.g>
In-Reply-To: <f2f8d12929bcbd630b2de3ce770a6763989ffcff.1648146897.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +get_sorted_objects_from_packs () {
> +	git show-index <$(cat) >raw &&
> +	cut -d" " -f2 raw | sort
> +}

As pointed out by Taylor, this "the standard input gives us the name of a file to be read" looks strange. It may work, and it may even give the easiest interface to use by all the callers, but if we were designing a more generic helper function suitable to be added to the test-lib*.sh, we wouldn't design it like so---instead it would be either "we read the contents of the .idx file from the standard input" or "the first argument is the name of the .idx file".

Show 11 quoted lines

>  test_expect_success '--write-midx -b packs non-kept objects' '
> +	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 &&
> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&

We probably want to sanity check "repack -a" by insisting "before" has found exactly one .idx file, before using it this way.

		test_line_count = 1 before &&
		before=$(cat before) &&
		>"${before%.idx}.keep"

Show 14 quoted lines

> +		# Create a non-kept pack-file
> +		test_commit other &&
> +		git repack &&
> +
> +		# Create loose objects
> +		test_commit loose &&
> +
> +		# Repack everything
> +		git repack --write-midx -a -b -d &&
> +
> +		# There should be two pack-files now, the
> +		# old, kept pack and the new, non-kept pack.
> +		find $objdir/pack -name "*.idx" | sort >after &&
> +		test_line_count = 2 after &&

OK.  "after" gets sorted because we will pass it to "comm" later.

> +		find $objdir/pack -name "*.keep" >kept &&
> +		test_line_count = 1 kept &&

Since we've made sure "before" is a one-liner earlier, we could just say

		test_cmp before kept &&

instead, no?

> +		# Get object list from the kept pack.
> +		get_sorted_objects_from_packs \
> +			<before \
> +			>old.objects &&

OK.

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

OK. This should give only one line of output but that is merely assumed. We know after has 2 and before has 1, but haven't made sure that before is a subset of after.

		test_line_count = 1 new-pack &&

> +		get_sorted_objects_from_packs \
> +			<new-pack \
> +			>new.objects &&

OK.

> +		# None of the objects in the new pack should
> +		# exist within the kept pack.
> +		comm -12 old.objects new.objects >shared.objects &&

Great.

Show 5 quoted lines

> +		test_must_be_empty shared.objects
> +	)
>  '
>  
>  test_expect_success TTY '--quiet disables progress' '