Re: precmd() and history

[Peter Stephenson <pws@xxxxxx>]

Steve Reid wrote:
> I'm having a problem with Zsh 3.0.2 under FreeBSD 2.1.0. It doesn't seem
> to honor the hist_ignore_dups or hist_ignore_space options. Also, just
> pressing enter will add a blank line to the history.
> 
> This only happened when I added the precmd() shell function.
>
> precmd() {
> 	fc -AI ~$USER/.history;
> 	fc -R ~$USER/.history;
> }                                         

(the above pruned down to bare essentials, the extra fc's are central
to the bug.)

This has finally made me do the last part of the history code tidy up
which I should have done with the other two bits.  The problems are
fixed partly as a side effect, partly as an effect of an additional
change to remhist().  

I haven't copied this to zsh-users since it turns out there is no
user-fixable part of the problem.  Apart from bugfixes, there
shouldn't be any user-visible changes either.  In fact, the only
change visible outside hist.c should be that histremmed is now the
rather more useful histactive (see comment at top of hist.c).

The essential point is that now the history line is only stored when
it actually needs to be.  This simplifies the previous confusing
overlap between hbegin() and hend().

This is a patch against 3.0.3-test3.  If I remember right and the
revamped HISTIGNOREDUPS mechanism appears in 3.1, that will require
extra changes to fix the problems above, since the new mechanism
compares words of the command line to determine differences; when
they have been read from a history file the words are not lexically
determined, simply space-separated.  In other words, if the previous
history line has been read from a file you need to revert to strcmp()
as used up to 3.0.x.

We are now essentially in the position to be able not to allocate the
history list at all for a non-interactive shell.  All that remains is
to do tests in the appropriate places before trying to access the
history list.  However, that's not entirely trivial.

The history mechanism gets everywhere, so this could do with some
testing.  In particular, changes to the history code traditionally
cause off-by-one errors with fc commands.  I haven't found any yet.

One more thing I noticed:

% echo foo !#<TAB>

fails silently and does ordinary completion instead.  This is not a
result of this patch.  !# (get current line up to previous word) still
works when it should.

*** Src/globals.h.oldh	Wed Jan  8 11:27:05 1997
--- Src/globals.h	Wed Jan  8 15:08:51 1997
***************
*** 451,459 ****
   
  EXTERN int nocorrect;
  
! /* != 0 means we have removed the current event from the history List */
   
! EXTERN int histremmed;
  
  /* current emulation (used to decide which set of option letters is used) */
  
--- 451,459 ----
   
  EXTERN int nocorrect;
  
! /* state of the history mechanism (see hist.c) */
   
! EXTERN int histactive;
  
  /* current emulation (used to decide which set of option letters is used) */
  
*** Src/hist.c.oldh	Wed Jan  8 11:26:53 1997
--- Src/hist.c	Wed Jan  8 14:58:02 1997
***************
*** 31,39 ****
  
  #include "zsh.h"
  
  extern int cs, ll;
  
- static Histent curhistent;
  /* Array of word beginnings and endings in current history line. */
  short *chwords;
  /* Max, actual position in chwords.
--- 31,52 ----
  
  #include "zsh.h"
  
+ /*
+  * Note on histactive: bit 0 says the history mechanism is active;
+  * bit 1 says junk the line being processed by the history
+  * when finished with it if (histactive & 1); bit 2 says we have already
+  * junked a line when !(histactive & 1).
+  *
+  * Note on curhist: with !(histactive & 1), this points to the
+  * last line actually added to the history list.  With (histactive & 1),
+  * the line does not get added to the list until hend(), if at all.
+  * However, curhist is incremented to reflect the current line anyway.
+  * Thus if the line is not added to the list, curhist must be
+  * decremented in hend().
+  */
+ 
  extern int cs, ll;
  
  /* Array of word beginnings and endings in current history line. */
  short *chwords;
  /* Max, actual position in chwords.
***************
*** 61,74 ****
  
  	/* Resize history line if necessary */
  	if (hptr - chline >= hlinesz) {
! 	    int flag = 0, oldsiz = hlinesz;
  
- 	    /* Maybe already linked to a history entry. */
- 	    if (curhistent->text == chline)
- 		flag = 1;
  	    chline = realloc(chline, hlinesz = oldsiz + 16);
- 	    if (flag)
- 		curhistent->text = chline;
  	    hptr = chline + oldsiz;
  	}
      }
--- 74,82 ----
  
  	/* Resize history line if necessary */
  	if (hptr - chline >= hlinesz) {
! 	    int oldsiz = hlinesz;
  
  	    chline = realloc(chline, hlinesz = oldsiz + 16);
  	    hptr = chline + oldsiz;
  	}
      }
