Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 7, 2018, 13:53 UTC
Message-ID: <CAM0VKjkf=51i1YPqdNm=pyPHaNNguXLu0T1iHDYv28jW92QTow@mail.gmail.com>
In-Reply-To: <20180126182734.GB27618@sigill.intra.peff.net>

On Fri, Jan 26, 2018 at 7:27 PM, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

> On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:
>
>> The second 'test_i18ngrep' invocation in the test 'curl redirects
>> respect whitelist' is missing its filename parameter.  This has
>> remained unnoticed since its introduction in f4113cac0 (http: limit
>> redirection to protocol-whitelist, 2015-09-22), because it would only
>> cause the test to fail if Git was built with a sufficiently old
>> libcurl version.  The test's two ||-chained 'test_i18ngrep'
>> invocations are supposed to check that either one of the two patterns
>> is present in 'git clone's error message.  As it happens, the first
>> invocation covers the error message from any reasonably up-to-date
>> libcurl, thus the second invocation, the one without the filename
>> parameter, isn't executed at all.  Apparently no one has run the test
>> suite's httpd tests with such an old libcurl in the last 2+ years, or
>> at least they haven't bothered to notify us about the failed test.
>
> Interesting find.
>
> The "too old" curl is older than 7.19.4, which we actually fail to build
> with since v2.12.0. So they probably did not even get as far as the
> tests. ;)

Oh, OK, I was not aware of that. The oldest non-maintenance release with the missing filename parameter is v2.7.0, so that's still a 5 releases time frame to notice it.

Anyway, I'm preparing v2 of this series, and I'm not sure what to do about this.

  - Should I simply drop the "your curl version is too old" pattern?  It
    would make sense, but it just doesn't feel quite right to remove it
    while the corresponding printf() is still there, even if it can't be
    triggered anymore.  However, cleaning up the curl version checks in
    http.c to remove this message is beyond the scope of this patch
    series.

  - Or leave it almost-as-is, only dropping the now unnecessary curly
    braces as Simon pointed out.  And perhaps a bit of update to the
    commit message.

I'd prefer the second option.

Show 8 quoted lines

>> Fix this by consolidating the two patterns into a single extended
>> regexp, eliminating the need for an ||-chained second 'test_i18ngrep'
>> invocation.
>
> OK. Once upon a time I think we had trouble with "grep -E", since some
> older systems had only "egrep". But I see we've introduced some "grep
> -E" invocations as far back as 2013 and nobody has complained, so it's
> probably fine.

Yeah, first I went with the more traditional "\(this\|that\)" pattern, but then noticed that 'grep -E' is already used in a couple of places, and picked the format that uses less escape characters.