Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:24
Ivan Todoroski [off-list ref] 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.
quoted hunk
--- 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.