r problem

r problem

[Tanaka Akira]

I found a problem with a builtin command `r'.

% zsh -f
coulee% r
r
r
r
r
...
r
r
r
r
zsh: job table full or recursion limit exceeded
coulee% echo $ZSH_VERSION
4.0.2
coulee%  
-- 
Tanaka Akira

Re: r problem

[Bart Schaefer]

This looks like a bug in gethistent().  It's returning the current command
as the previous command when the history should appear to be empty.  Wayne?

Another bug (?) is that the "r" command itself gets entered in the history
even when hist_no_store is set.  On the other hand, "history" and "fc" get
left out of the history with hist_no_store even when they've been redifined
as functions that have nothing to do with the history.  Hrm.

-- 
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: r problem

[Wayne Davison]

On Wed, 12 Sep 2001, Bart Schaefer wrote:
> This looks like a bug in gethistent().

It was actually a problem with addhistnum() in how it was choosing to
return boundary values.  I changed this, and then checked everything
that was calling addhistnum() to ensure that they should be able to
handle the new behavior.

> Another bug (?) is that the "r" command itself gets entered in the history
> even when hist_no_store is set.

Yes, I hadn't remembered to add that to the hist_no_store check.

> On the other hand, "history" and "fc" get left out of the history with
> hist_no_store even when they've been redefined as functions that have
> nothing to do with the history.

They are no longer omitted if you alias them, so that is a better way
to go at the moment.  With the advent of my HIST_TMPSTORE flag, I could
change the builtins to set this value and get rid of the command-line
scan.  I'll look into this later to see if I think this is a good idea.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/builtin.c
--- Src/builtin.c	2001/08/13 17:43:04	1.51
+++ Src/builtin.c	2001/09/13 18:12:12
@@ -1299,8 +1299,8 @@
     if (last == -1)
 	last = ops['l']? addhistnum(curline.histnum,-1,0) : first;
     if (first < firsthist())
-	first = firsthist();
-    if (last == -1)
+	first = firsthist() - (first == last);
+    if (last > curhist)
 	last = (minflag) ? curhist : first;
     else if (last < first)
 	last = first;
@@ -1368,7 +1368,7 @@
     if ((cmd = atoi(s))) {
 	if (cmd < 0)
 	    cmd = addhistnum(curline.histnum,cmd,HIST_FOREIGN);
-	if (cmd >= curline.histnum) {
+	if (cmd < firsthist()) {
 	    zwarnnam("fc", "bad history number: %d", 0, cmd);
 	    return -1;
 	}
Index: Src/hist.c
--- Src/hist.c	2001/08/07 19:53:19	1.31
+++ Src/hist.c	2001/09/13 18:12:13
@@ -845,7 +845,7 @@
     if (n)
 	he = movehistent(he, n, xflags);
     if (!he)
-	return dir < 0? firsthist() : curhist;
+	return dir < 0? firsthist() - 1 : curhist + 1;
     return he->histnum;
 }

@@ -993,6 +993,8 @@
 	    b += 8;
 	if (*b == 'h' && strncmp(b, "history", 7) == 0
 	 && (!b[7] || b[7] == ' '))
+	    return 1;
+	if (*b == 'r' && (!b[1] || b[1] == ' '))
 	    return 1;
 	if (*b == 'f' && b[1] == 'c' && b[2] == ' ' && b[3] == '-') {
 	    b += 3;
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: r problem

[Wayne Davison]

On Thu, 13 Sep 2001, Wayne Davison wrote:
> With the advent of my HIST_TMPSTORE flag, I could change the builtins
> to set this value and get rid of the command-line scan.  I'll look
> into this later to see if I think this is a good idea.

It's not.  That would be too much like what we used to have, and the
side-effects of dumping a history entry from inside a builtin function
are not something that I want to revisit.

So, we currently have the problem that I can define a function named "r"
and with HIST_NO_STORE set, invoking that function from the command-line
drops the command from the history.

One potential solution to this would be to change the history-line-drop
code to lookup "r" in a function list and avoid dropping "r" (but not
"builtin r") if we find it.  I don't know the function code well enough
to know if there is an inexpensive call that should_ignore_line() could
call for this or not.

Another solution is to leave things alone and let people use an alias to
redefine "r".  Like this:

    function r_func { echo foo }
    alias r=r_func

That's not as nice, but it works.  Comments?

..wayne..

Re: r problem

[Bart Schaefer]

On Sep 13,  5:52pm, Wayne Davison wrote:
}
} One potential solution to this would be to change the history-line-drop
} code to lookup "r" in a function list and avoid dropping "r" (but not
} "builtin r") if we find it.  I don't know the function code well enough
} to know if there is an inexpensive call that should_ignore_line() could
} call for this or not.

It's just a hash lookup:

	if (shfunctab->getnode(shfunctab, "r")) {
	    /* It's a function */
	}

As it would only be happening after the command name had already been
compared to (fc|history|r), I don't think it's prohibitively expensive,
though it's exactly what exec.c is going to be doing a few instructions
later.

-- 
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: r problem

[Wayne Davison]

On Sat, 15 Sep 2001, Bart Schaefer wrote:
> It's just a hash lookup:

Excellent.  That saves me some poking around that I hadn't gotten around
to doing.  Here's a patch that seems to fix things up quite well.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/hist.c
--- Src/hist.c	2001/09/13 18:19:11	1.32
+++ Src/hist.c	2001/09/15 06:22:08
@@ -989,14 +989,20 @@

     if (isset(HISTNOSTORE)) {
 	char *b = getjobtext(prog, NULL);
-	if (*b == 'b' && strncmp(b, "builtin ", 8) == 0)
+	int saw_builtin;
+	if (*b == 'b' && strncmp(b,"builtin ",8) == 0) {
 	    b += 8;
-	if (*b == 'h' && strncmp(b, "history", 7) == 0
-	 && (!b[7] || b[7] == ' '))
+	    saw_builtin = 1;
+	} else
+	    saw_builtin = 0;
+	if (*b == 'h' && strncmp(b,"history",7) == 0 && (!b[7] || b[7] == ' ')
+	 && (saw_builtin || !shfunctab->getnode(shfunctab,"history")))
 	    return 1;
-	if (*b == 'r' && (!b[1] || b[1] == ' '))
+	if (*b == 'r' && (!b[1] || b[1] == ' ')
+	 && (saw_builtin || !shfunctab->getnode(shfunctab,"r")))
 	    return 1;
-	if (*b == 'f' && b[1] == 'c' && b[2] == ' ' && b[3] == '-') {
+	if (*b == 'f' && b[1] == 'c' && b[2] == ' ' && b[3] == '-'
+	 && (saw_builtin || !shfunctab->getnode(shfunctab,"fc"))) {
 	    b += 3;
 	    do {
 		if (*++b == 'l')
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---