[PATCH] t4210: detect REG_ILLSEQ dynamically

[PATCH] t4210: detect REG_ILLSEQ dynamically

[Carlo Marcelo Arenas Belón]

7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
adds a REG_ILLSEQ prerequisite to avoid failures from the tests added with
4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
2019-06-28), but hardcodes it to be only enabled for FreeBSD.

Instead of hardcoding the affected platform, add a test using test-tool to
make sure it can be dynamically detected in other affected systems (like
DragonFlyBSD or macOS), and while at it, enhance the tool so it can report
better on the cause of failure and be silent when needed.

To minimize changes, it is assumed the compilation error is because of the
right type since we control the only caller, and is also assumed to affect
both basic and extended syntax (only extended is tested).  The description
of the first test which wasn't accurate has been also corrected, and the
suppressed tests restricted to the two affected engines only.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/helper/test-regex.c | 21 +++++++++++++++++----
 t/t4210-log-i18n.sh   | 24 ++++++++++++++++--------
 t/test-lib.sh         |  6 ------
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 10284cc56f..985fcb0f75 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
 {
 	const char *pat;
 	const char *str;
-	int flags = 0;
+	int ret, opt_s = 0, flags = 0;
 	regex_t r;
 	regmatch_t m[1];
+	char errbuf[64];
 
 	if (argc == 2 && !strcmp(argv[1], "--bug"))
 		return test_regex_bug();
 	else if (argc < 3)
 		usage("test-tool regex --bug\n"
-		      "test-tool regex <pattern> <string> [<options>]");
+		      "test-tool regex [--silent] <pattern> <string> [<options>]");
 
+	if (!strcmp(argv[1], "--silent")) {
+		opt_s++;
+		argv++;
+	}
 	argv++;
 	pat = *argv++;
 	str = *argv++;
@@ -67,8 +72,16 @@ int cmd__regex(int argc, const char **argv)
 	}
 	git_setup_gettext();
 
-	if (regcomp(&r, pat, flags))
-		die("failed regcomp() for pattern '%s'", pat);
+	ret = regcomp(&r, pat, flags);
+	if (ret) {
+		if (opt_s)
+			return 1;
+		else {
+			regerror(ret, &r, errbuf, sizeof(errbuf));
+			die("failed regcomp() for pattern '%s' (%s)",
+				pat, errbuf);
+		}
+	}
 	if (regexec(&r, str, 1, m, 0))
 		return 1;
 
diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index c3792081e6..24b9e7e72b 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -56,21 +56,29 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
 	test_must_be_empty actual
 '
 
+test_have_prereq GETTEXT_LOCALE &&
+! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED &&
+test_set_prereq REGEX_ILLSEQ
+
 for engine in fixed basic extended perl
 do
+	ireq=
 	prereq=
-	if test $engine = "perl"
-	then
-		prereq="PCRE"
-	else
-		prereq=""
-	fi
+	case $engine in
+	basic|extended)
+		ireq=!REGEX_ILLSEQ,
+		;;
+	perl)
+		prereq=PCRE
+		;;
+	esac
 	force_regex=
 	if test $engine != "fixed"
 	then
 	    force_regex=.*
 	fi
-	test_expect_success !MINGW,!REGEX_ILLSEQ,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
+
+	test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
 		cat >expect <<-\EOF &&
 		latin1
 		utf8
@@ -84,7 +92,7 @@ do
 		test_must_be_empty actual
 	"
 
-	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)" "
+	test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$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 0ea1e5a05e..81473fea1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1454,12 +1454,6 @@ 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.26.2.737.gf3227dd3d3

Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

[Eric Sunshine]

On Wed, May 13, 2020 at 7:17 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> 7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
> adds a REG_ILLSEQ prerequisite to avoid failures from the tests added with
> 4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
> 2019-06-28), but hardcodes it to be only enabled for FreeBSD.
>
> Instead of hardcoding the affected platform, add a test using test-tool to
> make sure it can be dynamically detected in other affected systems (like
> DragonFlyBSD or macOS), and while at it, enhance the tool so it can report
> better on the cause of failure and be silent when needed.
> [...]
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
>  {
> -       int flags = 0;
> +       int ret, opt_s = 0, flags = 0;
> [...]
> +       if (!strcmp(argv[1], "--silent")) {
> +               opt_s++;

Nit: When reading the declaration of 'opt_s', I found the name
inscrutable; it was only upon reading the actual code, that I
understood that it reflected whether --silent had been used. How about
giving it a more easily-understood name, such as 'silent'?

> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> @@ -56,21 +56,29 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
> +test_have_prereq GETTEXT_LOCALE &&
> +! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED &&
> +test_set_prereq REGEX_ILLSEQ

Nit: Is there precedent for formatting the code like this? At first
glance, I read these as three distinct statements rather than one
large composite statement. I wonder if indenting the continuation
lines would make this more easily-digested.

>  for engine in fixed basic extended perl
>  do
> +       ireq=
>         prereq=
> +       case $engine in
> +       basic|extended)
> +               ireq=!REGEX_ILLSEQ,
> +               ;;
> +       perl)
> +               prereq=PCRE
> +               ;;
> +       esac

Why do you introduce a new variable 'ireq' here considering that...

> +       test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
> +       test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "

...it is always used alongside 'prereq'? It seems that you could just
assign "!REGEX_ILLSEQ" to 'prereq' without need for 'ireq'. (And
'ireq' is a rather inscrutable name -- I have trouble figuring out
what it means.)

