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.