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" <bug-gnu-emacs@gnu.org>]

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