>                 LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> @@ -1454,12 +1454,6 @@ case $uname_s in
> -FreeBSD)
> -       test_set_prereq REGEX_ILLSEQ
> -       test_set_prereq POSIXPERM
> -       test_set_prereq BSLASHPSPEC
> -       test_set_prereq EXECKEEPSPID
> -       ;;

The commit message explains why you remove the 'REGEX_ILLSEQ', but why
are all the other lines removed, as well? Such removal seems unrelated
to the stated purpose of this patch.

Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
>> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
>>  {
>> -       int flags = 0;
>> +       int ret, opt_s = 0, flags = 0;
>> [...]
>> +       if (!strcmp(argv[1], "--silent")) {
>> +               opt_s++;
>
> Nit: When reading the declaration of 'opt_s', I found the name
> inscrutable; it was only upon reading the actual code, that I
> understood that it reflected whether --silent had been used. How about
> giving it a more easily-understood name, such as 'silent'?

Yup, that is a reasonable fix to the readability problem.

>> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
>> @@ -56,21 +56,29 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
>> +test_have_prereq GETTEXT_LOCALE &&
>> +! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED &&
>> +test_set_prereq REGEX_ILLSEQ
>
> Nit: Is there precedent for formatting the code like this? At first
> glance, I read these as three distinct statements rather than one
> large composite statement. I wonder if indenting the continuation
> lines would make this more easily-digested.

It's not like we are running three tests and expecting all of them
to succeed (or report a failure otherwise).  The first two are the
conditions we want to trigger the action the third one represents.

I'd prefer to see it written with "if then fi".  That would also be
a worthwhile readability fix.

>> -       test_set_prereq REGEX_ILLSEQ
>> -       test_set_prereq POSIXPERM
>> -       test_set_prereq BSLASHPSPEC
>> -       test_set_prereq EXECKEEPSPID
>> -       ;;
>
> The commit message explains why you remove the 'REGEX_ILLSEQ', but why
> are all the other lines removed, as well? Such removal seems unrelated
> to the stated purpose of this patch.

Yup, I was wondering about the same thing.

Thanks.

Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

[Carlo Marcelo Arenas Belón]

On Wed, May 13, 2020 at 11:44:53AM -0400, Eric Sunshine wrote:
> On Wed, May 13, 2020 at 7:17 AM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
> > diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> > @@ -56,21 +56,29 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
> > +test_have_prereq GETTEXT_LOCALE &&
> > +! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED &&
> > +test_set_prereq REGEX_ILLSEQ
> 
> Nit: Is there precedent for formatting the code like this? At first
> glance, I read these as three distinct statements rather than one
> large composite statement. I wonder if indenting the continuation
> lines would make this more easily-digested.

yes, I copied the syntax from t7812, and I agree looks ugly and would had
rather done it with an if as Junio suggested, but couldn't find precedent
in another tests.

indeed, I would rather go away with the whole prereq and set a variable
with a nice sounding name and use it below to `if test` the right tests,
would that be ok?

> >  for engine in fixed basic extended perl
> >  do
> > +       ireq=
> >         prereq=
> > +       case $engine in
> > +       basic|extended)
> > +               ireq=!REGEX_ILLSEQ,
> > +               ;;
> > +       perl)
> > +               prereq=PCRE
> > +               ;;
> > +       esac
> 
> Why do you introduce a new variable 'ireq' here considering that...
> 
> > +       test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
> > +       test_expect_success !MINGW,GETTEXT_LOCALE,$ireq$prereq "-c grep.patternType=$engine log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
> 
> ...it is always used alongside 'prereq'? It seems that you could just
> assign "!REGEX_ILLSEQ" to 'prereq' without need for 'ireq'. (And
> 'ireq' is a rather inscrutable name -- I have trouble figuring out
> what it means.)

sadly I can't because there are 3 tests, and only 2 of them (the ones shown
in the patch) will have that prerequisite dynamically added, while all
will have $prereq.

will send a v2 as an RFC with your suggestions

> >                 LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > @@ -1454,12 +1454,6 @@ case $uname_s in
> > -FreeBSD)
> > -       test_set_prereq REGEX_ILLSEQ
> > -       test_set_prereq POSIXPERM
> > -       test_set_prereq BSLASHPSPEC
> > -       test_set_prereq EXECKEEPSPID
> > -       ;;
> 
> The commit message explains why you remove the 'REGEX_ILLSEQ', but why
> are all the other lines removed, as well? Such removal seems unrelated
> to the stated purpose of this patch.

they were all added by the previous fix I am ammending and therefore are
no longer needed.

the 3 unrelated variables were originally copied from the '*' entry on
that case, and therefore FreeBSD will now use the ones there instead of
its special case.

Carlo

Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> indeed, I would rather go away with the whole prereq and set a variable
> with a nice sounding name and use it below to `if test` the right tests,
> would that be ok?

In a sense, a prerequisite is an overkill if the tests that need to
be guarded are very well isolated and in a single place.  Do we
expect other tests outside the context of this script may have to be
guarded by REG_ILLSEQ?

Re: [PATCH] t4210: detect REG_ILLSEQ dynamically

[Carlo Marcelo Arenas Belón]

On Wed, May 13, 2020 at 01:37:32PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > indeed, I would rather go away with the whole prereq and set a variable
> > with a nice sounding name and use it below to `if test` the right tests,
> > would that be ok?
> 
> In a sense, a prerequisite is an overkill if the tests that need to
> be guarded are very well isolated and in a single place.  Do we
> expect other tests outside the context of this script may have to be
> guarded by REG_ILLSEQ?

