threads / patch / 52360

patcht4210: skip i18n tests that don't work on FreeBSD

Subject: [PATCH] t4210: skip i18n tests that don't work on FreeBSD

## tl;dr

9 messages between Nov 27, 2019 and Dec 2, 2019. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Ed Maste· Nov 27, 2019, 15:17 UTC · lore
From: Ed Maste <emaste@freebsd.org>

A number of t4210-log-i18n tests added in 4e2443b181 set LC_ALL to a UTF-8 locale (is_IS.UTF-8) but then pass an invalid UTF-8 string to --grep. FreeBSD's regcomp() fails in this case with REG_ILLSEQ sequence which git then passes to die():

fatal: command line: '�': illegal byte sequence
When these tests were added the commit message stated:
| It's possible that this
| test breaks the "basic" and "extended" backends on some systems that
| are more anal than glibc about the encoding of locale issues with
| POSIX functions that I can remember
which seems to be the case here.

Extend test-lib.sh to add a FREEBSD prereq (akin to MINGW) and add !FREEBSD to these tests.

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 t/t4210-log-i18n.sh | 4 ++--
 t/test-lib.sh       | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

t/t4210-log-i18n.sh, t/test-lib.sh

diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index 6e61f57f09..d897eea1aa 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -70,7 +70,7 @@ do
 	then
 	    force_regex=.*
 	fi
-	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
+	test_expect_success !MINGW,!FREEBSD,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
 		cat >expect <<-\EOF &&
 		latin1
 		utf8
@@ -84,7 +84,7 @@ do
 		test_must_be_empty actual
 	"
 
-	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
+	test_expect_success !MINGW,!FREEBSD,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
 		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$invalid_e\" >actual &&
 		test_must_be_empty actual
 	"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 46c4440843..8693ff9eec 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1422,7 +1422,7 @@ else
 	'
 fi
 
-# Fix some commands on Windows
+# Fix some commands on Windows, and other OS-specific things
 uname_s=$(uname -s)
 case $uname_s in
 *MINGW*)
@@ -1453,6 +1453,12 @@ case $uname_s in
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
 	;;
+FreeBSD)
+	test_set_prereq FREEBSD
+	test_set_prereq POSIXPERM
+	test_set_prereq BSLASHPSPEC
+	test_set_prereq EXECKEEPSPID
+	;;
 *)
 	test_set_prereq POSIXPERM
 	test_set_prereq BSLASHPSPEC

-- 
2.24.0
Jeff King· Nov 27, 2019, 16:24 UTC · re: Ed Maste · lore

Re: [PATCH] t4210: skip i18n tests that don't work on FreeBSD

On Wed, Nov 27, 2019 at 03:17:08PM +0000, Ed Maste wrote:
Show 17 quoted lines
> From: Ed Maste <emaste@freebsd.org>
> 
> A number of t4210-log-i18n tests added in 4e2443b181 set LC_ALL to a UTF-8
> locale (is_IS.UTF-8) but then pass an invalid UTF-8 string to --grep.
> FreeBSD's regcomp() fails in this case with REG_ILLSEQ sequence which git
> then passes to die():
> 
> fatal: command line: '�': illegal byte sequence
> 
> When these tests were added the commit message stated:
> 
> | It's possible that this
> | test breaks the "basic" and "extended" backends on some systems that
> | are more anal than glibc about the encoding of locale issues with
> | POSIX functions that I can remember
> 
> which seems to be the case here.
Makes sense, but...
> Extend test-lib.sh to add a FREEBSD prereq (akin to MINGW) and add !FREEBSD
> to these tests.

