From 7b0f24ab1f9770408a08181ab9f8ac2b43e5ab9b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 23 Aug 2023 18:27:45 -0700
Subject: [PATCH] Fix behavior of 'eshell-hist-ignoredups' when set to 'erase'

* lisp/eshell/em-hist.el (eshell-add-input-to-history): Refactor to
use 'pcase' and correct the logic for the 'erase' case.

* test/lisp/eshell/em-hist-tests.el: Require our test helpers.
(eshell-write-readonly-history): Rename to...
(em-hist-test/write-readonly-history): ... this.
(em-hist-test/add-to-history/allow-dups)
(em-hist-test/add-to-history/no-consecutive-dups)
(em-hist-test/add-to-history/erase-dups): New tests (bug#63360).
---
 lisp/eshell/em-hist.el            | 27 ++++++++++---------
 test/lisp/eshell/em-hist-tests.el | 43 ++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 79232d8e9b5..9d4b72b01df 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -381,20 +381,19 @@ Input is entered into the input history ring, if the value of
 variable `eshell-input-filter' returns non-nil when called on the
 input."
   (when (and (funcall eshell-input-filter input)
-             (if (eq eshell-hist-ignoredups 'erase)
-                 ;; Remove any old occurrences of the input, and put
-                 ;; the new one at the end.
-                 (unless (ring-empty-p eshell-history-ring)
-                   (ring-remove eshell-history-ring
-	                        (ring-member eshell-history-ring input))
-                   t)
-               ;; Always add...
-               (or (null eshell-hist-ignoredups)
-                   ;; ... or add if it's not already present at the
-                   ;; end.
-	           (not (ring-p eshell-history-ring))
-	           (ring-empty-p eshell-history-ring)
-	           (not (string-equal (eshell-get-history 0) input)))))
+             (pcase eshell-hist-ignoredups
+               ('nil t)                 ; Always add to history
+               ('erase                  ; Add, removing any old occurrences
+                (when-let ((old-index (ring-member eshell-history-ring input)))
+                  ;; Remove the old occurence of this input so we can
+                  ;; add it to the end.  FIXME: Should we try to
+                  ;; remove multiple old occurrences, e.g. if the user
+                  ;; recently changed to using `erase'?
+                  (ring-remove eshell-history-ring old-index))
+                t)
+               (_                       ; Add if not already the latest entry
+                (or (ring-empty-p eshell-history-ring)
+                    (not (string-equal (eshell-get-history 0) input))))))
     (eshell-put-history input))
   (setq eshell-save-history-index eshell-history-index)
   (setq eshell-history-index nil))
diff --git a/test/lisp/eshell/em-hist-tests.el b/test/lisp/eshell/em-hist-tests.el
index 35ae6bdc239..0f143355115 100644
--- a/test/lisp/eshell/em-hist-tests.el
+++ b/test/lisp/eshell/em-hist-tests.el
@@ -22,8 +22,16 @@
 (require 'ert)
 (require 'ert-x)
 (require 'em-hist)
+(require 'eshell)
 
-(ert-deftest eshell-write-readonly-history ()
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+;;; Tests:
+
+(ert-deftest em-hist-test/write-readonly-history ()
   "Test that having read-only strings in history is okay."
   (ert-with-temp-file histfile
     (let ((eshell-history-ring (make-ring 2)))
@@ -33,6 +41,39 @@
                    (propertize "echo bar" 'read-only t))
       (eshell-write-history histfile))))
 
+(ert-deftest em-hist-test/add-to-history/allow-dups ()
+  "Test adding to history, allowing dups."
+  (let ((eshell-hist-ignoredups nil))
+    (with-temp-eshell
+     (eshell-insert-command "echo hi")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo hi")
+     (should (equal (ring-elements eshell-history-ring)
+                    '("echo hi" "echo bye" "echo bye" "echo hi"))))))
+
+(ert-deftest em-hist-test/add-to-history/no-consecutive-dups ()
+  "Test adding to history, ignoring consecutive dups."
+  (let ((eshell-hist-ignoredups t))
+    (with-temp-eshell
+     (eshell-insert-command "echo hi")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo hi")
+     (should (equal (ring-elements eshell-history-ring)
+                    '("echo hi" "echo bye" "echo hi"))))))
+
+(ert-deftest em-hist-test/add-to-history/erase-dups ()
+  "Test adding to history, erasing any old dups."
+  (let ((eshell-hist-ignoredups 'erase))
+    (with-temp-eshell
+     (eshell-insert-command "echo hi")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo bye")
+     (eshell-insert-command "echo hi")
+     (should (equal (ring-elements eshell-history-ring)
+                    '("echo hi" "echo bye"))))))
+
 (provide 'em-hist-test)
 
 ;;; em-hist-tests.el ends here
-- 
2.39.2