On Sun, Oct 17, 2004 at 05:21:18PM -0700, Bart Schaefer wrote:
> - It requires a writable directory rather than merely a writable histfile.

We already require this due to our never updating the histfile without
first creating the file $HISTFILE.LOCK.

>   Similarly, perhaps detect when the histfile is a symlink and fall back?

That's certainly possible, but I'm thinking that it's not something we
need to be concerned about.  However, I could be wrong.

> - You haven't tested for failure of unlink() [though I'm not sure what the
>   response to such failure would be ... except see below about O_EXCL].

The unlink() we don't really care about because I had meant to add
O_EXCL to the following open() call (and indeed had already tweaked that
into my local version).  The failure of the open will cause the existing
"can't write history file" error to be output, but it would be good to
upgrade that warning to mention which history file we were really trying
to write.

> - You haven't tested for failure of rename() -- and note that rename() may 
>   falsely report failure in the event of certain NFS errors.

Yes, we should generate a warning about that (though I don't know what
we should do about the false NFS errors you mention).  It seems to me
that we don't need to do anything after the warning (i.e. the
$HISTFILE.new file would be left intact, but we wouldn't try to copy it
to the actual $HISTFILE or anything like that).

> I wasn't aware of [$HISTFILE.$$]; it should be changed.  Using the PID
> as a disambiguator is also not safe when NFS is involved

Looks like a simple improvement to gettempname() to make it take an
optional prefix arg will allow us to use that to create a better unique
filename based on $HISTFILE.  I've attached a patch that implements
this, fixes an unsafe use of gettempname() in builtin.c, and adds a new
string function named bicat() that works like dyncat(), but uses
permanent memory instead of the heap.

> No, [$HISTFILE.new would] definitely be worse (even more likely to
> have conflict when multiple shells are rewriting the history), not
> better.

No, the .new file is only written out after the lock is successful, so
there's no conflict unless someone breaks a lock.  The main difference
would be whether there is a random $HISTFILE.$$ left lying around when
zsh gets killed when writing out the history file, as opposed to the
constant $HISTFILE.new file being left (since the latter would get
reused quickly, but the former would stick around).  I'm thinking that
having extra, partial copies of the histfile lying around isn't going to
be particularly useful for data recovery, so it might be better to
switch over to just using $HISTFILE.new.

I'm also attaching a second patch that contains an updated version of
the rewrite-histfile-to-temp-file patch, this time using $HISTFILE.new
as the temp-file's name.

Thanks for the feedback!

..wayne..