IMHO in the long term we probably should get rid of those tests (including
the need to look for REG_ILLSEQ), because the root cause of this failure is
that the test is really relying in undefined behaviour which happen to be
common to both glibc and our own compat regex, but that was relevant at that
time, because of a bug on the way UTF-8 was being detected.

POSIX regcomp[1], specifically mentions that returned and error is a valid
response to a badly encoded pattern, but the test cases were created to
specifically ensure no error is ever generated, and to make sure the response
(which is undefined, as per POSIX) was consistent for all engines:

"The regcomp() and regexec() functions are required to accept any null-terminated string as the pattern argument. If the meaning of the string is "undefined", the behavior of the function is "unspecified". IEEE Std 1003.1-2001 does not specify how the functions will interpret the pattern; they might return error codes, or they might do pattern matching in some completely unexpected way, but they should not do something like abort the process."

Carlo

[1] https://pubs.opengroup.org/onlinepubs/009695399/functions/regcomp.html

[RFC PATCH v2] t4210: detect REG_ILLSEQ dynamically

[Carlo Marcelo Arenas Belón]

7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
adds a REG_ILLSEQ prerequisite to avoid failures from the tests added with
4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
2019-06-28), but hardcodes it to be only enabled for FreeBSD.

Instead of hardcoding the affected platform, add a test using test-tool to
make sure it can be dynamically detected in other affected systems (like
DragonFlyBSD or macOS), and while at it, enhance the tool so it can report
better on the cause of failure and be silent when needed.

To minimize changes, it is assumed the regcomp error is of the right type
since we control the only caller, and is also assumed to affect both basic
and extended syntax (only extended is tested, but both behave the same in
all three affected platforms).

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.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
- nicer variable names and hopefully easier to understand logic

 t/helper/test-regex.c | 20 +++++++++++++++----
 t/t4210-log-i18n.sh   | 45 ++++++++++++++++++++++++++++---------------
 t/test-lib.sh         |  6 ------
 3 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 10284cc56f..a4bd1f3a36 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
 {
 	const char *pat;
 	const char *str;
-	int flags = 0;
+	int ret, silent = 0, flags = 0;
 	regex_t r;
 	regmatch_t m[1];
+	char errbuf[64];
 
 	if (argc == 2 && !strcmp(argv[1], "--bug"))
 		return test_regex_bug();
 	else if (argc < 3)
 		usage("test-tool regex --bug\n"
-		      "test-tool regex <pattern> <string> [<options>]");
+		      "test-tool regex [--silent] <pattern> <string> [<options>]");
 
+	if (!strcmp(argv[1], "--silent")) {
+		silent = 1;
+		argv++;
+	}
 	argv++;
 	pat = *argv++;
 	str = *argv++;
@@ -67,8 +72,15 @@ int cmd__regex(int argc, const char **argv)
 	}
 	git_setup_gettext();
 
-	if (regcomp(&r, pat, flags))
-		die("failed regcomp() for pattern '%s'", pat);
+	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;
+	}
 	if (regexec(&r, str, 1, m, 0))
 		return 1;
 
diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index c3792081e6..8fccb83020 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -10,6 +10,12 @@ latin1_e=$(printf '\351')
 # invalid UTF-8
 invalid_e=$(printf '\303\50)') # ")" at end to close opening "("
 
+if test_have_prereq GETTEXT_LOCALE &&
+	! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED
+then
+	have_reg_illseq=1
+fi
+
 test_expect_success 'create commits in different encodings' '
 	test_tick &&
 	cat >msg <<-EOF &&
@@ -61,33 +67,40 @@ do
 	prereq=
 	if test $engine = "perl"
 	then
-		prereq="PCRE"
-	else
-		prereq=""
+		prereq=PCRE
 	fi
 	force_regex=
 	if test $engine != "fixed"
 	then
-	    force_regex=.*
+		force_regex=.*
+	fi
+
+	if test -z "$have_reg_illseq" ||
+		test $engine = "fixed" || test $engine = "perl"
+	then
+		test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
+			cat >expect <<-\EOF &&
+			latin1
+			utf8
+			EOF
+			LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
+			test_cmp expect actual
+		"
 	fi
-	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
-		EOF
-		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
-		test_cmp expect actual
-	"
 
 	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
 		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&
 		test_must_be_empty actual
 	"
 
-	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
-	"
+	if test -z "$have_reg_illseq" ||
+		test $engine = "fixed" || test $engine = "perl"
+	then
+		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)" "
+			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
+		"
+	fi
 done
 
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ea1e5a05e..81473fea1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1454,12 +1454,6 @@ 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.26.2.812.g046d49d455

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

[Eric Sunshine]

On Wed, May 13, 2020 at 2:02 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> [...]
> 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.

> 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(...);
    }

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

[Junio C Hamano]

Is https://github.com/git/git/runs/678964255 related to this change?

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
> adds a REG_ILLSEQ prerequisite to avoid failures from the tests added with
> 4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
> 2019-06-28), but hardcodes it to be only enabled for FreeBSD.
>
> Instead of hardcoding the affected platform, add a test using test-tool to
> make sure it can be dynamically detected in other affected systems (like
> DragonFlyBSD or macOS), and while at it, enhance the tool so it can report
> better on the cause of failure and be silent when needed.
>
> To minimize changes, it is assumed the regcomp error is of the right type
> since we control the only caller, and is also assumed to affect both basic
> and extended syntax (only extended is tested, but both behave the same in
> all three affected platforms).
>
> 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.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> v2:
> - nicer variable names and hopefully easier to understand logic



