Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > Thanks for the patch. See below for some comments... > > On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi > <rodmichelassi@xxxxxxxxx> wrote: >> From: rodrigocmichelassi <rodmichelassi@xxxxxxxxx> > > 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@xxxxxxxxx> >> >> Co-authored-by: Isabella Caselli <icaselli@xxxxxx> >> Signed-off-by: Isabella Caselli <icaselli@xxxxxx> > > You'll probably want to order these trailers like this: > > Co-authored-by: Isabella Caselli <icaselli@xxxxxx> > Signed-off-by: Isabella Caselli <icaselli@xxxxxx> > Signed-off-by: Rodrigo Michelassi <rodmichelassi@xxxxxxxxx> 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.
