bug#33341: 27.0.50; Undo log merging and change groups

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

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

bug#33341: 27.0.50; Undo log merging and change groups

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@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

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@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.

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Lars Ingebrigtsen <larsi@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.

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@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.

bug#33341: 27.0.50; Undo log merging and change groups

[Stefan Monnier]

> 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

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

Stefan Monnier <monnier@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.

bug#33341: 27.0.50; Undo log merging and change groups

[Stefan Monnier]

> (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

bug#33341: 27.0.50; Undo log merging and change groups

[Michael Heerdegen]

Stefan Monnier <monnier@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.