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: 640 bytes --]

On Thu, 23 May 2024 at 06:35, Jim Porter <jporterbugs@gmail.com> wrote:

> I believe this should already be fixed by commit 7b0f24ab1f9 from
> bug#63360.
>
> However, I didn't bother to account for the case where there may be
> multiple entries to remove (I just added a FIXME note since it seemed
> like a low priority). If removing multiple duplicates is important to
> you, would you be willing to rebase your patch on top of the master
> branch? Thanks either way.
>

Thanks, not sure why I didn't find this when looking for an existing bug,
should have checked master first though.

Attached the rebased patch. Thanks for fixing this.

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

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

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 21029eae1bc..b171a2850ff 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -398,11 +398,9 @@ eshell-add-input-to-history
              (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 occurrence 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'?
+                (while-let ((old-index (ring-member eshell-history-ring input)))
+                  ;; Remove the old occurrences of this input so we can
+                  ;; add it to the end.
                   (ring-remove eshell-history-ring old-index))
                 t)
                (_                       ; Add if not already the latest entry
diff --git a/test/lisp/eshell/em-hist-tests.el b/test/lisp/eshell/em-hist-tests.el
index a4e1e01b124..40e6f90478d 100644
--- a/test/lisp/eshell/em-hist-tests.el
+++ b/test/lisp/eshell/em-hist-tests.el
@@ -163,6 +163,23 @@ em-hist-test/add-to-history/erase-dups
      (should (equal (ring-elements eshell-history-ring)
                     '("echo hi" "echo bye"))))))
 
+(ert-deftest em-hist-test/add-to-history/erase-existing-dups ()
+  "Test adding to history, erasing any old dups after switching to 'erase."
+  (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")
+     (eshell-insert-command "echo bye")
+     (setq eshell-hist-ignoredups 'erase)
+     (eshell-insert-command "echo hi")
+     (should (equal (ring-elements eshell-history-ring)
+                    '("echo hi" "echo bye" "echo bye" "echo bye")))
+     (eshell-insert-command "echo bye")
+     (should (equal (ring-elements eshell-history-ring)
+                    '("echo bye" "echo hi"))))))
+
 (provide 'em-hist-test)
 
 ;;; em-hist-tests.el ends here