Re: [PATCH] init.sh: disqualify shells for which set -x corrupts stderr

[Jim Meyering]

Jim Meyering wrote:
> Eric Blake wrote:
>> On 09/08/2010 03:17 PM, Jim Meyering wrote:
>>>
>>> +# Whether to reject a shell for which "set -x" corrupts stderr.
>>> +strict_=yes
>>> +
>>> +gl_set_x_corrupts_stderr_='$( (exec 3>&1; set -x; P=1 true 2>&3) 2>  
>>> /dev/null)'
>>> +
>>>   gl_shell_test_script_='
>>>   test $(echo y) = y || exit 1
>>> -test -z "$( (exec 3>&1; set -x; P=1 true 2>&3) 2>  /dev/null)" || exit 1
>>> +if test $strict_ = yes&&  test -n "$gl_set_x_corrupts_stderr_"; then
>>> +  exit 1
>>> +fi
>>
>> Hmm - the value of $strict_ and $gl_set_x_corrupts_stderr_ are known
>> to the parent shell, but not exported to the child shell.
>>
>>> +    for strict_ in yes no; do
>>
>> Reusing the same variable as you set earlier will leave $strict_ in
>> the last state that it was in during the shell search.  Is that
>> intended, or should you be iterating on a different variable name?
>>
>>> +      # Search for a shell that meets our requirements.
>>> +      for re_shell_ in "${CONFIG_SHELL:-no_shell}" /bin/sh bash dash zsh 
>>> pdksh
>>> +      do
>>> +        test "$re_shell_" = no_shell&&  continue
>>> +        test "$re_shell_" = fail&&  skip_ failed to find an adequate shell
>>> +        "$re_shell_" -c "$gl_shell_test_script_" 2>/dev/null
>>
>> You either need an eval on this line (to expand the embedded $strict_
>> within $gl_shell_test_script_), or you need to export some variables.
>
> Thanks for the quick review.
> I noticed that one right away ;-)
> Incremental below:
>
> I'll address your other points after some sleep.
>
>> Another thought - why do two passes?  Maybe a better option would be
>> doing one pass, with two successful exit statuses (9 if the shell
>> can't use set -x but otherwise works, 10 if the shell does both); then
>> iterate until you either find a 10, or else pick the first shell that
>> gave 9.

Good idea.
That's more efficient, but also a little more invasive.  Worth it, though.
I've done that and reworked the rest to eliminate the two
variables you mentioned above.

Here's an improved patch:

>From f7a9af7f49d267d084e47fab6853c81ad65f09e9 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 8 Sep 2010 22:24:22 +0200
Subject: [PATCH] init.sh: penalize a set-x-impaired shell; don't disqualify it

* tests/init.sh: Too many shells corrupt application stderr when
you set -x, so we can't afford to disqualify them, since at least
on Irix-6.5, that would disqualify all bourne shells.
Instead, use a more discerning approach.
When iterating through candidate shells, give each a score
of 10 for perfect, 9 if corrupts stderr upon set -x but passes
all other tests, and some other value if it is unacceptable.
Use the first shell that gets a score of 10.
If no shell scores 10, use the first that scored 9.
Finally, when VERBOSE=yes is requested and set -x might cause trouble, simply
issue a warning and refrain from enabling debug output.
---
 ChangeLog     |   14 ++++++++
 tests/init.sh |  105 +++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 90 insertions(+), 29 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 82937ac..2ac4e00 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2010-09-08  Jim Meyering  <address@hidden>
+
+       init.sh: penalize a set-x-impaired shell; don't disqualify it
+       * tests/init.sh: Too many shells corrupt application stderr when
+       you set -x, so we can't afford to disqualify them, since at least
+       on Irix-6.5, that would disqualify all bourne shells.
+       Instead, use a two-pass approach.
+       On the first pass, try to find a shell that meets the stricter
+       condition that set -x does not corrupt stderr.
+       If no shell meets the stricter condition, retest each candidate
+       shell, but without that extra condition.  Finally, when
+       VERBOSE=yes is requested and set -x might cause trouble, simply
+       issue a warning and refrain from enabling debug output.
+
 2010-09-08  Eric Blake  <address@hidden>

        unsetenv: fix OpenBSD bug
diff --git a/tests/init.sh b/tests/init.sh
index 9886a8d..c5d1961 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -109,48 +109,84 @@ fi
 # Use "9" to indicate success (rather than 0), in case some shell acts
 # like Solaris 10's /bin/sh but exits successfully instead of with status 2.

+# Eval this code in a subshell to determine a shell's suitability.
+# 10 - passes all tests; ok to use
+#  9 - ok, but enabling "set -x" corrupts application stderr; prefer higher 
score
+#  ? - not ok
 gl_shell_test_script_='
 test $(echo y) = y || exit 1
-test -z "$( (exec 3>&1; set -x; P=1 true 2>&3) 2> /dev/null)" || exit 1
-test -z "$EXEEXT" && exit 9
+score_=10
+if test "$VERBOSE" = yes; then
+  test -n "$( (exec 3>&1; set -x; P=1 true 2>&3) 2> /dev/null)" && score_=9
+fi
+test -z "$EXEEXT" && exit $score_
 shopt -s expand_aliases
 alias a-b="echo zoo"
 v=abx
      test ${v%x} = ab \
   && test ${v#a} = bx \
   && test $(a-b) = zoo \
-  && exit 9
+  && exit $score_
 '

 if test "x$1" = "x--no-reexec"; then
   shift
 else
-  # 'eval'ing the above code makes Solaris 10's /bin/sh exit with $? set to 2.
-  # It does not evaluate any of the code after the "unexpected" `('.  Thus,
-  # we must run it in a subshell.
-  ( eval "$gl_shell_test_script_" ) > /dev/null 2>&1
-  if test $? = 9; then
-    : # The current shell is adequate.  No re-exec required.
-  else
-    # Search for a shell that meets our requirements.
-    for re_shell_ in "${CONFIG_SHELL:-no_shell}" /bin/sh bash dash zsh pdksh 
fail
-    do
-      test "$re_shell_" = no_shell && continue
-      test "$re_shell_" = fail && skip_ failed to find an adequate shell
+  # Assume a working shell.  Export to subshells (setup_ needs this).
+  gl_set_x_corrupts_stderr_=false
+  export gl_set_x_corrupts_stderr_
+
+  # Record the first marginally acceptable shell.
+  marginal_=
+
+  # Search for a shell that meets our requirements.
+  for re_shell_ in __current__ "${CONFIG_SHELL:-no_shell}" \
+      /bin/sh bash dash zsh pdksh fail
+  do
+    test "$re_shell_" = no_shell && continue
+
+    # If we've made it all the way to the sentinel, "fail" without
+    # finding even a marginal shell, skip this test.
+    if test "$re_shell_" = fail; then
+      test -z "$marginal_" && skip_ failed to find an adequate shell
+      re_shell_=$marginal_
+      break
+    fi
+
+    # When testing the current shell, simply "eval" the test code.
+    # Otherwise, run it via $re_shell_ -c ...
+    if test "$re_shell_" = __current__; then
+      # 'eval'ing this code makes Solaris 10's /bin/sh exit with
+      # $? set to 2.  It does not evaluate any of the code after the
+      # "unexpected" first `('.  Thus, we must run it in a subshell.
+      ( eval "$gl_shell_test_script_" ) > /dev/null 2>&1
+    else
       "$re_shell_" -c "$gl_shell_test_script_" 2>/dev/null
-      if test $? = 9; then
-        # Found an acceptable shell.  Preserve -v and -x.
-        case $- in
-          *v*x* | *x*v*) opts_=-vx ;;
-          *v*) opts_=-v ;;
-          *x*) opts_=-x ;;
-          *) opts_= ;;
-        esac
-        exec "$re_shell_" $opts_ "$0" --no-reexec "$@"
-        echo "$ME_: exec failed" 1>&2
-        exit 127
-      fi
-    done
+    fi
+
+    st_=$?
+
+    # $re_shell_ works just fine.  Use it.
+    test $st_ = 10 && break
+
+    # If this is our first marginally acceptable shell, remember it.
+    if test "$st_:$marginal_" = 9: ; then
+      marginal_="$re_shell_"
+      gl_set_x_corrupts_stderr_=true
+    fi
+  done
+
+  if test "$re_shell_" != __current__; then
+    # Found a usable shell.  Preserve -v and -x.
+    case $- in
+      *v*x* | *x*v*) opts_=-vx ;;
+      *v*) opts_=-v ;;
+      *x*) opts_=-x ;;
+      *) opts_= ;;
+    esac
+    exec "$re_shell_" $opts_ "$0" --no-reexec "$@"
+    echo "$ME_: exec failed" 1>&2
+    exit 127
   fi
 fi

@@ -269,7 +305,18 @@ path_prepend_()

 setup_()
 {
-  test "$VERBOSE" = yes && set -x
+  if test "$VERBOSE" = yes; then
+    # Test whether set -x may cause the selected shell to corrupt an
+    # application's stderr.  Many do, including zsh-4.3.10 and the /bin/sh
+    # from SunOS 5.11, OpenBSD 4.7 and Irix 5.x and 6.5.
+    # If enabling verbose output this way would cause trouble, simply
+    # issue a warning and refrain.
+    if $gl_set_x_corrupts_stderr_; then
+      warn_ "using SHELL=$SHELL with 'set -x' corrupts stderr"
+    else
+      set -x
+    fi
+  fi

   initial_cwd_=$PWD

--
1.7.3.rc0.174.g69763