[PATCH 1/3] t4117: check for files using `test_path_is_file`

[PATCH 1/3] t4117: check for files using `test_path_is_file`

[Martin Ågren]

We `cat` files, but don't inspect or grab the contents in any way. These
`cat` calls look like remnants from a debug session, so it's tempting to
get rid of them. But they do actually verify that the files exist, which
might not necessarily be the case for some failure modes of `git apply
--reject`. Let's not lose that.

Convert the `cat` calls to use `test_path_is_file` instead. This is of
course still a minor change since we no longer verify that the files can
be opened for reading, but that is not something we usually worry about.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t4117-apply-reject.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh
index f7de6f077a..0ee93fe845 100755
--- a/t/t4117-apply-reject.sh
+++ b/t/t4117-apply-reject.sh
@@ -74,7 +74,7 @@ test_expect_success 'apply with --reject should fail but update the file' '
 	test_must_fail git apply --reject patch.1 &&
 	test_cmp expected file1 &&
 
-	cat file1.rej &&
+	test_path_is_file file1.rej &&
 	test_path_is_missing file2.rej
 '
 
@@ -87,7 +87,7 @@ test_expect_success 'apply with --reject should fail but update the file' '
 	test_path_is_missing file1 &&
 	test_cmp expected file2 &&
 
-	cat file2.rej &&
+	test_path_is_file file2.rej &&
 	test_path_is_missing file1.rej
 
 '
@@ -101,7 +101,7 @@ test_expect_success 'the same test with --verbose' '
 	test_path_is_missing file1 &&
 	test_cmp expected file2 &&
 
-	cat file2.rej &&
+	test_path_is_file file2.rej &&
 	test_path_is_missing file1.rej
 
 '
-- 
2.25.0

[PATCH 2/3] t9810: drop debug `cat` call

[Martin Ågren]

We `cat` kwdelfile.c, but don't inspect or grab the contents in any way.
This looks like a remnant from a debug session. Similar to the previous
commit, one could argue that `cat`-ing the file verifies that it didn't
disappear somehow. But because the very next thing we do after `cat`-ing
the file is to `grep` in it, we can safely drop the call to `cat`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t9810-git-p4-rcs.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index 57b533dc6f..e3836888ec 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -294,7 +294,6 @@ test_expect_success 'cope with rcs keyword file deletion' '
 		echo "\$Revision\$" >kwdelfile.c &&
 		p4 add -t ktext kwdelfile.c &&
 		p4 submit -d "Add file to be deleted" &&
-		cat kwdelfile.c &&
 		grep 1 kwdelfile.c
 	) &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.25.0

Re: [PATCH 2/3] t9810: drop debug `cat` call

[Junio C Hamano]

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

> We `cat` kwdelfile.c, but don't inspect or grab the contents in any way.
> This looks like a remnant from a debug session. Similar to the previous
> commit, one could argue that `cat`-ing the file verifies that it didn't
> disappear somehow. But because the very next thing we do after `cat`-ing
> the file is to `grep` in it, we can safely drop the call to `cat`.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  t/t9810-git-p4-rcs.sh | 1 -
>  1 file changed, 1 deletion(-)

The change does make sense, but makes me wonder why we need to
single this one out in a separate patch.  Doesn't this fall into the
same category of fixes done in 3/3?  Exactly the same rationale as
above applies to the first one in 3/3 on t0021---we can safely lose
the cat because the file is immediately grepped for a string, so
missing file will be caught as an error either way.

Puzzled...

> diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
> index 57b533dc6f..e3836888ec 100755
> --- a/t/t9810-git-p4-rcs.sh
> +++ b/t/t9810-git-p4-rcs.sh
> @@ -294,7 +294,6 @@ test_expect_success 'cope with rcs keyword file deletion' '
>  		echo "\$Revision\$" >kwdelfile.c &&
>  		p4 add -t ktext kwdelfile.c &&
>  		p4 submit -d "Add file to be deleted" &&
> -		cat kwdelfile.c &&
>  		grep 1 kwdelfile.c
>  	) &&
>  	git p4 clone --dest="$git" //depot &&

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

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t0021-conversion.sh                  | 1 -
 t/t1406-submodule-ref-store.sh         | 2 +-
 t/t1450-fsck.sh                        | 1 -
 t/t2107-update-index-basic.sh          | 1 -
 t/t3007-ls-files-recurse-submodules.sh | 1 -
 t/t5150-request-pull.sh                | 2 --
 t/t5409-colorize-remote-messages.sh    | 3 +--
 t/t5510-fetch.sh                       | 1 -
 t/t8003-blame-corner-cases.sh          | 1 -
 t/t9300-fast-import.sh                 | 1 -
 t/t9800-git-p4-basic.sh                | 1 -
 11 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 6c6d77b51a..dc664da551 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -795,7 +795,6 @@ test_expect_success PERL 'missing file in delayed checkout' '
 
 	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
 '
 
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index d199d872fb..36b7ef5046 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -75,7 +75,7 @@ test_expect_success 'for_each_reflog()' '
 '
 
 test_expect_success 'for_each_reflog_ent()' '
-	$RUN for-each-reflog-ent HEAD >actual && cat actual &&
+	$RUN for-each-reflog-ent HEAD >actual &&
 	head -n1 actual | grep first &&
 	tail -n2 actual | head -n1 | grep master.to.new
 '
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
 '
 
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
 '
 
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 318b5bce7e..4a08000713 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -130,7 +130,6 @@ test_expect_success '--recurse-submodules and pathspecs setup' '
 
 	git ls-files --recurse-submodules >actual &&
 	test_cmp expect actual &&
