[1/3] t4117: check for files using `test_path_is_file`
diff mbox series

Message ID dde0f85e5e3dd99a61b83df1b6eb572be8a3ff51.1582447606.git.martin.agren@gmail.com
State New
Headers show
Series
  • [1/3] t4117: check for files using `test_path_is_file`
Related show

Commit Message

Martin Ågren Feb. 23, 2020, 8:48 a.m. UTC
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(-)

Comments

Jeff King Feb. 23, 2020, 9:46 p.m. UTC | #1
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

Patch
diff mbox series

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
 
 '