Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2012, 17:40 UTC
Message-ID: <7v4ntaj61t.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4F715DBF.6030909@gmx.net>

Ivan Todoroski <grnch@gmx.net> writes:

Show 5 quoted lines

> These test cases focus only on testing the parsing of refs on stdin,
> without bothering with the rest of the fetch-pack machinery. We pass in
> the refs using different combinations of command line and stdin and then
> we watch fetch-pack's stdout to see whether it prints all the refs we
> specified and in the same order as we specified them.

Thanks for writing what it does concisely and clearly. It makes it very pleasant to review the patch.

It is sensible to expect that we see all the refs we told it to fetch, but I do not think it is sensible to require they come in the same order as we have given them to the command.

Show 14 quoted lines

> For the --stateless-rpc tests we cannot easily execute fetch-pack to the
> end because that would require simulating the remote protocol. We settle
> for only checking 2 cases:
>
> 1) Whether fetch-pack correctly fails to parse the refs if they are not
> terminated by a flush packet
>
> 2) Whether fetch-pack finishes parsing the refs without error when they
> are correctly terminated by a flush packet
>
> The fetch-pack invocation fails in both cases due to the missing remote
> side of the protocol, but it fails in different ways which allows us to
> determine how the refs parsing ended by inspecting the different error
> messages.

Ick.

Show 34 quoted lines

> ---
>  t/t5500-fetch-pack.sh |  100 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 9bf69e9a0f..f4de7d07c1 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -248,4 +248,104 @@ test_expect_success 'clone shallow object count' '
>  	grep "^count: 52" count.shallow
>  '
>  
> +
> +cat >stdin.exp <<EOF
> +refs/heads/C
> +refs/heads/A
> +refs/heads/D
> +refs/tags/C
> +refs/heads/B
> +refs/tags/A
> +refs/heads/E
> +refs/tags/B
> +refs/tags/E
> +refs/tags/D
> +EOF
> +
> +test_expect_success 'setup tests for --stdin' '
> +	for head in C D E F; do
> +		add $head
> +	done &&
> +	for head in A B C D E F; do
> +		git tag $head $head
> +	done
> +'

Drop ";" and write "do" on its own line with proper indentation.

Show 8 quoted lines

> +
> +test_expect_success 'fetch refs from cmdline, make sure it still works OK' '
> +	cd client &&
> +	git fetch-pack --no-progress .. $(cat ../stdin.exp) |
> +	cut -d " " -f 2 > ../stdin.act &&
> +	cd .. &&
> +	test_cmp stdin.exp stdin.act
> +'

 - Do not chdir around without being in a subprocess ();

 - Do not place the command you are testing that might crash on the
   upstream of the pipe;

 - style;

i.e.

	(
		cd client &&
                git fetch-pack --no-progress .. $(cat ../stdin.exp) >../stdin.raw
	) &&
        cut -d " " -f 2 <stdin.raw | sort >stdin.act &&
	test_cmp stdin.exp stdin.act

Note that I lifted the "in the same order" requirement, which should not be there. You may need to adjust the hardcoded stdin.exp to be sorted.

Show 8 quoted lines

> +test_expect_success 'fetch refs from stdin' '
> +	cd client &&
> +	cat ../stdin.exp |
> +	git fetch-pack --stdin --no-progress .. |
> +	cut -d " " -f 2 > ../stdin.act &&
> +	cd .. &&
> +	test_cmp stdin.exp stdin.act
> +'

In addition to the comments on the previous one:

 - Do not pipe output from cat.

i.e.

	(
		cd client &&
                git fetch-pack ... <../stdin.exp >stdin.raw
	) &&
	cut -d " " -f 2 <stdin.raw | sort >stdin.act &&
	test_cmp stdin.exp stdin.act

By the way, why are these not called "expect" and "actual" like most other tests?

Show 8 quoted lines

> +test_expect_success 'fetch mixed refs from cmdline and stdin in right order' '
> +	cd client &&
> +	tail -n +5 ../stdin.exp |
> +	git fetch-pack --stdin --no-progress .. $(head -n 4 ../stdin.exp) |
> +	cut -d " " -f 2 > ../stdin.act &&
> +	cd .. &&
> +	test_cmp stdin.exp stdin.act
> +'

Ditto.

Do we want to have a separate test to see what happens when there are dups in the input?

Show 10 quoted lines

> +# remove final newline and insert random spaces, NULs, and empty lines
> +head -c -1 stdin.exp | sed -e '
> +	1i
> +	2s/^\|$/   /g
> +	4s/^/    /
> +	6s/$/    /
> +	8s/$/\n     /
> +	9s/$/Ztrailing garbage/
> +	9s/^\|$/    /g
> +' | tr "Z" "\000" > stdin.spaces

Somebody may want to try this sed expression on Macs and BSDs, but more importantly...

Show 8 quoted lines

> +test_expect_success 'ignore leading/trailing spaces, empty lines and NULs' '
> +	cd client &&
> +	cat ../stdin.spaces |
> +	git fetch-pack --stdin --no-progress .. |
> +	cut -d " " -f 2 > ../stdin.act &&
> +	cd .. &&
> +	test_cmp stdin.exp stdin.act
> +'

... it is unclear why it is a good thing to let the input with garbage produce a result instead of erroring out.

The rest of the patch skimmed but not thoroughly reviewed; seems to have similar issues as already have been pointed out above.