revisiting history-file rewriting

revisiting history-file rewriting

[Wayne Davison]

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

On Wed, Mar 16, 2005 at 11:13:49AM +0000, Peter Stephenson wrote:
> some time around (western-style) Easter we should probably produce
> 4.2.5.

This makes me want to get approval on a better version of my history-
rewriting patch and get it into 4.2.5.  If some folks would take a look
at this and let me know: (1) do you like the patch for 4.3.0? and (2) do
you like the idea of putting it into 4.2.5 as well.

This patch adds the option HIST_OVERWRITE that is off by default.  This
changes the default history-rewriting strategy to one that uses the name
$HISTFILE.new for writing out the history lines, and then renaming it
over $HISTFILE when it is done (both actions happen with the history
file locked).  If the user chooses to set the HIST_OVERWRITE option, zsh
goes back to the current algorithm of directly re-writing the history
lines to the history file (also while locked).

The reason I chose to change the default is that it is safer to not
truncate the history file when rewriting it.

..wayne..

[-- Attachment #2: hist-dot-new.patch --]
[-- Type: text/plain, Size: 3316 bytes --]

--- Doc/Zsh/options.yo	12 Jan 2005 13:42:13 -0000	1.36
+++ Doc/Zsh/options.yo	16 Mar 2005 20:34:27 -0000
@@ -557,6 +557,14 @@ Note that the command lingers in the int
 command is entered before it vanishes, allowing you to briefly reuse
 or edit the line.
 )
