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

["Rocky Bernstein" <rocky.bernstein@xxxxxxxxx>]

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@xxxxxxx> wrote:
> On Wed, 1 Oct 2008 11:54:45 -0400
> "Rocky Bernstein" <rocky.bernstein@xxxxxxxxx> 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@xxxxxxx>                  Software Engineer
> CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
> Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070
>