bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Liu Hui]

Hi,

Eshell saves history by default when quitting the buffer or emacs, but
some history is missing when there are multiple eshell buffers. For
example,

1. emacs -Q
2. M-x eshell, and type foo
3. C-u M-x eshell, and type bar
4. quit emacs

I would expect both commands are saved, but in fact only one command
is saved in the file eshell-history-file-name. I think the reason is
the history file is always overwritten when an eshell buffer tries to
save the history, so maybe eshell should save commands by appending
to the file?

Best

--
Liu Hui

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Liu Hui]

Hi Jim,

> Eshell saves history by default when quitting the buffer or emacs, but
> some history is missing when there are multiple eshell buffers. For
> example,
>
> 1. emacs -Q
> 2. M-x eshell, and type foo
> 3. C-u M-x eshell, and type bar
> 4. quit emacs
>
> I would expect both commands are saved, but in fact only one command
> is saved in the file eshell-history-file-name. I think the reason is
> the history file is always overwritten when an eshell buffer tries to
> save the history, so maybe eshell should save commands by appending
> to the file?

Since Eshell can append new history now, I think it makes sense to
change the default behavior from overwriting to appending, i.e., using
'(eshell-write-history nil t)' in eshell-exit-hook and
eshell-save-some-history. WDYT?

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Eli Zaretskii]

> Cc: 66700@debbugs.gnu.org
> From: Liu Hui <liuhui1610@gmail.com>
> Date: Tue, 14 Nov 2023 12:40:00 +0800
> 
> Since Eshell can append new history now, I think it makes sense to
> change the default behavior from overwriting to appending, i.e., using
> '(eshell-write-history nil t)' in eshell-exit-hook and
> eshell-save-some-history. WDYT?

IMO, this is too soon to change the default behavior in incompatible
ways.  We need to wait for a while, perhaps at least one major
release, before we do that.

Thanks.

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Sean Whitton]

Hello,

On Tue 14 Nov 2023 at 02:32pm +02, Eli Zaretskii wrote:

>> Cc: 66700@debbugs.gnu.org
>> From: Liu Hui <liuhui1610@gmail.com>
>> Date: Tue, 14 Nov 2023 12:40:00 +0800
>>
>> Since Eshell can append new history now, I think it makes sense to
>> change the default behavior from overwriting to appending, i.e., using
>> '(eshell-write-history nil t)' in eshell-exit-hook and
>> eshell-save-some-history. WDYT?
>
> IMO, this is too soon to change the default behavior in incompatible
> ways.  We need to wait for a while, perhaps at least one major
> release, before we do that.

This is fair enough.  It would be really useful to have the default
behaviour now for those of used to it from other shells, though -- Liu,
perhaps you would be interested in adding a defcustom?

-- 
Sean Whitton

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Jim Porter]

On 11/14/2023 4:43 AM, Sean Whitton wrote:
> On Tue 14 Nov 2023 at 02:32pm +02, Eli Zaretskii wrote:
>>
>> IMO, this is too soon to change the default behavior in incompatible
>> ways.  We need to wait for a while, perhaps at least one major
>> release, before we do that.
> 
> This is fair enough.  It would be really useful to have the default
> behaviour now for those of used to it from other shells, though -- Liu,
> perhaps you would be interested in adding a defcustom?

Yeah, I think a defcustom would be good here. That would also make it 
easier for us to test out the new code and find any bugs.

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Liu Hui]

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

Jim Porter <jporterbugs@gmail.com> 于2023年11月15日周三 01:15写道：
>
> On 11/14/2023 4:43 AM, Sean Whitton wrote:
> > On Tue 14 Nov 2023 at 02:32pm +02, Eli Zaretskii wrote:
> >>
> >> IMO, this is too soon to change the default behavior in incompatible
> >> ways.  We need to wait for a while, perhaps at least one major
> >> release, before we do that.
> >
> > This is fair enough.  It would be really useful to have the default
> > behaviour now for those of used to it from other shells, though -- Liu,
> > perhaps you would be interested in adding a defcustom?
>
> Yeah, I think a defcustom would be good here. That would also make it
> easier for us to test out the new code and find any bugs.

Thank you all for your suggestions. A defcustom has been added in the
attached patch without changing previous behavior.

