Re: [PATCH] Shell function reporting

[Paul Eggert]

Eric Sunshine <address@hidden> writes:

> 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

At this point, CONFIG_SHELL must support functions, right?

>         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

I don't see how.  The 2nd time through, $CONFIG_SHELL must support
functions so the code must execute the "Yay!" comment only.

Unless CONFIG_SHELL differs from SHELL the 2nd time through, i.e.
they identify different shells the 2nd time through?  Then that would
explain your problem; but in that case it seems to me that we might
have more serious problems since SHELL is incorrect, and we should fix
SHELL somehow.

(There is another problem here: the current code is testing SHELL
rather than CONFIG_SHELL.  It is appropriate to test SHELL since that
(presumably) is the shell that is running this script.  But if SHELL
and CONFIG_SHELL differ, the code won't be testing CONFIG_SHELL, even
though perhaps it should be (if only to warn the user that the
CONFIG_SHELL setting may have problems).  But this is a different
matter than the one you're trying to to address.)


> One way to correct the logic is as follows:
> 
> case $CONFIG_SHELL in
>     CONFIG_SHELL=try_to_find_suitable_shell()
>     ....

This doesn't sound right either, because if CONFIG_SHELL is unset and
SHELL is suitable, the code should use SHELL rather than trying to
find one of our own.  (That is what the code does now.)


> >>> 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
>  ])

This won't print a diagnostic in the case where SHELL doesn't support
shell functions and CONFIG_SHELL is set.  The code should print a
diagnostic in that case, since the rest of the script will be executed
with SHELL and it may not work because of the lack of support for
shell functions.

Option 2 has the same problem.