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

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: 60467 <at> debbugs.gnu.org, yantar92 <at> posteo.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#60467: 30.0.50; primitive-undo: Changes to be undone by
 function different from announced
Date: Tue, 03 Jan 2023 16:48:41 +0200

> Date: Tue, 03 Jan 2023 13:44:01 +0000
> From: Gregory Heytings <gregory <at> heytings.org>
> cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 60467 <at> debbugs.gnu.org, 
>     yantar92 <at> 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?

Previous Next

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