Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Git List" <git@vger.kernel.org>, "Ed Maste" <emaste@freebsd.org>
Subject: Re: [PATCH] t4210: detect REG_ILLSEQ dynamically
Date: Wed, 13 May 2020 09:20:10 -0700
Message-ID: <xmqqmu6bvmbp.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAPig+cTUc2-ddWQ+oww5Ez7_N9qKgCuErk8OuOgPkXNrACdppw@mail.gmail.com> (Eric Sunshine's message of "Wed, 13 May 2020 11:44:53 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 11:16 Carlo Marcelo Arenas Belón
2020-05-13 15:44 ` Eric Sunshine
2020-05-13 16:20   ` Junio C Hamano [this message]
2020-05-13 20:18   ` Carlo Marcelo Arenas Belón
2020-05-13 20:37     ` Junio C Hamano
2020-05-13 21:04       ` Carlo Marcelo Arenas Belón
2020-05-13 18:02 ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2020-05-13 20:40   ` Eric Sunshine
2020-05-15 18:00   ` Junio C Hamano
2020-05-15 18:18     ` Carlo Marcelo Arenas Belón
2020-05-15 19:51 ` [PATCH " Carlo Marcelo Arenas Belón
2020-05-15 20:24   ` Junio C Hamano
2020-05-15 21:48     ` Junio C Hamano
2020-05-18 18:44   ` [PATCH v3 0/2] auto detect REG_ILLSEQ Carlo Marcelo Arenas Belón
2020-05-18 18:44     ` [PATCH v3 1/2] t/helper: teach test-regex to report pattern errors (like REG_ILLSEQ) Carlo Marcelo Arenas Belón
2020-05-18 20:15       ` Junio C Hamano
2020-05-18 18:44     ` [PATCH v3 2/2] t4210: detect REG_ILLSEQ dynamically and skip affected tests Carlo Marcelo Arenas Belón

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmu6bvmbp.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=carenas@gmail.com \
    --cc=emaste@freebsd.org \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git