fc -l doesn't match %h number?

fc -l doesn't match %h number?

[Rocky Bernstein]

It looks to me like "fc -l" is not showing a command in the history
that %h is counting when one enters a new subshell.
Consider this program:

#==============================
fc -ap /tmp/hist 10 10
hook() {
    line=''
    fc -l 2>/dev/null
    print subshell level: $ZSH_SUBSHELL
    print $ZSH_DEBUG_CMD
    vared -e -h -p 'hist<%h> ' line
    [[ $line == 'q' ]] && exit
    print -s -- "$line"
}

trap 'hook' DEBUG
x=2
( x=3;
  y=4
)
x=5
#==============================

Now let's run:
$ zsh fc.sh

subshell level: 0
x=2
hist<1> a

    1  a
subshell level: 0
(
	x=3
	y=4
)
hist<2> b

    1  a
subshell level: 1
x=3
hist<3>
# ...


Notice that the numbers in angle brackets keep increasing <1>, <2>,
<3> as they should. However above "hist<3>" we just see "1  a" listed
and not "b".

Am I doing something wrong? Thought's about what's going on here?

Thanks.

Re: fc -l doesn't match %h number?

[Bart Schaefer]

On Oct 1, 11:54am, Rocky Bernstein wrote:
} 
} Am I doing something wrong? Thought's about what's going on here?

Subshells have their own copy of the history, because they're forked.
I think the history mechanism is becoming confused because it's not
expecting to have anything added to the history in the middle of
command execution.

If you keep going from where you stopped:

hist<3> c
    1  a
    2  b
subshell level: 1
y=4 
hist<4> d
    1  a
subshell level: 0
x=5 
hist<3> 

Note that everything that happened at "level: 1" has vanished, including
the value shown by %h, when we get back to "level: 0".  Add a few more
lines to the script so as to keep going and:

hist<3> e
    1  a
    2  b
subshell level: 0
x=6 
hist<4> f
    1  a
    2  b
    3  e
subshell level: 0
x=7 
hist<5> q

Replace the ( ) with { } and it all works, so it has something to do
with prepping to fork.

Also, nothing is ever written to /tmp/hist even if INC_APPEND_HISTORY
is set, which is probably be a bug in "print -s".

Re: fc -l doesn't match %h number?

[Rocky Bernstein]

On Thu, Oct 2, 2008 at 2:21 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 1, 11:54am, Rocky Bernstein wrote:
...
> Note that everything that happened at "level: 1" has vanished, including
> the value shown by %h, when we get back to "level: 0".

I didn't mention this because I expect this. This is how subshells
work in general and if you look at the full debugger, this part in
fact is handled both here and in the more general case (aside from
bugs in my code).

I don't expect %h and the number of entries in  fc -l to be inconsistent though.

Re: fc -l doesn't match %h number?

[Peter Stephenson]

On Wed, 1 Oct 2008 11:54:45 -0400
"Rocky Bernstein" <rocky.bernstein@gmail.com> wrote:
> It looks to me like "fc -l" is not showing a command in the history
> that %h is counting when one enters a new subshell.

I had a moment to look at this when it seemed futile to try to start
something new...

I don't think the problem is the subshell, I think it's a question of
consistently looking for the latest line that's been added.  (As previously
mentioned, there is an inevitable issue with losing stuff on leaving
subshells which we're not worrying about, that's just how they work.)

If you do "fc -l" or "history" at the command line it deliberately
suppresses that command itself from the listing, even though it has by
then been added to the history list as that's done when the line has been
read and before execution (which, I think, makes perfect sense).  The same
suppression is happening here, but the line in question is the one that's
been added by "print -s", so you would normally expect to see it.  So it's
not that the line number to list isn't being incremented, it's just that
it's suppressing more than you expect.

Other glitches with the current way of doing things appeared when I added a
second "print -s", causing the last edited line to go further out of step
with the last entry in the history, and more entries to be missed from the
listing.

I think this patch has a chance of fixing it without doing anything
horrible --- it shouldn't change the usual interactive behaviour, when
there's no manipulation of the history behind the user's back.  There are
basically two points:

