Re: [PATCH 3/3] t: drop debug `cat` calls

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

Martin Ågren <martin.agren@gmail.com> writes:

> We `cat` files, but don't inspect or grab the contents in any way.
> Unlike in an earlier commit, there is no reason to suspect that these
> files could be missing, so `cat`-ing them is just wasted effort.
> ...
>  	rm -rf repo-cloned &&
>  	test_must_fail git clone repo repo-cloned 2>git-stderr.log &&
> -	cat git-stderr.log &&
>  	grep "error: .missing-delay\.a. was not filtered properly" git-stderr.log

Loss of cat does not change if git-stderr.log file went missing for
whatever reason (e.g. typo while updating the test), just like 2/3,
so the debugging cruft can safely be removed.

> -	$RUN for-each-reflog-ent HEAD >actual && cat actual &&
> +	$RUN for-each-reflog-ent HEAD >actual &&
>  	head -n1 actual | grep first &&

Very similar, as lack of 'actual' for whatever reason will manifest
in a failure for "grep" from finding 'first' anyway.

I sort of sympathetic to your desire to have two classes and
separate/distribute them into 2/3 and 3/3, but to me the line
between two classes looks like it was drawn along a wrong axis.

The proposed log message for this step hints that the criteria for
tests to be thrown into this category is that the output of 'cat' is
not used in any useful way, and the input of 'cat' is known to exist
(i.e. so "cat fails if the file is missing" is not part of the test),
but that applies to the one instance singled out in 2/3 (i.e. the
file in question, kwdelfile.c, is created with shell redirection,
just like "actual" file above is).

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index 02478bc4ec..d09eff503c 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -141,7 +141,6 @@ test_expect_success 'email without @ is okay' '
>  	git update-ref refs/heads/bogus "$new" &&
>  	test_when_finished "git update-ref -d refs/heads/bogus" &&
>  	git fsck 2>out &&
> -	cat out &&
>  	! grep "commit $new" out
>  '

This one on the other hand *DOES* rely on 'out' being created; we do
not want to take the failing 'grep' as a sign of success if it is
because 'out' is missing.

> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> index 2242cd098e..a30b7ca6bc 100755
> --- a/t/t2107-update-index-basic.sh
> +++ b/t/t2107-update-index-basic.sh
> @@ -9,7 +9,6 @@ Tests for command-line parsing and basic operation.
>  
>  test_expect_success 'update-index --nonsense fails' '
>  	test_must_fail git update-index --nonsense 2>msg &&
> -	cat msg &&
>  	test -s msg
>  '

This one does not.  "test -s msg" on non-existent msg will fail, so
this is closer to category 2/3.

So, I am OK to have two patches that catch two classes, but the
division between 2/3 and 3/3 in this series does not look the right
one.

I am also OK to have a single patch with updated log message, saying
"removal of 'cat <file>' may miss a failure mode that <file> did not
get created, which would have been caught as a test failure in the
original, but the <file>s used by cats removed in this patch are
either impossible to be missing (because a preceding step in the
test created it, or the &&-cascade would have failed if it failed to
create the file), or followed by another step in the test that would
fail if the file is missing (e.g. running grep on the file), so it is
safe to drop these cats", or something like that.