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

[Stefan Monnier]

> The cause of this bug is a bug in cancel-change-group,

Indeed.
Thanks Alan for the nice analysis (and sorry I didn't get to it earlier).

>                 (unless (eq last-command 'undo) (undo-start))   <=======
>                 ;; Make sure there's no confusion.
> ============>   (when (and (consp elt) (not (eq elt (last 
> pending-undo-list))))

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

I think there's a bug right here: the fact that the previous command was
`undo` shouldn't really matter.  Worse, we've made changes to the buffer
since that last `undo` so it's plain wrong to pass that old
`pending-undo-list` to `undo-more`.

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

I think we need something like the patch below (not really tested yet).
WDYT?

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

Indeed.  This is pretty delicate code, so a concise and easy to reproduce
test case is very welcome.



        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 13515ca7da..ebc8e320dc 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2972,13 +2972,14 @@ cancel-change-group
        ;; the body of `atomic-change-group' all changes can be undone.
        (widen)
        (let ((old-car (car-safe elt))
-             (old-cdr (cdr-safe elt)))
+             (old-cdr (cdr-safe elt))
+             (start-pul pending-undo-list))
           (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))
+                (setq pending-undo-list buffer-undo-list)
                 ;; Make sure there's no confusion.
                 (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                   (error "Undoing to some unrelated state"))
@@ -2991,7 +2992,13 @@ cancel-change-group
             ;; Reset the modified cons cell ELT to its original content.
             (when (consp elt)
               (setcar elt old-car)
-              (setcdr elt old-cdr))))))))
+              (setcdr elt old-cdr)))
+          ;; Let's not break a sequence of undos just because we
+          ;; tried to make a change and then undid it: preserve
+          ;; the original `pending-undo-list' if it's still valid.
+          (if (eq (undo--last-change-was-undo-p buffer-undo-list)
+                  start-pul)
+              (setq pending-undo-list start-pul)))))))
 
 ;;;; Display-related functions.