[-- Attachment #2: 0001-Add-option-eshell-history-append.patch --]
[-- Type: text/x-patch, Size: 2381 bytes --]

From e2cd3a9d28b35806d4e6a83e98f0dc25550651e2 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Thu, 16 Nov 2023 11:45:28 +0800
Subject: [PATCH] Add option `eshell-history-append'

* lisp/eshell/em-hist.el (eshell-history-append): New option.
(eshell--save-history): New function.
(eshell-hist-initialize):
(eshell-save-some-history): Replace eshell-write-history with
eshell--save-history, which respects the new option.
---
 lisp/eshell/em-hist.el | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index cf03f8399a6..79336204847 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -116,6 +116,12 @@ eshell-save-history-on-exit
 		 (const :tag "Ask" ask)
 		 (const :tag "Always save" t)))
 
+(defcustom eshell-history-append nil
+  "If non-nil, append new entries to the history file when saving history."
+  :type '(choice (const :tag "Overwrite history file" nil)
+		 (const :tag "Append new entries to file" t))
+  :version "30.1")
+
 (defcustom eshell-input-filter 'eshell-input-filter-default
   "Predicate for filtering additions to input history.
 Takes one argument, the input.  If non-nil, the input may be saved on
@@ -294,17 +300,21 @@ eshell-hist-initialize
     (if eshell-history-file-name
 	(eshell-read-history nil t))
 
-    (add-hook 'eshell-exit-hook #'eshell-write-history nil t))
+    (add-hook 'eshell-exit-hook #'eshell--save-history nil t))
 
   (unless eshell-history-ring
     (setq eshell-history-ring (make-ring eshell-history-size)))
 
-  (add-hook 'eshell-exit-hook #'eshell-write-history nil t)
+  (add-hook 'eshell-exit-hook #'eshell--save-history nil t)
 
   (add-hook 'kill-emacs-query-functions #'eshell-save-some-history)
 
   (add-hook 'eshell-input-filter-functions #'eshell-add-to-history nil t))
 
+(defun eshell--save-history ()
+  "Save the history for current Eshell buffer."
+  (eshell-write-history nil eshell-history-append))
+
 (defun eshell-save-some-history ()
   "Save the history for any open Eshell buffers."
   (dolist (buf (buffer-list))
@@ -318,7 +328,7 @@ eshell-save-some-history
 			(format-message
 			 "Save input history for Eshell buffer `%s'? "
 			 (buffer-name buf)))))
-	      (eshell-write-history)))))
+	      (eshell--save-history)))))
   t)
 
 (defun eshell/history (&rest args)
-- 
2.25.1

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Sean Whitton]

On Thu 16 Nov 2023 at 05:47pm +08, Liu Hui wrote:

> Jim Porter <jporterbugs@gmail.com> 于2023年11月15日周三 01:15写道：
>>
>> On 11/14/2023 4:43 AM, Sean Whitton wrote:
>> > On Tue 14 Nov 2023 at 02:32pm +02, Eli Zaretskii wrote:
>> >>
>> >> IMO, this is too soon to change the default behavior in incompatible
>> >> ways.  We need to wait for a while, perhaps at least one major
>> >> release, before we do that.
>> >
>> > This is fair enough.  It would be really useful to have the default
>> > behaviour now for those of used to it from other shells, though -- Liu,
>> > perhaps you would be interested in adding a defcustom?
>>
>> Yeah, I think a defcustom would be good here. That would also make it
>> easier for us to test out the new code and find any bugs.
>
> Thank you all for your suggestions. A defcustom has been added in the
> attached patch without changing previous behavior.

Thanks.  This looks good, but can you add a NEWS entry, please?

Ideally it would also get documented in the Eshell manual.

> From e2cd3a9d28b35806d4e6a83e98f0dc25550651e2 Mon Sep 17 00:00:00 2001
> From: Liu Hui <liuhui1610@gmail.com>
> Date: Thu, 16 Nov 2023 11:45:28 +0800
> Subject: [PATCH] Add option `eshell-history-append'
>
> * lisp/eshell/em-hist.el (eshell-history-append): New option.
> (eshell--save-history): New function.
> (eshell-hist-initialize):
> (eshell-save-some-history): Replace eshell-write-history with
> eshell--save-history, which respects the new option.
> ---
>  lisp/eshell/em-hist.el | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
> index cf03f8399a6..79336204847 100644
> --- a/lisp/eshell/em-hist.el
> +++ b/lisp/eshell/em-hist.el
> @@ -116,6 +116,12 @@ eshell-save-history-on-exit
>  		 (const :tag "Ask" ask)
>  		 (const :tag "Always save" t)))
>  
> +(defcustom eshell-history-append nil
> +  "If non-nil, append new entries to the history file when saving history."
> +  :type '(choice (const :tag "Overwrite history file" nil)
> +		 (const :tag "Append new entries to file" t))
> +  :version "30.1")
> +
>  (defcustom eshell-input-filter 'eshell-input-filter-default
>    "Predicate for filtering additions to input history.
>  Takes one argument, the input.  If non-nil, the input may be saved on
> @@ -294,17 +300,21 @@ eshell-hist-initialize
>      (if eshell-history-file-name
>  	(eshell-read-history nil t))
>  
> -    (add-hook 'eshell-exit-hook #'eshell-write-history nil t))
> +    (add-hook 'eshell-exit-hook #'eshell--save-history nil t))
>  
>    (unless eshell-history-ring
>      (setq eshell-history-ring (make-ring eshell-history-size)))
>  
> -  (add-hook 'eshell-exit-hook #'eshell-write-history nil t)
> +  (add-hook 'eshell-exit-hook #'eshell--save-history nil t)
>  
>    (add-hook 'kill-emacs-query-functions #'eshell-save-some-history)
>  
>    (add-hook 'eshell-input-filter-functions #'eshell-add-to-history nil t))
>  
> +(defun eshell--save-history ()
> +  "Save the history for current Eshell buffer."
> +  (eshell-write-history nil eshell-history-append))
> +
>  (defun eshell-save-some-history ()
>    "Save the history for any open Eshell buffers."
>    (dolist (buf (buffer-list))
> @@ -318,7 +328,7 @@ eshell-save-some-history
>  			(format-message
>  			 "Save input history for Eshell buffer `%s'? "
>  			 (buffer-name buf)))))
> -	      (eshell-write-history)))))
> +	      (eshell--save-history)))))
>    t)
>  
>  (defun eshell/history (&rest args)

-- 
Sean Whitton

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Liu Hui]

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

