Re: zsh exits suddenly.

Re: zsh exits suddenly.

[Peter Stephenson]

"Bart Schaefer" wrote:
> On Jun 23, 11:45pm, Tanaka Akira wrote:
> } Subject: zsh exits suddenly.
> }
> } I have a trouble that zsh exits as following.
> } 
> } carbon% ls <^D>
> } zsh: do you wish to see all 848 possibilities? <^C>n
> } carbon% ls <^M>
> } akr@carbon% 
> 
> I finally figured out where that's happening -- it's in ingetc(), which
> is called from the SIGINT handler() via inerrflush().  However, I still
> don't know how to fix the problem.

You were right, the problem is more general than the one I fixed in
http://www.zsh.org/mla/workers-1998/msg00430.html .  How about this?
It says 'don't flush input if there aren't any characters to flush'.
It's hard to see how that can be wrong, since ingetc() always fetches
new data a line at a time.  It seems to work.

I think setting lexstop to 0 in the signal code is too drastic:  it
should never have got to 1 in the first place.

More technical notes:

I can't be absolutely sure if all that lastc stuff is really needed.
It means, of course, that if there is more than one line in the
buffer, only the current one is flushed.  Is this the right behaviour?
As you can see from the 'strin' test, if the data being read is a
single string (e.g. from a $(...) substitution) it dumps the whole lot
anyway.  Before I rewrote the input stuff, everything was shoved onto
a single string, so it was harder to tell when you had to stop
flushing it: I wimped out on removing the lastc stuff since I wrongly
thought it wasn't doing any harm.  Maybe I'll experiment here with
eliminating all the lastc nonsense and just testing inbufct.  The
comment in inerrflush() may be a red herring, it dates from before the
new code.

*** Src/input.c.lastc	Wed Jun 24 10:24:24 1998
--- Src/input.c	Wed Jun 24 10:32:39 1998
***************
*** 427,433 ****
       * it is only used in the history code, where that is the only
       * completely safe way of discarding input.
       */
!     while ((strin || lastc != '\n') && !lexstop)
  	ingetc();
  }
  
--- 427,433 ----
       * it is only used in the history code, where that is the only
       * completely safe way of discarding input.
       */
!     while ((strin || lastc != '\n') && !lexstop && inbufct)
  	ingetc();
  }
  
-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 50 844536
WWW:  http://www.ifh.de/~pws/
Gruppo Teorico, Dipartimento di Fisica
Piazza Torricelli 2, 56100 Pisa, Italy

Re: zsh exits suddenly.

[Bart Schaefer]

On Jun 24, 10:51am, Peter Stephenson wrote:
} Subject: Re: zsh exits suddenly.
}
} I can't be absolutely sure if all that lastc stuff is really needed.
} It means, of course, that if there is more than one line in the
} buffer, only the current one is flushed.  Is this the right behaviour?

This isn't changed by your patch, right?  It was the behavior all along?

I think the intent is to prevent accept-line from executing a partial
line following a tty interrupt.  That probably ought to apply to multi-
line buffers as well.  The guideline should probably be, what does ZLE
do to a multiline buffer when send-break is executed?  The SIGINT handler
ought to do the same thing.  (In fact, why doesn't it call sendbreak()
when ZLE is active?)

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: zsh exits suddenly.

[Peter Stephenson]

"Bart Schaefer" wrote:
> On Jun 24, 10:51am, Peter Stephenson wrote:
> } Subject: Re: zsh exits suddenly.
> }
> } I can't be absolutely sure if all that lastc stuff is really needed.
> } It means, of course, that if there is more than one line in the
> } buffer, only the current one is flushed.  Is this the right behaviour?
> 
> This isn't changed by your patch, right?  It was the behavior all along?

Yes, this was exactly all it did before during an interactive read:
discard everything up to the next newline.

> The guideline should probably be, what does ZLE
> do to a multiline buffer when send-break is executed?  The SIGINT handler
> ought to do the same thing.  (In fact, why doesn't it call sendbreak()
> when ZLE is active?)

Actually, at the zle stage there shouldn't be any line input to flush
yet, so this doesn't really matter (i.e., after my last fix
inerrflush() always becomes a no-op here, regardless of the lastc
behaviour), but for what it's worth the complete multi-line buffer is
discarded in both cases.

The proper use of inerrflush() is when lexing the line just input.
I've found the difference: it's in a case like this:

% print !!:s/foo/bar/<ESC><RET>
print haha

