Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2020, 16:20 UTC
Message-ID: <xmqqmu6bvmbp.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cTUc2-ddWQ+oww5Ez7_N9qKgCuErk8OuOgPkXNrACdppw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 13 quoted lines

>> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
>> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
>>  {
>> -       int flags = 0;
>> +       int ret, opt_s = 0, flags = 0;
>> [...]
>> +       if (!strcmp(argv[1], "--silent")) {
>> +               opt_s++;
>
> Nit: When reading the declaration of 'opt_s', I found the name
> inscrutable; it was only upon reading the actual code, that I
> understood that it reflected whether --silent had been used. How about
> giving it a more easily-understood name, such as 'silent'?

Yup, that is a reasonable fix to the readability problem.

Show 10 quoted lines

>> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
>> @@ -56,21 +56,29 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
>> +test_have_prereq GETTEXT_LOCALE &&
>> +! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED &&
>> +test_set_prereq REGEX_ILLSEQ
>
> Nit: Is there precedent for formatting the code like this? At first
> glance, I read these as three distinct statements rather than one
> large composite statement. I wonder if indenting the continuation
> lines would make this more easily-digested.

It's not like we are running three tests and expecting all of them to succeed (or report a failure otherwise). The first two are the conditions we want to trigger the action the third one represents.

I'd prefer to see it written with "if then fi". That would also be a worthwhile readability fix.

Show 9 quoted lines

>> -       test_set_prereq REGEX_ILLSEQ
>> -       test_set_prereq POSIXPERM
>> -       test_set_prereq BSLASHPSPEC
>> -       test_set_prereq EXECKEEPSPID
>> -       ;;
>
> The commit message explains why you remove the 'REGEX_ILLSEQ', but why
> are all the other lines removed, as well? Such removal seems unrelated
> to the stated purpose of this patch.

Yup, I was wondering about the same thing.

Thanks.