zcalc bug

zcalc bug

[Matthias Kopfermann]

I found another confusing bug the following way when i used zcalc:
This is with a huge history-file of over 40000 entries which may be
very well related to the bug.

I called zcalc that way: (locale is enabled in zsh!)
`LANG=en_US LC_ALL=en_US zcalc'.

First, the result of the multiplication (2.8*16.0 =>44,8) had a `,' in it
which i didn't understand as i thought there would be a point in the output
when using `LC_ALL=en_US'.

I got the expected _point_ only with LC_ALL=C.

Second, after I returned to zsh my history-number-prompt had a total
wrong number with about 81000 entries in it, as it seemed looking at
the history-number-prompt.
My right prompt was not the way it was before, too. This happens when
there are lots and lots of entries in the path-part of the prompt which
then "confuses" my cvs-zsh.

my PS1 prompt is:

PS1=^O%{^[[34m%}_________%{^[[0m%}_________%{^[[34m%}_________%{^[[0m%}_________%{^[[34m%}_________%{^[[34m%}_________%{^[[0m%}_________
[%{^[[01m%}%{^[[32m%}%~%{^[[0m%}] %{^[[01m%}%{^[[0m%}TTY:%{^[[32m%}%l%{^[[0m%}__hist:%h%{^[[0m%}__%D{%d.%m.%y} %{^[[0m%}%{^[[01m%}%{^[[1m^[[41m%} %?
%{^[[01m%}%{^[[33m%}%v%{^[[0m%}%#%B %{^[[0m%}

my RPS1 is %T %{^[[1;34m%}`ipa`%{^[[m%}
where ipa shows my ip-address.


	Matthias

Re: zcalc bug

[Thomas Köhler]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

Hi,

Matthias Kopfermann wrote:
> I found another confusing bug the following way when i used zcalc:
> This is with a huge history-file of over 40000 entries which may be
> very well related to the bug.
> 
> I called zcalc that way: (locale is enabled in zsh!)
> `LANG=en_US LC_ALL=en_US zcalc'.
[...]
> Second, after I returned to zsh my history-number-prompt had a total
> wrong number with about 81000 entries in it, as it seemed looking at
> the history-number-prompt.
> My right prompt was not the way it was before, too. This happens when
> there are lots and lots of entries in the path-part of the prompt which
> then "confuses" my cvs-zsh.

As everybody seems to ignore this mail, I'll add a bit of
"problem can be seen here, too" (don't be confused by my lengthy
prompt, which includes lots of information) ;)

 15:52:29 up  6:50, 15 users,  load average: 0.15, 0.32, 0.32 
--INSERT--  zsh version: 4.2.0  Return Code: 0 
502 jean-luc@picard TTY:pts/15
~> print -P %h
502
 15:52:33 up  6:50, 15 users,  load average: 0.14, 0.32, 0.32 
--INSERT--  zsh version: 4.2.0  Return Code: 0 
503 jean-luc@picard TTY:pts/15
~> autoload zcalc
 15:53:15 up  6:51, 15 users,  load average: 0.06, 0.27, 0.30 
--INSERT--  zsh version: 4.2.0  Return Code: 0 
504 jean-luc@picard TTY:pts/15
~> zcalc
1> 1+2+3
6
2> 3*4
12
3> 
 15:53:29 up  6:51, 15 users,  load average: 0.05, 0.26, 0.30 
--INSERT--  zsh version: 4.2.0  Return Code: 0 
1014 jean-luc@picard TTY:pts/15
~> print -P %h
1014
 15:53:33 up  6:51, 15 users,  load average: 0.05, 0.26, 0.29 
--INSERT--  zsh version: 4.2.0  Return Code: 0 
1015 jean-luc@picard TTY:pts/15
~> history 1 | head -1 
  510  cd x

As everybody can see, the history numbers are totally messed up
after using zcalc :)
I'm not sure, but I think that this is not intended, is it? :)

Ciao,
Thomas

-- 
 Thomas Köhler       Email:       jean-luc@picard.franken.de
     <><             WWW:           http://jeanluc-picard.de
                     IRC:                           tkoehler
                     PGP public key available from Homepage!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: zcalc bug

[Peter Stephenson]

Thomas =?iso-8859-1?Q?K=F6hler?= wrote:
> print -P %h
> 504
> ~> zcalc
> 1> 1+2+3
> 6
> ~> print -P %h
> 1014

Aha, now I understand.  This much is sufficient to show the problem.
(By pruning it I may have made the numbers inconsistent, though
I think not).  Concise bug reports are a Good Thing.

zcalc switches the history to its own file, then at the end switches
back.  Unfortunately the only way of switching back to the main file at
the end, removing the zcalc stuff, is to read it back in again.  This
overwrites the numbers.

The only obvious alternative is to leave zcalc entries in the main
history, so in the previous example scrolling back from the final prompt
would take you through `print -P %h', `1+2+3', `print -P %h'.  This was
the older (and easier) behaviour.  I decided an attempt to separate the
histories would be better.  This could be configurable, but neither
behaviour is perfect.

There is no direct way of manipulating the history, only reading it and
writing or appending it.  There may be some hack I haven't thought of,
but the history number handling is buried deep inside.

Wayne, have you any idea how easy it would be to implement something to
make switching history files easy?  E.g. could it be just a matter of
resetting history numbers at the right point when reading in?

I have always dreamed of having a way of manipulating the history
directly using variables.  However, given all the features of the
history, this isn't easy to do properly.  I don't have remotely enough
time and there are many higher priority problems.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: zcalc bug

[Matthias Kopfermann]

another problem is the temporary file-name . if more than
one user uses zcalc the second user is not able to use zcalc
because:
zcalc:93: can't write history file /tmp/zshhist

        Matthias

Re: zcalc bug