Sean Whitton <spwhitton@spwhitton.name> 于2023年12月6日周三 23:33写道：
>
> On Thu 16 Nov 2023 at 05:47pm +08, Liu Hui wrote:
>
> > Jim Porter <jporterbugs@gmail.com> 于2023年11月15日周三 01:15写道：
> >>
> >> On 11/14/2023 4:43 AM, Sean Whitton wrote:
> >> > On Tue 14 Nov 2023 at 02:32pm +02, Eli Zaretskii wrote:
> >> >>
> >> >> IMO, this is too soon to change the default behavior in incompatible
> >> >> ways.  We need to wait for a while, perhaps at least one major
> >> >> release, before we do that.
> >> >
> >> > This is fair enough.  It would be really useful to have the default
> >> > behaviour now for those of used to it from other shells, though -- Liu,
> >> > perhaps you would be interested in adding a defcustom?
> >>
> >> Yeah, I think a defcustom would be good here. That would also make it
> >> easier for us to test out the new code and find any bugs.
> >
> > Thank you all for your suggestions. A defcustom has been added in the
> > attached patch without changing previous behavior.
>
> Thanks.  This looks good, but can you add a NEWS entry, please?
>
> Ideally it would also get documented in the Eshell manual.

Thanks for your feedback. I have updated the patch as suggested.

