GNU bug report logs - #33341
27.0.50; Undo log merging and change groups

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 33341 in the body.
You can then email your comments to 33341 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: Michael Heerdegen <michael_heerdegen <at> web.de>
To: bug-gnu-emacs <at> gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: 27.0.50; Undo log merging and change groups
Date: Sun, 11 Nov 2018 08:50:43 +0100

Hello,

I have been playing with the undo change group functions.  I found
`cancel-change-group' does not always work as expected.  For example, if
you define

(defun my-test-change-groups ()
  (interactive)
  (insert "0\n")
  (let ((g (prepare-change-group)))
    (activate-change-group g)
    (insert "b\n")
    (insert "c\n")
    (cancel-change-group g)))

and call that command in some random buffer, the final
`cancel-change-group' has no effect (i.e. nothing is reverted).  In
other, similar examples, `cancel-change-group' seems to revert more than
it should.

To cite (CC'd) Stefan's remark in emacs-help, "the undo entries for
(insert "0\n"), (insert "b\n"), and (insert "c\n") are merged into a
single entry in the undo log (as a form of optimization).  The
change-group code should prevent such a merge, e.g. by adding some dummy
undo element which will work like a "fence"".


Thanks,

Michael.




In GNU Emacs 27.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version 3.24.1)
 of 2018-11-11 built on drachen
Repository revision: c1095b03a933d55fe1cd357881f1ca6e16e06362
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Debian GNU/Linux buster/sid

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Thu, 26 Nov 2020 13:34:40 +0100

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> I have been playing with the undo change group functions.  I found
> `cancel-change-group' does not always work as expected.  For example, if
> you define
>
> (defun my-test-change-groups ()
>   (interactive)
>   (insert "0\n")
>   (let ((g (prepare-change-group)))
>     (activate-change-group g)
>     (insert "b\n")
>     (insert "c\n")
>     (cancel-change-group g)))
>
> and call that command in some random buffer, the final
> `cancel-change-group' has no effect (i.e. nothing is reverted).  In
> other, similar examples, `cancel-change-group' seems to revert more than
> it should.

Hm...  Well, after running this command, hitting "undo" removes all the
three lines, which is what I'd expect?  Since you cancelled the change
group?

Or...  Hm.  Well, calling accept-change-group has exactly the same
effect, so it seems like I'm misunderstanding how this is supposed to
work.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Thu, 26 Nov 2020 15:16:59 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Hm...  Well, after running this command, hitting "undo" removes all the
> three lines, which is what I'd expect?  Since you cancelled the change
> group?

AFAIU `cancel-change-group' itself should undo the change set, without
the user invoking `undo'.  See the implementation of
`atomic-change-group' for a use of this feature.


Michael.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Thu, 26 Nov 2020 15:49:29 +0100

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
> > Hm...  Well, after running this command, hitting "undo" removes all the
> > three lines, which is what I'd expect?  Since you cancelled the change
> > group?
>
> AFAIU `cancel-change-group' itself should undo the change set, without
> the user invoking `undo'.  See the implementation of
> `atomic-change-group' for a use of this feature.

BTW, coming back to my example:

#+begin_src emacs-lisp
(defun my-test-change-groups ()
  (interactive)
  (insert "0\n") ;; try to comment this line
  (let ((g (prepare-change-group)))
    (activate-change-group g)
    (insert "b\n")
    (insert "c\n")
    (cancel-change-group g)))
#+end_src

