Re: /bin/sh compatibility issue

[YONETANI Tomokazu <qhwt+dfly@xxxxxxxxxx>]

On Wed, May 10, 2006 at 05:59:05PM +0100, Steve O'Hara-Smith wrote:
> On Wed, 10 May 2006 17:49:38 +0200
> joerg@xxxxxxxxxxxxxxxxx wrote:
> 
> > On Wed, May 10, 2006 at 03:39:53PM +0000, Johannes Hofmann wrote:
> > > The following shell script outputs "false1" with all shells I tried
> > > but with /bin/sh on DragonFly. Is this a known incompatibility?
> > 
> > I have to read the POSIX definitions, but this doesn't look very wrong
> > at least. Actually, I can understand this behaviour at the very least.
> 
> 	It doesn't seem to be consistent with the description of -e
> in the sh manpage.
> 
> -e errexit
>   Exit immediately if any untested command fails in non-interactive mode.
>   The exit status of a command is considered to be explicitly tested if the
>   command is used to control an if, elif, while, or until; or if the command
>   is the left hand operand of an ``&&'' or ``||'' operator.
> 
> 	I've quoted the script with added comments.
> 
> #!/bin/sh
> set -e
> if true; then
>    false && echo "huh?"   # This shouldn't exit because false is LHS of &&
> fi
> echo "false1"
> false   # This should exit because false is not explicitly tested
> echo "false2"

/bin/sh in FreeBSD-4.x has the same problem, but not in FreeBSD-5.x or later.
The revisions 1.38,1.41 of eval.c fixes this behavior, but hasn't been MFC'ed
to 4.x(therefore we don't have it either).  The commitlog recommends
that it should be MFC'ed after longer than usual, but I think two and a half
year is long enough :)
Revisions 1.44-1.46 also seem to be -e related fixes.

diff -r 89a6bb686df8 bin/sh/eval.c
--- bin/sh/eval.c	Wed May 03 06:28:01 2006 +0000
+++ bin/sh/eval.c	Thu May 11 11:45:37 2006 +0900
@@ -202,7 +202,6 @@ evaltree(union node *n, int flags)
 	case NAND:
 		evaltree(n->nbinary.ch1, EV_TESTED);
 		if (evalskip || exitstatus != 0) {
-			flags |= EV_TESTED;
 			goto out;
 		}
 		evaltree(n->nbinary.ch2, flags);
@@ -243,25 +242,9 @@ evaltree(union node *n, int flags)
 		break;
 	case NFOR:
 		evalfor(n);
-		/*
-		 * The 'for' command does not set exitstatus, so the value
-		 * now in exitstatus is from the last command executed in
-		 * the 'for' loop.  That exit value had been tested (wrt
-		 * 'sh -e' checking) while processing that command, and
-		 * it should not be re-tested here.
-		 */
-		flags |= EV_TESTED;
 		break;
 	case NCASE:
 		evalcase(n, flags);
-		/*
-		 * The 'case' command does not set exitstatus, so the value
-		 * now in exitstatus is from the last command executed in
-		 * the 'case' block.  That exit value had been tested (wrt
-		 * 'sh -e' checking) while processing that command, and
-		 * it should not be re-tested here.
-		 */
-		flags |= EV_TESTED;
 		break;
 	case NDEFUN:
 		defun(n->narg.text, n->narg.next);
@@ -286,14 +269,9 @@ out:
 out:
 	if (pendingsigs)
 		dotrap();
-	/*
-	 * XXX - Like "!(n->type == NSEMI)", more types will probably
-	 * need to be excluded from this test. It's probably better
-	 * to set or unset EV_TESTED in the loop above than to bloat
-	 * the conditional here.
-	 */
 	if ((flags & EV_EXIT) || (eflag && exitstatus 
-	    && !(flags & EV_TESTED) && !(n->type == NSEMI)))
+	    && !(flags & EV_TESTED) && (n->type == NCMD ||
+	    n->type == NSUBSHELL)))
 		exitshell(exitstatus);
 }