The current code gives an error message for the substitution (assuming
of course there was no `foo' in the previous line), but prints `haha'
anyway.  Without the lastc stuff, the `print haha' is discarded along
with the rest of the previous line.

Oddly, the code for "event not found" and for "Ambiguous history
reference" performs the effect of inerrflush() itself, i.e. reading up
to a newline: this may well be just an oversight from before
inerrflush() was written.

So the question boils down to:  after a history subsitution failure,
should the rest of the input be discarded, or just the rest of the
line as at present?  (Note parsing failures don't flush the input in
any case, i.e.:

% if [[<ESC><RET>
then echo<ESC><RET>
fi<ESC><RET>
print haha

will always print `haha' after some error messages.  It also has an
extremely funny effect on the history lines stored.)

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 50 844536
WWW:  http://www.ifh.de/~pws/
Gruppo Teorico, Dipartimento di Fisica
Piazza Torricelli 2, 56100 Pisa, Italy

Re: zsh exits suddenly.

[Bart Schaefer]

On Jun 25, 10:33am, Peter Stephenson wrote:
} Subject: Re: zsh exits suddenly.
}
} The proper use of inerrflush() is when lexing the line just input.
} I've found the difference: it's in a case like this:
} 
} % print !!:s/foo/bar/<ESC><RET>
} print haha
} 
} The current code gives an error message for the substitution (assuming
} of course there was no `foo' in the previous line), but prints `haha'
} anyway.  Without the lastc stuff, the `print haha' is discarded along
} with the rest of the previous line.
} 
} So the question boils down to:  after a history subsitution failure,
} should the rest of the input be discarded, or just the rest of the
} line as at present?  (Note parsing failures don't flush the input in
} any case [...]

Aha.  I think both substitution errors -and- parsing failures ought to
consume the entire remainder of the input.   Consider what happens if
the statement following the error is something destructive like a "rm",
which was e.g. expected to happened in a different directory.

It's also probably a good idea in light of the strange way the results
end up in the history on a parse error.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

PATCH: 3.1.4: flushing input properly

[Peter Stephenson]

"Bart Schaefer" wrote:
> Aha.  I think both substitution errors -and- parsing failures ought to
> consume the entire remainder of the input.   Consider what happens if
> the statement following the error is something destructive like a "rm",
> which was e.g. expected to happened in a different directory.

I had come to the same conclusion for the same reason.  Now we're
getting somewhere.  The following was based on a non-vanilla
(raspberry ripple, if I remember right) 3.1.3, but if I've been
following properly it should work with 3.1.4, and replaces the input.c
patchfest of the last few weeks, including 4095 and yesterday's 4161.

All history and parsing errors now discard the input buffer completely
with no funny tests.  This should remove all the ^C problems, and the
possibilities Bart alluded to.  Furthermore, the unparsed code now
gets properly tacked on to the end of the history line, so you should
be able to retrieve it and edit it.  Furtherfurthermore, one of my
personal bugbears is removed: in something like

% cmd1 !word a long expression I took ages typing in<ESC><RET>
cmd2 and this bit here I was rather attached to

when !word fails, the remainder of the buffer is saved in the history
as it is being junked from the input, ending the long and inglorious
zsh tradition that anything after a history failure is lost forever.
So when you try and re-edit the line to fix it, all that's missing is
the !word that didn't work (it's a bit more work to get that to remain
verbatim, so I didn't try).

Most of the patch is converting inerrflush() to herrflush().  In fact,
inerrflush() is now only called from the signal handler, but I wanted
to alter that as little as possible.  I hope nobody was particularly
attached to discard_input().

A slightly different issue: it remains the case that the shell
blithely ignores parse errors, executing any commands it comes across
and causing merry hell wherever it goes:

% cat scr
if [[ very_unusual_circumstances ]]; then 
  echo rm -rf /
else
  echo do nothing
fi
echo everything worked fine.
% zsh -f scr
scr: parse error near `]]' [1]
rm -rf /
scr: parse error near `else' [3]
do nothing
scr: parse error near `fi' [5]
everything worked fine.

This is in fact a question about when to stop reading input
altogether; the commands weren't in the input buffer when the parse
error happened, so this patch has no effect.  You might well argue it
ought to give up on the first error, as ksh does.  That needs careful
handling, though, to distinguish between running scripts, running
interactively, and sourcing a file.

*** Src/hist.c.inerr	Fri Jun 26 09:51:13 1998
--- Src/hist.c	Fri Jun 26 10:02:19 1998
***************
*** 251,256 ****
--- 251,264 ----
  	inungetc(c);
  }
  
