diff mbox series

[GSoC] t1011: replace test -f with test_path_is_file

Message ID 20220409114458.23435-1-siddharthasthana31@gmail.com (mailing list archive)
State Superseded
Headers show
Series [GSoC] t1011: replace test -f with test_path_is_file | expand

Commit Message

Siddharth Asthana April 9, 2022, 11:44 a.m. UTC 
Use test_path_is_file() instead of 'test -f' for better debugging
information.
---
 t/t1011-read-tree-sparse-checkout.sh | 46 ++++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

Comments

Junio C Hamano April 11, 2022, 7:09 p.m. UTC | #1 
Siddharth Asthana <siddharthasthana31@gmail.com> writes:

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

missing Sign-off.

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

Given the definition of the helper function, i.e.

        test_path_is_file () {
                test "$#" -ne 1 && BUG "1 param"
                if ! test -f "$1"
                then
                        echo "File $1 doesn't exist"
                        false
                fi
        }

the new test will _complain_ "init.t doesn't exist" when we have
successfully run the test, while it will be _silent_ when init.t
that _should_ not exist is there.

Which is the complete opposite of the spirit of why we want to use
the helper when we expect the path "$1" to exist, i.e. loudly fail
when our expectation is _not_ met.

$ git grep '! test_path_is' t/

shows that we already have such a misuse of test_path_is_dir in one
place, but luckily we do not have any for test_path_is_file or other
similar helpers.  test_path_is_hidden is sort-of OK as that is not
about verbosity.

In these two test, we do not expect init.t or sub/added to _exist_
at all.  It's not like we are happy if we see init.d exist as a
directory (which is not a file).  test_path_is_missing is probably
the right helper to use.

It is not very plausible that we'd want to assert that existence of
a path as a file the only bad condition (i.e. we are happy if the
path did not exist or it is a directory, symlink, or a socket), so I
think the simple 

	Never use '! test_path_is_file'; test_path_is_missing may be
	what you are looking for.

is a good enough rule.

If not, we could allow the caller to write such a convoluted "only
existence of a path as a file is unacceptable and everything else is
good" assertion as

    test_path_is_file ! init.d

with something like

        test_path_is_file () {
		expecting_file=true
		if test "$1" = "!"
		then
			expecting_file=false
			shift
		fi
                test "$#" -ne 1 && BUG "1 param"
                if test -f "$1"
                then
                	$expecting_file || echo "File $1 exists"
                        $expecting_file
		else
			$expecting_file && echo "File $1 doesn't exist"
                        ! $expecting_file
                fi
        }

but I do not think we want to go that way.
Siddharth Asthana April 12, 2022, 8:21 p.m. UTC | #2 
Thanks a lot Junio for the feedback. I will send a new patch fixing it.

Best Wishes
Siddharth
diff mbox series

Patch

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index dd957be1b7..c0b97a622e 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_file init.t &&
+	! test_path_is_file 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_file 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_file 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_file init.t &&
+	! test_path_is_file 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_file 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_file 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_file 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_file 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_file 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_file sub/added &&
 	test_must_be_empty result
 '