PATCH: skip command from debug trap

PATCH: skip command from debug trap

[Peter Stephenson]

I believe this is the fairly simple code needed to skip a command from a
debug trap.

The only difficult bit is the way of triggering it, since it's long
established that any non-zero number returned from a trap causes the
enclosing function to return.  I was inclined to pick a negative number,
but Rocky wants that for something else.  So how about 255, which is
fairly special?

If we're going to do this kind of stuff there seems little point in
trying to retain the old default behaviour of debug traps, where it
won't work.

Index: README
===================================================================
RCS file: /cvsroot/zsh/zsh/README,v
retrieving revision 1.53
diff -u -r1.53 README
--- README	1 Jun 2008 18:35:50 -0000	1.53
+++ README	5 Aug 2008 14:17:32 -0000
@@ -56,6 +56,12 @@
 applies to expressions with forced splitting such as ${=1+"$@"}, but
 otherwise the case where SH_WORD_SPLIT is not set is unaffected.
 
+Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
+before the command to which they refer instead of after.  This is almost
+always the right behaviour for the intended purpose of debugging and is
+consistent with recent versions of other shells.  The option
+DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
+
 In previous versions of the shell it was possible to use index 0 in an
 array or string subscript to refer to the same element as index 1 if the
 option KSH_ARRAYS was not in effect.  This was a limited approximation to
Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.108
diff -u -r1.108 builtins.yo
--- Doc/Zsh/builtins.yo	10 Jun 2008 08:50:36 -0000	1.108
+++ Doc/Zsh/builtins.yo	5 Aug 2008 14:17:33 -0000
@@ -1301,8 +1301,13 @@
 after each command with a nonzero exit status.  tt(ERR) is an alias
 for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
 usual case).
+
 If var(sig) is tt(DEBUG) then var(arg) will be executed
-after each command.
+before each command if the option tt(DEBUG_BEFORE_CMD) is set
+(as it is by default), else after each command.  In the former case,
+executing a tt(return) with the value of 255 within the trap causes
+the command referred to to be skipped.
+
 If var(sig) is tt(0) or tt(EXIT)
 and the tt(trap) statement is executed inside the body of a function,
 then the command var(arg) is executed after the function completes.
Index: Doc/Zsh/func.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
retrieving revision 1.20
diff -u -r1.20 func.yo
--- Doc/Zsh/func.yo	30 Jul 2008 19:46:20 -0000	1.20
+++ Doc/Zsh/func.yo	5 Aug 2008 14:17:33 -0000
@@ -308,8 +308,10 @@
 )
 findex(TRAPDEBUG)
 item(tt(TRAPDEBUG))(
-Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
-is set, executed before each command instead.
+If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
+before each command; otherwise executed after each command.  In the
+former case, returning the value 255 from the trap function causes
+execution of the command to be skipped.
 )
 findex(TRAPEXIT)
 item(tt(TRAPEXIT))(
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.61
diff -u -r1.61 options.yo
--- Doc/Zsh/options.yo	12 Jun 2008 13:45:05 -0000	1.61
+++ Doc/Zsh/options.yo	5 Aug 2008 14:17:33 -0000
@@ -1046,7 +1046,7 @@
 ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
 has an explicit list.
 )
-pindex(DEBUG_BEFORE_CMD)
+pindex(DEBUG_BEFORE_CMD <D>)
 cindex(traps, DEBUG, before or after command)
 cindex(DEBUG trap, before or after command)
 item(tt(DEBUG_BEFORE_CMD))(
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.136
diff -u -r1.136 exec.c
--- Src/exec.c	1 Aug 2008 13:53:44 -0000	1.136
+++ Src/exec.c	5 Aug 2008 14:17:33 -0000
@@ -1061,6 +1061,9 @@
 	}
 
 	if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
+	    int otrapskip = trapskip;
+	    trapskip = 0;
+
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
@@ -1071,7 +1074,8 @@
 	     * Only execute the trap once per sublist, even
 	     * if the DEBUGBEFORECMD option changes.
 	     */
-	    donedebug = 1;
+	    donedebug = trapskip ? 2 : 1;
+	    trapskip = otrapskip;
 	} else
 	    donedebug = 0;
 
@@ -1087,6 +1091,16 @@
 
 	/* Loop through code followed by &&, ||, or end of sublist. */
 	code = *state->pc++;
+	if (donedebug == 2) {
+	    /* Skip sublist. */
+	    while (wc_code(code) == WC_SUBLIST) {
+		state->pc = state->pc + WC_SUBLIST_SKIP(code);
+		code = *state->pc++;
+	    }
+	    donetrap = 1;
+	    /* yucky but consistent... */
+	    goto sublist_done;
+	}
 	while (wc_code(code) == WC_SUBLIST) {
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    if (!oldnoerrexit)
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.43
diff -u -r1.43 options.c
--- Src/options.c	31 Jul 2008 08:44:21 -0000	1.43
+++ Src/options.c	5 Aug 2008 14:17:33 -0000
@@ -112,7 +112,7 @@
 {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},	 CSHJUNKIEQUOTES},
 {{NULL, "cshnullcmd",	      OPT_EMULATE|OPT_CSH},	 CSHNULLCMD},
 {{NULL, "cshnullglob",	      OPT_EMULATE|OPT_CSH},	 CSHNULLGLOB},
-{{NULL, "debugbeforecmd",     OPT_EMULATE},		 DEBUGBEFORECMD},
+{{NULL, "debugbeforecmd",     OPT_ALL},			 DEBUGBEFORECMD},
 {{NULL, "emacs",	      0},			 EMACSMODE},
 {{NULL, "equals",	      OPT_EMULATE|OPT_ZSH},	 EQUALS},
 {{NULL, "errexit",	      OPT_EMULATE},		 ERREXIT},
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.48
diff -u -r1.48 signals.c
--- Src/signals.c	1 Aug 2008 13:53:45 -0000	1.48
+++ Src/signals.c	5 Aug 2008 14:17:34 -0000
@@ -1076,6 +1076,11 @@
 /**/
 int trapisfunc;
 
+/* Signal to list code that we should skip the next statement. */
+
+/**/
+int trapskip;
+
 /**/
 void
 dotrapargs(int sig, int *sigtr, void *sigfn)
@@ -1188,7 +1193,9 @@
     execrestore();
     lexrestore();
 
