Re: [PATCH] {maint} tests: use more POSIX shell features our test scripts

[Stefano Lattarini]

Hi Eric, thanks for the review.

On 06/14/2012 07:52 PM, Eric Blake wrote:
> On 06/14/2012 10:17 AM, Stefano Lattarini wrote:
>> Since commit 'v1.12-36-g2d68fd9' of 2012-05-07, "configure: search a
>> sturdy POSIX shell to be used in the testsuite", the shell running
>> our test script is assured to be a POSIX-conforming shell, so we can
>> use the more modern and flexible idioms and features that we couldn't
>> use when we also aimed at compatibility with non-POSIX Bourne shells,
>> like Solaris /bin/sh.
>>
>> * t/README: Suggest to use POSIX shell features liberally in test cases,
>> with possible exception of Makefile recipes and configure shell code.
>> * Several tests: Adjust to use more POSIX shell features; e.g., $(...)
>> rather than `...`, $((...)) rather than `expr ...`, "if ! CMD; then ..."
>> instead of "if CMD; then :; else ...", and so on.
>> In several places, when using the 'test' built-in, prefer '-eq' over
>> '=' for numeric comparisons,
> 
> Good.
> 
>> and prefer "grep -c PATTERN FILE" over
>> "grep PATTERN FILE | wc -l".
> 
> Unrelated to POSIX shell, but worth doing.  At this point, though, it's
> not worth splitting into a separate patch.
>
Moreover, all of the "grep PATTERN FILE | wc -l" usages were in some `...`
command substitution that was being touched anyway by the present patch,
so ...

> A few questions, and points for further improvements:
> 
>> -  # Else, use GNU seq if available.
>> -  seq "$@" && return 0
>>    # Otherwise revert to a slower loop using expr(1).
>>    i=$seq_first
>>    while test $i -le $seq_last; do
>>      echo $i
>> -    i=`expr $i + $seq_incr`
>> +    i=$(($i + $seq_incr))
> 
> The comment about expr is now outdated.
>
Good catch, fixed.

>> -ACLOCAL_PATH="`pwd`/none:`pwd`/none2" $ACLOCAL --install && Exit 1
>> +ACLOCAL_PATH="$(pwd)/none:$(pwd)/none2" $ACLOCAL --install && Exit 1
> 
> General observation - now that we guarantee a POSIX shell, we are
> guaranteed that $PWD is sane,
>
Oh, I didn't know that $PWD was mandated by POSIX!  I thought it was a
Bash/Zsh extension ...  I stand corrected, and gladly so :-)

> and can shave a fork instead of using $(pwd).  Probably worth a followup
> patch.
>
Agreed.

