Re: [RFC PATCH v2] t4210: detect REG_ILLSEQ dynamically

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 13, 2020, 20:40 UTC
Message-ID: <CAPig+cTeUh6MP4LLvgx1BEr7csWG_o+1H_RZS=Curx4pGLSAcA@mail.gmail.com>
In-Reply-To: <20200513180213.45866-1-carenas@gmail.com>

On Wed, May 13, 2020 at 2:02 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 5 quoted lines

> [...]
> The description of the first test which wasn't accurate has been corrected,
> and unlike the original fix from 7187c7bbb8, all added entries to test-lib
> are no longer needed and only the 2 affected engines will have their tests
> suppressed.

I see this paragraph was updated in response to my question about why those additional test-lib.sh variable assignments were being dropped by the patch. However, this explanation gives no actual detail about why those assignments are unneeded, thus their removal is just as much of a head-scratcher as was v1 in which the commit message did not talk about them at all.

Show 12 quoted lines

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
> +       ret = regcomp(&r, pat, flags);
> +       if (ret) {
> +               if (!silent) {
> +                       regerror(ret, &r, errbuf, sizeof(errbuf));
> +                       die("failed regcomp() for pattern '%s' (%s)",
> +                               pat, errbuf);
> +               } else
> +                       return 1;
> +       }

This is pure nit, and I wouldn't necessarily want to see a re-roll just for this, but you could lose an indentation level and make the code a bit easier to grok by structuring it like this:

    if (ret) {
        if (silent)
            return 1;
        regerror(...);
        die(...);
    }