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?