Re: [PATCH] Shell function reporting

[Eric Sunshine]

On 06 Jan 2004 12:34:42 -0800, Paul Eggert wrote:
> Eric Sunshine <address@hidden> writes:
> > In the case when $SHELL does not support functions,
> > AS_SHELL_SANITIZE() reports that it could not find an appropriate
> > shell even if it actually locates such a shell (which it assigns to
> > CONFIG_SHELL).
> Hmm, how could it assign to CONFIG_SHELL and then report that it
> couldn't find a shell?  The code does this:
> CONFIG_SHELL=$as_dir/$as_base
> export CONFIG_SHELL
> exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
> so if it assigns to CONFIG_SHELL, it execs $CONFIG_SHELL, which means
> it shouldn't report that it can't find a shell.

Indeed, that's the way it is supposed to work, but that's not the way it is  
presently coded up.  If you trace through it, you will see the flaw.  Here is  
what the code is actually doing (pseudo-code):

if $SHELL supports functions
  # Yay!
else
  case $CONFIG_SHELL in
    '')
      CONFIG_SHELL=try_to_find_suitable_shell()
      if CONFIG_SHELL is set
        exec $CONFIG_SHELL $this_script
      endif
      ;;
    *)
      echo "Failed to find suitable shell"  <<-- WRONG!
      ;;
  esac
endif

Assuming the $SHELL test fails, the first time through CONFIG_SHELL is not  
set, so it tries to find a suitable shell.  If it finds one, then it exports  
CONFIG_SHELL re-invokes the script.  Now, the second time through,  
CONFIG_SHELL is set, so it falls down to the '*' case which incorrectly  
prints an error saying that the shell was not found (even though it was).

One way to correct the logic is as follows:

case $CONFIG_SHELL in
    CONFIG_SHELL=try_to_find_suitable_shell()
    if CONFIG_SHELL is set
      exec $CONFIG_SHELL $this_script
    else
      echo "Failed to find suitable shell"  <<-- CORRECT
    endif
    ;;
  *)
    ;;
esac

This way, we emit the error message iff if we actually fail to find a  
suitable shell.

> Under the proposed patch, 'configure' would sometimes ignore
> CONFIG_SHELL and substitute its own CONFIG_SHELL.  This doesn't seem
> right to me, as the user shouldn't be second-guessed.

I also came to this conclusion after sending out the patch and decided to  
send a simpler patch which still allows the user to shoot himself in the foot  
if he so desires.

> Yes, this looks like a problem to me too.  How about this patch instead?

No, that patch does not work either.  It also incorrectly complains that it  
could not find a shell even when it does find one.  I have included below two  
separate super simple patches, each of which corrects the logic.  The second  
of the two patches is lengthier, but it is also much simpler to understand  
since it doesn't suffer from the convolusions which gave rise to this bug in  
the first place.  Choose the patch which you like best.

> (I don't know what the "In the future" comment means, so I removed it.)

I presume that Paolo could explain it since he wrote it.

Eric


2004-01-06  Eric Sunshine  <address@hidden>

        * lib/m4sugar/m4sh.m4 (AS_SHELL_SANITIZE): Fixed bogus error reporting
        logic.  If $SHELL did not support shell functions, and if it found a
        shell which did support functions, it would complain that it failed to
        find such a shell (backward logic).  Worse, it did not complain if it
        failed to find a suitable shell.  Now it complains iff it fails to find
        a function-supporting shell.


>>> option 1 <<<

--- lib/m4sugar/m4sh.m4 Tue Jan  6 18:39:20 2004
+++ lib/m4sugar/m4sh.m4-fix     Tue Jan  6 18:37:32 2004
@@ -264,9 +264,11 @@
             exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
           ]);;
         esac
-       done]);;
-  *)
+       done])
+    # If we got this far, we failed to find a function-supporting shell.
     $1;;
+  *)
+    ;;
   esac
 ])


>>> option 1 <<<

--- lib/m4sugar/m4sh.m4 Tue Jan  6 19:04:41 2004
+++ lib/m4sugar/m4sh.m4-fix2    Tue Jan  6 19:04:26 2004
@@ -249,9 +249,8 @@
 fi

 dnl In the future, the `else' branch will be that in AS_INIT_WITH_SHELL_FN.
-AS_IF([_AS_SHELL_FN_WORK([$SHELL])], [], [
-  case $CONFIG_SHELL in
-  '')
+AS_IF([test -z "$CONFIG_SHELL"],
+  [AS_IF([_AS_SHELL_FN_WORK([$SHELL])], [], [
     _AS_PATH_WALK([/bin$PATH_SEPARATOR/usr/bin$PATH_SEPARATOR$PATH],
       [for as_base in sh bash ksh sh5; do
         case $as_dir in
@@ -264,11 +263,9 @@
             exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
           ]);;
         esac
-       done]);;
-  *)
-    $1;;
-  esac
-])
+       done])
+    $1
+])])

 # Work around bugs in pre-3.0 UWIN ksh.
 $as_unset ENV MAIL MAILPATH