Something rotten in tar completion

Something rotten in tar completion

[Peter Stephenson]

I've been seeing this for some time and haven't had a chance to look any
further, given which it's high time I reported it in any case in the
unlikely event someone's got some spare time.

If I try to complete after "tar xvzf ...", which is where I usually am
completing (I don't tend to bother completing inside tar files), it's
*incredibly* slow.  I suspect it's looking in the file, which it doesn't
need to at this point --- it's only completing the name of the file.
(It doesn't even know what file I'm using yet, in fact.)

Note this is a bog standard local disk.

What's even worse is I can't abort.  If I ^C it simply says "Killed after
41 seconds" but doesn't stop; if I do it repeatedly the time taken
increments but that's all.

It does finish eventually after a few minutes and give the right answer
it should have given in a small fraction of a second.  As far as I can
see there's no remaining sign I tried to abort it.

Haven't looked at whether my options and styles come into it, but will
if nobody else is seeing this (it's pretty darned obvious).

This is with the latest version, in case you hadn't guessed.

pws

Re: Something rotten in tar completion

[Bart Schaefer]

[Cc-ing PWS because mail I sent yesterday to zsh-users still hasn't
shown up there.]

On Dec 2,  3:54pm, Peter Stephenson wrote:
} Subject: Something rotten in tar completion
}
} If I try to complete after "tar xvzf ...", which is where I usually am
} completing (I don't tend to bother completing inside tar files), it's
} *incredibly* slow.  I suspect it's looking in the file, which it doesn't
} need to at this point --- it's only completing the name of the file.

Well, let's see ... _complete_debug with no zstyles at all ...

% tar xvzf <C-x ?>

It runs "tar --version" THREE TIMES to figure out which kind of tar and
what options it has.

Then:  _files -J -default- -g '*.((tar|TAR).(gz|GZ|Z)|tgz)(-.)'

... which invokes _path_files at least twice.  The result is a listing
of directory names and tar file names.

Tried the same thing with a partial file name on the line, got the same
results except directories were properly not completed.

But I don't see it examining the tarfile contents.  Looking at _tar, it
should only poke inside if it already has both a tarfile name and some
tar commands on the line.  Hmm, it IS legal to put a list of file names
to extract after "tar xf archive ..." -- it could be that it's trying
to populate that list because the "x" is present.

That does happen if I complete after "tar xvzf archive.tgz " but does
not if the cursor is still inside the word "archive.tgz".

At various places (_description, _path_files) it does look for the "fake",
"fake-always", and "fake-files" styles.  Could that have something to do
with it on your end?

Re: Something rotten in tar completion

[Peter Stephenson]

On Tue, 02 Dec 2014 08:48:58 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Well, let's see ... _complete_debug with no zstyles at all ...
> 
> % tar xvzf <C-x ?>

Oh, so now I've got to debug it.  Grmph...

> At various places (_description, _path_files) it does look for the "fake",
> "fake-always", and "fake-files" styles.  Could that have something to do
> with it on your end?

Turns out it was recursive-files.  I had that set for my "src"
subdirectory, which was a bit daft since it's got to search through any old
clutter to find a completion for a bare file name.  Removing that
removes the wait.

(I debugged this by adding SECONDS to _complete_debug as a float and
commenting out the thing in _main_complete that sets to 0 as an
integer.  I guess the fact no one else has needed to do this suggests
it's not a very command need.)

Would still be nice to be able to kill it...

pws

Re: Something rotten in tar completion

[Bart Schaefer]

On Dec 2,  5:26pm, Peter Stephenson wrote:
}
} Turns out it was recursive-files.  I had that set for my "src"
} subdirectory, which was a bit daft since it's got to search through any old
} clutter to find a completion for a bare file name.  Removing that
} removes the wait.
} 
} Would still be nice to be able to kill it...

In the most recent _main_complete, the message on the interrupt should
tell you where it was:

trap 'zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";

Getting that $funcstack name might be helpful.  In the meantime, it does
not seem useful to regenerate the recursive listing for every directory
in the recursive-files style:


diff --git a/Completion/Unix/Type/_files b/Completion/Unix/Type/_files
index 0f6fcd6..049b49c 100644
--- a/Completion/Unix/Type/_files
+++ b/Completion/Unix/Type/_files
@@ -121,9 +121,11 @@ for def in "$pats[@]"; do
           if _path_files -g "$pat" "$opts[@]" "$expl[@]"; then
 	    ret=0
 	  elif [[ $PREFIX$SUFFIX != */* ]] && zstyle -a ":completion:${curcontext}:$tag" recursive-files rfiles; then
+	    local subtree
+	    subtree=( **/*(/) )
 	    for rfile in $rfiles; do
 	      if [[ $PWD/ = ${~rfile} ]]; then
-		for prepath in **/*(/); do
+		for prepath in $subtree; do
 		  oprefix=$PREFIX
 		  PREFIX=$prepath/$PREFIX
 		  _path_files -g "$pat" "$opts[@]" "$expl[@]" && ret=0

-- 
Barton E. Schaefer

Re: Something rotten in tar completion

[Peter Stephenson]

(This mail wanted me to reply to

To: Zsh Hackers' List <zsh-workers@zsh.org.hsd1.ca.comcast.net>

and I got a message in Korean telling me the mail server wasn't happy.
Well, I think.  Not sure if that's a clue to other mail problems.  I
don't mean the Korean.)

On Thu, 04 Dec 2014 08:56:06 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Dec 2,  5:26pm, Peter Stephenson wrote:
> }
> } Turns out it was recursive-files.  I had that set for my "src"
> } subdirectory, which was a bit daft since it's got to search through any old
> } clutter to find a completion for a bare file name.  Removing that
> } removes the wait.
> } 
> } Would still be nice to be able to kill it...
> 
> In the most recent _main_complete, the message on the interrupt should
> tell you where it was:
> 
> trap 'zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
> 
> Getting that $funcstack name might be helpful.

It's always in _files or _path_files --- kind of where you'd expect from
the code you quoted.  I guess it's the glob that's not very
interruptible.

> In the meantime, it does
> not seem useful to regenerate the recursive listing for every directory
> in the recursive-files style:

Can I suggest only generating subtree if we have a matching file?

diff --git a/Completion/Unix/Type/_files b/Completion/Unix/Type/_files
index 0f6fcd6..e628cb3 100644
--- a/Completion/Unix/Type/_files
+++ b/Completion/Unix/Type/_files
@@ -121,9 +121,13 @@ for def in "$pats[@]"; do
           if _path_files -g "$pat" "$opts[@]" "$expl[@]"; then
 	    ret=0
 	  elif [[ $PREFIX$SUFFIX != */* ]] && zstyle -a
":completion:${curcontext}:$tag" recursive-files rfiles; then
+	    local subtree
 	    for rfile in $rfiles; do
 	      if [[ $PWD/ = ${~rfile} ]]; then
-		for prepath in **/*(/); do
+		if [[ -z $subtree ]]; then
+		  subtree=( **/*(/) )
+		fi
+		for prepath in $subtree; do
 		  oprefix=$PREFIX
 		  PREFIX=$prepath/$PREFIX
 		  _path_files -g "$pat" "$opts[@]" "$expl[@]" && ret=0

pws

Re: Something rotten in tar completion

[Bart Schaefer]

(Aside, I think I know WHAT is going on with my email not reaching the list,
now I just have to figure out WHY.)

On Dec 4,  8:56am, Bart Schaefer wrote:
}
} not seem useful to regenerate the recursive listing for every directory
} in the recursive-files style:
} 
} +	    local subtree
} +	    subtree=( **/*(/) )
}  	    for rfile in $rfiles; do
}  	      if [[ $PWD/ = ${~rfile} ]]; then
} -		for prepath in **/*(/); do
} +		for prepath in $subtree; do


I suppose the assumption is that $PWD/ will only match one of the rfiles
patterns, so this might actually be more work than before.  Ignore this
patch, sorry.

-- 
Barton E. Schaefer

Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 4,  5:12pm, Peter Stephenson wrote:
}
} It's always in _files or _path_files --- kind of where you'd expect from
} the code you quoted.  I guess it's the glob that's not very
} interruptible.

This seems to help:

diff --git a/Src/glob.c b/Src/glob.c
index ca7bc44..b3903f2 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -463,7 +463,7 @@ scanner(Complist q, int shortcircuit)
     int errssofar = errsfound;
     struct dirsav ds;
 
-    if (!q)
+    if (!q || errflag)
 	return;
     init_dirsav(&ds);
 

Re: Interrupting globs (Re: Something rotten in tar completion)

[Jérémie Roquet]

2014-12-05 9:20 GMT+01:00 Bart Schaefer <schaefer@brasslantern.com>:
> On Dec 4,  5:12pm, Peter Stephenson wrote:
> }
> } It's always in _files or _path_files --- kind of where you'd expect from
> } the code you quoted.  I guess it's the glob that's not very
> } interruptible.
>
> This seems to help:
>
> diff --git a/Src/glob.c b/Src/glob.c
> index ca7bc44..b3903f2 100644
> --- a/Src/glob.c
> +++ b/Src/glob.c
> @@ -463,7 +463,7 @@ scanner(Complist q, int shortcircuit)
>      int errssofar = errsfound;
>      struct dirsav ds;
>
> -    if (!q)
> +    if (!q || errflag)
>         return;
>      init_dirsav(&ds);

I've no Idea if the expansion of something like “ls /a/b/c<tab>” to
“ls /aaaaa/bbbbbb/cccccc/” is related to this code, but it seems to
have zero impact there: I'm still unable to interrupt the horribly
slow expansion using ctrl+c.

Before (zsh from a few weeks ago):
Killed by signal in  after 5s

After (zsh of right now + the above suggested modification):
Killed by signal in _path_files after 5s

…but I don't get my prompt back.

Best regards,

-- 
Jérémie

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 05 Dec 2014 00:20:23 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Dec 4,  5:12pm, Peter Stephenson wrote:
> }
> } It's always in _files or _path_files --- kind of where you'd expect from
> } the code you quoted.  I guess it's the glob that's not very
> } interruptible.
> 
> This seems to help:
> 
> diff --git a/Src/glob.c b/Src/glob.c
> index ca7bc44..b3903f2 100644
> --- a/Src/glob.c
> +++ b/Src/glob.c
> @@ -463,7 +463,7 @@ scanner(Complist q, int shortcircuit)
>      int errssofar = errsfound;
>      struct dirsav ds;
>  
> -    if (!q)
> +    if (!q || errflag)
>  	return;
>      init_dirsav(&ds);

Not for me: it's more basic than that.  The trap in _main_complete isn't
working *at all* --- or, rather, it's working too well, by trapping all
errors.

It's using an eval-style trap, so the "return" in it is forcing the
current function in the completion code to return with that status and
no error flagged.  That might have been good enough to interrupt a
simple case, but what's actually needed is returning non-zero from a
function-style trap, which causes the default signal handler to run ---
I should have spotted this at the time since I've had suspicions since
that trap turned up.

This actually prevents the message in the trap showing up --- we need
some cleverer zle trickery to do that --- and consequently the trap is
not actually any use as it stands.  But at least interruption is now
usable rather than unsuable.

diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
index fcd6366..765fb4b 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -128,8 +128,11 @@ _completer_num=1
 
 # We assume localtraps to be in effect here ...
 integer SECONDS=0
-trap 'zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
-      zle -R; return 130' INT QUIT
+TRPAINT TRAPQUIT() {
+  zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
+  zle -R
+  return 130
+}
 
 # Call the pre-functions.
 
pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Jérémie Roquet]

2014-12-05 15:50 GMT+01:00 Peter Stephenson <p.stephenson@samsung.com>:
> -trap 'zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
> -      zle -R; return 130' INT QUIT
> +TRPAINT TRAPQUIT() {
> +  zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
> +  zle -R
> +  return 130
> +}

It works well for the issue mentioned above (that is, “/a/b/c<tab>” →
“/aaaaaa/bbbbbb/cccccc” — now I can interrupt it). But it doesn't fix
the issue with “/aaaaaa/bbbbbb/cccccc/<tab>” in a (very) big
directory.

Before:
Killed by signal in _path_files after 9s

After:

(ie. : nothing)

…but I don't get my prompt back.

Best regards,

-- 
Jérémie

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 5 Dec 2014 16:04:13 +0100
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> You wrote:
> > +TRPAINT TRAPQUIT() {
> 
> Note the typo: AP transposed.

(by private mail...)

Hmm, that's interesting.

Fixing the TRAPINT puts back the ZLE info output by the trap.  However,
it doesn't fix the interrupts --- it's nothing like as bad as it was
with the wrong sort of trap, but I still need to ^C a few times to get
back to editing.  It might be once per completer.  (It's sort of vaguely
useful that I can ^C to get to the next completer but it's a bit
confusing and this obviously isn't the way it's designed.)

pws

-- 
Peter Stephenson <p.stephenson@samsung.com>  Principal Software Engineer
Tel: +44 (0)1223 434724                Samsung Cambridge Solution Centre
St John's House, St John's Innovation Park, Cowley Road,
Cambridge, CB4 0DS, UK

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 5 Dec 2014 15:29:17 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> Fixing the TRAPINT puts back the ZLE info output by the trap.  However,
> it doesn't fix the interrupts --- it's nothing like as bad as it was
> with the wrong sort of trap, but I still need to ^C a few times to get
> back to editing.  It might be once per completer.  (It's sort of vaguely
> useful that I can ^C to get to the next completer but it's a bit
> confusing and this obviously isn't the way it's designed.)

I think this is because eval is setting errflag unconditionally to 0.
Is that really correct?  Usually if you want to avoid a gross error in a
shell you use a subshell, not an eval (though other sorts of scripting
language do use eval for this sort of protection).

Certainly removing that seems to make interrupts much smoother; however,
it looks like the test system depends on the eval behaviour.  But
surely this means it's not actually possible to get interrupts to work
properly?

Could it be made conditional on being interactive (below, which does
what I want in the cases I've tried)?  Or should we be
setting a different flag / a different value of errflag if we received
an interrupt that should cause an abort?

BTW I'll commit the trap fix immediately.

diff --git a/Src/builtin.c b/Src/builtin.c
index c2af51f..f28b634 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5074,7 +5074,8 @@ eval(char **argv)
     if (fpushed)
 	funcstack = funcstack->prev;
 
-    errflag = 0;
+    if (errflag && !isset(INTERACTIVE))
+	errflag = 0;
     scriptname = oscriptname;
     ineval = oineval;
 
pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 5 Dec 2014 14:50:54 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> +TRPAINT TRAPQUIT() {
> +  zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
> +  zle -R
> +  return 130
> +}

(typo fixed in commit)

Sorry, there was another bit... we need to ensure multifuncdef is on in
completion functions for this.  It is by default but we only sanitise an
explicit list of options.

diff --git a/Completion/compinit b/Completion/compinit
index e42430d..9470c92 100644
--- a/Completion/compinit
+++ b/Completion/compinit
@@ -133,6 +133,7 @@ _comp_options=(
        extendedglob
        glob
        multibyte
+       multifuncdef
        nullglob
        rcexpandparam
        unset

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 5,  2:50pm, Peter Stephenson wrote:
} Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
}
} > This seems to help:
} > 
} > diff --git a/Src/glob.c b/Src/glob.c
} > index ca7bc44..b3903f2 100644
} > --- a/Src/glob.c
} > +++ b/Src/glob.c
} > @@ -463,7 +463,7 @@ scanner(Complist q, int shortcircuit)
} >      int errssofar = errsfound;
} >      struct dirsav ds;
} >  
} > -    if (!q)
} > +    if (!q || errflag)
} >  	return;
} >      init_dirsav(&ds);
} 
} Not for me: it's more basic than that.  The trap in _main_complete isn't
} working *at all* --- or, rather, it's working too well, by trapping all
} errors.

Not speaking directly for completion, but withOUT the above patch:

% : /**/*(/)

is not interruptible.  WITH the above patch, the recursive glob can be
interrupted.
 
} It's using an eval-style trap, so the "return" in it is forcing the
} current function in the completion code to return with that status and
} no error flagged.

That was actually intentional based on the behavior complaint that led
to the trap being added.  Changing the type of trap addresses a slightly
different problem.  What we need is some way to get both.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 5 Dec 2014 10:06:32 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Not speaking directly for completion, but withOUT the above patch:
> 
> % : /**/*(/)
> 
> is not interruptible.  WITH the above patch, the recursive glob can be
> interrupted.

Yes, I agree your patch is a good idea, too.  I did see a lot of
unnecessary goings on in scanner() even after errflag was being set
properly.

> } It's using an eval-style trap, so the "return" in it is forcing the
> } current function in the completion code to return with that status and
> } no error flagged.
> 
> That was actually intentional based on the behavior complaint that led
> to the trap being added.  Changing the type of trap addresses a slightly
> different problem.  What we need is some way to get both.

