[PATCH] ERR_EXIT with "for" loops and shell functions (Re: Bug report)

[Bart Schaefer <schaefer@brasslantern.com>]

On Dec 26,  6:35pm, Bart Schaefer wrote:
}
} This is very tricky because we have to propagate the value of $? from
} before the "for" keyword (including before the start of the function)
} into the "do" body, but must *not* *use* the value of $? for deciding
} whether to exit-on-error, because the nonzero value came from an "if"
} test.  There is a "noerrexit" flag that is supposed to cover this case,
} but it's not set when the very first "sublist" in a function body is
} a for-loop (there are likely to be other similar cases).

There's a further complication here, which is this:

    zsh -fc 'false; for x; do :; done; echo $?'
    1

A "for" loop which executes zero times is supposed to set $? == 0, or
at least it does so in bash.  Zsh 5.0.7 passes through $? unchanged when
the "for" does not loop.

Oddly, the current version from git behaves differently:

    Src/zsh -fc 'false; for x in; do false; done; echo $?' 
    0

But:

    Src/zsh -o null_glob -fc 'false; for x in _*; do false; done; echo $?'   
    1

I haven't traced through why a missing "in" list resets $? to zero,
whereas an empty list created by globbing passes through the outer $?.

Here's another difference from bash:

    bash -c 'false; select x in; do false; done; echo $?'
    bash: -c: line 0: syntax error near unexpected token `;'
    bash: -c: line 0: `false; select x in; do false; done; echo $?'

whereas zsh accepts a variety of syntax with inconsistent return: 

    zsh -fc 'true; select x; do :; done; echo $?'
    1
    zsh -fc 'true; select x in; do :; done; echo $?'
    0
    zsh -fc 'true; select x in $empty; do :; done; echo $?'
    1

The first and last of those three also pass syntax in bash and give a
loop that executes zero times and sets $? to zero rather than one.

The following patch attempts to fix all of this weirdness.  I'm a bit
concerned that there may still be cases where prefork substitutions
are performed when they actually should not be because the errexit
[not noerrexit] condition has not been tested soon enough -- it seems
as if zsh is checking errexit at the start of the next command rather
than immediately following the command that returned nonzero, in at
least some circumstances -- but I can't come up with a test case.

Possibly this whole thing needs to be rethought.

Anyway, the most controversial thing below is probably the change in the
return status of "select", which is now always zero unless something in
the loop body returns nonzero.  If we want to disallow empty-"in" syntax
we should do it elsewhere.


diff --git a/Src/exec.c b/Src/exec.c
index 6a7dbb1..eaf73df 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2632,6 +2632,10 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	}
     }
 
+    /* if we get this far, it is OK to pay attention to lastval again */
+    if (noerrexit == 2 && !is_shfunc)
+	noerrexit = 0;
+
     /* Do prefork substitutions */
     esprefork = (assign || isset(MAGICEQUALSUBST)) ? PREFORK_TYPESET : 0;
     if (args && htok)
diff --git a/Src/loop.c b/Src/loop.c
index 8bb1ec9..7b3bdd2 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -102,7 +102,10 @@ execfor(Estate state, int do_exec)
 		addlinknode(args, dupstring(*x));
 	}
     }
-    /* lastval = 0; */
+
+    if (!args || empty(args))
+	lastval = 0;
+
     loops++;
     pushheap();
     cmdpush(CS_FOR);
@@ -238,10 +241,10 @@ execselect(Estate state, UNUSED(int do_exec))
     }
     if (!args || empty(args)) {
 	state->pc = end;
-	return 1;
+	return 0;
     }
     loops++;
-    /* lastval = 0; */
+
     pushheap();
     cmdpush(CS_SELECT);
     usezle = interact && SHTTY != -1 && isset(USEZLE);
@@ -519,14 +522,17 @@ execif(Estate state, int do_exec)
 	s = 1;
 	state->pc = next;
     }
-    noerrexit = olderrexit;
 
     if (run) {
+	/* we need to ignore lastval until we reach execcmd() */
+	noerrexit = olderrexit ? olderrexit : lastval ? 2 : 0;
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
 	execlist(state, 1, do_exec);
 	cmdpop();
-    } else
+    } else {
+	noerrexit = olderrexit;
 	lastval = 0;
+    }
     state->pc = end;
 
     return lastval;

-- 
Barton E. Schaefer