bug#16377: Undo Tree regression: (error "Unrecognized entry in undo list undo-tree-canary")

[Toby Cubitt]

On Wed, Jul 05, 2017 at 11:25:10PM -0700, Keith David Bershatsky wrote:
> The existence and discussion surrounding `undo-tree.el` predates my
> usage of that library, but from what I can tell, at some point in the
> past `primitive-undo' was "improved" to start checking for errors and
> this resulted in bug #16377.

No, the new error checking just helpfully revealed an existing bug in
undo-tree's undo-in-region support (which frankly has always been flaky,
as it's extraordinarily difficult to reliably reproduce undo
bugs). There's even an `undo-tree-enable-undo-in-region' toggle to
disable undo-in-region support, for exactly this reason. Probably I
should make it default to "off" for now.

I'm way behind on dealing with undo-tree bug reports whilst busy with
"real life". #16377 is a very helpful bug report, which I can now
reproduce much more reliably with current Emacs version than I could when
it was originally reported. I hope to have time for a mammoth undo-tree
maintenance session later this summer. If that works out, I'll look into
it then.

> From an untrained layman's way of thinking, I am "guessing" that if
> undo-tree worked fine before `primitive-undo` was modified to throw an
> error, then perhaps it is not "a problem" with undo-tree except to the
> extent that undo-tree may need to "evolve" to play nice with the
> current version of `primitive-undo`.

The undo-tree-canary symbol should never end up in the list passed by
undo-tree to `primitive-undo', so it shouldn't matter whether it throws
an error. `primitive-undo' throwing an error on garbage, as in current
Emacs, makes complete sense from my perspective.

> Based on that "optional evolution theory", I created the workaround to
> just throw an informative message instead of an error.  However, I do
> not know if that approach could lead to problems.

I deliberately didn't touch `primitive-undo' in undo-tree. It was a c
primitive when I wrote undo-tree, and overriding primitives is not a good
idea.

Even now, I wouldn't recommend overriding it without rewriting undo-tree
so it completely rips out and replaces the Emacs undo system. (If that's
even possible -- it didn't used to be because too much was done in c.) As
currently implemented, undo-tree sits on top of the standard Emacs undo
system and overrides as little as possible.

I certainly wouldn't disable the error reporting, since that just masks
the bug, it doesn't fix it.

> I've spent quite a bit of time studying certain sections of the
>  undo-tree.el library, but there are sections of the code that are
>  still "Greek to me".  My understanding of the `undo-tree-canary`
>  symbol inside the `buffer-undo-list` is that it is a way for undo-tree
>  to check if it has interacted with the `buffer-undo-list`.

The *only* purpose of the undo-tree-canary is to detect when Emacs has
discarded undo history from buffer-undo-list before undo-tree got to look
at it. In that situation, the entire contents of buffer-undo-tree gets
discarded (because it only contains undo history that's being discarded),
and gets rebuilt afresh from the new contents of buffer-undo-list.

> There is only one situation I am aware of where the `undo-tree-canary`
> disappears, and it happens sometimes with garbage collection (bug
> #27214).  Whatever symbol is used, it needs to remain in the
> `buffer-undo-list` until `undo-tree-mode` is deactivated.

No, it's fine for it to be removed from buffer-undo-list, as long as this
only happens when the whole undo history is being discarded.

> I suppose the design could have been different, but Dr. Cubitt probably
> had several additiona l reasons for using a constant symbol such as the
> `undo-tree-canary`.

Anything that's an invalid undo entry would do the job. A symbol with the
package prefix is the obvious choice.

> Bug #16377 might very well be resolvable by tweaking/fixing
> undo-tree.el; however, the undo/redo in region code is still a few
> light years beyond my present abilities.

Unfortunately, it's also always been a few light years beyond my ability
to debug :-/

Best,
Toby
-- 
Dr T. S. Cubitt
Royal Society University Research Fellow
Quantum Information Theory
Department of Computer Science
University College London

email: tsc25@cantab.net
web:   www.dr-qubit.org