[PATCH] t0004: replace test -e with test_path_exists

[PATCH] t0004: replace test -e with test_path_exists

[PRASHANT S BISHT]

Replace old-style path existence checks with the modern test_path_exists
helper function that provides clearer diagnostic messages on failure.
When test -e fails, the output gives no indication of what went wrong.

These instances were found using:

  git grep "test -[efd]" t/ | grep -v "if test"

as suggested in the microproject ideas.
---
 t/t0004-unwritable.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 3bdafbae0f..2a9fc781b6 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -21,7 +21,7 @@ test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable reposi
 	test_must_fail git write-tree 2>out.write-tree
 '
 
-test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree'
+test_lazy_prereq WRITE_TREE_OUT 'test_path_exists "$TRASH_DIRECTORY/out.write-tree"'
 test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' '
 	cat >expect <<-\EOF &&
 	error: insufficient permission for adding an object to repository database .git/objects
@@ -36,7 +36,7 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository
 	test_must_fail git commit -m second 2>out.commit
 '
 
-test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit'
+test_lazy_prereq COMMIT_OUT 'test_path_exists "$TRASH_DIRECTORY/out.commit"'
 test_expect_success COMMIT_OUT 'commit output on unwritable repository' '
 	cat >expect <<-\EOF &&
 	error: insufficient permission for adding an object to repository database .git/objects
@@ -52,7 +52,7 @@ test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repo
 	test_must_fail git update-index file 2>out.update-index
 '
 
-test_lazy_prereq UPDATE_INDEX_OUT 'test -e "$TRASH_DIRECTORY"/out.update-index'
+test_lazy_prereq UPDATE_INDEX_OUT 'test_path_exists "$TRASH_DIRECTORY/out.update-index"'
 test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' '
 	cat >expect <<-\EOF &&
 	error: insufficient permission for adding an object to repository database .git/objects
@@ -69,7 +69,7 @@ test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '
 	test_must_fail git add file 2>out.add
 '
 
-test_lazy_prereq ADD_OUT 'test -e "$TRASH_DIRECTORY"/out.add'
+test_lazy_prereq ADD_OUT 'test_path_exists "$TRASH_DIRECTORY/out.add"'
 test_expect_success ADD_OUT 'add output on unwritable repository' '
 	cat >expect <<-\EOF &&
 	error: insufficient permission for adding an object to repository database .git/objects
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] t0004: replace test -e with test_path_exists

[Junio C Hamano]

PRASHANT S BISHT <prashantjee2025@gmail.com> writes:

> -test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree'
> +test_lazy_prereq WRITE_TREE_OUT 'test_path_exists "$TRASH_DIRECTORY/out.write-tree"'

I suspect this is utterly wrong.  As you wrote in the proposed log
message, test_path_exists is *NOT* about checking if the path
exists.  It rather is about *expecting* for the path to exist, and
fail *LOUDLY* if it does not.

You need to _think_ if we want a LOUD failure when somebody checks
if a path exists and conditionally skip setting a test prerequisite
when the path does not exist.  The original code is trying to be
quiet, as the check is done not because existence of the checked
path is good and lack of it is a test failure.  Lack of the path is
expected on places where the prerequisite is not set, and that by
itself is not a test failure that you want a LOUD report about.

Re: [PATCH] t0004: replace test -e with test_path_exists

[Jeff King]

On Mon, Mar 09, 2026 at 02:14:10PM -0700, Junio C Hamano wrote:

> PRASHANT S BISHT <prashantjee2025@gmail.com> writes:
> 
> > -test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree'
> > +test_lazy_prereq WRITE_TREE_OUT 'test_path_exists "$TRASH_DIRECTORY/out.write-tree"'
> 
> I suspect this is utterly wrong.  As you wrote in the proposed log
> message, test_path_exists is *NOT* about checking if the path
> exists.  It rather is about *expecting* for the path to exist, and
> fail *LOUDLY* if it does not.
> 
> You need to _think_ if we want a LOUD failure when somebody checks
> if a path exists and conditionally skip setting a test prerequisite
> when the path does not exist.  The original code is trying to be
> quiet, as the check is done not because existence of the checked
> path is good and lack of it is a test failure.  Lack of the path is
> expected on places where the prerequisite is not set, and that by
> itself is not a test failure that you want a LOUD report about.

I'm not sure I agree. Verbose prereq blocks can help with debugging.
Normally you would not see them at all, but if you are investigating why
a prereq did not trigger, you may want more output.

Without "-v" you would not see the output either way, like:

  ok 1 # skip some test (missing FOO)

But with it, it is the difference between:

  checking prerequisite: FOO
  
  mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
  (
  	cd "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
  	test -e foo
  
  )
  prerequisite FOO not satisfied
  ok 1 # skip some test (missing FOO)

and:

  checking prerequisite: FOO
  
  mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
  (
  	cd "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
  	test_path_exists foo
  
  )
  Path foo doesn't exist
  prerequisite FOO not satisfied
  ok 1 # skip some test (missing FOO)

Probably it's pretty obvious for a one-liner like this, but I think it
would help for a longer block.

-Peff

