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

Previous Next

Full log

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

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

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

Previous Next

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