+ /**/
+ void
+ herrflush(void)
+ {
+     while (!lexstop && inbufct)
+ 	hwaddc(ingetc());
+ }
+ 
  /* extract :s/foo/bar/ delimiters and arguments */
  
  /**/
***************
*** 272,279 ****
      zsfree(hsubr);
      hsubr = ptr2;
      if (hsubl && !strstr(subline, hsubl)) {
  	zerr("substitution failed", NULL, 0);
- 	inerrflush();
  	return 1;
      }
      return 0;
--- 280,287 ----
      zsfree(hsubr);
      hsubr = ptr2;
      if (hsubl && !strstr(subline, hsubl)) {
+ 	herrflush();
  	zerr("substitution failed", NULL, 0);
  	return 1;
      }
      return 0;
***************
*** 358,364 ****
  	    mev = ev = hconsearch(hsubl = ztrdup(buf), &marg);
  	    evset = 0;
  	    if (ev == -1) {
! 		inerrflush();
  		zerr("no such event: %s", buf, 0);
  		return -1;
  	    }
--- 366,372 ----
  	    mev = ev = hconsearch(hsubl = ztrdup(buf), &marg);
  	    evset = 0;
  	    if (ev == -1) {
! 		herrflush();
  		zerr("no such event: %s", buf, 0);
  		return -1;
  	    }
***************
*** 410,418 ****
  		ev = curhist;
  		evset = 1;
  	    } else if ((ev = hcomsearch(buf)) == -1) {
  		zerr("event not found: %s", buf, 0);
- 		while (c != '\n' && !lexstop)
- 		    c = ingetc();
  		return -1;
  	    } else
  		evset = 1;
--- 418,425 ----
  		ev = curhist;
  		evset = 1;
  	    } else if ((ev = hcomsearch(buf)) == -1) {
+ 		herrflush();
  		zerr("event not found: %s", buf, 0);
  		return -1;
  	    } else
  		evset = 1;
***************
*** 434,442 ****
  		    ehist = gethist(defev = mev);
  		    argc = getargc(ehist);
  		} else {
  		    zerr("Ambiguous history reference", NULL, 0);
- 		    while (c != '\n' && !lexstop)
- 			c = ingetc();
  		    return -1;
  		}
  
--- 441,448 ----
  		    ehist = gethist(defev = mev);
  		    argc = getargc(ehist);
  		} else {
+ 		    herrflush();
  		    zerr("Ambiguous history reference", NULL, 0);
  		    return -1;
  		}
  
***************
*** 493,499 ****
  		break;
  	    case 'h':
  		if (!remtpath(&sline)) {
! 		    inerrflush();
  		    zerr("modifier failed: h", NULL, 0);
  		    return -1;
  		}
--- 499,505 ----
  		break;
  	    case 'h':
  		if (!remtpath(&sline)) {
! 		    herrflush();
  		    zerr("modifier failed: h", NULL, 0);
  		    return -1;
  		}
***************
*** 500,506 ****
  		break;
  	    case 'e':
  		if (!rembutext(&sline)) {
! 		    inerrflush();
  		    zerr("modifier failed: e", NULL, 0);
  		    return -1;
  		}
--- 506,512 ----
  		break;
  	    case 'e':
  		if (!rembutext(&sline)) {
! 		    herrflush();
  		    zerr("modifier failed: e", NULL, 0);
  		    return -1;
  		}
***************
*** 507,513 ****
  		break;
  	    case 'r':
  		if (!remtext(&sline)) {
! 		    inerrflush();
  		    zerr("modifier failed: r", NULL, 0);
  		    return -1;
  		}
--- 513,519 ----
  		break;
  	    case 'r':
  		if (!remtext(&sline)) {
! 		    herrflush();
  		    zerr("modifier failed: r", NULL, 0);
  		    return -1;
  		}
***************
*** 514,520 ****
  		break;
  	    case 't':
  		if (!remlpaths(&sline)) {
! 		    inerrflush();
  		    zerr("modifier failed: t", NULL, 0);
  		    return -1;
  		}
--- 520,526 ----
  		break;
  	    case 't':
  		if (!remlpaths(&sline)) {
! 		    herrflush();
  		    zerr("modifier failed: t", NULL, 0);
  		    return -1;
  		}
***************
*** 526,532 ****
  		if (hsubl && hsubr)
  		    subst(&sline, hsubl, hsubr, gbal);
  		else {
! 		    inerrflush();
  		    zerr("no previous substitution", NULL, 0);
  		    return -1;
  		}
