Re: PATCH: Re: History bug (Re: Completion debugging)

Re: PATCH: Re: History bug (Re: Completion debugging)

[Sven Wischnowsky]

I wrote:

> Bart Schaefer wrote:
>
> ...
>
> > 
> > So there are still two 113s in the history list, but the prompt has the
> > right history number.  I'm expecting that particular shell to crash any
> > time now ...
> 
> I get a SEGV reproducibly after C-p C-n. If I take out my patch for
> this, I get it after the C-p.
> 
> Hm, maybe someone more knowledgeable with history stuff...?

After playing some more and a couple of SEGVs later...

There is so much mucking around curhist (and histline) in zle that I
wonder if there is a clean way (other than changing the meaning of
curhist in the core and then adapting zle).

Maybe we should just make `print -s' put the strings added while zle
is active into some list and then add those strings only after zle is
left? I.e. in a toplevel-loop() or somewhere around that.


Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: History bug (Re: Completion debugging)

[Bart Schaefer]

On May 4,  5:34pm, Sven Wischnowsky wrote:
} Subject: Re: PATCH: Re: History bug (Re: Completion debugging)
}
} There is so much mucking around curhist (and histline) in zle that I
} wonder if there is a clean way (other than changing the meaning of
} curhist in the core and then adapting zle).

Cf. "The one icky bit" in my "Dear old literal history" message.

} Maybe we should just make `print -s' put the strings added while zle
} is active into some list and then add those strings only after zle is
} left? I.e. in a toplevel-loop() or somewhere around that.

That's approximately what "print -z" does.  At least for _complete_debug,
the whole point is to get them into the history while you can still scroll
around and look at them.

Unfortunately that means either putting them into the future history or
changing the event number of the current command.  Neither works, at
least not easily, because the history is now stored as a ring rather
than as a simple linked list.  Wayne, are you out there?

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

Re: PATCH: Re: History bug (Re: Completion debugging)

[Wayne Davison]

> Neither works, at
> least not easily, because the history is now stored as a ring rather
> than as a simple linked list.  Wayne, are you out there?

It was always (that I know of) a ring.  I just changed it from being an
array (of pointers) into being a linked list (so that it was easy to remove
items from the middle).

I'd be glad to check into this sometime tonight (unless someone beats me to
it).

..wayne..

PATCH: History bug with "print -s"

[Wayne Davison]

Appended is a patch that should fix the funkiness associated with
running "print -s" while zle is active.  I've done some very basic
testing, and it appears to work fine.

However, if "print -s" is supposed to make the line immediately
available for history browsing (before pressing return), that is not
happening.  I can check into this next, but I don't have time to do
that right now.

The following diff is based on an unpatched 3.1.7-pre-2.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/builtin.c
@@ -2777,7 +2777,7 @@
 	int nwords = 0, nlen, iwords;
 	char **pargs = args;
 
-	ent = prepnexthistent(++curhist);
+	ent = prepnexthistent();
 	while (*pargs++)
 	    nwords++;
 	if ((ent->nwords = nwords)) {
Index: Src/hist.c
@@ -704,6 +704,36 @@
 {
 }
 
+/* these functions handle adding/removing curline to/from the hist_ring */
+
+static void
+linkcurline(void)
+{
+    if (!hist_ring)
+	hist_ring = curline.up = curline.down = &curline;
+    else {
+	curline.up = hist_ring;
+	curline.down = hist_ring->down;
+	hist_ring->down = hist_ring->down->up = &curline;
+	hist_ring = &curline;
+    }
+    curline.histnum = ++curhist;
+}
+
+static void
+unlinkcurline(void)
+{
+    curline.up->down = curline.down;
+    curline.down->up = curline.up;
+    if (hist_ring == &curline) {
+	if (!histlinect)
+	    hist_ring = NULL;
+	else
+	    hist_ring = curline.up;
+    }
+    curhist--;
+}
+
 /* initialize the history mechanism */
 
 /**/
@@ -745,15 +775,7 @@
     if (interact && isset(SHINSTDIN) && !strin) {
 	histactive = HA_ACTIVE;
 	attachtty(mypgrp);
-	if (!hist_ring)
-	    hist_ring = curline.up = curline.down = &curline;
-	else {
-	    curline.up = hist_ring;
-	    curline.down = hist_ring->down;
-	    hist_ring->down = hist_ring->down->up = &curline;
-	    hist_ring = &curline;
-	}
-	curline.histnum = ++curhist;
+	linkcurline();
 	defev = addhistnum(curhist, -1, HIST_FOREIGN);
     } else
 	histactive = HA_ACTIVE | HA_NOINC;
@@ -883,9 +905,13 @@
 
 /**/
 Histent
-prepnexthistent(int histnum)
+prepnexthistent(void)
 {
     Histent he;
+    int curline_in_ring = hist_ring == &curline;
+
+    if (curline_in_ring)
+	unlinkcurline();
 
     if (histlinect < histsiz) {
 	he = (Histent)zcalloc(sizeof *he);
@@ -920,8 +946,10 @@
 	}
 	freehistdata(hist_ring = he, 0);
     }
-    hist_ring->histnum = histnum;
-    return hist_ring;
+    he->histnum = ++curhist;
+    if (curline_in_ring)
+	linkcurline();
+    return he;
 }
 
 /* say we're done using the history mechanism */
@@ -937,17 +965,8 @@
 	  "BUG: chline is NULL in hend()");
     if (histdone & HISTFLAG_SETTY)
 	settyinfo(&shttyinfo);