-	cat actual &&
 	git ls-files --recurse-submodules "*" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 1ad4ecc29a..c1811ea0f4 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -150,7 +150,6 @@ test_expect_success 'pull request after push' '
 		git request-pull initial origin master:for-upstream >../request
 	) &&
 	sed -nf read-request.sed <request >digest &&
-	cat digest &&
 	{
 		read task &&
 		read repository &&
@@ -179,7 +178,6 @@ test_expect_success 'request asks HEAD to be pulled' '
 		git request-pull initial "$downstream_url" >../request
 	) &&
 	sed -nf read-request.sed <request >digest &&
-	cat digest &&
 	{
 		read task &&
 		read repository &&
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 2a8c449661..5d8f401d8e 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -56,14 +56,13 @@ test_expect_success 'short line' '
 
 test_expect_success 'case-insensitive' '
 	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&
-	cat output &&
 	test_decode_color <output >decoded &&
 	grep "<BOLD;RED>error<RESET>: error" decoded &&
 	grep "<BOLD;RED>ERROR<RESET>: also highlighted" decoded
 '
 
 test_expect_success 'leading space' '
-	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output &&        cat output &&
+	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output &&
 	test_decode_color <output >decoded &&
 	grep "  <BOLD;RED>error<RESET>: leading space" decoded
 '
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5f8f1c287f..4d3a55caaf 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -378,7 +378,6 @@ test_expect_success 'fetch from GIT URL with a non-applying branch.<name>.merge
 # the strange name is: a\!'b
 test_expect_success 'quoting of a strangely named repo' '
 	test_must_fail git fetch "a\\!'\''b" > result 2>&1 &&
-	cat result &&
 	grep "fatal: '\''a\\\\!'\''b'\''" result
 '
 
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 1c5fb1d1f8..9130b887d2 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -173,7 +173,6 @@ test_expect_success 'blame during cherry-pick with file rename conflict' '
 	git show HEAD@{1}:rodent > rodent &&
 	git add rodent &&
 	git blame -f -C -C1 rodent | sed -e "$pick_fc" >current &&
-	cat current &&
 	cat >expected <<-\EOF &&
 	mouse-Initial
 	mouse-Second
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index ae9950a9c2..3e41c58a13 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1047,7 +1047,6 @@ test_expect_success 'M: rename root to subdirectory' '
 	EOF
 	git fast-import <input &&
 	git diff-tree -M -r M4^ M4 >actual &&
-	cat actual &&
 	compare_diff_raw expect actual
 '
 
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 5856563068..c98c1dfc23 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -202,7 +202,6 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 		export PATH &&
 		test_expect_code 1 git p4 clone --dest="$git" //depot >errs 2>&1
 	) &&
-	cat errs &&
 	test_i18ngrep ! Traceback errs
 '
 
-- 
2.25.0

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

[Junio C Hamano]

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.

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

[Martin Ågren]

On Mon, 24 Feb 2020 at 20:33, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> >       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.
>
> >       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.

Heh. You don't want to know how long I waffled on whether to split the
one hunk out to 2/3 and make the rest 3/3 vs. having a slightly larger
2/2.

For the first patch 1/3, if we lose the cat entirely, we risk bugs in
*git* being hidden. For the hunk in patch 2/3, I first thought it was in
the same category, before I realized that kwdelfile.c disappearing would
be a bug in *p4* as opposed to *git p4*. Since we're not in the business
of testing/verifying other people's software, we can afford to drop that
call entirely. At one point, I had this in the commit message, but in
the end I figured one reason for the removal was enough and just kept
the "we'll soon grep" argument.

I realize now that the line between 2/3 and 3/3 is blurry.

FWIW, for patch 3/3 my reasoning was that for the similar concern about
the file not existing, we'd depend on the shell messing up the
redirection quite badly and not creating a file at all, yet continuing
with the && cascade. Which seemed like a pretty crazy bug. And again,
that shouldn't be our worry. (I see now that there's a case in 3/3 where
a buggy test_cmp could delete "actual" and we'd fail to notice after
this commit. That probably also sorts under pathological bugs...)

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

Let me re-roll with 1/2 (=1/3) and 2/2 (=1/3+2/3).

Thanks for a review.

Martin

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

[Junio C Hamano]

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

> For the first patch 1/3, if we lose the cat entirely, we risk bugs in
> *git* being hidden.

Yes, as Peff already said, the change to explicitly test if the file
exists makes perfect sense.

> For the hunk in patch 2/3, I first thought it was in the same
> category, before I realized that kwdelfile.c disappearing would be
> a bug in *p4* as opposed to *git p4*.  Since we're not in the
> business of testing/verifying other people's software, we can
> afford to drop that call entirely.

It is ">kwdelfile.c" shell redirection that creates that file.  I
agree with you that preparing for a bug that "p4 add -t ktext
kwdelfile.c" or "p4 submit" removes the file is a little too much.

Re: [PATCH 1/3] t4117: check for files using `test_path_is_file`

[Jeff King]

On Sun, Feb 23, 2020 at 09:48:34AM +0100, Martin Ågren wrote:

> We `cat` files, but don't inspect or grab the contents in any way. These
> `cat` calls look like remnants from a debug session, so it's tempting to
> get rid of them. But they do actually verify that the files exist, which
> might not necessarily be the case for some failure modes of `git apply
> --reject`. Let's not lose that.
> 
> Convert the `cat` calls to use `test_path_is_file` instead. This is of
> course still a minor change since we no longer verify that the files can
> be opened for reading, but that is not something we usually worry about.

This reasoning makes sense to me. Thanks for being careful in thinking
about possible ramifications of removing these otherwise useless-looking
cat statements.

This is and the other two patches all look good to me.

-Peff