Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays

[Junio C Hamano <gitster@pobox.com>]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> 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 behaviors by stating the following
> under '2.8.1 Consequences of Shell Errors' [3]:
>
>   "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 hiding the shell array syntax behind 'eval'
> that is only executed with Bash.
>
> [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)
> [3] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_01
>
> Reported-by: Max Kirillov <max@max630.net>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>
> Changes since v1:
>
>  - Hide the shell array syntax behind 'eval'.
>    (I'm fine with both versions, take your pick.)
>  - Corrected typo in commit message that Eric pointed out.
>  - Added a link to the relevant section in POSIX.

Let's treat this as an independent and more urgent fix-up.  I think
it is sufficient to apply it to 2.20.x track, even though we could
go back to 2.17.x and above.

And then let's tentatively kick the "stress test" series out of
'pu', and have that series rebuilt on top of 'master' and this
patch.

Thanks.

>
>  t/test-lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f1faa24b2..c34831a4de 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -323,12 +323,12 @@ do
>  		# this test is marked as such, and ignore '-x' if it
>  		# isn't executed with a suitable Bash version.
>  		if test -z "$test_untraceable" || {
> -		     test -n "$BASH_VERSION" && {
> +		     test -n "$BASH_VERSION" && eval '
>  		       test ${BASH_VERSINFO[0]} -gt 4 || {
>  			 test ${BASH_VERSINFO[0]} -eq 4 &&
>  			 test ${BASH_VERSINFO[1]} -ge 1
>  		       }
> -		     }
> +		     '
>  		   }
>  		then
>  			trace=t