Subject: 30.0.50; [PATCH] Improve read/append behavior of eshell history command
From: Liu Hui <[email-masked]>
To: [email-masked]
Date: Fri, 27 Oct 2023 12:15:00 +0800
List-Id: <debbugs-submit.debbugs.gnu.org>

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
From 20ad70995c5fbbb64e8501f5f1b216e17b3900ac Mon Sep 17 00:00:00 2001
From: Liu Hui <[email-masked]>
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