test-lib: check Bash version for '-x' without using shell arrays
diff mbox series

Message ID 20190101231949.8184-1-szeder.dev@gmail.com
State New
Headers show
Series
  • test-lib: check Bash version for '-x' without using shell arrays
Related show

Commit Message

SZEDER Gábor Jan. 1, 2019, 11:19 p.m. UTC
One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
reliably run with '-x' tracing enabled, unless it's executed with a
Bash version supporting BASH_XTRACEFD (since v4.1).  We have a lengthy
condition to check the version of the shell running the test script,
and disable tracing if it's not executed with a suitable Bash version
[2].

This condition uses non-portable shell array accesses to easily get
Bash's major and minor version number.  This didn't seem to be
problematic, because the simple commands expanding those array
accesses are only executed when the test script is actually run with
Bash.  When run with Dash, the only shell I have at hand that doesn't
support shell arrays, there are no issues, as it apparently skips
right over the non-executed simple commands without noticing the
non-supported constructs.

Alas, it has been reported that NetBSD's /bin/sh does complain about
them:

  ./test-lib.sh: 327: Syntax error: Bad substitution

where line 327 contains the first ${BASH_VERSINFO[0]} array access.

To my understanding both shells are right and conform to POSIX,
because the standard allows both behavior by stating the following
under '2.8.1 Consequences of Shell Errors':

  "An expansion error is one that occurs when the shell expansions
  define in wordexp are carried out (for example, "${x!y}", because
  '!' is not a valid operator); an implementation may treat these as
  syntax errors if it is able to detect them during tokenization,
  rather than during expansion."

Avoid this issue with NetBSD's /bin/sh (and potentially with other,
less common shells) by using a couple of parameter expansions to
extract the major and minor version numbers from $BASH_VERSION.

[1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
    2018-02-24)
[2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
    test scripts, 2018-02-24)

Reported-by: Max Kirillov <max@max630.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

This will certainly conflict with patches 3 and 6 in my stress testing
patch series at

  https://public-inbox.org/git/20181230191629.3232-1-szeder.dev@gmail.com/T/#u


 t/test-lib.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Johannes Sixt Jan. 2, 2019, 12:20 a.m. UTC | #1
