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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2025, 04:15 UTC
Message-ID: <xmqq1prk490o.fsf@gitster.g>
In-Reply-To: <CAPig+cT1VfY8QiUvrrV3-obTBP1439b6iwaebJtGwML5MScnQA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 8 quoted lines

> 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.

Show 6 quoted lines

>> 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.

Show 8 quoted lines

>> '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.

Show 10 quoted lines

>> 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.