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

[Junio C Hamano <gitster@pobox.com>]

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.