>
>  t/helper/test-regex.c | 20 +++++++++++++++----
>  t/t4210-log-i18n.sh   | 45 ++++++++++++++++++++++++++++---------------
>  t/test-lib.sh         |  6 ------
>  3 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
> index 10284cc56f..a4bd1f3a36 100644
> --- a/t/helper/test-regex.c
> +++ b/t/helper/test-regex.c
> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
>  {
>  	const char *pat;
>  	const char *str;
> -	int flags = 0;
> +	int ret, silent = 0, flags = 0;
>  	regex_t r;
>  	regmatch_t m[1];
> +	char errbuf[64];
>  
>  	if (argc == 2 && !strcmp(argv[1], "--bug"))
>  		return test_regex_bug();
>  	else if (argc < 3)
>  		usage("test-tool regex --bug\n"
> -		      "test-tool regex <pattern> <string> [<options>]");
> +		      "test-tool regex [--silent] <pattern> <string> [<options>]");
>  
> +	if (!strcmp(argv[1], "--silent")) {
> +		silent = 1;
> +		argv++;
> +	}
>  	argv++;
>  	pat = *argv++;
>  	str = *argv++;
> @@ -67,8 +72,15 @@ int cmd__regex(int argc, const char **argv)
>  	}
>  	git_setup_gettext();
>  
> -	if (regcomp(&r, pat, flags))
> -		die("failed regcomp() for pattern '%s'", pat);
> +	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;
> +	}
>  	if (regexec(&r, str, 1, m, 0))
>  		return 1;
>  
> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> index c3792081e6..8fccb83020 100755
> --- a/t/t4210-log-i18n.sh
> +++ b/t/t4210-log-i18n.sh
> @@ -10,6 +10,12 @@ latin1_e=$(printf '\351')
>  # invalid UTF-8
>  invalid_e=$(printf '\303\50)') # ")" at end to close opening "("
>  
> +if test_have_prereq GETTEXT_LOCALE &&
> +	! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED
> +then
> +	have_reg_illseq=1
> +fi
> +
>  test_expect_success 'create commits in different encodings' '
>  	test_tick &&
>  	cat >msg <<-EOF &&
> @@ -61,33 +67,40 @@ do
>  	prereq=
>  	if test $engine = "perl"
>  	then
> -		prereq="PCRE"
> -	else
> -		prereq=""
> +		prereq=PCRE
>  	fi
>  	force_regex=
>  	if test $engine != "fixed"
>  	then
> -	    force_regex=.*
> +		force_regex=.*
> +	fi
> +
> +	if test -z "$have_reg_illseq" ||
> +		test $engine = "fixed" || test $engine = "perl"
> +	then
> +		test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
> +			cat >expect <<-\EOF &&
> +			latin1
> +			utf8
> +			EOF
> +			LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
> +			test_cmp expect actual
> +		"
>  	fi
> -	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
> -		EOF
> -		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
> -		test_cmp expect actual
> -	"
>  
>  	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
>  		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&
>  		test_must_be_empty actual
>  	"
>  
> -	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
> -	"
> +	if test -z "$have_reg_illseq" ||
> +		test $engine = "fixed" || test $engine = "perl"
> +	then
> +		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)" "
> +			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
> +		"
> +	fi
>  done
>  
>  test_done
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0ea1e5a05e..81473fea1d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1454,12 +1454,6 @@ 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

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

[Carlo Marcelo Arenas Belón]

On Fri, May 15, 2020 at 11:00:50AM -0700, Junio C Hamano wrote:
> Is https://github.com/git/git/runs/678964255 related to this change?

yes; when built without PCRE then fixed can also trigger REG_SIGILL

Carlo

[PATCH v2] t4210: detect REG_ILLSEQ dynamically

[Carlo Marcelo Arenas Belón]

7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
adds a REG_ILLSEQ prerequisite to avoid failures from the tests added with
4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
2019-06-28), but hardcodes it to be only enabled for FreeBSD.

Instead of hardcoding the affected platform, add a test using test-tool to
make sure it can be dynamically detected in other affected systems (like
DragonFlyBSD or macOS), and while at it, enhance the tool so it can report
better on the cause of failure and be silent when needed.

To minimize changes, it is assumed the regcomp error is of the right type
since we control the only caller, and is also assumed to affect both basic
and extended syntax (only extended is tested, but both behave the same in
all three affected platforms).

The description of the first test which wasn't accurate has been corrected,
and unlike the original fix from 7187c7bbb8, that created a special case
for FreeBSD by adding REG_ILLSEQ to the common set in test-lib, this does
not need changes to test-lib and therefore had those reverted.

Only the affected engines will have their tests suppressed, which include
"fixed" if the PCRE optimization that uses LIBPCRE2 since b65abcafc7
(grep: use PCRE v2 for optimized fixed-string search, 2019-07-01) is not
available.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
v2:
- address all suggestions from Eric
- reorder tests and add a helper functions (probably overly verbose) to
  clarify when the tests will be skipped (no prereq is used)

 t/helper/test-regex.c | 19 +++++++++---
 t/t4210-log-i18n.sh   | 70 ++++++++++++++++++++++++++++++++-----------
 t/test-lib.sh         |  6 ----
 3 files changed, 68 insertions(+), 27 deletions(-)

diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 10284cc56f..7a8ddce45b 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
 {
 	const char *pat;
 	const char *str;
-	int flags = 0;
+	int ret, silent = 0, flags = 0;
 	regex_t r;
 	regmatch_t m[1];
+	char errbuf[64];
 
 	if (argc == 2 && !strcmp(argv[1], "--bug"))
 		return test_regex_bug();
 	else if (argc < 3)
 		usage("test-tool regex --bug\n"
-		      "test-tool regex <pattern> <string> [<options>]");
+		      "test-tool regex [--silent] <pattern> <string> [<options>]");
 
