[v2] t7611: replace test -f with test_path_is* helpers

Commit Message

Meet Soni Dec. 20, 2024, 1:06 p.m. UTC

Replace `test -f` and `test ! -f` with `test_path_is_file` and
`test_path_is_missing` for better verbosity.

While `test -f` ensures that the file exists and is a regular file,
`test_path_is_file` provides clearer error messages on failure. Similarly,
`test ! -f`, used to check either the absence of a regular file or the
presence of a directory, has been replaced with `test_path_is_missing` for
better readability and consistent handling of such cases.

Signed-off-by: Meet Soni <[e-mail]>
---
Updated commit message and subject according to review.

 t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Dec. 20, 2024, 2:16 p.m. UTC | #1

Meet Soni <[e-mail]> writes:

> Replace `test -f` and `test ! -f` with `test_path_is_file` and
> `test_path_is_missing` for better verbosity.

OK.  "verbosity" -> "debuggability" perhaps, but that is minor.

> While `test -f` ensures that the file exists and is a regular file,
> `test_path_is_file` provides clearer error messages on failure.

Correct.

> Similarly,
> `test ! -f`, used to check either the absence of a regular file or the
> presence of a directory, has been replaced with `test_path_is_missing` for
> better readability and consistent handling of such cases.

This is misleading.  If you rewrite "test ! -f" that intends to be
happy when it sees a directory, you would be changing the meaning of
the program if you replace it with test_path_is_missing.  Rather,
when you see "test ! -f foo", you should first think if the intent
of the test script there is to allow presence of "foo" that is not a
file, and if that is not the case, in other words, the "test ! -f"
you see should have been written "test ! -e" (i.e. "I do not want to
see 'foo' there on the filesystem, no matter what kind of filesystem
entity it is!"), it would give us the same meaning with better
debuggability to rewrite such "test ! -f" with test_path_is_missing.

IOW, unlike "test -f foo" that can pretty much blindly replaceable
with "test_path_is_file foo" without thinking at all, you must be a
bit more careful.  And _if_ you did such a more careful analysis
before replacing "test ! -f", then you should restate the above,
perhaps something along the lines of ...

    On the other hand, `test ! -f` literally means there should not
    be a file (implication is that we are OK if there is a directory
    or device nodes or other things at the given path).  But by
    looking at each of these in the test individually, many of them
    should rather have said "test ! -e", i.e. "there shouldn't be
    anything at the given path on the filesystem".  Rewrite these
    cases to test_path_is_missing for better debuggability.

I didn't check myself if all of the "test ! -f" you touched are
indeed the original should have said "test ! -e".  Hopefully you
have done it yourself?

Thanks.

Patch

diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index d6975ca48d..1a251485e1 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -54,13 +54,13 @@  test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '
 '
 
 test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)"
 '
 
 test_expect_success 'fails without MERGE_HEAD (completed merge)' '
 	git merge clean_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	# Merge successfully completed
 	post_merge_head="$(git rev-parse HEAD)" &&
 	test_must_fail git merge --abort 2>output &&
@@ -68,7 +68,7 @@  test_expect_success 'fails without MERGE_HEAD (completed merge)' '
 '
 
 test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$post_merge_head" = "$(git rev-parse HEAD)"
 '
 
@@ -79,10 +79,10 @@  test_expect_success 'Forget previous merge' '
 test_expect_success 'Abort after --no-commit' '
 	# Redo merge, but stop before creating merge commit
 	git merge --no-commit clean_branch &&
-	test -f .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_HEAD &&
 	# Abort non-conflicting merge
 	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff)" &&
 	test -z "$(git diff --staged)"
@@ -91,10 +91,10 @@  test_expect_success 'Abort after --no-commit' '
 test_expect_success 'Abort after conflicts' '
 	# Create conflicting merge
 	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_HEAD &&
 	# Abort conflicting merge
 	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff)" &&
 	test -z "$(git diff --staged)"
@@ -105,7 +105,7 @@  test_expect_success 'Clean merge with dirty index fails' '
 	git add foo &&
 	git diff --staged > expect &&
 	test_must_fail git merge clean_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff)" &&
 	git diff --staged > actual &&
@@ -114,7 +114,7 @@  test_expect_success 'Clean merge with dirty index fails' '
 
 test_expect_success 'Conflicting merge with dirty index fails' '
 	test_must_fail git merge conflict_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff)" &&
 	git diff --staged > actual &&
@@ -129,10 +129,10 @@  test_expect_success 'Reset index (but preserve worktree changes)' '
 
 test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
 	git merge --no-commit clean_branch &&
-	test -f .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_HEAD &&
 	# Abort merge
 	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&
@@ -141,10 +141,10 @@  test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
 
 test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '
 	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_HEAD &&
 	# Abort merge
 	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&
@@ -159,7 +159,7 @@  test_expect_success 'Fail clean merge with conflicting dirty worktree' '
 	echo xyzzy >> bar &&
 	git diff > expect &&
 	test_must_fail git merge --no-commit clean_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&
@@ -168,7 +168,7 @@  test_expect_success 'Fail clean merge with conflicting dirty worktree' '
 
 test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '
 	test_must_fail git merge conflict_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&
@@ -183,7 +183,7 @@  test_expect_success 'Fail clean merge with matching dirty worktree' '
 	echo bart > bar &&
 	git diff > expect &&
 	test_must_fail git merge --no-commit clean_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&
@@ -194,7 +194,7 @@  test_expect_success 'Fail conflicting merge with matching dirty worktree' '
 	echo barf > bar &&
 	git diff > expect &&
 	test_must_fail git merge conflict_branch &&
-	test ! -f .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
 	test -z "$(git diff --staged)" &&
 	git diff > actual &&