Well, if it aborts completely, which is certainly what I want, it will
of course return from the current function, so I've forgotten why you'd
ever want to do anything else.

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 05 Dec 2014 18:13:30 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > } It's using an eval-style trap, so the "return" in it is forcing the
> > } current function in the completion code to return with that status and
> > } no error flagged.
> > 
> > That was actually intentional based on the behavior complaint that led
> > to the trap being added.  Changing the type of trap addresses a slightly
> > different problem.  What we need is some way to get both.
> 
> Well, if it aborts completely, which is certainly what I want, it will
> of course return from the current function, so I've forgotten why you'd
> ever want to do anything else.

The trap goes back to zsh-workers/32322 (it got modified later but that
was largely cosmetic).  The rationale there, it appears, was it was
making difficulties in interrupting easier to test (by displaying the
message, I presume) --- not that it was fixing anything.  So as far as I
can see the underlying complaint was the same as mine, to return the
shell to the point where I get control of the command line as soon as
possible.  Am I missing something?  Do we actually need a trap at all to
achieve this fundamental goal?  Because the message might be helpful I
don't propose removing it as long as we can (as I think we can) work
around the remaining interrupt issues other ways.  I'm just trying to
understand if it has some other purpose.

On the other problem I came up with, that eval is resetting errflag even
if you've interrupted: how about the following?  Add a bit to errflag to
signify that the user interrupted the shell rather than that some
internal error (e.g. syntax) occurred.  Only reset this new bit in a few
key places: the main command loop when executing, the top of ZLE when
editing being the obvious places.  Convert other "errflag = 0"
assignments case by case so that they just remove bit 0; then eval can
continue to do its job of acting as a sandbox but without screwing up
the behaviour of interrupts.  I think doing that is fairly mechanical
and it achieves what's needed without compromising anything else.

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Fri, 5 Dec 2014 20:34:17 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On the other problem I came up with, that eval is resetting errflag even
> if you've interrupted: how about the following?  Add a bit to errflag to
> signify that the user interrupted the shell rather than that some
> internal error (e.g. syntax) occurred.  Only reset this new bit in a few
> key places: the main command loop when executing, the top of ZLE when
> editing being the obvious places.  Convert other "errflag = 0"
> assignments case by case so that they just remove bit 0; then eval can
> continue to do its job of acting as a sandbox but without screwing up
> the behaviour of interrupts.  I think doing that is fairly mechanical
> and it achieves what's needed without compromising anything else.

Here's my first go; it does seem to do what I want, and as a by-product
fixes all the little race conditions we've always had that meant you
couldn't interrupt chunks of code that were executed in any kind of
sandbox because the condition got reset afterwards.  I think a few of
these have been annoying me over the years.

The general strategy is to use the bit ERRFLAG_ERROR for internal
failures and ERRFLAG_INT for user interrupts.  There are only two
cases of the latter: on an untrapped SIGINT, the obvious case, and also
on a trapped SIGINT or SIGQUIT where we've been told to behave as if the
trap didn't trap the error condition.  That's straightforward for
SIGINT, less so for SIGQUIT but I took my cue from the fact that Bart
thought it worthwhile trapping SIGQUIT as an interactive "no, I really
mean abort" in completion, which implies that if we trap it we want it
to work at least as well as SIGINT.

Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
reset.  ERRFLAG_INT gets reset only in the following cases:

- in the main command loop.  This is what causes the shell not to exit but
instead go back to the main command loop when you ^C a command.

- at the start of zleread, so we can read the next thing to do whatever
just happened.  I'm not sure this is particularly useful since
in this case you'd typically expect the previous condition to have
occurred and it could mean e.g. you ignore an interrupt that
occurred just before a "vared".

- when we just finished completion.  This is needed so that the cases
that got this whole business kicked off behave as now (but more
reliably) --- a ^C gets you back to the command line, but the command
line is not trashed as it would be if you ^Ced outside completion (try
it if you're confused).  There's a race here, but it's no worse than it
ever was.

To ensure ERRFLAG_INT doesn't get reset unnecessarily there are a number
of cases where restoring errflag to a previously saved value keeps the
ERRFLAG_INT bit if it got set in the meanwhile.  I hope the rationale
here is obvious --- the ERRFLAG_ERROR is an internal state that needs
resetting, the ERRFLAG_INT an asynchronous condition where the user
doesn't care what the internal state is.

By the way, looking at the patch below you might wonder if it wouldn't
be more efficient to add a separate flag for interrupt error conditions
to test.  It wouldn't --- there are many more cases where errflag is
tested than when it is set, not affected by the patch below.

I suspect we'll just have to try this out and see how it works.

pws


diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 63c79a7..7b6130c 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -308,7 +308,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 
     prog = parse_string(zjoin(args, ' ', 1), 0);
     if (!prog) {
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	scriptname = oscriptname;
 	ineval = oineval;
 	return 1;
diff --git a/Src/Modules/zutil.c b/Src/Modules/zutil.c
index 1cca0c4..c894950 100644
--- a/Src/Modules/zutil.c
+++ b/Src/Modules/zutil.c
@@ -301,7 +301,8 @@ setstypat(Style s, char *pat, Patprog prog, char **vals, int eval)
 	int ef = errflag;
 
 	eprog = parse_string(zjoin(vals, ' ', 1), 0);
-	errflag = ef;
+	/* Keep any user interrupt error status */
+	errflag = ef | (errflag & ERRFLAG_INT);
 
 	if (!eprog)
 	{
@@ -394,10 +395,11 @@ evalstyle(Stypat p)
     unsetparam("reply");
     execode(p->eval, 1, 0, "style");
     if (errflag) {
-	errflag = ef;
+	/* Keep any user interrupt error status */
+	errflag = ef | (errflag & ERRFLAG_INT);
 	return NULL;
     }
-    errflag = ef;
+    errflag = ef | (errflag & ERRFLAG_INT);
 
     queue_signals();
     if ((ret = getaparam("reply")))
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 35d410c..b0c6e06 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -1671,7 +1671,7 @@ set_comp_sep(void)
     noaliases = ona;
     strinend();
     inpop();
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     noerrs = ne;
     lexrestore();
     wb = owb;
diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
index 0b7a324..d15c2d1 100644
--- a/Src/Zle/compctl.c
+++ b/Src/Zle/compctl.c
@@ -1879,7 +1879,7 @@ ccmakehookfn(UNUSED(Hookdef dummy), struct ccmakedat *dat)
 	if (!m || !(m = m->next))
 	    break;
 
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
     }
     redup(osi, 0);
     dat->lst = 1;
@@ -2121,7 +2121,7 @@ getreal(char *str)
     if (!errflag && nonempty(l) &&
 	((char *) peekfirst(l)) && ((char *) peekfirst(l))[0])
 	return dupstring(peekfirst(l));
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
 
     return dupstring(str);
 }
@@ -2599,7 +2599,7 @@ makecomplistlist(Compctl cc, char *s, int incmd, int compadd)
 	makecomplistflags(cc, s, incmd, compadd);
 
     /* Reset some information variables for the next try. */
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     offs = oloffs;
     wb = owb;
     we = owe;
@@ -2847,7 +2847,7 @@ sep_comp_string(char *ss, char *s, int noffs)
     noaliases = ona;
     strinend();
     inpop();
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     noerrs = ne;
     lexrestore();
     wb = owb;
@@ -3725,7 +3725,7 @@ makecomplistflags(Compctl cc, char *s, int incmd, int compadd)
 	noaliases = ona;
 	strinend();
 	inpop();
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	lexrestore();
 	/* Fine, now do full expansion. */
 	prefork(foo, 0);
diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index fcceb67..93438a0 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -1092,7 +1092,7 @@ do_single(Cmatch m)
 			noerrs = 1;
 			parsestr(p);
 			singsub(&p);
-			errflag = 0;
+			errflag &= ~ERRFLAG_ERROR;
 			noerrs = ne;
 		    }
 		} else {
diff --git a/Src/Zle/textobjects.c b/Src/Zle/textobjects.c
index 85d014b..37d2c0a 100644
--- a/Src/Zle/textobjects.c
+++ b/Src/Zle/textobjects.c
@@ -275,7 +275,7 @@ selectargument(UNUSED(char **args))
     noaliases = ona;
     strinend();
     inpop();
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     noerrs = ne;
     lexrestore();
     zlemetacs = ocs;
diff --git a/Src/Zle/zle_hist.c b/Src/Zle/zle_hist.c
index 9f65994..88623bb 100644
--- a/Src/Zle/zle_hist.c
+++ b/Src/Zle/zle_hist.c
@@ -853,8 +853,10 @@ pushlineoredit(char **args)
 	free(zhline);
     }
     ret = pushline(args);
-    if (!isfirstln)
-	errflag = done = 1;
+    if (!isfirstln) {
+	errflag |= ERRFLAG_ERROR;
+	done = 1;
+    }
     clearlist = 1;
     return ret;
 }
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index caa052b..616f354 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -744,7 +744,7 @@ raw_getbyte(long do_keytmout, char *cptr)
 			}
 			if (errflag) {
 			    /* No sensible way of handling errors here */
-			    errflag = 0;
+			    errflag &= ~ERRFLAG_ERROR;
 			    /*
 			     * Paranoia: don't run the hooks again this
 			     * time.
@@ -882,7 +882,7 @@ getbyte(long do_keytmout, int *timeout)
 		die = 0;
 		if (!errflag && !retflag && !breaks && !exit_pending)
 		    continue;
-		errflag = 0;
+		errflag &= ~ERRFLAG_ERROR;
 		breaks = obreaks;
 		errno = old_errno;
 		return lastchar = EOF;
@@ -1075,7 +1075,7 @@ zlecore(void)
 		DECCS();
 	    handleundo();
 	} else {
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	    break;
 	}
 #ifdef HAVE_POLL
@@ -1233,6 +1233,10 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
 
     zleactive = 1;
     resetneeded = 1;
+    /*
+     * Start of the main zle read.
+     * Fully reset error conditions, including error interrupt.
+     */
     errflag = retflag = 0;
     lastcol = -1;
     initmodifier(&zmod);
@@ -1658,7 +1662,7 @@ bin_vared(char *name, char **args, Options ops, UNUSED(int func))
     }
     if (!t || errflag) {
 	/* error in editing */
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	breaks = obreaks;
 	if (t)
 	    zsfree(t);
@@ -1778,7 +1782,7 @@ recursiveedit(UNUSED(char **args))
     zrefresh();
     zlecore();
 
-    locerror = errflag;
+    locerror = errflag ? 1 : 0;
     errflag = done = eofsent = 0;
 
     return locerror;
diff --git a/Src/Zle/zle_misc.c b/Src/Zle/zle_misc.c
index d432acf..23286fc 100644
--- a/Src/Zle/zle_misc.c
+++ b/Src/Zle/zle_misc.c
@@ -1041,7 +1041,7 @@ copyprevshellword(UNUSED(char **args))
 int
 sendbreak(UNUSED(char **args))
 {
-    errflag = 1;
+    errflag |= ERRFLAG_ERROR;
     return 1;
 }
 
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index b15d91c..864f804 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -829,7 +829,7 @@ docomplete(int lst)
 	    if (olst == COMP_EXPAND_COMPLETE &&
 		!strcmp(ol, zlemetaline)) {
 		zlemetacs = ocs;
-		errflag = 0;
+		errflag &= ~ERRFLAG_ERROR;
 
 		if (!compfunc) {
 		    char *p;
@@ -877,6 +877,19 @@ docomplete(int lst)
 
     active = 0;
     makecommaspecial(0);
+
+    /*
+     * As a special case, we reset user interrupts here.
+     * That's because completion is an intensive piece of
+     * computation that the user might want to interrupt separately
+     * from anything else going on.  If they do, they probably
+     * want to keep the line edit buffer intact.
+     *
+     * There's a race here that the user might hit ^C just
+     * after completion exited anyway, but that's inevitable.
+     */
+    errflag &= ~ERRFLAG_INT;
+
     return dat[1];
 }
 
@@ -1394,7 +1407,8 @@ get_comp_string(void)
     }
     strinend();
     inpop();
-    errflag = lexflags = 0;
+    lexflags = 0;
+    errflag &= ~ERRFLAG_ERROR;
     if (parbegin != -1) {
 	/* We are in command or process substitution if we are not in
 	 * a $((...)). */
@@ -2917,7 +2931,7 @@ getcurcmd(void)
     popheap();
     strinend();
     inpop();
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     unmetafy_line();
     lexrestore();
 
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 08a32c3..de91182 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1715,7 +1715,8 @@ zlecallhook(char *name, char *arg)
     execzlefunc(thingy, args, 1);
     unrefthingy(thingy);
 
-    errflag = saverrflag;
+    /* Retain any user interrupt error status */
+    errflag = saverrflag | (errflag & ERRFLAG_INT);
     retflag = savretflag;
 }
 
diff --git a/Src/builtin.c b/Src/builtin.c
index c2af51f..ad3a192 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -422,7 +422,7 @@ execbuiltin(LinkList args, Builtin bn)
 	argc -= argv - argarr;
 
 	if (errflag) {
-	    errflag = 0;
+	    errflag &= ~ERRFLAG_ERROR;
 	    return 1;
 	}
 
@@ -3136,7 +3136,7 @@ bin_unset(char *name, char **argv, Options ops, int func)
 		    }
 		}
 		returnval = errflag;
-		errflag = 0;
+		errflag &= ~ERRFLAG_ERROR;
 	    } else {
 		zerrnam(name, "%s: invalid element for unset", s);
 		returnval = 1;
@@ -4242,7 +4242,7 @@ bin_print(char *name, char **args, Options ops, int func)
 		if (*argp) {
 		    width = (int)mathevali(*argp++);
 		    if (errflag) {
-			errflag = 0;
+			errflag &= ~ERRFLAG_ERROR;
 			ret = 1;
 		    }
 		}
@@ -4272,7 +4272,7 @@ bin_print(char *name, char **args, Options ops, int func)
 		    if (*argp) {
 			prec = (int)mathevali(*argp++);
 			if (errflag) {
-			    errflag = 0;
+			    errflag &= ~ERRFLAG_ERROR;
 			    ret = 1;
 			}
 		    }
@@ -4452,7 +4452,7 @@ bin_print(char *name, char **args, Options ops, int func)
 			zlongval = (curarg) ? mathevali(curarg) : 0;
 			if (errflag) {
 			    zlongval = 0;
-			    errflag = 0;
+			    errflag &= ~ERRFLAG_ERROR;
 			    ret = 1;
 			}
 			print_val(zlongval)
@@ -4481,7 +4481,7 @@ bin_print(char *name, char **args, Options ops, int func)
 			} else doubleval = 0;
 			if (errflag) {
 			    doubleval = 0;
-			    errflag = 0;
+			    errflag &= ~ERRFLAG_ERROR;
 			    ret = 1;
 			}
 			print_val(doubleval)
@@ -4494,7 +4494,7 @@ bin_print(char *name, char **args, Options ops, int func)
 			zulongval = (curarg) ? mathevali(curarg) : 0;
 			if (errflag) {
 			    zulongval = 0;
-			    errflag = 0;
+			    errflag &= ~ERRFLAG_ERROR;
 			    ret = 1;
 			}
 			print_val(zulongval)
@@ -4871,7 +4871,7 @@ zexit(int val, int from_where)
     in_exit = -1;
     /*
      * We want to do all remaining processing regardless of preceding
-     * errors.
+     * errors, even user interrupts.
      */
     errflag = 0;
 
@@ -5074,7 +5074,7 @@ eval(char **argv)
     if (fpushed)
 	funcstack = funcstack->prev;
 
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     scriptname = oscriptname;
     ineval = oineval;
 
@@ -6101,7 +6101,7 @@ bin_test(char *name, char **argv, UNUSED(Options ops), int func)
     condlex = zshlex;
 
     if (errflag) {
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	lexrestore();
 	return 1;
     }
@@ -6278,7 +6278,7 @@ bin_let(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int func))
     while (*argv)
 	val = matheval(*argv++);
     /* Errors in math evaluation in let are non-fatal. */
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     /* should test for fabs(val.u.d) < epsilon? */
     return (val.type == MN_INTEGER) ? val.u.l == 0 : val.u.d == 0.0;
 }
diff --git a/Src/exec.c b/Src/exec.c
index a5f8771..c18f3cd 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -59,7 +59,7 @@ mod_export int noerrs;
 /**/
 int nohistsave;
 
-/* error/break flag */
+/* error flag: bits from enum errflag_bits */
 
 /**/
 mod_export int errflag;
@@ -1601,7 +1601,8 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			    (killpg(jobtab[list_pipe_job].gleader, 0) == -1 ? 2 : 1);
 			list_pipe_pid = pid;
 			list_pipe_start = bgtime;
-			nowait = errflag = 1;
+			nowait = 1;
+			errflag |= ERRFLAG_ERROR;
 			breaks = loops;
 			close(synch[1]);
 			read_loop(synch[0], &dummy, 1);