Before we start growing more system-specific prereqs here, can we add a layer of indirection? Convert this test to use a REGEX_ALLOW_ILLSEQ prereq (or maybe there's a better name), and then set it for both mingw and freebsd?

-Peff
Ed Maste· Nov 27, 2019, 13:07 UTC · re: Jeff King · lore

Re: [PATCH] t4210: skip i18n tests that don't work on FreeBSD

On Wed, 27 Nov 2019 at 11:24, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
>
> > Extend test-lib.sh to add a FREEBSD prereq (akin to MINGW) and add !FREEBSD
> > to these tests.
>
> Before we start growing more system-specific prereqs here, can we add a
> layer of indirection? Convert this test to use a REGEX_ALLOW_ILLSEQ
> prereq (or maybe there's a better name), and then set it for both mingw
> and freebsd?

Thanks, and that makes sense, but should we make the sense the opposite way - set REGEX_ALLOW_ILLSEQ only for glibc (including Cygwin, I guess)?

This also applies only to two cases ("latin1 + locale" and "latin1 + locale + invalid needle"). There are other !MINGW tests in t4210-log-i18n.sh which do not use invalid UTF-8 and work fine on FreeBSD.

Jeff King· Nov 27, 2019, 17:01 UTC · re: Ed Maste · lore

Re: [PATCH] t4210: skip i18n tests that don't work on FreeBSD

On Wed, Nov 27, 2019 at 08:07:30AM -0500, Ed Maste wrote:
Show 13 quoted lines
> On Wed, 27 Nov 2019 at 11:24, Jeff King <peff@peff.net> wrote:
> >
> > > Extend test-lib.sh to add a FREEBSD prereq (akin to MINGW) and add !FREEBSD
> > > to these tests.
> >
> > Before we start growing more system-specific prereqs here, can we add a
> > layer of indirection? Convert this test to use a REGEX_ALLOW_ILLSEQ
> > prereq (or maybe there's a better name), and then set it for both mingw
> > and freebsd?
> 
> Thanks, and that makes sense, but should we make the sense the
> opposite way - set REGEX_ALLOW_ILLSEQ only for glibc (including
> Cygwin, I guess)?

Possibly. There's a long tail of platforms we support, and we don't really know how they behave (are they OK, or is it that nobody has bothered to check yet?). I'd be inclined to blacklist platforms where we know it doesn't work, in order to let the test run on as many as possible.

I guess we'd need to invert the prereq then to systems that actually report ILLSEQ (maybe REGEX_ILLSEQ, and set it for FreeBSD?).

> This also applies only to two cases ("latin1 + locale" and "latin1 +
> locale + invalid needle"). There are other !MINGW tests in
> t4210-log-i18n.sh which do not use invalid UTF-8 and work fine on
> FreeBSD.

Hmm, yeah. It's not clear to me if !MINGW is there because of the ILLSEQ issue, or just because it doesn't handle these tests at all. So maybe it would make sense to just set it for FreeBSD for now, and mingw folks can clean it up further if they want to dig into it.

-Peff
Ed Maste· Nov 27, 2019, 17:15 UTC · re: Ed Maste · lore

[PATCH v2] t4210: skip i18n tests that don't work on FreeBSD

From: Ed Maste <emaste@freebsd.org>

A number of t4210-log-i18n tests added in 4e2443b181 set LC_ALL to a UTF-8 locale (is_IS.UTF-8) but then pass an invalid UTF-8 string to --grep. FreeBSD's regcomp() fails in this case with REG_ILLSEQ, "illegal byte sequence," which git then passes to die():

fatal: command line: '�': illegal byte sequence
When these tests were added the commit message stated:
| It's possible that this
| test breaks the "basic" and "extended" backends on some systems that
| are more anal than glibc about the encoding of locale issues with
| POSIX functions that I can remember
which seems to be the case here.

Extend test-lib.sh to add a REGEX_ILLSEQ prereq, set it on FreeBSD, and add !REGEX_ILLSEQ to the two affected tests.

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 t/t4210-log-i18n.sh | 4 ++--
 t/test-lib.sh       | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

t/t4210-log-i18n.sh, t/test-lib.sh

diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index 6e61f57f09..c3792081e6 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -70,7 +70,7 @@ do
 	then
 	    force_regex=.*
 	fi
-	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
+	test_expect_success !MINGW,!REGEX_ILLSEQ,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
 		cat >expect <<-\EOF &&
 		latin1
 		utf8
@@ -84,7 +84,7 @@ do
 		test_must_be_empty actual
 	"
 
-	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
+	test_expect_success !MINGW,!REGEX_ILLSEQ,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
 		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$invalid_e\" >actual &&
 		test_must_be_empty actual
 	"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 46c4440843..3b2b8795fd 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1422,7 +1422,7 @@ else
 	'
 fi
 
-# Fix some commands on Windows
+# Fix some commands on Windows, and other OS-specific things
 uname_s=$(uname -s)
 case $uname_s in
 *MINGW*)
@@ -1453,6 +1453,12 @@ case $uname_s in
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
 	;;
+FreeBSD)
+	test_set_prereq REGEX_ILLSEQ
+	test_set_prereq POSIXPERM
+	test_set_prereq BSLASHPSPEC
+	test_set_prereq EXECKEEPSPID
+	;;
 *)
 	test_set_prereq POSIXPERM
 	test_set_prereq BSLASHPSPEC

-- 
2.24.0
Jeff King· Nov 27, 2019, 17:20 UTC · re: Ed Maste · lore

Re: [PATCH v2] t4210: skip i18n tests that don't work on FreeBSD

On Wed, Nov 27, 2019 at 05:15:07PM +0000, Ed Maste wrote:
> Extend test-lib.sh to add a REGEX_ILLSEQ prereq, set it on FreeBSD, and
> add !REGEX_ILLSEQ to the two affected tests.
Thanks, this is much nicer.
Show 6 quoted lines
> +FreeBSD)
> +	test_set_prereq REGEX_ILLSEQ
> +	test_set_prereq POSIXPERM
> +	test_set_prereq BSLASHPSPEC
> +	test_set_prereq EXECKEEPSPID
> +	;;

