patcht0004: replace test -e with test_path_exists
5 messages between Mar 9, 2026 and Mar 16, 2026, from PRASHANT S BISHT, Junio C Hamano, Jeff King.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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(-)
Show changes to t/t0004-unwritable.sh +4 −4
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
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
On Mon, Mar 09, 2026 at 02:14:10PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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
Jeff King <peff@peff.net> writes:
Show 30 quoted lines
> 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
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(-)
Show changes to t/t4200-rerere.sh +13 −13
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)
PRASHANT S BISHT