***************
*** 188,197 ****
  int
  getargc(Histent ehist)
  {
!   if (ehist == curhistent)
!     return chwordpos ? chwordpos/2-1 : 0;
!   else
!     return ehist->nwords-1;
  }
  
  /* Perform history substitution, returning the next character afterwards. */
--- 196,202 ----
  int
  getargc(Histent ehist)
  {
!     return ehist->nwords ? ehist->nwords-1 : 0;
  }
  
  /* Perform history substitution, returning the next character afterwards. */
***************
*** 577,584 ****
  void
  hbegin(void)
  {
      isfirstln = isfirstch = 1;
!     histremmed = errflag = histdone = spaceflag = 0;
      stophist = (!interact || unset(BANGHIST) || unset(SHINSTDIN)) << 1;
      chline = hptr = zcalloc(hlinesz = 16);
      chwords = zalloc((chwordlen = 16)*sizeof(short));
--- 582,591 ----
  void
  hbegin(void)
  {
+     Histent curhistent;
+ 
      isfirstln = isfirstch = 1;
!     errflag = histdone = spaceflag = 0;
      stophist = (!interact || unset(BANGHIST) || unset(SHINSTDIN)) << 1;
      chline = hptr = zcalloc(hlinesz = 16);
      chwords = zalloc((chwordlen = 16)*sizeof(short));
***************
*** 589,604 ****
  	curhistent->ftim = time(NULL);
      if (interact && isset(SHINSTDIN) && !strin) {
  	attachtty(mypgrp);
! 	defev = curhist++;
! 	curhistent = gethistent(curhist);
! 	zsfree(curhistent->text);
! 	if (curhistent->nwords)
! 	    zfree(curhistent->words, curhistent->nwords*2*sizeof(short));
! 	curhistent->text = chline;
! 	curhistent->words = NULL;
! 	curhistent->nwords = 0;
      } else
! 	histremmed = 1;
  }
  
  /* say we're done using the history mechanism */
--- 596,607 ----
  	curhistent->ftim = time(NULL);
      if (interact && isset(SHINSTDIN) && !strin) {
  	attachtty(mypgrp);
! 	defev = curhist;
! 	histactive = 1;
      } else
! 	histactive = 3;
! 
!     curhist++;
  }
  
  /* say we're done using the history mechanism */
***************
*** 610,621 ****
      int flag, save = 1;
      Histent he;
  
!     if (!chline)
! 	return 1;
!     if (!interact || strin || unset(SHINSTDIN)) {
  	zfree(chline, hlinesz);
  	zfree(chwords, chwordlen*sizeof(short));
  	chline = NULL;
  	return 1;
      }
      flag = histdone;
--- 613,625 ----
      int flag, save = 1;
      Histent he;
  
!     DPUTS(!chline, "BUG: chline is NULL in hend()");
!     if (histactive & 2) {
  	zfree(chline, hlinesz);
  	zfree(chwords, chwordlen*sizeof(short));
  	chline = NULL;
+ 	curhist--;
+ 	histactive = 0;
  	return 1;
      }
      flag = histdone;
***************
*** 652,661 ****
  	} else
  	    zsfree(ptr);
      }
-     curhistent->stim = time(NULL);
-     curhistent->ftim = 0L;
-     curhistent->flags = 0;
      if (save) {
  #ifdef DEBUG
  	/* debugging only */
  	if (chwordpos%2) {
--- 656,671 ----
  	} else
  	    zsfree(ptr);
      }
      if (save) {
+ 	Histent curhistent = gethistent(curhist);
+ 	zsfree(curhistent->text);
+ 	if (curhistent->nwords)
+ 	    zfree(curhistent->words, curhistent->nwords*2*sizeof(short));
+ 
+ 	curhistent->text = ztrdup(chline);
+ 	curhistent->stim = time(NULL);
+ 	curhistent->ftim = 0L;
+ 	curhistent->flags = 0;
  #ifdef DEBUG
  	/* debugging only */
  	if (chwordpos%2) {
***************
*** 673,689 ****
  		   curhistent->nwords*2*sizeof(short));
  	}
      } else
! 	remhist();
!     if (chline && !curhistent->text)
! 	zfree(chline, hlinesz);
!     if (curhistent->text) {
! 	char *s = ztrdup(curhistent->text);
! 
! 	zfree(curhistent->text, hlinesz);
! 	curhistent->text = s;
!     }
      zfree(chwords, chwordlen*sizeof(short));
      chline = NULL;
      return !(flag & HISTFLAG_NOEXEC || errflag);
  }
  
--- 683,693 ----
  		   curhistent->nwords*2*sizeof(short));
  	}
      } else
