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

[GNU bug Tracking System]

Your message dated Wed, 23 Aug 2023 18:31:00 -0700
with message-id <d898418a-0cb7-0e7d-8c69-022885451c0b@gmail.com>
and subject line Re: bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase
has caused the debbugs.gnu.org bug report #63360,
regarding Bug+fix for eshell-hist-ignore-dups 'erase
to be marked as done.

(If you believe you have received this mail in error, please contact
help-debbugs@gnu.org.)


-- 
63360: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63360
GNU Bug Tracking System
Contact help-debbugs@gnu.org with problems
> --- Begin Message ---
>
>
>
> Subject: 
>
> Re: Bug+fix for eshell-hist-ignore-dups 'erase
>
>
>
>
> Date: 
>
> Sun, 7 May 2023 18:38:42 +0000
>
>
>
>
> 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)
>
>
>
> --- End Message ---
> --- Begin Message ---
>
>
>
> Subject: 
>
> Re: bug#63360: Bug+fix for eshell-hist-ignore-dups 'erase
>
>
>
>
> Date: 
>
> Wed, 23 Aug 2023 18:31:00 -0700
>
>
>
>
> 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.
>
>
> --- End Message ---