-    if (trapret > 0) {
+    trapskip = (trapret == 255 && sig == SIGDEBUG);
+
+    if (trapret > 0 && !trapskip) {
 	if (isfunc) {
 	    breaks = loops;
 	    errflag = 1;



-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH: skip command from debug trap

[Stephane Chazelas]

On Tue, Aug 05, 2008 at 03:27:18PM +0100, Peter Stephenson wrote:
> I believe this is the fairly simple code needed to skip a command from a
> debug trap.
> 
> The only difficult bit is the way of triggering it, since it's long
> established that any non-zero number returned from a trap causes the
> enclosing function to return.  I was inclined to pick a negative number,
> but Rocky wants that for something else.  So how about 255, which is
> fairly special?
[...]

How about a special variable containing the code to be run
available from the trap and that can be set to something else
within the trap.

Or how about using some form of exception raising?

Also, I don't know if it's been mentionned before, put to have a
DEBUG trap before every command, it seems that we should be able
to use PS4 with set -x, though there may not be any easy way
_not_ to start a subshell:

PS4='$(debug_function)'

-- 
Stéphane

Re: PATCH: skip command from debug trap

[Rocky Bernstein]

First and foremost, thanks for the change.

When I first read this I was eager to try it out. So I rushed to apply
the patch and put in a "skip" command. When this didn't work I then
reread - there is more unfinished work?

I'm not fussy about the number used. I don't understand why 255 is any
more special than say 4366 as a return from the TRAP instruction, but
255 is okay.

And if one  wants to use a negative number I don't see why that can't
be accommodated for either. For example if -1 is used, then -2 could
mean skip one return level and -3 two levels. Okay, maybe not such a
good idea if this is to be useful and easy for someone to remember.
However if the number where a high negative number, it probably can't
get confused with a return level because you will probably run out of
stack first. Recent ksh barfs on more than 2048 return levels (RELEASE
file 08-06-17 entry).

As for using PS4 to call a trace function, sure it's possible to do
things this way, but I not see what the merit is. Especially when
there is a more straightforward and conventional way to do things,
i.e. a command call such as trap.


On Tue, Aug 5, 2008 at 10:27 AM, Peter Stephenson <pws@csr.com> wrote:
> I believe this is the fairly simple code needed to skip a command from a
> debug trap.
>
> The only difficult bit is the way of triggering it, since it's long
> established that any non-zero number returned from a trap causes the
> enclosing function to return.  I was inclined to pick a negative number,
> but Rocky wants that for something else.  So how about 255, which is
> fairly special?
>
> If we're going to do this kind of stuff there seems little point in
> trying to retain the old default behaviour of debug traps, where it
> won't work.
>
> Index: README
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/README,v
> retrieving revision 1.53
> diff -u -r1.53 README
> --- README      1 Jun 2008 18:35:50 -0000       1.53
> +++ README      5 Aug 2008 14:17:32 -0000
> @@ -56,6 +56,12 @@
>  applies to expressions with forced splitting such as ${=1+"$@"}, but
>  otherwise the case where SH_WORD_SPLIT is not set is unaffected.
>
> +Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
> +before the command to which they refer instead of after.  This is almost
> +always the right behaviour for the intended purpose of debugging and is
> +consistent with recent versions of other shells.  The option
> +DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
> +
>  In previous versions of the shell it was possible to use index 0 in an
>  array or string subscript to refer to the same element as index 1 if the
>  option KSH_ARRAYS was not in effect.  This was a limited approximation to
> Index: Doc/Zsh/builtins.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
> retrieving revision 1.108
> diff -u -r1.108 builtins.yo
> --- Doc/Zsh/builtins.yo 10 Jun 2008 08:50:36 -0000      1.108
> +++ Doc/Zsh/builtins.yo 5 Aug 2008 14:17:33 -0000
> @@ -1301,8 +1301,13 @@
>  after each command with a nonzero exit status.  tt(ERR) is an alias
>  for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
>  usual case).
> +
>  If var(sig) is tt(DEBUG) then var(arg) will be executed
> -after each command.
> +before each command if the option tt(DEBUG_BEFORE_CMD) is set
> +(as it is by default), else after each command.  In the former case,
> +executing a tt(return) with the value of 255 within the trap causes
> +the command referred to to be skipped.
> +
>  If var(sig) is tt(0) or tt(EXIT)
>  and the tt(trap) statement is executed inside the body of a function,
>  then the command var(arg) is executed after the function completes.
> Index: Doc/Zsh/func.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
> retrieving revision 1.20
> diff -u -r1.20 func.yo
> --- Doc/Zsh/func.yo     30 Jul 2008 19:46:20 -0000      1.20
> +++ Doc/Zsh/func.yo     5 Aug 2008 14:17:33 -0000
> @@ -308,8 +308,10 @@
>  )
>  findex(TRAPDEBUG)
>  item(tt(TRAPDEBUG))(
> -Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
> -is set, executed before each command instead.
> +If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
> +before each command; otherwise executed after each command.  In the
> +former case, returning the value 255 from the trap function causes
> +execution of the command to be skipped.
>  )
>  findex(TRAPEXIT)
>  item(tt(TRAPEXIT))(
> Index: Doc/Zsh/options.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
> retrieving revision 1.61
> diff -u -r1.61 options.yo
> --- Doc/Zsh/options.yo  12 Jun 2008 13:45:05 -0000      1.61
> +++ Doc/Zsh/options.yo  5 Aug 2008 14:17:33 -0000
> @@ -1046,7 +1046,7 @@
>  ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
>  has an explicit list.
>  )
> -pindex(DEBUG_BEFORE_CMD)
> +pindex(DEBUG_BEFORE_CMD <D>)
>  cindex(traps, DEBUG, before or after command)
>  cindex(DEBUG trap, before or after command)
>  item(tt(DEBUG_BEFORE_CMD))(
> Index: Src/exec.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
> retrieving revision 1.136
> diff -u -r1.136 exec.c
> --- Src/exec.c  1 Aug 2008 13:53:44 -0000       1.136
> +++ Src/exec.c  5 Aug 2008 14:17:33 -0000
> @@ -1061,6 +1061,9 @@
>        }
>
>        if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
> +           int otrapskip = trapskip;
> +           trapskip = 0;
> +
>            exiting = donetrap;
>            ret = lastval;
>            dotrap(SIGDEBUG);
> @@ -1071,7 +1074,8 @@
>             * Only execute the trap once per sublist, even
>             * if the DEBUGBEFORECMD option changes.
>             */
> -           donedebug = 1;
> +           donedebug = trapskip ? 2 : 1;
> +           trapskip = otrapskip;
>        } else
>            donedebug = 0;
>
> @@ -1087,6 +1091,16 @@
>
>        /* Loop through code followed by &&, ||, or end of sublist. */
>        code = *state->pc++;
> +       if (donedebug == 2) {
> +           /* Skip sublist. */
> +           while (wc_code(code) == WC_SUBLIST) {
> +               state->pc = state->pc + WC_SUBLIST_SKIP(code);
> +               code = *state->pc++;
> +           }
> +           donetrap = 1;
> +           /* yucky but consistent... */
> +           goto sublist_done;
> +       }
>        while (wc_code(code) == WC_SUBLIST) {
>            next = state->pc + WC_SUBLIST_SKIP(code);
>            if (!oldnoerrexit)
> Index: Src/options.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/options.c,v
> retrieving revision 1.43
> diff -u -r1.43 options.c
> --- Src/options.c       31 Jul 2008 08:44:21 -0000      1.43
> +++ Src/options.c       5 Aug 2008 14:17:33 -0000
> @@ -112,7 +112,7 @@
>  {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},     CSHJUNKIEQUOTES},
>  {{NULL, "cshnullcmd",        OPT_EMULATE|OPT_CSH},      CSHNULLCMD},
>  {{NULL, "cshnullglob",       OPT_EMULATE|OPT_CSH},      CSHNULLGLOB},
> -{{NULL, "debugbeforecmd",     OPT_EMULATE},             DEBUGBEFORECMD},
> +{{NULL, "debugbeforecmd",     OPT_ALL},                         DEBUGBEFORECMD},
>  {{NULL, "emacs",             0},                        EMACSMODE},
>  {{NULL, "equals",            OPT_EMULATE|OPT_ZSH},      EQUALS},
>  {{NULL, "errexit",           OPT_EMULATE},              ERREXIT},
> Index: Src/signals.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
> retrieving revision 1.48
> diff -u -r1.48 signals.c
> --- Src/signals.c       1 Aug 2008 13:53:45 -0000       1.48
> +++ Src/signals.c       5 Aug 2008 14:17:34 -0000
> @@ -1076,6 +1076,11 @@
>  /**/
>  int trapisfunc;
>
> +/* Signal to list code that we should skip the next statement. */
> +
> +/**/
> +int trapskip;
> +
>  /**/
>  void
>  dotrapargs(int sig, int *sigtr, void *sigfn)
> @@ -1188,7 +1193,9 @@
>     execrestore();
>     lexrestore();
>
> -    if (trapret > 0) {
> +    trapskip = (trapret == 255 && sig == SIGDEBUG);
> +
> +    if (trapret > 0 && !trapskip) {
>        if (isfunc) {
>            breaks = loops;
>            errflag = 1;
>
>
>
> --
> Peter Stephenson <pws@csr.com>                  Software Engineer
> CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
> Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070
>

Re: PATCH: skip command from debug trap

[Peter Stephenson]

On Tue, 5 Aug 2008 19:47:12 -0400
"Rocky Bernstein" <rocky.bernstein@gmail.com> wrote:
> First and foremost, thanks for the change.
> 
> When I first read this I was eager to try it out. So I rushed to apply
> the patch and put in a "skip" command. When this didn't work I then
> reread - there is more unfinished work?

Yes.  There are about four cases: in a script, in a function, debugging
trap is inline, debugging trap is a function.  This now covers all four
cases.  (I'll need to add tests for all.)

I've now got the following to work as a script and a function:

trap '[[ $LINENO = 3 ]] && return 255' DEBUG; print $$; sleep 5
print $LINENO two
print $LINENO three
print $LINENO four

prints "2 two" and "4 four".  It also works if the top line is replaced
with

integer l=0; TRAPDEBUG() { (( l++ == 3 )) && return 255 }; print $$; sleep 5

(it's documented that LINENO in a trap function applies to the function
itself, as with any other function).

> I'm not fussy about the number used. I don't understand why 255 is any
> more special than say 4366 as a return from the TRAP instruction, but
> 255 is okay.
> 
> And if one  wants to use a negative number I don't see why that can't
> be accommodated for either.

An option to "return" is quite tempting.  This separates out this behaviour
from any other.  It removes any reliance on obscure numerology. It makes it
quite explicit this particular mechanism is in use.  Currently return
doesn't take options at all, so the result is also completely compatible
with existing versions.  (In fact, the lack of option parsing, even --, in
return is strictly a bug, so this even makes it more compatible with other
shells.)

Stephane's suggestions don't really work:  we don't have an exception
framework, and at this level we are dealing in parsed command structures,
not text.


Index: README
===================================================================
RCS file: /cvsroot/zsh/zsh/README,v
retrieving revision 1.53
diff -u -r1.53 README
--- README	1 Jun 2008 18:35:50 -0000	1.53
+++ README	6 Aug 2008 09:43:20 -0000
@@ -56,6 +56,12 @@
 applies to expressions with forced splitting such as ${=1+"$@"}, but
 otherwise the case where SH_WORD_SPLIT is not set is unaffected.
 
+Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
+before the command to which they refer instead of after.  This is almost
+always the right behaviour for the intended purpose of debugging and is
+consistent with recent versions of other shells.  The option
+DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
+
 In previous versions of the shell it was possible to use index 0 in an
 array or string subscript to refer to the same element as index 1 if the
 option KSH_ARRAYS was not in effect.  This was a limited approximation to
Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.108
diff -u -r1.108 builtins.yo
--- Doc/Zsh/builtins.yo	10 Jun 2008 08:50:36 -0000	1.108
+++ Doc/Zsh/builtins.yo	6 Aug 2008 09:43:21 -0000
@@ -1301,8 +1301,13 @@
 after each command with a nonzero exit status.  tt(ERR) is an alias
 for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
 usual case).
+
 If var(sig) is tt(DEBUG) then var(arg) will be executed
-after each command.
+before each command if the option tt(DEBUG_BEFORE_CMD) is set
+(as it is by default), else after each command.  In the former case,
+executing a tt(return) with the value of 255 within the trap causes
+the command referred to to be skipped.
+
 If var(sig) is tt(0) or tt(EXIT)
 and the tt(trap) statement is executed inside the body of a function,
 then the command var(arg) is executed after the function completes.
Index: Doc/Zsh/func.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
retrieving revision 1.20
diff -u -r1.20 func.yo
--- Doc/Zsh/func.yo	30 Jul 2008 19:46:20 -0000	1.20
+++ Doc/Zsh/func.yo	6 Aug 2008 09:43:21 -0000
@@ -308,8 +308,10 @@
 )
 findex(TRAPDEBUG)
 item(tt(TRAPDEBUG))(
-Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
-is set, executed before each command instead.
+If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
+before each command; otherwise executed after each command.  In the
+former case, returning the value 255 from the trap function causes
+execution of the command to be skipped.
 )
 findex(TRAPEXIT)
 item(tt(TRAPEXIT))(
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.61
diff -u -r1.61 options.yo
--- Doc/Zsh/options.yo	12 Jun 2008 13:45:05 -0000	1.61
+++ Doc/Zsh/options.yo	6 Aug 2008 09:43:21 -0000
@@ -1046,7 +1046,7 @@
 ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
 has an explicit list.
 )
-pindex(DEBUG_BEFORE_CMD)
+pindex(DEBUG_BEFORE_CMD <D>)
 cindex(traps, DEBUG, before or after command)
 cindex(DEBUG trap, before or after command)
 item(tt(DEBUG_BEFORE_CMD))(
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.200
diff -u -r1.200 builtin.c
--- Src/builtin.c	31 Jul 2008 08:44:20 -0000	1.200
+++ Src/builtin.c	6 Aug 2008 09:43:22 -0000
@@ -4458,6 +4458,16 @@
 	breaks = nump ? minimum(num,loops) : 1;
 	break;
     case BIN_RETURN:
+	if (trapreturn == -2 && num == 255) {
+	    /* skip a single command */
+	    trapreturn = lastval = num;
+	    if (trapisfunc) {
+		/* If trap is a function, return from that first */
+		retflag = 1;
+		breaks = loops;
+	    }
+	    return lastval;
+	}
 	if (isset(INTERACTIVE) || locallevel || sourcelevel) {
 	    retflag = 1;
 	    breaks = loops;
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.136
diff -u -r1.136 exec.c
--- Src/exec.c	1 Aug 2008 13:53:44 -0000	1.136
+++ Src/exec.c	6 Aug 2008 09:43:22 -0000
@@ -1061,6 +1061,9 @@
 	}
 
 	if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
+	    int otrapskip = trapskip;
+	    trapskip = 0;
+
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
@@ -1071,7 +1074,8 @@
 	     * Only execute the trap once per sublist, even
 	     * if the DEBUGBEFORECMD option changes.
 	     */
-	    donedebug = 1;
+	    donedebug = trapskip ? 2 : 1;
+	    trapskip = otrapskip;
 	} else
 	    donedebug = 0;
 
@@ -1087,6 +1091,18 @@
 
 	/* Loop through code followed by &&, ||, or end of sublist. */
 	code = *state->pc++;
+	if (donedebug == 2) {
+	    /* Skip sublist. */
+	    while (wc_code(code) == WC_SUBLIST) {
+		state->pc = state->pc + WC_SUBLIST_SKIP(code);
+		if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END)
+		    break;
+		code = *state->pc++;
+	    }
+	    donetrap = 1;
+	    /* yucky but consistent... */
+	    goto sublist_done;
+	}
 	while (wc_code(code) == WC_SUBLIST) {
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    if (!oldnoerrexit)
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.43
diff -u -r1.43 options.c
--- Src/options.c	31 Jul 2008 08:44:21 -0000	1.43
+++ Src/options.c	6 Aug 2008 09:43:22 -0000
@@ -112,7 +112,7 @@
 {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},	 CSHJUNKIEQUOTES},
 {{NULL, "cshnullcmd",	      OPT_EMULATE|OPT_CSH},	 CSHNULLCMD},
 {{NULL, "cshnullglob",	      OPT_EMULATE|OPT_CSH},	 CSHNULLGLOB},
-{{NULL, "debugbeforecmd",     OPT_EMULATE},		 DEBUGBEFORECMD},
+{{NULL, "debugbeforecmd",     OPT_ALL},			 DEBUGBEFORECMD},
 {{NULL, "emacs",	      0},			 EMACSMODE},
 {{NULL, "equals",	      OPT_EMULATE|OPT_ZSH},	 EQUALS},
 {{NULL, "errexit",	      OPT_EMULATE},		 ERREXIT},
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.48
diff -u -r1.48 signals.c
--- Src/signals.c	1 Aug 2008 13:53:45 -0000	1.48
+++ Src/signals.c	6 Aug 2008 09:43:22 -0000
@@ -1076,6 +1076,11 @@
 /**/
 int trapisfunc;
 
+/* Signal to list code that we should skip the next statement. */
+
+/**/
+int trapskip;
+
 /**/
 void
 dotrapargs(int sig, int *sigtr, void *sigfn)
@@ -1165,7 +1170,9 @@
     }
     runhookdef(AFTERTRAPHOOK, NULL);
 
