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

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

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