--- 532,538 ----
  		if (hsubl && hsubr)
  		    subst(&sline, hsubl, hsubr, gbal);
  		else {
! 		    herrflush();
  		    zerr("no previous substitution", NULL, 0);
  		    return -1;
  		}
***************
*** 544,550 ****
  		upcase(&sline);
  		break;
  	    default:
! 		inerrflush();
  		zerr("illegal modifier: %c", NULL, c);
  		return -1;
  	    }
--- 550,556 ----
  		upcase(&sline);
  		break;
  	    default:
! 		herrflush();
  		zerr("illegal modifier: %c", NULL, c);
  		return -1;
  	    }
***************
*** 995,1006 ****
  	ret = argc;
      else if (c == '%') {
  	if (evset) {
! 	    inerrflush();
  	    zerr("Ambiguous history reference", NULL, 0);
  	    return -2;
  	}
  	if (marg == -1) {
! 	    inerrflush();
  	    zerr("%% with no previous word matched", NULL, 0);
  	    return -2;
  	}
--- 1001,1012 ----
  	ret = argc;
      else if (c == '%') {
  	if (evset) {
! 	    herrflush();
  	    zerr("Ambiguous history reference", NULL, 0);
  	    return -2;
  	}
  	if (marg == -1) {
! 	    herrflush();
  	    zerr("%% with no previous word matched", NULL, 0);
  	    return -2;
  	}
***************
*** 1233,1239 ****
  
      ret = quietgethist(ev);
      if (!ret) {
! 	inerrflush();
  	zerr("no such event: %d", NULL, ev);
      }
      return ret;
--- 1239,1245 ----
  
      ret = quietgethist(ev);
      if (!ret) {
! 	herrflush();
  	zerr("no such event: %d", NULL, ev);
      }
      return ret;
***************
*** 1248,1254 ****
  
      if (arg2 < arg1 || arg1 >= nwords || arg2 >= nwords) {
  	/* remember, argN is indexed from 0, nwords is total no. of words */
! 	inerrflush();
  	zerr("no such word in event", NULL, 0);
  	return NULL;
      }
--- 1254,1260 ----
  
      if (arg2 < arg1 || arg1 >= nwords || arg2 >= nwords) {
  	/* remember, argN is indexed from 0, nwords is total no. of words */
! 	herrflush();
  	zerr("no such word in event", NULL, 0);
  	return NULL;
      }
*** Src/input.c.inerr	Tue Jun  2 17:16:03 1998
--- Src/input.c	Wed Jun 24 10:52:16 1998
***************
*** 101,109 ****
  static int inbufleft;		/* Characters left in current input
  				   stack element */
  
- static int lastc;		/* used as flag that end of line was reached */
  
- 
   /* Input must be stacked since the input queue is used by
    * various different parts of the shell.
    */
--- 101,107 ----
***************
*** 178,185 ****
  int
  ingetc(void)
  {
      if (lexstop)
! 	return lastc = ' ';
      for (;;) {
  	if (inbufleft) {
  	    inbufleft--;
--- 176,185 ----
  int
  ingetc(void)
  {
+     char lastc;
+ 
      if (lexstop)
! 	return ' ';
      for (;;) {
  	if (inbufleft) {
  	    inbufleft--;
***************
*** 202,212 ****
  	 */
  	if (strin || errflag) {
  	    lexstop = 1;
! 	    return lastc = ' ';
  	}
  	/* As a last resort, get some more input */
  	if (inputline())
! 	    return lastc = ' ';
      }
  }
  
--- 202,212 ----
  	 */
  	if (strin || errflag) {
  	    lexstop = 1;
! 	    return ' ';
  	}
  	/* As a last resort, get some more input */
  	if (inputline())
! 	    return ' ';
      }
  }
  
***************
*** 422,433 ****
  void
  inerrflush(void)
  {
!     /*
!      * This always goes character by character, but at present
!      * it is only used in the history code, where that is the only
!      * completely safe way of discarding input.
!      */
!     while ((strin || lastc != '\n') && !lexstop)
  	ingetc();
  }
  
--- 422,428 ----
  void
  inerrflush(void)
  {
!     while (!lexstop && inbufct)
  	ingetc();
  }
  
*** Src/parse.c.inerr	Fri Jun 26 11:06:51 1998
--- Src/parse.c	Fri Jun 26 11:07:52 1998
***************
*** 72,78 ****
  
  #define YYERROR  { tok = LEXERR; return NULL; }
  #define YYERRORV { tok = LEXERR; return; }
! #define COND_ERROR(X,Y) do{zerr(X,Y,0);discard_input();YYERROR}while(0)
  
  #define make_list()     allocnode(N_LIST)
  #define make_sublist()  allocnode(N_SUBLIST)
--- 72,78 ----
  
  #define YYERROR  { tok = LEXERR; return NULL; }
  #define YYERRORV { tok = LEXERR; return; }
! #define COND_ERROR(X,Y) do{herrflush();zerr(X,Y,0);YYERROR}while(0)
  
  #define make_list()     allocnode(N_LIST)
  #define make_sublist()  allocnode(N_SUBLIST)
***************
*** 142,149 ****
  	    yyerror();
  	    return NULL;
  	}
  	yyerror();
- 	discard_input();
  	return NULL;
      } else {
  	l->right = par_event();
--- 142,149 ----
  	    yyerror();
  	    return NULL;
  	}
+ 	herrflush();
  	yyerror();
  	return NULL;
      } else {
  	l->right = par_event();
***************
*** 1362,1385 ****
      n->right = (void *) c;
      n->ntype = NT_SET(N_COND, NT_STR, NT_STR, 0, 0);
      return n;
- }
- 
- /**/
- static void
- discard_input(void)
- {
-     errflag = 0;
-     if (isnewlin <= 0) {
- 	/* Discard remaining stuff after a parse error. */
- 	int c;
- 
- 	hwbegin(0);
- 	while ((c = hgetc()) != '\n' && !lexstop);
- 	if (c == '\n')
- 	    hungetc('\n');
- 	hwend();
-     }
-     errflag = 1;
  }
  
  /**/
--- 1362,1367 ----

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 50 844536
WWW:  http://www.ifh.de/~pws/
Gruppo Teorico, Dipartimento di Fisica
Piazza Torricelli 2, 56100 Pisa, Italy

Re: PATCH: 3.1.4: flushing input properly

[Bart Schaefer]

On Jun 26, 12:17pm, Peter Stephenson wrote:
} Subject: PATCH: 3.1.4: flushing input properly
}
} Most of the patch is converting inerrflush() to herrflush().  In fact,
} inerrflush() is now only called from the signal handler, but I wanted
} to alter that as little as possible.  I hope nobody was particularly
} attached to discard_input().

