Re: [PATCH v2 2/5] t6421: add tests checking for excessive object downloads during merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2021, 04:49 UTC
Message-ID: <xmqqk0mtm6yr.fsf@gitster.g>
In-Reply-To: <0f786cfb4c9507a224b1dc3ee6eca9f5ac5c2ae8.1623796907.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +		for i in `test_seq 1 88`; do
> +			echo content $i >dir/unchanged/file_$i
> +		done &&

        for i in $(...)
        do
                ... || return 1
        done &&

Show 8 quoted lines

> +test_expect_merge_algorithm failure failure 'Objects downloaded for single relevant rename' '
> +	test_setup_repo &&
> +	git clone --sparse --filter=blob:none "file://$(pwd)/server" objects-single &&
> +	(
> +		cd objects-single &&
> +
> +		git rev-list --objects --all --missing=print |
> +			grep '\?' >missing-objects-before &&

The closing and reopening of single quote here is somewhat misleading. Wouldn't

		grep "?" >... &&

work just as well?

> +		git rev-list --objects --all --missing=print |
> +			grep ^? >missing-objects-after &&
> +		test_cmp missing-objects-before missing-objects-after |
> +			grep "^[-+]?" >found-and-new-objects &&

I do not think we specify what test_cmp's output looks like; we only guarantee that it is the right tool to use for expecting two paths have the same contents, and it may give some human readable output.

Do not assume you can grep in the output; use "diff -u" if you mean you expect things that exist on the left side and missing from the right hand side are prefixed with '-' and vice versa for '+'.

Or sort "missing-objects-after" and "missing-objects-before" and run "comm" on them, which might be more stable. Depending on the order two invocations of rev-list spews out the "missing" objects, you may even see the same object mentioned with "-" and "+" in the diff output if lines appear to be moved around.

Show 6 quoted lines

> +		# We should not have any NEW missing objects
> +		! grep ^+ found-and-new-objects &&
> +		# Fetched 2+1=3 objects, so should have 3 fewer missing objects
> +		test_line_count = 3 found-and-new-objects
> +	)
> +'

I think similar comments apply to the tests in the remainder of the patch.

Thanks.