PATCH: revamped history-search-{for,back}ward for 3.1.4

PATCH: revamped history-search-{for,back}ward for 3.1.4

[Wayne Davison]

> It's my assertion that history-search-*ward are now broken and
> should be made to behave the way they used to (but not implemented
> the way they used to be implemented).

Here's one way to implement most of the old behavior without using a
special ZLE flag:

My idea is that most of the time the search gets initialized when we
first leave the "curhist" line, and then we continue to execute the
last search when we're moving around in the history.  This makes the
search more modal than before, since the 3.0.x code used to start a
new search if you did anything but another search.

The new code allows two exceptions to start a new search while up in
the history:  if you set the mark away from the start of the line, or
if the current line becomes too short for the current search to
succeed.

Let me know what you think of the idea.

Since this behavior relies on having a working spaceinline() function
(my code depends on the mark being set to the start of the line),
either apply that patch or just change line 87 of zle_utils.c to be:

    if (mark > cs)

instead of:

    if (mark >= cs)

I happened to find and fix a few other minor problems in zle_hist.c,
but I'll send that stuff separately from this.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/Zle/zle_hist.c
@@ -369,17 +369,20 @@
 	feep();
 }
 
+static int histpos;
+
 /**/
 void
 historysearchbackward(void)
 {
-    int histpos, histmpos, hl = histline;
+    int hl = histline;
     char *s;
 
-    for (histpos = histmpos = 0; histpos < ll && !iblank(line[histpos]);
-	histpos++, histmpos++)
-	if(imeta(line[histpos]))
-	    histmpos++;
+    if (histline == curhist || ll < histpos || mark != 0) {
+	for (histpos = 0; histpos < ll && !iblank(line[histpos]); histpos++) ;
+	if (histpos < ll)
+	    histpos++;
+    }
     for (;;) {
 	hl--;
 	if (!(s = zle_get_event(hl))) {
@@ -387,7 +390,6 @@
 	    return;
 	}
 	if (metadiffer(s, (char *) line, histpos) < 0 &&
-	    iblank(s[histmpos] == Meta ? s[histmpos+1]^32 : s[histmpos]) &&
 	    metadiffer(s, (char *) line, ll))
 	    break;
     }
@@ -398,22 +400,21 @@
 void
 historysearchforward(void)
 {
-    int histpos, histmpos, hl = histline;
+    int hl = histline;
     char *s;
 
-    for (histpos = histmpos = 0; histpos < ll && !iblank(line[histpos]);
-	histpos++, histmpos++)
-	if(imeta(line[histpos]))
-	    histmpos++;
+    if (histline == curhist || ll < histpos || mark != 0) {
+	for (histpos = 0; histpos < ll && !iblank(line[histpos]); histpos++) ;
+	if (histpos < ll)
+	    histpos++;
+    }
     for (;;) {
 	hl++;
 	if (!(s = zle_get_event(hl))) {
 	    feep();
 	    return;
 	}
-	if (metadiffer(s, (char *) line, histpos) < (histline == curhist) &&
-	    (!s[histmpos] ||
-	     iblank(s[histmpos] == Meta ? s[histmpos+1]^32 : s[histmpos])) &&
+	if (metadiffer(s, (char *) line, histpos) < (hl == curhist) &&
 	    metadiffer(s, (char *) line, ll))
 	    break;
     }
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: PATCH: revamped history-search-{for,back}ward for 3.1.4

[Bart Schaefer]

On Jun 9,  3:42pm, Wayne Davison wrote:
} Subject: PATCH: revamped history-search-{for,back}ward for 3.1.4
}
} > It's my assertion that history-search-*ward are now broken and
} > should be made to behave the way they used to (but not implemented
} > the way they used to be implemented).
} 
} My idea is that most of the time the search gets initialized when we
} first leave the "curhist" line, and then we continue to execute the
} last search when we're moving around in the history.