! 	curhist--;
!     zfree(chline, hlinesz);
      zfree(chwords, chwordlen*sizeof(short));
      chline = NULL;
+     histactive = 0;
      return !(flag & HISTFLAG_NOEXEC || errflag);
  }
  
***************
*** 693,702 ****
  void
  remhist(void)
  {
!     if (!histremmed) {
! 	histremmed = 1;
! 	curhist--;
!     }
  }
  
  /* Gives current expansion word if not last word before chwordpos. */
--- 697,713 ----
  void
  remhist(void)
  {
!     if (!(histactive & 1)) {
! 	if (!(histactive & 4)) {
! 	    /* make sure this doesn't show up when we do firsthist() */
! 	    Histent he = gethistent(curhist);
! 	    zsfree(he->text);
! 	    he->text = NULL;
! 	    histactive |= 4;
! 	    curhist--;
! 	}
!     } else
! 	histactive |= 2;
  }
  
  /* Gives current expansion word if not last word before chwordpos. */
***************
*** 1059,1067 ****
  struct histent *
  quietgethist(int ev)
  {
      if (ev < firsthist() || ev > curhist)
  	return NULL;
!     return gethistent(ev);
  }
  
  /**/
--- 1070,1090 ----
  struct histent *
  quietgethist(int ev)
  {
+     static struct histent storehist;
+ 
      if (ev < firsthist() || ev > curhist)
  	return NULL;
!     if (ev == curhist && (histactive & 1)) {
! 	/* The current history line has not been stored.  Build it up
! 	 * from other variables.
! 	 */
! 	storehist.text = chline;
! 	storehist.nwords = chwordpos/2;
! 	storehist.words = chwords;
! 
! 	return &storehist;
!     } else
! 	return gethistent(ev);
  }
  
  /**/
***************
*** 1092,1107 ****
  getargs(Histent elist, int arg1, int arg2)
  {
      char *ret;
!     short *words;
!     int pos1, nwords;
! 
!     if (elist == curhistent) {
!       words = chwords;
!       nwords = chwordpos/2;
!     } else {
!       words = elist->words;
!       nwords = elist->nwords;
!     }
  
      if (arg1 >= nwords || arg2 >= nwords) {
  	/* remember, argN is indexed from 0, nwords is total no. of words */
--- 1115,1122 ----
  getargs(Histent elist, int arg1, int arg2)
  {
      char *ret;
!     short *words = elist->words;
!     int pos1, nwords = elist->nwords;
  
      if (arg1 >= nwords || arg2 >= nwords) {
  	/* remember, argN is indexed from 0, nwords is total no. of words */
*** Src/lex.c.oldh	Wed Jan  8 14:13:19 1997
--- Src/lex.c	Wed Jan  8 14:06:41 1997
***************
*** 47,53 ****
      int dbparens;
      int isfirstln;
      int isfirstch;
!     int histremmed;
      int histdone;
      int spaceflag;
      int stophist;
--- 47,53 ----
      int dbparens;
      int isfirstln;
      int isfirstch;
!     int histactive;
      int histdone;
      int spaceflag;
      int stophist;
***************
*** 95,101 ****
      ls->dbparens = dbparens;
      ls->isfirstln = isfirstln;
      ls->isfirstch = isfirstch;
!     ls->histremmed = histremmed;
      ls->histdone = histdone;
      ls->spaceflag = spaceflag;
      ls->stophist = stophist;
--- 95,101 ----
      ls->dbparens = dbparens;
      ls->isfirstln = isfirstln;
      ls->isfirstch = isfirstch;
!     ls->histactive = histactive;
      ls->histdone = histdone;
      ls->spaceflag = spaceflag;
      ls->stophist = stophist;
***************
*** 140,146 ****
      dbparens = lstack->dbparens;
      isfirstln = lstack->isfirstln;
      isfirstch = lstack->isfirstch;
!     histremmed = lstack->histremmed;
      histdone = lstack->histdone;
      spaceflag = lstack->spaceflag;
      stophist = lstack->stophist;
--- 140,146 ----
      dbparens = lstack->dbparens;
      isfirstln = lstack->isfirstln;
      isfirstch = lstack->isfirstch;
!     histactive = lstack->histactive;
      histdone = lstack->histdone;
      spaceflag = lstack->spaceflag;
      stophist = lstack->stophist;
 
-- 
Peter Stephenson <pws@xxxxxx>       Tel: +49 33762 77366
WWW:  http://www.ifh.de/~pws/       Fax: +49 33762 77413
Deutsches Elektronen-Synchrotron --- Institut fuer Hochenergiephysik Zeuthen
DESY-IfH, 15735 Zeuthen, Germany.