bug#66768: 30.0.50; [PATCH] Improve read/append behavior of eshell history command

[Jim Porter <jporterbugs@gmail.com>]

On 10/26/2023 9:15 PM, Liu Hui wrote:
> This patch fixes some corner cases of the eshell history command about
> reading (-r) and appending (-a).

Thanks for the patch. I think this makes sense overall, though I do want 
to go through em-hist.el at some point and rework how shared history 
functions. (I use shared history from multiple Zsh shells pretty 
extensively, so I'm hoping to at least provide the necessary hooks to 
make Eshell work like my setup.)

In the meantime though, this is a step in the right direction. Some 
comments on the code:

> +(defvar eshell-hist--new-items 0
> +  "The number of new history items that have not been written to
> +file.  This variable is local in each eshell buffer.")

To prevent mistakes, I'd set this to nil here, and then call 
'(setq-local eshell-hist--new-items 0)' in 'eshell-hist-initialize'.

> -(defun eshell-write-history (&optional filename append)
> +(defun eshell-write-history (&optional filename append new-items)

I don't think this new argument is necessary. I suppose you did this for 
backwards-compatibility, but I'd say that the current appending behavior 
is just a bug, so you don't need to add an explicit way to opt into your 
new behavior; just do it whenever 'append' is non-nil.

Some regression tests would also be nice. There are already a few in 
test/lisp/eshell/em-hist-tests.el that you should be able to use as a basis.