[PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Rodrigo Michelassi]

From: rodrigocmichelassi <rodmichelassi@gmail.com>

'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach

Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>

Co-authored-by: Isabella Caselli <icaselli@usp.br>
Signed-off-by: Isabella Caselli <icaselli@usp.br>
---
 t/t2400-worktree-add.sh | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 90638fa886..7ab38ac4c4 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -42,8 +42,8 @@ test_expect_success '"add" using - shorthand' '
 
 test_expect_success '"add" refuses to checkout locked branch' '
 	test_must_fail git worktree add zere main &&
-	! test -d zere &&
-	! test -d .git/worktrees/zere
+	! test_path_is_dir zere &&
+	! test_path_is_dir .git/worktrees/zere
 '
 
 test_expect_success 'checking out paths not complaining about linked checkouts' '
@@ -70,14 +70,14 @@ test_expect_success '"add" worktree' '
 test_expect_success '"add" worktree with lock' '
 	git worktree add --detach --lock here-with-lock main &&
 	test_when_finished "git worktree unlock here-with-lock || :" &&
-	test -f .git/worktrees/here-with-lock/locked
+	test_path_is_file .git/worktrees/here-with-lock/locked
 '
 
 test_expect_success '"add" worktree with lock and reason' '
 	lock_reason="why not" &&
 	git worktree add --detach --lock --reason "$lock_reason" here-with-lock-reason main &&
 	test_when_finished "git worktree unlock here-with-lock-reason || :" &&
-	test -f .git/worktrees/here-with-lock-reason/locked &&
+	test_path_is_file .git/worktrees/here-with-lock-reason/locked &&
 	echo "$lock_reason" >expect &&
 	test_cmp expect .git/worktrees/here-with-lock-reason/locked
 '
@@ -412,14 +412,14 @@ test_expect_success '"add --orphan" with empty repository' '
 test_expect_success '"add" worktree with orphan branch and lock' '
 	git worktree add --lock --orphan -b orphanbr orphan-with-lock &&
 	test_when_finished "git worktree unlock orphan-with-lock || :" &&
-	test -f .git/worktrees/orphan-with-lock/locked
+	test_path_is_file .git/worktrees/orphan-with-lock/locked
 '
 
 test_expect_success '"add" worktree with orphan branch, lock, and reason' '
 	lock_reason="why not" &&
 	git worktree add --detach --lock --reason "$lock_reason" orphan-with-lock-reason main &&
 	test_when_finished "git worktree unlock orphan-with-lock-reason || :" &&
-	test -f .git/worktrees/orphan-with-lock-reason/locked &&
+	test_path_is_file .git/worktrees/orphan-with-lock-reason/locked &&
 	echo "$lock_reason" >expect &&
 	test_cmp expect .git/worktrees/orphan-with-lock-reason/locked
 '
@@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '
 
 test_expect_success '"add" worktree with --no-checkout' '
 	git worktree add --no-checkout -b swamp swamp &&
-	! test -e swamp/init.t &&
+	! test_path_is_executable swamp/init.t &&
 	git -C swamp reset --hard &&
 	test_cmp init.t swamp/init.t
 '
@@ -497,7 +497,7 @@ test_expect_success 'put a worktree under rebase' '
 
 test_expect_success 'add a worktree, checking out a rebased branch' '
 	test_must_fail git worktree add new-rebase under-rebase &&
-	! test -d new-rebase
+	! test_path_is_dir new-rebase
 '
 
 test_expect_success 'checking out a rebased branch from another worktree' '
@@ -535,7 +535,7 @@ test_expect_success 'checkout a branch under bisect' '
 		git worktree list >actual &&
 		grep "under-bisect.*detached HEAD" actual &&
 		test_must_fail git worktree add new-bisect under-bisect &&
-		! test -d new-bisect
+		! test_path_is_dir new-bisect
 	)
 '
 
@@ -1165,7 +1165,7 @@ test_expect_success '"add" not tripped up by magic worktree matching"' '
 
 test_expect_success FUNNYNAMES 'sanitize generated worktree name' '
 	git worktree add --detach ".  weird*..?.lock.lock" &&
-	test -d .git/worktrees/---weird-.-
+	test_path_is_dir .git/worktrees/---weird-.-
 '
 
 test_expect_success '"add" should not fail because of another bad worktree' '
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Eric Sunshine]

Thanks for the patch. See below for some comments...

On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi
<rodmichelassi@gmail.com> wrote:
> From: rodrigocmichelassi <rodmichelassi@gmail.com>

The From: header name/address should match your Signed-off-by:
trailer, so you'll probably need to adjust your mailer settings.

> replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

Let's prefix the subject with the area you're touching. In this case,
the test number would be appropriate, so:

    t2400: replace 'test -[efd]' with test_path_* calls

> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach

A better way to convince reviewers that this is a good idea is to
explain why these functions are superior. In this case, it's because
they emit useful diagnostic information when they detect a failing
condition, whereas `test` itself does not.

Please wrap the commit message at about the 72-column mark.

> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>
>
> Co-authored-by: Isabella Caselli <icaselli@usp.br>
> Signed-off-by: Isabella Caselli <icaselli@usp.br>

