accept-and-infer-next-history is broken?

accept-and-infer-next-history is broken?

[Bart Schaefer]

infer-next-history is OK:

schaefer[603] make<M-x>
execute: infer-next-history<RET>
schaefer[603] Src/zsh -f

But accept-and is not:

schaefer[603] make<M-x>
execute: accept-and-infer-next-history<RET>
(FEEP)
(build runs)
schaefer[604] 

I just tried accept-line-and-down-history and that works too.  I've stared
at the code and can't see why accept-and-infer-next-history fails; it is
for all intents identical to infer-next-history.

Relevant setopts:

noappendhistory       off
nobanghist            off
cshjunkiehistory      off
extendedhistory       off
histallowclobber      off
nohistbeep            off
histexpiredupsfirst   on
histfindnodups        on
histignorealldups     off
histignoredups        on
histignorespace       off
histnofunctions       off
histnostore           off
histreduceblanks      on
histsavenodups        off
histverify            off
incappendhistory      off
sharehistory          off

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: accept-and-infer-next-history is broken?

[Wayne Davison]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1775 bytes --]

On Thu, 24 May 2001, Bart Schaefer wrote:
> infer-next-history is OK:
> But accept-and is not:

Actually they both appear to share the same bug, but it gets tickled by
where you are in the history.  Since accept-and-infer-next-history
leaves you up in the history list while just infer-next-history + CR
does not, you don't usually notice the bug with just infer-next-history
(unless you try to use it multiple times in a row).

So, I think that the proper solution is to modify the algorithm like
this:

  if (we have a line in the history below us
   && the current line is unchanged from the history version) {
      use the next line;
  else
      search from the bottom of the history for a match and infer;

This means that a-a-i-n-h works just like accept-line-and-down-history
after the first inferred line.  An alternate approach would be to just
always start the search from the bottom of the history.  Thoughts?

The attached patch implements my pseudo-code above.  It also unifies the
infer algorithm in a single support function.

Another change I made was to have the code ignore the transient edits in
the history list when it is searching for and storing the inferred line
(also affecting accept-line-and-down-history).  I've been bitten by
this before:  I've searched for a line I need, I decide to put a line I
encountered on the way into the kill ring with Ctrl-U, continued on to
find the real command I wanted to run, and then pressed Ctrl-O to use
accept-line-and-down-history.  If the line I deleted was actually next
in the history, I get an empty line as the inferred next history line,
not the actual next history line.

Finally, I changed the function to be like a-l-a-down-history in that it
does not set "done = 1" if the infer fails.

..wayne..

[-- Attachment #2: Fix infer-next-history problems --]
[-- Type: TEXT/PLAIN, Size: 1982 bytes --]

Index: Src/Zle/zle_hist.c
--- Src/Zle/zle_hist.c	2000/02/23 15:18:49	1.1.1.14
+++ Src/Zle/zle_hist.c	2001/05/25 20:42:36
@@ -252,7 +252,7 @@
 
     if (!(he = movehistent(quietgethist(histline), 1, HIST_FOREIGN)))
 	return 1;
-    zpushnode(bufstack, ztrdup(ZLETEXT(he)));
+    zpushnode(bufstack, ztrdup(he->text));
     done = 1;
     stackhist = he->histnum;
     return 0;
@@ -891,23 +891,36 @@
 	kungetct = savekeys;
 }
 
+static Histent
+infernexthist(char **args)
+{
+    Histent he;
+
+    if (!(he = quietgethist(histline)))
+	return NULL;
+    if (!metadiffer(he->text, (char *)line, ll)
+     && (he = movehistent(he, 1, HIST_FOREIGN)) != NULL)
+	return he;
+    for (he = movehistent(hist_ring, -2, HIST_FOREIGN);
+	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
+	if (!metadiffer(he->text, (char *) line, ll))
+	    return movehistent(he, 1, HIST_FOREIGN);
+    }
+    return NULL;
+}
+
 /**/
 int
 acceptandinfernexthistory(char **args)
 {
     Histent he;
 
+    if (!(he = infernexthist(args)))
+	return 1;
+    zpushnode(bufstack, ztrdup(he->text));
     done = 1;
-    for (he = movehistent(quietgethist(histline), -2, HIST_FOREIGN);
-	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
-	if (!metadiffer(ZLETEXT(he), (char *) line, ll)) {
-	    he = movehistent(he, 1, HIST_FOREIGN);
-	    zpushnode(bufstack, ztrdup(ZLETEXT(he)));
-	    stackhist = he->histnum;
-	    return 0;
-	}
-    }
-    return 1;
+    stackhist = he->histnum;
+    return 0;
 }
 
 /**/
@@ -916,15 +929,11 @@
 {
     Histent he;
 
-    for (he = movehistent(quietgethist(histline), -2, HIST_FOREIGN);
-	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
-	if (!metadiffer(ZLETEXT(he), (char *) line, ll)) {
-	    he = movehistent(he, 1, HIST_FOREIGN);
-	    zle_setline(he);
-	    return 0;
-	}
-    }
-    return 1;
+    if (!(he = infernexthist(args)))
+	return 1;
+
+    zle_setline(he);
+    return 0;
 }
 
 /**/

Re: accept-and-infer-next-history is broken?

[Bart Schaefer]

On May 25,  1:47pm, Wayne Davison wrote:
} 
}   if (we have a line in the history below us
}    && the current line is unchanged from the history version) {
}       use the next line;
}   else
}       search from the bottom of the history for a match and infer;