-    if (!(histactive & HA_NOINC)) {
-	curline.up->down = curline.down;
-	curline.down->up = curline.up;
-	if (hist_ring == &curline) {
-	    if (!histlinect)
-		hist_ring = NULL;
-	    else
-		hist_ring = curline.up;
-	}
-	curhist--;
-    }
+    if (!(histactive & HA_NOINC))
+	unlinkcurline();
     if (histactive & (HA_NOSTORE|HA_NOINC)) {
 	zfree(chline, hlinesz);
 	zfree(chwords, chwordlen*sizeof(short));
@@ -1023,7 +1042,7 @@
 	    freehistdata(he, 0);
 	} else {
 	    keepflags = 0;
-	    he = prepnexthistent(++curhist);
+	    he = prepnexthistent();
 	}
 
 	he->text = ztrdup(chline);
@@ -1777,7 +1796,7 @@
 		lasthist.stim = stim;
 	    }
 
-	    he = prepnexthistent(++curhist);
+	    he = prepnexthistent();
 	    he->text = ztrdup(pt);
 	    he->flags = newflags;
 	    if ((he->stim = stim) == 0)
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: PATCH: Re: History bug (Re: Completion debugging)

[Sven Wischnowsky]

Bart Schaefer wrote:

> On May 3,  9:07am, Sven Wischnowsky wrote:
> } Subject: PATCH: Re: History bug (Re: Completion debugging)
> }
> } > Why are there two of numbers 24, 29 and 32?  In each case, the first of the
> } > two was inserted by calling "print -s ..." during completion.  Apparently
> } > that doesn't work very well.
> } 
> } Of course this isn't only in completion, but in every widget.
> } 
> } The problem is that the history number for the currently edited line
> } is `reserved' and the `print -s' makes it be used. The patch below is
> } the simplest solution I can think of.
> 
> OOOooh, this is fun.
> 
> Change the "print -zR" in _complete_debug to "print -sR", and then:
> 
> zagzig[112] ls <C-x?>
> Debugging output left in /tmp/zsh13117ls3
> (listing omitted)
> zagzig[112] ls <C-n>
> zagzig[112] emacs /tmp/zsh13117ls3 ;: "ls -CF "<C-p>
> zagzig[112] ls <wait a few minutes, then RET>
> (listing omitted)
> zagzig[114] history -d
>   113  09:48  emacs /tmp/zsh13117ls3 ;: "ls -CF "
>   113  09:51  ls
> zagzig[115] 
> 
> So there are still two 113s in the history list, but the prompt has the
> right history number.  I'm expecting that particular shell to crash any
> time now ...

I get a SEGV reproducibly after C-p C-n. If I take out my patch for
this, I get it after the C-p.

Hm, maybe someone more knowledgeable with history stuff...?

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

PATCH: Re: History bug (Re: Completion debugging)

[Sven Wischnowsky]

Bart Schaefer wrote:

> Look at this snippet of "history" output:
> 
>    21  cd zsh-3.1.6
>    22  setup_compinit
>    24  emacs /tmp/zsh149find1 ;: find\ -
>    24  history
>    25  emacs /tmp/zsh149find1 ;: find\ -
>    26  history
>    28  emacs /tmp/zsh149find2 ;: find\ 
>    29  emacs /tmp/zsh149echo3 ;: echo\ foo\ 
>    29  history
>    30  reload _complete_debug
>    32  emacs /tmp/zsh149find4 ;: 'find -'
>    32  history
> 
> Why are there two of numbers 24, 29 and 32?  In each case, the first of the
> two was inserted by calling "print -s ..." during completion.  Apparently
> that doesn't work very well.

Of course this isn't only in completion, but in every widget.

The problem is that the history number for the currently edited line
is `reserved' and the `print -s' makes it be used. The patch below is
the simplest solution I can think of.

Bye
 Sven

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.12
diff -u -r1.12 builtin.c
--- Src/builtin.c	2000/05/02 15:52:44	1.12
+++ Src/builtin.c	2000/05/03 07:07:17
@@ -2777,7 +2777,7 @@
 	int nwords = 0, nlen, iwords;
 	char **pargs = args;
 
-	ent = prepnexthistent(++curhist);
+	ent = prepnexthistent(zleactive ? curhist++ : ++curhist);
 	while (*pargs++)
 	    nwords++;
 	if ((ent->nwords = nwords)) {

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: History bug (Re: Completion debugging)

[Bart Schaefer]

On May 3,  9:07am, Sven Wischnowsky wrote:
} Subject: PATCH: Re: History bug (Re: Completion debugging)
}
} > Why are there two of numbers 24, 29 and 32?  In each case, the first of the
} > two was inserted by calling "print -s ..." during completion.  Apparently
} > that doesn't work very well.
} 
} Of course this isn't only in completion, but in every widget.
} 
} The problem is that the history number for the currently edited line
} is `reserved' and the `print -s' makes it be used. The patch below is
} the simplest solution I can think of.

OOOooh, this is fun.

Change the "print -zR" in _complete_debug to "print -sR", and then:

zagzig[112] ls <C-x?>
Debugging output left in /tmp/zsh13117ls3
(listing omitted)
zagzig[112] ls <C-n>
zagzig[112] emacs /tmp/zsh13117ls3 ;: "ls -CF "<C-p>
zagzig[112] ls <wait a few minutes, then RET>
(listing omitted)
zagzig[114] history -d
  113  09:48  emacs /tmp/zsh13117ls3 ;: "ls -CF "
  113  09:51  ls
zagzig[115] 

So there are still two 113s in the history list, but the prompt has the
right history number.  I'm expecting that particular shell to crash any
time now ...

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