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: History bug with "print -s"

[Sven Wischnowsky]

Wayne Davison wrote:

> 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.

Yes, that's what Bart wanted. And it would be nice to have, wouldn't it?

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

I've committed it.

Bye
 Sven


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