+	if (!strcmp(argv[1], "--silent")) {
+		silent = 1;
+		argv++;
+	}
 	argv++;
 	pat = *argv++;
 	str = *argv++;
@@ -67,8 +72,14 @@ int cmd__regex(int argc, const char **argv)
 	}
 	git_setup_gettext();
 
-	if (regcomp(&r, pat, flags))
-		die("failed regcomp() for pattern '%s'", pat);
+	ret = regcomp(&r, pat, flags);
+	if (ret) {
+		if (silent)
+			return 1;
+
+		regerror(ret, &r, errbuf, sizeof(errbuf));
+		die("failed regcomp() for pattern '%s' (%s)", pat, errbuf);
+	}
 	if (regexec(&r, str, 1, m, 0))
 		return 1;
 
diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index c3792081e6..a89f456817 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -10,6 +10,12 @@ latin1_e=$(printf '\351')
 # invalid UTF-8
 invalid_e=$(printf '\303\50)') # ")" at end to close opening "("
 
+if test_have_prereq GETTEXT_LOCALE &&
+	! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED
+then
+	have_reg_illseq=1
+fi
+
 test_expect_success 'create commits in different encodings' '
 	test_tick &&
 	cat >msg <<-EOF &&
@@ -56,38 +62,68 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin
 	test_must_be_empty actual
 '
 
+trigger_undefined_behaviour()
+{
+	local engine=$1
+
+	case $engine in
+	fixed)
+		if test -n "$have_reg_illseq" &&
+			! test_have_prereq LIBPCRE2
+		then
+			return 0
+		else
+			return 1
+		fi
+		;;
+	basic|extended)
+		if test -n "$have_reg_illseq"
+		then
+			return 0
+		else
+			return 1
+		fi
+		;;
+	perl)
+		return 1
+		;;
+	esac
+}
+
 for engine in fixed basic extended perl
 do
 	prereq=
 	if test $engine = "perl"
 	then
-		prereq="PCRE"
-	else
-		prereq=""
+		prereq=PCRE
 	fi
 	force_regex=
 	if test $engine != "fixed"
 	then
-	    force_regex=.*
+		force_regex='.*'
 	fi
-	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
-		EOF
-		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
-		test_cmp expect actual
-	"
 
 	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
-		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&
+		LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&
 		test_must_be_empty actual
 	"
 
-	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
-	"
+	if ! trigger_undefined_behaviour $engine
+	then
+		test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
+			cat >expect <<-\EOF &&
+			latin1
+			utf8
+			EOF
+			LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
+			test_cmp expect 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)" "
+			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
+		"
+	fi
 done
 
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ea1e5a05e..81473fea1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1454,12 +1454,6 @@ 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.26.2.812.g046d49d455

Re: [PATCH v2] t4210: detect REG_ILLSEQ dynamically

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
> index 10284cc56f..7a8ddce45b 100644
> --- a/t/helper/test-regex.c
> +++ b/t/helper/test-regex.c
> @@ -41,16 +41,21 @@ int cmd__regex(int argc, const char **argv)
>  {
>  	const char *pat;
>  	const char *str;
> -	int flags = 0;
> +	int ret, silent = 0, flags = 0;
>  	regex_t r;
>  	regmatch_t m[1];
> +	char errbuf[64];
>  
>  	if (argc == 2 && !strcmp(argv[1], "--bug"))
>  		return test_regex_bug();
>  	else if (argc < 3)
>  		usage("test-tool regex --bug\n"
> -		      "test-tool regex <pattern> <string> [<options>]");
> +		      "test-tool regex [--silent] <pattern> <string> [<options>]");
>
> +	if (!strcmp(argv[1], "--silent")) {
> +		silent = 1;
> +		argv++;
> +	}

This looks fishy---if argc==3 and the first one is "--silent", only
the <pattern> is left in argv and before taking <string> out of the
argv, we need to ensure argc is still large enough, but I do not
think that is done below:

>  	argv++;
>  	pat = *argv++;
>  	str = *argv++;

So str here would be NULL and/or *argv++ would have given you an
out-of-bounds access already.

> @@ -67,8 +72,14 @@ int cmd__regex(int argc, const char **argv)
>  	}
>  	git_setup_gettext();
>  
> -	if (regcomp(&r, pat, flags))
> -		die("failed regcomp() for pattern '%s'", pat);
> +	ret = regcomp(&r, pat, flags);
> +	if (ret) {
> +		if (silent)
> +			return 1;
> +
> +		regerror(ret, &r, errbuf, sizeof(errbuf));
> +		die("failed regcomp() for pattern '%s' (%s)", pat, errbuf);
> +	}
>  	if (regexec(&r, str, 1, m, 0))
>  		return 1;

Not that it matters _too_ much as this is merely a test helper and
it would not hurt anybody as long as our callers are careful.

> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> index c3792081e6..a89f456817 100755
> --- a/t/t4210-log-i18n.sh
> +++ b/t/t4210-log-i18n.sh
> @@ -10,6 +10,12 @@ latin1_e=$(printf '\351')
>  # invalid UTF-8
>  invalid_e=$(printf '\303\50)') # ")" at end to close opening "("
>  
> +if test_have_prereq GETTEXT_LOCALE &&
> +	! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e $latin1_e EXTENDED
> +then
> +	have_reg_illseq=1
> +fi