Re: [PATCH] t0004: replace test -e with test_path_exists

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Without "-v" you would not see the output either way, like:
>
>   ok 1 # skip some test (missing FOO)
>
> But with it, it is the difference between:
>
>   checking prerequisite: FOO
>   
>   mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
>   (
>   	cd "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
>   	test -e foo
>   
>   )
>   prerequisite FOO not satisfied
>   ok 1 # skip some test (missing FOO)
>
> and:
>
>   checking prerequisite: FOO
>   
>   mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
>   (
>   	cd "$TRASH_DIRECTORY/prereq-test-dir-FOO" &&
>   	test_path_exists foo
>   
>   )
>   Path foo doesn't exist
>   prerequisite FOO not satisfied
>   ok 1 # skip some test (missing FOO)

Sorry, but I am not convinced.

It is as if satisfying FOO is the norm, and not satisifying FOO,
i.e., missing path "foo", is something worth reporting about.

If the test reported both success and failure loudly, it may be a
different story, though.

> Probably it's pretty obvious for a one-liner like this, but I think it
> would help for a longer block.
>
> -Peff

[PATCH v2] t4200: convert test -[df] checks to test_path_* helpers

[PRASHANT S BISHT]

Replace old-style path existence checks in t4200-rerere.sh with
the appropriate test_path_* helper functions. These helpers provide
clearer diagnostic messages on failure than the raw shell test
builtin.

Signed-off-by: Prashant S Bisht <prashantjee2025@gmail.com>
---
 t/t4200-rerere.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 204325f4d5..1717f407c8 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -72,7 +72,7 @@ test_expect_success 'nothing recorded without rerere' '
 	rm -rf .git/rr-cache &&
 	git config rerere.enabled false &&
 	test_must_fail git merge first &&
-	! test -d .git/rr-cache
+	test_path_is_missing .git/rr-cache
 '
 
 test_expect_success 'activate rerere, old style (conflicting merge)' '
@@ -84,8 +84,8 @@ test_expect_success 'activate rerere, old style (conflicting merge)' '
 	sha1=$(sed "s/	.*//" .git/MERGE_RR) &&
 	rr=.git/rr-cache/$sha1 &&
 	grep "^=======\$" $rr/preimage &&
-	! test -f $rr/postimage &&
-	! test -f $rr/thisimage
+	test_path_is_missing $rr/postimage &&
+	test_path_is_missing $rr/thisimage
 '
 
 test_expect_success 'rerere.enabled works, too' '
@@ -110,8 +110,8 @@ test_expect_success 'set up rr-cache' '
 
 test_expect_success 'rr-cache looks sane' '
 	# no postimage or thisimage yet
-	! test -f $rr/postimage &&
-	! test -f $rr/thisimage &&
+	test_path_is_missing $rr/postimage &&
+	test_path_is_missing $rr/thisimage &&
 
 	# preimage has right number of lines
 	cnt=$(sed -ne "/^<<<<<<</,/^>>>>>>>/p" $rr/preimage | wc -l) &&
@@ -167,7 +167,7 @@ test_expect_success 'first postimage wins' '
 	git show first:a1 | sed "s/To die: t/To die! T/" >expect &&
 
 	git commit -q -a -m "prefer first over second" &&
-	test -f $rr/postimage &&
+	test_path_is_file $rr/postimage &&
 
 	oldmtimepost=$(test-tool chmtime --get -60 $rr/postimage) &&
 
@@ -190,14 +190,14 @@ test_expect_success 'rerere clear' '
 	mv $rr/postimage .git/post-saved &&
 	echo "$sha1	a1" | tr "\012" "\000" >.git/MERGE_RR &&
 	git rerere clear &&
-	! test -d $rr
+	test_path_is_missing $rr
 '
 
 test_expect_success 'leftover directory' '
 	git reset --hard &&
 	mkdir -p $rr &&
 	test_must_fail git merge first &&
-	test -f $rr/preimage
+	test_path_is_file $rr/preimage
 '
 
 test_expect_success 'missing preimage' '
@@ -205,7 +205,7 @@ test_expect_success 'missing preimage' '
 	mkdir -p $rr &&
 	cp .git/post-saved $rr/postimage &&
 	test_must_fail git merge first &&
-	test -f $rr/preimage
+	test_path_is_file $rr/preimage
 '
 
 test_expect_success 'set up for garbage collection tests' '
@@ -230,16 +230,16 @@ test_expect_success 'set up for garbage collection tests' '
 
 test_expect_success 'gc preserves young or recently used records' '
 	git rerere gc &&
-	test -f $rr/preimage &&
-	test -f $rr2/preimage
+	test_path_is_file $rr/preimage &&
+	test_path_is_file $rr2/preimage
 '
 
 test_expect_success 'old records rest in peace' '
 	test-tool chmtime =$just_over_60_days_ago $rr/postimage &&
 	test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
 	git rerere gc &&
-	! test -f $rr/preimage &&
-	! test -f $rr2/preimage
+	test_path_is_missing $rr/preimage &&
+	test_path_is_missing $rr2/preimage
 '
 
 rerere_gc_custom_expiry_test () {
-- 
2.50.1 (Apple Git-155)