@@ -1634,7 +1635,10 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			list_pipe_child = 1;
 			opts[INTERACTIVE] = 0;
 			if (errbrk_saved) {
-			    errflag = prev_errflag;
+			    /*
+			     * Keep any user interrupt bit in errflag.
+			     */
+			    errflag = prev_errflag | (errflag & ERRFLAG_INT);
 			    breaks = prev_breaks;
 			}
 			break;
@@ -1719,12 +1723,14 @@ execpline2(Estate state, wordcode pcode,
 
 	    if (pipe(synch) < 0) {
 		zerr("pipe failed: %e", errno);
-		lastval = errflag = 1;
+		lastval = 1;
+		errflag |= ERRFLAG_ERROR;
 		return;
 	    } else if ((pid = zfork(&bgtime)) == -1) {
 		close(synch[0]);
 		close(synch[1]);
-		lastval = errflag = 1;
+		lastval = 1;
+		errflag |= ERRFLAG_ERROR;
 		return;
 	    } else if (pid) {
 		char dummy, *text;
@@ -2560,7 +2566,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		while (next && *next == '-' && strlen(next) >= 2) {
 		    if (!firstnode(args)) {
 			zerr("exec requires a command to execute");
-			errflag = lastval = 1;
+			lastval = 1;
+			errflag |= ERRFLAG_ERROR;
 			goto done;
 		    }
 		    uremnode(args, firstnode(args));
@@ -2577,12 +2584,14 @@ execcmd(Estate state, int input, int output, int how, int last1)
 			    } else {
 				if (!firstnode(args)) {
 				    zerr("exec requires a command to execute");
-				    errflag = lastval = 1;
+				    lastval = 1;
+				    errflag |= ERRFLAG_ERROR;
 				    goto done;
 				}
 				if (!nextnode(firstnode(args))) {
 				    zerr("exec flag -a requires a parameter");
-				    errflag = lastval = 1;
+				    lastval = 1;
+				    errflag |= ERRFLAG_ERROR;
 				    goto done;
 				}
 				exec_argv0 = (char *)
@@ -2598,7 +2607,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 			    break;
 			default:
 			    zerr("unknown exec flag -%c", *cmdopt);
-			    errflag = lastval = 1;
+			    lastval = 1;
+			    errflag |= ERRFLAG_ERROR;
 			    return;
 			}
 		    }
@@ -2661,7 +2671,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		    } else if (!nullcmd || !*nullcmd || opts[CSHNULLCMD] ||
 			       (cflags & BINF_PREFIX)) {
 			zerr("redirection with no command");
-			errflag = lastval = 1;
+			lastval = 1;
+			errflag |= ERRFLAG_ERROR;
 			return;
 		    } else if (!nullcmd || !*nullcmd || opts[SHNULLCMD]) {
 			if (!args)
@@ -2691,7 +2702,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		    if (varspc)
 			addvars(state, varspc, 0);
 		    if (errflag)
-			lastval = errflag;
+			lastval = errflag ? 1 : 0;
 		    else
 			lastval = cmdoutval;
 		    if (isset(XTRACE)) {
@@ -2795,7 +2806,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    }
 	}
 	if (!nextnode(firstnode(args)))
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
     }
 
     if (type == WC_FUNCDEF) {
@@ -2940,7 +2951,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	} else if ((pid = zfork(&bgtime)) == -1) {
 	    close(synch[0]);
 	    close(synch[1]);
-	    lastval = errflag = 1;
+	    lastval = 1;
+	    errflag |= ERRFLAG_ERROR;
 	    goto fatal;
 	}
 	if (pid) {
@@ -3529,7 +3541,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		else
 		    exit(1);
 	    }
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	}
     }
     if (newxtrerr) {
@@ -3759,8 +3771,10 @@ gethere(char **strp, int typ)
 
 	parsestr(buf);
 
-	if (!errflag)
-	    errflag = ef;
+	if (!errflag) {
+	    /* Retain any user interrupt error */
+	    errflag = ef | (errflag & ERRFLAG_INT);
+	}
     }
     s = dupstring(buf);
     zfree(buf, bsiz);
@@ -3854,7 +3868,7 @@ getoutput(char *cmd, int qt)
 	return readoutput(stream, qt);
     }
     if (mpipe(pipes) < 0) {
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	cmdoutpid = 0;
 	return NULL;
     }
@@ -3864,7 +3878,7 @@ getoutput(char *cmd, int qt)
 	/* fork error */
 	zclose(pipes[0]);
 	zclose(pipes[1]);
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	cmdoutpid = 0;
 	child_unblock();
 	return NULL;
@@ -4274,7 +4288,7 @@ execcond(Estate state, UNUSED(int do_exec))
      * into a shell error.
      */
     if (stat == 2)
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
     cmdpop();
     if (isset(XTRACE)) {
 	fprintf(xtrerr, " ]]\n");
@@ -4314,7 +4328,7 @@ execarith(Estate state, UNUSED(int do_exec))
 	fflush(xtrerr);
     }
     if (errflag) {
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	return 2;
     }
     /* should test for fabs(val.u.d) < epsilon? */
@@ -4932,7 +4946,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 						(name = fname)))) {
 	    zwarn("%s: function not defined by file", name);
 	    if (noreturnval)
-		errflag = 1;
+		errflag |= ERRFLAG_ERROR;
 	    else
 		lastval = 1;
 	    goto doneshfunc;
diff --git a/Src/glob.c b/Src/glob.c
index ca7bc44..f5ac47a 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -682,7 +682,7 @@ parsecomplist(char *instr)
 	/* Now get the next path component if there is one. */
 	l1 = (Complist) zhalloc(sizeof *l1);
 	if ((l1->next = parsecomplist(instr)) == NULL) {
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	    return NULL;
 	}
 	l1->pat = patcompile(NULL, compflags | PAT_ANY, NULL);
@@ -728,7 +728,7 @@ parsecomplist(char *instr)
 	    return (ef && !l1->next) ? NULL : l1;
 	}
     }
-    errflag = 1;
+    errflag |= ERRFLAG_ERROR;
     return NULL;
 }
 
@@ -1790,7 +1790,7 @@ zglob(LinkList list, LinkNode np, int nountok)
 	    insertlinknode(list, node, ostr);
 	    return;
 	}
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	zerr("bad pattern: %s", ostr);
 	return;
     }
@@ -1873,7 +1873,8 @@ zglob(LinkList list, LinkNode np, int nountok)
 				tmpptr->sortstrs[iexec] = tmpptr->name;
 			}
 
-			errflag = ef;
+			/* Retain any user interrupt error status */
+			errflag = ef | (errflag & ERRFLAG_INT);
 			lastval = lv;
 		    } else {
 			/* Failed, let's be safe */
@@ -3733,7 +3734,8 @@ qualsheval(char *name, UNUSED(struct stat *buf), UNUSED(off_t days), char *str)
 	execode(prog, 1, 0, "globqual");
 
 	ret = lastval;
-	errflag = ef;
+	/* Retain any user interrupt error status */
+	errflag = ef | (errflag & ERRFLAG_INT);
 	lastval = lv;
 
 	if (!(inserts = getaparam("reply")) &&
diff --git a/Src/hist.c b/Src/hist.c
index 7fe843a..a006170 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -287,7 +287,8 @@ ihgetc(void)
 	c = histsubchar(c);
 	if (c < 0) {
 	    /* bad expansion */
-	    errflag = lexstop = 1;
+	    lexstop = 1;
+	    errflag |= ERRFLAG_ERROR;
 	    return ' ';
 	}
     }
@@ -721,7 +722,7 @@ histsubchar(int c)
 		    noerrs = 1;
 		    parse_subst_string(sline);
 		    noerrs = one;
-		    errflag = oef;
+		    errflag = oef | (errflag & ERRFLAG_INT);
 		    remnulargs(sline);
 		    untokenize(sline);
 		}
@@ -880,7 +881,8 @@ hbegin(int dohist)
     char *hf;
 
     isfirstln = isfirstch = 1;
-    errflag = histdone = 0;
+    errflag &= ~ERRFLAG_ERROR;
+    histdone = 0;
     if (!dohist)
 	stophist = 2;
     else if (dohist != 2)
@@ -3182,7 +3184,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
     noaliases = ona;
     strinend();
     inpop();
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     nocomments = onc;
     noerrs = ne;
     lexrestore();
diff --git a/Src/init.c b/Src/init.c
index 6551661..3cbd4e5 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -123,6 +123,11 @@ loop(int toplevel, int justonce)
 		    hbegin(1);
 		else
 		    stophist = hstop;
+		/*
+		 * Reset all errors, including user interupts.
+		 * This is what allows ^C in an interactive shell
+		 * to return us to the command line.
+		 */
 		errflag = 0;
 	    }
 	}
@@ -178,7 +183,15 @@ loop(int toplevel, int justonce)
 
 		/* The only permanent storage is from getpermtext() */
 		zsfree(cmdstr);
-		errflag = 0;
+		/*
+		 * Note this does *not* remove a user interrupt error
+		 * condition, even though we're at the top level loop:
+		 * that would be inconsistent with the case where
+		 * we didn't execute a preexec function.  This is
+		 * an implementation detail that an interrupting user
+		 * does't care about.
+		 */
+		errflag &= ~ERRFLAG_ERROR;
 	    }
 	    if (stopmsg)	/* unset 'you have stopped jobs' flag */
 		stopmsg--;
@@ -689,7 +702,7 @@ init_term(void)
     {
 	if (isset(INTERACTIVE))
 	    zerr("can't find terminal definition for %s", term);
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	termflags |= TERM_BAD;
 	return 0;
     } else {
@@ -1336,7 +1349,7 @@ source(char *s)
 
     if (prog) {
 	pushheap();
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	execode(prog, 1, 0, "filecode");
 	popheap();
 	if (errflag)
@@ -1379,7 +1392,7 @@ source(char *s)
     lineno = oldlineno;              /* our current lineno                   */
     loops = oloops;                  /* the # of nested loops we are in      */
     dosetopt(SHINSTDIN, oldshst, 1, opts); /* SHINSTDIN option               */
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     if (!exit_pending)
 	retflag = 0;
     scriptname = old_scriptname;
diff --git a/Src/input.c b/Src/input.c
index 4ac7e6e..9552331 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -291,7 +291,8 @@ inputline(void)
     }
     if (errflag) {
 	free(ingetcline);
-	return lexstop = errflag = 1;
+	errflag |= ERRFLAG_ERROR;
+	return lexstop = 1;
     }
     if (isset(VERBOSE)) {
 	/* Output the whole line read so far. */
diff --git a/Src/jobs.c b/Src/jobs.c
index 6663a40..6e47e5e 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -509,7 +509,7 @@ update_job(Job jn)
 			prev_errflag = errflag;
 		    }
 		    breaks = loops;
-		    errflag = 1;
+		    errflag |= ERRFLAG_ERROR;
 		    inerrflush();
 		}
 	    } else {
@@ -526,7 +526,7 @@ update_job(Job jn)
 	    prev_errflag = errflag;
 	}
 	breaks = loops;
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	inerrflush();
     }
     if (somestopped && jn->stat & STAT_SUPERJOB)
@@ -581,7 +581,7 @@ update_job(Job jn)
 		    breaks = loops;
 	    } else {
 		breaks = loops;
-		errflag = 1;
+		errflag |= ERRFLAG_ERROR;
 	    }
 	    check_cursh_sig(sig);
 	}
@@ -1444,12 +1444,7 @@ zwaitjob(int job, int wait_cmd)
 		restore_queue_signals(q);
 		return 128 + last_signal;
 	    }
-	    /* Commenting this out makes ^C-ing a job started by a function
-	       stop the whole function again.  But I guess it will stop
-	       something else from working properly, we have to find out
-	       what this might be.  --oberon
-
-	    errflag = 0; */
+	    errflag &= ~ERRFLAG_ERROR;
 	    if (subsh) {
 		killjb(jn, SIGCONT);
 		jn->stat &= ~STAT_STOPPED;
diff --git a/Src/lex.c b/Src/lex.c
index 1a854f5..68a892a 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -385,7 +385,7 @@ lexrestore(void)
     ecnfunc = ln->ecnfunc;
     hlinesz = ln->hlinesz;
     toklineno = ln->toklineno;
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     free(ln);
 
     unqueue_signals();
@@ -1725,7 +1725,8 @@ parse_subst_string(char *s)
     inpop();
     DPUTS(cmdsp, "BUG: parse_subst_string: cmdstack not empty.");
     lexrestore();
-    errflag = err;
+    /* Keep any interrupt error status */
+    errflag = err | (errflag & ERRFLAG_INT);
     if (ctok == LEXERR) {
 	untokenize(s);
 	return 1;
diff --git a/Src/loop.c b/Src/loop.c
index 82d2fe3..69805ea 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -259,7 +259,8 @@ execselect(Estate state, UNUSED(int do_exec))
 				   0, ZLCON_SELECT);
 		    if (errflag)
 			str = NULL;
-		    errflag = oef;
+		    /* Keep any user interrupt error status */
+		    errflag = oef | (errflag & ERRFLAG_INT);
 	    	} else {
 		    str = promptexpand(prompt3, 0, NULL, NULL, NULL);
 		    zputs(str, stderr);
@@ -671,7 +672,7 @@ exectry(Estate state, int do_exec)
     /* The always clause. */
     save_try_errflag = try_errflag;
     try_errflag = (zlong)errflag;
-    errflag = 0;
+    errflag &= ~ERRFLAG_ERROR;
     save_retflag = retflag;
     retflag = 0;
     save_breaks = breaks;
@@ -682,7 +683,10 @@ exectry(Estate state, int do_exec)
     state->pc = always;
     execlist(state, 1, do_exec);
 
-    errflag = try_errflag ? 1 : 0;
+    if (try_errflag)
+	errflag |= ERRFLAG_ERROR;
+    else
+	errflag &= ~ERRFLAG_ERROR;
     try_errflag = save_try_errflag;
     if (!retflag)
 	retflag = save_retflag;
diff --git a/Src/params.c b/Src/params.c
index 61edc5d..bdace79 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2654,7 +2654,7 @@ assignsparam(char *s, char *val, int flags)
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
 	zsfree(val);
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	return NULL;
     }
     queue_signals();
@@ -2783,7 +2783,7 @@ assignaparam(char *s, char **val, int flags)
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
 	freearray(val);
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	return NULL;
     }
     queue_signals();
@@ -2799,7 +2799,7 @@ assignaparam(char *s, char **val, int flags)
 	    zerr("%s: attempt to set slice of associative array",
 		 v->pm->node.nam);
 	    freearray(val);
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	    return NULL;
 	}
 	v = NULL;
@@ -2870,13 +2870,13 @@ sethparam(char *s, char **val)
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
 	freearray(val);
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	return NULL;
     }
     if (strchr(s, '[')) {
 	freearray(val);
 	zerr("nested associative arrays not yet supported");
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	return NULL;
     }
     if (unset(EXECOPT))
@@ -2916,7 +2916,7 @@ setnparam(char *s, mnumber val)
 
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 	return NULL;
     }
     if (unset(EXECOPT))
diff --git a/Src/parse.c b/Src/parse.c
index 4ceeb4e..c1709e0 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -71,13 +71,14 @@ struct heredocs *hdocs;
 
 #define YYERROR(O)  { tok = LEXERR; ecused = (O); return 0; }
 #define YYERRORV(O) { tok = LEXERR; ecused = (O); return; }