-    if (trapreturn > 0 && isfunc) {
+    if (trapreturn == 255 && sig == SIGDEBUG) {
+	trapskip = 1;
+    } else if (trapreturn > 0 && isfunc) {
 	/*
 	 * Context was its own function.  We propagate the return
 	 * value specially.  Return value zero means don't do
@@ -1188,7 +1195,7 @@
     execrestore();
     lexrestore();
 
-    if (trapret > 0) {
+    if (!trapskip && trapret > 0) {
 	if (isfunc) {
 	    breaks = loops;
 	    errflag = 1;

Re: PATCH: skip command from debug trap

[Bart Schaefer]

On Aug 6, 10:47am, Peter Stephenson wrote:
}
} An option to "return" is quite tempting.  This separates out this behaviour
} from any other.  It removes any reliance on obscure numerology. It makes it
} quite explicit this particular mechanism is in use.  Currently return
} doesn't take options at all, so the result is also completely compatible
} with existing versions.

Umm, the argument to return is interpreted as a math expression.  So for
example

	x=-3
	return -x

returns "3", and

	x=3
	return --x

returns "2".  How are you going to make any kind of option parsing for
return behave compatibly with that? 

} (In fact, the lack of option parsing, even --, in return is strictly a
} bug, so this even makes it more compatible with other shells.)

It'd have to be only in emulation mode, then.

Re: PATCH: skip command from debug trap

[Rocky Bernstein]

On Wed, Aug 6, 2008 at 10:22 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Aug 6, 10:47am, Peter Stephenson wrote:
> }
> } An option to "return" is quite tempting.  This separates out this behaviour
> } from any other.  It removes any reliance on obscure numerology. It makes it
> } quite explicit this particular mechanism is in use.  Currently return
> } doesn't take options at all, so the result is also completely compatible
> } with existing versions.
>
> Umm, the argument to return is interpreted as a math expression.  So for
> example
>
>        x=-3
>        return -x
>
> returns "3", and
>
>        x=3
>        return --x
>
> returns "2".  How are you going to make any kind of option parsing for
> return behave compatibly with that?

How about "return $((x))", or "return $((-x))"?

Re: PATCH: skip command from debug trap

[Stephane Chazelas]

On Wed, Aug 06, 2008 at 07:22:36AM -0700, Bart Schaefer wrote:
[...]
> 	x=3
> 	return --x
> 
> returns "2".  How are you going to make any kind of option parsing for
> return behave compatibly with that? 

See for instance kill(1). To kill a process group, you have to
do:

kill -- -<pid>

So you'd have to do

return -- -3

> } (In fact, the lack of option parsing, even --, in return is strictly a
> } bug, so this even makes it more compatible with other shells.)
> 
> It'd have to be only in emulation mode, then.

-- is not a valid math expression, so changing return -- so that
it is the same as "return" instead of giving an error message
shouldn't be a problem.

Changing return -1 so that it outputs an error message instead
of returning with $? == -1 could break existing scripts.

BTW, is this:

$ zsh -c '(){echo test;return 1}; echo $?'
test
0

the expected output?

-- 
Stéphane

Re: PATCH: skip command from debug trap

[Peter Stephenson]

On Wed, 6 Aug 2008 15:59:17 +0100
Stephane Chazelas <Stephane_Chazelas@yahoo.fr> wrote:
> On Wed, Aug 06, 2008 at 07:22:36AM -0700, Bart Schaefer wrote:
> [...]
> > 	x=3
> > 	return --x
> > 
> > returns "2".  How are you going to make any kind of option parsing for
> > return behave compatibly with that? 

Grrrr.  All the control commands do that, and all for the sake of making
the command line less clear by omitting the $((...)).

However, return has never taken more than one argument up to now, so
there's nothing to stop us only evaluating the final argument
mathematically and require "return -s 0" to skip a statement and behave as
if it returned 0.  We have a choice of rule: (i) with more than one
argument, option and argument parsing becomes like other commands (so you
need a -- if there's possibly a negative expression at the end but
otherwise if it doesn't look like an option you still get math processing)
(ii) with more than one argument, the last argument is always a value.  I
think (i) is probably slightly clearer.

"return --" has always been an error; I don't see why it shouldn't be made
to do what it does in other shells in all modes.  Generally we've taken the
attitude that anything previously an error is fair game for use as
extensions.

> BTW, is this:
> 
> $ zsh -c '(){echo test;return 1}; echo $?'
> test
> 0
> 
> the expected output?

That's a bug.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.136
diff -u -r1.136 exec.c
--- Src/exec.c	1 Aug 2008 13:53:44 -0000	1.136
+++ Src/exec.c	6 Aug 2008 15:18:27 -0000
@@ -3870,7 +3870,7 @@
 {
     Shfunc shf;
     char *s = NULL;
-    int signum, nprg, sbeg, nstrs, npats, len, plen, i, htok = 0;
+    int signum, nprg, sbeg, nstrs, npats, len, plen, i, htok = 0, ret = 0;
     Wordcode beg = state->pc, end;
     Eprog prog;
     Patprog *pp;
@@ -3941,6 +3941,7 @@
 	    addlinknode(args, shf->node.nam);
 
 	    execshfunc(shf, args);
+	    ret = lastval;
 	    break;
 	} else {
 	    /* is this shell function a signal trap? */
@@ -3963,7 +3964,7 @@
 	}
     }
     state->pc = end;
