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

Previous Next

Full log

Message #98 received at 60467 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 60467 <at> debbugs.gnu.org, Gregory Heytings <gregory <at> heytings.org>,
 Ihor Radchenko <yantar92 <at> posteo.net>
Subject: Re: bug#60467: 30.0.50; primitive-undo: Changes to be undone by
 function different from announced
Date: Mon, 09 Jan 2023 01:03:43 -0500

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.