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

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

[Robin Campbell Joy]

[-- 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

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

[Jim Porter]

On 5/22/2024 1:21 AM, Robin Campbell Joy wrote:
> Severity: minor
> Tags: patch
> 
> In case of eshell-hist-ingoredups set to 'erase, history handling isn't
> working correctly.

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.

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

[Robin Campbell Joy]

[-- 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

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

[Jim Porter]

On 5/22/2024 11:22 PM, Robin Campbell Joy wrote:
> 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.

Thanks for the updated patch. From a visual inspection, this all looks 
good to me.

Just two small things and then I think this is good to merge:

1) Could you add a commit message in the usual style (you can look at 
the other Emacs commits to get an idea of what these look like), and 
then generate the patch file with `git format-patch master`? This'll 
make it easier for me to apply the patch without having to write up a 
commit message myself.

2) Have you filled out copyright assignment paperwork with the FSF? 
While the code changes are below the maximum (15 lines) for no 
paperwork, the tests push it over. (I'm not 100% sure if we count test 
code for the copyright stuff.) I don't think I can check the paperwork 
status for people myself, but hopefully Eli (CCed) can help if needed.

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

[Eli Zaretskii]

> Cc: 71107@debbugs.gnu.org, eli@gnu.org
> Date: Thu, 23 May 2024 16:33:51 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> 2) Have you filled out copyright assignment paperwork with the FSF? 
> While the code changes are below the maximum (15 lines) for no 
> paperwork, the tests push it over. (I'm not 100% sure if we count test 
> code for the copyright stuff.) I don't think I can check the paperwork 
> status for people myself, but hopefully Eli (CCed) can help if needed.

Robin's assignment is on file, so we can accept this and other
contributions without limitations.

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

[Robin Campbell Joy]

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

On Fri, 24 May 2024 at 01:33, Jim Porter <jporterbugs@gmail.com> wrote:

> On 5/22/2024 11:22 PM, Robin Campbell Joy wrote:
> > 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.
>
> Thanks for the updated patch. From a visual inspection, this all looks
> good to me.
>
> Just two small things and then I think this is good to merge:
>
> 1) Could you add a commit message in the usual style (you can look at
> the other Emacs commits to get an idea of what these look like), and
> then generate the patch file with `git format-patch master`? This'll
> make it easier for me to apply the patch without having to write up a
> commit message myself.
>

Sure, hope this will do.

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

[-- Attachment #2: 0001-Erase-existing-duplicates-in-eshell-history-ring.patch --]
[-- Type: application/octet-stream, Size: 3126 bytes --]

From 5080ad2c1c706c4110ee3a57422f1bcf998cfbbf Mon Sep 17 00:00:00 2001
From: Robin Joy <emacs@robinjoy.net>
Date: Fri, 24 May 2024 14:26:39 +0200
Subject: [PATCH] Erase existing duplicates in eshell-history-ring

Erase all existing duplicates instead of just the last duplicate entry
when `eshell-hist-ignoredups` is set to 'erase. Multiple duplicates can
exist in case `eshell-hist-ignoredups` was set to something else than
'erase in the past or if the history file contains duplicates.

* lisp/eshell/em-hist.el (eshell-add-input-to-history): Remove all
duplicates from history ring.

* test/lisp/eshell/em-hist-tests.el (erase-existing-dups): New test.
---
 lisp/eshell/em-hist.el            |  8 +++-----
 test/lisp/eshell/em-hist-tests.el | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

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
-- 
2.45.1

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

[Jim Porter]

On 5/24/2024 5:29 AM, Robin Campbell Joy wrote:
> Sure, hope this will do.

Thanks, that looks good to me. I've merged your patch to the master 
branch as 984fb346fdf. Closing this bug now.

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

[Jim Porter]

On 5/23/2024 10:55 PM, Eli Zaretskii wrote:
> Robin's assignment is on file, so we can accept this and other
> contributions without limitations.

Thanks for checking.