This looks good.  Just one question ... why does discard_input() clear
and then set errflag, and why doesn't herrflush() need to do the same?

} A slightly different issue: it remains the case that the shell
} blithely ignores parse errors, executing any commands it comes across

Hrm.  That's particularly bad given that "zsh -ef" does the same thing.
It at least ought to give up when told to exit on error.

} You might well argue it
} ought to give up on the first error, as ksh does.  That needs careful
} handling, though, to distinguish between running scripts, running
} interactively, and sourcing a file.

I believe the reason it behaves as it does is for purposes of sourcing
a file, so that syntax errors in .zshrc et. al. don't cause the entire
init process to fail.  That's a nicety I'd rather not give up.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.1.4: flushing input properly

[Zefram]

Bart Schaefer wrote:
>I believe the reason it behaves as it does is for purposes of sourcing
>a file, so that syntax errors in .zshrc et. al. don't cause the entire
>init process to fail.  That's a nicety I'd rather not give up.

I think it'd be perfectly reasonable to ignore the remainder (or, indeed,
the entirety) of a startup script when a parse error is encountered.

-zefram

Re: PATCH: 3.1.4: flushing input properly

[Peter Stephenson]

"Bart Schaefer" wrote:
> This looks good.  Just one question ... why does discard_input() clear
> and then set errflag, and why doesn't herrflush() need to do the same?

I was careful to put herrflush() before calls to zerr() etc., and the
history mechanism shouldn't be active with errflag already set.  If
the code as written doesn't work, I'd rather know where and why
errflag was unexpectedly being set beforehand rather than blindly
unsetting it.  (Note it only affects adding the rest of the buffer onto
the history line; the input mechanism will happily return characters
already stored with errflag set, it just won't input a new line.)  As
for setting it after, you will see that every call to herrflush() is
followed immediately by a  call to an error function, whose task it
properly is.

As for why discard_input() did it... first of all, the existing code
didn't always call the error function after rather than before, and
secondly discard_input() was a bit out of place in parse.c rather than
hist.c so its interaction with the history code was a bit murkier.  I
think it was just one those bits that never got tidied up properly
till now.  (The other complexities in discard_input() were what were
dividing up the history line in a strange way, so I think they're well
out of it.)

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 50 844536
WWW:  http://www.ifh.de/~pws/
Gruppo Teorico, Dipartimento di Fisica
Piazza Torricelli 2, 56100 Pisa, Italy