-    return 0;
+    return ret;
 }
 
 /* Main entry point to execute a shell function. */


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH: skip command from debug trap

[Rocky Bernstein]

On Wed, Aug 6, 2008 at 5:47 AM, Peter Stephenson <pws@csr.com> wrote:
> On Tue, 5 Aug 2008 19:47:12 -0400
> "Rocky Bernstein" <rocky.bernstein@gmail.com> wrote:
>> First and foremost, thanks for the change.
>>
>> When I first read this I was eager to try it out. So I rushed to apply
>> the patch and put in a "skip" command. When this didn't work I then
>> reread - there is more unfinished work?
>
> Yes.  There are about four cases: in a script, in a function, debugging
> trap is inline, debugging trap is a function.  This now covers all four
> cases.  (I'll need to add tests for all.)
>
> I've now got the following to work as a script and a function:
>
> trap '[[ $LINENO = 3 ]] && return 255' DEBUG; print $$; sleep 5
> print $LINENO two
> print $LINENO three
> print $LINENO four
>
> prints "2 two" and "4 four".  It also works if the top line is replaced
> with
>
> integer l=0; TRAPDEBUG() { (( l++ == 3 )) && return 255 }; print $$; sleep 5
>
> (it's documented that LINENO in a trap function applies to the function
> itself, as with any other function).
>
>> I'm not fussy about the number used. I don't understand why 255 is any
>> more special than say 4366 as a return from the TRAP instruction, but
>> 255 is okay.
>>
>> And if one  wants to use a negative number I don't see why that can't
>> be accommodated for either.
>
> An option to "return" is quite tempting.  This separates out this behaviour
> from any other.  It removes any reliance on obscure numerology.

Others have noted the challenges in adding an option to return. Making
the semantics of the return statement already more complicated doesn't
seem wise.

A couple other approaches are setting a variable or calling a routine.
 For example  "trap_return --skip"  or TRAP_RETURN="skip"

> It makes it
> quite explicit this particular mechanism is in use.  Currently return
> doesn't take options at all, so the result is also completely compatible
> with existing versions.  (In fact, the lack of option parsing, even --, in
> return is strictly a bug, so this even makes it more compatible with other
> shells.)
>
> Stephane's suggestions don't really work:  we don't have an exception
> framework, and at this level we are dealing in parsed command structures,
> not text.
>
>
> Index: README
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/README,v
> retrieving revision 1.53
> diff -u -r1.53 README
> --- README      1 Jun 2008 18:35:50 -0000       1.53
> +++ README      6 Aug 2008 09:43:20 -0000
> @@ -56,6 +56,12 @@
>  applies to expressions with forced splitting such as ${=1+"$@"}, but
>  otherwise the case where SH_WORD_SPLIT is not set is unaffected.
>
> +Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
> +before the command to which they refer instead of after.  This is almost
> +always the right behaviour for the intended purpose of debugging and is
> +consistent with recent versions of other shells.  The option
> +DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
> +
>  In previous versions of the shell it was possible to use index 0 in an
>  array or string subscript to refer to the same element as index 1 if the
>  option KSH_ARRAYS was not in effect.  This was a limited approximation to
> Index: Doc/Zsh/builtins.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
> retrieving revision 1.108
> diff -u -r1.108 builtins.yo
> --- Doc/Zsh/builtins.yo 10 Jun 2008 08:50:36 -0000      1.108
> +++ Doc/Zsh/builtins.yo 6 Aug 2008 09:43:21 -0000
> @@ -1301,8 +1301,13 @@
>  after each command with a nonzero exit status.  tt(ERR) is an alias
>  for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
>  usual case).
> +
>  If var(sig) is tt(DEBUG) then var(arg) will be executed
> -after each command.
> +before each command if the option tt(DEBUG_BEFORE_CMD) is set
> +(as it is by default), else after each command.  In the former case,
> +executing a tt(return) with the value of 255 within the trap causes
> +the command referred to to be skipped.
> +
>  If var(sig) is tt(0) or tt(EXIT)
>  and the tt(trap) statement is executed inside the body of a function,
>  then the command var(arg) is executed after the function completes.
> Index: Doc/Zsh/func.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
> retrieving revision 1.20
> diff -u -r1.20 func.yo
> --- Doc/Zsh/func.yo     30 Jul 2008 19:46:20 -0000      1.20
> +++ Doc/Zsh/func.yo     6 Aug 2008 09:43:21 -0000
> @@ -308,8 +308,10 @@
>  )
>  findex(TRAPDEBUG)
>  item(tt(TRAPDEBUG))(
> -Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
> -is set, executed before each command instead.
> +If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
> +before each command; otherwise executed after each command.  In the
> +former case, returning the value 255 from the trap function causes
> +execution of the command to be skipped.
>  )
>  findex(TRAPEXIT)
>  item(tt(TRAPEXIT))(
> Index: Doc/Zsh/options.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
> retrieving revision 1.61
> diff -u -r1.61 options.yo
> --- Doc/Zsh/options.yo  12 Jun 2008 13:45:05 -0000      1.61
> +++ Doc/Zsh/options.yo  6 Aug 2008 09:43:21 -0000
> @@ -1046,7 +1046,7 @@
>  ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
>  has an explicit list.
>  )
> -pindex(DEBUG_BEFORE_CMD)
> +pindex(DEBUG_BEFORE_CMD <D>)
>  cindex(traps, DEBUG, before or after command)
>  cindex(DEBUG trap, before or after command)
>  item(tt(DEBUG_BEFORE_CMD))(
> Index: Src/builtin.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
> retrieving revision 1.200
> diff -u -r1.200 builtin.c
> --- Src/builtin.c       31 Jul 2008 08:44:20 -0000      1.200
> +++ Src/builtin.c       6 Aug 2008 09:43:22 -0000
> @@ -4458,6 +4458,16 @@
>        breaks = nump ? minimum(num,loops) : 1;
>        break;
>     case BIN_RETURN:
> +       if (trapreturn == -2 && num == 255) {
> +           /* skip a single command */
> +           trapreturn = lastval = num;
> +           if (trapisfunc) {
> +               /* If trap is a function, return from that first */
> +               retflag = 1;
> +               breaks = loops;
> +           }
> +           return lastval;
> +       }
>        if (isset(INTERACTIVE) || locallevel || sourcelevel) {
>            retflag = 1;
>            breaks = loops;
> Index: Src/exec.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
> retrieving revision 1.136
> diff -u -r1.136 exec.c
> --- Src/exec.c  1 Aug 2008 13:53:44 -0000       1.136
> +++ Src/exec.c  6 Aug 2008 09:43:22 -0000
> @@ -1061,6 +1061,9 @@
>        }
>
>        if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
> +           int otrapskip = trapskip;
> +           trapskip = 0;
> +
>            exiting = donetrap;
>            ret = lastval;
>            dotrap(SIGDEBUG);
> @@ -1071,7 +1074,8 @@
>             * Only execute the trap once per sublist, even
>             * if the DEBUGBEFORECMD option changes.
>             */
> -           donedebug = 1;
> +           donedebug = trapskip ? 2 : 1;
> +           trapskip = otrapskip;
>        } else
>            donedebug = 0;
>
> @@ -1087,6 +1091,18 @@
>
>        /* Loop through code followed by &&, ||, or end of sublist. */
>        code = *state->pc++;
> +       if (donedebug == 2) {
> +           /* Skip sublist. */
> +           while (wc_code(code) == WC_SUBLIST) {
> +               state->pc = state->pc + WC_SUBLIST_SKIP(code);
> +               if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END)
> +                   break;
> +               code = *state->pc++;
> +           }
> +           donetrap = 1;
> +           /* yucky but consistent... */
> +           goto sublist_done;
> +       }
>        while (wc_code(code) == WC_SUBLIST) {
>            next = state->pc + WC_SUBLIST_SKIP(code);
>            if (!oldnoerrexit)
> Index: Src/options.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/options.c,v
> retrieving revision 1.43
> diff -u -r1.43 options.c
> --- Src/options.c       31 Jul 2008 08:44:21 -0000      1.43
> +++ Src/options.c       6 Aug 2008 09:43:22 -0000
> @@ -112,7 +112,7 @@
>  {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},     CSHJUNKIEQUOTES},
>  {{NULL, "cshnullcmd",        OPT_EMULATE|OPT_CSH},      CSHNULLCMD},
>  {{NULL, "cshnullglob",       OPT_EMULATE|OPT_CSH},      CSHNULLGLOB},
> -{{NULL, "debugbeforecmd",     OPT_EMULATE},             DEBUGBEFORECMD},
> +{{NULL, "debugbeforecmd",     OPT_ALL},                         DEBUGBEFORECMD},
>  {{NULL, "emacs",             0},                        EMACSMODE},
>  {{NULL, "equals",            OPT_EMULATE|OPT_ZSH},      EQUALS},
>  {{NULL, "errexit",           OPT_EMULATE},              ERREXIT},
> Index: Src/signals.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
> retrieving revision 1.48
> diff -u -r1.48 signals.c
> --- Src/signals.c       1 Aug 2008 13:53:45 -0000       1.48
> +++ Src/signals.c       6 Aug 2008 09:43:22 -0000
> @@ -1076,6 +1076,11 @@
>  /**/
>  int trapisfunc;
>
> +/* Signal to list code that we should skip the next statement. */
> +
> +/**/
> +int trapskip;
> +
>  /**/
>  void
>  dotrapargs(int sig, int *sigtr, void *sigfn)
> @@ -1165,7 +1170,9 @@
>     }
>     runhookdef(AFTERTRAPHOOK, NULL);
>
> -    if (trapreturn > 0 && isfunc) {
> +    if (trapreturn == 255 && sig == SIGDEBUG) {
> +       trapskip = 1;
> +    } else if (trapreturn > 0 && isfunc) {
>        /*
>         * Context was its own function.  We propagate the return
>         * value specially.  Return value zero means don't do
> @@ -1188,7 +1195,7 @@
>     execrestore();
>     lexrestore();
>
> -    if (trapret > 0) {
> +    if (!trapskip && trapret > 0) {
>        if (isfunc) {
>            breaks = loops;
>            errflag = 1;
>
>

Re: PATCH: skip command from debug trap

[Peter Stephenson]

"Rocky Bernstein" wrote:
> Others have noted the challenges in adding an option to return. Making
> the semantics of the return statement already more complicated doesn't
> seem wise.

However, as I've pointed out I can guarantee to make that compatible
with the current shell, and at the same time make return work as in
other shells (apart from the math eval of the numeric argument, which
would cause an error in other shells anyway) which it doesn't at the
moment.  Once we have the latter adding an option is trivial.  Adding an
option to a builtin is about the simplest thing it's possible to do.

> A couple other approaches are setting a variable or calling a routine.
>  For example  "trap_return --skip"  or TRAP_RETURN="skip"

On the other hand, I can't make this compatible with existing versions
(the standard namespace pollution problem).

I don't like adding a new builtin just for this.

The variable version is doable, we've done similar things before.  You'd
have to note that it didn't force return from the current environment,
either the inline trap or TRAPDEBUG.  You'd also have to be prepared for
the shell to manipulate the variable behind your back, else you'd run
into problems with having it set on future traps.  It's not disastrous,
but I'm not convinced this is simpler.  In fact, at the moment it seems
to me manifestly much more complicated.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH: skip command from debug trap

[Rocky Bernstein]

On Wed, Aug 6, 2008 at 1:54 PM, Peter Stephenson <pws@csr.com> wrote:
> "Rocky Bernstein" wrote:
>> Others have noted the challenges in adding an option to return. Making
>> the semantics of the return statement already more complicated doesn't
>> seem wise.
>
> However, as I've pointed out I can guarantee to make that compatible
> with the current shell, and at the same time make return work as in
> other shells (apart from the math eval of the numeric argument, which
> would cause an error in other shells anyway) which it doesn't at the
> moment.  Once we have the latter adding an option is trivial.  Adding an
> option to a builtin is about the simplest thing it's possible to do.

Ok. Adding an option to the return builtin is simple, even if there is
no other shell that I know of that currently does this.

But given the choice of adding
   1) an option in the return statement everywhere that is specific to
just "trap DEBUG" or
   2) specifying what specific numbers do when on a return from "trap DEBUG",

isn't 2) simpler and more consistent with programming in shell
languages work? I take it as a given that every function or command is
going to have error codes that are somewhat arbitrary and that I'm not
going to intuit.

For example POSIX 1003.1 specifies that grep returns 1 when no lines
were selected and greater than 1 if there was an error, and for xargs
it says return code 126 means "The utility specified by *utility* was
found but could not be invoked. Seems a bit arbitrary and not
something I am likely to remember past my current need. But I think
someone working with shells is used to the idea that different
functions or commands return numbers indicate certain things.


>
>> A couple other approaches are setting a variable or calling a routine.
>>  For example  "trap_return --skip"  or TRAP_RETURN="skip"
>
> On the other hand, I can't make this compatible with existing versions
> (the standard namespace pollution problem).
>
> I don't like adding a new builtin just for this.
>
> The variable version is doable, we've done similar things before.  You'd
> have to note that it didn't force return from the current environment,
> either the inline trap or TRAPDEBUG.  You'd also have to be prepared for
> the shell to manipulate the variable behind your back, else you'd run
> into problems with having it set on future traps.  It's not disastrous,
> but I'm not convinced this is simpler.  In fact, at the moment it seems
> to me manifestly much more complicated.
>
> --
> Peter Stephenson <pws@csr.com>                  Software Engineer
> CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
> Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070
>

Re: PATCH: skip command from debug trap

[Peter Stephenson]

"Rocky Bernstein" wrote:
> But given the choice of adding
>    1) an option in the return statement everywhere that is specific to
> just "trap DEBUG" or
>    2) specifying what specific numbers do when on a return from "trap DEBUG",
> 
> isn't 2) simpler and more consistent with programming in shell
> languages work? I take it as a given that every function or command is
> going to have error codes that are somewhat arbitrary and that I'm not
> going to intuit.

I'm in two minds about this.  If we didn't have the existing rule that
any non-zero return status from TRAPDEBUG, or any explicit return from
within an inline trap, forced an immediate return, then I'd agree simply
adding to the set of useful statuses was cleaner and more natural.  As
it is, we're now forced to pick some numeric value the user wouldn't
naturally want to return from am enclosing context (since the return
value is propagated).

The option to return seems to me natural enough, because as "return"
means "just return", so "return with an option" means "return but with
slightly different semantics".  Having a different return status meaning
to execute different code (rather than simply provide a different test
for the caller, as normal) is an unusual enough effect that it doesn't
strike me as the unequivocal answer.

But, whatever.  If there's a number we all really like to mean "skip",
we can go with that.  It's already working, is easy to document, and
doesn't need any changes to parsing.

(By the way, it wouldn't be too hard, if not completely trivial, to pass
down the code about to be executed in a variable, say DEBUG_CMD_LINE, as
reconstructed text, i.e. the same sort of format as what you get if you
get the shell to output a shell function that's already loaded.  But
it's messy enough that I won't unless it's definitely useful.)

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH: skip command from debug trap

[Bart Schaefer]

Wow, longest thread in quite a while.

I'll preface this with a "gotcha" that I just noticed.  If you execute
these commands:

	TRAPDEBUG() { return 1 }
	setopt DEBUG_BEFORE_CMD IGNORE_EOF

You've just rendered your shell useless.  You can't even exit from it
(except by way of the ten-EOFs failsafe we put in some while ago).

On Aug 6, 10:30am, Rocky Bernstein wrote:
}
} >        x=3
} >        return --x
} >
} > returns "2".  How are you going to make any kind of option parsing for
} > return behave compatibly with that?
} 
} How about "return $((x))", or "return $((-x))"?

On Aug 6,  3:59pm, Stephane Chazelas wrote:
}
} See for instance kill(1). To kill a process group, you have to
} do:
} 
} kill -- -<pid>
} 
} So you'd have to do
} 
} return -- -3

You're both answering the inverse of the question I asked.  I *didn't*
ask "How do you make returning a math expression compatible with return
taking an option?"  The issue, as PWS understands, is that "return" is
already accepting math expressions, so introducing option parsing would
break that (unless done with some caveats).

} -- is not a valid math expression, so changing return -- so that
} it is the same as "return" instead of giving an error message
} shouldn't be a problem.

True, but not helpful all by itself.

On Aug 6,  4:34pm, Peter Stephenson wrote:
}
} Grrrr. All the control commands do that, and all for the sake of
} making the command line less clear by omitting the $((...)).

That's not really why, of course ... if you go back far enough, IIRC
you'll come to a version of the shell that has "return --x" but that
does not have $((...)).  Be that as it may ...

} However, return has never taken more than one argument up to now,
} so there's nothing to stop us only evaluating the final argument
} mathematically and require "return -s 0" to skip a statement and
} behave as if it returned 0.

I can live with that, if Rocky can live with the fact that to get the
debug trap to return the current value of $? and also skip the next
statement, one must do "return -s $?" or "return -s --".   Counting
arguments prevents any equivalence of "return" with no arguments and
"return" with only the skip option.

However, I have an alternate proposal (more on that below).

} We have a choice of rule: (i) with more than one argument, option and
} argument parsing becomes like other commands (so you need a -- if
} there's possibly a negative expression at the end but otherwise if it
} doesn't look like an option you still get math processing) (ii) with
} more than one argument, the last argument is always a value. I think
} (i) is probably slightly clearer.

Currently more than one argument is an error, so there shouldn't be any
way for an existing zsh script to run afoul of choice (i).

On Aug 6,  6:54pm, Peter Stephenson wrote:
}
} "Rocky Bernstein" wrote:
} > A couple other approaches are setting a variable or calling a routine.
} >  For example  "trap_return --skip"  or TRAP_RETURN="skip"
} 
} I don't like adding a new builtin just for this.
} 
} The variable version is doable, we've done similar things before.  You'd
} have to note that it didn't force return from the current environment,
} either the inline trap or TRAPDEBUG.  You'd also have to be prepared for
} the shell to manipulate the variable behind your back, else you'd run
} into problems with having it set on future traps.  It's not disastrous,
} but I'm not convinced this is simpler.  In fact, at the moment it seems
} to me manifestly much more complicated.

Here's another possibility.  Presently it's fairly useless to combine
"setopt ERR_EXIT" (set -x) with TRAPDEBUG, because any nonzero return
from the trap will cause the entire shell to exit, rather than just the
surrounding function.

I propose that ERR_EXIT be unset on entry to TRAPDEBUG, always.  Then at
return from the function, if ERR_EXIT has become set, treat that as an
indication to skip the command (and restore ERR_EXIT to whatever its
pre-function state was).  If you setopt ERR_EXIT and return non-zero
you still get what you always would (anyway, if you really wanted the
shell to exit, you can just call "exit" from the trap).  I can imagine
someone having done something arcane where they expect a failure of a
command inside a debug trap to kill their shell only when ERR_EXIT has
already been set globally, but it seems unlikely.

On Aug 6,  3:09pm, Rocky Bernstein wrote:
}
} But given the choice of adding
}    1) an option in the return statement everywhere that is specific to
} just "trap DEBUG" or
}    2) specifying what specific numbers do when on a return from "trap DEBUG",
} 
} isn't 2) simpler and more consistent with programming in shell
} languages work?

In other contexts return values above 128 are treated as indicative of
a caught signal.  If there were an imaginary signal that trips the DEBUG
trap, we could make return of $((128+SIGDEBUG)) mean something special.
But an out-of-band mechanism seems better.

I don't really have anything to say to PWS's most recent message on this.

Re: PATCH: skip command from debug trap

[Peter Stephenson]

On Wed, 06 Aug 2008 18:00:10 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Wow, longest thread in quite a while.
> 
> I'll preface this with a "gotcha" that I just noticed.  If you execute
> these commands:
> 
> 	TRAPDEBUG() { return 1 }
> 	setopt DEBUG_BEFORE_CMD IGNORE_EOF
> 
> You've just rendered your shell useless.  You can't even exit from it
> (except by way of the ten-EOFs failsafe we put in some while ago).

I suppose that's power vs. responsibility.

> Here's another possibility.  Presently it's fairly useless to combine
> "setopt ERR_EXIT" (set -x) with TRAPDEBUG, because any nonzero return
> from the trap will cause the entire shell to exit, rather than just the
> surrounding function.
> 
> I propose that ERR_EXIT be unset on entry to TRAPDEBUG, always.  Then at
> return from the function, if ERR_EXIT has become set, treat that as an
> indication to skip the command (and restore ERR_EXIT to whatever its
> pre-function state was).  If you setopt ERR_EXIT and return non-zero
> you still get what you always would (anyway, if you really wanted the
> shell to exit, you can just call "exit" from the trap).  I can imagine
> someone having done something arcane where they expect a failure of a
> command inside a debug trap to kill their shell only when ERR_EXIT has
> already been set globally, but it seems unlikely.

This seems quite neat, it even gets round the nastiness of working out
which level of the function call stack your at to fiddle with the return
value.  It seems perfectly reasonable to make non-use of ERR_EXIT within
DEBUG traps a documented feature:  we already do this during initialisation
scripts and can invoke the same mechanism trivially here.  That's not quite
what you said---you said let it behave as normal but with the additional
feature, but given we have the other mechanism to suppress its use, it
seems neater to apply that here when hijacking the option, right?

Here's the resulting code.  While looking at the other options I made the
trap_return stuff neater by adding a state variable and saving and
restoring properly in source() (the previous hack was what caused the
problems Rocky saw and this fixes the underlying issue much more neatly).

One minor point is that on digging further I found that trapreturn, now
trap_return, is already saved and restored by execsave() and execrestore()
so I didn't need to do that within the trap handler as I previously
claimed.

Index: README
===================================================================
RCS file: /cvsroot/zsh/zsh/README,v
retrieving revision 1.53
diff -u -r1.53 README
--- README	1 Jun 2008 18:35:50 -0000	1.53
+++ README	7 Aug 2008 10:07:08 -0000
@@ -56,6 +56,12 @@
 applies to expressions with forced splitting such as ${=1+"$@"}, but
 otherwise the case where SH_WORD_SPLIT is not set is unaffected.
 
+Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
+before the command to which they refer instead of after.  This is almost
+always the right behaviour for the intended purpose of debugging and is
+consistent with recent versions of other shells.  The option
+DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
+
 In previous versions of the shell it was possible to use index 0 in an
 array or string subscript to refer to the same element as index 1 if the
 option KSH_ARRAYS was not in effect.  This was a limited approximation to
Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.108
diff -u -r1.108 builtins.yo
--- Doc/Zsh/builtins.yo	10 Jun 2008 08:50:36 -0000	1.108
+++ Doc/Zsh/builtins.yo	7 Aug 2008 10:07:09 -0000
@@ -1301,8 +1301,15 @@
 after each command with a nonzero exit status.  tt(ERR) is an alias
 for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
 usual case).
+
 If var(sig) is tt(DEBUG) then var(arg) will be executed
-after each command.
+before each command if the option tt(DEBUG_BEFORE_CMD) is set
+(as it is by default), else after each command.  In the former
+case it is possible to skip the next command; see
+the description of the tt(ERR_EXIT) option in
+ifzman(zmanref(zshoptions))\
+ifnzman(noderef(Description of Options)).
+
 If var(sig) is tt(0) or tt(EXIT)
 and the tt(trap) statement is executed inside the body of a function,
 then the command var(arg) is executed after the function completes.
Index: Doc/Zsh/func.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
retrieving revision 1.20
diff -u -r1.20 func.yo
--- Doc/Zsh/func.yo	30 Jul 2008 19:46:20 -0000	1.20
+++ Doc/Zsh/func.yo	7 Aug 2008 10:07:09 -0000
@@ -308,8 +308,12 @@
 )
 findex(TRAPDEBUG)
 item(tt(TRAPDEBUG))(
-Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
-is set, executed before each command instead.
+If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
+before each command; otherwise executed after each command.  In the former
+case it is possible to skip the next command; see the description of the
+tt(ERR_EXIT) option in
+ifzman(zmanref(zshoptions))\
+ifnzman(noderef(Description of Options)).
 )
 findex(TRAPEXIT)
 item(tt(TRAPEXIT))(
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.61
diff -u -r1.61 options.yo
--- Doc/Zsh/options.yo	12 Jun 2008 13:45:05 -0000	1.61
+++ Doc/Zsh/options.yo	7 Aug 2008 10:07:09 -0000
@@ -1046,7 +1046,7 @@
 ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
 has an explicit list.
 )
-pindex(DEBUG_BEFORE_CMD)
+pindex(DEBUG_BEFORE_CMD <D>)
 cindex(traps, DEBUG, before or after command)
 cindex(DEBUG trap, before or after command)
 item(tt(DEBUG_BEFORE_CMD))(
@@ -1060,6 +1060,13 @@
 If a command has a non-zero exit status, execute the tt(ZERR)
 trap, if set, and exit.  This is disabled while running initialization
 scripts.
+
+The behaviour is also disabled inside tt(DEBUG) traps.  In this
+case the option is handled specially: it is unset on entry to
+the trap.  If the option tt(DEBUG_BEFORE_CMD) is set,
+as it is by default, and the option tt(ERR_EXIT) is found to have been set
+on exit, then the command for which the tt(DEBUG) trap is being executed is
+skipped.  The option is restored after the trap exits.
 )
 pindex(ERR_RETURN)
 cindex(function return, on error)
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.200
diff -u -r1.200 builtin.c
--- Src/builtin.c	31 Jul 2008 08:44:20 -0000	1.200
+++ Src/builtin.c	7 Aug 2008 10:07:10 -0000
@@ -4462,8 +4462,10 @@
 	    retflag = 1;
 	    breaks = loops;
 	    lastval = num;
-	    if (trapreturn == -2)
-		trapreturn = lastval;
+	    if (trap_state == TRAP_STATE_PRIMED && trap_return == -2) {
+		trap_state = TRAP_STATE_FORCE_RETURN;
+		trap_return = lastval;
+	    }
 	    return lastval;
 	}
 	zexit(num, 0);	/* else treat return as logout/exit */
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.136
diff -u -r1.136 exec.c
--- Src/exec.c	1 Aug 2008 13:53:44 -0000	1.136
+++ Src/exec.c	7 Aug 2008 10:07:10 -0000
@@ -65,16 +65,23 @@
 mod_export int errflag;
  
 /*
- * Status of return from a trap.
+ * State of trap return value.  Value is from enum trap_state.
+ */
+
+/**/
+int trap_state;
+
+/*
+ * Value associated with return from a trap.
  * This is only active if we are inside a trap, else its value
  * is irrelevant.  It is initialised to -1 for a function trap and
  * -2 for a non-function trap and if negative is decremented as
  * we go deeper into functions and incremented as we come back up.
  * The value is used to decide if an explicit "return" should cause
  * a return from the caller of the trap; it does this by setting
- * trapreturn to a status (i.e. a non-negative value).
+ * trap_return to a status (i.e. a non-negative value).
  *
- * In summary, trapreturn is
+ * In summary, trap_return is
  * - zero unless we are in a trap
  * - negative in a trap unless it has triggered.  Code uses this
  *   to detect an active trap.
@@ -83,7 +90,7 @@
  */
  
 /**/
-int trapreturn;
+int trap_return;
  
 /* != 0 if this is a subshell */
  
@@ -1061,6 +1068,10 @@
 	}
 
 	if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
+	    int oerrexit_opt = opts[ERREXIT];
+	    opts[ERREXIT] = 0;
+	    noerrexit = 1;
+
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
@@ -1071,7 +1082,8 @@
 	     * Only execute the trap once per sublist, even
 	     * if the DEBUGBEFORECMD option changes.
 	     */
-	    donedebug = 1;
+	    donedebug = isset(ERREXIT) ? 2 : 1;
+	    opts[ERREXIT] = oerrexit_opt;
 	} else
 	    donedebug = 0;
 
