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

From: Junio C Hamano <[email-masked]>
Date: Sun, 15 Jun 2025 21:15:19 -0700

Eric Sunshine <[email-masked]> writes:

> Thanks for the patch. See below for some comments...
>
> On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi
> <[email-masked]> wrote:
>> From: rodrigocmichelassi <[email-masked]>
>
> 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 <[email-masked]>
>>
>> Co-authored-by: Isabella Caselli <[email-masked]>
>> Signed-off-by: Isabella Caselli <[email-masked]>
>
> You'll probably want to order these trailers like this:
>
>     Co-authored-by: Isabella Caselli <[email-masked]>
>     Signed-off-by: Isabella Caselli <[email-masked]>
>     Signed-off-by: Rodrigo Michelassi <[email-masked]>

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.