-#define COND_ERROR(X,Y) do { \
-  zwarn(X,Y); \
-  herrflush(); \
-  if (noerrs != 2) \
-    errflag = 1; \
-  YYERROR(ecused) \
-} while(0)
+#define COND_ERROR(X,Y) \
+    do {					\
+	zwarn(X,Y);				\
+	herrflush();				\
+	if (noerrs != 2)			\
+	    errflag |= ERRFLAG_ERROR;		\
+	YYERROR(ecused)				\
+	    } while(0)
 
 
 /* 
@@ -506,7 +507,7 @@ par_event(void)
 	yyerror(1);
 	herrflush();
 	if (noerrs != 2)
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	ecused--;
 	return 0;
     } else {
@@ -2339,7 +2340,7 @@ yyerror(int noerr)
 	    zwarn("parse error");
     }
     if (!noerr && noerrs != 2)
-	errflag = 1;
+	errflag |= ERRFLAG_ERROR;
 }
 
 /*
@@ -3031,7 +3032,7 @@ build_dump(char *nam, char *dump, char **files, int ali, int map, int flags)
 	file = metafy(file, flen, META_REALLOC);
 
 	if (!(prog = parse_string(file, 1)) || errflag) {
-	    errflag = 0;
+	    errflag &= ~ERRFLAG_ERROR;
 	    close(dfd);
 	    zfree(file, flen);
 	    zwarnnam(nam, "can't read file: %s", *files);
@@ -3141,7 +3142,7 @@ build_cur_dump(char *nam, char *dump, char **names, int match, int map,
 	    for (hn = shfunctab->nodes[i]; hn; hn = hn->next)
 		if (cur_add_func(nam, (Shfunc) hn, lnames, progs,
 				 &hlen, &tlen, what)) {
-		    errflag = 0;
+		    errflag &= ~ERRFLAG_ERROR;
 		    close(dfd);
 		    unlink(dump);
 		    return 1;
@@ -3166,7 +3167,7 @@ build_cur_dump(char *nam, char *dump, char **names, int match, int map,
 			pattry(pprog, hn->nam) &&
 			cur_add_func(nam, (Shfunc) hn, lnames, progs,
 				     &hlen, &tlen, what)) {
-			errflag = 0;
+			errflag &= ~ERRFLAG_ERROR;
 			close(dfd);
 			unlink(dump);
 			return 1;
@@ -3177,13 +3178,13 @@ build_cur_dump(char *nam, char *dump, char **names, int match, int map,
 	    if (errflag ||
 		!(shf = (Shfunc) shfunctab->getnode(shfunctab, *names))) {
 		zwarnnam(nam, "unknown function: %s", *names);
-		errflag = 0;
+		errflag &= ~ERRFLAG_ERROR;
 		close(dfd);
 		unlink(dump);
 		return 1;
 	    }
 	    if (cur_add_func(nam, shf, lnames, progs, &hlen, &tlen, what)) {
-		errflag = 0;
+		errflag &= ~ERRFLAG_ERROR;
 		close(dfd);
 		unlink(dump);
 		return 1;
@@ -3192,7 +3193,7 @@ build_cur_dump(char *nam, char *dump, char **names, int match, int map,
     }
     if (empty(progs)) {
 	zwarnnam(nam, "no functions");
-	errflag = 0;
+	errflag &= ~ERRFLAG_ERROR;
 	close(dfd);
 	unlink(dump);
 	return 1;
diff --git a/Src/prompt.c b/Src/prompt.c
index 0cc9ef9..3552575 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -192,8 +192,11 @@ promptexpand(char *s, int ns, char *rs, char *Rs, unsigned int *txtchangep)
 	if (*s == Nularg && s[1] == '\0')
 	    *s = '\0';
 
-	/* Ignore errors and status change in prompt substitution */
-	errflag = olderr;
+	/*
+	 * Ignore errors and status change in prompt substitution.
+	 * However, keep any user interrupt error that occurred.
+	 */
+	errflag = olderr | (errflag & ERRFLAG_INT);
 	lastval = oldval;
     }
 
diff --git a/Src/signals.c b/Src/signals.c
index e728505..899f121 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -619,7 +619,7 @@ zhandler(int sig)
 		zexit(SIGINT, 1);
             if (list_pipe || chline || simple_pline) {
                 breaks = loops;
-                errflag = 1;
+                errflag |= ERRFLAG_INT;
 		inerrflush();
 		check_cursh_sig(SIGINT);
             }
@@ -640,6 +640,11 @@ zhandler(int sig)
 	    if (idle >= 0 && idle < tmout)
 		alarm(tmout - idle);
 	    else {
+		/*
+		 * We want to exit now.
+		 * Cancel all errors, including a user interrupt
+		 * which is now redundant.
+		 */
 		errflag = noerrs = 0;
 		zwarn("timeout");
 		stopmsg = 1;
@@ -1267,7 +1272,18 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
 	!(isfunc && new_trap_return == 0)) {
 	if (isfunc) {
 	    breaks = loops;
-	    errflag = 1;
+	    /*
+	     * For SIGINT we behave the same as the default behaviour
+	     * i.e. we set the error bit indicating an interrupt.
+	     * We do this with SIGQUIT, too, even though we don't
+	     * handle SIGQUIT by default.  That's to try to make
+	     * it behave a bit more like its normal behaviour when
+	     * the trap handler has told us that's what it wants.
+	     */
+	    if (sig == SIGINT || sig == SIGQUIT)
+		errflag |= ERRFLAG_INT;
+	    else
+		errflag |= ERRFLAG_ERROR;
 	}
 	lastval = new_trap_return;
 	/* return triggered */
@@ -1282,8 +1298,12 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
 	     */
 	    lastval = olastval;
 	}
-	if (try_tryflag)
-	    errflag = traperr;
+	if (try_tryflag) {
+	    if (traperr)
+		errflag |= ERRFLAG_ERROR;
+	    else
+		errflag &= ~ERRFLAG_ERROR;
+	}
 	breaks += obreaks;
 	/* return not triggered: restore old flag */
 	retflag = oretflag;
diff --git a/Src/subst.c b/Src/subst.c
index 61aa1c1..43932c2 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2822,7 +2822,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
 		haserr = parse_subst_string(s);
 		noerrs = one;
 		if (!quoteerr) {
-		    errflag = oef;
+		    /* Retain user interrupt error status */
+		    errflag = oef | (errflag & ERRFLAG_INT);
 		    if (haserr)
 			shtokenize(s);
 		} else if (haserr || errflag) {
@@ -3249,8 +3250,10 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
 		haserr = 1;
 	}
 	noerrs = one;
-	if (!quoteerr)
-	    errflag = oef;
+	if (!quoteerr) {
+	    /* Retain user interrupt error status */
+	    errflag = oef | (errflag & ERRFLAG_INT);
+	}
 	if (haserr || errflag)
 	    return NULL;
     }
@@ -3483,8 +3486,10 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
 		    untokenize(*ap);
 		}
 		noerrs = one;
-		if (!quoteerr)
-		    errflag = oef;
+		if (!quoteerr) {
+		    /* Retain any user interrupt error status */
+		    errflag = oef | (errflag & ERRFLAG_INT);
+		}
 		else if (haserr || errflag) {
 		    zerr("parse error in parameter value");
 		    return NULL;
@@ -3516,8 +3521,10 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
 		    noerrs = 1;
 		haserr = parse_subst_string(val);
 		noerrs = one;
-		if (!quoteerr)
-		    errflag = oef;
+		if (!quoteerr) {
+		    /* Retain any user interrupt error status */
+		    errflag = oef | (errflag & ERRFLAG_INT);
+		}
 		else if (haserr || errflag) {
 		    zerr("parse error in parameter value");
 		    return NULL;
@@ -4086,7 +4093,8 @@ modify(char **str, char **ptr)
 			    noerrs = 1;
 			    parse_subst_string(copy);
 			    noerrs = one;
-			    errflag = oef;
+			    /* Retain any user interrupt error status */
+			    errflag = oef | (errflag & ERRFLAG_INT);
 			    remnulargs(copy);
 			    untokenize(copy);
 			}
@@ -4161,7 +4169,8 @@ modify(char **str, char **ptr)
 			noerrs = 1;
 			parse_subst_string(*str);
 			noerrs = one;
-			errflag = oef;
+			/* Retain any user interrupt error status */
+			errflag = oef | (errflag & ERRFLAG_INT);
 			remnulargs(*str);
 			untokenize(*str);
 		    }
diff --git a/Src/utils.c b/Src/utils.c
index 9268147..d6bb614 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -153,7 +153,7 @@ VA_DCL
 
     if (errflag || noerrs) {
 	if (noerrs < 2)
-	    errflag = 1;
+	    errflag |= ERRFLAG_ERROR;
 	return;
     }
 
@@ -161,7 +161,7 @@ VA_DCL
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(NULL, fmt, ap);
     va_end(ap);
-    errflag = 1;
+    errflag |= ERRFLAG_ERROR;
 }
 
 /**/
@@ -181,7 +181,7 @@ VA_DCL
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(cmd, fmt, ap);
     va_end(ap);
-    errflag = 1;
+    errflag |= ERRFLAG_ERROR;
 }
 
 /**/
@@ -330,7 +330,7 @@ zerrmsg(FILE *file, const char *fmt, va_list ap)
 		num = va_arg(ap, int);
 		if (num == EINTR) {
 		    fputs("interrupt\n", file);
-		    errflag = 1;
+		    errflag |= ERRFLAG_ERROR;
 		    return;
 		}
 		errmsg = strerror(num);
diff --git a/Src/zsh.h b/Src/zsh.h
index 031deaf..b366e0f 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2623,6 +2623,20 @@ enum trap_state {
 #define IN_EVAL_TRAP() \
     (intrap && !trapisfunc && traplocallevel == locallevel)
 
+/*
+ * Bits in the errflag variable.
+ */
+enum errflag_bits {
+    /*
+     * Standard internal error bit.
+     */
+    ERRFLAG_ERROR = 1,
+    /*
+     * User interrupt.
+     */
+    ERRFLAG_INT = 2
+};
+
 /***********/
 /* Sorting */
 /***********/

Re: Interrupting globs (Re: Something rotten in tar completion)

[Ray Andrews]

On 12/05/2014 02:07 PM, Peter Stephenson wrote:
>
>> continue to do its job of acting as a sandbox but without screwing up
>>
What is a sandbox?

So there was no distinction between an internal error and a user break?
I thought the philosophy was to go overboard making very fine
distinctions between various breaks, and that would seem to be
the very first one to make.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Fri, Dec 5, 2014 at 11:07 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Fri, 5 Dec 2014 20:34:17 +0000
> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>> On the other problem I came up with, that eval is resetting errflag even
>> if you've interrupted: how about the following?  Add a bit to errflag to
>> signify that the user interrupted the shell rather than that some
>> internal error (e.g. syntax) occurred.  Only reset this new bit in a few
>> key places: the main command loop when executing, the top of ZLE when
>> editing being the obvious places.  Convert other "errflag = 0"
>> assignments case by case so that they just remove bit 0; then eval can
>> continue to do its job of acting as a sandbox but without screwing up
>> the behaviour of interrupts.  I think doing that is fairly mechanical
>> and it achieves what's needed without compromising anything else.
>
> Here's my first go; it does seem to do what I want, and as a by-product
> fixes all the little race conditions we've always had that meant you
> couldn't interrupt chunks of code that were executed in any kind of
> sandbox because the condition got reset afterwards.  I think a few of
> these have been annoying me over the years.
>
> The general strategy is to use the bit ERRFLAG_ERROR for internal
> failures and ERRFLAG_INT for user interrupts.  There are only two
> cases of the latter: on an untrapped SIGINT, the obvious case, and also
> on a trapped SIGINT or SIGQUIT where we've been told to behave as if the
> trap didn't trap the error condition.  That's straightforward for
> SIGINT, less so for SIGQUIT but I took my cue from the fact that Bart
> thought it worthwhile trapping SIGQUIT as an interactive "no, I really
> mean abort" in completion, which implies that if we trap it we want it
> to work at least as well as SIGINT.
>
> Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
> reset.  ERRFLAG_INT gets reset only in the following cases:
>
> - in the main command loop.  This is what causes the shell not to exit but
> instead go back to the main command loop when you ^C a command.
>
> - at the start of zleread, so we can read the next thing to do whatever
> just happened.  I'm not sure this is particularly useful since
> in this case you'd typically expect the previous condition to have
> occurred and it could mean e.g. you ignore an interrupt that
> occurred just before a "vared".
>
> - when we just finished completion.  This is needed so that the cases
> that got this whole business kicked off behave as now (but more
> reliably) --- a ^C gets you back to the command line, but the command
> line is not trashed as it would be if you ^Ced outside completion (try
> it if you're confused).  There's a race here, but it's no worse than it
> ever was.
>
> To ensure ERRFLAG_INT doesn't get reset unnecessarily there are a number
> of cases where restoring errflag to a previously saved value keeps the
> ERRFLAG_INT bit if it got set in the meanwhile.  I hope the rationale
> here is obvious --- the ERRFLAG_ERROR is an internal state that needs
> resetting, the ERRFLAG_INT an asynchronous condition where the user
> doesn't care what the internal state is.
>
> By the way, looking at the patch below you might wonder if it wouldn't
> be more efficient to add a separate flag for interrupt error conditions
> to test.  It wouldn't --- there are many more cases where errflag is
> tested than when it is set, not affected by the patch below.
>
> I suspect we'll just have to try this out and see how it works.

This seems to work well for me in the cases you talked about, but I
quickly noticed one surprising problem. I have some stuff in my
chpwd() hook to show git branches and stuff, and these used to be
interruptible by ctrl-c (the commands are very fast with hot cache,
but can be somewhat painful with cold cache, like 5-10 seconds delay).
With the patch, I cannot interrupt them (sometimes?).

chpwd () {
    stty -echo >&/dev/null
    test -f .tdldb && tdll -1 >&2
    test -d .git && {
        git branch
        test -d .git/svn && {
            echo -n r
            git svn find-rev master
        }
        git name-rev HEAD
        git describe --tags HEAD 2> /dev/null
    } >&2
    if [[ "$_NONOCDLS" = 1 ]]
    then
        ls $LS_OPTIONS >&2
    fi
}

The git branch is actually git brunch, which is git aliased to
brunch  -- alias for '!git branch -v|sed -e 's/t/ /g' -e 's/(.{'$((
$(tput cols) - 1 ))'})
There's no .tdldb in that directory either, but including it unmodified anyway.

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sat, Dec 6, 2014 at 1:36 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> brunch  -- alias for '!git branch -v|sed -e 's/t/ /g' -e 's/(.{'$((

This is the .gitconfig line, didn't notice the completion listing
mungled it up a bit. Not likely that it matters, but I don't want you
guys to think I'm some kind of crazy person that replaces the letter t
with spaces in commit messages.
brunch = !git branch -v|sed -e 's/\\t/ /g' -e 's/\\(.\\{'$(( $(tput
cols) - 1 ))'\\}\\)..\\+/\\1$/'

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sat, Dec 6, 2014 at 1:36 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Fri, Dec 5, 2014 at 11:07 PM, Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
>> On Fri, 5 Dec 2014 20:34:17 +0000
>> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>>> On the other problem I came up with, that eval is resetting errflag even
>>> if you've interrupted: how about the following?  Add a bit to errflag to
>>> signify that the user interrupted the shell rather than that some
>>> internal error (e.g. syntax) occurred.  Only reset this new bit in a few
>>> key places: the main command loop when executing, the top of ZLE when
>>> editing being the obvious places.  Convert other "errflag = 0"
>>> assignments case by case so that they just remove bit 0; then eval can
>>> continue to do its job of acting as a sandbox but without screwing up
>>> the behaviour of interrupts.  I think doing that is fairly mechanical
>>> and it achieves what's needed without compromising anything else.
>>
>> Here's my first go; it does seem to do what I want, and as a by-product
>> fixes all the little race conditions we've always had that meant you
>> couldn't interrupt chunks of code that were executed in any kind of
>> sandbox because the condition got reset afterwards.  I think a few of
>> these have been annoying me over the years.
>>
>> The general strategy is to use the bit ERRFLAG_ERROR for internal
>> failures and ERRFLAG_INT for user interrupts.  There are only two
>> cases of the latter: on an untrapped SIGINT, the obvious case, and also
>> on a trapped SIGINT or SIGQUIT where we've been told to behave as if the
>> trap didn't trap the error condition.  That's straightforward for
>> SIGINT, less so for SIGQUIT but I took my cue from the fact that Bart
>> thought it worthwhile trapping SIGQUIT as an interactive "no, I really
>> mean abort" in completion, which implies that if we trap it we want it
>> to work at least as well as SIGINT.
>>
>> Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
>> reset.  ERRFLAG_INT gets reset only in the following cases:
>>
>> - in the main command loop.  This is what causes the shell not to exit but
>> instead go back to the main command loop when you ^C a command.
>>
>> - at the start of zleread, so we can read the next thing to do whatever
>> just happened.  I'm not sure this is particularly useful since
>> in this case you'd typically expect the previous condition to have
>> occurred and it could mean e.g. you ignore an interrupt that
>> occurred just before a "vared".
>>
>> - when we just finished completion.  This is needed so that the cases
>> that got this whole business kicked off behave as now (but more
>> reliably) --- a ^C gets you back to the command line, but the command
>> line is not trashed as it would be if you ^Ced outside completion (try
>> it if you're confused).  There's a race here, but it's no worse than it
>> ever was.
>>
>> To ensure ERRFLAG_INT doesn't get reset unnecessarily there are a number
>> of cases where restoring errflag to a previously saved value keeps the
>> ERRFLAG_INT bit if it got set in the meanwhile.  I hope the rationale
>> here is obvious --- the ERRFLAG_ERROR is an internal state that needs
>> resetting, the ERRFLAG_INT an asynchronous condition where the user
>> doesn't care what the internal state is.
>>
>> By the way, looking at the patch below you might wonder if it wouldn't
>> be more efficient to add a separate flag for interrupt error conditions
>> to test.  It wouldn't --- there are many more cases where errflag is
>> tested than when it is set, not affected by the patch below.
>>
>> I suspect we'll just have to try this out and see how it works.
>
> This seems to work well for me in the cases you talked about, but I
> quickly noticed one surprising problem. I have some stuff in my
> chpwd() hook to show git branches and stuff, and these used to be
> interruptible by ctrl-c (the commands are very fast with hot cache,
> but can be somewhat painful with cold cache, like 5-10 seconds delay).
> With the patch, I cannot interrupt them (sometimes?).
>
> chpwd () {
>     stty -echo >&/dev/null
>     test -f .tdldb && tdll -1 >&2
>     test -d .git && {
>         git branch
>         test -d .git/svn && {
>             echo -n r
>             git svn find-rev master
>         }
>         git name-rev HEAD
>         git describe --tags HEAD 2> /dev/null
>     } >&2
>     if [[ "$_NONOCDLS" = 1 ]]
>     then
>         ls $LS_OPTIONS >&2
>     fi
> }

Ah, I think I understand what's happening now. Prior to the patch,
pressing ctrl-c would abort out of chpwd() completely, but now it just
aborts whichever single command is running. Since I have three git
commands in there, I now need to press ctrl-c three times to get back
to the prompt quickly. (I would like it to only require one).

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sat, Dec 6, 2014 at 1:52 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sat, Dec 6, 2014 at 1:36 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Fri, Dec 5, 2014 at 11:07 PM, Peter Stephenson
>> <p.w.stephenson@ntlworld.com> wrote:
>>> On Fri, 5 Dec 2014 20:34:17 +0000
>>> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>>>> On the other problem I came up with, that eval is resetting errflag even
>>>> if you've interrupted: how about the following?  Add a bit to errflag to
>>>> signify that the user interrupted the shell rather than that some
>>>> internal error (e.g. syntax) occurred.  Only reset this new bit in a few
>>>> key places: the main command loop when executing, the top of ZLE when
>>>> editing being the obvious places.  Convert other "errflag = 0"
>>>> assignments case by case so that they just remove bit 0; then eval can
>>>> continue to do its job of acting as a sandbox but without screwing up
>>>> the behaviour of interrupts.  I think doing that is fairly mechanical
>>>> and it achieves what's needed without compromising anything else.
>>>
>>> Here's my first go; it does seem to do what I want, and as a by-product
>>> fixes all the little race conditions we've always had that meant you
>>> couldn't interrupt chunks of code that were executed in any kind of
>>> sandbox because the condition got reset afterwards.  I think a few of
>>> these have been annoying me over the years.
>>>
>>> The general strategy is to use the bit ERRFLAG_ERROR for internal
>>> failures and ERRFLAG_INT for user interrupts.  There are only two
>>> cases of the latter: on an untrapped SIGINT, the obvious case, and also
>>> on a trapped SIGINT or SIGQUIT where we've been told to behave as if the
>>> trap didn't trap the error condition.  That's straightforward for
>>> SIGINT, less so for SIGQUIT but I took my cue from the fact that Bart
>>> thought it worthwhile trapping SIGQUIT as an interactive "no, I really
>>> mean abort" in completion, which implies that if we trap it we want it
>>> to work at least as well as SIGINT.
>>>
>>> Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
>>> reset.  ERRFLAG_INT gets reset only in the following cases:
>>>
>>> - in the main command loop.  This is what causes the shell not to exit but
>>> instead go back to the main command loop when you ^C a command.
>>>
>>> - at the start of zleread, so we can read the next thing to do whatever
>>> just happened.  I'm not sure this is particularly useful since
>>> in this case you'd typically expect the previous condition to have
>>> occurred and it could mean e.g. you ignore an interrupt that
>>> occurred just before a "vared".
>>>
>>> - when we just finished completion.  This is needed so that the cases
>>> that got this whole business kicked off behave as now (but more
>>> reliably) --- a ^C gets you back to the command line, but the command
>>> line is not trashed as it would be if you ^Ced outside completion (try
>>> it if you're confused).  There's a race here, but it's no worse than it
>>> ever was.
>>>
>>> To ensure ERRFLAG_INT doesn't get reset unnecessarily there are a number
>>> of cases where restoring errflag to a previously saved value keeps the
>>> ERRFLAG_INT bit if it got set in the meanwhile.  I hope the rationale
>>> here is obvious --- the ERRFLAG_ERROR is an internal state that needs
>>> resetting, the ERRFLAG_INT an asynchronous condition where the user
>>> doesn't care what the internal state is.
>>>
>>> By the way, looking at the patch below you might wonder if it wouldn't
>>> be more efficient to add a separate flag for interrupt error conditions
>>> to test.  It wouldn't --- there are many more cases where errflag is
>>> tested than when it is set, not affected by the patch below.
>>>
>>> I suspect we'll just have to try this out and see how it works.
>>
>> This seems to work well for me in the cases you talked about, but I
>> quickly noticed one surprising problem. I have some stuff in my
>> chpwd() hook to show git branches and stuff, and these used to be
>> interruptible by ctrl-c (the commands are very fast with hot cache,
>> but can be somewhat painful with cold cache, like 5-10 seconds delay).
>> With the patch, I cannot interrupt them (sometimes?).
>>
> Ah, I think I understand what's happening now. Prior to the patch,
> pressing ctrl-c would abort out of chpwd() completely, but now it just
> aborts whichever single command is running. Since I have three git
> commands in there, I now need to press ctrl-c three times to get back
> to the prompt quickly. (I would like it to only require one).

Another difference: the menu completion listing could previously be
aborted with ctrl-c and keep the command line. It now closes the
listing and aborts the command line. Additionally, with menu
selection, you could previously ctrl-c out of selection and get to the
menu, ctrl-c that again, and still have the command line. Now you just
go straight from selection to a new empty command line.

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

I was extremely busy yesterday and couldn't reply to this stuff in the
order it was occurring, so apologies while I catch up back-to-front,
so to speak.

On Dec 6, 12:49pm, Mikael Magnusson wrote:
} Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
}
} >>> I suspect we'll just have to try this out and see how it works.
} >>
} >> This seems to work well for me in the cases you talked about, but I
} >> quickly noticed one surprising problem. I have some stuff in my
} >> chpwd() hook to show git branches and stuff, and these used to be
} >> interruptible by ctrl-c (the commands are very fast with hot cache,
} >> but can be somewhat painful with cold cache, like 5-10 seconds delay).
} >> With the patch, I cannot interrupt them (sometimes?).
} >>
} > Ah, I think I understand what's happening now. Prior to the patch,
} > pressing ctrl-c would abort out of chpwd() completely, but now it just
} > aborts whichever single command is running. Since I have three git
} > commands in there, I now need to press ctrl-c three times to get back
} > to the prompt quickly. (I would like it to only require one).