This sounds like exactly what I expected it to do, and this ...

} Another change I made was to have the code ignore the transient edits in
} the history list when it is searching for and storing the inferred line

... is also something I'd been wishing for.

} This means that a-a-i-n-h works just like accept-line-and-down-history
} after the first inferred line.  An alternate approach would be to just
} always start the search from the bottom of the history.  Thoughts?

Well, here's the thing ... I use `setopt hist_expire_dups_first'.  My
fear is that the next thing that I'd like inferred, might have expired.
I DON'T want inferences to skip over the expired history slots and use
what follows ... I'd rather have it feep at me.  On the other hand, I'd
like to have the best chance possible of inferring something useful.  So
whether the algorithm needs to always search from the bottom depends on
what "we have a line in the history below us" means.
 
} Finally, I changed the function to be like a-l-a-down-history in that it
} does not set "done = 1" if the infer fails.

What practical effect does that have, exactly?

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: accept-and-infer-next-history is broken?

[Wayne Davison]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1936 bytes --]

On Sat, 26 May 2001, Bart Schaefer wrote:
> I DON'T want inferences to skip over the expired history slots and use
> what follows ... I'd rather have it feep at me.

Sounds like a nice idea.  I can even think of a fairly easy way to do
this, which I can implement after 4.0.1 gets released.

> So whether the algorithm needs to always search from the bottom
> depends on what "we have a line in the history below us" means.

I've been reflecting on this some more, and, I think I may like my
alternate approach better (i.e. always search from the bottom, at least
in the case of accept-and-infer-n-h).  The main reason for this is that
it gives the user the choice of how to pick the next history item.  If
you want to proceed in exactly the same sequence as before, use
accept-line-and-down-history.  If you want to make a new inference from
the current line, use accept-and-infer-next-history (which does a new
search).  It would even be logically equivalent to pressing CR +
up-arrow + infer-next-history, so that even argues in its favor, IMO.

> } Finally, I changed the function to be like a-l-a-down-history in
> } that it does not set "done = 1" if the infer fails.
>
> What practical effect does that have, exactly?

Translation:  if it can't infer a new line, it feeps and doesn't accept
the line either.  This is how accept-line-and-down-history works, and I
thought that it was a good idea for accept-and-infer-next-history to
work this way too.

OK, so here's an alternate patch to my first one.  This version does not
change the way infer-next-history works.  This allows the user to press
up-arrow and infer-next-history to try to find a match further back in
the history after each infer-next-history.  One way that this is useful
is that if you're using accept-and-infer-next-history over and over
again and you end up in the wrong sequence, using up-arrow +
infer-next-history should get you back on track.

..wayne..

[-- Attachment #2: New infer-next-history patch --]
[-- Type: TEXT/PLAIN, Size: 1817 bytes --]

Index: Src/Zle/zle_hist.c
--- Src/Zle/zle_hist.c	2000/02/23 15:18:49	1.1.1.14
+++ Src/Zle/zle_hist.c	2001/05/26 06:36:02
@@ -252,7 +252,7 @@
 
     if (!(he = movehistent(quietgethist(histline), 1, HIST_FOREIGN)))
 	return 1;
-    zpushnode(bufstack, ztrdup(ZLETEXT(he)));
+    zpushnode(bufstack, ztrdup(he->text));
     done = 1;
     stackhist = he->histnum;
     return 0;
@@ -891,23 +891,29 @@
 	kungetct = savekeys;
 }
 
+static Histent
+infernexthist(Histent he, char **args)
+{
+    for (he = movehistent(he, -2, HIST_FOREIGN);
+	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
+	if (!metadiffer(he->text, (char *) line, ll))
+	    return movehistent(he, 1, HIST_FOREIGN);
+    }
+    return NULL;
+}
+
 /**/
 int
 acceptandinfernexthistory(char **args)
 {
     Histent he;
 
+    if (!(he = infernexthist(hist_ring, args)))
+	return 1;
+    zpushnode(bufstack, ztrdup(he->text));
     done = 1;
-    for (he = movehistent(quietgethist(histline), -2, HIST_FOREIGN);
-	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
-	if (!metadiffer(ZLETEXT(he), (char *) line, ll)) {
-	    he = movehistent(he, 1, HIST_FOREIGN);
-	    zpushnode(bufstack, ztrdup(ZLETEXT(he)));
-	    stackhist = he->histnum;
-	    return 0;
-	}
-    }
-    return 1;
+    stackhist = he->histnum;
+    return 0;
 }
 
 /**/
@@ -916,15 +922,10 @@
 {
     Histent he;
 
-    for (he = movehistent(quietgethist(histline), -2, HIST_FOREIGN);
-	 he; he = movehistent(he, -1, HIST_FOREIGN)) {
-	if (!metadiffer(ZLETEXT(he), (char *) line, ll)) {
-	    he = movehistent(he, 1, HIST_FOREIGN);
-	    zle_setline(he);
-	    return 0;
-	}
-    }
-    return 1;
+    if (!(he = infernexthist(quietgethist(histline), args)))
+	return 1;
+    zle_setline(he);
+    return 0;
 }
 
 /**/