Subject: bug#66768: closed (Re: bug#66768: 30.0.50; [PATCH] Improve read/append behavior of eshell history command)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 11 Nov 2023 02:04:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
66768: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66768
GNU Bug Tracking System
Contact [email-masked] with problems

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

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!

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