This is almost certainly a thinko (or a missed comparison of the value of
errflag) somewhere in the errflag patch, as it implies that errflag is
NOT remaining set, and I can't come up with why using a different value
for interrupts would cause that.

} Another difference: the menu completion listing could previously be
} aborted with ctrl-c and keep the command line. It now closes the
} listing and aborts the command line. Additionally, with menu
} selection, you could previously ctrl-c out of selection and get to the
} menu, ctrl-c that again, and still have the command line. Now you just
} go straight from selection to a new empty command line.

Does this happen ...

(a) with the "trap ... INT" -> "TRAPINT()" change in _main_complete?  Or

(b) with PWS's change to errflag?  Or

(c) only with both?

I suspect this is related to why I originally used the "trap" form -- I
will have to refresh my memory, but I think having the trap run in the
context of the caller was important in some way.  (Reset in subshells
may also be a factor.)

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 5,  3:17pm, Jeremie Roquet wrote:
} Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
}
} > -    if (!q)
} > +    if (!q || errflag)
} 
} I've no Idea if the expansion of something like "ls /a/b/c<tab>" to
} "ls /aaaaa/bbbbbb/cccccc/" is related to this code, but it seems to
} have zero impact there: I'm still unable to interrupt the horribly
} slow expansion using ctrl+c.

Do you have a matcher-list zstyle?

In the "trap ... INT" formulation of the trap in _main_complete, the
loop that retries each of the completers for every element of the
matcher-list does not get interrupted by ^C, even though the pass
that is in progress when you interrupt does stop.

This means you have to hit ^C at least once for every matcher, and maybe
also once for every completer that does something time-consuming.

The formulation using TRAPINT() blows all the way out of _main_complete,
which certainly stops the completion at that point but also screws up
all sorts of cleanup that _main_complete would normally do, such as
setting up _lastcomp, and restoring the values of ZLS_COLORS and the
_comppostfuncs array.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

Small clarification ...

On Dec 6,  1:50pm, Bart Schaefer wrote:
}
} The formulation using TRAPINT() blows all the way out of _main_complete,

In the absence of an "eval" somewhere in the call sequence, that is.

} which certainly stops the completion at that point but also screws up
} all sorts of cleanup that _main_complete would normally do, such as
} setting up _lastcomp, and restoring the values of ZLS_COLORS and the
} _comppostfuncs array.

Of course THOSE problems existed even before the "trap ... INT" was put
into _main_complete, so TRAPINT() is in that sense no worse than what
we had for years.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 5,  4:32pm, Ray Andrews wrote:
} Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
}
} On 12/05/2014 02:07 PM, Peter Stephenson wrote:
} >
} >> continue to do its job of acting as a sandbox but without screwing up
}
} What is a sandbox?

A place where you can safely play without breaking anything important,
and where most of the mess is safely contained.

In this context, that means a way to execute a command that may fail in
an unexpected or at least unpredictable way, without having that failure
change the rest of the shell state.

} So there was no distinction between an internal error and a user break?
} I thought the philosophy was to go overboard making very fine
} distinctions between various breaks, and that would seem to be
} the very first one to make.

Problem here is that there really is no such thing as a "user break."

There is the arrival of a signal from the operating system; some of the
time you can assume that certain signals must have been triggered by
something a user did, but the OS doesn't distinguish the source of the
signal, only the type of signal.  The INTterrupt signal is *usually*
generated from the user's keyboard input, but doesn't have to be.

What Peter proposes is that zsh assume the interrupt always came from
the user, even though we don't really know.  Previously zsh behaved as
if ignorant, and if it was able to continue after an error, it did so.

As subsequent reports from Mikael have indicated, attempting to make
this distinction is going to be fraught with unexpected difficulties.
We're mucking with things pretty fundamental to the shell's operation.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 6,  1:40am, Mikael Magnusson wrote:
}
} On Sat, Dec 6, 2014 at 1:36 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
} > brunch  -- alias for '!git branch -v|sed -e 's/t/ /g' -e 's/(.{'$((
} 
} This is the .gitconfig line, didn't notice the completion listing
} mungled it up a bit.

Hmm, somebody needs some more quoting on what they pass to compadd?

} Not likely that it matters, but I don't want you
} guys to think I'm some kind of crazy person that replaces the letter t
} with spaces in commit messages.

We might think yourtkeyboardthastatbrokentspacebar ...

Re: Interrupting globs (Re: Something rotten in tar completion)

[Ray Andrews]

On 12/06/2014 02:27 PM, Bart Schaefer wrote:
> On Dec 5,  4:32pm, Ray Andrews wrote:
> } Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
> }
> } On 12/05/2014 02:07 PM, Peter Stephenson wrote:
> } >
> } >> continue to do its job of acting as a sandbox but without screwing up
> }
> } What is a sandbox?
>
> A place where you can safely play without breaking anything important,
> and where most of the mess is safely contained.
>
> In this context, that means a way to execute a command that may fail in
> an unexpected or at least unpredictable way, without having that failure
> change the rest of the shell state.
An excellent metaphor then.  Sounds like a very good thing to have.
>
> } So there was no distinction between an internal error and a user break?
> } I thought the philosophy was to go overboard making very fine
> } distinctions between various breaks, and that would seem to be
> } the very first one to make.
>
> Problem here is that there really is no such thing as a "user break."
>
> There is the arrival of a signal from the operating system; some of the
> time you can assume that certain signals must have been triggered by
> something a user did, but the OS doesn't distinguish the source of the
> signal, only the type of signal.  The INTterrupt signal is *usually*
> generated from the user's keyboard input, but doesn't have to be.
>
Sheesh.  In DOS ^C is always a hardware interrupt, plain and simple.
It's read at the BIOS level. So we can't/don't just check the kb buffer?
It seems strange that the OS has no special flag for a keyboard
generated interupt.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sat, Dec 6, 2014 at 6:48 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> I was extremely busy yesterday and couldn't reply to this stuff in the
> order it was occurring, so apologies while I catch up back-to-front,
> so to speak.
>
> On Dec 6, 12:49pm, Mikael Magnusson wrote:
> } Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
> }
> } >>> I suspect we'll just have to try this out and see how it works.
> } >>
> } >> This seems to work well for me in the cases you talked about, but I
> } >> quickly noticed one surprising problem. I have some stuff in my
> } >> chpwd() hook to show git branches and stuff, and these used to be
> } >> interruptible by ctrl-c (the commands are very fast with hot cache,
> } >> but can be somewhat painful with cold cache, like 5-10 seconds delay).
> } >> With the patch, I cannot interrupt them (sometimes?).
> } >>
> } > Ah, I think I understand what's happening now. Prior to the patch,
> } > pressing ctrl-c would abort out of chpwd() completely, but now it just
> } > aborts whichever single command is running. Since I have three git
> } > commands in there, I now need to press ctrl-c three times to get back
> } > to the prompt quickly. (I would like it to only require one).
>
> This is almost certainly a thinko (or a missed comparison of the value of
> errflag) somewhere in the errflag patch, as it implies that errflag is
> NOT remaining set, and I can't come up with why using a different value
> for interrupts would cause that.
>
> } Another difference: the menu completion listing could previously be
> } aborted with ctrl-c and keep the command line. It now closes the
> } listing and aborts the command line. Additionally, with menu
> } selection, you could previously ctrl-c out of selection and get to the
> } menu, ctrl-c that again, and still have the command line. Now you just
> } go straight from selection to a new empty command line.
>
> Does this happen ...
>
> (a) with the "trap ... INT" -> "TRAPINT()" change in _main_complete?  Or
>
> (b) with PWS's change to errflag?  Or
>
> (c) only with both?
>
> I suspect this is related to why I originally used the "trap" form -- I
> will have to refresh my memory, but I think having the trap run in the
> context of the caller was important in some way.  (Reset in subshells
> may also be a factor.)

It doesn't happen with only (a). It does happen with only (b). (c)
behaves the same as (b), as far as I can tell.

@@ -883,7 +883,7 @@ getbyte(long do_keytmout, int *timeout)
         die = 0;
         if (!errflag && !retflag && !breaks && !exit_pending)
             continue;
-        errflag = 0;
+        errflag &= ~ERRFLAG_ERROR;
         breaks = obreaks;
         errno = old_errno;
         return lastchar = EOF;

Undoing only this hunk fixes this for me. I can't find anything that
stops being interruptible but I only tried for a minute.


@@ -1444,12 +1444,7 @@ zwaitjob(int job, int wait_cmd)
         restore_queue_signals(q);
         return 128 + last_signal;
         }
-        /* Commenting this out makes ^C-ing a job started by a function
-           stop the whole function again.  But I guess it will stop
-           something else from working properly, we have to find out
-           what this might be.  --oberon
-
-        errflag = 0; */
+        errflag &= ~ERRFLAG_ERROR;
         if (subsh) {
         killjb(jn, SIGCONT);
         jn->stat &= ~STAT_STOPPED;

And commenting that line back out fixes my chpwd() hook ctrl-c thing.
(or changing it to errflag &= ~ERRFLAG_INT; but I have no idea if that
makes any sense).

I'll run with these two changes for a while and see if anything pops out.

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

Trimming some context ... still a lot, tho ...

On Dec 7,  2:42am, Mikael Magnusson wrote:
} Subject: Re: Interrupting globs (Re: Something rotten in tar completion)
}
} > } > Ah, I think I understand what's happening now. Prior to the patch,
} > } > pressing ctrl-c would abort out of chpwd() completely, but now it just
} > } > aborts whichever single command is running.
} >
} > } Another difference: the menu completion listing could previously be
} > } aborted with ctrl-c and keep the command line. It now closes the
} > } listing and aborts the command line. Additionally, with menu
} > } selection, you could previously ctrl-c out of selection and get to the
} > } menu, ctrl-c that again, and still have the command line. Now you just
} > } go straight from selection to a new empty command line.
} >
} 
} @@ -883,7 +883,7 @@ getbyte(long do_keytmout, int *timeout)
}          die = 0;
}          if (!errflag && !retflag && !breaks && !exit_pending)
}              continue;
} -        errflag = 0;
} +        errflag &= ~ERRFLAG_ERROR;
}          breaks = obreaks;
}          errno = old_errno;
}          return lastchar = EOF;
} 
} Undoing only this hunk fixes this for me. I can't find anything that
} stops being interruptible but I only tried for a minute.

This probably indicates that one or more of the callers of getbyte() (or
someone even earlier in the stack) need to clear user interrupts, rather
than that they should be cleared here.

It makes some kind of sense that code which could previously assume that
errflag had been completely cleared, might after this patch need to
explicitly handle interrupt conditions.

