bug#34405: 26.1; atomic change group after undo fails to cancel

bug#34405: 26.1; atomic change group after undo fails to cancel

[Braun Gábor]

Hi,

-------------------- File test.el ----------------------------------
;; -*- lexical-binding: t; -*-
(defun test-fun ()
  "Test atomic change group, no visible effect."
  (interactive)
  (catch 'test
    (atomic-change-group
      (save-excursion
        (goto-char (point-min))
        (insert "!!! TEST: you shouldn't see this !!!")
        (throw 'test t)))))

(global-set-key [(control c) ?a] #'test-fun)
--------------------------------------------------------------------

Start emacs by the command

emacs -Q -l test.el

Press the following keys: a C-_ C-c a

A "!!! TEST: you shouldn't see this !!!" gets inserted at the top of
buffer *scratch*, and the message "Undoing to some unrelated state"
appears in the echo area.  I expect that "C-c a" has no visible effect
(as is the case if one omits "a C-_" before "C-c a").


A variant: in function cancel-change-group change line
"(unless (eq last-command 'undo) (undo-start))"
into "(undo-start)".
I.e. to have a self-containd test:

------------------- file test2.el ----------------------------------
;; -*- lexical-binding: t; -*-
(defun test-fun ()
  "Test atomic change group, no visible effect."
  (interactive)
  (catch 'test
    (atomic-change-group
      (save-excursion
        (goto-char (point-min))
        (insert "!!! TEST: you shouldn't see this !!!")
        (throw 'test t)))))

(global-set-key [(control c) ?a] #'test-fun)

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

Run emacs as

emacs -Q -l test2.el

Press the following keys (same as above): a C-_ C-c a

Now the displayed buffer *scratch* is in its original form, no message
in the echo area as expected.

Discussion:

I do not claim that the change above in cancel-change-group is a correct
fix, especially no claim that pending-undo-list will not get corrupted.
It is intended only as a proof that in its original definition
cancel-change-group fails while it was still possible to properly cancel
the change group.

Best wishes,

     Gábor

System information (for the first test, not the second one):

In GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.2)
 of 2018-12-26, modified by Debian built on x86-ubc-01
Windowing system distributor 'The X.Org Foundation', version 
11.0.12003000
System Description:	Debian GNU/Linux buster/sid

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Undo!
cancel-change-group: Undoing to some unrelated state

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --build
 x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --enable-libsystemd
 --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-3ThesY/emacs-26.1+1=. -fstack-
protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: hu_HU.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 95434 9148)
 (symbols 48 20367 1)
 (miscs 40 44 118)
 (strings 32 28319 1131)
 (string-bytes 1 742545)
 (vectors 16 14644)
 (vector-slots 8 496930 10762)
 (floats 8 49 118)
 (intervals 56 263 0)
 (buffers 992 11))

bug#34405: 26.1; patch for cancel-change-group to work after undo

[Braun Gábor]

Hi,

Please consider the following fix to the problem, patching function 
cancel-change-group.

In the original implementation, the only form depending on whether the 
last command was an undo is the line

(unless (eq last-command 'undo) (undo-start))

which sets pending-undo-list to buffer-undo-list, when the last command 
was not an undo.  When it was, then pending-undo-list probably contains 
the pending undo entries from a buffer state before the start of the 
change group, which are obviously irrelevant for cancelling the change 
group.

The fix below is to use buffer-undo-list instead of pending-undo-list,
essentially inlining the call to undo-more for this change.  
As a side effect, the value of pending-undo-list will no longer be 
changed by cancel-change-group (unless something in the undo log changes 
it), but as far as I see, it doesn't matter, as its value is useful only 
directly after an undo command.

--- lisp/subr.el
+++ lisp/subr.el
@@ -2669,14 +2669,17 @@
               (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"))
+                  (setcar elt nil) (setcdr elt nil)
+                  ;; Make sure there's no confusion.
+                  (unless (eq elt (last buffer-undo-list))
+                    (error "Undoing to some unrelated state")))
                 ;; Undo it all.
                 (save-excursion
-                  (while (listp pending-undo-list) (undo-more 1)))
+                  (let ((undo-in-progress t))
+                    (while buffer-undo-list
+                      ;; Undo one step, removing it from undo log.
+                      (setq buffer-undo-list
+                            (primitive-undo 1 buffer-undo-list)))))
                 ;; Revert the undo info to what it was when we grabbed
                 ;; the state.
                 (setq buffer-undo-list elt))

Best wishes,

	Gábor

bug#34405: 26.1; patch for cancel-change-group to work after undo

[Eli Zaretskii]

> From: Braun Gábor <braungb88@gmail.com>
> Date: Tue, 12 Nov 2019 18:05:48 +0100
> 
> Please consider the following fix to the problem, patching function 
> cancel-change-group.

Thanks.

If this problem could be solved locally in the atomic groups code, I'd
prefer that.  If not, then let's postpone changes in the low-level
undo machinery until after the emacs-27 branch is cut, as I'd like to
avoid adding more changes in such deep innards.

bug#34405: 26.1; patch for cancel-change-group to work after undo

[Braun Gábor]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

>
> > Please consider the following fix to the problem, patching function
> > cancel-change-group.
>
> Thanks.
>
> If this problem could be solved locally in the atomic groups code, I'd
> prefer that.


The patch is local to the atomic groups code: it only changes
the function cancel-change-group, whose purpose is to abort an atomic group.

  If not, then let's postpone changes in the low-level
> undo machinery


There is no intention to change there anything.

 I'd like to
> avoid adding more changes in such deep innards.
>

That is perfectly understandable.

>

[-- Attachment #2: Type: text/html, Size: 1502 bytes --]

bug#34405: 26.1; 2-line patch

[Braun Gábor]

Hi,

Please find below a small patch to the problem, which works for me in 
Emacs 26.1.
Instead of calling undo-start, it binds pending-undo-list so that its 
value is restored after canceling an atomic change group.  Presumably this 
restoration is what the original code meant to do.

(Feel free to reorder the bindings in let if you don't like
pending-undo-list between old-car and old-cdr.)

Best wishes,

	Gábor

--- lisp/subr.el
+++ lisp/subr.el
@@ -2664,13 +2664,13 @@ cancel-change-group
 	;; the body of `atomic-change-group' all changes can be undone.
 	(widen)
 	(let ((old-car (car-safe elt))
+	      (pending-undo-list buffer-undo-list)
 	      (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"))

bug#34405: bug#26061: 26.0.50; cancel-change-group fails with "unrelated state" error if used after an undo

[Lars Ingebrigtsen]

Braun Gábor <braungb88@gmail.com> writes:

> -------------------- File test.el ----------------------------------
> ;; -*- lexical-binding: t; -*-
> (defun test-fun ()
>   "Test atomic change group, no visible effect."
>   (interactive)
>   (catch 'test
>     (atomic-change-group
>       (save-excursion
>         (goto-char (point-min))
>         (insert "!!! TEST: you shouldn't see this !!!")
>         (throw 'test t)))))
>
> (global-set-key [(control c) ?a] #'test-fun)
> --------------------------------------------------------------------
>
> Start emacs by the command
>
> emacs -Q -l test.el
>
> Press the following keys: a C-_ C-c a
>
> A "!!! TEST: you shouldn't see this !!!" gets inserted at the top of
> buffer *scratch*, and the message "Undoing to some unrelated state"
> appears in the echo area.

I can reproduce this problem in Emacs 26.1, but it seems to be gone in
Emacs 27.1 (and 28), so I'm going to go ahead and guess that this has
been fixed in the years since this was reported, and I'm closing this
bug report.  If this is still a problem in recent Emacs versions, please
respond to the debbugs address and we'll reopen.

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

bug#34405: bug#26061: 26.0.50; cancel-change-group fails with "unrelated state" error if used after an undo

[Braun Gábor]

> I can reproduce this problem in Emacs 26.1, but it seems to be gone in
> Emacs 27.1 (and 28), so I'm going to go ahead and guess that this has
> been fixed

I can see the fix in the following snippet of `cancel-change-group' in 
Emacs 27.1 code.  The faulty logic with `last-command' and `undo-start' 
has been gone.  (... denotes omitted code.)

    (let (...
	      ;; Use `pending-undo-list' temporarily since `undo-more' needs
	      ;; it, but restore it afterwards so as not to mess with an
	      ;; ongoing sequence of `undo's.
	      (pending-undo-list
	       ;; Use `buffer-undo-list' unconditionally (bug#39680).
	       buffer-undo-list))

Thank you for checking in recent Emacs versions.

Best wishes,

	Gábor