Re: [PATCH] t4210: detect REG_ILLSEQ dynamically
From: Eric Sunshine <hidden>
Date: 2020-05-13 15:45:07
On Wed, May 13, 2020 at 7:17 AM Carlo Marcelo Arenas Belón [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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'?
quoted hunk ↗ jump to hunk
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
+ ;;
+ esacWhy 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.)
quoted hunk ↗ jump to hunk
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.