Re: [PATCH] t7812: expect failure for grep -i with invalid UTF-8 data

From: Todd Zullinger <tmz@pobox.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Andreas Schwab" <schwab@linux-m68k.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Beat Bolli" <dev+git@drbeat.li>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>
Subject: Re: [PATCH] t7812: expect failure for grep -i with invalid UTF-8 data
Date: Sun, 1 Dec 2019 13:32:03 -0500	[thread overview]
Message-ID: <20191201183203.GC17681@pobox.com> (raw)
In-Reply-To: <xmqqo8wsypit.fsf@gitster-ct.c.googlers.com>

Hi,

Junio C Hamano wrote:
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
>> On Nov 29 2019, Todd Zullinger wrote:
>>
>>> When the 'grep with invalid UTF-8 data' tests were added/adjusted in
>>> 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
>>> and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
>>> 2019-07-26) they lacked a redirect which caused them to falsely succeed
>>> on most architectures.  They failed on big-endian arches where the test
>>> never reached the portion which was missing the redirect.
>>
>> It's not about big vs little endian, it's only about JIT vs non-JIT.
> 
> So, which one of JIT / non-JIT sides did the test fail unexpectedly?

On s390x, the initial:

    test_might_fail git grep -hi "Æ" invalid-0x80 >actual

fails to produce any output in actual, but since we use
test_might_fail, the test happily continues to:

    test_cmp expected actual

which fails.

The test output from and s390x build:

    expecting success of 7812.11 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i':
        test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
        test_cmp expected actual &&
        test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
        test_cmp expected actual
    ++ test_might_fail git grep -hi Æ invalid-0x80
    ++ test_must_fail ok=success git grep -hi Æ invalid-0x80
    ++ case "$1" in
    ++ _test_ok=success
    ++ shift
    ++ git grep -hi Æ invalid-0x80
    fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set
    ++ exit_code=128
    ++ test 128 -eq 0
    ++ test_match_signal 13 128
    ++ test 128 = 141
    ++ test 128 = 269
    ++ return 1
    ++ test 128 -gt 129
    ++ test 128 -eq 127
    ++ test 128 -eq 126
    ++ return 0
    ++ test_cmp expected actual
    ++ diff -u expected actual
    --- expected    2019-10-19 21:56:08.634252012 +0000
    +++ actual      2019-10-19 21:56:08.714252012 +0000
    @@ -1 +0,0 @@
    -ævar
    error: last command exited with $?=1
    not ok 11 - PCRE v2: grep non-ASCII from invalid UTF-8 data with -i
    #
    #               test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
    #               test_cmp expected actual &&
    #               test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
    #               test_cmp expected actual
    #
    # failed 1 among 11 test(s)

After Andreas' missing redirect fix, that still fails on
s390x (not surprisingly).  But now systems with JIT enabled
fail at:

    test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 >actual &&
    test_cmp expected actual

Though we say that the command must fail, so we shouldn't be
surprised that 'expect' and 'actual' don't match.  It would
be more surprising if they did. :)

> Should I do s/on big-endian arches/with PCRE with JIT disabled/
> while queuing the patch?

Here's how I changed the commit message locally.  I was
going to wait a day or so for any other feedback on the
actual test changes, being a holiday weekend in the US (and
more generally a weekend).

1:  d9aeaf0c98 ! 1:  d0c083db78 t7812: expect failure for grep -i with invalid UTF-8 data
    @@ Commit message
         8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
         and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
         2019-07-26) they lacked a redirect which caused them to falsely succeed
    -    on most architectures.  They failed on big-endian arches where the test
    -    never reached the portion which was missing the redirect.
    +    on most systems.  The 'grep -i' test failed on systems where JIT was
    +    disabled as it never reached the portion which was missing the redirect.

    -    A recent patch add the missing redirect and exposed the fact that the
    -    'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails on
    -    all architectures.
    +    A recent patch added the missing redirect and exposed the fact that the
    +    'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails
    +    regardless of whether JIT is enabled.

         Based on the final paragraph in in 870eea8166:

Thanks for pointing out the proper reasoning to use in the
commit message Andreas.  I hadn't looked at the Fedora pcre2
package to see that it explicitly disables JIT on s390x.

I'm not sure if s390x is supported upstream or not -- it
doesn't appear to have a specific entry in the sljit config
header¹, so it seems likely it's not well-tested at the
least.  (Not that any of that is our concern here.)

¹ https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h

Thanks for the follow-up Junio.

-- 
Todd