if you comment the line including the `insert' call before the change
group is prepared the thing works as expected.  So there is a problem
with this insertion (in the same command?) before preparing the group,
or this is actually not allowed but not documented accordingly.

Michael.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Thu, 26 Nov 2020 21:26:03 +0100

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

#+begin_src emacs-lisp
(defun my-test-change-groups ()
  (interactive)
  (insert "0\n")
  (let ((g (prepare-change-group)))
    (activate-change-group g)
    (insert "b\n")
    (insert "c\n")
    (cancel-change-group g)))
#+end_src

Adding an explicit `undo-boundary' call before preparing the change
group fixes the problem for me.  Should `prepare-change-group' do
something like that implicitly?

Michael.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Thu, 26 Nov 2020 15:55:42 -0500

> Adding an explicit `undo-boundary' call before preparing the change
> group fixes the problem for me.  Should `prepare-change-group' do
> something like that implicitly?

Not if it should do it, but at least it should work correctly if it's
not done already.
Could you check why it breaks in the absence of an undo-boundary?


        Stefan

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Fri, 27 Nov 2020 02:11:23 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> Could you check why it breaks in the absence of an undo-boundary?

I have edebugged `org-entry-get'.  Short version: it is as it seems.
Here is the example again:

#+begin_src emacs-lisp
(defun my-test-change-groups ()
  (interactive)
  (insert "0\n")
  ;; (undo-boundary)
  (let ((g (prepare-change-group)))
    (activate-change-group g)
    (insert "b\n")
    (insert "c\n")
    (cancel-change-group g)))
#+end_src

Without the boundary, all changes are combined into one (1 . 10) undo
entry.  And then these code lines

#+begin_src emacs-lisp
;; Temporarily truncate the undo log at ELT.
(when (consp elt)
  (setcar elt nil) (setcdr elt nil))
#+end_src

cause that were the undo should happen:

#+begin_src emacs-lisp
;; Undo it all.
(save-excursion
  (while (listp pending-undo-list) (undo-more 1)))
#+end_src

nothing is undone.

With the boundary, we have two undo entries, and the truncated log still
contains the entry for the first `insert' call.

Regards,

Michael.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 33341 <at> debbugs.gnu.org
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Fri, 27 Nov 2020 09:44:27 -0500

> (defun my-test-change-groups ()
>   (interactive)
>   (insert "0\n")
>   ;; (undo-boundary)
>   (let ((g (prepare-change-group)))
>     (activate-change-group g)
>     (insert "b\n")
>     (insert "c\n")
>     (cancel-change-group g)))

I see it's a bug indeed: the problem is that `insert` optimizes the undo
entries by reusing a previous undo entry is that was
a consecutive insertion, so when you get to `cancel-change-group` the
`buffer-undo-list` only has a single entry that represent the
combination of all 3 insertions.

I just pushed the patch below which should fix it.


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index e009dcc2b9..1cf3a49fe4 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3036,7 +3036,10 @@ activate-change-group
   (dolist (elt handle)
     (with-current-buffer (car elt)
       (if (eq buffer-undo-list t)
-	  (setq buffer-undo-list nil)))))
+	  (setq buffer-undo-list nil)
+	;; Add a boundary to make sure the upcoming changes won't be
+	;; merged with any previous changes (bug#33341).
+	(undo-boundary)))))
 
 (defun accept-change-group (handle)
   "Finish a change group made with `prepare-change-group' (which see).
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 67f7fc9749..e3f798d11c 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -551,5 +551,17 @@ subr-replace-regexp-in-string
   (should (equal (replace-regexp-in-string "\\`\\|x" "z" "--xx--")
                  "z--zz--")))
 
+(ert-deftest subr-tests--change-group-33341 ()
+  (with-temp-buffer
+    (buffer-enable-undo)
+    (insert "0\n")
+    ;; (undo-boundary)
+    (let ((g (prepare-change-group)))
+      (activate-change-group g)
+      (insert "b\n")
+      (insert "c\n")
+      (cancel-change-group g))
+    (should (equal (buffer-string) "0\n"))))
+
 (provide 'subr-tests)
 ;;; subr-tests.el ends here

Message #31 received at 33341-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 33341-done <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#33341: 27.0.50; Undo log merging and change groups
Date: Fri, 27 Nov 2020 18:43:09 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> diff --git a/lisp/subr.el b/lisp/subr.el
> index e009dcc2b9..1cf3a49fe4 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> [...]
> +	(undo-boundary)))))

Confirmed that it's fixed with that commit.

Thanks!

Michael.

Previous Next

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