OK.  Have we cleared have_reg_illseq shell variable before we reach
this point?  If not, we should (think: environment variable end user
had before starting the test).

> @@ -56,38 +62,68 @@ test_expect_success !MINGW 'log --grep does
>  	test_must_be_empty actual
>  '
>  
> +trigger_undefined_behaviour()
> +{

Style:

	triggers_undefined_behaviour () {

My first two readings of this patch mistakenly told me that the name
of the function was an instruction to the test to trigger an
undefined behaviour to see what happens, but this helper answers a
question "does the given engine trigger an undefined behaviour (with
the test data we are going to throw at it)?", right?  Perhaps rename
the helper to "triggerS_undefined_behaviour" would reduce the risk
of inviting such a misinterpretation.

> +	local engine=$1
> +
> +	case $engine in
> +	fixed)
> +		if test -n "$have_reg_illseq" &&
> +			! test_have_prereq LIBPCRE2
> +		then
> +			return 0
> +		else
> +			return 1
> +		fi
> +		;;
> +	basic|extended)
> +		if test -n "$have_reg_illseq"
> +		then
> +			return 0
> +		else
> +			return 1
> +		fi
> +		;;
> +	perl)
> +		return 1
> +		;;
> +	esac
> +}

... and the return value is true for "yes it would trigger undefined
behaviour" and false for "no it would not".

>  for engine in fixed basic extended perl
>  do
>  	prereq=
>  	if test $engine = "perl"
>  	then
> +		prereq=PCRE
>  	fi
>  	force_regex=
>  	if test $engine != "fixed"
>  	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)" "
> +		LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&

Can we do something to these overlong lines, by the way?

>  		test_must_be_empty actual
>  	"
>  
> +	if ! trigger_undefined_behaviour $engine
> +	then

Much easier to read than the ILLSEQ prerequisite, I would think,
even though the overlong lines are annoying.

> +		test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep searches in log output encoding (latin1 + locale)" "
> +			cat >expect <<-\EOF &&
> +			latin1
> +			utf8
> +			EOF
> +			LC_ALL=$is_IS_locale git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
> +			test_cmp expect 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)" "
> +			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
> +		"
> +	fi
>  done
>  
>  test_done
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0ea1e5a05e..81473fea1d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1454,12 +1454,6 @@ 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

Nice to be able to drop one case arm from here.  Thanks.

Re: [PATCH v2] t4210: detect REG_ILLSEQ dynamically

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>  	else if (argc < 3)
>>  		usage("test-tool regex --bug\n"
>> -		      "test-tool regex <pattern> <string> [<options>]");
>> +		      "test-tool regex [--silent] <pattern> <string> [<options>]");
>>
>> +	if (!strcmp(argv[1], "--silent")) {
>> +		silent = 1;
>> +		argv++;
>> +	}
>
> This looks fishy---if argc==3 and the first one is "--silent", only
> the <pattern> is left in argv and before taking <string> out of the
> argv, we need to ensure argc is still large enough, but I do not
> think that is done below:
> ...
> Not that it matters _too_ much as this is merely a test helper and
> it would not hurt anybody as long as our callers are careful.

But it still bothers me.  Perhaps like this?  

If I were writing this from scratch, I would probably increment
argv++ once as early as possible and consistently access argv[0]
or *argv++, but that would make the patch even noisier.

 t/helper/test-regex.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 7a8ddce45b..e68b4f6a73 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -46,16 +46,23 @@ int cmd__regex(int argc, const char **argv)
 	regmatch_t m[1];
 	char errbuf[64];
 
-	if (argc == 2 && !strcmp(argv[1], "--bug"))
-		return test_regex_bug();
-	else if (argc < 3)
-		usage("test-tool regex --bug\n"
-		      "test-tool regex [--silent] <pattern> <string> [<options>]");
+	if (argc < 2)
+		goto usage;
 
+	if (!strcmp(argv[1], "--bug")) {
+		if (argc == 2)
+			return test_regex_bug();
+		else
+			goto usage;
+	}
 	if (!strcmp(argv[1], "--silent")) {
-		silent = 1;
+		silent = 0;
 		argv++;
+		argc--;
 	}
+	if (argc < 3)
+		goto usage;
+
 	argv++;
 	pat = *argv++;
 	str = *argv++;
@@ -84,4 +91,8 @@ int cmd__regex(int argc, const char **argv)
 		return 1;
 
 	return 0;
+
+usage:
+	usage("test-tool regex --bug\n"
+	      "test-tool regex [--silent] <pattern> <string> [<options>]");
 }

[PATCH v3 0/2] auto detect REG_ILLSEQ

[Carlo Marcelo Arenas Belón]

This is a reroll for cb/t4210-illseq-auto-detect that includes all
feedback and split on two steps for easy of reviewing.

While the refactoring in test-regex.c is a little more intrusive that
needed, makes the logic easier to follow and the different use cases
more straightforward.

Note that there is still a leak in the use of regex_t, and had kept
all original assumptions for simplicity (no explicit check on REG_ILLSEQ,
or unlikely differences between BASIC/EXTENDED).

Carlo Marcelo Arenas Belón (2):
  t/helper: teach test-regex to report pattern errors (like REG_ILLSEQ)
  t4210: detect REG_ILLSEQ dynamically and skip affected tests

 t/helper/test-regex.c | 94 ++++++++++++++++++++++++++++++-------------
 t/t4210-log-i18n.sh   | 77 ++++++++++++++++++++++++++---------
 t/test-lib.sh         |  6 ---
 3 files changed, 125 insertions(+), 52 deletions(-)

