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

[Gregory Heytings]

> > I don't even understand what this is supposed to do.
>
> Yet you happily threw it away :-(

Because there are no such precautions elsewhere in the code, and the 
comment above ("In case garbage collection has removed OLD-BUL") does not 
explain what its purpose is.  A few lines below, old-bul is used without 
such a precaution.  Of course, if it has a purpose, it's okay to keep it.

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

Aha.  So this means that garbage-collect could be called between (funcall 
body) and the (while ...) loop two lines below, and could have removed 
elements that were added to buffer-undo-list by (funcall body), right? 
If so, that's what the comment should have said.

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

Given what you write above, that's probably what it meant to do indeed. 
But it also warns when buffer-undo-list is initially nil, such as with the 
recipe of this bug report.  If adding such a warning is useful, we should 
check whether old-bul was initially non-nil.

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

It took me a half hour to figure out what the original code was doing with 
this 'ap-elt' to which 'buffer-undo-list' is added, and subsequenty 
modified by the loop below, which does not refer to buffer-undo-list, and 
which also updates 'old-bul' in an unclear way.  I'm biased of course, but 
I think I would have understood the code in my patch much easier: it does 
one thing (adding the new undo element) after the other (building the list 
included in that undo element).