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

[Junio C Hamano <gitster@pobox.com>]

Ivan Todoroski <grnch@gmx.net> writes:

> 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.

> 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.

> ---
>  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.

> +
> +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.

> +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?

> +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?

> +# 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...

> +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.