bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase

bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase

[Alexander Kozhevnikov]

Bug:

In these two lines in em-hist.el (in the function eshell-add-input-to-history):

                   (ring-remove eshell-history-ring
                        (ring-member eshell-history-ring input))

Since `ring-member` will return nil if there is no duplicate, but
`ring-remove` will delete the last entry if given a nil index, a
history entry is always deleted even if there is no duplicate.

Crucially, this happens even if the history ring is smaller than
eshell-history-size - the history ring never grows. In the most
egregious case, if the user starts from a history with one entry,
their history ring will get stuck at just one entry.

(If the user starts with zero history entries, their history ring will
also get stuck at zero entries if eshell-hist-ignoredups is 'erase,
but the cause is somewhere else in that same function - haven't had
time to track that down more precisely yet.)

I bet this reproduces on all versions of Emacs from latest going back
at least a few years, but most of my devices are on 28.2 in
particular.

Reproduce:

From `emacs -Q`, evaluate the following:

; turn on the relevant setting
(setq eshell-hist-ignoredups 'erase)

; start with a clean blank history
(setq eshell-history-file-name "/tmp/a-new-eshell-history-for-test")

Start eshell with `M-x eshell RET`.

Manually insert one entry into the history ring after starting eshell
with a lower-level function to get started, for example by evaluating:

Now run any other command inside eshell and observe that the history
ring never grows, even though it should grow up to the default limit
(128).

Fix:

Replace the above two lines with these three lines, or something
similar. (I don't think this is substantial enough to be copyrightable
and thus shouldn't require copyright assignment, but let me know if
you need me to get those papers signed.)

                   (let ((index (ring-member eshell-history-ring input)))
                     (when index
                       (ring-remove eshell-history-ring index)))

bug#63362: Bug+fix for eshell-hist-ignore-dups 'erase

[Alexander Kozhevnikov]

Ah! The second bug (the case where there are zero history entries) is
due to the immediately surrounding lines:

                 (unless (ring-empty-p eshell-history-ring)
                   ...
                   t)

Same exact steps to reproduce, just don't add one entry to the history
ring - leave it empty.

The cause is that these lines are inside a big condition of a big
`when` - but that "condition" is complecting two things:

1. the actual predicate - should we add this input to the history ring? and
2. the history management side-effects that need to happen if that
predicate is true, before the history addition is made.

So the key observation is that this `(unless ...)` is part of the
surrounding `(and ...)` but is not actually there to influence the
condition! It's there to catch a case which requires a different
side-effect. But when the inner `(unless ...)` was added, the `t` got
accidentally/wrongly scooped into the `(unless ...)` along with the
side-effect.

I think the ideal fix here is a refactor that makes the big picture
clearer (I can provide one if asked, but that would almost certainly
have enough creative substance to require copyright assignment, and
would need to wait on the paperwork). But a good-enough, minimally
disruptive fix that is too mechanical and small to be copyrightable is
just to change the `(unless ... t)` to a `(progn (unless ...) t)`:

                 (progn
                   (unless (ring-empty-p eshell-history-ring)
                   ...)
                 t)

bug#63362: Bug+fix for eshell-hist-ignore-dups 'erase

[Jim Porter]

On 5/7/2023 11:38 AM, Alexander Kozhevnikov wrote:
> I think the ideal fix here is a refactor that makes the big picture
> clearer (I can provide one if asked, but that would almost certainly
> have enough creative substance to require copyright assignment, and
> would need to wait on the paperwork).

Thanks for this analysis. I'll try and take a look at this in more 
detail, and perhaps refactor things a bit in this part of the code as 
you suggest. (I've wanted to clean up the Eshell history logic a bit 
already, since I'd like to make it a bit more usable when you have 
multiple Eshell buffers.)

First step though is for me to write up a few regression tests, I think. 
That should make it easier to do some refactoring without breaking anything.

bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase

[Jim Porter]

Version: 30.1

On 5/7/2023 11:38 AM, Alexander Kozhevnikov wrote:
> I think the ideal fix here is a refactor that makes the big picture
> clearer (I can provide one if asked, but that would almost certainly
> have enough creative substance to require copyright assignment, and
> would need to wait on the paperwork).

Thanks for the analysis (and sorry about the long delay in following up 
on this!). I think you're right that this function needs a refactor, so 
I've now done so. I've also added regression tests for all three 
settings of 'eshell-hist-ignoredups', so hopefully this won't ever break 
in the future.

I've merged a fix for this to master as 7b0f24ab1f9, so closing this bug 
now.