>> @@ -79,7 +79,7 @@ check_count=0
>>  check_ ()
>>  {
>>    set +x # Temporary disable shell traces to remove noise from log files.
>> -  incr_ check_count
>> +  check_count=$(($check_count + 1))
> 
> incr_ still looks nicer, and worth keeping as a utility function (even
> if it is changed to be implemented with $(()) instead of expr under the
> hood).
>
In that case, it should be moved out of 'tap-functions.sh' and inside 'defs'.
But the above use of 'incr_' was the only one outside 'tap-functions.sh', so I
wouldn't care to much.

In the long (looong) run, I hope to be able to merge most of 'tap-functions.sh'
and a good part of 'defs' in an external "testsuite library" for shell scripts
(maybe Gnulib's 'tests/init.sh'?  that would be great), in which case it might
make sense to re-introduce the 'incr_' function.

>> +++ b/t/autodist.sh
>> @@ -39,7 +39,7 @@ list=`$AUTOMAKE --help \
>>          | sed -n '/^Files.*automatically distributed.*if found.*always/,/^ 
>> *$/p' \
>>          | sed 1d`
>>  # Normalize whitespace, just in case.
>> -list=`echo $list`
>> +list=$(echo $list)
> 
> This idiom seems common, but wastes a command substitution fork() in
> place of:
> 
> set - $list
> list="$*"
> 
Given the abysmal number of forks out testsuite trigger (just think about any
"./configure" or "make distcheck" invocation!), I wouldn't worry about such
micro-optimization, especially when it involves some minor obfuscation.

>> +++ b/t/ax/tap-functions.sh
>> @@ -31,24 +31,10 @@ tap_fail_count_=0
>>  tap_xfail_count_=0
>>  tap_xpass_count_=0
>>  
>> -# The first "test -n" tries to avoid extra forks when possible.
>> -if test -n "${ZSH_VERSION}${BASH_VERSION}" \
>> -     || (eval 'test $((1 + 1)) = 2') >/dev/null 2>&1
>> -then
>> -  # Outer use of 'eval' needed to protect dumber shells from parsing
>> -  # errors.
>> -  eval 'incr_ () { eval "$1=\$((\${$1} + 1))"; }'
> 
> I think we might as well have:
> 
> incr_ () { eval "$1=\$(($1 + 1))"; }
> 
> as it is still a useful idiom.
> 
>>    case $tap_result_,$tap_directive_ in
>> -    ok,) incr_ tap_pass_count_;;                # Passed.
>> -    not\ ok,TODO) incr_ tap_xfail_count_;;      # Expected failure.
>> -    not\ ok,*) incr_ tap_fail_count_ ;;         # Failed.
>> -    ok,TODO) incr_ tap_xpass_count_ ;;          # Unexpected pass.
>> -    ok,SKIP) incr_ tap_skip_count_ ;;           # Skipped.
>> -    *) bailout_ "internal error in 'result_'";; # Can't happen.
>> +    ok,)                                                # Passed.
>> +      tap_pass_count_=$(($tap_pass_count_ + 1))         ;;
> 
> Case in point - look at the size explosion when you don't keep incr_ around.
> 
> 
>> @@ -169,7 +161,7 @@ skip_ () { result_ 'ok' -D SKIP ${1+"$@"}; }
>>  skip_row_ ()
>>  {
>>    skip_count_=$1; shift
>> -  for i_ in `seq_ $skip_count_`; do skip_ ${1+"$@"}; done
>> +  for i_ in $(seq_ $skip_count_); do skip_ ${1+"$@"}; done
> 
> Now that we assume POSIX shells, is it safe to assume that "$@" is no
> longer buggy?  (Honest question; I haven't researched it well enough
> yet).
>
Me neither, so I decided to err on the side of safety and keep the idiom.

> Or, should we enhance our filter that guarantees us a decent
> shell to also weed out shells where "$@" is broken?
>
I think this is a good idea, yes (for a follow-up patch).

>> +++ b/t/compile4.sh
>> @@ -25,25 +25,24 @@ get_shell_script compile
>>  mkdir sub
>>  
>>  cat >sub/foo.c <<'EOF'
>> -int
>> -foo ()
>> +int foo (void)
> 
> Why the cosmetic change?
>
Consistency with other usages throughout the testsuite (not a "foolish
consistency" IMO: it saves vertical space).

> This violates the GNU Coding Standards
> preferred formatting.
>
Yes, but the reason behind that formatting is enhanced greppability of
the source files (a very good reason in general!), which is not relevant
here (the "sources" being used only by the testsuite, self-contained,
temporary, and mostly dummy).

>> +++ b/t/compile5.sh
>> @@ -46,7 +46,7 @@ case '@host_os@' in
>>      skip_ "target OS is not MinGW"
>>      ;;
>>  esac
>> -case @build_os@ in
>> +case '@build_os@' in
> 
> Why the added quotes?
>
Not sure how this slipped through; will revert.

> Will @biuld_os@ ever expand to more than one shell word?
>
It has never done until today, and I doubt it will start now.  So let's
revert the change as useless noise.

> 
>> @@ -66,45 +66,56 @@ setup_vars_for_compression_format ()
>>    esac
>>  }
>>  
>> +have_compressor ()
>> +{
>> +  test $# -eq 1 || fatal_ "have_compressor(): bad usage"
>> +  case $1 in
>> +    # Assume gzip(1) is available on every reasonable portability target.
>> +    gzip)
>> +      return 0;;
>> +    # On Cygwin, as of 9/2/2012, 'compress' is provided by sharutils
>> +    # and is just a dummy script that is not able to actually compress
>> +    # (it can only decompress).  So, check that the 'compress' program
>> +    # is actually able to compress input.
>> +    # Note that, at least on GNU/Linux, 'compress' does (and is
>> +    # documented to) exit with status 2 if the output is larger than
>> +    # the input after (attempted) compression; so we need to pass it
>> +    # an input that it can actually reduce in size when compressing.
>> +    compress)
>> +      for x in 1 2 3 4 5 6 7 8; do
>> +        echo aaaaaaaaaaaaaaaaaaaaa
>> +      done | compress -c >/dev/null && return 0
>> +      return 1
>> +      ;;
>> +    *)
>> +      # Redirect to stderr to avoid pollutinh the output, in case this
> 
> s/pollutinh/polluting/ while doing this code motion.
>
Fixed.