I'm not sure how we find all such places, though.  Some of them might
not even test errflag, at present; many might do so.

} @@ -1444,12 +1444,7 @@ zwaitjob(int job, int wait_cmd)
}          restore_queue_signals(q);
}          return 128 + last_signal;
}          }
} -        /* Commenting this out makes ^C-ing a job started by a function
} -           stop the whole function again.  But I guess it will stop
} -           something else from working properly, we have to find out
} -           what this might be.  --oberon
} -
} -        errflag = 0; */
} +        errflag &= ~ERRFLAG_ERROR;
}          if (subsh) {
}          killjb(jn, SIGCONT);
}          jn->stat &= ~STAT_STOPPED;
} 
} And commenting that line back out fixes my chpwd() hook ctrl-c thing.
} (or changing it to errflag &= ~ERRFLAG_INT; but I have no idea if that
} makes any sense).

Both things fit with Oberon's comment, actually.  Based on the comment,
what we do NOT want to do there is clear ERRFLAG_INT.  (That comment has
been there for more than 25 years, so if something else were going to
have stopped working properly I think we'd have found out by now.)

Whether clearing internal errors at that point is also necessary is a
fresh mystery.  I guess I'd try leaving it &= ~ERRFLAG_INT for a while.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 6,  8:45pm, Bart Schaefer wrote:
} 
} } -        /* Commenting this out makes ^C-ing a job started by a function
} } -           stop the whole function again.  But I guess it will stop
} } -           something else from working properly, we have to find out
} } -           what this might be.  --oberon
} } -
} } -        errflag = 0; */
} } +        errflag &= ~ERRFLAG_ERROR;
} } 
} } And commenting that line back out fixes my chpwd() hook ctrl-c thing.
} } (or changing it to errflag &= ~ERRFLAG_INT; but I have no idea if that
} } makes any sense).
} 
} Both things fit with Oberon's comment, actually.  Based on the comment,
} what we do NOT want to do there is clear ERRFLAG_INT.

Wait ... I think I have something backward there.

errflag &= ~ERRFLAG_ERROR should leave ERRFLAG_INT unaffected, which is
what seems to be wanted.  So if ~ERRFLAG_INT (or no change at all) works,
then the important value of errflag there *is* ERRFLAG_ERR.

Which means something changed errflag from ERRFLAG_INT to ERRFLAG_ERR ?
But that doesn't make sense, looking at the patch.

OK, now I'm just confused.  Sorry.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

Whew, caught up I think ...

On Dec 5, 10:07pm, Peter Stephenson wrote:
}
} The general strategy is to use the bit ERRFLAG_ERROR for internal
} failures and ERRFLAG_INT for user interrupts.  There are only two
} cases of the latter: on an untrapped SIGINT, the obvious case, and also
} on a trapped SIGINT or SIGQUIT where we've been told to behave as if the
} trap didn't trap the error condition.  That's straightforward for
} SIGINT, less so for SIGQUIT but I took my cue from the fact that Bart
} thought it worthwhile trapping SIGQUIT as an interactive "no, I really
} mean abort" in completion, which implies that if we trap it we want it
} to work at least as well as SIGINT.

I would say that INT should be "less drastic" than QUIT.  For example
in the cases Mikael mentions in subsequent messages -- stopping menu
selection, etc. -- the ideal thing would be if QUIT broke all the way
out to a new command line while INT backed up only one level.  However,
I'm not sure how QUIT behaved before (or even whether it is enabled as
a keyboard-generated signal, since we don't have a handler for it) --
I trapped both INT and QUIT there because they both *could* come from
the keyboard, if stty were configured for it.

} Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
} reset.  ERRFLAG_INT gets reset [...]
} 
} - at the start of zleread, so we can read the next thing to do whatever
} just happened.  I'm not sure this is particularly useful

But the situation before this patch is the same, is it not?

} - when we just finished completion.  This is needed so that the cases
} that got this whole business kicked off behave as now (but more
} reliably) --- a ^C gets you back to the command line, but the command
} line is not trashed as it would be if you ^Ced outside completion (try
} it if you're confused).  There's a race here, but it's no worse than it
} ever was.

I think Mikael's example shows there are sub-cases of completion where we
need to add clearing of interrupts, rather than backing all the way out
of completion.

} To ensure ERRFLAG_INT doesn't get reset unnecessarily there are a number
} of cases where restoring errflag to a previously saved value keeps the
} ERRFLAG_INT bit if it got set in the meanwhile.  I hope the rationale
} here is obvious --- the ERRFLAG_ERROR is an internal state that needs
} resetting, the ERRFLAG_INT an asynchronous condition where the user
} doesn't care what the internal state is.

Are there any cases where errflag is unconditionally restored, or did you
change all such save/restore pairs?

Either the ternary is irrelevant here, or the "if (errflag)" is:

} @@ -2691,7 +2702,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
}  		    if (varspc)
}  			addvars(state, varspc, 0);
}  		    if (errflag)
} -			lastval = errflag;
} +			lastval = errflag ? 1 : 0;
}  		    else
}  			lastval = cmdoutval;
}  		    if (isset(XTRACE)) {

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

(Note, two competing patches herein, do not apply both.)

On Dec 5,  8:34pm, Peter Stephenson wrote:
}
} The trap goes back to zsh-workers/32322 (it got modified later but that
} was largely cosmetic).  The rationale there, it appears, was it was
} making difficulties in interrupting easier to test (by displaying the
} message, I presume) --- not that it was fixing anything.  So as far as I
} can see the underlying complaint was the same as mine, to return the
} shell to the point where I get control of the command line as soon as
} possible.  Am I missing something?

No, I'd forgotten the exact circumstances.  However, as I mentioned in
another part of the thread, there's something I missed as well, which
is that if you ^C all the way out of _main_complete, some interesting
(if not always important) bits of state don't get restored.

If we stick with the TRAPINT(), then we need this ...

diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
index 91b68fe..bc63e83 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -129,7 +129,7 @@ _completer_num=1
 # We assume localtraps to be in effect here ...
 integer SECONDS=0
 TRAPINT TRAPQUIT() {
-  zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
+  zle -M "Killed by signal in ${funcstack[2]} after ${SECONDS}s";
   zle -R
   return 130
 }

... and we also need an always-block or something to cover cleanup,
which may mean rearranging other parts of _main_complete.

Or we could do this, which stops the matcher-list and completer loops
(and any tag loops or the like that are also in play) and then does
the rest of _main_complete as usual:

diff --git a/Completion/Base/Core/_main_complete
b/Completion/Base/Core/_main_complete
index 91b68fe..5c4e368 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -126,13 +126,14 @@ fi
 
 _completer_num=1
 
-# We assume localtraps to be in effect here ...
-integer SECONDS=0
-TRAPINT TRAPQUIT() {
+# We assume localtraps and no_localloops to be in effect here ...
+float SECONDS=0
+trap '
   zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
   zle -R
+  repeat 1 break 10000	# break out of any reasonable number of loops
   return 130
-}
+' INT QUIT
 
 # Call the pre-functions.
 

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sun, Dec 7, 2014 at 6:59 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> Or we could do this, which stops the matcher-list and completer loops
> (and any tag loops or the like that are also in play) and then does
> the rest of _main_complete as usual:
>
> diff --git a/Completion/Base/Core/_main_complete
> b/Completion/Base/Core/_main_complete
> index 91b68fe..5c4e368 100644
> --- a/Completion/Base/Core/_main_complete
> +++ b/Completion/Base/Core/_main_complete
> @@ -126,13 +126,14 @@ fi
>
>  _completer_num=1
>
> -# We assume localtraps to be in effect here ...
> -integer SECONDS=0
> -TRAPINT TRAPQUIT() {
> +# We assume localtraps and no_localloops to be in effect here ...
> +float SECONDS=0
> +trap '
>    zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
>    zle -R
> +  repeat 1 break 10000 # break out of any reasonable number of loops
>    return 130
> -}
> +' INT QUIT
>
>  # Call the pre-functions.

shortloops is not in _comp_options, so this needs to be repeat 1; do
break 10000; done, if you go with this one.

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sat, 06 Dec 2014 21:59:11 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> No, I'd forgotten the exact circumstances.  However, as I mentioned in
> another part of the thread, there's something I missed as well, which
> is that if you ^C all the way out of _main_complete, some interesting
> (if not always important) bits of state don't get restored.
> 
> If we stick with the TRAPINT(), then we need this ...
> 
> diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
> index 91b68fe..bc63e83 100644
> --- a/Completion/Base/Core/_main_complete
> +++ b/Completion/Base/Core/_main_complete
> @@ -129,7 +129,7 @@ _completer_num=1
>  # We assume localtraps to be in effect here ...
>  integer SECONDS=0
>  TRAPINT TRAPQUIT() {
> -  zle -M "Killed by signal in ${funcstack[1]} after ${SECONDS}s";
> +  zle -M "Killed by signal in ${funcstack[2]} after ${SECONDS}s";
>    zle -R
>    return 130
>  }

That's probably the way to go --- my gut feel is the behaviour of the
other form of trap is too far from what we need to make it a good
starting point.

I'll get onto the discussion about internal changes separately.  Rather
than replying to everything separately, I might try to summarise what I
think we've learnt so far and you can tell me what's missing...

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

I'm going to try and summarise things so far.

Code:

I've just pushed a branch "interrupt_abort" to the server which is top
of tree with (so far) just my initial patch, which we know to be far
from the end of the story.  (It's *not* on the master branch everyone is
currently using.)  I'd suggest anyone with a weak stomach or limited
grasp of what the issue actually is (I mean, even more limited than
Bart, Mikael and me :-/) keep off this for now.  I won't bother with a
ChangeLog for this branch, just a git log; if we think something useful
emerges, which isn't 100% obvious, I'll squash it and rebase it on to
master when the time comes.  (I'm now official head git for firmware at
work so I've been learning heavily about this stuff.)

Mikael and Bart and the usual suspects should certainly feel free to
commit here if they want to play.

Other issues:

Mikael needs to ^C three times to get back to the command line ---
apparently related to the code from Sven I took at its word.  I'm
confused here, too; I put back removing non-interrupt errors (I'll just
refer to errors and interrupts from now on though I realise that's a bit
glib) and it seems to be resetting interrupts somehow.  For now the fix might be
simply put back in the previous state with Sven's statement that it once
did something, plus an additional comment that we're confused, and come back to this when we understand the general picture later.

Where to reset interrupt state in the editor, in particular to enable
aborts to menu completion: getbyte() or somewhere more specific?  It's
possible the getbyte() case suggested by Mikael is actually a reasonable
place to do this.  The argument would be that this is where the user
next gets the chance to do some editing again.  However, maybe that
screws up some form of recursive reading of stuff?  Certainly Bart's
remark, that there are sub-cases we need to add, was also my first
inclination and perhaps we ought to try that first as it's a bit more
controlled.

INT vs. QUIT --- no real feel for this, and I'm not sure how important
it is in practice, but I think the change I made, where a TRAPQUIT that
returns non-zero sets ERRFLAG_INT, should at least improve things if
we're going to use TRAPQUIT at all, since that wouldn't have flagged an
error before --- there was no special SIGQUIT handling, so telling the
signal handler to behave as it normally would doesn't help; normally it
wouldn't have handled SIGQUIT at all.  (I suppose if we wanted to be
really consistent we'd have exited the shell after handling a TRAPQUIT
with a non-zero return value?  That's obviously a separate issue.)

More general points:

The current code, both in completion functions an internally, don't give
us much control over cleaning up on a keyboard interrupt (and didn't before
--- Bart): yes, ick.  I don't think shells do this very well generally.
I agree we might want to look at some always blocks.  BTW, I didn't
actually think through the handling of "always" blocks for this patch,
so it might want looking at.

It's also just occurred to me I may have introduced some rare but
entirely possible read-modify-write races because we set the ERRFLAG_INT
bit in interrupts and set the other and clear both bits separately in
the main shell.  I guess it would be better to queue interrupts whenever
we add or remove a single bit of errflag; that's probably not often
enough to cause efficiency issues since should only be round significant
chunks of shell code, or when an error has actually occurred.  Opinions?

> Are there any cases where errflag is unconditionally restored, or did you
> change all such save/restore pairs?

None of the cases of restoration unconditionally restore completely any
more, they all keep the interrupt bit if it got set in the meanwhileX.
I think we'd have to identify a place where it's sufficiently obvious to
a user hitting ^C that they're going back to that point, i.e. it's not
just an implementation detail in shell code as far as the person at the
keyboard is concerned.  I didn't spot one that looked plausible --- the
cases where we want to strip the interrupt bit don't seem to be the same
ones where we restore the previous error status.  But I could easily
have missed one.

> Either the ternary is irrelevant here, or the "if (errflag)" is:

Yes, the code inside the test should be "lastval = 1".  I'll commit
that.

I'll post other things I think want to go on the interrupt_abort branch
before I commit those.

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sun, 7 Dec 2014 17:07:13 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> Mikael needs to ^C three times to get back to the command line ---
> apparently related to the code from Sven I took at its word.  I'm
> confused here, too; I put back removing non-interrupt errors (I'll
> just refer to errors and interrupts from now on though I realise
> that's a bit glib) and it seems to be resetting interrupts somehow.
> For now the fix might be simply put back in the previous state with
> Sven's statement that it once did something, plus an additional
> comment that we're confused, and come back to this when we understand
> the general picture later.

That's this, which I will commit onto the interrupt_abort branch and
push since as noted it just puts the code the way it's been for about
two decades.

Just be to sure, can Mikael confirm this does remove the need for the
extra ^Cs?  Thanks.

pws

diff --git a/Src/jobs.c b/Src/jobs.c
index 6e47e5e..3c2a21a 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1444,7 +1444,19 @@ zwaitjob(int job, int wait_cmd)
 		restore_queue_signals(q);
 		return 128 + last_signal;
 	    }
-	    errflag &= ~ERRFLAG_ERROR;
+           /* Commenting this out makes ^C-ing a job started by a function
+              stop the whole function again.  But I guess it will stop
+              something else from working properly, we have to find out
+              what this might be.  --oberon
+
+	      When attempting to separate errors and interrupts, we
+	      assumed because of the previous comment it would be OK
+	      to remove ERRFLAG_ERROR and leave ERRFLAG_INT set, since
+	      that's the one related to ^C.  But that doesn't work.
+	      There's something more here we don't understand.  --pws
+
+           errflag = 0; */
+
 	    if (subsh) {
 		killjb(jn, SIGCONT);
 		jn->stat &= ~STAT_STOPPED;

Re: Interrupting globs (Re: Something rotten in tar completion)

[Oliver Kiddle]

Peter wrote:
> 
> It's also just occurred to me I may have introduced some rare but
> entirely possible read-modify-write races because we set the ERRFLAG_INT
> bit in interrupts and set the other and clear both bits separately in
> the main shell.  I guess it would be better to queue interrupts whenever
> we add or remove a single bit of errflag; that's probably not often
> enough to cause efficiency issues since should only be round significant
> chunks of shell code, or when an error has actually occurred.  Opinions?

Could that perhaps be solved by making errflag a volatile sig_atomic_t
instead of an int?

Oliver

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sat, 6 Dec 2014 12:49:19 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Another difference: the menu completion listing could previously be
> aborted with ctrl-c and keep the command line. It now closes the
> listing and aborts the command line.

Here's a suggestion: when we go back to the main keymap, assume that if
the user typed ^C in the mean time they want to continue editing from
this point.  I think that fixes this case.

(This is only on the interrupt_abort branch, again.)

> Additionally, with menu selection, you could previously ctrl-c out of
> selection and get to the menu, ctrl-c that again, and still have the
> command line. Now you just go straight from selection to a new empty
> command line.

This second case was already working for me (interrupt_abort branch with
Sven-comment-restoration, commit e385312e0937).  I tried both
MENU_COMPLETE and AUTO_COMPLETE behaviour.  There may be another way in.
Does the patch below help with this, too?

I'm intending to commit this and then stop patching to allow us to take
stock of what effects are now present on the interrupt_abort branch.  So
feel free to re-report any bad features.

pws

diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
index 30d25eb..48f210c 100644
--- a/Src/Zle/zle_keymap.c
+++ b/Src/Zle/zle_keymap.c
@@ -504,6 +504,16 @@ mod_export void
 selectlocalmap(Keymap m)
 {
     localkeymap = m;
+    if (!m)
+    {
+	/*
+	 * No local keymap; so we are returning to the global map.  If
+	 * the user ^Ced in the local map, they probably just want to go
+	 * back to normal editing.  So remove the interrupt error
+	 * status.
+	 */
+	errflag &= ~ERRFLAG_INT;
+    }
 }
 
 /* Reopen the currently selected keymap, in case it got deleted.  This *

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sun, 07 Dec 2014 18:37:33 +0100
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Peter wrote:
> > 
> > It's also just occurred to me I may have introduced some rare but
> > entirely possible read-modify-write races because we set the ERRFLAG_INT
> > bit in interrupts and set the other and clear both bits separately in
> > the main shell.  I guess it would be better to queue interrupts whenever
> > we add or remove a single bit of errflag; that's probably not often
> > enough to cause efficiency issues since should only be round significant
> > chunks of shell code, or when an error has actually occurred.  Opinions?
> 
> Could that perhaps be solved by making errflag a volatile sig_atomic_t
> instead of an int?

It should probably at least be volatile anyway, given it's set in
interrupts regardless of the interrupt_abort changes.

It's not clear to me this is good enough to fix read/modify/write races;
as far as I can see the intention is this makes reads and writes
separately atomic.  However, I'm not an expert on this so I may be
wrong.

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 7,  5:07pm, Peter Stephenson wrote:
}
} Other issues:
} 
} Mikael needs to ^C three times to get back to the command line ---
} apparently related to the code from Sven I took at its word.

(Sven is Oberon?  If that's true, I'd entirely forgotten it.)

} I agree we might want to look at some always blocks.  BTW, I didn't
} actually think through the handling of "always" blocks for this patch,
} so it might want looking at.

In particular with respect to always-blocks, there should be some way
for TRY_BLOCK_ERROR to clear interrupts as well as (maybe even instead
of?  TRY_BLOCK_INTERRUPT?) internal errors.  A trap that returns zero
only clears interrupt if there isn't another trap in a deeper scope.

Maybe TRY_BLOCK_ERROR does still clear both in the patch, I didn't
parse it that carefully.

} It's also just occurred to me I may have introduced some rare but
} entirely possible read-modify-write races because we set the ERRFLAG_INT
} bit in interrupts and set the other and clear both bits separately in
} the main shell.  I guess it would be better to queue interrupts whenever
} we add or remove a single bit of errflag

I don't think single bits are a problem because it's always done with
bitwise-or.  If anything, we'd need to queue any time we clear the
interrupt bit, so that a signal that comes in while we're clearing it
gets treated as if arrived after clearing.  In practice I don't think
it makes that much difference.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

Bart Schaefer wrote:
> On Dec 7,  5:07pm, Peter Stephenson wrote:
> }
> } Other issues:
> } 
> } Mikael needs to ^C three times to get back to the command line ---
> } apparently related to the code from Sven I took at its word.
> 
> (Sven is Oberon?  If that's true, I'd entirely forgotten it.)

I'm pretty sure I didn't make that up.
 
> } I agree we might want to look at some always blocks.  BTW, I didn't
> } actually think through the handling of "always" blocks for this patch,
> } so it might want looking at.
> 
> In particular with respect to always-blocks, there should be some way
> for TRY_BLOCK_ERROR to clear interrupts as well as (maybe even instead
> of?  TRY_BLOCK_INTERRUPT?) internal errors.  A trap that returns zero
> only clears interrupt if there isn't another trap in a deeper scope.

Yes, probably.  I realised we'd better not do that with bits in the shell
code as the documentation suggests setting TRY_BLOCK_ERROR to 0 clears
errors.  We'd want clearing interrupts to be a separate explicit action.

> Maybe TRY_BLOCK_ERROR does still clear both in the patch, I didn't
> parse it that carefully.

The current code causes ERRFLAG_INT to propagate and only updates
ERRFLAG_ERROR owing to TRY_BLOCK_ERROR.  I suppose that's correct for
the basic operation, i.e. unless we do some something special as above
interrupts propagate.

However, I think it definitely needs to clear ERRFLAG_INT for the
duration of the always clause, or the code won't execute at all, even if
it restores it later.  The patch below does this and notes the other issue.

> } It's also just occurred to me I may have introduced some rare but
> } entirely possible read-modify-write races because we set the ERRFLAG_INT
> } bit in interrupts and set the other and clear both bits separately in
> } the main shell.  I guess it would be better to queue interrupts whenever
> } we add or remove a single bit of errflag
> 
> I don't think single bits are a problem because it's always done with
> bitwise-or.  If anything, we'd need to queue any time we clear the
> interrupt bit, so that a signal that comes in while we're clearing it
> gets treated as if arrived after clearing.  In practice I don't think
> it makes that much difference.

I'm worried about the various occurrences of this:

	    /* Keep any user interrupt error status */
	    errflag = oef | (errflag & ERRFLAG_INT);

which can cause

- read errflag to determine ERRFLAG_INT bit; find it's not set
- interrupt sets ERRFLAG_INT
- errflag gets set to value without ERRFLAG_INT.

It's a narrow race, but I think it's a real one.

pws

diff --git a/Src/loop.c b/Src/loop.c
index 69805ea..9b0a8d7 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -643,7 +643,7 @@ exectry(Estate state, int do_exec)
 {
     Wordcode end, always;
     int endval;
-    int save_retflag, save_breaks, save_contflag;
+    int save_retflag, save_breaks, save_contflag, try_interrupt;
     zlong save_try_errflag, save_try_tryflag;
 
     end = state->pc + WC_TRY_SKIP(state->pc[-1]);
@@ -671,8 +671,10 @@ exectry(Estate state, int do_exec)
 
     /* The always clause. */
     save_try_errflag = try_errflag;
-    try_errflag = (zlong)errflag;
-    errflag &= ~ERRFLAG_ERROR;
+    try_errflag = (zlong)(errflag & ERRFLAG_ERROR);
+    try_interrupt = errflag & ERRFLAG_INT;
+    /* We need to reset all errors to allow the block to execute */
+    errflag = 0;
     save_retflag = retflag;
     retflag = 0;
     save_breaks = breaks;
@@ -687,6 +689,17 @@ exectry(Estate state, int do_exec)
 	errflag |= ERRFLAG_ERROR;
     else
 	errflag &= ~ERRFLAG_ERROR;
+    /*
+     * TODO: currently, we always restore the interrupt
+     * error status.  We should have a way of clearing it.
+     * Doing this with try_errflag (the shell variable TRY_BLOCK_ERROR)
+     * is probably not a good idea since currently that's documented
+     * such that setting it to 0 clears errors, and we don't want
+     * to clear interrupts as a side effect.  So it probably needs
+     * a different variable.
+     */
+    if (try_interrupt)
+	errflag |= ERRFLAG_INT;
     try_errflag = save_try_errflag;
     if (!retflag)
 	retflag = save_retflag;

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 7,  6:59pm, Peter Stephenson wrote:
}
} I'm worried about the various occurrences of this:
} 
} 	    /* Keep any user interrupt error status */
} 	    errflag = oef | (errflag & ERRFLAG_INT);

