bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Ihor Radchenko]

Hi,

Original report:
https://orgmode.org/list/25520.20685.583180.776610@gargle.gargle.HOWL

I am not sure if it is Emacs bug, but I have difficulty understanding
the origin of the following error:

1. Create /tmp/bug.org with the following contents

* h1
** h2

2. emacs -Q /tmp/bug.org
3. Move point to h2
4. M-x org-promote-subtree
5. M-x undo
6. Observe "Changes to be undone by function different from announced" error

The functions in question are below and I fail to see anything that may
be wrong there.

(defun org-promote-subtree ()
  "Promote the entire subtree.
See also `org-promote'."
  (interactive)
  (save-excursion
    (org-back-to-heading t)
    (combine-change-calls (point) (save-excursion (org-end-of-subtree t))
      (org-with-limited-levels (org-map-tree 'org-promote))))
  (org-fix-position-after-promote))

(defun org-promote ()
  "Promote the current heading higher up the tree."
  (org-with-wide-buffer
   (org-back-to-heading t)
   (let* ((after-change-functions (remq 'flyspell-after-change-function
					after-change-functions))
	  (level (save-match-data (funcall outline-level)))
	  (up-head (concat (make-string (org-get-valid-level level -1) ?*) " "))
	  (diff (abs (- level (length up-head) -1))))
     (cond
      ((and (= level 1) org-allow-promoting-top-level-subtree)
       (replace-match "# " nil t))
      ((= level 1)
       (user-error "Cannot promote to level 0.  UNDO to recover if necessary"))
      (t (replace-match (apply #'propertize up-head (text-properties-at (match-beginning 0))) t)))
     (unless (= level 1)
       (when org-auto-align-tags (org-align-tags))
       (when org-adapt-indentation (org-fixup-indentation (- diff))))
     (run-hooks 'org-after-promote-entry-hook))))  

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2022-12-26 built on localhost
Repository revision: cc29fab3a66c59e77d0ff67c0f3e2e34ec80a03c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Gentoo Linux

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Sun, 01 Jan 2023 13:40:08 +0000
> 
> 1. Create /tmp/bug.org with the following contents
> 
> * h1
> ** h2
> 
> 2. emacs -Q /tmp/bug.org
> 3. Move point to h2
> 4. M-x org-promote-subtree
> 5. M-x undo
> 6. Observe "Changes to be undone by function different from announced" error

The error message text is confusing, what it wants to say is this:

  Changes undone by a function are different from the announced ones

The problem is here:

                   (apply fun args) ;; Use `save-current-buffer'?
                   ;; Check that the function did what the entry
                   ;; said it would do.
                   (unless (and (= start start-mark)
                                (= (+ delta end) end-mark))  <<<<<<<<<<<<<<<
                     (error "Changes to be undone by function different from announced"))

After re-inserting the deleted string "** " with its face information,
'end-marker' is shifted by 3 position, and is now at buffer position
13, whereas 'end' is still 10 and 'delta' is 1, so their sum is 11.

Also note that if you begin from an empty Org buffer and insert the
initial text "* h1\n** h2\n", and then run the recipe, the problem
doesn't happen, because the "* " part is deleted before "apply fun"
returns, and then 'end-mark' is in its expected position 11.

Adding Stefan, in case how might have ideas for what is wrong here.

This problem exists also in Emacs 29.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> I am not sure if it is Emacs bug, but I have difficulty understanding 
> the origin of the following error:
>
> 1. Create /tmp/bug.org with the following contents
>
> * h1
> ** h2
>
> 2. emacs -Q /tmp/bug.org
> 3. Move point to h2
> 4. M-x org-promote-subtree
> 5. M-x undo
> 6. Observe "Changes to be undone by function different from announced" error
>

The culprit is 85e0a69567 (in the Org repository), and the bug is fixed 
by:

diff --git a/lisp/org.el b/lisp/org.el
index a98a7f5a0..becb302e1 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6554,8 +6554,7 @@ See also `org-promote'."
    (interactive)
    (save-excursion
      (org-back-to-heading t)
-    (combine-change-calls (point) (save-excursion (org-end-of-subtree t))
-      (org-with-limited-levels (org-map-tree 'org-promote))))
+    (org-with-limited-levels (org-map-tree 'org-promote)))
    (org-fix-position-after-promote))

  (defun org-demote-subtree ()
@@ -6564,8 +6563,7 @@ See `org-demote' and `org-promote'."
    (interactive)
    (save-excursion
      (org-back-to-heading t)
-    (combine-change-calls (point) (save-excursion (org-end-of-subtree t))
-      (org-with-limited-levels (org-map-tree 'org-demote))))
+    (org-with-limited-levels (org-map-tree 'org-demote)))
    (org-fix-position-after-promote))

  (defun org-do-promote ()

Perhaps another better fix exists.  If it's an Emacs bug, which is 
possible, it's in combine-change-calls.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> If it's an Emacs bug, which is possible, it's in combine-change-calls.
>

A few more details on the bug.  It's still unclear to me whether the bug 
is in Org or in Emacs.

When the file is opened we start with buffer-undo-list = nil.

The (org-with-limited-levels (org-map-tree 'org-promote)) body in 
org-promote-subtree, called inside a combine-change-calls, adds four 
entries to buffer-undo-list:

("** " . 6) <timestamp> (9 . 11) <timestamp>

which means that three characters have been removed and two have been 
added in the buffer, which seems correct, although the (9 . 11) entry 
looks a bit suspicious.  Perhaps (6 . 8) would have been expected there? 
However, this change does not happen outside the region on which 
combine-change-calls is called, which is 6-11.

Upon returning from combine-change-calls, buffer-undo-list contains four 
elements, when a single element would have been expected, if I understand 
its docstring correctly:

(apply 1 6 10 #'undo--wrap-and-run-primitive-undo 6 10 (("** " . 6))) <timestamp> (9 . 11) <timestamp>

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Ihor Radchenko]

Gregory Heytings <gregory@heytings.org> writes:

> The (org-with-limited-levels (org-map-tree 'org-promote)) body in 
> org-promote-subtree, called inside a combine-change-calls, adds four 
> entries to buffer-undo-list:
>
> ("** " . 6) <timestamp> (9 . 11) <timestamp>
>
> which means that three characters have been removed and two have been 
> added in the buffer, which seems correct

The actual function doing the work is

(replace-match (apply #'propertize up-head (text-properties-at (match-beginning 0))) t)

The match is "** " and the replacement is "* ".

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


It turns out that the bug is indeed not in Org, but in Emacs.  The part of 
combine-change-call which creates the undo-list element was totally 
broken.  For example, with a buffer-undo-list like

<undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3>

ofter body has been evaluated, the buffer-undo-list after 
combine-change-call is

(apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) <timestamp>  <undo element 2> <timestamp> nil <undo element 3>

which is clearly wrong, because <undo element 1> and <undo element 2> 
should be considered as a single undo step.  IOW, after 
combine-change-call buffer-undo-list should be:

(apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1> <undo element 2>)) nil <undo element 3>

Patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-combine-change-call.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 2655 bytes --]

From 2261de4d20d006c5344615d49ade539541577f7e Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 3 Jan 2023 09:23:14 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite the part which
creates the undo-list element.  Fixes bug#60467.
---
 lisp/subr.el | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9087f9a404..25ca211225 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4934,31 +4934,24 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+        (when (and (not (eq buffer-undo-list t))
+                   (not (eq buffer-undo-list old-bul)))
+          (let ((ptr buffer-undo-list) body-undo-list)
+            (while (not (eq ptr old-bul))
+              (unless (and (consp (car ptr))
+                           (eq (caar ptr) t))
+                (push (car ptr) body-undo-list))
+              (setq ptr (cdr ptr)))
+            (setq body-undo-list (nreverse body-undo-list))
+            (push (list 'apply
+	                (- end end-marker)
+	                beg
+	                (marker-position end-marker)
+	                #'undo--wrap-and-run-primitive-undo
+	                beg (marker-position end-marker)
+                        body-undo-list)
+                  buffer-undo-list)
+            (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
-- 
2.39.0

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Cc: 60467@debbugs.gnu.org
> Date: Tue, 03 Jan 2023 09:41:52 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> It turns out that the bug is indeed not in Org, but in Emacs.  The part of 
> combine-change-call which creates the undo-list element was totally 
> broken.  For example, with a buffer-undo-list like
> 
> <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3>
> 
> ofter body has been evaluated, the buffer-undo-list after 
> combine-change-call is
> 
> (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) <timestamp>  <undo element 2> <timestamp> nil <undo element 3>
> 
> which is clearly wrong, because <undo element 1> and <undo element 2> 
> should be considered as a single undo step.  IOW, after 
> combine-change-call buffer-undo-list should be:
> 
> (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1> <undo element 2>)) nil <undo element 3>
> 
> Patch attached.

Hmm... scary, even if this is intended for the master branch.  Could
you at least keep the few comments which were in the original code,
and perhaps also add comments explaining all those tricky tests with
consp, equality to t and to old-bul, the significance of (car ptr) and
(caar ptr), etc.?  This would make the code more maintainer-friendly,
as the need to constantly consult the spec of the various undo-list
elements (and hope the spec is complete and accurate ;-) would be
avoided.

I'd prefer to fix this in Emacs 29 if feasible, but unless the changes
are much simpler and/or more clearly documented, so that it becomes
possible to assess their risks, I don't see how that could happen.

I presume that all the undo tests we have still pass?  If so, how
about adding a test for this bug?

Thanks.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


>> Patch attached.
>
> Hmm... scary, even if this is intended for the master branch.  Could you 
> at least keep the few comments which were in the original code, and 
> perhaps also add comments explaining all those tricky tests with consp, 
> equality to t and to old-bul, the significance of (car ptr) and (caar 
> ptr), etc.?  This would make the code more maintainer-friendly, as the 
> need to constantly consult the spec of the various undo-list elements 
> (and hope the spec is complete and accurate ;-) would be avoided.
>

Is the attached patch better in that respect?

>
> I'd prefer to fix this in Emacs 29 if feasible, but unless the changes 
> are much simpler and/or more clearly documented, so that it becomes 
> possible to assess their risks, I don't see how that could happen.
>

I don't see how the changes could be made simpler, sorry, but now they are 
documented.  Feel free to ask further questions if something is still 
unclear.

>
> I presume that all the undo tests we have still pass?
>

They do.

>
> If so, how about adding a test for this bug?
>

I'll see if I can do that, but it seems tricky.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-combine-change-call.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3207 bytes --]

From 8f65a74ec1b86b3f5f295972c99b0b461cc7bd28 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 3 Jan 2023 13:29:06 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite the part which
creates the undo-list element.  Fixes bug#60467.
---
 lisp/subr.el | 56 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9087f9a404..da105d3474 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4934,31 +4934,37 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+	;; If buffer-undo-list is neither t (in which case undo
+	;; information is not recorded) nor equal to buffer-undo-list
+	;; before body was evaluated (in which case evaluating body
+	;; did not add items to buffer-undo-list) ...
+	(when (and (not (eq buffer-undo-list t))
+		   (not (eq buffer-undo-list old-bul)))
+	  (let ((ptr buffer-undo-list) body-undo-list)
+	    ;; ... then loop over buffer-undo-list, until the head of
+	    ;; buffer-undo-list before body was evaluated is found ...
+	    (while (not (eq ptr old-bul))
+	      ;; ... and add the entries to body-undo-list, unless
+	      ;; they are of the form (t . <something>), which are
+	      ;; entries that record buffer modification timestamps.
+	      (unless (and (consp (car ptr))
+			   (eq (caar ptr) t))
+		(push (car ptr) body-undo-list))
+	      (setq ptr (cdr ptr)))
+	    (setq body-undo-list (nreverse body-undo-list))
+	    ;; Add an (apply ...) entry to buffer-undo-list, using
+	    ;; body-undo-list ...
+	    (push (list 'apply
+			(- end end-marker)
+			beg
+			(marker-position end-marker)
+			#'undo--wrap-and-run-primitive-undo
+			beg (marker-position end-marker)
+			body-undo-list)
+		  buffer-undo-list)
+	    ;; ... and set the cdr of buffer-undo-list to
+	    ;; buffer-undo-list before body was evaluated.
+	    (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
-- 
2.39.0

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Tue, 03 Jan 2023 13:44:01 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Stefan Monnier <monnier@iro.umontreal.ca>, 60467@debbugs.gnu.org, 
>     yantar92@posteo.net
> 
> > Hmm... scary, even if this is intended for the master branch.  Could you 
> > at least keep the few comments which were in the original code, and 
> > perhaps also add comments explaining all those tricky tests with consp, 
> > equality to t and to old-bul, the significance of (car ptr) and (caar 
> > ptr), etc.?  This would make the code more maintainer-friendly, as the 
> > need to constantly consult the spec of the various undo-list elements 
> > (and hope the spec is complete and accurate ;-) would be avoided.
> 
> Is the attached patch better in that respect?

To some extent, yes.  But there's too much of comments that just say
what the code does without explaining.  So let me ask some questions
instead:

> -			      ;; In case garbage collection has removed OLD-BUL.
> -			      (cdr ptr)

Why should we toss this precaution?

> -		  (unless (cdr ptr)
> -		    (message "combine-change-calls: buffer-undo-list broken"))

And this one?

> +	;; If buffer-undo-list is neither t (in which case undo
> +	;; information is not recorded) nor equal to buffer-undo-list
> +	;; before body was evaluated (in which case evaluating body
> +	;; did not add items to buffer-undo-list) ...
> +	(when (and (not (eq buffer-undo-list t))
> +		   (not (eq buffer-undo-list old-bul)))
> +	  (let ((ptr buffer-undo-list) body-undo-list)
> +	    ;; ... then loop over buffer-undo-list, until the head of
> +	    ;; buffer-undo-list before body was evaluated is found ...
> +	    (while (not (eq ptr old-bul))
> +	      ;; ... and add the entries to body-undo-list, unless
> +	      ;; they are of the form (t . <something>), which are
> +	      ;; entries that record buffer modification timestamps.
> +	      (unless (and (consp (car ptr))
> +			   (eq (caar ptr) t))
> +		(push (car ptr) body-undo-list))
> +	      (setq ptr (cdr ptr)))
> +	    (setq body-undo-list (nreverse body-undo-list))
> +	    ;; Add an (apply ...) entry to buffer-undo-list, using
> +	    ;; body-undo-list ...
> +	    (push (list 'apply
> +			(- end end-marker)
> +			beg
> +			(marker-position end-marker)
> +			#'undo--wrap-and-run-primitive-undo
> +			beg (marker-position end-marker)
> +			body-undo-list)
> +		  buffer-undo-list)
> +	    ;; ... and set the cdr of buffer-undo-list to
> +	    ;; buffer-undo-list before body was evaluated.
> +	    (setcdr buffer-undo-list old-bul)))

So basically you are saying that the current code stops too early and
doesn't collect all the undo entries that need to be combined, because
timestamp entries get in the way, is that right?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>> -			      ;; In case garbage collection has removed OLD-BUL.
>> -			      (cdr ptr)
>
> Why should we toss this precaution?
>

I don't even understand what this is supposed to do.  By definition, 
garbage collection cannot collect something that is still referred to.

>> -		  (unless (cdr ptr)
>> -		    (message "combine-change-calls: buffer-undo-list broken"))
>
> And this one?
>

This one is just plain wrong.  It assumes that buffer-undo-list is non-nil 
initially.

>> +	;; If buffer-undo-list is neither t (in which case undo
>> +	;; information is not recorded) nor equal to buffer-undo-list
>> +	;; before body was evaluated (in which case evaluating body
>> +	;; did not add items to buffer-undo-list) ...
>> +	(when (and (not (eq buffer-undo-list t))
>> +		   (not (eq buffer-undo-list old-bul)))
>> +	  (let ((ptr buffer-undo-list) body-undo-list)
>> +	    ;; ... then loop over buffer-undo-list, until the head of
>> +	    ;; buffer-undo-list before body was evaluated is found ...
>> +	    (while (not (eq ptr old-bul))
>> +	      ;; ... and add the entries to body-undo-list, unless
>> +	      ;; they are of the form (t . <something>), which are
>> +	      ;; entries that record buffer modification timestamps.
>> +	      (unless (and (consp (car ptr))
>> +			   (eq (caar ptr) t))
>> +		(push (car ptr) body-undo-list))
>> +	      (setq ptr (cdr ptr)))
>> +	    (setq body-undo-list (nreverse body-undo-list))
>> +	    ;; Add an (apply ...) entry to buffer-undo-list, using
>> +	    ;; body-undo-list ...
>> +	    (push (list 'apply
>> +			(- end end-marker)
>> +			beg
>> +			(marker-position end-marker)
>> +			#'undo--wrap-and-run-primitive-undo
>> +			beg (marker-position end-marker)
>> +			body-undo-list)
>> +		  buffer-undo-list)
>> +	    ;; ... and set the cdr of buffer-undo-list to
>> +	    ;; buffer-undo-list before body was evaluated.
>> +	    (setcdr buffer-undo-list old-bul)))
>
> So basically you are saying that the current code stops too early and 
> doesn't collect all the undo entries that need to be combined, because 
> timestamp entries get in the way, is that right?
>

Yes.  The current code apparently meant to skip these entries, but it does 
not work at all.  Replacing a broken code that does not work with a 
clean(er) code that does work seems the right thing.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> -			      ;; In case garbage collection has removed OLD-BUL.
>>> -			      (cdr ptr)
>> Why should we toss this precaution?
> I don't even understand what this is supposed to do.

Yet you happily threw it away :-(

> By definition, garbage collection cannot collect something that is
> still referred to.

`garbage-collect` does a bit more than collect garbage.
It also truncates undo lists among other things.

>>> -		  (unless (cdr ptr)
>>> -		    (message "combine-change-calls: buffer-undo-list broken"))
>> And this one?
> This one is just plain wrong.  It assumes that buffer-undo-list is
> non-nil initially.

AFAICT it warns when the GC's truncation has thrown stuff away (in which
case there's a good chance the `apply` element we're building is
incorrect).

But indeed, it probably misfires if `buffer-undo-list` is nil initially.

> Yes.  The current code apparently meant to skip these entries, but it does
> not work at all.  Replacing a broken code that does not work with
> a clean(er) code that does work seems the right thing.

FWIW, I don't see what's cleaner about the new code.
Don't get me wrong: it's not worse either.  The two look
pretty much equivalent to me (well, the layout and ordering is different
and your code builds a new list where the old code reuses the old list
elements, but these are details that don't make much difference to
neither the complexity nor the size of the code, from where I stand).

Also, I agree that it's arguably easier/cleaner to install your change
(which drops timestamp entries instead of exiting the loop at the first
such entry) into your version of the code than into the current one (see
patch below for my attempt to do that).

But I don't understand why we should drop timestamp entries at all, so
I'd first like to hear if Alan has an explanation for his choice to stop
at timestamp entries: he presumably went to the trouble to write that
extra code for a good reason.


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 5fb150994ec..d84cd33c3a9 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4971,12 +4971,13 @@ combine-change-calls-1
 		  (while (and (not (eq (cdr ptr) old-bul))
 			      ;; In case garbage collection has removed OLD-BUL.
 			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
+			      )
+		    (if (and (consp (cdr ptr))
+			       (consp (cadr ptr))
+			       (eq (caadr ptr) t))
+			;; Don't include timestamp entries.
+			(setcdr ptr (cddr ptr))
+		      (setq ptr (cdr ptr))))
 		  (unless (cdr ptr)
 		    (message "combine-change-calls: buffer-undo-list broken"))
 		  (setcdr ptr nil)

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>> I don't even understand what this is supposed to do.
>
> Yet you happily threw it away :-(
>

Because there are no such precautions elsewhere in the code, and the 
comment above ("In case garbage collection has removed OLD-BUL") does not 
explain what its purpose is.  A few lines below, old-bul is used without 
such a precaution.  Of course, if it has a purpose, it's okay to keep it.

>> By definition, garbage collection cannot collect something that is 
>> still referred to.
>
> `garbage-collect` does a bit more than collect garbage. It also 
> truncates undo lists among other things.
>

Aha.  So this means that garbage-collect could be called between (funcall 
body) and the (while ...) loop two lines below, and could have removed 
elements that were added to buffer-undo-list by (funcall body), right? 
If so, that's what the comment should have said.

>> This one is just plain wrong.  It assumes that buffer-undo-list is 
>> non-nil initially.
>
> AFAICT it warns when the GC's truncation has thrown stuff away (in which 
> case there's a good chance the `apply` element we're building is 
> incorrect).
>

Given what you write above, that's probably what it meant to do indeed. 
But it also warns when buffer-undo-list is initially nil, such as with the 
recipe of this bug report.  If adding such a warning is useful, we should 
check whether old-bul was initially non-nil.

>> Yes.  The current code apparently meant to skip these entries, but it 
>> does not work at all.  Replacing a broken code that does not work with 
>> a clean(er) code that does work seems the right thing.
>
> FWIW, I don't see what's cleaner about the new code.
>

It took me a half hour to figure out what the original code was doing with 
this 'ap-elt' to which 'buffer-undo-list' is added, and subsequenty 
modified by the loop below, which does not refer to buffer-undo-list, and 
which also updates 'old-bul' in an unclear way.  I'm biased of course, but 
I think I would have understood the code in my patch much easier: it does 
one thing (adding the new undo element) after the other (building the list 
included in that undo element).

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


Updated patch attached, incorporating your comments.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-combine-change-call.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3563 bytes --]

From 1422d8ec3100f49d69fefea488ccda782f983eec Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 3 Jan 2023 16:48:58 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite the part which
creates the undo-list element.  Fixes bug#60467.
---
 lisp/subr.el | 64 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9087f9a404..3c0268a49d 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4934,31 +4934,45 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+	;; If buffer-undo-list is neither t (in which case undo
+	;; information is not recorded) nor equal to buffer-undo-list
+	;; before body was evaluated (in which case (funcall body) did
+	;; not add items to buffer-undo-list) ...
+	(unless (or (eq buffer-undo-list t)
+		    (eq buffer-undo-list old-bul))
+	  (let ((ptr buffer-undo-list) body-undo-list)
+	    ;; ... then loop over buffer-undo-list, until the head of
+	    ;; buffer-undo-list before body was evaluated is found, or
+	    ;; ptr is nil (which may happen if garbage-collect has
+	    ;; been called after (funcall body) and has truncated
+	    ;; buffer-undo-list) ...
+	    (while (and (not (eq ptr old-bul))
+			ptr)
+	      ;; ... and add the entries to body-undo-list, unless
+	      ;; they are of the form (t . <something>), which are
+	      ;; entries that record buffer modification timestamps.
+	      (unless (and (consp (car ptr))
+			   (eq (caar ptr) t))
+		(push (car ptr) body-undo-list))
+	      (setq ptr (cdr ptr)))
+	    ;; Warn if garbage-collect has truncated buffer-undo-list
+	    ;; behind our back.
+	    (when (and old-bul (not ptr))
+	      (message "combine-change-calls: buffer-undo-list broken"))
+	    (setq body-undo-list (nreverse body-undo-list))
+	    ;; Add an (apply ...) entry to buffer-undo-list, using
+	    ;; body-undo-list ...
+	    (push (list 'apply
+			(- end end-marker)
+			beg
+			(marker-position end-marker)
+			#'undo--wrap-and-run-primitive-undo
+			beg (marker-position end-marker)
+			body-undo-list)
+		  buffer-undo-list)
+	    ;; ... and set the cdr of buffer-undo-list to
+	    ;; buffer-undo-list before body was evaluated.
+	    (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
-- 
2.39.0

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>>> I don't even understand what this is supposed to do.
>> 
>> Yet you happily threw it away :-(
>
> Because there are no such precautions elsewhere in the code, and the 
> comment above ("In case garbage collection has removed OLD-BUL") does 
> not explain what its purpose is.  A few lines below, old-bul is used 
> without such a precaution.  Of course, if it has a purpose, it's okay to 
> keep it.
>

I didn't remember the main reason why I removed that (cdr ptr).  The 
reason is not only that the comment above it is wrong, but also that this 
(cdr ptr) condition itself is wrong: it makes that loop stop when the last 
element of the buffer-undo-list is reached.  When buffer-undo-list is 
initially nil, (funcall body) adds a number of entries in 
buffer-undo-list, and there is no reason to exit that loop without 
processing its last element.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>> I don't even understand what this is supposed to do.
>>> Yet you happily threw it away :-(
>>
>> Because there are no such precautions elsewhere in the code, and the
>> comment above ("In case garbage collection has removed OLD-BUL") does not
>> explain what its purpose is.  A few lines below, old-bul is used without
>> such a precaution.  Of course, if it has a purpose, it's okay to keep it.
>>
>
> I didn't remember the main reason why I removed that (cdr ptr).  The reason
> is not only that the comment above it is wrong, but also that this (cdr ptr)
> condition itself is wrong: it makes that loop stop when the last element of
> the buffer-undo-list is reached.  When buffer-undo-list is initially nil,
> (funcall body) adds a number of entries in buffer-undo-list, and there is no
> reason to exit that loop without processing its last element.

I think for your loop you'd need to check `ptr` instead of (cdr
ptr), indeed.  For the loop currently in `subr.el` I think the (cdr ptr)
is OK because we use the `setcdr` to truncate the list, so that last
cons (whose `cdr` may be nil) will be in the list included in the
(apply ....) entry.


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> For the loop currently in `subr.el` I think the (cdr ptr) is OK because 
> we use the `setcdr` to truncate the list, so that last cons (whose `cdr` 
> may be nil) will be in the list included in the (apply ....) entry.
>

Indeed, now I see it, and I think (without being 100% sure) that it's OK. 
It's yet another obscure point of the original code, though.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

Another data point, about the risk assessment aspect: this function is not 
used by any package on ELPA, and in core it is only used by Org (with 
which this bug was identified), newcomment.el (2 uses) and dired (a single 
use, in which the code is in fact bypassed because buffer-undo-list is 
let-bound to t in the body).

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Wed, 04 Jan 2023 10:50:30 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, Alan Mackenzie <acm@muc.de>, 
>     Eli Zaretskii <eliz@gnu.org>, yantar92@posteo.net
> 
> 
> Another data point, about the risk assessment aspect: this function is not 
> used by any package on ELPA, and in core it is only used by Org (with 
> which this bug was identified), newcomment.el (2 uses) and dired (a single 
> use, in which the code is in fact bypassed because buffer-undo-list is 
> let-bound to t in the body).

Which version of Org? the one that's on the release branch or a newer
one?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>> Another data point, about the risk assessment aspect: this function is 
>> not used by any package on ELPA, and in core it is only used by Org 
>> (with which this bug was identified), newcomment.el (2 uses) and dired 
>> (a single use, in which the code is in fact bypassed because 
>> buffer-undo-list is let-bound to t in the body).
>
> Which version of Org? the one that's on the release branch or a newer 
> one?
>

All versions or Org, including the one that's on the release branch.  The 
calls to combine-change-call were added to Org on Oct 16 2021 by 
85e0a69567 (in the Org repository).

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Wed, 04 Jan 2023 09:24:09 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, Alan Mackenzie <acm@muc.de>, 
>     Eli Zaretskii <eliz@gnu.org>, yantar92@posteo.net
> 
> > For the loop currently in `subr.el` I think the (cdr ptr) is OK because 
> > we use the `setcdr` to truncate the list, so that last cons (whose `cdr` 
> > may be nil) will be in the list included in the (apply ....) entry.
> >
> 
> Indeed, now I see it, and I think (without being 100% sure) that it's OK. 
> It's yet another obscure point of the original code, though.

I hope all these revelations will find their way into comments to this
tricky code.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>>> For the loop currently in `subr.el` I think the (cdr ptr) is OK 
>>> because we use the `setcdr` to truncate the list, so that last cons 
>>> (whose `cdr` may be nil) will be in the list included in the (apply 
>>> ....) entry.
>>
>> Indeed, now I see it, and I think (without being 100% sure) that it's 
>> OK. It's yet another obscure point of the original code, though.
>
> I hope all these revelations will find their way into comments to this 
> tricky code.
>

Well, I hope this tricky code will be replaced by the non-tricky code I 
proposed, in which none of these obscurities exist... ;-)

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>> For the loop currently in `subr.el` I think the (cdr ptr) is OK because
>>>> we use the `setcdr` to truncate the list, so that last cons (whose `cdr`
>>>> may be nil) will be in the list included in the (apply ....) entry.
>>>
>>> Indeed, now I see it, and I think (without being 100% sure) that it's
>>> OK. It's yet another obscure point of the original code, though.
>>
>> I hope all these revelations will find their way into comments to this
>> tricky code.
>>
>
> Well, I hope this tricky code will be replaced by the non-tricky code
> I proposed, in which none of these obscurities exist... ;-)

My hope is that we can just remove the timestamp special case, in which
case we can keep the current code mostly unchanged.


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>> Well, I hope this tricky code will be replaced by the non-tricky code I 
>> proposed, in which none of these obscurities exist... ;-)
>
> My hope is that we can just remove the timestamp special case, in which 
> case we can keep the current code mostly unchanged.
>

Then our hopes are different.  What's wrong exactly with replacing a piece 
of code that requires a long discussion with question marks everywhere to 
be understood by a piece of well-documented code that is much more 
readable and "evidently" does what it is supposed to do?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Wed, 04 Jan 2023 18:16:07 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, acm@muc.de, Eli Zaretskii <eliz@gnu.org>, 
>     yantar92@posteo.net
> 
> > My hope is that we can just remove the timestamp special case, in which 
> > case we can keep the current code mostly unchanged.
> >
> 
> Then our hopes are different.  What's wrong exactly with replacing a piece 
> of code that requires a long discussion with question marks everywhere to 
> be understood by a piece of well-documented code that is much more 
> readable and "evidently" does what it is supposed to do?

The old code was working for quite a few years, so it isn't all wrong.
Minimizing changes also minimizes the risk of introducing new exciting
bugs.  For a release branch, both these aspects are a clear win.

Why do you object so much to leaving the timestamps in the undo-list?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


>>> My hope is that we can just remove the timestamp special case, in 
>>> which case we can keep the current code mostly unchanged.
>>
>> Then our hopes are different.  What's wrong exactly with replacing a 
>> piece of code that requires a long discussion with question marks 
>> everywhere to be understood by a piece of well-documented code that is 
>> much more readable and "evidently" does what it is supposed to do?
>
> The old code was working for quite a few years, so it isn't all wrong.
>

It has only a few callers.  In fact, its only callers for a long time have 
been (1) comment-region-default and uncomment-region-default, which, by 
chance, do not insert timestamps between the undo elements, and (2) 
dired-readin in which (as I said upthread) this part of the function is 
simply not executed because buffer-undo-list is let-bound to t around 
body.  In effect, the first caller that actually exercises its logic in 
full is Org.

>
> Why do you object so much to leaving the timestamps in the undo-list?
>

I do not object to leaving timestamps in the undo-list, I object to 
leaving the code in the state it is, because I think of the future person 
who will try to make sense of it in 2028.  I'm equally fine with the two 
attached patches, one in which timestamps are kept an the other in which 
they aren't.

And now I believe it's better if I bow out of this thread, and let you and 
Stefan decide what the best course of action is.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-combine-change-call-without-timestamps.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call-without-timestamps.patch, Size: 6820 bytes --]

From 15797b21f663464b5599560e2c62df55a81b77cf Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 4 Jan 2023 20:50:49 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite and document
the part which creates the undo-list element.  Fixes bug#60467.
* test/src/undo-tests.el (undo-test-combine-change-calls-1)
(undo-test-combine-change-calls-2)
(undo-test-combine-change-calls-3): Add three tests for 'undo'
with 'combine-change-calls'.
---
 lisp/subr.el           | 65 +++++++++++++++++++++++---------------
 test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9087f9a404..7e201f4bc9 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4934,31 +4934,46 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+	;; If buffer-undo-list is neither t (in which case undo
+	;; information is not recorded) nor equal to buffer-undo-list
+	;; before body was funcalled (in which case (funcall body) did
+	;; not add items to buffer-undo-list) ...
+	(unless (or (eq buffer-undo-list t)
+		    (eq buffer-undo-list old-bul))
+	  (let ((ptr buffer-undo-list) body-undo-list)
+	    ;; ... then loop over buffer-undo-list, until the head of
+	    ;; buffer-undo-list before body was funcalled is found, or
+	    ;; ptr is nil (which may happen if garbage-collect has
+	    ;; been called after (funcall body) and has removed
+	    ;; entries of buffer-undo-list that were added by (funcall
+	    ;; body)) ...
+	    (while (and ptr (not (eq ptr old-bul)))
+	      ;; ... and add the entries to body-undo-list, unless
+	      ;; they are of the form (t . <something>), which are
+	      ;; entries that record buffer modification timestamps.
+	      (unless (and (consp (car ptr))
+			   (eq (caar ptr) t))
+		(push (car ptr) body-undo-list))
+	      (setq ptr (cdr ptr)))
+	    (setq body-undo-list (nreverse body-undo-list))
+	    ;; Warn if garbage-collect has truncated buffer-undo-list
+	    ;; behind our back.
+	    (when (and old-bul (not ptr))
+	      (message
+               "combine-change-calls: buffer-undo-list has been truncated"))
+	    ;; Add an (apply ...) entry to buffer-undo-list, using
+	    ;; body-undo-list ...
+	    (push (list 'apply
+			(- end end-marker)
+			beg
+			(marker-position end-marker)
+			#'undo--wrap-and-run-primitive-undo
+			beg (marker-position end-marker)
+			body-undo-list)
+		  buffer-undo-list)
+	    ;; ... and set the cdr of buffer-undo-list to
+	    ;; buffer-undo-list before body was funcalled.
+	    (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el
index 84151d3b5d..fd45a9101f 100644
--- a/test/src/undo-tests.el
+++ b/test/src/undo-tests.el
@@ -439,6 +439,78 @@ undo-test-region-mark-adjustment
 
     (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb"))))
 
+(ert-deftest undo-test-combine-change-calls-1 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 1: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' non-nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-2 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 2: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (save-buffer)
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-3 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 3: a file-visiting buffer with `buffer-undo-list' nil and
+`buffer-modified-p' nil when `combine-change-calls' is called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "ABC DEF")
+      (save-buffer)
+      (kill-buffer))
+    (with-current-buffer (find-file tempfile)
+      (should (= (length buffer-undo-list) 0))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 1)))))
+
 (defun undo-test-all (&optional interactive)
   "Run all tests for \\[undo]."
   (interactive "p")
-- 
2.39.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Fix-combine-change-call-with-timestamps.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call-with-timestamps.patch, Size: 6604 bytes --]

From a1c435749f1f4346cfd07fdb893f6721233fe9b7 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 4 Jan 2023 20:49:22 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite and document
the part which creates the undo-list element.  Fixes bug#60467.
* test/src/undo-tests.el (undo-test-combine-change-calls-1)
(undo-test-combine-change-calls-2)
(undo-test-combine-change-calls-3): Add three tests for 'undo'
with 'combine-change-calls'.
---
 lisp/subr.el           | 60 ++++++++++++++++++++---------------
 test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9087f9a404..106fa71b12 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4934,31 +4934,41 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+	;; If buffer-undo-list is neither t (in which case undo
+	;; information is not recorded) nor equal to buffer-undo-list
+	;; before body was funcalled (in which case (funcall body) did
+	;; not add items to buffer-undo-list) ...
+	(unless (or (eq buffer-undo-list t)
+		    (eq buffer-undo-list old-bul))
+	  (let ((ptr buffer-undo-list) body-undo-list)
+	    ;; ... then loop over buffer-undo-list, until the head of
+	    ;; buffer-undo-list before body was funcalled is found, or
+	    ;; ptr is nil (which may happen if garbage-collect has
+	    ;; been called after (funcall body) and has removed
+	    ;; entries of buffer-undo-list that were added by (funcall
+	    ;; body)), and add these entries to body-undo-list.
+	    (while (and ptr (not (eq ptr old-bul)))
+	      (push (car ptr) body-undo-list)
+	      (setq ptr (cdr ptr)))
+	    (setq body-undo-list (nreverse body-undo-list))
+	    ;; Warn if garbage-collect has truncated buffer-undo-list
+	    ;; behind our back.
+	    (when (and old-bul (not ptr))
+	      (message
+               "combine-change-calls: buffer-undo-list has been truncated"))
+	    ;; Add an (apply ...) entry to buffer-undo-list, using
+	    ;; body-undo-list ...
+	    (push (list 'apply
+			(- end end-marker)
+			beg
+			(marker-position end-marker)
+			#'undo--wrap-and-run-primitive-undo
+			beg (marker-position end-marker)
+			body-undo-list)
+		  buffer-undo-list)
+	    ;; ... and set the cdr of buffer-undo-list to
+	    ;; buffer-undo-list before body was funcalled.
+	    (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el
index 84151d3b5d..fd45a9101f 100644
--- a/test/src/undo-tests.el
+++ b/test/src/undo-tests.el
@@ -439,6 +439,78 @@ undo-test-region-mark-adjustment
 
     (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb"))))
 
+(ert-deftest undo-test-combine-change-calls-1 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 1: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' non-nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-2 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 2: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (save-buffer)
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-3 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 3: a file-visiting buffer with `buffer-undo-list' nil and
+`buffer-modified-p' nil when `combine-change-calls' is called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "ABC DEF")
+      (save-buffer)
+      (kill-buffer))
+    (with-current-buffer (find-file tempfile)
+      (should (= (length buffer-undo-list) 0))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 1)))))
+
 (defun undo-test-all (&optional interactive)
   "Run all tests for \\[undo]."
   (interactive "p")
-- 
2.39.0

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


>> If so, how about adding a test for this bug?
>
> I'll see if I can do that, but it seems tricky.
>

Here you go.  Without the patch tests 2 and 3 fail, with the patch all 
three tests pass.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Three-new-tests-for-undo-with-combine-change-calls.patch --]
[-- Type: text/x-diff; name=Three-new-tests-for-undo-with-combine-change-calls.patch, Size: 3474 bytes --]

From 333a1b46aa1a430fb8cb7a848946b0a5ea2a0ede Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 3 Jan 2023 14:54:09 +0000
Subject: [PATCH] Three new tests for 'undo' with 'combine-change-calls'

* test/src/undo-tests.el (undo-test-combine-change-calls-1)
(undo-test-combine-change-calls-2)
(undo-test-combine-change-calls-3): Add three tests for 'undo'
with 'combine-change-calls'.  See bug#60467.
---
 test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el
index 84151d3b5d..fd45a9101f 100644
--- a/test/src/undo-tests.el
+++ b/test/src/undo-tests.el
@@ -439,6 +439,78 @@ undo-test-region-mark-adjustment
 
     (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb"))))
 
+(ert-deftest undo-test-combine-change-calls-1 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 1: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' non-nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-2 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 2: a file-visiting buffer with `buffer-undo-list' non-nil
+and `buffer-modified-p' nil when `combine-change-calls' is
+called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "A")
+      (undo-boundary)
+      (insert "B")
+      (undo-boundary)
+      (insert "C")
+      (undo-boundary)
+      (insert " ")
+      (undo-boundary)
+      (insert "D")
+      (undo-boundary)
+      (insert "E")
+      (undo-boundary)
+      (insert "F")
+      (should (= (length buffer-undo-list) 14))
+      (save-buffer)
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 15)))))
+
+(ert-deftest undo-test-combine-change-calls-3 ()
+  "Test how `combine-change-calls' updates `buffer-undo-list'.
+Case 3: a file-visiting buffer with `buffer-undo-list' nil and
+`buffer-modified-p' nil when `combine-change-calls' is called."
+  (ert-with-temp-file tempfile
+    (with-current-buffer (find-file tempfile)
+      (insert "ABC DEF")
+      (save-buffer)
+      (kill-buffer))
+    (with-current-buffer (find-file tempfile)
+      (should (= (length buffer-undo-list) 0))
+      (goto-char (point-min))
+      (combine-change-calls (point-min) (point-max)
+        (re-search-forward "ABC ")
+        (replace-match "Z "))
+      (should (= (length buffer-undo-list) 1)))))
+
 (defun undo-test-all (&optional interactive)
   "Run all tests for \\[undo]."
   (interactive "p")
-- 
2.39.0

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3>
>
> ofter body has been evaluated, the buffer-undo-list after
> combine-change-call is
>
> (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>))
> <timestamp>  <undo element 2> <timestamp> nil <undo element 3>
>
> which is clearly wrong

Indeed.  Which begs the question: why does the current code stop when it
sees a timestamp?

Alan?  Do you remember why you did that?
What would go wrong if we applied a patch like the one below?


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 5fb150994ec..6f51ac90ce5 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4972,10 +4972,11 @@ combine-change-calls-1
 			      ;; In case garbage collection has removed OLD-BUL.
 			      (cdr ptr)
 			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
+			      ;; (not (and (consp (cdr ptr))
+			      ;;   	(consp (cadr ptr))
+			      ;;   	(eq (caadr ptr) t)
+			      ;;   	(setq old-bul (cdr ptr))))
+			      )
 		    (setq ptr (cdr ptr)))
 		  (unless (cdr ptr)
 		    (message "combine-change-calls: buffer-undo-list broken"))

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> What would go wrong if we applied a patch like the one below?
>

With that patch, the "combine-change-calls: buffer-undo-list broken" 
message would be displayed with the recipe of this bug report.  And 
timestamp entries would be added to what is "body-undo-list" in my patch. 
It's not clear to me that this could cause problems, but I guess it's 
safer to not include them, given that they were never included, and that 
the intention of the original code was to exclude them.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> With that patch, the "combine-change-calls: buffer-undo-list broken" message
> would be displayed with the recipe of this bug report.

IIUC this is a separate issue which is fairly easy to fix (and doesn't
need to be fixed on `emacs-29`) with something like the patch below.

> And timestamp entries would be added to what is "body-undo-list" in my
> patch. It's not clear to me that this could cause problems, but
> I guess it's safer to not include them, given that they were never
> included,

AFAICT when they were not included, they resulted in a broken undo
entry (as evidenced by the current bug report), so anything we do in
their presence is "safer" than what we did before :-)

Let's wait to see what Alan has to say,


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 5fb150994ec..ff3a7c403d0 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4970,15 +4970,16 @@ combine-change-calls-1
 		(progn
 		  (while (and (not (eq (cdr ptr) old-bul))
 			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
+			      (or (cdr ptr)
+			          (progn
+			            (message "combine-change-calls: buffer-undo-list broken")
+			            nil))
 			      ;; Don't include a timestamp entry.
 			      (not (and (consp (cdr ptr))
 					(consp (cadr ptr))
 					(eq (caadr ptr) t)
 					(setq old-bul (cdr ptr)))))
 		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
 		  (setcdr ptr nil)
 		  (push ap-elt buffer-undo-list)
 		  (setcdr buffer-undo-list old-bul)))))

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Alan Mackenzie]

Hello, Stefan.

On Tue, Jan 03, 2023 at 10:16:54 -0500, Stefan Monnier wrote:
> > <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3>

> > ofter body has been evaluated, the buffer-undo-list after
> > combine-change-call is

> > (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>))
> > <timestamp>  <undo element 2> <timestamp> nil <undo element 3>

> > which is clearly wrong

> Indeed.  Which begs the question: why does the current code stop when it
> sees a timestamp?

> Alan?  Do you remember why you did that?

I'm afraid not.  On 2018-04-01, I noted down that timestamps get "caught
up inside the undo--wrap-and-run-primitive-undo entry.".  But I neglected
to be more specific, and can no longer remember exactly why.

> What would go wrong if we applied a patch like the one below?

I don't know.  Possibly nothing.  Maybe the problem that that code was
meant to solve was also solved by some other code.

>         Stefan


> diff --git a/lisp/subr.el b/lisp/subr.el
> index 5fb150994ec..6f51ac90ce5 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -4972,10 +4972,11 @@ combine-change-calls-1
>  			      ;; In case garbage collection has removed OLD-BUL.
>  			      (cdr ptr)
>  			      ;; Don't include a timestamp entry.
> -			      (not (and (consp (cdr ptr))
> -					(consp (cadr ptr))
> -					(eq (caadr ptr) t)
> -					(setq old-bul (cdr ptr)))))
> +			      ;; (not (and (consp (cdr ptr))
> +			      ;;   	(consp (cadr ptr))
> +			      ;;   	(eq (caadr ptr) t)
> +			      ;;   	(setq old-bul (cdr ptr))))
> +			      )
>  		    (setq ptr (cdr ptr)))
>  		  (unless (cdr ptr)
>  		    (message "combine-change-calls: buffer-undo-list broken"))

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Alan?  Do you remember why you did that?
> I'm afraid not.  On 2018-04-01, I noted down that timestamps get "caught
> up inside the undo--wrap-and-run-primitive-undo entry.".  But I neglected
> to be more specific, and can no longer remember exactly why.
>
>> What would go wrong if we applied a patch like the one below?
>
> I don't know.  Possibly nothing.  Maybe the problem that that code was
> meant to solve was also solved by some other code.