>> @@ -171,7 +182,7 @@ can_compress ()
>>          && $MAKE dist-$format \
>>          && test -f $tarname-1.0.$suffix \
>>          && ls -l *$tarname* \
>> -        && test "`ls *$tarname*`" = $tarname-1.0.$suffix'
>> +        && test "$(ls *$tarname*)" = $tarname-1.0.$suffix'
> 
> Why are we wasting an ls process?  Wouldn't echo work?
>
Yes; so let's use echo.

> For that matter, even:
> 
> && test *$tarname* = $tarname-1.0.$suffix
> 
> would work in the success case (and hopefully cause a test syntax error
> in the failure case).
>
I don't like that "hopefully" ;-)  Let's go for echo instead -- dumb but safe.

>> @@ -430,7 +441,7 @@ ls -l # For debugging.
>>  cd ..
>>  
>>  oPATH=$PATH
>> -PATH=`pwd`/bin$PATH_SEPARATOR$PATH; export PATH
>> +PATH=$(pwd)/bin$PATH_SEPARATOR$PATH; export PATH
> 
> We're assuming a POSIX shell; shouldn't that mean that we can now write:
> 
> export PATH=$PWD/bin$PATH_SEPARATOR$PATH
>
> or do we still have to worry about shells that don't support 'export
> name=$expansion'?
>
I honestly don't know.  Git's testsuite (which assumes a POSIX shell) seems
to avoid uses of "export VAR=VAL", and that suggests that some POSIX shells
might have problems grasping the idiom -- or at least that the Git's
developers believe so.  To be safer, we might need to add a spy test to
the testsuite to verify how well the idiom actually works in the wild; if
no problems arise, we can add a check for it in out configure.ac, and then
assume it works in our testcases.

> And even if we assume export with assignment works,
> do we still have to worry about shells that word-split expansion, where
> you'd have to write:
> 
> export PATH="$PWD/bin$PATH_SEPARATOR$PATH"
>
Absolutely:

  $ dash -c 'x="@ @"; export x=$x; echo $x'
  export: 1: @: bad variable name

  $ /bin/sh -c 'x="@ @"; export x=$x; echo $x' # On NetBSD
  export: @: bad variable name

(although it works with Bash, Zsh, and even Solaris /bin/ksh).

>> +++ b/t/gcj3.sh
>> @@ -31,7 +31,6 @@ END
>>  $ACLOCAL
>>  $AUTOMAKE
>>  
>> -num=`grep depcomp Makefile.in | wc -l`
>> -test $num -gt 1
>> +test $($FGREP -c depcomp Makefile.in) -gt 1
> 
> Are you sure this idiom is safe on shells that don't handle Ctrl-C very
> well?
>
Is this relevant, considering we have installed an exit trap that bails
out on common signals?  For example, with NetBSD 5.1 /bin/sh:

  $ trap : 2
  $ sh -c 'trap "exit 99" 2; test -z "$(sleep 4; echo x)"' # Then I press ^C
  ^C
  $ echo $?
  99

> As documented in autoconf, even some /bin/sh that otherwise look
> like POSIX (I think it was a BSD machine) will elide a terminateed
> command substitution and still evaluate the resulting expression; in
> cases where the result is like 'test -gt 1', the syntax error has the
> desired effect, but I'm worried there are other places where you could
> end up with problems when mixing a command substitution in the middle of
> a complex command instead of using a temporary.
> 

>> +++ b/t/hdr-vars-defined-once.sh
> 
> This is as far as I got in my line-by-line review in the time I have
> today; if you want to wait for another few days, I can resume my review
> in time for your initial push.  Or you can go ahead and push, and I can
> report any further issues for followup patches, since the overall idea
> looks good and most of the changes appear to have been mechanical.
>
I will gladly wait a couple of days before pushing, in the hope you'll
find time to complete your review.

Thanks,
  Stefano