-- 
2.27.0.rc0.183.gde8f92d652

[PATCH v3 1/2] t/helper: teach test-regex to report pattern errors (like REG_ILLSEQ)

[Carlo Marcelo Arenas Belón]

7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
adds a REG_ILLSEQ prerequisite to avoid failures from the tests added in
4e2443b181 (log tests: test regex backends in "--encode=<enc>" tests,
2019-06-28), but hardcodes it to be only enabled in FreeBSD.

Instead of hardcoding the affected platform, teach the test-regex helper,
how to validate a pattern and report back, so it can be used to detect the
same issue in other affected systems (like DragonFlyBSD or macOS).

While at it, refactor the tool so it can report back the source of the
errors it founds, and can be invoked also in a --silent mode, when needed,
for backward compatibility.  A missing flag has been added and the code
reformatted, as well as updates to the way the parameters are handled, for
consistency.

To minimize changes, it is assumed the regcomp error is of the right type
since we control the only caller, and is also assumed to affect both basic
and extended syntax (only basic is tested, but both behave the same in all
three affected platforms since they use the same function).

Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/helper/test-regex.c | 94 ++++++++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 28 deletions(-)

diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 10284cc56f..d6f28ca8d1 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -1,5 +1,4 @@
 #include "test-tool.h"
-#include "git-compat-util.h"
 #include "gettext.h"
 
 struct reg_flag {
@@ -8,12 +7,13 @@ struct reg_flag {
 };
 
 static struct reg_flag reg_flags[] = {
-	{ "EXTENDED",	 REG_EXTENDED	},
-	{ "NEWLINE",	 REG_NEWLINE	},
-	{ "ICASE",	 REG_ICASE	},
-	{ "NOTBOL",	 REG_NOTBOL	},
+	{ "EXTENDED",	REG_EXTENDED	},
+	{ "NEWLINE",	REG_NEWLINE	},
+	{ "ICASE",	REG_ICASE	},
+	{ "NOTBOL",	REG_NOTBOL	},
+	{ "NOTEOL",	REG_NOTEOL	},
 #ifdef REG_STARTEND
-	{ "STARTEND",	 REG_STARTEND	},
+	{ "STARTEND",	REG_STARTEND	},
 #endif
 	{ NULL, 0 }
 };
@@ -41,36 +41,74 @@ int cmd__regex(int argc, const char **argv)
 {
 	const char *pat;
 	const char *str;
-	int flags = 0;
+	int ret, silent = 0, flags = 0;
 	regex_t r;
 	regmatch_t m[1];
-
-	if (argc == 2 && !strcmp(argv[1], "--bug"))
-		return test_regex_bug();
-	else if (argc < 3)
-		usage("test-tool regex --bug\n"
-		      "test-tool regex <pattern> <string> [<options>]");
+	char errbuf[64];
 
 	argv++;
-	pat = *argv++;
-	str = *argv++;
-	while (*argv) {
-		struct reg_flag *rf;
-		for (rf = reg_flags; rf->name; rf++)
-			if (!strcmp(*argv, rf->name)) {
-				flags |= rf->flag;
-				break;
-			}
-		if (!rf->name)
-			die("do not recognize %s", *argv);
+	argc--;
+
+	if (!argc)
+		goto usage;
+
+	if (!strcmp(*argv, "--bug")) {
+		if (argc == 1)
+			return test_regex_bug();
+		else
+			goto usage;
+	}
+	if (!strcmp(*argv, "--silent")) {
+		silent = 1;
 		argv++;
+		argc--;
+	}
+	if (!argc)
+		goto usage;
+
+	pat = *argv++;
+	if (argc == 1)
+		str = NULL;
+	else {
+		str = *argv++;
+		while (*argv) {
+			struct reg_flag *rf;
+			for (rf = reg_flags; rf->name; rf++)
+				if (!strcmp(*argv, rf->name)) {
+					flags |= rf->flag;
+					break;
+				}
+			if (!rf->name)
+				die("do not recognize flag %s", *argv);
+			argv++;
+		}
 	}
 	git_setup_gettext();
 
-	if (regcomp(&r, pat, flags))
-		die("failed regcomp() for pattern '%s'", pat);
-	if (regexec(&r, str, 1, m, 0))
-		return 1;
+	ret = regcomp(&r, pat, flags);
+	if (ret) {
+		if (silent)
+			return ret;
+
+		regerror(ret, &r, errbuf, sizeof(errbuf));
+		die("failed regcomp() for pattern '%s' (%s)", pat, errbuf);
+	}
+	if (!str)
+		return 0;
+
+	ret = regexec(&r, str, 1, m, 0);
+	if (ret) {
+		if (silent || ret == REG_NOMATCH)
+			return ret;
+
+		regerror(ret, &r, errbuf, sizeof(errbuf));
+		die("failed regexec() for subject '%s' (%s)", str, errbuf);
+	}
 
 	return 0;
+usage:
+	usage("\ttest-tool regex --bug\n"
+	      "\ttest-tool regex [--silent] <pattern>\n"
+	      "\ttest-tool regex [--silent] <pattern> <string> [<options>]");
+	return -1;
 }
-- 
2.27.0.rc0.183.gde8f92d652

Re: [PATCH v3 1/2] t/helper: teach test-regex to report pattern errors (like REG_ILLSEQ)

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Based-on-patch-by: Junio C Hamano <gitster@pobox.com>

This is sufficiently different from what I suggested that I do not
deserve the above line, I would think.

> +	ret = regcomp(&r, pat, flags);
> +	if (ret) {
> +		if (silent)
> +			return ret;
> +
> +		regerror(ret, &r, errbuf, sizeof(errbuf));
> +		die("failed regcomp() for pattern '%s' (%s)", pat, errbuf);

Nice.

> +	}
> +	if (!str)
> +		return 0;
> +
> +	ret = regexec(&r, str, 1, m, 0);
> +	if (ret) {
> +		if (silent || ret == REG_NOMATCH)
> +			return ret;
> +
> +		regerror(ret, &r, errbuf, sizeof(errbuf));
> +		die("failed regexec() for subject '%s' (%s)", str, errbuf);
> +	}
>  
>  	return 0;
> +usage:
> +	usage("\ttest-tool regex --bug\n"
> +	      "\ttest-tool regex [--silent] <pattern>\n"
> +	      "\ttest-tool regex [--silent] <pattern> <string> [<options>]");
> +	return -1;
>  }

[PATCH v3 2/2] t4210: detect REG_ILLSEQ dynamically and skip affected tests

[Carlo Marcelo Arenas Belón]

7187c7bbb8 (t4210: skip i18n tests that don't work on FreeBSD, 2019-11-27)
adds a REG_ILLSEQ prerequisite, and to do that copies the common branch in
test-lib and expands it to include it in a special case for FreeBSD.

Instead; test for it using a previously added extension to test-tool and
use that, together with a function that identifies when regcomp/regexec
will be called with broken patterns to avoid any test that would otherwise
rely on undefined behaviour.

The description of the first test which wasn't accurate has been corrected,
and the test rearranged for clarity, including a helper function that avoids
overly long lines.

Only the affected engines will have their tests suppressed, also including
"fixed" if the PCRE optimization that uses LIBPCRE2 since b65abcafc7
(grep: use PCRE v2 for optimized fixed-string search, 2019-07-01) is not
available.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t4210-log-i18n.sh | 77 ++++++++++++++++++++++++++++++++++-----------
 t/test-lib.sh       |  6 ----
 2 files changed, 59 insertions(+), 24 deletions(-)

diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index c3792081e6..d2dfcf164e 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -10,6 +10,13 @@ latin1_e=$(printf '\351')
 # invalid UTF-8
 invalid_e=$(printf '\303\50)') # ")" at end to close opening "("
 
+have_reg_illseq=
+if test_have_prereq GETTEXT_LOCALE &&
+	! LC_ALL=$is_IS_locale test-tool regex --silent $latin1_e
+then
+	have_reg_illseq=1
+fi
+
 test_expect_success 'create commits in different encodings' '
 	test_tick &&
 	cat >msg <<-EOF &&
@@ -51,43 +58,77 @@ test_expect_success !MINGW 'log --grep does not find non-reencoded values (utf8)
 	test_must_be_empty actual
 '
 
-test_expect_success !MINGW 'log --grep does not find non-reencoded values (latin1)' '
+test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
 	git log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&
 	test_must_be_empty actual
 '
 
+triggers_undefined_behaviour () {
+	local engine=$1
+
+	case $engine in
+	fixed)
+		if test -n "$have_reg_illseq" &&
+			! test_have_prereq LIBPCRE2
+		then
+			return 0
+		fi
+		;;
+	basic|extended)
+		if test -n "$have_reg_illseq"
+		then
+			return 0
+		fi
+		;;
+	esac
+	return 1
+}
+
+mismatched_git_log () {
+	local pattern=$1
+
+	LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \
+		--grep=$pattern
+}
+
 for engine in fixed basic extended perl
 do
 	prereq=
 	if test $engine = "perl"
 	then
