GNU bug report logs - #66768
30.0.50; [PATCH] Improve read/append behavior of eshell history command

Previous Next

Full log

Message #8 received at 66768 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Liu Hui <liuhui1610 <at> gmail.com>, 66768 <at> debbugs.gnu.org
Subject: Re: bug#66768: 30.0.50; [PATCH] Improve read/append behavior of
 eshell history command
Date: Fri, 3 Nov 2023 12:38:35 -0700

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.