(i) When listing, we want to base the list on the last line added, not the
last line edited by the user, since these drift away from one another when
you add lines using "print -s" (that's my second discovery that's not
displayed by Rocky's original test).  We do not want to do this when
reexecuting instead of listing, since we could end up reexecuting a line
added with "print -s" that the user hasn't seen, so I've left that the way
it is.  (The presence of "print -s" stuff catches up with you eventually if
you do !-2 from the next command line, or whatever, but there's no escaping
that without a completely separate numbering scheme.)

(ii) If we find that that the last line added is not the same as the last
edited line, we want to show all lines added, since it means the user has
added a line non-interactively since the line edited and wants to see it.
Slight complication: although the last edited line was therefore not a line
directly asking for history to be listed, it might have been a line that
triggered a "print -s", then asked for history to be listed.  However, I
think that's OK: the fact there's that extra entry in between is enough to
show the user something's happened to warrant the extra history showing up.

It's vaguely possible there's some case I've missed that means that the
last edited line drifts away from the last added line in some other way that
might cause the last entry not to be the correct one.  Luckily, the
temporary history mechanism (the last line is always there for immediate
recall even if it won't be saved long term) seems to fix this up in the
cases I've tried.

The explanation and comments are considerably longer than the three
lines of code that have changed.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.210
diff -u -r1.210 builtin.c
--- Src/builtin.c	29 Sep 2008 08:46:31 -0000	1.210
+++ Src/builtin.c	6 Oct 2008 17:43:16 -0000
@@ -1416,7 +1416,19 @@
     /* default values of first and last, and range checking */
     if (last == -1) {
 	if (OPT_ISSET(ops,'l') && first < curhist) {
-	    last = addhistnum(curline.histnum,-1,0);
+	    /*
+	     * When listing base our calculations on curhist,
+	     * to show anything added since the edited history line.
+	     * Also, in that case curhist will have been modified
+	     * past the current history line; then we want to
+	     * show everything, because the user expects to
+	     * see the result of "print -s".  Otherwise, we subtract
+	     * -1 from the line, because the user doesn't usually expect
+	     * to see the command line that caused history to be
+	     * listed.
+	     */
+	    last = (curline.histnum == curhist) ? addhistnum(curhist,-1,0)
+		: curhist;
 	    if (last < firsthist())
 		last = firsthist();
 	}
@@ -1424,7 +1436,15 @@
 	    last = first;
     }
     if (first == -1) {
-	first = OPT_ISSET(ops,'l')? addhistnum(curline.histnum,-16,0)
+	/*
+	 * When listing, we want to see everything that's been
+	 * added to the history, including by print -s, so use
+	 * curhist.
+	 * When reexecuting, we want to restrict to the last edited
+	 * command line to avoid giving the user a nasty turn
+	 * if some helpful soul ran "print -s 'rm -rf /'".
+	 */
+	first = OPT_ISSET(ops,'l')? addhistnum(curhist,-16,0)
 			: addhistnum(curline.histnum,-1,0);
 	if (first < 1)
 	    first = 1;


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: fc -l doesn't match %h number?

[Rocky Bernstein]

Sorry for the delay. Had a chance to try out and remove some other bugs I had.

Everything looks great now and with this zshdb is at a minimal stage
for general release.

On Mon, Oct 6, 2008 at 2:10 PM, Peter Stephenson <pws@csr.com> wrote:
> On Wed, 1 Oct 2008 11:54:45 -0400
> "Rocky Bernstein" <rocky.bernstein@gmail.com> wrote:
>> It looks to me like "fc -l" is not showing a command in the history
>> that %h is counting when one enters a new subshell.
>
> I had a moment to look at this when it seemed futile to try to start
> something new...
>
> I don't think the problem is the subshell, I think it's a question of
> consistently looking for the latest line that's been added.  (As previously
> mentioned, there is an inevitable issue with losing stuff on leaving
> subshells which we're not worrying about, that's just how they work.)
>
> If you do "fc -l" or "history" at the command line it deliberately
> suppresses that command itself from the listing, even though it has by
> then been added to the history list as that's done when the line has been
> read and before execution (which, I think, makes perfect sense).  The same
> suppression is happening here, but the line in question is the one that's
> been added by "print -s", so you would normally expect to see it.  So it's
> not that the line number to list isn't being incremented, it's just that
> it's suppressing more than you expect.
>
> Other glitches with the current way of doing things appeared when I added a
> second "print -s", causing the last edited line to go further out of step
> with the last entry in the history, and more entries to be missed from the
> listing.
>
> I think this patch has a chance of fixing it without doing anything
> horrible --- it shouldn't change the usual interactive behaviour, when
> there's no manipulation of the history behind the user's back.  There are
> basically two points:
>
> (i) When listing, we want to base the list on the last line added, not the
> last line edited by the user, since these drift away from one another when
> you add lines using "print -s" (that's my second discovery that's not
> displayed by Rocky's original test).  We do not want to do this when
> reexecuting instead of listing, since we could end up reexecuting a line
> added with "print -s" that the user hasn't seen, so I've left that the way
> it is.  (The presence of "print -s" stuff catches up with you eventually if
> you do !-2 from the next command line, or whatever, but there's no escaping
> that without a completely separate numbering scheme.)
>
> (ii) If we find that that the last line added is not the same as the last
> edited line, we want to show all lines added, since it means the user has
> added a line non-interactively since the line edited and wants to see it.
> Slight complication: although the last edited line was therefore not a line
> directly asking for history to be listed, it might have been a line that
> triggered a "print -s", then asked for history to be listed.  However, I
> think that's OK: the fact there's that extra entry in between is enough to
> show the user something's happened to warrant the extra history showing up.
>
> It's vaguely possible there's some case I've missed that means that the
> last edited line drifts away from the last added line in some other way that
> might cause the last entry not to be the correct one.  Luckily, the
> temporary history mechanism (the last line is always there for immediate
> recall even if it won't be saved long term) seems to fix this up in the
> cases I've tried.
>
> The explanation and comments are considerably longer than the three
> lines of code that have changed.
>
> Index: Src/builtin.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
> retrieving revision 1.210
> diff -u -r1.210 builtin.c
> --- Src/builtin.c       29 Sep 2008 08:46:31 -0000      1.210
> +++ Src/builtin.c       6 Oct 2008 17:43:16 -0000
> @@ -1416,7 +1416,19 @@
>     /* default values of first and last, and range checking */
>     if (last == -1) {
>        if (OPT_ISSET(ops,'l') && first < curhist) {
> -           last = addhistnum(curline.histnum,-1,0);
> +           /*
> +            * When listing base our calculations on curhist,
> +            * to show anything added since the edited history line.
> +            * Also, in that case curhist will have been modified
> +            * past the current history line; then we want to
> +            * show everything, because the user expects to
> +            * see the result of "print -s".  Otherwise, we subtract
> +            * -1 from the line, because the user doesn't usually expect
> +            * to see the command line that caused history to be
> +            * listed.
> +            */
> +           last = (curline.histnum == curhist) ? addhistnum(curhist,-1,0)
> +               : curhist;
>            if (last < firsthist())
>                last = firsthist();
>        }
> @@ -1424,7 +1436,15 @@
>            last = first;
>     }
>     if (first == -1) {
> -       first = OPT_ISSET(ops,'l')? addhistnum(curline.histnum,-16,0)
> +       /*
> +        * When listing, we want to see everything that's been
> +        * added to the history, including by print -s, so use
> +        * curhist.
> +        * When reexecuting, we want to restrict to the last edited
> +        * command line to avoid giving the user a nasty turn
> +        * if some helpful soul ran "print -s 'rm -rf /'".
> +        */
> +       first = OPT_ISSET(ops,'l')? addhistnum(curhist,-16,0)
>                        : addhistnum(curline.histnum,-1,0);
>        if (first < 1)
>            first = 1;
>
>
> --
> Peter Stephenson <pws@csr.com>                  Software Engineer
> CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
> Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070
>