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

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

[Ed Maste]

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)?

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.

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

[Ed Maste]

On Wed, 27 Nov 2019 at 12:20, Jeff King <peff@peff.net> wrote:
>
> 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.

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

[Ed Maste]

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(-)

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

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

[Jeff King]

On Wed, Nov 27, 2019 at 03:17:08PM +0000, Ed Maste wrote:

> 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

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

[Jeff King]

On Wed, Nov 27, 2019 at 08:07:30AM -0500, Ed Maste wrote:

> 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

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

[Ed Maste]

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(-)

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

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

[Jeff King]

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

-Peff

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

[Ed Maste]

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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