@@ -1087,6 +1099,18 @@
 
 	/* Loop through code followed by &&, ||, or end of sublist. */
 	code = *state->pc++;
+	if (donedebug == 2) {
+	    /* Skip sublist. */
+	    while (wc_code(code) == WC_SUBLIST) {
+		state->pc = state->pc + WC_SUBLIST_SKIP(code);
+		if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END)
+		    break;
+		code = *state->pc++;
+	    }
+	    donetrap = 1;
+	    /* yucky but consistent... */
+	    goto sublist_done;
+	}
 	while (wc_code(code) == WC_SUBLIST) {
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    if (!oldnoerrexit)
@@ -1177,12 +1201,20 @@
 	noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
+	    /*
+	     * Save and restore ERREXIT for consistency with
+	     * DEBUGBEFORECMD, even though it's not used.
+	     */
+	    int oerrexit_opt = opts[ERREXIT];
+	    opts[ERREXIT] = 0;
+	    noerrexit = 1;
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
 	    lastval = ret;
 	    donetrap = exiting;
 	    noerrexit = oldnoerrexit;
+	    opts[ERREXIT] = oerrexit_opt;
 	}
 
 	cmdsp = csp;
@@ -4144,8 +4176,8 @@
 
     oargv0 = NULL;
     obreaks = breaks;;