+pindex(HIST_OVERWRITE)
+item(tt(HIST_OVERWRITE))(
+When the history file is rewritten, we normally write out a new file
+and rename it over the old one.  If this option is set, we instead
+overwrite the old history file directly.  Use this only if you have
+special needs, as it is possible to lose history entries if zsh gets
+interrupted during the history-file writing with this option set.
+)
 pindex(HIST_REDUCE_BLANKS)
 item(tt(HIST_REDUCE_BLANKS))(
 Remove superfluous blanks from each command line
--- Src/hist.c	22 Jan 2005 04:03:15 -0000	1.58
+++ Src/hist.c	16 Mar 2005 20:34:27 -0000
@@ -2004,7 +2004,7 @@ readhistfile(char *fn, int err, int read
 void
 savehistfile(char *fn, int err, int writeflags)
 {
-    char *t, *start = NULL;
+    char *t, *tmpfile, *start = NULL;
     FILE *out;
     Histent he;
     zlong xcurhist = curhist - !!(histactive & HA_ACTIVE);
@@ -2041,12 +2041,17 @@ savehistfile(char *fn, int err, int writ
 	    extended_history = 1;
     }
     if (writeflags & HFILE_APPEND) {
+	tmpfile = NULL;
 	out = fdopen(open(unmeta(fn),
 			O_CREAT | O_WRONLY | O_APPEND | O_NOCTTY, 0600), "a");
-    }
-    else {
+    } else if (isset(HISTOVERWRITE)) {
+	tmpfile = NULL;
 	out = fdopen(open(unmeta(fn),
 			 O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600), "w");
+    } else {
+	tmpfile = bicat(unmeta(fn), ".new");
+	unlink(tmpfile);
+	out = fdopen(open(tmpfile, O_WRONLY | O_CREAT | O_EXCL, 0600), "w");
     }
     if (out) {
 	for (; he && he->histnum <= xcurhist; he = down_histent(he)) {
@@ -2091,6 +2096,11 @@ savehistfile(char *fn, int err, int writ
 	    lasthist.text = ztrdup(start);
 	}
 	fclose(out);
+	if (tmpfile) {
+	    if (rename(tmpfile, unmeta(fn)) < 0)
+		zerr("can't rename %s.new to $HISTFILE", fn, 0);
+	    free(tmpfile);
+	}
 
 	if (writeflags & HFILE_SKIPOLD
 	 && !(writeflags & (HFILE_FAST | HFILE_NO_REWRITE))) {
@@ -2110,8 +2120,13 @@ savehistfile(char *fn, int err, int writ
 	    pophiststack();
 	    histactive = remember_histactive;
 	}
-    } else if (err)
-	zerr("can't write history file %s", fn, 0);
+    } else if (err) {
+	if (tmpfile) {
+	    zerr("can't write history file %s.new", fn, 0);
+	    free(tmpfile);
+	} else
+	    zerr("can't write history file %s", fn, 0);
+    }
 
     unlockhistfile(fn);
 }
--- Src/options.c	3 Sep 2004 09:47:49 -0000	1.21
+++ Src/options.c	16 Mar 2005 20:34:27 -0000
@@ -137,6 +137,7 @@ static struct optname optns[] = {
 {NULL, "histignorespace",     0,			 HISTIGNORESPACE},
 {NULL, "histnofunctions",     0,			 HISTNOFUNCTIONS},
 {NULL, "histnostore",	      0,			 HISTNOSTORE},
+{NULL, "histoverwrite",       0,			 HISTOVERWRITE},
 {NULL, "histreduceblanks",    0,			 HISTREDUCEBLANKS},
 {NULL, "histsavenodups",      0,			 HISTSAVENODUPS},
 {NULL, "histverify",	      0,			 HISTVERIFY},
--- Src/zsh.h	25 Feb 2005 10:21:02 -0000	1.70
+++ Src/zsh.h	16 Mar 2005 20:34:28 -0000
@@ -1513,6 +1513,7 @@ enum {
     HISTIGNORESPACE,
     HISTNOFUNCTIONS,
     HISTNOSTORE,
+    HISTOVERWRITE,
     HISTREDUCEBLANKS,
     HISTSAVENODUPS,
     HISTVERIFY,

Re: revisiting history-file rewriting

[Geoff Wing]

Wayne Davison <wayned@users.sourceforge.net> typed:
: This patch adds the option HIST_OVERWRITE that is off by default.  This
: changes the default history-rewriting strategy to one that uses the name
: $HISTFILE.new for writing out the history lines, and then renaming it

Didn't we used to do $HISTFILE.<pid> ?  Is that out of favour?

Regards,
Geoff

Re: revisiting history-file rewriting

[Wayne Davison]

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

On Thu, Mar 17, 2005 at 02:09:43AM +0000, Geoff Wing wrote:
> Didn't we used to do $HISTFILE.<pid> ?  Is that out of favour?

We used to use that idiom as a stepping stone to creating a
$HISTFILE.LOCK file, but now days we use the gettempfile() function
instead (since this avoids a potential problem with the same pid being
allocated on different hosts).  The name used once we lock down the
system can be constant without clashing with another process, but I
suppose it could clash with a file that the user has created.  The
attached patch can be applied to my previous patch to make the history
code use gettempfile() to return an opened $HISTFILE.XXXXXX name for
the new file instead of using $HISTFILE.new.

..wayne..

[-- Attachment #2: hist-tempname.patch --]
[-- Type: text/plain, Size: 1404 bytes --]

--- Src/hist.c.new	2005-03-16 18:26:16.638813544 -0800
+++ Src/hist.c	2005-03-16 18:40:32.629680617 -0800
@@ -2049,9 +2049,14 @@ savehistfile(char *fn, int err, int writ
 	out = fdopen(open(unmeta(fn),
 			 O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600), "w");
     } else {
-	tmpfile = bicat(unmeta(fn), ".new");
-	unlink(tmpfile);
-	out = fdopen(open(tmpfile, O_WRONLY | O_CREAT | O_EXCL, 0600), "w");
+	int fd;
+	if ((fd = gettempfile(fn, 0, &tmpfile)) >= 0) {
+	    if (!(out = fdopen(fd, "w")))
+		close(fd);
+	} else {
+	    tmpfile = bicat(unmeta(fn), ".XXXXXX");
+	    out = NULL;
+	}
     }
     if (out) {
 	for (; he && he->histnum <= xcurhist; he = down_histent(he)) {
@@ -2097,8 +2102,10 @@ savehistfile(char *fn, int err, int writ
 	}
 	fclose(out);
 	if (tmpfile) {
-	    if (rename(tmpfile, unmeta(fn)) < 0)
-		zerr("can't rename %s.new to $HISTFILE", fn, 0);
+	    if (rename(tmpfile, unmeta(fn)) < 0) {
+		tmpfile = metafy(tmpfile, -1, META_REALLOC);
+		zerr("can't rename %s to $HISTFILE", tmpfile, 0);
+	    }
 	    free(tmpfile);
 	}
 
@@ -2122,7 +2129,8 @@ savehistfile(char *fn, int err, int writ
 	}
     } else if (err) {
 	if (tmpfile) {
-	    zerr("can't write history file %s.new", fn, 0);
+	    tmpfile = metafy(tmpfile, -1, META_REALLOC);
+	    zerr("can't write history file %s", tmpfile, 0);
 	    free(tmpfile);
 	} else
 	    zerr("can't write history file %s", fn, 0);

Re: revisiting history-file rewriting

[Bart Schaefer]

On Mar 16, 12:40pm, Wayne Davison wrote:
}
} This patch adds the option HIST_OVERWRITE that is off by default.

I'm OK with this, though I think the documentation should explain in
more detail how it interacts with APPEND_HISTORY, INC_APPEND_HISTORY,
and SHARE_HISTORY.

Also, I know the "new" convention is to use a HIST_ prefix, but it
would be nice to be consistent with APPEND_HISTORY et al.  We already
have HIST_APPEND; while we're at it, perhaps we add HIST_APPEND_INC,
HIST_SHARE, and OVERWRITE_HISTORY with appropriate aliasing?

Re: revisiting history-file rewriting

[Peter Stephenson]

Wayne Davison wrote:
> This patch adds the option HIST_OVERWRITE that is off by default.  This
> changes the default history-rewriting strategy to one that uses the name
> $HISTFILE.new for writing out the history lines, and then renaming it
> over $HISTFILE when it is done (both actions happen with the history
> file locked).

How would this change the result when multiple shells exit at once?
I'm not entirely clear how the locking affects the fact that multiple
shells want to rewrite the file in this case.

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


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

**********************************************************************

Re: revisiting history-file rewriting

[Andrey Borzenkov]

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

On Thursday 17 March 2005 05:49, Wayne Davison wrote:
> On Thu, Mar 17, 2005 at 02:09:43AM +0000, Geoff Wing wrote:
> > Didn't we used to do $HISTFILE.<pid> ?  Is that out of favour?
>
> We used to use that idiom as a stepping stone to creating a
> $HISTFILE.LOCK file, but now days we use the gettempfile() function
> instead (since this avoids a potential problem with the same pid being
> allocated on different hosts). 


If you mention it.

I have a bunch of

...
-rw-------   1 bor  bor        0 Feb 27 22:06 .zsh_history.G4hj3H
-rw-------   1 bor  bor       27 Feb 23 14:46 .zsh_history.H4YHV6
-rw-------   1 bor  bor       28 Mar 13 10:53 .zsh_history.H6UJsW
...

with contents like 

9354 localhost.localdomain

(yes, it is my system name; I installed it three years ago to give a short try 
and it stuck).

how can I get rid of them?

-adrey

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

Re: revisiting history-file rewriting

[Wayne Davison]

On Fri, Mar 18, 2005 at 07:18:25PM +0300, Andrey Borzenkov wrote:
> I have a bunch of [.zsh_history.XXXXXX files]
> with contents like "9354 localhost.localdomain"

You must manually remove them.  I'm curious why you're getting a regular
occurrence of them, though.  Do you not use any of the history-appending
options (e.g. APPEND_HISTORY or SHARE_HISTORY)?  These temp files should
only remain if zsh gets killed while it is waiting for the *.LOCK file,
and a zsh that is appending history will not try to re-write the history
file when it has received a signal telling it to die, so that usually
avoids these droppings.  If no history-appending option is specified,
then having multiple zshs running when the system goes down does tend to
cause these files to be left around.

The locking algorithm has an alternate code section that avoids the
creation of a temporary file, but I think that this alternative might
not be good for NFS (since it relies on O_EXCL).  Perhaps we should
make this locking choice an option instead of conditionally compiled?

This discussion reminded me of why I originally chose to use the name
$HISTFILE.new instead of a unique name for rewriting the history file:
it avoids extra copies of the history file lying around when zsh gets
killed during the rewriting (and since this should only happen if the
user does not have a history-appending option enabled, it doesn't cause
the loss of any extra information that wouldn't have already been lost
by the multiple zsh processes all overwriting each others history
file).

..wayne..

Re: revisiting history-file rewriting

[Wayne Davison]

On Fri, Mar 18, 2005 at 10:49:08AM +0000, Peter Stephenson wrote:
> How would this change the result when multiple shells exit at once?
> I'm not entirely clear how the locking affects the fact that multiple
> shells want to rewrite the file in this case.

It doesn't change that behavior except to make it safer -- if zsh is
forcefully killed it might be in the middle of writing out a new copy of
the history file instead of a truncated version of the real history
file.

Note also that for anyone using one of the history-appending options
that I've already eliminated the multi-shell locking contention at
system shutdown time because zsh no longer tries to rewrite the history
file on a kill signal (and doesn't need to touch the history file at all
if it has no history data left to append).

..wayne..

Re: revisiting history-file rewriting

[Wayne Davison]

On Fri, Mar 18, 2005 at 09:51:45AM -0800, Wayne Davison wrote:
> Do you not use any of the history-appending options (e.g.
> APPEND_HISTORY or SHARE_HISTORY)?

Oops, I just realized that it is INC_APPEND_HISTORY that causes no
contention at shell exit, not APPEND_HISTORY (SHARE_HISTORY was right).

..wayne..

Re: revisiting history-file rewriting

[Wayne Davison]

On Thu, Mar 17, 2005 at 06:03:38PM +0000, Bart Schaefer wrote:
> I'm OK with this, though I think the documentation should explain in
> more detail how it interacts with APPEND_HISTORY, INC_APPEND_HISTORY,
> and SHARE_HISTORY.

Yeah, my docs were pretty minimal.

> We already have HIST_APPEND; while we're at it, perhaps we add
> HIST_APPEND_INC, HIST_SHARE, and OVERWRITE_HISTORY with appropriate
> aliasing?

I don't like OVERWRITE_HISTORY because it sounds like something that
conflicts with APPEND_HISTORY.  Perhaps my option name needs to be
improved -- how about HIST_WRITE_IN_PLACE?  That seems to better
indicate that it is effecting how the history file is written out
rather than affecting how data is put into the history file.

As for the other aliases, I wouldn't mind seeing them added.

..wayne..

Re: revisiting history-file rewriting

[Peter Stephenson]

Wayne Davison wrote:
> On Fri, Mar 18, 2005 at 10:49:08AM +0000, Peter Stephenson wrote:
> > How would this change the result when multiple shells exit at once?
> > I'm not entirely clear how the locking affects the fact that multiple
> > shells want to rewrite the file in this case.
> 
> It doesn't change that behavior except to make it safer -- if zsh is
> forcefully killed it might be in the middle of writing out a new copy of
> the history file instead of a truncated version of the real history
> file.
> 
> Note also that for anyone using one of the history-appending options
> that I've already eliminated the multi-shell locking contention at
> system shutdown time because zsh no longer tries to rewrite the history
> file on a kill signal (and doesn't need to touch the history file at all
> if it has no history data left to append).

This sounds fine.

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


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

**********************************************************************

Re: revisiting history-file rewriting

[Bart Schaefer]

On Mar 18, 10:19am, Wayne Davison wrote:
} Subject: Re: revisiting history-file rewriting
}
} I don't like OVERWRITE_HISTORY because it sounds like something that
} conflicts with APPEND_HISTORY.  Perhaps my option name needs to be
} improved -- how about HIST_WRITE_IN_PLACE?

That name seems a bit unwieldy, but I suppose one won't be typing it all
that often.

Appending to the history writes in place, too, though, doesn't it?  Or
at least incremental appending does?

Maybe HIST_SAVE_BY_COPY, defaulting to on?

Another random thought, far too late for its own good:

HIST_SAVE_NO_DUPS default off should have been HIST_SAVE_DUPS default on,
so as to avoid the double negative in NO_HIST_SAVE_NO_DUPS.  Oh, well.

Re: revisiting history-file rewriting

[Wayne Davison]

On Fri, Mar 18, 2005 at 09:15:37PM +0000, Bart Schaefer wrote:
> Appending to the history writes in place, too, though, doesn't it?  Or
> at least incremental appending does?

This option pertains to when the file is rewritten, which some modes
only do accasionally (the incremental appending modes) and others do for
their entire update strategy.

> Maybe HIST_SAVE_BY_COPY, defaulting to on?

Hmm.  That's pretty good.  The docs could clarify that the "SAVE" part
just refers to when the file is rewritten, and does not affect
incremental appending.

..wayne..

Re: revisiting history-file rewriting

[Wayne Davison]

On Wed, Mar 16, 2005 at 12:40:59PM -0800, Wayne Davison wrote:
> This makes me want to get approval on a better version of my history-
> rewriting patch and get it into 4.2.5.

OK, I've incorporated the feedback and committed a slightly modified
version of the original patch that changed the option to be
HIST_SAVE_BY_COPY (enabled by default) and documented it a little
better.  I did not change the use of $HISTFILE.new for the copy's
filename because I think that this will help to avoid extra directory
droppings (of history temp-files) from accumulating.  If folks disagree
with that decision, we can always check in a patch similar to the one I
posted earlier.

So, do we want this in 4.2.5?  I'm not sure if the folks that responded
were approving of this part of the check-in or not.

..wayne..

Re: revisiting history-file rewriting

[Bart Schaefer]

On Mar 18,  2:53pm, Wayne Davison wrote:
}
} So, do we want this in 4.2.5?

I'd say "yes, provided that we're reasonably sure the implementation isn't
introducing any new bugs."

Re: revisiting history-file rewriting

[Peter Stephenson]

Bart Schaefer wrote:
> On Mar 18,  2:53pm, Wayne Davison wrote:
> }
> } So, do we want this in 4.2.5?
> 
> I'd say "yes, provided that we're reasonably sure the implementation isn't
> introducing any new bugs."

I'm not fussy, but if it seems to be working it can be.  Otherwise it
can wait till 4.2.6.

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


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

**********************************************************************