[Peter Stephenson]

Matthias Kopfermann wrote:
> another problem is the temporary file-name . if more than
> one user uses zcalc the second user is not able to use zcalc
> because:
> zcalc:93: can't write history file /tmp/zshhist

That's easy, it was just using a stupid name.

Index: Functions/Misc/zcalc
===================================================================
RCS file: /cvsroot/zsh/zsh/Functions/Misc/zcalc,v
retrieving revision 1.9
diff -u -r1.9 zcalc
--- Functions/Misc/zcalc	18 Apr 2003 13:20:50 -0000	1.9
+++ Functions/Misc/zcalc	13 May 2004 15:56:46 -0000
@@ -88,7 +88,7 @@
 
 # can't be local since required in EXIT trap
 zcalc_orighist=$HISTFILE 
-local temphist=${TMPPREFIX}hist SAVEHIST=$HISTSIZE
+local temphist=${TMPPREFIX}zcalc_hist.$$ SAVEHIST=$HISTSIZE
 HISTFILE=$temphist
 fc -W
 

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: zcalc bug

[Matthias Kopfermann]

one last thing that i am not sure about.
I am quite sure it's not that easy but still a bit
confusing:

with LC_NUMERIC=de_DE  zcalc

i get floating point shown as something like
`44,2'  , because Germany has "," instead of ".".

But i cannot use 3*4,2 to calculate.
A mixture of both seems quite peculiar to me.

Normally the programs that do show a "," in the result let
me use the "," in the input, too.

        Matthias

        PS: It's not that _I_ need to enter values with ","
        in it, but I don't see the use of switching the
        style between input and output.

Re: zcalc bug

[Peter Stephenson]

Matthias Kopfermann wrote:
> one last thing that i am not sure about.
> I am quite sure it's not that easy but still a bit
> confusing:
> 
> with LC_NUMERIC=de_DE  zcalc
> 
> i get floating point shown as something like
> `44,2'  , because Germany has "," instead of ".".
> 
> But i cannot use 3*4,2 to calculate.
> A mixture of both seems quite peculiar to me.

Yes, it's a nuisance: that's because zsh does it's own scanning.

It's not that easy to fix.  Consider:

% print $(( 3,4 ))
4

The standard comma operator is specifically defined for math mode, and
whitespace is not significant.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: zcalc bug

[Matthias Kopfermann]

On Thu, May 13, 2004 at 06:47:00PM +0100, You wrote:
> Matthias Kopfermann wrote:
> 
> It's not that easy to fix.  Consider:
> 
> % print $(( 3,4 ))
> 4
> 

i saw a sweet output when doing

print $((3*4,))

BUG: math: not enough wallabies in outback.

now consider this cold output of bash :):

echo $((3*4,))
bash: 3*4,: syntax error: operand expected (error token is
",")

okay, i am mean :)

        Matthias

Re: zcalc bug

[Peter Stephenson]

Matthias Kopfermann wrote:
> i saw a sweet output when doing
> 
> print $((3*4,))
> 
> BUG: math: not enough wallabies in outback.

Fair dinkum blue (sorry, Geoff)...

The comment above it says
	/* Make sure anyone seeing this message reports it. */
so this worked.

The real bug is that it continues the calculation and outputs 12,
without reporting a user error.

I think this is the fix.  The precedence of comma was the highest
precedence, which incorrectly indicated it didn't need an argument.
It simply(?) needs to be at an intervening precedence, without
disturbing the other ordering.

Index: Src/math.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/math.c,v
retrieving revision 1.19
diff -u -r1.19 math.c
--- Src/math.c	11 Aug 2003 10:45:10 -0000	1.19
+++ Src/math.c	13 May 2004 19:59:42 -0000
@@ -168,8 +168,8 @@
      0,  16, 0
 };
 