-    if (trapreturn < 0)
-	trapreturn--;
+    if (trap_state == TRAP_STATE_PRIMED)
+	trap_return--;
     oldlastval = lastval;
     oldnumpipestats = numpipestats;
     if (noreturnval) {
@@ -4263,8 +4295,8 @@
 
     endtrapscope();
 
-    if (trapreturn < -1)
-	trapreturn++;
+    if (trap_state == TRAP_STATE_PRIMED)
+	trap_return++;
     ret = lastval;
     if (noreturnval) {
 	lastval = oldlastval;
@@ -4527,7 +4559,9 @@
     es->badcshglob = badcshglob;
     es->cmdoutpid = cmdoutpid;
     es->cmdoutval = cmdoutval;
-    es->trapreturn = trapreturn;
+    es->trap_return = trap_return;
+    es->trap_state = trap_state;
+    es->trapisfunc = trapisfunc;
     es->noerrs = noerrs;
     es->subsh_close = subsh_close;
     es->underscore = ztrdup(underscore);
@@ -4554,7 +4588,9 @@
     badcshglob = exstack->badcshglob;
     cmdoutpid = exstack->cmdoutpid;
     cmdoutval = exstack->cmdoutval;
-    trapreturn = exstack->trapreturn;
+    trap_return = exstack->trap_return;
+    trap_state = exstack->trap_state;
+    trapisfunc = exstack->trapisfunc;
     noerrs = exstack->noerrs;
     subsh_close = exstack->subsh_close;
     setunderscore(exstack->underscore);
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.90
diff -u -r1.90 init.c
--- Src/init.c	4 Aug 2008 19:47:52 -0000	1.90
+++ Src/init.c	7 Aug 2008 10:07:10 -0000
@@ -191,10 +191,6 @@
 	    exit(lastval);
 	if (((!interact || sourcelevel) && errflag) || retflag)
 	    break;
-	if (intrap && trapreturn >= 0) {
-	    lastval = trapreturn;
-	    trapreturn = 0;
-	}
 	if (isset(SINGLECOMMAND) && toplevel) {
 	    if (sigtrapped[SIGEXIT])
 		dotrap(SIGEXIT);
@@ -880,7 +876,8 @@
     lastmailcheck = time(NULL);
     locallevel = sourcelevel = 0;
     sfcontext = SFC_NONE;
-    trapreturn = 0;
+    trap_return = 0;
+    trap_state = TRAP_STATE_INACTIVE;
     noerrexit = -1;
     nohistsave = 1;
     dirstack = znewlinklist();
@@ -1060,6 +1057,7 @@
     char *old_scriptname = scriptname, *us;
     unsigned char *ocs;
     int ocsp;
+    int otrap_return = trap_return, otrap_state = trap_state;
 
     if (!s || 
 	(!(prog = try_source_file((us = unmeta(s)))) &&
@@ -1090,6 +1088,13 @@
     dosetopt(SHINSTDIN, 0, 1);
     scriptname = s;
 
+    /*
+     * The special return behaviour of traps shouldn't
+     * trigger in files sourced from traps; the return
+     * is just a return from the file.
+     */
+    trap_state = TRAP_STATE_INACTIVE;
+
     sourcelevel++;
     if (prog) {
 	pushheap();
@@ -1100,6 +1105,9 @@
 	loop(0, 0);		     /* loop through the file to be sourced  */
     sourcelevel--;
 
+    trap_state = otrap_state;
+    trap_return = otrap_return;
+
     /* restore the current shell state */
     if (prog)
 	freeeprog(prog);
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.43
diff -u -r1.43 options.c
--- Src/options.c	31 Jul 2008 08:44:21 -0000	1.43
+++ Src/options.c	7 Aug 2008 10:07:10 -0000
@@ -112,7 +112,7 @@
 {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},	 CSHJUNKIEQUOTES},
 {{NULL, "cshnullcmd",	      OPT_EMULATE|OPT_CSH},	 CSHNULLCMD},
 {{NULL, "cshnullglob",	      OPT_EMULATE|OPT_CSH},	 CSHNULLGLOB},
-{{NULL, "debugbeforecmd",     OPT_EMULATE},		 DEBUGBEFORECMD},
+{{NULL, "debugbeforecmd",     OPT_ALL},			 DEBUGBEFORECMD},
 {{NULL, "emacs",	      0},			 EMACSMODE},
 {{NULL, "equals",	      OPT_EMULATE|OPT_ZSH},	 EQUALS},
 {{NULL, "errexit",	      OPT_EMULATE},		 ERREXIT},
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.48
diff -u -r1.48 signals.c
--- Src/signals.c	1 Aug 2008 13:53:45 -0000	1.48
+++ Src/signals.c	7 Aug 2008 10:07:10 -0000
@@ -1082,12 +1082,10 @@
 {
     LinkList args;
     char *name, num[4];
-    int trapret = 0;
     int obreaks = breaks;
     int oretflag = retflag;
-    int otrapreturn = trapreturn;
     int isfunc;
-    int traperr;
+    int traperr, new_trap_state, new_trap_return;
 
     /* if signal is being ignored or the trap function		      *
      * is NULL, then return					      *
@@ -1123,6 +1121,7 @@
     *sigtr |= ZSIG_IGNORED;
 
     lexsave();
+    /* execsave will save the old trap_return and trap_state */
     execsave();
     breaks = retflag = 0;
     runhookdef(BEFORETRAPHOOK, NULL);
@@ -1148,7 +1147,8 @@
 	sprintf(num, "%d", sig);
 	zaddlinknode(args, num);
 
-	trapreturn = -1;	/* incremented by doshfunc */
+	trap_return = -1;	/* incremented by doshfunc */
+	trap_state = TRAP_STATE_PRIMED;
 	trapisfunc = isfunc = 1;
 
 	sfcontext = SFC_SIGNAL;
@@ -1156,46 +1156,32 @@
 	sfcontext = osc;
 	freelinklist(args, (FreeFunc) NULL);
 	zsfree(name);
-
     } else {
-	trapreturn = -2;	/* not incremented, used at current level */
+	trap_return = -2;	/* not incremented, used at current level */
+	trap_state = TRAP_STATE_PRIMED;
 	trapisfunc = isfunc = 0;
 
 	execode(sigfn, 1, 0);
     }
     runhookdef(AFTERTRAPHOOK, NULL);
 
-    if (trapreturn > 0 && isfunc) {
-	/*
-	 * Context was its own function.  We propagate the return
-	 * value specially.  Return value zero means don't do
-	 * anything special, so don't handle it.
-	 */
-	trapret = trapreturn;
-    } else if (trapreturn >= 0 && !isfunc) {
-	/*
-	 * Context was an inline trap.  If an explicit return value
-	 * was used, we need to set `lastval'.  Otherwise we use the
-	 * value restored by execrestore.  In this case, all return
-	 * values indicate an explicit return from the current function,
-	 * so always handle specially.  trapreturn is always restored by
-	 * execrestore.
-	 */
-	trapret = trapreturn + 1;
-    }
     traperr = errflag;
-    trapreturn = otrapreturn;
+
+    /* Grab values before they are restored */
+    new_trap_state = trap_state;
+    new_trap_return = trap_return;
+
     execrestore();
     lexrestore();
 
-    if (trapret > 0) {
+    if (new_trap_state == TRAP_STATE_FORCE_RETURN &&
+	/* zero return from function isn't special */
+	!(isfunc && new_trap_return == 0)) {
 	if (isfunc) {
 	    breaks = loops;
 	    errflag = 1;
-	    lastval = trapret;
-	} else {
-	    lastval = trapret-1;
 	}
+	lastval = new_trap_return;
 	/* return triggered */
 	retflag = 1;
     } else {
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.139
diff -u -r1.139 zsh.h
--- Src/zsh.h	31 Jul 2008 08:44:21 -0000	1.139
+++ Src/zsh.h	7 Aug 2008 10:07:10 -0000
@@ -921,7 +921,9 @@
     int badcshglob;
     pid_t cmdoutpid;
     int cmdoutval;
-    int trapreturn;
+    int trap_return;
+    int trap_state;
+    int trapisfunc;
     int noerrs;
     int subsh_close;
     char *underscore;
@@ -2225,6 +2227,24 @@
 #define ZSIG_ALIAS	(1<<3)  /* Trap is stored under an alias */
 #define ZSIG_SHIFT	4
 
+/*
+ * State of traps, stored in trap_state.
+ */
+enum trap_state {
+    /* Traps are not active; trap_return is not useful. */
+    TRAP_STATE_INACTIVE,
+    /*
+     * Traps are set but haven't triggered; trap_return gives
+     * minus function depth.
+     */
+    TRAP_STATE_PRIMED,
+    /*
+     * Trap has triggered to force a return; trap_return givens
+     * return value.
+     */
+    TRAP_STATE_FORCE_RETURN
+};
+
 /***********/
 /* Sorting */
 /***********/
Index: Test/A05execution.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A05execution.ztst,v
retrieving revision 1.5
diff -u -r1.5 A05execution.ztst
--- Test/A05execution.ztst	8 Nov 2006 10:38:07 -0000	1.5
+++ Test/A05execution.ztst	7 Aug 2008 10:07:10 -0000
@@ -124,6 +124,7 @@
 0:TRAPEXIT
 >Exit
 
+  unsetopt DEBUG_BEFORE_CMD
   unfunction fn
   print 'TRAPDEBUG() {
       print Line $LINENO
@@ -138,6 +139,7 @@
 >Line 1
 >Line 1
 
+  unsetopt DEBUG_BEFORE_CMD
   unfunction fn
   print 'trap '\''print Line $LINENO'\'' DEBUG
     :
Index: Test/C03traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C03traps.ztst,v
retrieving revision 1.13
diff -u -r1.13 C03traps.ztst
--- Test/C03traps.ztst	6 Aug 2008 08:54:23 -0000	1.13
+++ Test/C03traps.ztst	7 Aug 2008 10:07:10 -0000
@@ -402,6 +402,19 @@
 0: trapreturn handling bug is properly fixed
 ?./zsh-trapreturn-bug2:5: no such file or directory: ./fdasfsdafd
 
+  fn() {
+    setopt localtraps localoptions debugbeforecmd
+    trap '(( LINENO == 4 )) && setopt errexit' DEBUG
+    print $LINENO three
+    print $LINENO four
+    print $LINENO five
+    [[ -o errexit ]] && print "Hey, ERREXIT is set!"
+  }
+  fn
+1:Skip line from DEBUG trap
+>3 three
+>5 five
+
 %clean
 
   rm -f TRAPEXIT

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH: skip command from debug trap

[Bart Schaefer]

On Aug 7, 11:11am, Peter Stephenson wrote:
} Subject: Re: PATCH: skip command from debug trap
}
} On Wed, 06 Aug 2008 18:00:10 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > 
} > 	TRAPDEBUG() { return 1 }
} > 	setopt DEBUG_BEFORE_CMD IGNORE_EOF
} > 
} > You've just rendered your shell useless.  You can't even exit from it
} > (except by way of the ten-EOFs failsafe we put in some while ago).
} 
} I suppose that's power vs. responsibility.

