[GSoC,v2] t1011: replace test -f with test_path_is_file

Commit Message

Siddharth Asthana April 12, 2022, 8:37 p.m. UTC

Use test_path_is_file() instead of 'test -f' for better debugging
information.

Signed-off-by: Siddharth Asthana <[e-mail]>
---
 t/t1011-read-tree-sparse-checkout.sh | 46 ++++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

Comments

Christian Couder April 14, 2022, 8:19 a.m. UTC | #1

On Wed, Apr 13, 2022 at 2:06 AM Siddharth Asthana
<[e-mail]> wrote:
>
> Use test_path_is_file() instead of 'test -f' for better debugging
> information.

Actually it looks like you are also using test_path_is_missing() now,
so you might want to update the commit message, and maybe also the
commit subject.

In the commit message it might be nice if there were some explanations
about why `test_path_is_missing PATH` should be used instead of `!
test_path_is_file PATH` or `test ! -f PATH` or `! test -f PATH`.

The diff part of the patch looks good to me. Thanks!

Junio C Hamano April 14, 2022, 4:42 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> In the commit message it might be nice if there were some explanations
> about why `test_path_is_missing PATH` should be used instead of `!
> test_path_is_file PATH` or `test ! -f PATH` or `! test -f PATH`.

Meaning "the original used '! test -f foo' but what it meant was
that it did not want to see 'foo' on the filesystem, regardless of
its type, so it should have been '!test -e foo' to begin with"?

I guess it does not hurt, as the original would have passed by
mistake if these paths were on the filesystem as directories, but
the new code would behave differently, and even if it is a "bugfix",
it still is a behaviour change that may be worth explaining.

> The diff part of the patch looks good to me. Thanks!

Siddharth Asthana April 16, 2022, 1:55 p.m. UTC | #3

Thanks a lot Christian and Junio for the review. I will send a patch to 
update the commit message.

Best Wishes
Siddharth

Patch

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index dd957be1b7..63a553d7b3 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -57,8 +57,8 @@  test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '
 	read_tree_u_must_succeed -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt result &&
-	test -f init.t &&
-	test -f sub/added
+	test_path_is_file init.t &&
+	test_path_is_file sub/added
 '
 
 test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-checkout and enabled' '
@@ -67,8 +67,8 @@  test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-
 	read_tree_u_must_succeed --no-sparse-checkout -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt result &&
-	test -f init.t &&
-	test -f sub/added
+	test_path_is_file init.t &&
+	test_path_is_file sub/added
 '
 
 test_expect_success 'read-tree with empty .git/info/sparse-checkout' '
@@ -85,8 +85,8 @@  test_expect_success 'read-tree with empty .git/info/sparse-checkout' '
 	S subsub/added
 	EOF
 	test_cmp expected.swt result &&
-	! test -f init.t &&
-	! test -f sub/added
+	test_path_is_missing init.t &&
+	test_path_is_missing sub/added
 '
 
 test_expect_success 'match directories with trailing slash' '
@@ -101,8 +101,8 @@  test_expect_success 'match directories with trailing slash' '
 	read_tree_u_must_succeed -m -u HEAD &&
 	git ls-files -t > result &&
 	test_cmp expected.swt-noinit result &&
-	test ! -f init.t &&
-	test -f sub/added
+	test_path_is_missing init.t &&
+	test_path_is_file sub/added
 '
 
 test_expect_success 'match directories without trailing slash' '
@@ -110,8 +110,8 @@  test_expect_success 'match directories without trailing slash' '
 	read_tree_u_must_succeed -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-noinit result &&
-	test ! -f init.t &&
-	test -f sub/added
+	test_path_is_missing init.t &&
+	test_path_is_file sub/added
 '
 
 test_expect_success 'match directories with negated patterns' '
@@ -129,9 +129,9 @@  EOF
 	git read-tree -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-negation result &&
-	test ! -f init.t &&
-	test ! -f sub/added &&
-	test -f sub/addedtoo
+	test_path_is_missing init.t &&
+	test_path_is_missing sub/added &&
+	test_path_is_file sub/addedtoo
 '
 
 test_expect_success 'match directories with negated patterns (2)' '
@@ -150,9 +150,9 @@  EOF
 	git read-tree -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-negation2 result &&
-	test -f init.t &&
-	test -f sub/added &&
-	test ! -f sub/addedtoo
+	test_path_is_file init.t &&
+	test_path_is_file sub/added &&
+	test_path_is_missing sub/addedtoo
 '
 
 test_expect_success 'match directory pattern' '
@@ -160,8 +160,8 @@  test_expect_success 'match directory pattern' '
 	read_tree_u_must_succeed -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-noinit result &&
-	test ! -f init.t &&
-	test -f sub/added
+	test_path_is_missing init.t &&
+	test_path_is_file sub/added
 '
 
 test_expect_success 'checkout area changes' '
@@ -176,15 +176,15 @@  test_expect_success 'checkout area changes' '
 	read_tree_u_must_succeed -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-nosub result &&
-	test -f init.t &&
-	test ! -f sub/added
+	test_path_is_file init.t &&
+	test_path_is_missing sub/added
 '
 
 test_expect_success 'read-tree updates worktree, absent case' '
 	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&
 	read_tree_u_must_succeed -m -u HEAD^ &&
-	test ! -f init.t
+	test_path_is_missing init.t
 '
 
 test_expect_success 'read-tree will not throw away dirty changes, non-sparse' '
@@ -229,7 +229,7 @@  test_expect_success 'read-tree adds to worktree, absent case' '
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	read_tree_u_must_succeed -u -m HEAD^ &&
-	test ! -f sub/added
+	test_path_is_missing sub/added
 '
 
 test_expect_success 'read-tree adds to worktree, dirty case' '
@@ -248,7 +248,7 @@  test_expect_success 'index removal and worktree narrowing at the same time' '
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout removed &&
 	git ls-files sub/added >result &&
-	test ! -f sub/added &&
+	test_path_is_missing sub/added &&
 	test_must_be_empty result
 '