[-- Attachment #2: 0001-Add-option-eshell-history-append.patch --]
[-- Type: text/x-patch, Size: 4774 bytes --]

From acf054d4832ce248d8441dfbb0418b69d4ecfe98 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Thu, 16 Nov 2023 11:45:28 +0800
Subject: [PATCH] Add option `eshell-history-append'

* lisp/eshell/em-hist.el (eshell-history-append): New option.
(eshell--save-history): New function.
(eshell-hist-initialize):
(eshell-save-some-history): Replace eshell-write-history with
eshell--save-history, which respects the new option.
* doc/misc/eshell.texi (History): Update.
* etc/NEWS: Announce the change.  (Bug#66700)
---
 doc/misc/eshell.texi   | 18 +++++++++++++-----
 etc/NEWS               |  6 ++++++
 lisp/eshell/em-hist.el | 16 +++++++++++++---
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index e8aa8cdc6a3..389b4cbbc21 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1237,11 +1237,19 @@ History
 beginning with @code{foo} is accessible by @code{!foo:n}.
 
 @vindex eshell-history-file-name
-The history ring is loaded from a file at the start of every session,
-and written back to the file at the end of every session.  The file path
-is specified in @code{eshell-history-file-name}.  Unlike other shells,
-such as Bash, Eshell can not be configured to keep a history ring of a
-different size than that of the history file.
+@vindex eshell-history-append
+The history is loaded to the history ring from a file at the start of
+every session, and saved to the file at the end of every session.  The
+default history saving behavior is overwriting the file with the whole
+history ring of the session.  By setting @code{eshell-history-append}
+to non-@code{nil}, the history will be saved by appending new entries
+of that session to the file, which could prevent potential history
+loss with multiple Eshell sessions.  The file path is specified in
+@code{eshell-history-file-name}.  Unlike other shells, such as Bash,
+Eshell can not be configured to control the size of the history file.
+Especially, when @code{eshell-history-append} is non-@code{nil}, the
+size of file will keep increasing and the recommended way to truncate
+the file is to run the @samp{history -w} command in an Eshell session.
 
 Since the default buffer navigation and searching key-bindings are
 still present in the Eshell buffer, the commands for history
diff --git a/etc/NEWS b/etc/NEWS
index c55719416d3..e14ab4aed27 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -617,6 +617,12 @@ calling external rgrep.
 +++
 *** If a command exits abnormally, the Eshell prompt now shows its exit code.
 
++++
+*** New user option 'eshell-history-append'.
+If non-nil, each Eshell session will save history by appending new
+entries of that session to the history file rather than overwriting
+the file with the whole history of the session.  The default is nil.
+
 ** Minibuffer and Completions
 
 *** New commands 'previous-line-completion' and 'next-line-completion'.
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index cf03f8399a6..79336204847 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -116,6 +116,12 @@ eshell-save-history-on-exit
 		 (const :tag "Ask" ask)
 		 (const :tag "Always save" t)))
 
+(defcustom eshell-history-append nil
+  "If non-nil, append new entries to the history file when saving history."
+  :type '(choice (const :tag "Overwrite history file" nil)
+		 (const :tag "Append new entries to file" t))
+  :version "30.1")
+
 (defcustom eshell-input-filter 'eshell-input-filter-default
   "Predicate for filtering additions to input history.
 Takes one argument, the input.  If non-nil, the input may be saved on
@@ -294,17 +300,21 @@ eshell-hist-initialize
     (if eshell-history-file-name
 	(eshell-read-history nil t))
 
-    (add-hook 'eshell-exit-hook #'eshell-write-history nil t))
+    (add-hook 'eshell-exit-hook #'eshell--save-history nil t))
 
   (unless eshell-history-ring
     (setq eshell-history-ring (make-ring eshell-history-size)))
 
-  (add-hook 'eshell-exit-hook #'eshell-write-history nil t)
+  (add-hook 'eshell-exit-hook #'eshell--save-history nil t)
 
   (add-hook 'kill-emacs-query-functions #'eshell-save-some-history)
 
   (add-hook 'eshell-input-filter-functions #'eshell-add-to-history nil t))
 
+(defun eshell--save-history ()
+  "Save the history for current Eshell buffer."
+  (eshell-write-history nil eshell-history-append))
+
 (defun eshell-save-some-history ()
   "Save the history for any open Eshell buffers."
   (dolist (buf (buffer-list))
@@ -318,7 +328,7 @@ eshell-save-some-history
 			(format-message
 			 "Save input history for Eshell buffer `%s'? "
 			 (buffer-name buf)))))
-	      (eshell-write-history)))))
+	      (eshell--save-history)))))
   t)
 
 (defun eshell/history (&rest args)
-- 
2.25.1

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Sean Whitton]

Hello,

I've installed the change, thank you again.

I copy-edited your manual text a bit while doing so.

-- 
Sean Whitton

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Jim Porter]

On 12/7/2023 3:33 AM, Sean Whitton wrote:
> I've installed the change, thank you again.
> 
> I copy-edited your manual text a bit while doing so.

Thanks for merging. This fell off my radar.

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Liu Hui]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> 于2023年11月14日周二 20:32写道：

> > Since Eshell can append new history now, I think it makes sense to
> > change the default behavior from overwriting to appending, i.e., using
> > '(eshell-write-history nil t)' in eshell-exit-hook and
> > eshell-save-some-history. WDYT?
>
> IMO, this is too soon to change the default behavior in incompatible
> ways.  We need to wait for a while, perhaps at least one major
> release, before we do that.
>
> Thanks.

It is not an incompatible change at the user level. It is actually a
bug fix for people using multiple eshell sessions. For people using a
single eshell buffer, the history inside eshell, when reading back
from the file, is always same no matter how history is saved, and the
only difference is the content of history file.

As others have suggested, we could add an option to toggle the
behavior with appending new history as the default. It allows more
testing and users can easily revert back to old behavior. Do you think
it is acceptable?

bug#66700: 30.0.50; Some history is missing with multiple eshell buffers

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Wed, 15 Nov 2023 17:59:23 +0800
> Cc: jporterbugs@gmail.com, 66700@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> 于2023年11月14日周二 20:32写道：
> 
> > IMO, this is too soon to change the default behavior in incompatible
> > ways.  We need to wait for a while, perhaps at least one major
> > release, before we do that.
> >
> > Thanks.
> 
> It is not an incompatible change at the user level. It is actually a
> bug fix for people using multiple eshell sessions. For people using a
> single eshell buffer, the history inside eshell, when reading back
> from the file, is always same no matter how history is saved, and the
> only difference is the content of history file.
> 
> As others have suggested, we could add an option to toggle the
> behavior with appending new history as the default. It allows more
> testing and users can easily revert back to old behavior. Do you think
> it is acceptable?

An option is, of course, acceptable, but its default should not change
the previous behavior.  We could consider changing the default later,
after we have enough experience with this option, and can ask users
whether they like the "append" behavior to be the default.