GNU bug report logs - #31211
27.0.50; Pruning of command-history in command-execute is off by one

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31211 in the body.
You can then email your comments to 31211 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Pruning of command-history in command-execute is off by one
Date: Wed, 18 Apr 2018 19:36:36 +0100

Evaluating the following:

(let ((history-length 1))
  (dotimes (_ 2)
    (command-execute #'ignore t))
  (length command-history))

gives 2, instead of the expected 1.

Patch addressing this to follow.

-- 
Basil

In GNU Emacs 27.0.50 (build 6, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2018-04-18 built on thunk
Repository revision: 5dff4905d73d0d42447ff4b114d1af726a689c6a
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux buster/sid

Message #8 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Wed, 18 Apr 2018 19:43:37 +0100

[Message part 1 (text/plain, inline)]

tags 31211 patch
quit

[0001-Improve-simple.el-history-and-ring-pruning.patch (text/x-diff, attachment)]

[0002-Minor-simple.el-simplifications-bug-31211.patch (text/x-diff, attachment)]

[Message part 4 (text/plain, inline)]

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Patch addressing this to follow.

I attach two patches.

The first likens the history and ring pruning in command-execute,
kill-new, and push-mark to that in add-to-history, thus also fixing the
off-by-one error in command-execute.

The second suggests some minor refactors/simplifications in surrounding
functions.

Thanks,

-- 
Basil

Message #13 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Wed, 18 Apr 2018 19:52:17 +0100

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> The second suggests some minor refactors/simplifications in surrounding
> functions.

P.S. I see that, with kill-append-merge-undo enabled, kill-append
     removes the next undo boundary it finds in buffer-undo-list.
     This search for the next undo boundary, however, starts with the
     second element of buffer-undo-list.  Is it reasonable to expect
     that the first element of buffer-undo-list is never nil here?

-- 
Basil

Message #16 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Fri, 20 Apr 2018 08:53:23 -0400

severity 31211 minor
quit

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Subject: [PATCH 1/2] Improve simple.el history and ring pruning
>
> * lisp/simple.el (command-execute):
> Fix off-by-one error in command-history pruning. (bug#31211)
> (kill-new, push-mark): Prune kill-ring, mark-ring and
> global-mark-ring in a single pass, as add-to-history does.

You need to change call-interactively in callint.c in order to fix the
test case from your OP though, right?  This part:

      /* Don't keep command history around forever.  */
      if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
        {
          Lisp_Object teml = Fnthcdr (Vhistory_length, Vcommand_history);
          if (CONSP (teml))
            XSETCDR (teml, Qnil);
        }

Message #21 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Sun, 29 Apr 2018 20:54:56 +0100

[0001-Fix-off-by-one-history-pruning-bug-31211.patch (text/x-diff, attachment)]

[0002-Minor-simple.el-simplifications.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

Noam Postavsky <npostavs <at> gmail.com> writes:

> You need to change call-interactively in callint.c in order to fix the
> test case from your OP though, right?  This part:
>
>       /* Don't keep command history around forever.  */
>       if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
>         {
>           Lisp_Object teml = Fnthcdr (Vhistory_length, Vcommand_history);
>           if (CONSP (teml))
>             XSETCDR (teml, Qnil);
>         }

Whoops, right you are; I jumped the gun on that one.

Is there any reason why we can't use add-to-history in places like
Fcall_interactively in src/callint.c and read_minibuf in src/minibuf.c,
rather than duplicating its logic and falling into off-by-one traps?

I attach a patch which delegates to add-to-history in various such
places, on the assumption this is kosher.  Please let me know whether
something like this would be acceptable and/or how it can made so. 

The second attachment comprises the same minor lisp/simple.el touch-ups
as in my last email.

Thanks,

-- 
Basil

Message #24 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Sun, 29 Apr 2018 18:43:02 -0400

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Is there any reason why we can't use add-to-history in places like
> Fcall_interactively in src/callint.c and read_minibuf in src/minibuf.c,
> rather than duplicating its logic and falling into off-by-one traps?

Sometimes there can be bootstrapping problems (e.g., the C code tries to
call Lisp code that hasn't been loaded yet).  In this case, I don't
think call-interactively should be needed during bootstrap, so it's
probably fine.

> I attach a patch which delegates to add-to-history in various such
> places, on the assumption this is kosher.  Please let me know whether
> something like this would be acceptable and/or how it can made so. 
>
> The second attachment comprises the same minor lisp/simple.el touch-ups
> as in my last email.

Thanks, I'll push to master in a few days assuming there are no
objections.

Message #27 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Mon, 30 Apr 2018 01:51:41 +0100

[0001-Fix-off-by-one-history-pruning-bug-31211.patch (text/x-diff, attachment)]

[0002-Minor-simple.el-simplifications.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

Noam Postavsky <npostavs <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> The second attachment comprises the same minor lisp/simple.el touch-ups
>> as in my last email.
>
> Thanks, I'll push to master in a few days assuming there are no
> objections.

Thanks.  AFAICT the call to marker-position added in the second patch is
actually redundant, so I'm reattaching the patches with that call
removed.  Sorry about the nuisance.

-- 
Basil

Message #30 received at 31211 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: 31211 <at> debbugs.gnu.org
Subject: Re: bug#31211: 27.0.50;
 Pruning of command-history in command-execute is off by one
Date: Wed, 02 May 2018 20:37:58 -0400

tags 31211 fixed
close 31211 27.1
quit

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Thanks.  AFAICT the call to marker-position added in the second patch is
> actually redundant, so I'm reattaching the patches with that call
> removed.  Sorry about the nuisance.

No worries, this kind of thing is exactly why I like to wait a bit
before pushing.  I've now pushed to master.

[1: f2c74543ed]: 2018-05-02 20:18:07 -0400
  Fix off-by-one history pruning (bug#31211)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f2c74543edc7e8d07655b459ba8898eec9b6d4e8

[2: 74ff5ade80]: 2018-05-02 20:20:25 -0400
  Minor simple.el simplifications (Bug#31211)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=74ff5ade8002a1a2cc8956607310e5466f2ed596

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.