test-lib: fix non-portable pattern bracket expressions
diff mbox series

Message ID 20190211195803.1682-1-szeder.dev@gmail.com
State New
Headers show
Series
  • test-lib: fix non-portable pattern bracket expressions
Related show

Commit Message

SZEDER Gábor Feb. 11, 2019, 7:58 p.m. UTC
Use a '!' character to start a non-matching pattern bracket
expression, as specified by POSIX in Shell Command Language section
2.13.1 Patterns Matching a Single Character [1].

I used '^' instead in three places in the previous three commits, to
verify that the arguments of the '--stress=' and '--stress-limit='
options and the values of various '*_PORT' environment variables are
valid numbers.  With certain shells, at least with dash (upstream and
in Ubuntu 14.04) and mksh, this led to various undesired behaviors:

  # error message in case of a valid number
  $ ~/src/dash/src/dash ./t3903-stash.sh --stress=8
  error: --stress=<N> requires the number of jobs to run

  # not the expected error message
  $ ~/src/dash/src/dash ./t3903-stash.sh --stress=foo
  ./t3903-stash.sh: 238: test: Illegal number: foo

  # no error message at all?!
  $ mksh ./t3903-stash.sh --stress=foo
  $ echo $?
  0

Some other shells, e.g. Bash (even in posix mode), ksh, dash in Ubuntu
16.04 or later, are apparently happy to accept '^' just as well.

[1] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_13

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Should go on top of 'sg/stress-test'.


 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Feb. 11, 2019, 10:29 p.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> Use a '!' character to start a non-matching pattern bracket
> expression, as specified by POSIX in Shell Command Language section
> 2.13.1 Patterns Matching a Single Character [1].
>
> I used '^' instead in three places in the previous three commits, to

Ah, thanks for catching them.  We should have caught these during
review.

> Should go on top of 'sg/stress-test'.

Thanks.
Jeff King Feb. 11, 2019, 11:46 p.m. UTC | #2
On Mon, Feb 11, 2019 at 08:58:03PM +0100, SZEDER Gábor wrote:

> Use a '!' character to start a non-matching pattern bracket
> expression, as specified by POSIX in Shell Command Language section
> 2.13.1 Patterns Matching a Single Character [1].

Just when I think I know every little gotcha in the shell, I learn
another one. :) Thanks for fixing this, and for digging up the POSIX
reference.

-Peff
SZEDER Gábor Feb. 12, 2019, 12:30 a.m. UTC | #3
On Mon, Feb 11, 2019 at 06:46:26PM -0500, Jeff King wrote:
> On Mon, Feb 11, 2019 at 08:58:03PM +0100, SZEDER Gábor wrote:
> 
> > Use a '!' character to start a non-matching pattern bracket
> > expression, as specified by POSIX in Shell Command Language section
> > 2.13.1 Patterns Matching a Single Character [1].
> 
> Just when I think I know every little gotcha in the shell,

I've already gave up on that :)

> I learn
> another one. :) Thanks for fixing this, and for digging up the POSIX
> reference.

I had to, that 16.04's dash worked, but neither dash in older LTS nor
newer upstream version did was particularly puzzling.  Turns out that
in 0.5.8-1 Ubuntu (Debian? dunno.) started to configure dash with
'--enable-fnmatch', which makes it understand '^' as well.
Jeff King Feb. 12, 2019, 12:34 a.m. UTC | #4
On Tue, Feb 12, 2019 at 01:30:18AM +0100, SZEDER Gábor wrote:

> > I learn
> > another one. :) Thanks for fixing this, and for digging up the POSIX
> > reference.
> 
> I had to, that 16.04's dash worked, but neither dash in older LTS nor
> newer upstream version did was particularly puzzling.  Turns out that
> in 0.5.8-1 Ubuntu (Debian? dunno.) started to configure dash with
> '--enable-fnmatch', which makes it understand '^' as well.

Ah, doubly puzzling. It works fine in my Debian dash (but fnmatch was
enabled in 0.5.8-1 there, too).

-Peff
Carlo Arenas Feb. 12, 2019, 10:47 a.m. UTC | #5
On Mon, Feb 11, 2019 at 4:35 PM Jeff King <peff@peff.net> wrote:
>
> Ah, doubly puzzling. It works fine in my Debian dash (but fnmatch was
> enabled in 0.5.8-1 there, too).

FWIW, it is reported by both checkbashisms and shellcheck if "linting"
could be considered of help to track these kind of issues.

other "portability" issues seem safe, mainly because they are either silly
(like the use of local) or meant to be used only with bash (as on the mingw
specific functions or the bash version check that was broken recently for
NetBSD) but the functions run_with_limited_{cmdline,stack} that use `ulimit -s`
might be worth looking into

Carlo

Patch
diff mbox series

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 92cf8f812c..969e2ba6da 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1289,7 +1289,7 @@  test_set_port () {
 			port=$(($port + 10000))
 		fi
 		;;
-	*[^0-9]*|0*)
+	*[!0-9]*|0*)
 		error >&7 "invalid port number: $port"
 		;;
 	*)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 77eff04c92..4e7cb52b57 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -144,7 +144,7 @@  do
 	--stress=*)
 		stress=${opt#--*=}
 		case "$stress" in
-		*[^0-9]*|0*|"")
+		*[!0-9]*|0*|"")
 			echo "error: --stress=<N> requires the number of jobs to run" >&2
 			exit 1
 			;;
@@ -155,7 +155,7 @@  do
 	--stress-limit=*)
 		stress_limit=${opt#--*=}
 		case "$stress_limit" in
-		*[^0-9]*|0*|"")
+		*[!0-9]*|0*|"")
 			echo "error: --stress-limit=<N> requires the number of repetitions" >&2
 			exit 1
 			;;