Thanks.  So maybe we should strip timestamps on the release branch
(just to be on the safe side) and keep them on `master` (since it's
likely that we don't need to filter them out, which gives simpler code
and might even be arguably more correct).

On `emacs-29` the patch I suggest is:

diff --git a/lisp/subr.el b/lisp/subr.el
index 62f72734e14..34dd847e9d5 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4946,13 +4946,13 @@ combine-change-calls-1
 		(progn
 		  (while (and (not (eq (cdr ptr) old-bul))
 			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
-		    (setq ptr (cdr ptr)))
+			      (cdr ptr))
+		    (if (and (consp (cdr ptr))
+			     (consp (cadr ptr))
+			     (eq (caadr ptr) t))
+			;; Don't include a timestamp entry.
+			(setcdr ptr (cddr ptr))
+		      (setq ptr (cdr ptr))))
 		  (unless (cdr ptr)
 		    (message "combine-change-calls: buffer-undo-list broken"))
 		  (setcdr ptr nil)

I think it's the simplest&safest change to the code that fixes this bug.
[ Of course, it should be accompanied by Gregory's tests.  ]

Then on master I suggest:

diff --git a/lisp/subr.el b/lisp/subr.el
index d1d3c76caf8..9e50b1e7f91 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4966,21 +4966,20 @@ combine-change-calls-1
 		       beg
 		       (marker-position end-marker)
 		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
+		       beg (marker-position end-marker)
+		       ;; We will truncate this list by side-effect below.
+		       buffer-undo-list))
 		(ptr buffer-undo-list))
 	    (if (not (eq buffer-undo-list old-bul))
 		(progn
 		  (while (and (not (eq (cdr ptr) old-bul))
 			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr)
-			      ;; Don't include a timestamp entry.
-			      (not (and (consp (cdr ptr))
-					(consp (cadr ptr))
-					(eq (caadr ptr) t)
-					(setq old-bul (cdr ptr)))))
+			      (or (cdr ptr)
+			          (progn
+			            (message "combine-change-calls: buffer-undo-list broken")
+			            nil)))
 		    (setq ptr (cdr ptr)))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