-#define TOPPREC 17
-#define ARGPREC (TOPPREC-1)
+#define TOPPREC 18
+#define ARGPREC 16
 
 static int type[TOKCOUNT] =
 {
Index: Test/C01arith.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C01arith.ztst,v
retrieving revision 1.7
diff -u -r1.7 C01arith.ztst
--- Test/C01arith.ztst	17 Dec 2003 20:47:40 -0000	1.7
+++ Test/C01arith.ztst	13 May 2004 19:59:42 -0000
@@ -110,3 +110,23 @@
   print $(( ))
 0:empty math parse e.g. $(( )) acts like a zero
 >0
+
+  print $(( a = ))
+1:empty assignment
+?(eval):1: bad math expression: operand expected at `'
+
+  print $(( 3, ))
+1:empty right hand of comma
+?(eval):1: bad math expression: operand expected at `'
+
+  print $(( 3,,4 ))
+1:empty middle of comma
+?(eval):1: bad math expression: operand expected at `,4 '
+
+  print $(( (3 + 7, 4), 5 ))
+0:commas and parentheses, part 1
+>5
+
+  print $(( 5, (3 + 7, 4) ))
+0:commas and parentheses, part 1
+>4

-- 
Peter Stephenson <pws@pwstephenson.fsnet.co.uk>
Work: pws@csr.com
Web: http://www.pwstephenson.fsnet.co.uk

Re: zcalc bug

[Wayne Davison]

On Thu, May 13, 2004 at 03:57:58PM +0100, Peter Stephenson wrote:
> Wayne, have you any idea how easy it would be to implement something
> to make switching history files easy?

I'd like to see something that doesn't involve saving and restoring the
lines of the current history to a file, but instead just shunts aside
the internal data temporarily.  This would be the least disruptive,
especially since some info is lost when it gets written out and re-read
(I'm thinking of the local/imported distinction when sharing history
between multiple shells as at least one such thing that is lost).

A couple different potential idioms for surfacing this to the user come
to mind:

    pushhist; ...; pophist

    setopt localoptions tmphist

Both would do approximately the same thing internally, but I would
assume that a push/pop idiom would be expected to handle more than one
saved history at a time.  Either one would save and restore the history-
related environment variables (like HISTFILE & HISTSIZE) but (I assume)
not any of the history-related options (those a script could set locally
using the already-existing means).

It doesn't seem like it would be too hard to find all the variables that
need to be tweaked (maybe putting them into a single structure) and then
implementing something that can save and restore them under user control.

What do you think?

..wayne..

Re: zcalc bug

[Peter Stephenson]

Wayne Davison wrote:
>     pushhist; ...; pophist
> 
> It doesn't seem like it would be too hard to find all the variables that
> need to be tweaked (maybe putting them into a single structure) and then
> implementing something that can save and restore them under user control.

This would definitely be nice to have.

Do you think it's easy to keep the state of external files consistent
while you do that?

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: zcalc bug

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On Fri, May 14, 2004 at 10:23:45AM +0100, Peter Stephenson wrote:
> This would definitely be nice to have.

OK, I created an initial implementation of this today (it was pretty
easy, as I expected).  See if you like it.  It works in my limited
testing (i.e. a modified zcalc works quite nicely), but I may have
missed something.

> Do you think it's easy to keep the state of external files consistent
> while you do that?

I'm not sure what you mean by this.  My code isn't doing anything with
files, so the modified zcalc function still loads the .zcalc_history
file with "fc -R" and its restore function writes it out with "fc -W".
However, no fc calls are needed to save and restore the current history.

It would be possible to change my current, very simple pushhist/pophist
syntax to have pushhist optionally take some parameters to setup the new
history environment, for instance:

pushhist 200 100 ~/.zcalc_history

would be just like this shell code:

pushhist
HISTSIZE=200
SAVEHIST=100
HISTFILE=~/.zcalc_history
[[ -f $HISTFILE ]] && fc -R

And the pophist function could likewise be changed to work like this:

[[ ! -z $HISTFILE && $SAVEHIST > 0 ]] && fc -W
pophist

(Although maybe that should be "pophist -W" or something.)  However,
that may be overkill since it is so easy to code up those extra few
shell lines.

Appended is my patch.

..wayne..

[-- Attachment #2: pushpop.patch --]
[-- Type: text/plain, Size: 4419 bytes --]

--- Functions/Misc/zcalc	13 May 2004 17:08:31 -0000	1.10
+++ Functions/Misc/zcalc	14 May 2004 23:54:10 -0000
@@ -86,22 +86,16 @@
 emulate -L zsh
 setopt extendedglob
 
-# can't be local since required in EXIT trap
-zcalc_orighist=$HISTFILE
-local temphist=${TMPPREFIX}zcalc_hist.$$ SAVEHIST=$HISTSIZE
-HISTFILE=$temphist
-fc -W
-
-local HISTSIZE=0
-HISTSIZE=$SAVEHIST
+pushhist
+HISTSIZE=200
+SAVEHIST=100
 HISTFILE=~/.zcalc_history
 [[ -f $HISTFILE ]] && fc -R
 
 zcalc_restore() {
     unfunction zcalc_restore
     fc -W
-    HISTFILE=$zcalc_orighist
-    fc -R
+    pophist
 }
 trap zcalc_restore HUP INT QUIT EXIT
 
--- Src/builtin.c	23 Apr 2004 11:17:15 -0000	1.118
+++ Src/builtin.c	14 May 2004 23:54:11 -0000
@@ -100,9 +100,11 @@ static struct builtin builtins[] =
 #endif
 
     BUILTIN("popd", 0, bin_cd, 0, 1, BIN_POPD, NULL, NULL),
+    BUILTIN("pophist", 0, bin_histstack, 0, 1, BIN_POPHIST, NULL, NULL),
     BUILTIN("print", BINF_PRINTOPTS, bin_print, 0, -1, BIN_PRINT, "abcC:Df:ilmnNoOpPrRsu:z-", NULL),
     BUILTIN("printf", 0, bin_print, 1, -1, BIN_PRINTF, NULL, NULL),
     BUILTIN("pushd", BINF_SKIPINVALID | BINF_SKIPDASH | BINF_DASHDASHVALID, bin_cd, 0, 2, BIN_PUSHD, "sPL", NULL),
+    BUILTIN("pushhist", 0, bin_histstack, 0, 1, BIN_PUSHHIST, NULL, NULL),
     BUILTIN("pushln", BINF_PRINTOPTS, bin_print, 0, -1, BIN_PRINT, NULL, "-nz"),
     BUILTIN("pwd", 0, bin_pwd, 0, 0, 0, "rLP", NULL),
     BUILTIN("r", 0, bin_fc, 0, -1, BIN_R, "nrl", NULL),
@@ -1448,6 +1450,20 @@ bin_fc(char *nam, char **argv, Options o
     return retval;
 }
 
+/**/
+int
+bin_histstack(char *nam, char **argv, Options ops, int func)
+{
+    /* pushhist/pophist is only permitted in interactive shells */
+    if (!interact) {
+	zwarnnam(nam, "not interactive shell", NULL, 0);
+	return 1;
+    }
+    if (func == BIN_PUSHHIST)
+	return pushhiststack();
+    return pophiststack();
+}
+
 /* History handling functions: these are called by ZLE, as well as  *
  * the actual builtins.  fcgetcomm() gets a history line, specified *
  * either by number or leading string.  fcsubs() performs a given   *
--- Src/hashtable.h	11 Sep 2003 07:00:08 -0000	1.4
+++ Src/hashtable.h	14 May 2004 23:54:12 -0000
@@ -59,6 +59,8 @@
 #define BIN_ENABLE   25
 #define BIN_PRINTF   26
 #define BIN_COMMAND  27
+#define BIN_PUSHHIST 28
+#define BIN_POPHIST  29
 
 /* These currently depend on being 0 and 1. */
 #define BIN_SETOPT    0
--- Src/hist.c	11 May 2004 21:45:36 -0000	1.48
+++ Src/hist.c	14 May 2004 23:54:13 -0000
@@ -30,6 +30,21 @@
 #include "zsh.mdh"
 #include "hist.pro"
 
+struct histsave {
+    char *histfile;
+    HashTable histtab;
+    Histent hist_ring;
+    int curhist;
+    int histlinect;
+    int histsiz;
+    int savehistsiz;
+};
+
+static struct histsave histsave_stack[5];
+static int histsave_stack_len = 0;
+#define MAX_HISTSAVE_PUSH (sizeof histsave_stack / sizeof histsave_stack[0])
+
+
 /* Functions to call for getting/ungetting a character and for history
  * word control. */
 
@@ -2329,3 +2344,72 @@ bufferwords(LinkList list, char *buf, in
 
     return list;
 }
+
+/**/
+int
+pushhiststack(void)
+{
+    struct histsave *h;
+    int curline_in_ring = hist_ring == &curline;
+
+    if (histsave_stack_len == MAX_HISTSAVE_PUSH)
+	return 1;
+
+    if (curline_in_ring)
+	unlinkcurline();
+
+    h = &histsave_stack[histsave_stack_len++];
+
+    h->histfile = ztrdup(getsparam("HISTFILE"));
+    h->histtab = histtab;
+    h->hist_ring = hist_ring;
+    h->curhist = curhist;
+    h->histlinect = histlinect;
+    h->histsiz = histsiz;
+    h->savehistsiz = savehistsiz;
+
+    unsetparam("HISTFILE");
+    histtab = NULL;
+    hist_ring = NULL;
+    curhist = 0;
+    histlinect = 0;
+    histsiz = DEFAULT_HISTSIZE;
+    savehistsiz = 0;
+
+    inithist();
+    if (curline_in_ring)
+	linkcurline();
+
+    return 0;
+}
+
+
+/**/
+int
+pophiststack(void)
+{
+    struct histsave *h;
+    int curline_in_ring = hist_ring == &curline;
+
+    if (histsave_stack_len == 0)
+	return 1;
+
+    if (curline_in_ring)
+	unlinkcurline();
+    deletehashtable(histtab);
+
+    h = &histsave_stack[--histsave_stack_len];
+
+    setsparam("HISTFILE", h->histfile);
+    histtab = h->histtab;
+    hist_ring = h->hist_ring;
+    curhist = h->curhist;
+    histlinect = h->histlinect;
+    histsiz = h->histsiz;
+    savehistsiz = h->savehistsiz;
+
+    if (curline_in_ring)
+	linkcurline();
+
+    return 0;
+}

Re: zcalc bug

[Peter Stephenson]

Wayne Davison wrote:
> OK, I created an initial implementation of this today (it was pretty
> easy, as I expected).  See if you like it.  It works in my limited
> testing (i.e. a modified zcalc works quite nicely), but I may have
> missed something.

Thanks.  Just got a chance to try this.  Seems fine, so far.

> > Do you think it's easy to keep the state of external files consistent
> > while you do that?
> 
> I'm not sure what you mean by this.  My code isn't doing anything with
> files, so the modified zcalc function still loads the .zcalc_history
> file with "fc -R" and its restore function writes it out with "fc -W".
> However, no fc calls are needed to save and restore the current history.

Well, it does seem to get the history numbers right when reading the
file back.

> It would be possible to change my current, very simple pushhist/pophist
> syntax to have pushhist optionally take some parameters to setup the new
> history environment, for instance:
> 
> pushhist 200 100 ~/.zcalc_history
> 
> would be just like this shell code:
> 
> pushhist
> HISTSIZE=200
> SAVEHIST=100
> HISTFILE=~/.zcalc_history
> [[ -f $HISTFILE ]] && fc -R
> 
> And the pophist function could likewise be changed to work like this:
> 
> [[ ! -z $HISTFILE && $SAVEHIST > 0 ]] && fc -W
> pophist

You're proposing that you would *only* need the pushhist and pophist
functions to manipulate the history?  That sounds a good thing to me.

Something else that occurs to me is that it would be useful to have the
history automatically popped when you leave the function scope.  That
saves a lot of messing on with traps, which will never be completely
safe.  However, there may be uses where you want to stay in an
alternative history between functions, though I can't think of any off
the top of my head, so I don't know if that's the right thing to do in
general.  It would certainly make zcalc simpler and more reliable.
(There are various mechanisms associated with function scoping, without
looking I'm not sure which would be the best here.)

I'm not sure a separate command is necessary; it could be a new
option to history (or both history and fc).

We could get essentially all the above with:

  history -p 200 100 ~/.zcalc_history

push the history as you suggested

  history -P

pop the history, saving the temporary $HISTFILE first and restoring
the original one

history -Lp 200 100 ~/.zcalc_history

  push the history for the current local scope, restoring it automatically
  when the function exits.

I could be persuaded to the view that the local behaviour is the right
one, so you never need an explicit pop, and the syntax becomes even
simpler.  That significantly reduces the likelihood of people getting
their history screwed up.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

[PATCH] local history support, take 2

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

On Tue, May 18, 2004 at 12:28:46PM +0100, Peter Stephenson wrote:
> Well, it does seem to get the history numbers right when reading the
> file back.

If you mean the original history file, it doesn't read it back -- it
restores the internal memory back, so everything is just as it was
before the push.

> You're proposing that you would *only* need the pushhist and pophist
> functions to manipulate the history?  That sounds a good thing to me.

Yes, I'm liking the idea more and more as I think about it.  I can
imagine myself using some sub-history files to make certain tasks easier
to redo.  For instance, if I'm doing a new release of one of my
projects, I could push to a "release history" file that has those
commands unobscured by other commands and in an easy-to-reuse order;
then pop back to the normal history when I'm done.

> Something else that occurs to me is that it would be useful to have the
> history automatically popped when you leave the function scope.

That would certainly be nice.  It was the reason behind my original
"setopt localoptions tmphist" idea (which wasn't very flexible).  I
haven't tried to do this yet, though.

>   history -p 200 100 ~/.zcalc_history
>   history -P

Seems reasonable to me, so I changed my implementation to work that way.
I reordered my suggested options to make them a little more flexible:

    history -p [[[HISTFILE] HISTSIZE] SAVEHIST]

This allows the user to get a clean history with "history -p" (with the
default, new-shell values for HISTFILE, HISTSIZE, and SAVEHIST).  To
switch to a new file (reading it in) but leaving HISTSIZE and SAVEHIST
alone (like zcalc does) is now "history -p ~/.zcalc_history".  The
final abbreviated option is to just set the desired HISTSIZE value
(after the HISTFILE value) and have it affect both HISTFILE and
SAVEHIST.

> I could be persuaded to the view that the local behaviour is the right
> one, so you never need an explicit pop, and the syntax becomes even
> simpler.

I think it would be nice to both have a pop option and to have that
option automatically get executed on leaving a function scope if the
user didn't already run it.  The reason I'm thinking I want an explicit
option is so I can push/pop the history interactively.

Here's my latest patch.  In addition to the change in syntax described
above, it preserves another internal data item that I noticed needed to
be saved, and it uses the new push/pop functions in place of some old
push/pop code that was needed to rewrite the history file without
affecting the user's history.  I also modified zexit() to be sure that
it pops back to the top history file when it is doing a final saving of
the history data.  Finally, the push stack is reallocated as needed
instead of failing after 5 pushes.

..wayne..

[-- Attachment #2: localhistory.patch --]
[-- Type: text/plain, Size: 6250 bytes --]

--- Functions/Misc/zcalc	13 May 2004 17:08:31 -0000	1.10
+++ Functions/Misc/zcalc	18 May 2004 19:15:46 -0000
@@ -86,22 +86,13 @@
 emulate -L zsh
 setopt extendedglob
 
-# can't be local since required in EXIT trap
-zcalc_orighist=$HISTFILE
-local temphist=${TMPPREFIX}zcalc_hist.$$ SAVEHIST=$HISTSIZE
-HISTFILE=$temphist
-fc -W
-
-local HISTSIZE=0
-HISTSIZE=$SAVEHIST
-HISTFILE=~/.zcalc_history
-[[ -f $HISTFILE ]] && fc -R
+# push to our own history file
+history -p ~/.zcalc_history
 
 zcalc_restore() {
     unfunction zcalc_restore
-    fc -W
-    HISTFILE=$zcalc_orighist
-    fc -R
+    # pop back to original history
+    history -P
 }
 trap zcalc_restore HUP INT QUIT EXIT
 
--- Src/builtin.c	23 Apr 2004 11:17:15 -0000	1.118
+++ Src/builtin.c	18 May 2004 19:15:47 -0000
@@ -82,7 +82,7 @@ static struct builtin builtins[] =
     BUILTIN("hashinfo", 0, bin_hashinfo, 0, 0, 0, NULL, NULL),
 #endif
 
-    BUILTIN("history", 0, bin_fc, 0, -1, BIN_FC, "nrdDfEim", "l"),
+    BUILTIN("history", 0, bin_fc, 0, -1, BIN_FC, "nrdDfEimpP", "l"),
     BUILTIN("integer", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL, bin_typeset, 0, -1, 0, "Hghi:%lprtux", "i"),
     BUILTIN("jobs", 0, bin_fg, 0, -1, BIN_JOBS, "dlpZrs", NULL),
     BUILTIN("kill", 0, bin_kill, 0, -1, 0, NULL, NULL),
@@ -1308,6 +1308,34 @@ bin_fc(char *nam, char **argv, Options o
 	zwarnnam(nam, "not interactive shell", NULL, 0);
 	return 1;
     }
+    if (OPT_ISSET(ops,'p')) {
+	char *hf = "";
+	int hs = DEFAULT_HISTSIZE;
+	int shs = 0;
+	if (*argv) {
+	    hf = *argv++;
+	    if (*argv) {
+		hs = atoi(*argv++);
+		if (*argv)
+		    shs = atoi(*argv++);
+		else
+		    shs = hs;
+	    } else {
+		hs = histsiz;
+		shs = savehistsiz;
+	    }
+	}
+	if (!pushhiststack(hf, hs, shs))
+	    return 1;
+	if (*hf)
+	    readhistfile(hf, 1, HFILE_USE_OPTIONS);
+	return 0;
+    }
+    if (OPT_ISSET(ops,'P')) {
+	if (!nohistsave)
+	    savehistfile(NULL, 1, HFILE_USE_OPTIONS);
+	return !pophiststack();
+    }
     /* with the -m option, the first argument is taken *
      * as a pattern that history lines have to match   */
     if (*argv && OPT_ISSET(ops,'m')) {
@@ -4073,8 +4101,11 @@ zexit(int val, int from_where)
 	killrunjobs(from_where == 1);
     }
     if (isset(RCS) && interact) {
-	if (!nohistsave)
+	if (!nohistsave) {
 	    savehistfile(NULL, 1, HFILE_USE_OPTIONS);
+	    while (pophiststack())
+		savehistfile(NULL, 1, HFILE_USE_OPTIONS);
+	}
 	if (islogin && !subsh) {
 	    sourcehome(".zlogout");
 #ifdef GLOBAL_ZLOGOUT
--- Src/hist.c	18 May 2004 18:45:05 -0000	1.49
+++ Src/hist.c	18 May 2004 19:15:49 -0000
@@ -1791,13 +1791,28 @@ resizehistents(void)
 }
 
 /* Remember the last line in the history file so we can find it again. */
-static struct {
+struct histfile_stats {
     char *text;
     time_t stim, mtim;
     off_t fpos, fsiz;
     int next_write_ev;
 } lasthist;
 
+struct histsave {
+    struct histfile_stats lasthist;
+    char *histfile;
+    HashTable histtab;
+    Histent hist_ring;
+    int curhist;
+    int histlinect;
+    int histsiz;
+    int savehistsiz;
+};
+
+static struct histsave *histsave_stack;
+static int histsave_stack_size = 0;
+static int histsave_stack_pos = 0;
+
 static int histfile_linect;
 
 static int
@@ -2078,32 +2093,14 @@ savehistfile(char *fn, int err, int writ
 	fclose(out);
 
 	if ((writeflags & (HFILE_SKIPOLD | HFILE_FAST)) == HFILE_SKIPOLD) {
-	    HashTable remember_histtab = histtab;
-	    Histent remember_hist_ring = hist_ring;
-	    int remember_histlinect = histlinect;
-	    int remember_curhist = curhist;
-	    int remember_histsiz = histsiz;
-	    int remember_histactive = histactive;
-
-	    hist_ring = NULL;
-	    curhist = histlinect = 0;
-	    histsiz = savehistsiz;
-	    histactive = 0;
-	    createhisttable(); /* sets histtab */
-
+	    /* The NULL leaves HISTFILE alone, preserving fn's value. */
+	    pushhiststack(NULL, savehistsiz, savehistsiz);
 	    hist_ignore_all_dups |= isset(HISTSAVENODUPS);
 	    readhistfile(fn, err, 0);
 	    hist_ignore_all_dups = isset(HISTIGNOREALLDUPS);
 	    if (histlinect)
 		savehistfile(fn, err, 0);
-	    deletehashtable(histtab);
-
-	    curhist = remember_curhist;
-	    histlinect = remember_histlinect;
-	    hist_ring = remember_hist_ring;
-	    histtab = remember_histtab;
-	    histsiz = remember_histsiz;
-	    histactive = remember_histactive;
+	    pophiststack();
 	}
     } else if (err)
 	zerr("can't write history file %s", fn, 0);
@@ -2331,3 +2328,84 @@ bufferwords(LinkList list, char *buf, in
 
     return list;
 }
+
+/**/
+int
+pushhiststack(char *hf, int hs, int shs)
+{
+    struct histsave *h;
+    int curline_in_ring = hist_ring == &curline;
+
+    if (histsave_stack_pos == histsave_stack_size) {
+	histsave_stack_size += 5;
+	histsave_stack = zrealloc(histsave_stack,
+			    histsave_stack_size * sizeof (struct histsave));
+    }
+
+    if (curline_in_ring)
+	unlinkcurline();
+
+    h = &histsave_stack[histsave_stack_pos++];
+
+    h->lasthist = lasthist;
+    h->histfile = hf ? ztrdup(getsparam("HISTFILE")) : NULL;
+    h->histtab = histtab;
+    h->hist_ring = hist_ring;
+    h->curhist = curhist;
+    h->histlinect = histlinect;
+    h->histsiz = histsiz;
+    h->savehistsiz = savehistsiz;
+
+    memset(&lasthist, 0, sizeof lasthist);
+    if (hf) {
+	if (*hf)
+	    setsparam("HISTFILE", ztrdup(hf));
+	else
+	    unsetparam("HISTFILE");
+    }
+    hist_ring = NULL;
+    curhist = histlinect = 0;
+    histsiz = hs;
+    savehistsiz = shs;
+    inithist(); /* sets histtab */
+
+    if (curline_in_ring)
+	linkcurline();
+
+    return 1;
+}
+
+
+/**/
+int
+pophiststack(void)
+{
+    struct histsave *h;
+    int curline_in_ring = hist_ring == &curline;
+
+    if (histsave_stack_pos == 0)
+	return 0;
+
+    if (curline_in_ring)
+	unlinkcurline();
+
+    deletehashtable(histtab);
+    zsfree(lasthist.text);
+
+    h = &histsave_stack[--histsave_stack_pos];
+
+    lasthist = h->lasthist;
+    if (h->histfile)
+	setsparam("HISTFILE", h->histfile);
+    histtab = h->histtab;
+    hist_ring = h->hist_ring;
+    curhist = h->curhist;
+    histlinect = h->histlinect;
+    histsiz = h->histsiz;
+    savehistsiz = h->savehistsiz;
+
+    if (curline_in_ring)
+	linkcurline();
+
+    return 1;
+}

Re: [PATCH] local history support, take 2

[Wayne Davison]

On Tue, May 18, 2004 at 12:50:47PM -0700, Wayne Davison wrote:
> Here's my latest patch.

Note that this patch has a problem if the original history did not have
HISTFILE set -- any new value set in the pushed history is not removed
on pop.  I've got a fix for that, but it's a pretty minor glitch, so I
won't repost the whole thing.  If you want to see my latest patch, you
can grab it here:

    http://www.blorf.net/localhistory.patch

..wayne..

Re: [PATCH] local history support, take 2

[Peter Stephenson]

Wayne Davison wrote:
> > Something else that occurs to me is that it would be useful to have the
> > history automatically popped when you leave the function scope.
> 
> That would certainly be nice.  It was the reason behind my original
> "setopt localoptions tmphist" idea (which wasn't very flexible).  I
> haven't tried to do this yet, though.

I might get a moment to look at this bit if you don't.

> I think it would be nice to both have a pop option and to have that
> option automatically get executed on leaving a function scope if the
> user didn't already run it.  The reason I'm thinking I want an explicit
> option is so I can push/pop the history interactively.

Fine, if you've got a use for it.  I had a vague feeling people using it
from the command line would expect something a bit more flexible, like
pushd/popd, which was beyond what I was thinking about.

> I reordered my suggested options to make them a little more flexible:
> 
>     history -p [[[HISTFILE] HISTSIZE] SAVEHIST]

I suspect you mean

    history -p [HISTFILE [HISTSIZE [SAVEHIST]]]

or it's a little bit tricky to work out.

> Here's my latest patch.

Looks fine.  Some comments on the details:

I get an error if the HISTFILE on the command line doesn't exist.
Presumably it should be silently created.  The file was created anyway,
but the error in the test function I used left me in the empty new
history; that problem will go away with the local proposal.

Probably there should be an error if there are too many arguments to
`history -p' or `history -P'.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: [PATCH] local history support, take 2

[Wayne Davison]

On Wed, May 19, 2004 at 10:40:25AM +0100, Peter Stephenson wrote:
> I might get a moment to look at this bit if you don't.

Feel free.  I had one idea on how to implement this:

The pushhiststack() function how returns the value of histsave_stack_pos
(i.e. the 1-relative spot where we stored the item).  This could be put
into a local variable named HISTPOP when `fc -p` was called for the
first time in a function.  Then, when HISTPOP got deleted, zsh would
call a new routine that would pop the list down through that value
(saving each history file on the way).

There may well be better ways to implement this than that, though (i.e.
not using the environment).

> I suspect you mean
> 
>     history -p [HISTFILE [HISTSIZE [SAVEHIST]]]

Yes, I certainly did.

When I went to document the new options, I noted that the entire
description of the "history" command is that it is the same as `fc -l'.
Because of this, I decided to switch the -p/-P options over to fc.

> I get an error if the HISTFILE on the command line doesn't exist.

Thanks -- fixed.

> Probably there should be an error if there are too many arguments

Yes, I had planned to do that, and it is now done.

You can fetch the latest patch from here:

    http://www.blorf.net/localhistory.patch

Shall I go ahead and check this in?  Or do we want to consider this a
bit more?  For instance, if you want to suggest a more pushd/popd style
command set for interactive use, let me know.

..wayne..

Re: [PATCH] local history support, take 2

[Peter Stephenson]

Wayne Davison wrote:
> Because of this, I decided to switch the -p/-P options over to fc.

Probably allowing it with both would be better, since `history' is a more
natural command.

> Shall I go ahead and check this in?  Or do we want to consider this a
> bit more?

Looks essentially OK to me as it is, it can be enhanced later if we feel
like it.

I think we'll need to be on the watch for conditions caused by errors
--- e.g. an error in popping due to problems with the temporary history
failing to restore the permanent history even though that's OK.  That
just needs some ad hoc testing.

pws

Re: [PATCH] local history support, take 2

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Wed, May 19, 2004 at 10:37:50PM +0100, Peter Stephenson wrote:
> Probably allowing it with both would be better, since `history' is a more
> natural command.

I agree that the command name reads better, but since it has always
implied listing of history entries, it seems a little weird to change
that.  I went ahead and added the -p & -P options to the history
builtin, but I didn't document it yet.

> Looks essentially OK to me as it is, it can be enhanced later if we
> feel like it.

OK, I checked it in.  I also started to work on an auto-pop feature
from a function.  Attached is a patch that almost works right, but has a
couple areas that need to be improved.  Perhaps someone can chime in
here with some advice on how to either do this the right way or fix the
problems.

I chose to add my pop-checking hook into level() -- the routine that
decrements locallevel.  One problem I encountered is that this routine
is called on function exit _before_ the traps are run.  This means that
if someone puts a manual "fc -P" call into a trap, it will try to do
an extra pop after the auto-pop for leaving the function.  I suppose we
can just tell people not to do that...

The other problem I noticed is how the saving of the parameters
interacts with local variables.  Take these two functions for instance:

function foo {
    local HISTSIZE=2221
    fc -p ~/.newhist 2222 2222
    echo $HISTFILE $HISTSIZE
}

function bar {
    fc -p
    local HISTFILE=~/.newhist HISTSIZE=2223
    echo $HISTFILE $HISTSIZE
}

In function foo the history-list push saves off the 2221 from the local
variable, so we have to be sure to call the history-list pop function
before removing the local variables (which is what I currently do).

However, in function bar the history-list push saves off the global
value of the HISTFILE and HISTSIZE variables, which are then made local.
When the function exits, the history-list pop first restores the global
values of the variables to the environment, and then the local-variable
restoration unsets HISTFILE and makes HISTSIZE=30 (the default value).

Is this something we can easily fix?  Perhaps with a different way of
saving the environment values?  Or is this something where we tell users
that they can't both use "fc -p" and make the variables local?

..wayne..

[-- Attachment #2: autopop.patch --]
[-- Type: text/plain, Size: 2530 bytes --]

--- Functions/Misc/zcalc	20 May 2004 22:23:11 -0000	1.11
+++ Functions/Misc/zcalc	21 May 2004 01:22:44 -0000
@@ -89,13 +89,6 @@ setopt extendedglob
 # push to our own history file
 fc -p ~/.zcalc_history
 
-zcalc_restore() {
-    unfunction zcalc_restore
-    # pop back to original history
-    fc -P
-}
-trap zcalc_restore HUP INT QUIT EXIT
-
 local line ans base defbase forms match mbegin mend psvar optlist opt arg
 local compcontext="-math-"
 integer num outdigits outform=1
--- Src/builtin.c	20 May 2004 22:22:43 -0000	1.119
+++ Src/builtin.c	21 May 2004 01:22:45 -0000
@@ -4110,7 +4110,7 @@ zexit(int val, int from_where)
     }
     if (isset(RCS) && interact) {
 	if (!nohistsave) {
-	    saveandpophiststack(0);
+	    saveandpophiststack(1);
 	    savehistfile(NULL, 1, HFILE_USE_OPTIONS);
 	}
 	if (islogin && !subsh) {
--- Src/hist.c	20 May 2004 22:23:02 -0000	1.50
+++ Src/hist.c	21 May 2004 01:36:08 -0000
@@ -1807,6 +1807,7 @@ static struct histsave {
     int histlinect;
     int histsiz;
     int savehistsiz;
+    int locallevel;
 } *histsave_stack;
 static int histsave_stack_size = 0;
 static int histsave_stack_pos = 0;
@@ -2374,6 +2375,7 @@ pushhiststack(char *hf, int hs, int shs)
     h->histlinect = histlinect;
     h->histsiz = histsiz;
     h->savehistsiz = savehistsiz;
+    h->locallevel = locallevel;
 
     memset(&lasthist, 0, sizeof lasthist);
     if (hf) {
@@ -2433,14 +2435,26 @@ pophiststack(void)
     return histsave_stack_pos + 1;
 }
 
+/* If down_through > 0, pop all array items >= the 1-relative index value.
+ * If down_through < 0, pop (-1)*down_through levels off the stack.
+ * If down_through == 0, pop any stack items from a higher locallevel.
+ */
+
 /**/
 int
 saveandpophiststack(int down_through)
 {
-    if (down_through < 0)
+    if (!down_through) {
+	down_through = histsave_stack_pos;
+	while (down_through > 0
+	 && histsave_stack[down_through-1].locallevel > locallevel)
+	    down_through--;
+	down_through++;
+    } else if (down_through < 0) {
 	down_through += histsave_stack_pos + 1;
-    if (down_through <= 0)
-	down_through = 1;
+	if (down_through <= 0)
+	    down_through = 1;
+    }
     if (histsave_stack_pos < down_through)
 	return 0;
     do {
--- Src/params.c	6 Apr 2004 13:01:10 -0000	1.81
+++ Src/params.c	21 May 2004 01:22:49 -0000
@@ -3583,6 +3583,7 @@ mod_export void
 endparamscope(void)
 {
     locallevel--;
+    saveandpophiststack(0); /* Pops anything from a higher locallevel */
     scanhashtable(paramtab, 0, 0, 0, scanendscope, 0);
 }
 

Re: [PATCH] local history support, take 2

[Wayne Davison]

On Thu, May 20, 2004 at 06:37:38PM -0700, Wayne Davison wrote:
> I chose to add my pop-checking hook into level() -- the routine that

Whoops, I had meant to look up that function name and edit it into my
message, but I sent it off without doing that.  The place I hooked into
was endparamscope(), as the patch indicated.

..wayne..

Re: [PATCH] local history support, take 2

[Peter Stephenson]

Wayne Davison wrote:
> OK, I checked it in.  I also started to work on an auto-pop feature
> from a function.  Attached is a patch that almost works right, but has a
> couple areas that need to be improved.  Perhaps someone can chime in
> here with some advice on how to either do this the right way or fix the
> problems.

Is this going to be optional eventually?  It's a little confusing having
`fc -P' as well, otherwise.  You can use them as a pair at the command
line but I think it should be deliberate to select one behaviour or the
other.

You can also reject attempts to use `fc -P' if the history is marked for
automatic restoration which gets around the other problem.

> function bar {
>     fc -p
>     local HISTFILE=~/.newhist HISTSIZE=2223
>     echo $HISTFILE $HISTSIZE
> }
> 
> However, in function bar the history-list push saves off the global
> value of the HISTFILE and HISTSIZE variables, which are then made local.
> When the function exits, the history-list pop first restores the global
> values of the variables to the environment, and then the local-variable
> restoration unsets HISTFILE and makes HISTSIZE=30 (the default value).
> 
> Is this something we can easily fix?  Perhaps with a different way of
> saving the environment values?  Or is this something where we tell users
> that they can't both use "fc -p" and make the variables local?

I think it needs to be documented that if you're using commands you
shouldn't touch the variables by hand, otherwise confusion like this is
inevitable.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: [PATCH] local history support, take 2

[Wayne Davison]

On Fri, May 21, 2004 at 10:15:41AM +0100, Peter Stephenson wrote:
> Is this going to be optional eventually?

Yeah, I think that it should be optional just to be more flexible -- I
can imagine someone wanting to call a push in a function without the
auto-pop being forced upon them.  I've added your earlier suggestion of
the option "-a" to indicate that an automatic pop is desired.

> You can also reject attempts to use `fc -P' if the history is marked
> for automatic restoration which gets around the other problem.

Actually, manually popping an automatic history doesn't cause any
problems in the current code (the auto-pop of the history list happens
prior to the restoration of local variables, so the early pop causes no
problems that aren't present for the auto-pop).

> I think it needs to be documented that if you're using commands you
> shouldn't touch the variables by hand, otherwise confusion like this
> is inevitable.

It's fine if the user tweaks the values after a history push because
they're just manipulating the current history buffer.  The only problem
comes in if they declare a variable to be local in a way that conflicts
with the restoration order.  I'm advising users to either omit marking
the variables as local, or to declare them local at the top of the
function and use the automatic-pop form of fc.  (The other sequence that
works fine is calling a non-automatic `fc -p' prior to localization, and
then calling `fc -P' from outside the function, but I don't think we
need to tell the user that.)

I've checked in my updated version.  We can continue to improve it, as
desired, but I think it works quite well now.

..wayne..