One point:  This becomes even easier to screw up if DEBUG_BEFORE_CMD is
the default behavior, and would be mystifying to a typical user.
 
} > I propose that ERR_EXIT be unset on entry to TRAPDEBUG, always.  Then at
} > return from the function, if ERR_EXIT has become set, treat that as an
} > indication to skip the command (and restore ERR_EXIT to whatever its
} > pre-function state was).

After sending this I thought "hmm, I could just as easily have suggested
a new option."  I guess it's a good thing I didn't.

} > If you setopt ERR_EXIT and return non-zero
} > you still get what you always would (anyway, if you really wanted the
} > shell to exit, you can just call "exit" from the trap).
} 
} This seems quite neat, it even gets round the nastiness of working out
} which level of the function call stack your at to fiddle with the return
} value.  It seems perfectly reasonable to make non-use of ERR_EXIT within
} DEBUG traps a documented feature:  we already do this during initialisation
} scripts and can invoke the same mechanism trivially here.  That's not quite
} what you said---you said let it behave as normal but with the additional
} feature, but given we have the other mechanism to suppress its use, it
} seems neater to apply that here when hijacking the option, right?

Yes; in fact when I first wrote it down I suggested exactly that, but
then noticed that it wasn't necessary to disable it entirely and backed
out that part.  If it's actually *easier* this way, the "you can just
call 'exit'" still applies.