+		  ;; Truncate the list that's in the `apply' entry.
 		  (setcdr ptr nil)
 		  (push ap-elt buffer-undo-list)
 		  (setcdr buffer-undo-list old-bul)))))

We could also use Gregory's code on `master`.  Obviously Gregory prefers
it, tho it's marginally less efficient since it creates a new list
instead of re-using the list elements already present.


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Cc: 60467@debbugs.gnu.org, Gregory Heytings <gregory@heytings.org>,
>  Ihor Radchenko <yantar92@posteo.net>
> Date: Mon, 09 Jan 2023 01:03:43 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Thanks.  So maybe we should strip timestamps on the release branch
> (just to be on the safe side) and keep them on `master` (since it's
> likely that we don't need to filter them out, which gives simpler code
> and might even be arguably more correct).
> 
> On `emacs-29` the patch I suggest is:

Fine with me, thanks.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Fine with me, thanks.

OK, pushed.
I also pushed Gregory's new tests.  I think we can close this now.


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: acm@muc.de,  60467-done@debbugs.gnu.org,  gregory@heytings.org,
>   yantar92@posteo.net
> Date: Fri, 13 Jan 2023 17:45:06 -0500
> 
> > Fine with me, thanks.
> 
> OK, pushed.
> I also pushed Gregory's new tests.  I think we can close this now.