You'll probably want to order these trailers like this:

    Co-authored-by: Isabella Caselli <icaselli@usp.br>
    Signed-off-by: Isabella Caselli <icaselli@usp.br>
    Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -42,8 +42,8 @@ test_expect_success '"add" using - shorthand' '
>  test_expect_success '"add" refuses to checkout locked branch' '
>         test_must_fail git worktree add zere main &&
> -       ! test -d zere &&
> -       ! test -d .git/worktrees/zere
> +       ! test_path_is_dir zere &&
> +       ! test_path_is_dir .git/worktrees/zere

Take a look at the definition of `test_path_is_dir` from
t/test-lib-functions.sh to see why this change is undesirable:

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

The test wants to assert that those directories do *not* exist, which
means that if `git worktree add` is working correctly, then the
directories indeed will not exist. However, `test_path_is_dir` is
going to complain that they don't exist, which is the opposite of what
we want. So, even though the test will continue to pass following this
change (due to the negation `!`), it's going to be spewing unwanted
and confusing error messages.

What you want instead is a test_path_* function which asserts that a
path does not exist. Probably the closest we have is
`test_path_is_missing` which seems a good semantic match for what this
test intends with regards to those directories.

> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '
>  test_expect_success '"add" worktree with --no-checkout' '
>         git worktree add --no-checkout -b swamp swamp &&
> -       ! test -e swamp/init.t &&
> +       ! test_path_is_executable swamp/init.t &&

If you look at the definition of `test_path_is_executable` in
t/test-lib-functions.sh, you'll see that this change is similarly
undesirable.

The same comments apply to several other changes in this patch.

Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Carlo Marcelo Arenas Belón]

On Sun, Jun 15, 2025 at 11:08:27PM -0800, Rodrigo Michelassi wrote:
> From: rodrigocmichelassi <rodmichelassi@gmail.com>
> 
> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach

Splitting this long line, into multiple lines of about 70ish columns is
better, see the relevant documents in Documentation/ for useful suggestions.

> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>

This certifies that you are the author of the code, an therefore should go
after Isabella's, who might be the original author which you improved upon.

> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '
>  
>  test_expect_success '"add" worktree with --no-checkout' '
>  	git worktree add --no-checkout -b swamp swamp &&
> -	! test -e swamp/init.t &&
> +	! test_path_is_executable swamp/init.t &&

this is not acurate translation, `test -e` is true if there is any "file"
with that name, the equivalent for that helper function would be `test -x`

Carlo

Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for the patch. See below for some comments...
>
> On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi
> <rodmichelassi@gmail.com> wrote:
>> From: rodrigocmichelassi <rodmichelassi@gmail.com>
>
> The From: header name/address should match your Signed-off-by:
> trailer, so you'll probably need to adjust your mailer settings.

This is an in-body header most likely added by git-send-email, so
the name string is what the commit object recorded as its author.
What needs to be adjusted is not the mailer settings, but user.name,
if that is indeed the case.

>> replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'
>
> Let's prefix the subject with the area you're touching. In this case,
> the test number would be appropriate, so:
>
>     t2400: replace 'test -[efd]' with test_path_* calls

Excellent.

>> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach
>
> A better way to convince reviewers that this is a good idea is to
> explain why these functions are superior. In this case, it's because
> they emit useful diagnostic information when they detect a failing
> condition, whereas `test` itself does not.
>
> Please wrap the commit message at about the 72-column mark.

Good.

>> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>
>>
>> Co-authored-by: Isabella Caselli <icaselli@usp.br>
>> Signed-off-by: Isabella Caselli <icaselli@usp.br>
>
> You'll probably want to order these trailers like this:
>
>     Co-authored-by: Isabella Caselli <icaselli@usp.br>
>     Signed-off-by: Isabella Caselli <icaselli@usp.br>
>     Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>

Very true.  And without a blank line in between.

I too spotted the misuse of negation in the test script you spotted.
Thanks for explaining why they are bad and what should be used
instead.

Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> On Sun, Jun 15, 2025 at 11:08:27PM -0800, Rodrigo Michelassi wrote:
>> From: rodrigocmichelassi <rodmichelassi@gmail.com>
>> 
>> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach
>
> Splitting this long line, into multiple lines of about 70ish columns is
> better, see the relevant documents in Documentation/ for useful suggestions.
>
>> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>
>
> This certifies that you are the author of the code, an therefore should go
> after Isabella's, who might be the original author which you improved upon.
>
>> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '
>>  
>>  test_expect_success '"add" worktree with --no-checkout' '
>>  	git worktree add --no-checkout -b swamp swamp &&
>> -	! test -e swamp/init.t &&
>> +	! test_path_is_executable swamp/init.t &&
>
> this is not acurate translation, `test -e` is true if there is any "file"
> with that name, the equivalent for that helper function would be `test -x`

Yes, "test_path_is_missing" is what you want here, without any "!",
to make sure that the path does not exist.

Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'

[Eric Sunshine]

On Mon, Jun 16, 2025 at 12:15 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> From: rodrigocmichelassi <rodmichelassi@gmail.com>
> >
> > The From: header name/address should match your Signed-off-by:
> > trailer, so you'll probably need to adjust your mailer settings.
>
> This is an in-body header most likely added by git-send-email, so
> the name string is what the commit object recorded as its author.
> What needs to be adjusted is not the mailer settings, but user.name,
> if that is indeed the case.

Thanks for the correction.