histignoredups done properly

histignoredups done properly

[Peter Stephenson]

Here's another largely cosmetic patch, though this one is more likely
to have unpleasant side effects I haven't noticed and should now
probably be delayed until after 3.0 is released.  (I don't think it
*does*, but virtually any code with pointers and history lists
*might*.)

This fixes histignoredups so that only lines which are really
different are stored; insignificant changes in whitespace are not
treated as differences.  Significant whitespace changes, for example
inside quotation marks, are treated as a difference.  The code simply
uses the extent of command line words which is already recorded.

There is one subtlety:  since you may deliberately alter just the
whitespace on a line, for example pruning a space at the end which
resulted from completion, it is the more recent version which is
stored in the history list, replacing the older one.

One consequent change is that the start time in the extended history
now always reflects the start time of the last command, rather than
the start time of the first command of a repeated sequence.  The
previous behaviour is easy to restore if that seems better (it doesn't
to me, but I can see possible arguments for it).

*** Src/hist.c.id	Fri Aug  2 15:07:29 1996
--- Src/hist.c	Mon Aug 12 14:26:51 1996
***************
*** 627,632 ****
--- 627,653 ----
  	histremmed = 1;
  }
  
+ /* compare current line with history entry using only text in words */
+ 
+ /**/
+ int
+ histcmp(Histent he)
+ {
+     int iword, lword;
+     int nwords = chwordpos/2;
+ 
+     if (nwords != he->nwords)
+ 	return 1;
+ 
+     for (iword = 0; iword < 2*nwords; iword += 2)
+ 	if ((lword = chwords[iword+1]-chwords[iword])
+ 	    != he->words[iword+1]-he->words[iword] ||
+ 	    memcmp(he->text+he->words[iword], chline+chwords[iword], lword))
+ 	    return 1;
+ 
+     return 0;
+ }
+ 
  /* say we're done using the history mechanism */
  
  /**/
***************
*** 634,640 ****
  hend(void)
  {
      int flag, save = 1;
!     Histent he;
  
      if (!chline)
  	return 1;
--- 655,661 ----
  hend(void)
  {
      int flag, save = 1;
!     Histent he = NULL;
  
      if (!chline)
  	return 1;
***************
*** 655,665 ****
  		*--hptr = '\0';
  	    } else
  		save = 0;
- 	he = gethistent(curhist - 1);
  	if (!*chline || !strcmp(chline, "\n") ||
- 	    (isset(HISTIGNOREDUPS) && he->text && !strcmp(he->text, chline)) ||
  	    (isset(HISTIGNORESPACE) && spaceflag))
  	    save = 0;
      }
      if (flag & (HISTFLAG_DONE | HISTFLAG_RECALL)) {
  	char *ptr;
--- 676,701 ----
  		*--hptr = '\0';
  	    } else
  		save = 0;
  	if (!*chline || !strcmp(chline, "\n") ||
  	    (isset(HISTIGNORESPACE) && spaceflag))
  	    save = 0;