Unfortunately, this doesn't work right.  Consider what happens in the
following case, where <M-p> is history-search-backward and <C-p> is
up-line-or-history:

zsh% ech<M-p>
zsh% echo this is a test<C-p>
zsh% fc -R historytest<M-p>

At the second <M-p>, `histpos' remains 3 (the length of "ech"), but the
first three characters of `line' are "fc ".  As a result, the wrong string
is searched for.  This is very similar to the bug that Zefram's patch
from a year and a half ago was supposed to fix; it just isn't sufficient
to remember and re-use only the `histpos'.

You can do a little better if you also save `hl' and reset the search if 
`histline' differs from the last time through, but that's still not quite
right.

} I happened to find and fix a few other minor problems in zle_hist.c,
} but I'll send that stuff separately from this.

Looking forward to it.

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

PATCH: even better history-search-{for,back}ward for 3.1.4

[Wayne Davison]

"Bart Schaefer" writes:
> Unfortunately, this doesn't work right.

Yeah, that code didn't save enough state.  I've appended a new
patch that does a better job.

> You can do a little better if you also save `hl' and reset the
> search if `histline' differs from the last time through, but
> that's still not quite right.

Cool -- I had the same idea, plus I also decided to save cs and the
search prefix, which allows the code to do a much better job at
figuring out when to start a new search.  It is still possible to
fool it into continuing the last search when the old code would have
started a new one, but it does behave much more like the code in
3.0.5 now.

A known deficiency is that if you use uplineorsearch and pass
through a multi-line history entry, the code will start a new
search, which may not be what you want (if it was searching for
a prefix and not a command name).  This should be easy enough
to fix, though.

> zsh% ech<M-p>
> zsh% echo this is a test<C-p>
> zsh% fc -R historytest<M-p>

I find it humorous that my old code did the right thing in this case
but for the wrong reason.  The new code will still search for "fc ",
but because it properly decided to start a new search.

This patch assumes that you've removed the previous one.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: zle_hist.c
@@ -369,55 +369,69 @@
 	feep();
 }
 
+static int histpos, srch_hl, srch_cs = -1;
+static char *srch_str;
+
 /**/
 void
 historysearchbackward(void)
 {
-    int histpos, histmpos, hl = histline;
+    int hl = histline;
     char *s;
 
-    for (histpos = histmpos = 0; histpos < ll && !iblank(line[histpos]);
-	histpos++, histmpos++)
-	if(imeta(line[histpos]))
-	    histmpos++;
+    if (hl == curhist || hl != srch_hl || cs != srch_cs || mark != 0
+     || memcmp(srch_str, line, histpos) != 0) {
+	zfree(srch_str, histpos);
+	for (histpos = 0; histpos < ll && !iblank(line[histpos]); histpos++) ;
+	if (histpos < ll)
+	    histpos++;
+	srch_str = zalloc(histpos);
+	memcpy(srch_str, line, histpos);
+    }
     for (;;) {
 	hl--;
 	if (!(s = zle_get_event(hl))) {
 	    feep();
 	    return;
 	}
-	if (metadiffer(s, (char *) line, histpos) < 0 &&
-	    iblank(s[histmpos] == Meta ? s[histmpos+1]^32 : s[histmpos]) &&
-	    metadiffer(s, (char *) line, ll))
+	if (metadiffer(s, srch_str, histpos) < 0 &&
+	    metadiffer(s, srch_str, ll))
 	    break;
     }
     zle_goto_hist(hl);
+    srch_hl = hl;
+    srch_cs = cs;
 }
 
 /**/
 void
 historysearchforward(void)
 {
-    int histpos, histmpos, hl = histline;
+    int hl = histline;
     char *s;
 
-    for (histpos = histmpos = 0; histpos < ll && !iblank(line[histpos]);
-	histpos++, histmpos++)
-	if(imeta(line[histpos]))
-	    histmpos++;
+    if (hl == curhist || hl != srch_hl || cs != srch_cs || mark != 0
+     || memcmp(srch_str, line, histpos) != 0) {
+	zfree(srch_str, histpos);
+	for (histpos = 0; histpos < ll && !iblank(line[histpos]); histpos++) ;
+	if (histpos < ll)
+	    histpos++;
+	srch_str = zalloc(histpos);
+	memcpy(srch_str, line, histpos);
+    }
     for (;;) {
 	hl++;
 	if (!(s = zle_get_event(hl))) {
 	    feep();
 	    return;
 	}
-	if (metadiffer(s, (char *) line, histpos) < (histline == curhist) &&
-	    (!s[histmpos] ||
-	     iblank(s[histmpos] == Meta ? s[histmpos+1]^32 : s[histmpos])) &&
-	    metadiffer(s, (char *) line, ll))
+	if (metadiffer(s, srch_str, histpos) < (hl == curhist) &&
+	    metadiffer(s, srch_str, ll))
 	    break;
     }
     zle_goto_hist(hl);
+    srch_hl = hl;
+    srch_cs = cs;
 }
 
 /**/
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: PATCH: even better history-search-{for,back}ward for 3.1.4

[Bart Schaefer]

On Jun 10,  1:51am, Wayne Davison wrote:
} Subject: PATCH: even better history-search-{for,back}ward for 3.1.4
}
} "Bart Schaefer" writes:
} > zsh% ech<M-p>
} > zsh% echo this is a test<C-p>
} > zsh% fc -R historytest<M-p>
} 
} I find it humorous that my old code did the right thing in this case
} but for the wrong reason.  The new code will still search for "fc ",
} but because it properly decided to start a new search.

I hope not ... the new code ought to search for "fc" (without the space),
because of the !iblank(line[histpos]).  By neither interpretation was the
old code doing the right thing.

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

Re: PATCH: even better history-search-{for,back}ward for 3.1.4

[Wayne Davison]

"Bart Schaefer" writes:
> I hope not ... the new code ought to search for "fc" (without the
> space),

No, the only time history-search-backward omits the space is if you
start the search without a space on the line:

% ech<M-p>

As opposed to these:

% ech <M-p>
% ech command<M-p>

These examples only search for the command "ech", as opposed to
the prefix "ech".

> [note] the !iblank(line[histpos]).

You missed the following increment of histpos if it is less than ll
(i.e., if we found a space).  This is identical to how 3.0.5 works.

..wayne..

Re: PATCH: even better history-search-{for,back}ward for 3.1.4

[Bart Schaefer]

} > I hope not ... the new code ought to search for "fc" (without the
} > space),
} 
} No, the only time history-search-backward omits the space is if you
} start the search without a space on the line

That means that a line consisting of exactly one word, say "ls", does
not match a search for `a line beginning with the first word of "ls -l"'.
It also means that a line beginning with "ls	" (tab) won't match.

I see that this has actually been a bug all along, and your patch is at
least better in that you can get rid of the space by cursor positioning;
but surely if metadiffer() can't handle `word boundary' as a token, then
it should get special-cased in the caller rather than using space as a
poor substitute ...

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

Re: PATCH: even better history-search-{for,back}ward for 3.1.4

[Wayne Davison]

"Bart Schaefer" writes:
> That means that a line consisting of exactly one word, say "ls", does
> not match a search for `a line beginning with the first word of "ls -l"'.

This is as it should be.  If someone searches for "ls ", he doesn't
want to see just "ls".  In fact, even if you're searching for the
prefix "ls" (w/o space), the code skips a line of just "ls" as
a useless match (the only exception to that is that it will allow
the match if we are returning to the "curhist" line at the end of
the history).

> It also means that a line beginning with "ls	" (tab) won't match.

Yes.  It would be nice to fix this, but this is almost never a
problem in actual practice since most people don't input tabs into
their command lines.  Your suggestion for adding a word-boundary
option to metadiffer() sounds like the right solution.

..wayne..