Subject: bug#63362: closed (Re: bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 24 Aug 2023 01:32:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
63360: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63360
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase
From: Jim Porter <[email-masked]>
To: Alexander Kozhevnikov <[email-masked]>, [email-masked]
Date: Wed, 23 Aug 2023 18:31:00 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

Subject: Re: Bug+fix for eshell-hist-ignore-dups 'erase
From: Alexander Kozhevnikov <[email-masked]>
To: [email-masked]
Date: Sun, 7 May 2023 18:38:42 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

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)