Thanks.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Ihor Radchenko]

Gregory Heytings <gregory@heytings.org> writes:

> The culprit is 85e0a69567 (in the Org repository), and the bug is fixed 
> by:
>
> -    (combine-change-calls (point) (save-excursion (org-end-of-subtree t))
> -      (org-with-limited-levels (org-map-tree 'org-promote))))
> +    (org-with-limited-levels (org-map-tree 'org-promote)))
>     (org-fix-position-after-promote))

Unfortunately, `combine-change-calls' is there for a reason.
Heading manipulation involves adding/removing stars. When done in batch,
this involves drastic AST changes. `combine-change-calls' here is used to
merge AST cache modifications into a single update request. It makes
orders of magnitudes performance improvement in some scenarios.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60467: New problem introduced

[Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi all,

I'm reopening this bug because the fix has introduced a new problem.

This can be highlighted with the following procedure (verified with
Emacs 29.0.92) :
1. Open a file: emacs -Q <file>
2. Move point to a line you can comment
3. M-x comment-line
4. M-x undo
5. Check buffer modification status in modeline or use (buffer-
modified-p)

Without the patched version of combine-change-calls-1, the buffer
status is "not modified since last save". Same result if you use
another command, for example kill-line,  at step 2.
With new version of combine-change-calls-1, the buffer is seen as
modified.

I'm not sure but I imagine it has something to do with the lack of
timestamp in buffer-undo-list created by combine-change-calls-1.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

[Please don't change the Subject when you reopen a bug.]

> Date: Thu, 22 Jun 2023 21:28:44 +0200
> From:  Frédéric Giquel via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Hi all,
> 
> I'm reopening this bug because the fix has introduced a new problem.
> 
> This can be highlighted with the following procedure (verified with
> Emacs 29.0.92) :
> 1. Open a file: emacs -Q <file>
> 2. Move point to a line you can comment
> 3. M-x comment-line
> 4. M-x undo
> 5. Check buffer modification status in modeline or use (buffer-
> modified-p)
> 
> Without the patched version of combine-change-calls-1, the buffer
> status is "not modified since last save". Same result if you use
> another command, for example kill-line,  at step 2.
> With new version of combine-change-calls-1, the buffer is seen as
> modified.
> 
> I'm not sure but I imagine it has something to do with the lack of
> timestamp in buffer-undo-list created by combine-change-calls-1.

Stefan, can you please look into this?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> Stefan, can you please look into this?
>

FYI, that issue is fixed with the Fix-combine-change-call-with-timestamps 
patch.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Stefan, can you please look into this?
> FYI, that issue is fixed with the
> Fix-combine-change-call-with-timestamps patch.

Be my guest,


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Stefan, can you please look into this?
> FYI, that issue is fixed with the
> Fix-combine-change-call-with-timestamps patch.

Oh, but I don't think this is OK for the `emacs-29` branch, so we still
need a more targetted fix for the `emacs-29` branch.


        Stefan

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>>> Stefan, can you please look into this?
>>
>> FYI, that issue is fixed with the 
>> Fix-combine-change-call-with-timestamps patch.
>
> Oh, but I don't think this is OK for the `emacs-29` branch, so we still 
> need a more targetted fix for the `emacs-29` branch.
>

Well, that bug was already discussed at length six months ago, and the 
minimal change that was in the end installed (59c3c53efa) introduced a new 
bug.  Should we design another minimal change, at the risk of introducing 
another bug?  ISTM that the safe thing to do is the refactoring that was 
suggested.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Mon, 26 Jun 2023 15:18:45 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, 
>     Frédéric Giquel <frederic.giquel@laposte.net>, 
>     Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>, 
>     Alan Mackenzie <acm@muc.de>
> 
> >> FYI, that issue is fixed with the 
> >> Fix-combine-change-call-with-timestamps patch.
> >
> > Oh, but I don't think this is OK for the `emacs-29` branch, so we still 
> > need a more targetted fix for the `emacs-29` branch.
> 
> Well, that bug was already discussed at length six months ago, and the 
> minimal change that was in the end installed (59c3c53efa) introduced a new 
> bug.  Should we design another minimal change, at the risk of introducing 
> another bug?  ISTM that the safe thing to do is the refactoring that was 
> suggested.

If there's no safer way than the refactoring suggested back then, we
will have to stay with this regression in Emacs 29.1 and install a
better fix only on master.  After all, the fact that buffer-modified-p
is not undone is a minor inconvenience at best.

I'm sorry, but I cannot wait too much time longer before the release.
The Emacs 29.1 release cycle already took significantly longer than I
ever imagined.  So if someone can think about some clever ideas to fix
what we have on the release branch by small and safe changes, please
speak up soon.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>> Well, that bug was already discussed at length six months ago, and the 
>> minimal change that was in the end installed (59c3c53efa) introduced a 
>> new bug.  Should we design another minimal change, at the risk of 
>> introducing another bug?  ISTM that the safe thing to do is the 
>> refactoring that was suggested.
>
> If there's no safer way than the refactoring suggested back then, we 
> will have to stay with this regression in Emacs 29.1 and install a 
> better fix only on master.  After all, the fact that buffer-modified-p 
> is not undone is a minor inconvenience at best.
>

Indeed.  A safer way probably exists, but I would not dare writing that 
patch.  I believe Stefan is in a better position to write it.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Sat, 01 Jul 2023 14:14:11 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, 
>     monnier@iro.umontreal.ca, acm@muc.de
> 
> 
> >> Well, that bug was already discussed at length six months ago, and the 
> >> minimal change that was in the end installed (59c3c53efa) introduced a 
> >> new bug.  Should we design another minimal change, at the risk of 
> >> introducing another bug?  ISTM that the safe thing to do is the 
> >> refactoring that was suggested.
> >
> > If there's no safer way than the refactoring suggested back then, we 
> > will have to stay with this regression in Emacs 29.1 and install a 
> > better fix only on master.  After all, the fact that buffer-modified-p 
> > is not undone is a minor inconvenience at best.
> >
> 
> Indeed.  A safer way probably exists, but I would not dare writing that 
> patch.  I believe Stefan is in a better position to write it.

How about installing your patch, which you proposed back then, on
master, then?

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

Ping!

> Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net,
>  monnier@iro.umontreal.ca, acm@muc.de
> Date: Sat, 01 Jul 2023 17:27:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Sat, 01 Jul 2023 14:14:11 +0000
> > From: Gregory Heytings <gregory@heytings.org>
> > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, 
> >     monnier@iro.umontreal.ca, acm@muc.de
> > 
> > 
> > >> Well, that bug was already discussed at length six months ago, and the 
> > >> minimal change that was in the end installed (59c3c53efa) introduced a 
> > >> new bug.  Should we design another minimal change, at the risk of 
> > >> introducing another bug?  ISTM that the safe thing to do is the 
> > >> refactoring that was suggested.
> > >
> > > If there's no safer way than the refactoring suggested back then, we 
> > > will have to stay with this regression in Emacs 29.1 and install a 
> > > better fix only on master.  After all, the fact that buffer-modified-p 
> > > is not undone is a minor inconvenience at best.
> > >
> > 
> > Indeed.  A safer way probably exists, but I would not dare writing that 
> > patch.  I believe Stefan is in a better position to write it.
> 
> How about installing your patch, which you proposed back then, on
> master, then?
> 
> 
> 
> 

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

Ping! Ping!

Can we please finish handling of this issue?

> Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net,
>  monnier@iro.umontreal.ca, acm@muc.de
> Date: Sat, 15 Jul 2023 10:46:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping!
> 
> > Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net,
> >  monnier@iro.umontreal.ca, acm@muc.de
> > Date: Sat, 01 Jul 2023 17:27:52 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > > Date: Sat, 01 Jul 2023 14:14:11 +0000
> > > From: Gregory Heytings <gregory@heytings.org>
> > > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, 
> > >     monnier@iro.umontreal.ca, acm@muc.de
> > > 
> > > 
> > > >> Well, that bug was already discussed at length six months ago, and the 
> > > >> minimal change that was in the end installed (59c3c53efa) introduced a 
> > > >> new bug.  Should we design another minimal change, at the risk of 
> > > >> introducing another bug?  ISTM that the safe thing to do is the 
> > > >> refactoring that was suggested.
> > > >
> > > > If there's no safer way than the refactoring suggested back then, we 
> > > > will have to stay with this regression in Emacs 29.1 and install a 
> > > > better fix only on master.  After all, the fact that buffer-modified-p 
> > > > is not undone is a minor inconvenience at best.
> > > >
> > > 
> > > Indeed.  A safer way probably exists, but I would not dare writing that 
> > > patch.  I believe Stefan is in a better position to write it.
> > 
> > How about installing your patch, which you proposed back then, on
> > master, then?
> > 
> > 
> > 
> > 
> 
> 
> 
> 

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


>
> Can we please finish handling of this issue?
>

It's not clear to me what you want: do you want to fix this issue on the 
emacs-29 branch or on the master branch?  We now have two bug reports on 
this (the one by Frédéric Giquel in this bug, and the one by Eason Huang 
in bug#64989).  I just verified (again) that the 
Fix-combine-change-call-with-timestamps patch fixes these two recipes.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Thu, 10 Aug 2023 11:28:33 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, 
>     monnier@iro.umontreal.ca, acm@muc.de
> 
> > Can we please finish handling of this issue?
> 
> It's not clear to me what you want: do you want to fix this issue on the 
> emacs-29 branch or on the master branch?

I suggested to install your fix on master.

> We now have two bug reports on this (the one by Frédéric Giquel in
> this bug, and the one by Eason Huang in bug#64989).  I just verified
> (again) that the Fix-combine-change-call-with-timestamps patch fixes
> these two recipes.

If bug#64989 could be fixed by a simpler, safer change (which remains
to be seen), we might do that on emacs-29.  If not, at least we will
have this fixed in Emacs 30.

Thanks.

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

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


>> It's not clear to me what you want: do you want to fix this issue on 
>> the emacs-29 branch or on the master branch?
>
> I suggested to install your fix on master.
>

Okay, but see below.

>> We now have two bug reports on this (the one by Frédéric Giquel in this 
>> bug, and the one by Eason Huang in bug#64989).  I just verified (again) 
>> that the Fix-combine-change-call-with-timestamps patch fixes these two 
>> recipes.
>
> If bug#64989 could be fixed by a simpler, safer change (which remains to 
> be seen), we might do that on emacs-29.  If not, at least we will have 
> this fixed in Emacs 30.
>

Bug#64989 is the same bug, only the recipe differs.  And the change I 
suggested six months ago is safe, it replaces an obscure and bugged piece 
of code with a refactored and clear piece of code.  I attach it again 
(that patch is for emacs-29, the one for master would be similar).

If Stefan (or anyone else) fixes the bug in a safer way, that patch is not 
necessary.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-combine-change-call.patch --]
[-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3453 bytes --]

From 169b2064a4c5200fbfdfd7c6f43cb18e4f854b92 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 10 Aug 2023 13:49:15 +0000
Subject: [PATCH] Fix combine-change-call

* lisp/subr.el (combine-change-calls-1): Rewrite and document
the part which creates the undo-list element.  Fixes bug#60467
and bug#64989.
---
 lisp/subr.el | 60 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 872c701dbe7..d0760e92474 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4978,31 +4978,41 @@ combine-change-calls-1
 	      (kill-local-variable 'before-change-functions))
 	    (if local-acf (setq after-change-functions acf)
 	      (kill-local-variable 'after-change-functions))))
-        (when (not (eq buffer-undo-list t))
-          (let ((ap-elt
-		 (list 'apply
-		       (- end end-marker)
-		       beg
-		       (marker-position end-marker)
-		       #'undo--wrap-and-run-primitive-undo
-		       beg (marker-position end-marker) buffer-undo-list))
-		(ptr buffer-undo-list))
-	    (if (not (eq buffer-undo-list old-bul))
-		(progn
-		  (while (and (not (eq (cdr ptr) old-bul))
-			      ;; In case garbage collection has removed OLD-BUL.
-			      (cdr ptr))
-		    (if (and (consp (cdr ptr))
-			     (consp (cadr ptr))
-			     (eq (caadr ptr) t))
-			;; Don't include a timestamp entry.
-			(setcdr ptr (cddr ptr))
-		      (setq ptr (cdr ptr))))
-		  (unless (cdr ptr)
-		    (message "combine-change-calls: buffer-undo-list broken"))
-		  (setcdr ptr nil)
-		  (push ap-elt buffer-undo-list)
-		  (setcdr buffer-undo-list old-bul)))))
+	;; If buffer-undo-list is neither t (in which case undo
+	;; information is not recorded) nor equal to buffer-undo-list
+	;; before body was funcalled (in which case (funcall body) did
+	;; not add items to buffer-undo-list) ...
+	(unless (or (eq buffer-undo-list t)
+		    (eq buffer-undo-list old-bul))
+	  (let ((ptr buffer-undo-list) body-undo-list)
+	    ;; ... then loop over buffer-undo-list, until the head of
+	    ;; buffer-undo-list before body was funcalled is found, or
+	    ;; ptr is nil (which may happen if garbage-collect has
+	    ;; been called after (funcall body) and has removed
+	    ;; entries of buffer-undo-list that were added by (funcall
+	    ;; body)), and add these entries to body-undo-list.
+	    (while (and ptr (not (eq ptr old-bul)))
+	      (push (car ptr) body-undo-list)
+	      (setq ptr (cdr ptr)))
+	    (setq body-undo-list (nreverse body-undo-list))
+	    ;; Warn if garbage-collect has truncated buffer-undo-list
+	    ;; behind our back.
+	    (when (and old-bul (not ptr))
+	      (message
+               "combine-change-calls: buffer-undo-list has been truncated"))
+	    ;; Add an (apply ...) entry to buffer-undo-list, using
+	    ;; body-undo-list ...
+	    (push (list 'apply
+			(- end end-marker)
+			beg
+			(marker-position end-marker)
+			#'undo--wrap-and-run-primitive-undo
+			beg (marker-position end-marker)
+			body-undo-list)
+		  buffer-undo-list)
+	    ;; ... and set the cdr of buffer-undo-list to
+	    ;; buffer-undo-list before body was funcalled.
+	    (setcdr buffer-undo-list old-bul)))
 	(if (not inhibit-modification-hooks)
 	    (run-hook-with-args 'after-change-functions
 				beg (marker-position end-marker)
-- 
2.39.2

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Eli Zaretskii]

> Date: Thu, 10 Aug 2023 13:55:56 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, 
>     monnier@iro.umontreal.ca, acm@muc.de
> 
> >> It's not clear to me what you want: do you want to fix this issue on 
> >> the emacs-29 branch or on the master branch?
> >
> > I suggested to install your fix on master.
> >
> 
> Okay, but see below.

I see that you haven't yet installed your fix on master; please do.

TIA

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[Gregory Heytings]

>
> I see that you haven't yet installed your fix on master; please do.
>

I was waiting for your and Stefan's comments.  Now done (d622602452), and 
closing this bug (again?).

bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced

[sbaugh]

Incidentally, this bug has an easy reproduction in emacs -Q on Emacs 29
and before:

(with-temp-buffer
  (insert "foo")
  (setq-local buffer-undo-list nil
              comment-start    ";")
  (comment-line 1))

which produces the "combine-change-calls: buffer-undo-list broken"
warning.  This is easy to trigger in practice: simply open a file
already containing text, and comment a line as your first action.

Anyway, this was also fixed by Stefan's change 977630b5285809a57e50,
* lisp/subr.el (combine-change-calls-1): Fix bug#60467

(I send this email just because I spent some time tracking this down
only to find it was already fixed.  Maybe my smallish reproducer will
somehow be helpful to someone...)