bug#71107: 29.3; eshell-hist/Incorrect history handling with eshell-hist-ignoredups 'erase

[Robin Campbell Joy <emacs@robinjoy.net>]

[-- Attachment #1.1: Type: text/plain, Size: 525 bytes --]

Severity: minor
Tags: patch

In case of eshell-hist-ingoredups set to 'erase, history handling isn't
working correctly.

1) If the input ring is empty, no elements are added to the ring.
2) If elements are on the ring, but the element added to the ring is not
yet in the ring, the last element is deleted.
3) When switching to 'erase with multiple duplicates in the ring, only
the last equal element is removed.

emacs -Q
(require 'eshell)
(setq eshell-hist-ignoredups 'erase)
(eshell)
$ echo foo
foo
M-p -> Empty input ring

[-- Attachment #1.2: Type: text/html, Size: 628 bytes --]

[-- Attachment #2: eshell-hist-ignoredups.patch --]
[-- Type: application/octet-stream, Size: 4140 bytes --]

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 4081f2a7f85..e7665de3ab7 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -385,10 +385,12 @@ eshell-add-input-to-history
              (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)
+                 (or (unless (ring-empty-p eshell-history-ring)
+                       (let (index)
+                         (while (setq index (ring-member eshell-history-ring input))
+                           (ring-remove eshell-history-ring index)))
+                       t)
+                     t)
                ;; Always add...
                (or (null eshell-hist-ignoredups)
                    ;; ... or add if it's not already present at the
diff --git a/test/lisp/eshell/em-hist-tests.el b/test/lisp/eshell/em-hist-tests.el
index d325e3a6402..04d7b1a0670 100644
--- a/test/lisp/eshell/em-hist-tests.el
+++ b/test/lisp/eshell/em-hist-tests.el
@@ -33,6 +33,51 @@ eshell-write-readonly-history
                    (propertize "echo bar" 'read-only t))
       (eshell-write-history histfile))))
 
+(ert-deftest eshell-add-history-element-to-empty-ring-with-erase-dups ()
+  "Test that we add a new history item to an empty history ring with `eshell-hist-ignoredups' set to 'erase."
+  (ert-with-test-buffer (:name "eshell-hist-test")
+    (let ((eshell-history-ring (make-ring 2))
+          (eshell-hist-ignoredups 'erase))
+      (eshell-add-input-to-history "echo foo")
+      (should (= (ring-length eshell-history-ring) 1))
+      (should (= (ring-member eshell-history-ring "echo foo") 0)))))
+
+(ert-deftest eshell-add-history-element-to-non-empty-ring-with-erase-dups-erases-duplicate ()
+  "Test that when we add an existing history item to a non empty history ring with `eshell-hist-ignoredups' set to 'erase the existing element is erased."
+  (ert-with-test-buffer (:name "eshell-hist-test")
+    (let ((eshell-history-ring (make-ring 2))
+          (eshell-hist-ignoredups 'erase))
+      (eshell-add-input-to-history "echo foo")
+      (eshell-add-input-to-history "echo foo")
+      (should (= (ring-length eshell-history-ring) 1))
+      (should (= (ring-member eshell-history-ring "ls") 0)))))
+
+(ert-deftest eshell-add-history-element-to-non-empty-ring-with-erase-dups-erases-duplicates ()
+  "Test that when we add an existing history item to a non empty history ring with `eshell-hist-ignoredups' set to 'erase the existing elements are erased."
+  (ert-with-test-buffer (:name "eshell-hist-test")
+    (let ((eshell-history-ring (make-ring 5))
+          (eshell-hist-ignoredups nil))
+      (eshell-add-input-to-history "echo foo")
+      (eshell-add-input-to-history "echo bar")
+      (eshell-add-input-to-history "echo foo")
+      (should (= (ring-length eshell-history-ring) 3))
+      (setq eshell-hist-ignoredups 'erase)
+      (eshell-add-input-to-history "echo foo")
+      (should (= (ring-length eshell-history-ring) 2))
+      (should (= (ring-member eshell-history-ring "echo foo") 0))
+      (should (= (ring-member eshell-history-ring "echo bar") 1))))))
+
+(ert-deftest eshell-add-history-element-to-non-empty-ring-with-erase-dups-keeps-non-duplicates ()
+  "Test that when we add a new  history item to a non empty history ring with `eshell-hist-ignoredups' set to 'erase the existing elements are not erased."
+  (ert-with-test-buffer (:name "eshell-hist-test")
+    (let ((eshell-history-ring (make-ring 2))
+          (eshell-hist-ignoredups 'erase))
+      (eshell-add-input-to-history "echo foo")
+      (eshell-add-input-to-history "echo bar")
+      (should (= (ring-length eshell-history-ring) 2))
+      (should (= (ring-member eshell-history-ring "echo foo") 1))
+      (should (= (ring-member eshell-history-ring "echo bar") 0)))))
+
 (provide 'em-hist-test)
 
 ;;; em-hist-tests.el ends here