I believe that could be rewritten as

	errflag &= ~ERRFLAG_ERROR;
	errflag |= oef;

to avoid reading errflag and then writing back to it.  That's likely
more efficient than queueing/unqueing signals.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sat, 06 Dec 2014 21:18:28 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } Correspondingly, most of the time only the ERRFLAG_ERROR bit gets
> } reset.  ERRFLAG_INT gets reset [...]
> } 
> } - at the start of zleread, so we can read the next thing to do whatever
> } just happened.  I'm not sure this is particularly useful
> 
> But the situation before this patch is the same, is it not?

Didn't reply to this bit in my attempted summary.

Yes, as I've only reduced the places where it gets reset it shouldn't
cause any new problems.  Theoretically...

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sun, 07 Dec 2014 18:59:04 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> > In particular with respect to always-blocks, there should be some way
> > for TRY_BLOCK_ERROR to clear interrupts as well as (maybe even instead
> > of?  TRY_BLOCK_INTERRUPT?) internal errors.  A trap that returns zero
> > only clears interrupt if there isn't another trap in a deeper scope.
> 
> Yes, probably.  I realised we'd better not do that with bits in the shell
> code as the documentation suggests setting TRY_BLOCK_ERROR to 0 clears
> errors.  We'd want clearing interrupts to be a separate explicit action.

I think this is a mechanical addition of a parallel variable, in as much
as anything is mechanical here...  Not entirely sure when we'd want to
use it, but it would get us out of jail free if the new interrupt_abort
code is riding roughshod over some shell code where we need to ensure
the state gets fully restored, so is probably worth having.

I suppose, theoretically, this means we can fix up any problems in the
new C code within shell code.  (It's now a decade and a half since I
gave up theoretical physics, but I still interpret the word
"theoretically" as meaning "I get paid to do this whatever happens in
the real world".  As I don't get paid to do this, the meaning is even
stronger.)

pws


diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 5833d6b..8da0c64 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -754,6 +754,14 @@ It may be reset, clearing the error condition.  See
 ifzman(em(Complex Commands) in zmanref(zshmisc))\
 ifnzman(noderef(Complex Commands))
 )
+vinde(TRY_BLOCK_INTERRUPT)
+item(tt(TRY_BLOCK_INTERRUPT) <S>)(
+This variable works in a similar way to tt(TRY_BLOCK_ERROR), but
+represents the status of an interrupt from the keyboard, typically
+by the user typing tt(^C).  If set to 0, any such interrupt will
+be reset; otherwise, the interrupt is propagated after the tt(always)
+block.
+)
 vindex(TTY)
 item(tt(TTY))(
 The name of the tty associated with the shell, if any.
diff --git a/Src/loop.c b/Src/loop.c
index 9b0a8d7..4daa605 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -633,6 +633,14 @@ execcase(Estate state, int do_exec)
 zlong
 try_errflag = -1;
 
+/**
+ * Corrresponding interrupt error status form `try' block.
+ */
+
+/**/
+zlong
+try_interrupt = -1;
+
 /**/
 zlong
 try_tryflag = 0;
@@ -643,8 +651,8 @@ exectry(Estate state, int do_exec)
 {
     Wordcode end, always;
     int endval;
-    int save_retflag, save_breaks, save_contflag, try_interrupt;
-    zlong save_try_errflag, save_try_tryflag;
+    int save_retflag, save_breaks, save_contflag;
+    zlong save_try_errflag, save_try_tryflag, save_try_interrupt;
 
     end = state->pc + WC_TRY_SKIP(state->pc[-1]);
     always = state->pc + 1 + WC_TRY_SKIP(*state->pc);
@@ -671,8 +679,9 @@ exectry(Estate state, int do_exec)
 
     /* The always clause. */
     save_try_errflag = try_errflag;
+    save_try_interrupt = try_interrupt;
     try_errflag = (zlong)(errflag & ERRFLAG_ERROR);
-    try_interrupt = errflag & ERRFLAG_INT;
+    try_interrupt = (zlong)((errflag & ERRFLAG_INT) ? 1 : 0);
     /* We need to reset all errors to allow the block to execute */
     errflag = 0;
     save_retflag = retflag;
@@ -689,18 +698,12 @@ exectry(Estate state, int do_exec)
 	errflag |= ERRFLAG_ERROR;
     else
 	errflag &= ~ERRFLAG_ERROR;
-    /*
-     * TODO: currently, we always restore the interrupt
-     * error status.  We should have a way of clearing it.
-     * Doing this with try_errflag (the shell variable TRY_BLOCK_ERROR)
-     * is probably not a good idea since currently that's documented
-     * such that setting it to 0 clears errors, and we don't want
-     * to clear interrupts as a side effect.  So it probably needs
-     * a different variable.
-     */
     if (try_interrupt)
 	errflag |= ERRFLAG_INT;
+    else
+	errflag &= ~ERRFLAG_INT;
     try_errflag = save_try_errflag;
+    try_interrupt = save_try_interrupt;
     if (!retflag)
 	retflag = save_retflag;
     if (!breaks)
diff --git a/Src/params.c b/Src/params.c
index bdace79..79088d1 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -331,6 +331,7 @@ IPDEF5("SHLVL", &shlvl, varinteger_gsu),
 #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0}
 IPDEF6("OPTIND", &zoptind, varinteger_gsu),
 IPDEF6("TRY_BLOCK_ERROR", &try_errflag, varinteger_gsu),
+IPDEF6("TRY_BLOCK_INTERRUPT", &try_interrupt, varinteger_gsu),
 
 #define IPDEF7(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL},BR((void *)B),GSU(varscalar_gsu),0,0,NULL,NULL,NULL,0}
 #define IPDEF7U(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL|PM_UNSET},BR((void *)B),GSU(varscalar_gsu),0,0,NULL,NULL,NULL,0}

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 7,  8:20pm, Peter Stephenson wrote:
}
} +vinde(TRY_BLOCK_INTERRUPT)
} +item(tt(TRY_BLOCK_INTERRUPT) <S>)(
} +This variable works in a similar way to tt(TRY_BLOCK_ERROR), but
} +represents the status of an interrupt from the keyboard typically
} +by the user typing tt(^C).  If set to 0, any such interrupt will
} +be reset; otherwise, the interrupt is propagated after the tt(always)
} +block.

I worry a little about describing it like this.  It's an INT signal,
but whether it came "from the keyboard" is not really known.  If you
move the word "typically" to before the word "from", the description
becomes more accurate.

It might also be worth mentioning that if an interrupt happens DURING
the always-block, then the always block will stop and THAT interrupt
is also propagated.

This all could lead to code like this:

    {
      : Dammit, stop interrupting me
    } always {
      () {
	integer queued_signal=0
	() {
	  setopt localoptions localtraps
	  TRAPINT() {
	    (( ++queued_signal ))
	    return 0
	  }
	  while interrupts are impossible
	  do something important
	  done
	}
	if (( queued_signal ))
	then TRY_BLOCK_INTERRUPT=1	# Does this work?
	fi
      }
    }

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Sun, Dec 7, 2014 at 6:39 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Sat, 6 Dec 2014 12:49:19 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> Another difference: the menu completion listing could previously be
>> aborted with ctrl-c and keep the command line. It now closes the
>> listing and aborts the command line.
>
> Here's a suggestion: when we go back to the main keymap, assume that if
> the user typed ^C in the mean time they want to continue editing from
> this point.  I think that fixes this case.
>
> (This is only on the interrupt_abort branch, again.)
>
>> Additionally, with menu selection, you could previously ctrl-c out of
>> selection and get to the menu, ctrl-c that again, and still have the
>> command line. Now you just go straight from selection to a new empty
>> command line.
>
> This second case was already working for me (interrupt_abort branch with
> Sven-comment-restoration, commit e385312e0937).  I tried both
> MENU_COMPLETE and AUTO_COMPLETE behaviour.  There may be another way in.
> Does the patch below help with this, too?
>
> I'm intending to commit this and then stop patching to allow us to take
> stock of what effects are now present on the interrupt_abort branch.  So
> feel free to re-report any bad features.

Yeah, I just tried the branch as of 'fix typo' and both of my issues
are gone. (I did notice if you restart menu selection after ctrl-c and
then ctrl-c it again, and so on, it keeps selecting the next entry,
but it always did that now that I check).

I'm not sure if depending on menu completion and menu selection having
a different keymap feels right, then again, it does work.

(Haven't read rest of thread yet).

-- 
Mikael Magnusson

Interrupts in completion, traps in _main_complete

[Bart Schaefer]

On Dec 7,  4:21pm, Peter Stephenson wrote:
}
} That's probably the way to go --- my gut feel is the behaviour of the
} other form of trap is too far from what we need to make it a good
} starting point.


I took a look through _main_complete and it does quite a bit of saving and
restoring of global completion settings e.g. in order to implement zstyles.
None of it is nicely isolated, i.e., we're going to need several always-
blocks -- and/or possibly some things declared "local", which I'm not sure
why was never done in the first place -- or else a significant rewrite.

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sun, 7 Dec 2014 11:58:28 -0800
art Schaefer <schaefer@brasslantern.com> wrote:
> On Dec 7,  6:59pm, Peter Stephenson wrote:
> }
> } I'm worried about the various occurrences of this:
> } 
> } 	    /* Keep any user interrupt error status */
> } 	    errflag = oef | (errflag & ERRFLAG_INT);
> 
> I believe that could be rewritten as
> 
> 	errflag &= ~ERRFLAG_ERROR;
> 	errflag |= oef;
> 
> to avoid reading errflag and then writing back to it.  That's likely
> more efficient than queueing/unqueing signals.

That's better but I think you can still get an ERRFLAG_INT missed
between reading errflag, "and"ing it with ~ERRFLAG_ERROR, and writing it
back again.  It depends on the instruction set whether that's atomic but
there's no guarantee either there or in C.

pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Sun, 7 Dec 2014 17:19:20 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> diff --git a/Src/jobs.c b/Src/jobs.c
> index 6e47e5e..3c2a21a 100644
> --- a/Src/jobs.c
> +++ b/Src/jobs.c
> @@ -1444,7 +1444,19 @@ zwaitjob(int job, int wait_cmd)
>  		restore_queue_signals(q);
>  		return 128 + last_signal;
>  	    }
> -	    errflag &= ~ERRFLAG_ERROR;
> +           /* Commenting this out makes ^C-ing a job started by a function
> +              stop the whole function again.  But I guess it will stop
> +              something else from working properly, we have to find out
> +              what this might be.  --oberon
> +
> +	      When attempting to separate errors and interrupts, we
> +	      assumed because of the previous comment it would be OK
> +	      to remove ERRFLAG_ERROR and leave ERRFLAG_INT set, since
> +	      that's the one related to ^C.  But that doesn't work.
> +	      There's something more here we don't understand.  --pws
> +
> +           errflag = 0; */
> +
>  	    if (subsh) {
>  		killjb(jn, SIGCONT);
>  		jn->stat &= ~STAT_STOPPED;

Aha!  Spotted when trying out TRY_BLOCK_INTERRUPT.

I only switched ERRFLAG_ERROR to ERRFLAG_INT when the shell *itself*
gets the interrupt.  The case here, and in the test I was running, is
where we propagate a SIGINT detected by WTERMSIGing (er, that's a verb,
right?) a process that exited because it received the ^C (and the shell
wasn't part of the foreground process group).

The following patch (applicable to the interrupt_abort branch) makes
TRY_BLOCK_INTERRUPT do sensible things in that case.  Needless to say I
haven't dared back off the patch quoted above again...

By the way, the cases below already handle SIGINT and SIGQUIT in
parallel, which suggest the code I added to do this for mimicking signals
on return from a trap is at least consistent.

diff --git a/Src/jobs.c b/Src/jobs.c
index 3c2a21a..a668b07 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -509,7 +509,7 @@ update_job(Job jn)
 			prev_errflag = errflag;
 		    }
 		    breaks = loops;
-		    errflag |= ERRFLAG_ERROR;
+		    errflag |= ERRFLAG_INT;
 		    inerrflush();
 		}
 	    } else {
@@ -526,7 +526,7 @@ update_job(Job jn)
 	    prev_errflag = errflag;
 	}
 	breaks = loops;
-	errflag |= ERRFLAG_ERROR;
+	errflag |= ERRFLAG_INT;
 	inerrflush();
     }
     if (somestopped && jn->stat & STAT_SUPERJOB)
@@ -581,7 +581,7 @@ update_job(Job jn)
 		    breaks = loops;
 	    } else {
 		breaks = loops;
-		errflag |= ERRFLAG_ERROR;
+		errflag |= ERRFLAG_INT;
 	    }
 	    check_cursh_sig(sig);
 	}


pws

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Mon, Dec 8, 2014 at 12:18 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Sun, 7 Dec 2014 17:19:20 +0000
> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>> diff --git a/Src/jobs.c b/Src/jobs.c
>> index 6e47e5e..3c2a21a 100644
>> --- a/Src/jobs.c
>> +++ b/Src/jobs.c
>> @@ -1444,7 +1444,19 @@ zwaitjob(int job, int wait_cmd)
>>               restore_queue_signals(q);
>>               return 128 + last_signal;
>>           }
>> -         errflag &= ~ERRFLAG_ERROR;
>> +/*...*/
>> +           errflag = 0; */
>> +
>>           if (subsh) {
>>               killjb(jn, SIGCONT);
>>               jn->stat &= ~STAT_STOPPED;
>
> Aha!  Spotted when trying out TRY_BLOCK_INTERRUPT.
>
> I only switched ERRFLAG_ERROR to ERRFLAG_INT when the shell *itself*
> gets the interrupt.  The case here, and in the test I was running, is
> where we propagate a SIGINT detected by WTERMSIGing (er, that's a verb,
> right?) a process that exited because it received the ^C (and the shell
> wasn't part of the foreground process group).
>
> The following patch (applicable to the interrupt_abort branch) makes
> TRY_BLOCK_INTERRUPT do sensible things in that case.  Needless to say I
> haven't dared back off the patch quoted above again...
>
> By the way, the cases below already handle SIGINT and SIGQUIT in
> parallel, which suggest the code I added to do this for mimicking signals
> on return from a trap is at least consistent.

(Since this is maybe a little known feature of git, :/foo is the first
reachable commit on any branch with the string foo in the commit name,
you can literally say
git revert :/'this is a very good solution with no drawbacks'
).

Good news and bad news, with this latest patch, I can revert :/'Put
back commenting' and still ctrl-c out of cd ../linux in one go, BUT,
when I do, with or without the revert, it immediately puts me back at
the prompt without executing preexec().

Test case for you guys:
% zsh -f
% precmd() echo yes
% sleep 2; echo no
^C
yes

With :/'Ensure propagation' and :/'Put back' (eg, current tip of
interrupt_abort), I get neither printed.

Reverting :/'Put back' still gets me neither (ie, it makes no
difference, which I guess is the goal of this, so yay?).

With both reverted, I get what I originally reported (obviously)
^C
no
yes

With only :/'Ensure propagation' reverted (eg, at the previous tip), I get
^C
yes

So I guess some other place needs to clear interrupts as well, or the
"return to commandline" clear should be before precmd being called?
(If that's nonsense it's because I haven't looked at the code for that
at all).

-- 
Mikael Magnusson

Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson]

On Mon, 8 Dec 2014 13:43:11 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Test case for you guys:
> % zsh -f
> % precmd() echo yes
> % sleep 2; echo no
> ^C
> yes

I will assume it's uncontroversial that we do want the yes and not the
no.  It looks reasonable.  (As an entirely separate matter, I wonder if
precmd should have the error status available to it?)

> With :/'Ensure propagation' and :/'Put back' (eg, current tip of
> interrupt_abort), I get neither printed.
>
> So I guess some other place needs to clear interrupts as well, or the
> "return to commandline" clear should be before precmd being called?
> (If that's nonsense it's because I haven't looked at the code for that
> at all).

I think we should clear it before and after precmd to be safe.

It's not really clear to me why this worked before; one of the various
other errflag-clearings must have come in before we reached the
top-level loop.  I'm not sure if that was intentional or accidental, but
presumably somewhere high up in loop() with toplevel == 1 is right.  I
can't work out if it should be even higher, before the history mechanism
starts.

pws

diff --git a/Src/init.c b/Src/init.c
index 3cbd4e5..3059087 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -118,6 +118,14 @@ loop(int toplevel, int justonce)
 	    if (interact && toplevel) {
 	        int hstop = stophist;
 		stophist = 3;
+		/*
+		 * Reset all errors including the interrupt error status
+		 * immediately, so preprompt runs regardless of what
+		 * just happened.  We'll reset again below as a
+		 * precaution to ensure we get back to the command line
+		 * no matter what.
+		 */
+		errflag = 0;
 		preprompt();
 		if (stophist != 3)
 		    hbegin(1);

Re: Interrupting globs (Re: Something rotten in tar completion)

[Mikael Magnusson]

On Mon, Dec 8, 2014 at 2:03 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Mon, 8 Dec 2014 13:43:11 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> Test case for you guys:
>> % zsh -f
>> % precmd() echo yes
>> % sleep 2; echo no
>> ^C
>> yes
>
> I will assume it's uncontroversial that we do want the yes and not the
> no.  It looks reasonable.  (As an entirely separate matter, I wonder if
> precmd should have the error status available to it?)

Indeed, I update my prompt with the current directory in precmd, and
not doing that is quite confusing. (No opinion on this, but it seems
to right now?)

>> With :/'Ensure propagation' and :/'Put back' (eg, current tip of
>> interrupt_abort), I get neither printed.
>>
>> So I guess some other place needs to clear interrupts as well, or the
>> "return to commandline" clear should be before precmd being called?
>> (If that's nonsense it's because I haven't looked at the code for that
>> at all).
>
> I think we should clear it before and after precmd to be safe.
>
> It's not really clear to me why this worked before; one of the various
> other errflag-clearings must have come in before we reached the
> top-level loop.  I'm not sure if that was intentional or accidental, but
> presumably somewhere high up in loop() with toplevel == 1 is right.  I
> can't work out if it should be even higher, before the history mechanism
> starts.

With this patch, everything seems to work as expected for me. (famous
last words)

Re: Interrupting globs (Re: Something rotten in tar completion)

[Bart Schaefer]

On Dec 8,  1:03pm, Peter Stephenson wrote:
}
} I think we should clear it before and after precmd to be safe.
} 
} It's not really clear to me why this worked before; one of the various
} other errflag-clearings must have come in before we reached the
} top-level loop.  I'm not sure if that was intentional or accidental, but
} presumably somewhere high up in loop() with toplevel == 1 is right.  I
} can't work out if it should be even higher, before the history mechanism
} starts.

I suspect it should be cleared before the history starts but as long as
it is cleared before anything is actually *read* the chances for it to
go wrong are probably very small.

Re: Interrupts in completion, traps in _main_complete

[Peter Stephenson]

On Sun, 07 Dec 2014 15:01:40 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Dec 7,  4:21pm, Peter Stephenson wrote:
> }
> } That's probably the way to go --- my gut feel is the behaviour of the
> } other form of trap is too far from what we need to make it a good
> } starting point.
> 
> I took a look through _main_complete and it does quite a bit of saving and
> restoring of global completion settings e.g. in order to implement zstyles.
> None of it is nicely isolated, i.e., we're going to need several always-
> blocks -- and/or possibly some things declared "local", which I'm not sure
> why was never done in the first place -- or else a significant rewrite.

I'm probably missing a lot, but it struck me that if you're aborting out
of completion completely, as it were, then the stuff that doesn't get
executed doesn't typically matter all that much.  The stuff you really
want to get right on exit --- options, IFS, etc. --- is already local.

Beyond that I couldn't offhand say which the most important things to
restore were.  If you aborted completion, did you want to set _lastcomp
or do you actually want to steer clear of setting it?

pws

Re: Interrupts in completion, traps in _main_complete

[Bart Schaefer]

On Dec 8,  8:27pm, Peter Stephenson wrote:
}
} I'm probably missing a lot, but it struck me that if you're aborting out
} of completion completely, as it were, then the stuff that doesn't get
} executed doesn't typically matter all that much.  The stuff you really
} want to get right on exit --- options, IFS, etc. --- is already local.

I'm looking at LISTPROMPT, MENUPROMPT, MENUSCROLL, MENUSELECT, MENUMODE,
and ZLS_COLORS.  Of which I guess only ZLS_COLORS gets reset right now,
but on an interrupt even it won't get restored.

Incidentally, MENUMODE appears to be entirely undocumented.

} Beyond that I couldn't offhand say which the most important things to
} restore were.  If you aborted completion, did you want to set _lastcomp
} or do you actually want to steer clear of setting it?

There is that question ... also, did you want compstate[old_list] or not?
What about other values of compstate?

Whether you want the post-funcs to run probably depends on what the
pre-funcs were, so that's a bit ugly.

Re: Interrupts in completion, traps in _main_complete

[Peter Stephenson]

On Mon, 8 Dec 2014 20:43:10 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Dec 8,  8:27pm, Peter Stephenson wrote:
> }
> } I'm probably missing a lot, but it struck me that if you're aborting out
> } of completion completely, as it were, then the stuff that doesn't get
> } executed doesn't typically matter all that much.  The stuff you really
> } want to get right on exit --- options, IFS, etc. --- is already local.
> 
> I'm looking at LISTPROMPT, MENUPROMPT, MENUSCROLL, MENUSELECT, MENUMODE,
> and ZLS_COLORS.  Of which I guess only ZLS_COLORS gets reset right now,
> but on an interrupt even it won't get restored.
> 
> Incidentally, MENUMODE appears to be entirely undocumented.

Apart from ZLE_COLORS it looks like we're not in imminent danger of making
anything worse.

I don't really understand what's going on with ZLE_COLORS, however.  In
the majority of cases it just seems to be saved and restored as if it
were local.  The only exception seems to be if the show-ambiguity style
is set it gets saved for use in another completion.  What is it about
show-ambiguity that requires this?  Is it so that if we redisplay a
listing without going through _main_complete again it appears correct?

I wonder if we're close to the point where applying this branch to the
master is reasonable.  It doesn't have to be 100% right, just good
enough that people who use the bleeding edge don't fall flat on their
faces.

pws

Re: Interrupts in completion, traps in _main_complete

[Peter Stephenson]

On Tue, 9 Dec 2014 11:26:47 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Mon, 8 Dec 2014 20:43:10 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> > On Dec 8,  8:27pm, Peter Stephenson wrote:
> > } I'm probably missing a lot, but it struck me that if you're aborting out
> > } of completion completely, as it were, then the stuff that doesn't get
> > } executed doesn't typically matter all that much.  The stuff you really
> > } want to get right on exit --- options, IFS, etc. --- is already local.
> > 
> > I'm looking at LISTPROMPT, MENUPROMPT, MENUSCROLL, MENUSELECT, MENUMODE,
> > and ZLS_COLORS.  Of which I guess only ZLS_COLORS gets reset right now,
> > but on an interrupt even it won't get restored.
> 
> I don't really understand what's going on with ZLE_COLORS, however.  In
> the majority of cases it just seems to be saved and restored as if it
> were local.  The only exception seems to be if the show-ambiguity style
> is set it gets saved for use in another completion.  What is it about
> show-ambiguity that requires this?  Is it so that if we redisplay a
> listing without going through _main_complete again it appears correct?

Came in in zsh-workers/12029, currently

http://www.zsh.org/mla/workers/2000/msg02851.html

I think the only relevant facts in the associated email are that colour
handling "should be cleaner now" and that it should have been in a
separate patch.

The simultaneous change in _setup looks like it's relevant.  It's been
rewritten not to use ZLS_COLORS from before, but it will set the
_comp_colors that subsequently gets saved into ZLS_COLORS.

In summary: er, well, yeah.

pws

Re: Interrupts in completion, traps in _main_complete

[Peter Stephenson]

On Tue, 09 Dec 2014 11:50:41 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > I don't really understand what's going on with ZLE_COLORS, however.  In
> > the majority of cases it just seems to be saved and restored as if it
> > were local.  The only exception seems to be if the show-ambiguity style
> > is set it gets saved for use in another completion.  What is it about
> > show-ambiguity that requires this?  Is it so that if we redisplay a
> > listing without going through _main_complete again it appears correct?
> 
> In summary: er, well, yeah.

It seems ZLS_COLORS does need to survive after the end of
_main_complete, which is a bit of a messy special case.  But the patch
ensures ZLS_COLORS gets restored on an interrupt.

I don't know what, if anything, $comppostfuncs is used for apart from
_all_matches, but we could introduce a separate variable compcleanupfuncs
that goes inside the always block.  Nothing in _all_matches_end looks
crucial; it does unset _all_matches_context but that's unimportant until
the next time _all_matches runs.

diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
index 91b68fe..23b91d3 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -43,6 +43,8 @@ local -a precommands
 
 typeset -U _lastdescr _comp_ignore _comp_colors
 
+{
+
 [[ -z "$curcontext" ]] && curcontext=:::
 
 zstyle -s ":completion:${curcontext}:" insert-tab tmp || tmp=yes
@@ -349,17 +351,20 @@ fi
    ( "$_comp_force_list" = ?*  && nm -ge _comp_force_list ) ]] &&
     compstate[list]="${compstate[list]//messages} force"
 
-if [[ "$compstate[old_list]" = keep ]]; then
-  if [[ $_saved_colors_set = 1 ]]; then
-    ZLS_COLORS="$_saved_colors"
+} always {
+  # Stuff we always do to clean up.
+  if [[ "$compstate[old_list]" = keep ]]; then
+    if [[ $_saved_colors_set = 1 ]]; then
+      ZLS_COLORS="$_saved_colors"
+    else
+      unset ZLS_COLORS
+    fi
+  elif (( $#_comp_colors )); then
+    ZLS_COLORS="${(j.:.)_comp_colors}"
   else
     unset ZLS_COLORS
   fi
-elif (( $#_comp_colors )); then
-  ZLS_COLORS="${(j.:.)_comp_colors}"
-else
-  unset ZLS_COLORS
-fi
+}
 
 # Now call the post-functions.
 

interrupt_abort incorporation

[Peter Stephenson]

On Tue, 9 Dec 2014 11:26:47 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I wonder if we're close to the point where applying this branch
> [interrupt_abort] to the master is reasonable.  It doesn't have to be
> 100% right, just good enough that people who use the bleeding edge
> don't fall flat on their faces.

I'll do this in the next couple of days if no one has anything that
needs fixing immediately.  It needs wider testing.

pws

Re: interrupt_abort incorporation

[Peter Stephenson]

On Wed, 10 Dec 2014 10:02:45 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Tue, 9 Dec 2014 11:26:47 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > I wonder if we're close to the point where applying this branch
> > [interrupt_abort] to the master is reasonable.  It doesn't have to be
> > 100% right, just good enough that people who use the bleeding edge
> > don't fall flat on their faces.
> 
> I'll do this in the next couple of days if no one has anything that
> needs fixing immediately.  It needs wider testing.

Now present: the interrupt_abort branch was rebased onto master and
squashed to a single commit, so there's no merge scar and if you want to
attempt to revert it for investigation you can do it in one go.

I'll leave the interrupt_abort branch lying around for reference for the
time being.

pws