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

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

[Liu Hui]

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

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

[-- Attachment #2: 0001-Improve-read-append-behavior-of-eshell-history-comma.patch --]
[-- Type: text/x-patch, Size: 6038 bytes --]

From 20ad70995c5fbbb64e8501f5f1b216e17b3900ac Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Fri, 27 Oct 2023 10:07:09 +0800
Subject: [PATCH] Improve read/append behavior of eshell history command

* lisp/eshell/em-hist.el (eshell-hist--new-items): New variable.
(eshell-hist-initialize): Set `eshell-hist--new-items'.
(eshell/history): Change the behavior of 'history -a' to "append new
history in current buffer to history file".  Update help text.
(eshell-add-input-to-history): Increase counter of new history items.
(eshell-read-history): Respect eshell-hist-ignoredups option.
(eshell-write-history): Add optional argument NEW-ITEMS and support
writing only new history items to history file.
---
 lisp/eshell/em-hist.el | 47 ++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 9d4b72b01df..46bd8735c3c 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -195,6 +195,9 @@ eshell-history-ring
 (defvar eshell-history-index nil)
 (defvar eshell-matching-input-from-input-string "")
 (defvar eshell-save-history-index nil)
+(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.")
 
 (defvar-keymap eshell-isearch-map
   :doc "Keymap used in isearch in Eshell."
@@ -283,6 +286,7 @@ eshell-hist-initialize
 
   (make-local-variable 'eshell-history-index)
   (make-local-variable 'eshell-save-history-index)
+  (make-local-variable 'eshell-hist--new-items)
 
   (if (minibuffer-window-active-p (selected-window))
       (setq-local eshell-save-history-on-exit nil)
@@ -323,11 +327,11 @@ eshell/history
   (eshell-eval-using-options
    "history" args
    '((?r "read" nil read-history
-	 "read from history file to current history list")
+	 "clear current history list and read from history file to it")
      (?w "write" nil write-history
 	 "write current history list to history file")
      (?a "append" nil append-history
-	 "append current history list to history file")
+	 "append new history in current buffer to history file")
      (?h "help" nil nil "display this usage message")
      :usage "[n] [-rwa [filename]]"
      :post-usage
@@ -351,7 +355,7 @@ eshell/history
      (cond
       (read-history (eshell-read-history file))
       (write-history (eshell-write-history file))
-      (append-history (eshell-write-history file t))
+      (append-history (eshell-write-history file 'append 'new-items))
       (t
        (let* ((index (1- (or length (ring-length eshell-history-ring))))
 	      (ref (- (ring-length eshell-history-ring) index)))
@@ -394,6 +398,8 @@ eshell-add-input-to-history
                (_                       ; Add if not already the latest entry
                 (or (ring-empty-p eshell-history-ring)
                     (not (string-equal (eshell-get-history 0) input))))))
+    (setq eshell-hist--new-items
+          (min eshell-history-size (1+ eshell-hist--new-items)))
     (eshell-put-history input))
   (setq eshell-save-history-index eshell-history-index)
   (setq eshell-history-index nil))
@@ -455,21 +461,28 @@ eshell-read-history
 		      (re-search-backward "^[ \t]*\\([^#\n].*\\)[ \t]*$"
 					  nil t))
 	    (let ((history (match-string 1)))
-	      (if (or (null ignore-dups)
-		      (ring-empty-p ring)
-		      (not (string-equal (ring-ref ring 0) history)))
-		  (ring-insert-at-beginning
+              (if (or (ring-empty-p ring)
+                      (null ignore-dups)
+                      (and (not (string-equal
+                                 (ring-ref ring (1- (ring-length ring)))
+                                 history))
+                           (not (and (eq ignore-dups 'erase)
+                                     (ring-member ring history)))))
+                  (ring-insert-at-beginning
 		   ring (subst-char-in-string ?\177 ?\n history))))
 	    (setq count (1+ count))))
 	(setq eshell-history-ring ring
-	      eshell-history-index nil))))))
+	      eshell-history-index nil
+              eshell-hist--new-items 0))))))
 
-(defun eshell-write-history (&optional filename append)
+(defun eshell-write-history (&optional filename append new-items)
   "Writes the buffer's `eshell-history-ring' to a history file.
-The name of the file is given by the variable
-`eshell-history-file-name'.  The original contents of the file are
-lost if `eshell-history-ring' is not empty.  If
-`eshell-history-file-name' is nil this function does nothing.
+If the optional argument FILENAME is nil, the value of
+`eshell-history-file-name' is used.  This function does nothing
+if the value resolves to nil.  The optional argument APPEND has
+the same meaning as that in `write-region'.  If the optional
+argument NEW-ITEMS is non-nil, writes only new history items that
+have not been written to any history file yet.
 
 Useful within process sentinels.
 
@@ -480,13 +493,14 @@ eshell-write-history
      ((or (null file)
 	  (equal file "")
 	  (null eshell-history-ring)
-	  (ring-empty-p eshell-history-ring))
+	  (ring-empty-p eshell-history-ring)
+          (and new-items (= eshell-hist--new-items 0)))
       nil)
      ((not (file-writable-p resolved-file))
       (message "Cannot write history file %s" resolved-file))
      (t
       (let* ((ring eshell-history-ring)
-	     (index (ring-length ring)))
+	     (index (if new-items eshell-hist--new-items (ring-length ring))))
 	;; Write it all out into a buffer first.  Much faster, but
 	;; messier, than writing it one line at a time.
 	(with-temp-buffer
@@ -499,7 +513,8 @@ eshell-write-history
 	      (subst-char-in-region start (1- (point)) ?\n ?\177)))
 	  (eshell-with-private-file-modes
 	   (write-region (point-min) (point-max) resolved-file append
-			 'no-message))))))))
+			 'no-message)))
+        (setq eshell-hist--new-items 0))))))
 
 (defun eshell-list-history ()
   "List in help buffer the buffer's input history."
-- 
2.25.1

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

[Jim Porter]

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.

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

[Liu Hui]

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

Jim Porter <jporterbugs@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.

[-- Attachment #2: 0001-Improve-read-append-behavior-of-eshell-history-comma.patch --]
[-- Type: application/x-patch, Size: 7814 bytes --]

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

[Jim Porter]

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!