Re: [PATCH 3/3] t: drop debug `cat` calls
Martin Ågren <martin.agren@gmail.com> writes:
Show 8 quoted lines
> 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).
Show 11 quoted lines
> 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.
Show 11 quoted lines
> 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.
