bug#39680: 27.0.60; electric-pair-mode broken by undo

[Alan Mackenzie <acm@muc.de>]

Hello, Kévin and Stefan.

On Wed, Feb 19, 2020 at 19:34:37 +0100, Kévin Le Gouguec wrote:
> Hello,

> Commit e66d5a1c45 (2019-02-19T00:00:44Z!monnier@iro.umontreal.ca) might
> have introduced a bug.  From emacs -Q:

> 1. C-x b foo RET
> 2. M-x electric-pair-mode RET
> 3. (
>     - A closing parenthesis has been inserted.
> 4. C-b C-f
>     - This is to break undo grouping.
> 5. a
> 6. C-_
> 7. (

> In Emacs 26.3, buffer foo contains "(())" and point is after the
> innermost opening bracket.

> In Emacs 27, buffer foo contains "()" and point is after the closing
> bracket.  The *Messages* buffer shows:

> > cancel-change-group: Undoing to some unrelated state

The cause of this bug is a bug in cancel-change-group, part of the
atomic-change-group group of functions.  The commit you (Kévin) refer to
above substitutes atomic-change-group for a simple insertion and
deletion of a character (for a good reason).

cancel-change-group looks like this:

(defun cancel-change-group (handle)
  "Finish a change group made with `prepare-change-group' (which see).
This finishes the change group by reverting all of its changes."
  (dolist (elt handle)
    (with-current-buffer (car elt)
      (setq elt (cdr elt))
      (save-restriction
        ;; Widen buffer temporarily so if the buffer was narrowed within
        ;; the body of `atomic-change-group' all changes can be undone.
        (widen)
        (let ((old-car (car-safe elt))
              (old-cdr (cdr-safe elt)))
          (unwind-protect
              (progn
                ;; Temporarily truncate the undo log at ELT.
                (when (consp elt)
                  (setcar elt nil) (setcdr elt nil))
                (unless (eq last-command 'undo) (undo-start))   <=======
                ;; Make sure there's no confusion.
============>   (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                  (error "Undoing to some unrelated state"))
                ;; Undo it all.
                (save-excursion
                  (while (listp pending-undo-list) (undo-more 1)))
                ;; Revert the undo info to what it was when we grabbed
                ;; the state.
                (setq buffer-undo-list elt))
            ;; Reset the modified cons cell ELT to its original content.
            (when (consp elt)
              (setcar elt old-car)
              (setcdr elt old-cdr))))))))

On entry to this function, HANDLE has the value:

    ((#<buffer asdf> (2 . 3) nil ("a" . 2) (#<marker at 2 in asdf> . -1)
      nil (2 . 3)))

.  At the first indicated spot above, last-command is indeed 'undo, so
undo-start is not invoked.

Since the undo which undid "a" emptied pending-undo-list,
pending-undo-list has been set to t.

So when the eq is done in the second indicated spot, pending-undo-list
is not ELT (the first cons cell from (cdr handle)).  The function thus
spuriously signals the error "Undoing to some unrelated state".

#########################################################################

I admit I don't fully understand the mechanism of atomic-change-group,
but I see the problem as the EQ comparison on the <======= line.  If
pending-undo-list has been replaced by t (after being exhausted by the
previous 'undo operation), there is no point EQing it with the cons from
HANDLE.

So, as a first approximation to a fix, I added a (consp
pending-undo-list) into this test, as follow:


diff --git a/lisp/subr.el b/lisp/subr.el
index b5ec0de156..8b7d9b5451 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2975,7 +2975,9 @@ cancel-change-group
                 ;; Temporarily truncate the undo log at ELT.
                 (when (consp elt)
                   (setcar elt nil) (setcdr elt nil))
-                (unless (eq last-command 'undo) (undo-start))
+                (unless (and (eq last-command 'undo)
+                             (consp pending-undo-list))
+                  (undo-start))
                 ;; Make sure there's no confusion.
                 (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                   (error "Undoing to some unrelated state"))


Stefan, what is your view on this attempted patch?  Is it sound?

[ .... ]

> Thank you for your time.

Thank you for a good bug report, conveniently reduced to a minimum test
case.

> In GNU Emacs 28.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
>  of 2020-02-19 built on my-little-tumbleweed
> Repository revision: e1e1bd8f85c53fea9f61b6ec99b461ddd93461b9
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
> System Description: openSUSE Tumbleweed

> Configured using:
>  'configure --with-xwidgets --with-cairo'

-- 
Alan Mackenzie (Nuremberg, Germany).