-		prereq="PCRE"
-	else
-		prereq=""
+		prereq=PCRE
 	fi
 	force_regex=
 	if test $engine != "fixed"
 	then
-	    force_regex=.*
+		force_regex='.*'
 	fi
-	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
-		EOF
-		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$latin1_e\" >actual &&
-		test_cmp expect actual
-	"
 
-	test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "-c grep.patternType=$engine log --grep does not find non-reencoded values (latin1 + locale)" "
-		LC_ALL=\"$is_IS_locale\" git -c grep.patternType=$engine log --encoding=ISO-8859-1 --format=%s --grep=\"$force_regex$utf8_e\" >actual &&
-		test_must_be_empty actual
+	test_expect_success $prereq "config grep.patternType=$engine" "
+		git config grep.patternType $engine
 	"
 
-	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_expect_success GETTEXT_LOCALE,$prereq "log --grep does not find non-reencoded values (latin1 + locale)" "
+		mismatched_git_log '$force_regex$utf8_e' >actual &&
 		test_must_be_empty actual
 	"
+
+	if ! triggers_undefined_behaviour $engine
+	then
+		test_expect_success !MINGW,GETTEXT_LOCALE,$prereq "log --grep searches in log output encoding (latin1 + locale)" "
+			cat >expect <<-\EOF &&
+			latin1
+			utf8
+			EOF
+			mismatched_git_log '$force_regex$latin1_e' >actual &&
+			test_cmp expect actual
+		"
+
+		test_expect_success GETTEXT_LOCALE,$prereq "log --grep does not die on invalid UTF-8 value (latin1 + locale + invalid needle)" "
+			mismatched_git_log '$force_regex$invalid_e' >actual &&
+			test_must_be_empty actual
+		"
+	fi
 done
 
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ea1e5a05e..81473fea1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1454,12 +1454,6 @@ 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.27.0.rc0.183.gde8f92d652