I scratched my head at these for a minute, but I see you are just covering the bits set in the "*" case that we now no longer trigger.

It would probably be cleaner to set them ahead of time and just unset them selectively in MINGW, etc. But we don't have any way to unset a prereq, so lets' go with this for now. :)

-Peff
Ed Maste· Nov 27, 2019, 13:44 UTC · re: Jeff King · lore

Re: [PATCH v2] t4210: skip i18n tests that don't work on FreeBSD

On Wed, 27 Nov 2019 at 12:20, Jeff King <peff@peff.net> wrote:
Show 16 quoted lines
>
> On Wed, Nov 27, 2019 at 05:15:07PM +0000, Ed Maste wrote:
>
> > +FreeBSD)
> > +     test_set_prereq REGEX_ILLSEQ
> > +     test_set_prereq POSIXPERM
> > +     test_set_prereq BSLASHPSPEC
> > +     test_set_prereq EXECKEEPSPID
> > +     ;;
>
> I scratched my head at these for a minute, but I see you are just
> covering the bits set in the "*" case that we now no longer trigger.
>
> It would probably be cleaner to set them ahead of time and just unset
> them selectively in MINGW, etc. But we don't have any way to unset a
> prereq, so lets' go with this for now. :)

Indeed, I'm happy to help with reworking this later on - perhaps informed by finding other operating systems that need REGEX_ILLSEQ.

Ed Maste· Dec 2, 2019, 11:03 UTC · re: Jeff King · lore

Re: [PATCH v2] t4210: skip i18n tests that don't work on FreeBSD

On Wed, 27 Nov 2019 at 12:20, Jeff King <peff@peff.net> wrote:
>
> It would probably be cleaner to set them ahead of time and just unset
> them selectively in MINGW, etc. But we don't have any way to unset a
> prereq, so lets' go with this for now. :)

Also, what's the next step for this patch (is there anything I should do)? I'd like to next work on getting FreeBSD CI going (via Cirrus-CI) once this is sorted, so that I can start with no failing tests.

Junio C Hamano· Dec 2, 2019, 17:01 UTC · re: Jeff King · lore

Re: [PATCH v2] t4210: skip i18n tests that don't work on FreeBSD

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Wed, Nov 27, 2019 at 05:15:07PM +0000, Ed Maste wrote:
>
>> Extend test-lib.sh to add a REGEX_ILLSEQ prereq, set it on FreeBSD, and
>> add !REGEX_ILLSEQ to the two affected tests.
>
> Thanks, this is much nicer.
>
>> +FreeBSD)
>> +	test_set_prereq REGEX_ILLSEQ
>> +	test_set_prereq POSIXPERM
>> +	test_set_prereq BSLASHPSPEC
>> +	test_set_prereq EXECKEEPSPID
>> +	;;
>
> I scratched my head at these for a minute, but I see you are just
> covering the bits set in the "*" case that we now no longer trigger.
>
> It would probably be cleaner to set them ahead of time and just unset
> them selectively in MINGW, etc. But we don't have any way to unset a
> prereq, so lets' go with this for now. :)
Thanks, both.  Queued.  Will merge to 'next' shortly.

← back to recent threads