Am 02.01.19 um 00:19 schrieb SZEDER Gábor:
> Alas, it has been reported that NetBSD's /bin/sh does complain about
> them:
> 
>    ./test-lib.sh: 327: Syntax error: Bad substitution
> 
> where line 327 contains the first ${BASH_VERSINFO[0]} array access.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f1faa24b2..f47a191e3b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -324,9 +324,12 @@ do
>   		# isn't executed with a suitable Bash version.
>   		if test -z "$test_untraceable" || {
>   		     test -n "$BASH_VERSION" && {
> -		       test ${BASH_VERSINFO[0]} -gt 4 || {
> -			 test ${BASH_VERSINFO[0]} -eq 4 &&
> -			 test ${BASH_VERSINFO[1]} -ge 1
> +		       bash_major=${BASH_VERSION%%.*}
> +		       bash_minor=${BASH_VERSION#*.}
> +		       bash_minor=${bash_minor%%.*}
> +		       test $bash_major -gt 4 || {
> +			 test $bash_major -eq 4 &&
> +			 test $bash_minor -ge 1
>   		       }
>   		     }
>   		   }
> 

Would it perhaps be simpler to just hide the syntax behind eval? Like

  		if test -z "$test_untraceable" || {
  		     test -n "$BASH_VERSION" && eval '
		       test ${BASH_VERSINFO[0]} -gt 4 || {
			 test ${BASH_VERSINFO[0]} -eq 4 &&
			 test ${BASH_VERSINFO[1]} -ge 1
		       }
  		     '

-- Hannes
Junio C Hamano Jan. 2, 2019, 4:46 p.m. UTC | #2
Johannes Sixt <j6t@kdbg.org> writes:

> Would it perhaps be simpler to just hide the syntax behind eval? Like
>
>  		if test -z "$test_untraceable" || {
>  		     test -n "$BASH_VERSION" && eval '
> 		       test ${BASH_VERSINFO[0]} -gt 4 || {
> 			 test ${BASH_VERSINFO[0]} -eq 4 &&
> 			 test ${BASH_VERSINFO[1]} -ge 1
> 		       }
>  		     '

It indeed is somewhat easier to see that your version does exactly
the same as the original than the posted patch, but the pattern
substitutions are not all that bad, either.
Eric Sunshine Jan. 2, 2019, 6:05 p.m. UTC | #3
On Tue, Jan 1, 2019 at 6:20 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> [...]
> To my understanding both shells are right and conform to POSIX,
> because the standard allows both behavior by stating the following

s/behavior/behaviors/

> under '2.8.1 Consequences of Shell Errors':
>
> Reported-by: Max Kirillov <max@max630.net>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Carlo Arenas Jan. 2, 2019, 6:31 p.m. UTC | #4
Tested-By: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Jeff King Jan. 3, 2019, 4:52 a.m. UTC | #5
On Wed, Jan 02, 2019 at 01:20:47AM +0100, Johannes Sixt wrote:

> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 0f1faa24b2..f47a191e3b 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -324,9 +324,12 @@ do
> >   		# isn't executed with a suitable Bash version.
> >   		if test -z "$test_untraceable" || {
> >   		     test -n "$BASH_VERSION" && {
> > -		       test ${BASH_VERSINFO[0]} -gt 4 || {
> > -			 test ${BASH_VERSINFO[0]} -eq 4 &&
> > -			 test ${BASH_VERSINFO[1]} -ge 1
> > +		       bash_major=${BASH_VERSION%%.*}
> > +		       bash_minor=${BASH_VERSION#*.}
> > +		       bash_minor=${bash_minor%%.*}
> > +		       test $bash_major -gt 4 || {
> > +			 test $bash_major -eq 4 &&
> > +			 test $bash_minor -ge 1
> >   		       }
> >   		     }
> >   		   }
> > 
> 
> Would it perhaps be simpler to just hide the syntax behind eval? Like
> 
>  		if test -z "$test_untraceable" || {
>  		     test -n "$BASH_VERSION" && eval '
> 		       test ${BASH_VERSINFO[0]} -gt 4 || {
> 			 test ${BASH_VERSINFO[0]} -eq 4 &&
> 			 test ${BASH_VERSINFO[1]} -ge 1
> 		       }
>  		     '

That was my first thought, too. :)

The parsing here is simple enough that I'd be fine either with the
original patch, or an eval-based version (and otherwise, the goal and
description seem quite good to me).

-Peff
SZEDER Gábor Jan. 3, 2019, 11:36 a.m. UTC | #6
On Wed, Jan 02, 2019 at 12:19:49AM +0100, SZEDER Gábor wrote:
> To my understanding both shells are right and conform to POSIX,
> because the standard allows both behavior by stating the following
> under '2.8.1 Consequences of Shell Errors':
> 
>   "An expansion error is one that occurs when the shell expansions
>   define in wordexp are carried out (for example, "${x!y}", because

That "define" above always stops my (non-native) English parser for a
moment or two...  shouldn't it be s/define/defined/ ?

It's not a copy-paste error, POSIX does indeed write "define" there,
see:

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_01

(First paragraph below the table and the Notes.)

>   '!' is not a valid operator); an implementation may treat these as
>   syntax errors if it is able to detect them during tokenization,
>   rather than during expansion."
>

Patch
diff mbox series

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..f47a191e3b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -324,9 +324,12 @@  do
 		# isn't executed with a suitable Bash version.
 		if test -z "$test_untraceable" || {
 		     test -n "$BASH_VERSION" && {
-		       test ${BASH_VERSINFO[0]} -gt 4 || {
-			 test ${BASH_VERSINFO[0]} -eq 4 &&
-			 test ${BASH_VERSINFO[1]} -ge 1
+		       bash_major=${BASH_VERSION%%.*}
+		       bash_minor=${BASH_VERSION#*.}
+		       bash_minor=${bash_minor%%.*}
+		       test $bash_major -gt 4 || {
+			 test $bash_major -eq 4 &&
+			 test $bash_minor -ge 1
 		       }
 		     }
 		   }