Re: [PATCH] Remove NOERREXIT_UNTIL_EXEC

[Philippe Altherr <philippe.altherr@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3883 bytes --]

Stupid me. I forgot the patch! Here it is.

Philippe


On Sat, Dec 3, 2022 at 1:31 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> The noerrexit NOERREXIT_UNTIL_EXEC bit is never set and can therefore be
> removed. In other words the patch only removes dead code.
>
> I have been intrigued by the NOERREXIT_UNTIL_EXEC bit for a while. I
> didn't understand its purpose nor why the NOERREXIT_EXIT and
> NOERREXIT_RETURN bits wouldn't be enough to solve the problem at hand.
> After finally having a closer look, I must admit that I still don't
> understand. However, I discovered that the bit is never set and can safely
> be removed. More on that below.
>
> The bit was introduced in commit d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065>,
> where the noerrexit value 2 corresponds to today's NOERREXIT_UNTIL_EXEC and
> the value 1 to NOERREXIT_EXIT. There was no distinction yet between
> NOERREXIT_EXIT and NOERREXIT_RETURN. At that time, there was also no
> this_noerrexit variable. The distinction between NOERREXIT_EXIT and
> NOERREXIT_RETURN was introduced in commit 97d4bdb
> <https://github.com/zsh-users/zsh/commit/97d4bdbc7e86e6e8da0d4a059b118ffab289d3a9>,
> which also introduced today's noerrexit bits.
>
> One thing that distinguishes the NOERREXIT_UNTIL_EXEC bit from the other
> noerrexit bits is that it seems intended for the caller(s) of the function
> that sets it, while the other bits are intended for the callee(s). This is
> similar to today's this_noerrexit variable. I wonder whether this bit was a
> first attempt at solving the problem that is solved in today's code by the
> this_noerrexit variable. This would explain why everything is still fine
> even though the bit is no longer set.
>
> In today's code, the NOERREXIT_UNTIL_EXEC bit is only ever set in loop.c
> at line 578
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L578>.
> This line can only be reached if run != 0 && run !=2 && olderrexit == 0 &&
> lastval == 0. The loop above
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551> can
> only be exited at one of the following lines:
>
> - line 551
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551>=>
> run == 0
> - line 557
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L557>
> => run == 0 || run == 2
> - line 565
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L565>
> => run == 1 && lastval != 0
> - line 568
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L568>
> => run == 0 && lastval == 0
>
> None of these cases lead to line 578. Thus the NOERREXIT_UNTIL_EXEC bit is
> never set and can safely be removed.
>
> The code "noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN)" at line 580
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L580> can
> be simplified into "noerrexit = olderrexit" because NOERREXIT_UNTIL_EXEC
> was the only noerrexit bit that was flowing from callees to their caller.
>
> Obviously all tests still pass. And I checked that the tests introduced by commit
> d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065> are
> still present.
>
> I have built this patch on top of my patch
> patch-06-fix-eval-and-source-statements.txt but I don't think that it
> depends on it nor on any of my previous patches (but I haven't tried to
> confirm it).
>
> QUESTION: Should NOERREXIT_SIGNAL's value
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/zsh.h#L2226>
> be changed to 4 (from 8) since the value 4 is no longer used
> by NOERREXIT_UNTIL_EXEC?
>
> Philippe
>
>

[-- Attachment #1.2: Type: text/html, Size: 4841 bytes --]

[-- Attachment #2: patch-07-remove-noerrexit-until-exec.txt --]
[-- Type: text/plain, Size: 2165 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 8ff6489ec..1dd569019 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1559,14 +1559,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	state->pc--;
 sublist_done:
 
-	/*
-	 * See hairy code near the end of execif() for the
-	 * following.  "noerrexit " only applies until
-	 * we hit execcmd on the way down.  We're now
-	 * on the way back up, so don't restore it.
-	 */
-	if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
-	    noerrexit = oldnoerrexit;
+	noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
 	    /*
@@ -3246,10 +3239,6 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     } else
 	preargs = NULL;
 
-    /* if we get this far, it is OK to pay attention to lastval again */
-    if (noerrexit & NOERREXIT_UNTIL_EXEC)
-	noerrexit = 0;
-
     /* Do prefork substitutions.
      *
      * Decide if we need "magic" handling of ~'s etc. in
diff --git a/Src/loop.c b/Src/loop.c
index 7c3e04b8a..61543ed73 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -569,23 +569,14 @@ execif(Estate state, int do_exec)
 	s = 1;
 	state->pc = next;
     }
+    noerrexit = olderrexit;
 
     if (run) {
-	/* we need to ignore lastval until we reach execcmd() */
-	if (olderrexit || run == 2)
-	    noerrexit = olderrexit;
-	else if (lastval)
-	    noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_UNTIL_EXEC;
-	else
-	    noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN);
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
 	execlist(state, 1, do_exec);
 	cmdpop();
-    } else {
-	noerrexit = olderrexit;
-	if (!retflag && !errflag)
-	    lastval = 0;
-    }
+    } else if (!retflag && !errflag)
+	lastval = 0;
     state->pc = end;
     this_noerrexit = 1;
 
diff --git a/Src/zsh.h b/Src/zsh.h
index 40f9ea537..703231ca2 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2220,8 +2220,6 @@ enum noerrexit_bits {
     NOERREXIT_EXIT = 1,
     /* Suppress ERR_RETURN: per function call */
     NOERREXIT_RETURN = 2,
-    /* NOERREXIT only needed on way down */
-    NOERREXIT_UNTIL_EXEC = 4,
     /* Force exit on SIGINT */
     NOERREXIT_SIGNAL = 8
 };