+ 	else if (save) {
+ #ifdef DEBUG
+ 	    /* debugging only */
+ 	    if (chwordpos%2) {
+ 		hwend();
+ 		DPUTS(1, "internal:  uncompleted line in history");
+ 	    }
+ #endif
+ 	    /* get rid of pesky \n which we've nulled out:
+ 	     * needed here by HISTIGNOREDUPS code, but needed anyway.
+ 	     */
+ 	    if (!chline[chwords[chwordpos-2]])
+ 		chwordpos -= 2;
+ 	    he = gethistent(curhist - 1);
+ 	    if (isset(HISTIGNOREDUPS) && he->text && !histcmp(he))
+ 		save = 2;
+ 	}
      }
      if (flag & (HISTFLAG_DONE | HISTFLAG_RECALL)) {
  	char *ptr;
***************
*** 682,697 ****
      curhistent->ftim = 0L;
      curhistent->flags = 0;
      if (save) {
! #ifdef DEBUG
! 	/* debugging only */
! 	if (chwordpos%2) {
! 	    hwend();
! 	    DPUTS(1, "internal:  uncompleted line in history");
  	}
- #endif
- 	/* get rid of pesky \n which we've already nulled out */
- 	if (!chline[chwords[chwordpos-2]])
- 	    chwordpos -= 2;
  	if ((curhistent->nwords = chwordpos/2)) {
  	    curhistent->words =
  		(short *)zalloc(curhistent->nwords*2*sizeof(short));
--- 718,736 ----
      curhistent->ftim = 0L;
      curhistent->flags = 0;
      if (save) {
! 	if (save == 2) {
! 	    /* Don't duplicate history entry, but use the current rather than
! 	     * the previous one, in case minor changes were made to it.
! 	     */
! 	    Histent hold = curhistent;
! 	    zsfree(he->text);
! 	    if (he->nwords)
! 		zfree(he->words, he->nwords*2*sizeof(short));
! 	    curhist--;
! 	    *he = *curhistent;
! 	    curhistent = he;
! 	    hold->text = NULL;
  	}
  	if ((curhistent->nwords = chwordpos/2)) {
  	    curhistent->words =
  		(short *)zalloc(curhistent->nwords*2*sizeof(short));

-- 
Peter Stephenson <pws@ifh.de>       Tel: +49 33762 77366
WWW:  http://www.ifh.de/~pws/       Fax: +49 33762 77330
Deutches Electronen-Synchrotron --- Institut fuer Hochenergiephysik Zeuthen
DESY-IfH, 15735 Zeuthen, Germany.

hist_strip_spaces (was Re: histignoredups done properly)

[Wayne Davison]

Peter Stephenson writes:
> This fixes histignoredups so that only lines which are really
> different are stored; insignificant changes in whitespace are not
> treated as differences.

Nice change.  I was working on something similar over the weekend:  the
removal of insignificant spaces from history lines.  Since I think my
change complements your patch, I've based this diff on previously
applying your histignoredups patch.

Since it may be that not everyone will want this, I made it depend on a
new option, HIST_STRIP_SPACES.  I haven't modified the documentation yet,
however.

Note that since the histstrip() routine may change curhistent->text, I
had to change the cleanup code that frees chline.  The new code has the
additional benefit of being much clearer in what it is trying to do (it
took me a while to figure out what the old code was for).

..wayne..
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/globals.h
@@ -720,6 +720,7 @@
     {"histignoredups", 		'h',  0,    0},
     {"histignorespace", 	'g',  0,    0},
     {"histnostore", 		0,    0,    0},
+    {"histstripspaces",		0,    0,    0},
     {"histverify", 		0,    0,    0},
     {"hup", 			0,    0,    OPT_EMULATE|OPT_ZSH},
     {"ignorebraces", 		'I',  0,    OPT_EMULATE|OPT_SH},
Index: Src/hist.c
@@ -648,6 +648,36 @@
     return 0;
 }
 
+/**/
+void
+histstrip(Histent he)
+{
+    int i, len;
+    int limit = he->nwords*2;
+    char *str, *s;
+
+    for (i = 0, len = he->nwords-1; i < limit; i += 2)
+	len += he->words[i+1] - he->words[i];
+    if (len == he->words[limit-1])
+	return;
+
+    str = zalloc(len+1);
+
+    for (i = 0, s = str; i < limit; i += 2) {
+	len = he->words[i+1] - he->words[i];
+	memcpy(s, he->text + he->words[i], len);
+	he->words[i] = s - str;
+	he->words[i+1] = he->words[i] + len;
+	s += len;
+	*s++ = ' ';
+    }
+    s[-1] = '\0';
+
+    if (he->text != chline)
+	zsfree(he->text);
+    he->text = str;
+}
+
 /* say we're done using the history mechanism */
 
 /**/
@@ -694,7 +724,7 @@
 		chwordpos -= 2;
 	    he = gethistent(curhist - 1);
 	    if (isset(HISTIGNOREDUPS) && he->text && !histcmp(he))
-		save = 2;
+		save = isset(HISTSTRIPSPACES)? 0 : 2;
 	}
     }
     if (flag & (HISTFLAG_DONE | HISTFLAG_RECALL)) {
@@ -737,16 +767,13 @@
 	    memcpy(curhistent->words, chwords,
 		   curhistent->nwords*2*sizeof(short));
 	}
+	if (isset(HISTSTRIPSPACES))
+	    histstrip(curhistent);
     } else
 	remhist();
-    if (chline && !curhistent->text)
-	zfree(chline, hlinesz);
-    if (curhistent->text) {
-	char *s = ztrdup(curhistent->text);
-
-	zfree(curhistent->text, hlinesz);
-	curhistent->text = s;
-    }
+    if (curhistent->text == chline)
+	curhistent->text = ztrdup(chline);
+    zfree(chline, hlinesz);
     zfree(chwords, chwordlen*sizeof(short));
     chline = NULL;
     return !(flag & HISTFLAG_NOEXEC || errflag);
Index: Src/zsh.h
@@ -1102,6 +1102,7 @@
     HISTIGNOREDUPS,
     HISTIGNORESPACE,
     HISTNOSTORE,
+    HISTSTRIPSPACES,
     HISTVERIFY,
     HUP,
     IGNOREBRACES,
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: hist_strip_spaces

[Wayne Davison]

One problem with my patch is an optimization I added to check if
the line really needs to be rewritten -- it didn't properly handle
trailing spaces.  The following patch fixes this.

..wayne..
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/hist.c
@@ -658,7 +658,7 @@
 
     for (i = 0, len = he->nwords-1; i < limit; i += 2)
 	len += he->words[i+1] - he->words[i];
-    if (len == he->words[limit-1])
+    if (he->text[len] == '\0')
 	return;
 
     str = zalloc(len+1);
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: hist_strip_spaces (was Re: histignoredups done properly)

[Andrej Borsenkow]

On Mon, 12 Aug 1996, Wayne Davison wrote:

> Peter Stephenson writes:
> > This fixes histignoredups so that only lines which are really
> > different are stored; insignificant changes in whitespace are not
> > treated as differences.
> 

[ snipped ]

> Since it may be that not everyone will want this, I made it depend on a
> new option, HIST_STRIP_SPACES.  I haven't modified the documentation yet,
> however.
> 

The idea is really nice. Here is another one:

I often find itself in a position, where I use several command in a loop;
e.g. (simplified)

% vi
% cc
% ./a.out

or like. What about "HIST_IGNORE_ALL_DUPS" option, which would keep only
globally unique command line? It would be very easy to add this (now it
shold probably be combined with strip_spaces as well).

Any comments?

greetings

-------------------------------------------------------------------------
Andrej Borsenkow 		Fax:   +7 (095) 252 01 05
SNI ITS Moscow			Tel:   +7 (095) 252 13 88

NERV:  borsenkow.msk		E-Mail: borsenkow.msk@sni.de
-------------------------------------------------------------------------

Re: hist_strip_spaces

[Wayne Davison]

One last(?) problem with my too-hurried patch -- it sometimes adds
spaces between tokens that don't need them (like ';' and '>').  This
diff takes care of it.

..wayne..
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/hist.c
@@ -652,12 +652,14 @@
 void
 histstrip(Histent he)
 {
-    int i, len;
+    int i, len, needspace;
     int limit = he->nwords*2;
     char *str, *s;
 
-    for (i = 0, len = he->nwords-1; i < limit; i += 2)
-	len += he->words[i+1] - he->words[i];
+    for (i = 0, len = 0; i < limit; i += 2) {
+	len += he->words[i+1] - he->words[i]
+	     + (i > 0 && he->words[i] > he->words[i-1]);
+    }
     if (he->text[len] == '\0')
 	return;
 
@@ -665,13 +667,13 @@
 
     for (i = 0, s = str; i < limit; i += 2) {
 	len = he->words[i+1] - he->words[i];
-	memcpy(s, he->text + he->words[i], len);
+	needspace = (i < limit-2 && he->words[i+2] > he->words[i+1]);
+	memcpy(s, he->text + he->words[i], len + needspace);
 	he->words[i] = s - str;
 	he->words[i+1] = he->words[i] + len;
-	s += len;
-	*s++ = ' ';
+	s += len + needspace;
     }
-    s[-1] = '\0';
+    *s = '\0';
 
     if (he->text != chline)
 	zsfree(he->text);
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---