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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66768 in the body.
You can then email your comments to 66768 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Liu Hui <liuhui1610 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50;
 [PATCH] Improve read/append behavior of eshell history command
Date: Fri, 27 Oct 2023 12:15:00 +0800

[Message part 1 (text/plain, inline)]

Hi,

This patch fixes some corner cases of the eshell history command about
reading (-r) and appending (-a).

Recipe:

1. create a sample history file containing some lines, e.g.

    ls

2. emacs -Q --eval "(setq eshell-history-file-name ...)"  -f eshell

3. type following commands:

$ ls
$ history -a
$ cd /tmp
$ history -a

'history -a' doesn't distinguish old history from new history items of
current buffer, and always appends the whole history list whenever it
is called, resulting in a mess of the content of history file:

    ls
    ls
    ls
    history -a
    ls
    ls
    history -a
    cd /tmp/
    history -a

If another eshell buffer or emacs instance is launched before we quit
this eshell buffer, they will read messed history. Thus this patch
changes behavior of 'history -a' from "append current history list to
history file" to "append new history in current buffer to history
file", which is also consistent with bash's 'history -a'.

4. continue to type:

$ (setq eshell-hist-ignoredups t)
$ history -a; history -r
$ history

'history -r', which calls eshell-read-history, doesn't remove
consecutive "ls" duplicates from the history file. Thus this patch
amends eshell-read-history to make it respect the
eshell-hist-ignoredups option (both t and 'erase).

Since 'history -r' replaces current history list, which is actually
equivalent to bash's 'history -c; history -r', I have updated the help
text. Maybe it should be split into two commands, i.e. adding 'history
-c'?


Best,

--
Liu Hui

[0001-Improve-read-append-behavior-of-eshell-history-comma.patch (text/x-patch, attachment)]

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.

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

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 66768 <at> debbugs.gnu.org
Subject: Re: bug#66768: 30.0.50; [PATCH] Improve read/append behavior of
 eshell history command
Date: Tue, 7 Nov 2023 18:14:30 +0800

[Message part 1 (text/plain, inline)]

Jim Porter <jporterbugs <at> gmail.com> 于2023年11月4日周六 03:38写道：

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

Thanks for your comments! I've updated the patch to address all
concerns.

[0001-Improve-read-append-behavior-of-eshell-history-comma.patch (application/x-patch, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 66768-done <at> debbugs.gnu.org
Subject: Re: bug#66768: 30.0.50; [PATCH] Improve read/append behavior of
 eshell history command
Date: Fri, 10 Nov 2023 18:03:07 -0800

Version: 30.1

On 11/7/2023 2:14 AM, Liu Hui wrote:
> Thanks for your comments! I've updated the patch to address all
> concerns.

Everything in your patch looks good to me. I've now merged it as 
8b3969006fe (with a small editorial change to a docstring), and added a 
few extra regression tests for the history code on top as f2b162f8ee5.

Closing this now. Thanks for helping improve Eshell!

Previous Next

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