search

emacs-evil / evil

The extensible vi layer for Emacs.
GNU General Public License v3.0
3.36k stars 282 forks source link

Remove undo-tree dependency #1074

Closed alienbogart closed 4 years ago

alienbogart commented 6 years ago

Issue

Undo tree is an undo package that cannot keep undos.

Environment

Emacs version: Emacs 26.1.50 Operating System: Manjaro (current) Evil version: Evil version 1.2.13 Evil installation type: MELP Graphical/Terminal: Graphical Tested in a make emacs session (see CONTRIBUTING.md): Yes Undo configurations:

(use-package undo-tree
  :ensure t
  :init
  (setq undo-limit 78643200)
  (setq undo-outer-limit 104857600)
  (setq undo-strong-limit 157286400)
  (setq undo-tree-mode-lighter " UN")
  (setq undo-tree-auto-save-history t)
  (setq undo-tree-enable-undo-in-region nil)
  (setq undo-tree-history-directory-alist '(("." . "~/emacs.d/undo")))
 (add-hook 'undo-tree-visualizer-mode-hook (lambda ()
                                              (undo-tree-visualizer-selection-mode)
                                              (setq display-line-numbers nil)))
  :config
  (global-undo-tree-mode 1))

Reproduction steps

Expected behavior

You should be able to undo and/or redo every single change

Actual behavior

When it works, I'm able to undo 5 changes at the most. Half the time I can't undo anything at all. Sometimes I get the message unrecognized entry in undo list undo-tree-canary while I'm performing the undo.

Further notes

It is not just me:

https://old.reddit.com/r/emacs/comments/85t95p/undo_tree_unrecognized_entry_in_undo_list/ https://debbugs.gnu.org/cgi/bugreport.cgi?bug=16523 https://debbugs.gnu.org/cgi/bugreport.cgi?bug=16377 https://lists.ourproject.org/pipermail/implementations-list/2014-November/002091.html https://github.com/syl20bnr/spacemacs/issues/298 https://github.com/syl20bnr/spacemacs/issues/9903

I also must add that I had this same problem before with entirely different configurations, other versions of Emacs and operating systems.

npostavs commented 6 years ago

Make 30 changes on a files an save each step

Is there some specific set of changes needed for this? I tried with the below and got no errors (this doesn't load evil, but as far as I understand the bug is not related to evil-mode).

~/src/emacs$ emacs -Q -l bug-1074evil-undo-tree/setup.el -f bug-1074-do-changes -f kill-emacs
~/src/emacs$ emacs -Q -l bug-1074evil-undo-tree/setup.el -f bug-1074-undo --eval '(sit-for 2)' -f bug-1074-redo

Where is bug-1074evil-undo-tree/setup.el is

(defconst bug-1074-dir
  (file-name-directory (or load-file-name buffer-file-name)))

(setq undo-limit 78643200)
(setq undo-outer-limit 104857600)
(setq undo-strong-limit 157286400)
(setq undo-tree-mode-lighter " UN")
(setq undo-tree-auto-save-history t)
(setq undo-tree-enable-undo-in-region nil)
(setq undo-tree-history-directory-alist `(("." . ,(expand-file-name "undo" bug-1074-dir))))
(add-hook 'undo-tree-visualizer-mode-hook
          (lambda ()
            (undo-tree-visualizer-selection-mode)
            (setq display-line-numbers nil)))

(add-to-list 'load-path (expand-file-name "../elpa/packages/undo-tree/" bug-1074-dir))

(require 'undo-tree)

(global-undo-tree-mode 1)

(defun bug-1074-do-changes ()
  (interactive)
  (let ((file (expand-file-name "xx.txt" bug-1074-dir)))
    (with-current-buffer (find-file-noselect file)
      (pop-to-buffer-same-window (current-buffer))
      (dotimes (i 30)
        (insert (format "%d\n" i))
        (save-buffer)))))

(defun bug-1074-undo ()
  (interactive)
  (let ((file (expand-file-name "xx.txt" bug-1074-dir)))
    (with-current-buffer (find-file-noselect file)
      (pop-to-buffer-same-window (current-buffer))
      (dotimes (i 30)
        (undo-tree-undo)))))

(defun bug-1074-redo ()
  (interactive)
  (let ((file (expand-file-name "xx.txt" bug-1074-dir)))
    (with-current-buffer (find-file-noselect file)
      (pop-to-buffer-same-window (current-buffer))
      (dotimes (i 30)
        (undo-tree-redo)))))
dolorsitatem commented 5 years ago

You're correct that the problem is with undo-tree. I have this problem, too. And it's a nightmare. The undo simply isn't reliable. I've lost work many times because of this.

Unfortunately, there are two problems here. First, while it is reasonable to expect a set of steps to reproduce the error, a definitive way to reproduce it has yet to be found. Refer to the links @mrbig033 provided to see various other suggested steps. No one has found a way to reproduce it reliably. Second, the package author is unlikely to fix the problem any time soon. To quote the author from one of the linked bug reports,

The undo-tree undo-in-region code is fiendishly complex, hard to test, and under-tested.

He's apparently a busy man, too.

The previous link suggests recompiling Evil without the undo-tree dependency. There has to be a better way!

Maybe there's a way to disable the loading of undo-tree in the first place? I'm not very experienced with Elisp, but is there something I can do help find a solution?

npostavs commented 5 years ago

The previous link suggests recompiling Evil without the undo-tree dependency. Maybe there's a way to disable the loading of undo-tree in the first place?

From what I read there, recompiling simply has the same effect as (global-undo-tree-mode -1) (i.e., you lose redo, though I guess if you can get used to Emacs' redo-via-undoing-undo flow it could be workable).

I'm not very experienced with Elisp, but is there something I can do help find a solution?

Well, finding a way to reproduce the problem, that's really what we're missing.

noctuid commented 5 years ago

I've never not been able to reproduce this issue with my config and stopped using undo tree entirely a while ago. @lawlist may know how to reliably reproduce this problem. From my understanding, he has fixed this issue for various cases in his fork.

lawlist commented 5 years ago

(setq undo-tree-enable-undo-in-region nil)

See: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=16377#55

Quote from Dr. Cubitt:

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.
noctuid commented 5 years ago

That's been in my config and has never affected/prevented corruption (with persistent undo). Based on the linked spacemacs issues, other users have had the same experience. Are you saying that this is the cause of corruption (or is at least the cause with persistence disabled)?

lawlist commented 5 years ago

I am going to go out on a limb and assume corruption with persistent undo might mean that Emacs crashes or fails to load the undo-tree history file. If that is the case, there are two causes I have encountered: (1) on OSX, the undo tree is rather large and Emacs 26+ cannot handle it; or, (2) the undo-tree history file contains # and Emacs cannot read it; e.g., #<marker in no buffer> or #<overlay in no buffer>. This can be determined by manually inspecting the undo-tree history file and searching for the # symbol. The remedies for 1 and 2 are different. If you are referring to a third situation I have not yet seen, then I'd be interested in documenting it. Although Eli strongly discourages this (setq-default bidi-display-reordering nil) at least temporarily while inspecting the undo-tree history file, permits me to go through that buffer without Emacs bogging down -- remove that setting after you are finished debugging.

tgbugs commented 5 years ago

Chiming in with another report that (setq undo-tree-enable-undo-in-region nil) does not solve the issue. It seems to me that the bug is triggered almost 100% of the time when there is a branch in the tree. If I undo backward past the branch and they try to go forward again from the branch point, I get the canary error. However, I have not tested this scientifically, so it could be my brain playing tricks on me.

AitBits commented 5 years ago

For me, It happens even with persistent mode off. It's not causing problem though, since I don't trust it anymore and simply don't rely on it.

NightMachinery commented 5 years ago

I have a more basic problem with undo-tree; It bundles too many changes as a single change. E.g., I have typed, deleted, inserted a new line, then ran a command I didn't like, and so I undo, and go back to the beginning.

TheBB commented 5 years ago

@NightMachinary for that you can try experimenting with evil-want-fine-undo.

lawlist commented 5 years ago

@NightMachinary -- see my answer on stackexchange: https://emacs.stackexchange.com/a/47349/2287

dolorsitatem commented 5 years ago

Maybe there's a way to disable the loading of undo-tree in the first place? I'm not very experienced with Elisp, but is there something I can do help find a solution?

I am more experienced with Elisp now.

It seems that evil will default to (Emacs) undo if it can't find undo-tree. Removing undo-tree from the load-path seems to do the trick. This can be done as follows:

  1. Get the absolute path of undo-tree, either by looking in ~/.emacs.d/elpa/ or by looking at the value of load-path after undo-tree has been loaded (and using C-h v load-path).
  2. Reassign load-path to itself with undo-tree removed: (setq load-path (remove "/home/lorem/.emacs.d/elpa/undo-tree-0.6.5" load-path))

Altogether, that section of my init.el looks like:

(package-initialize)
(require 'package)
(add-to-list 'package-archives
         '("melpa" . "http://melpa.org/packages/") t)
(package-refresh-contents)

;; Don't load undo-tree. 
(setq load-path (remove "/home/lorem/.emacs.d/elpa/undo-tree-0.6.5" load-path))

Unfortunately, you can't just delete undo-tree altogether. Some of the navigation requires goto-chg.el which uses it. The top matter of evil.el states,

;; Evil requires goto-last-change' andgoto-last-change-reverse' ;; function for the corresponding motions g; g, as well as the ;; last-change-register `.'. One package providing these functions is ;; goto-chg.el: ;; ;; http://www.emacswiki.org/emacs/GotoChg ;; ;; Without this package the corresponding motions will raise an error.

I wonder whether one of the packages mentioned on the emacswiki, which don't rely on undo-tree,

could be used instead?

Hi-Angel commented 5 years ago 
;; Don't load undo-tree. It's the devil.
(setq load-path (remove "/home/lorem/.emacs.d/elpa/undo-tree-0.6.5" load-path))

What's your version of evil? This solution didn't work at least since January 2017. I tried your solution both with the older evil and the most recent one. Trying redo after applying the changes always results in Wrong type argument: commandp, redo.

FTR emacs stackexchange question about this bug has a similar answer, with the same effect.

dolorsitatem commented 5 years ago

I'm running evil 1.2.14. When I use C-r for redo, I also get the message Wrong type argument: commandp, redo. This is expected.

By default, C-r runs the undo-tree-redo command. Removing undo-tree from the load path means that the undo-tree-redo function is no longer available. The evil-normal-state-map references something that doesn't exist. Hence, we would anticipate some error (i.e. the message) and expect normal undo/redo Emacs behavior. And that's what we see.

Emacs doesn't have a native "redo" function, per se. It only has undo. To redo, you must "undo the undo". In practice this means:

  1. Press a key that breaks the undo cycle, such as <ESC> or j
  2. Call undo, by pressing u or C-/, to "undo the undo"

Admittedly, not loading undo-tree is kind of a kludge. But it does produce the desired behavior (i.e. replacing an unreliable undo/redo with a reliable one (via native Emacs undo)) as well as frees up a two-key combo :).

You can learn more about how Emacs undo works by looking at the undo documentation and this reddit post that has nice pictures.

TheBB commented 4 years ago

A new undo-tree release has been made (0.7), which looks like it goes some way toward mitigating errors people are seeing. Let me know if you experience problems with the new version.

Hi-Angel commented 4 years ago

It seems, ELPA haven't updated yet the package, so here's a few links so people wouldn't search them:

  1. undo-tree page with links to download at the bottom
  2. Blog post about finding what caused the corruption.
TheBB commented 4 years ago

It's definitely on ELPA: https://elpa.gnu.org/packages/undo-tree.html

Hi-Angel commented 4 years ago

It's definitely on ELPA: https://elpa.gnu.org/packages/undo-tree.html

Oh, indeed, this is odd. I'm still seeing latest version as 20170706.246 even when I package-list-packages from emacs -Q. Oh well, it's probably then something on my side.

npostavs commented 4 years ago

I'm still seeing latest version as 20170706.246

That sounds like a MELPA version. Not sure how, as undo-tree doesn't seem to be present in MELPA. Maybe it used to be?

tgbugs commented 4 years ago

@Hi-Angel In the list-packages view click on undo-tree and 0.7 should show up under Other versions:.

Alexander-Shukaev commented 4 years ago

Having learned the potential root causes from this blog post, I sparked a discussion about this new release regarding possible suggestions and further improvements <Lost or corrupted `undo-tree' history>. Feel free to join.

alienbogart commented 4 years ago

I am now using undo-fu and it's working well for me.

ideasman42 commented 4 years ago

I've recently made some updates to undo-fu, namely, ensuring redo doesn't undo after existing undo/redo commands.

I think this is suitable for evil-mode.

lockie commented 4 years ago

I think this is suitable for evil-mode.

undo-fu seems to lack saving history to file, so I don't think it is.

ideasman42 commented 4 years ago

There are existing packages that can save the undo history although I didn't try them out yet: see, https://stackoverflow.com/questions/2985050

Said differently, if this feature is important, it may be best to support via existing packages made for the purpose of restoring the session.

Stability is a feature too, over the years there have been many reports of bugs, this should be taken into account when considering alternatives to undo-tree, exactly how it's done - I'm not so fussed - undo-tree could be optional or there could be a way to switch out undo back-ends.

ideasman42 commented 4 years ago

I think this is suitable for evil-mode.

undo-fu seems to lack saving history to file, so I don't think it is.

Just looked into undohist, extended it a little and think this can support undo-fu without having to merge them into a single package. (link to repo)

ideasman42 commented 4 years ago

Update, undo-fu-session is now available on melpa for use with undo-fu.

condy0919 commented 4 years ago

Reconsider switching to undo-fu please.

doom has dropped undo-tree and switched to undo-fu https://github.com/hlissner/doom-emacs/issues/2339

lockie commented 4 years ago

Despite recent updates, undo-tree is still very unstable package (e.g. it yields random freezes on file saving and it prevents opening files with undo-tree-mapc: Wrong type argument: listp, \.\.\. error). Switching to undo-fu can actually be a good idea.

lawlist commented 4 years ago

@lockie -- have you by chance altered the settings for print-level and/or print-length that both have a default value of nil? The error message is most likely caused because the data was truncated with three dots ....

lockie commented 4 years ago

@lawlist thanks for the tip, but no, I haven't set those explicitly and, as describe-variable shows, they are both set to nil.

b3n commented 4 years ago

Please correct me if I am wrong, but my understanding is once we have undo-redo (https://github.com/emacs-mirror/emacs/blob/bbbab82a7117e08a77433f5ad39b34f5e03a014c/etc/NEWS#L77) Evil won't need to depend on undo-tree.

ideasman42 commented 4 years ago

@b3n right, although I'd argue evil could already make undo-tree optional, as undo-fu is an existing alternative to undo-tree.

alienbogart commented 4 years ago

@b3n right, although I'd argue evil could already make undo-tree optional, as undo-fu is an existing alternative to undo-tree.

I am now using Doom Emacs which comes with undo-fu by default. It presents no issues.

lockie commented 4 years ago

Can we please have the ability to set undo package in evil without resorting to Doom Emacs? As mentioned before, undo-tree is very unstable (matter of fact, it just froze my Emacs completely on file saving).

wedens commented 4 years ago

You don't really need to resort to doom, if you don't want to. Doom just disables global-undo-tree-mode and uses undo-fu.

You can put (global-undo-tree-mode -1) after (evil-mode) and use undo-fu.

I'd rather have undo-tree opt-in, but this workaround seems to work for now.

lockie commented 4 years ago

@wedens that did the trick, thanks!

alienbogart commented 4 years ago

You don't really need to resort to doom, if you don't want to. Doom just disables global-undo-tree-mode and uses undo-fu.

You can put (global-undo-tree-mode -1) after (evil-mode) and use undo-fu.

I'd rather have undo-tree opt-in, but this workaround seems to work for now.

That what I was doing before switching to Doom.

beyondpie commented 4 years ago

Hi guys, so finally we still cannot remove undo-tree (or set is as optional) in spacemacs?

wasamasa commented 4 years ago

Personally, I'd rather move to native undo-redo (as provided by the upcoming Emacs 28 release, backportable to 27.1 and probably earlier) than using undo-fu. In any case, a pluggable undo system is needed. I'm not sure how to do this in a non-breaking/-annoying way, here's some options:

Another problem I've noticed is that the existing integration of undo-tree is already pretty magic and does lots of key remapping. For example Evil binds C-r to redo (the relevant commit speaks of not directly using undo-tree commands) and undo-tree remaps redo to undo-tree-redo. I'd rather not trample over the elisp function namespace (there's a redo and redo+ package defining such a function), but doing any changes there would break any customizations doing similar tricks. I've searched GitHub and found a few users remapping other keys to point to undo and redo, to reach those I'd need to do something like Magit's nagging popups telling you to customize something so that it goes away (like said proposed customizable).

Feedback on this is welcome.

ideasman42 commented 4 years ago

@wasamasa agree using undo-redo seems a good/reasonable default. Then at least alternatives can be optionally supported.

If undo-redo had existed I might not have written undo-fu, however it still provides switching between linear/non-linear undo traversal in a way that's quite convenient, so there is still use in supporting it IMHO, even if it's using undo-redo under the hood.

Either of the first two suggestions seem fine. The second option (evil-undo-system) would cause the least complaints from existing users if it defaults to undo-tree.

Note that there is undo-fu-session, a standalone package for persistent undo that works with emacs built-in undo system (undo-redo and undo-fu).

wasamasa commented 4 years ago

@ideasman42 Please test https://github.com/emacs-evil/evil/pull/1360, it allows you to (setq evil-undo-system 'undo-fu).

ideasman42 commented 4 years ago

@wasamasa tested evil-undo-system:

wasamasa commented 4 years ago

:init is intentional, it's the same as nearly all customizables in this regard: Either customize before Evil has been loaded (that's what :init does) or use custom.el to customize at any time. Or use the evil-set-undo-system helper function directly, your choice.

I've run into that undo-tree error initially when fixing the test cases, but didn't have it when trying with my normal installation. I guess evil-set-undo-system requires some of the now deleted undo-tree integration code? In any case, I'll figure out what the issue with my installation is and hope the error manifests itself normally for me.

wasamasa commented 4 years ago

@ideasman42 See https://github.com/emacs-evil/evil/pull/1360#issuecomment-705443960 for my latest progress on this.

tsc25 commented 3 years ago

Undo-tree author+maintainer here. I turned Evil recently :) So might be able to help investigate this...

The "unrecognized entry in undo list undo-tree-canary" error when using undo-tree and Evil may due to a small number of evil-mode commands directly manipulating buffer-undo-list. You can't do that and just expect undo-tree-mode to make sense of it. In particular evil-paste-pop, maybe evil-repeat-pop mess around with buffer-undo-list in ways I'm not sure are safe. (A number of other evil-mode commands push new elements onto buffer-undo-list, but just pushing elements onto the list is safe.)

Undo-tree-mode puts the symbol 'undo-tree-canary at the very end of buffer-undo-list, to detect when Emacs GC has discarded undo history. (See this post for a more in-depth explanation.) Evil commands need to ensure they maintain the invariant that the final two elements of buffer-undo-list are always '(nil undo-tree-canary) when undo-tree-mode is enabled. Otherwise they are likely to break undo.

If anyone has a recipe to reproduce the error, starting from emacs -Q, I'd be happy to look into it further as and when I have time.

Toby

duianto commented 3 years ago

This seems to be a case where evil-repeat-pop causes the error:

Unrecognized entry in undo list undo-tree-canary

Reproduction steps:

With the evil and undo-tree packages installed.

emacs -q --load C:\Users\username\AppData\Roaming\.custom-init.el

(require 'package)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/") t)
(add-to-list 'package-archives '("org" . "https://orgmode.org/elpa/") t)

(setq ring-bell-function 'ignore)
(setq user-emacs-directory "C:/Users/username/AppData/Roaming/.custom.emacs.d")
(setq package-user-dir "C:/Users/username/AppData/Roaming/.custom.emacs.d/elpa/")

(setq-default frame-title-format '("Custom init.el"))

(package-initialize)

(require 'evil)
(evil-mode 1)

(evil-set-undo-system 'undo-tree)
(global-undo-tree-mode)
(add-hook 'evil-local-mode-hook 'turn-on-undo-tree-mode)

(message "Custom packages Loaded")

On startup the variable buffer-undo-list has the value:

(nil
 (409 . 798)
 (#("

" 0 1
(face variable-pitch help-echo "For information about GNU Emacs and the GNU system, type C-h C-a.")
1 2
(face variable-pitch help-echo "For information about GNU Emacs and the GNU system, type C-h C-a."))
  . 409)
 (3 . 412)
 (nil keymap nil 2 . 3)
 (nil rear-nonsticky nil 2 . 3)
 (nil display nil 2 . 3)
 (1 . 3)
 (t . 0))

Observed:

The minibuffer shows:

Unrecognized entry in undo list undo-tree-canary

When the steps above (from switch buffer...), are followed right after starting Emacs -q, without checking the buffer-undo-list first, then it has the value:

(nil undo-tree-canary nil
     (nil rear-nonsticky nil 147 . 148)
     (#("
" 0 1
(fontified nil))
      . -149)
     (147 . 150)
     146)

I don't know if it's helpful but if the buffer-undo-list variable is checked right after starting Emacs.

Then follow the steps above from (switch buffer...), now the buffer-undo-list variable has the value:

(nil . #1=(undo-tree-canary nil
                (nil rear-nonsticky nil 147 . 148)
                (#("
" 0 1
(fontified nil))
                 . -149)
                (147 . 150)
                146 nil . #1#))

System Info

evil-20210109.807
undo-tree-0.7.5
GNU Emacs 27.1 (build 1, x86_64-w64-mingw32) of 2020-08-21
Windows 10 Version 2004
tsc25 commented 3 years ago

@duianto You are a genius at coming up with these steps-to-reproduce! As always, this was really helpful. At least for this example, the problem is indeed evil-mode making assumptions about how buffer-undo-list behaves that are not valid in undo-tree-mode.

The problem here is buried within the evil-with-undo macro, and the evil-undo-pop function. But it's subtle.

The evil-with-undo macro executes its body with buffer-undo-list let-bound to nil, then nconc's any new undo entries that get generated onto the front of buffer-undo-list at the end of the macro. I don't fully understand at the moment why it does things this way. Surely prepending new undo entries onto the front of buffer-undo-list is what Emacs does anyway, so why bypass that mechanism and then do the same thing by hand? Ultimately, doing this is what causes the bug (see below). Is it just to temporarily enable undo in buffers where it's disabled? If so, there may be safer ways of doing this.

In any case, whatever the reason for it, at first sight this way of doing things might appear to be safe. As long as no undo-tree command is called, undo-tree-mode doesn't touch buffer-undo-list (or do anything at all, in fact - undo-tree-mode code only ever runs when an undo or redo command is called), and buffer-undo-list gets appended to exactly as in vanilla Emacs. Just with an extra undo-tree-canary entry at the very end; but that gets left intact by evil-with-undo's nconc, as required.

However, @duianto's sequence up to the C-. (evil-repeat-pop) leaves u (evil-undo) as the entry-before-last in evil-repeat-ring, and the C-. causes that undo operation to be repeated. So the above assumption is false: an undo-tree command does get called whilst inside evil-with-undo, namely undo-tree-undo (via repeating the evil-undo). undo-tree-undo sees the empty buffer-undo-list value let-bound by evil-with-undo, and adds an undo-tree-canary on the end of it, as it's supposed to when it encounters an empty buffer-undo-list. evil-with-undo then nconc's that onto the front of the real buffer-undo-list, which puts an undo-tree-canary value in the middle of buffer-undo-list (as well as the original one still at the end of buffer-undo-list).

The second C-. then calls evil-undo-pop, which call's Emacs vanilla undo function. That's definitely wrong anyway when in undo-tree-mode - it's very unlikely to work correctly, since undo takes its undo history from buffer-undo-list, whereas undo-tree-mode transfers the history to buffer-undo-tree whenever it gets an opportunity. (This could potentially account for other instances of corrupted undo history in evil-mode + undo-tree-mode.) But combined with the above evil-with-undo-induced bug, it results in undo trying to undo changes from the b0rked buffer-undo-list with undo-tree-canary in the middle, triggering the "Unrecognized entry" error.

A kludgy fix for this particular issue would be for evil-with-undo to strip any undo-tree-canary value it finds from evil-temporary-undo before nconc'ing it onto the front of buffer-undo-list. In (very rudimentary) testing, this at least prevents the error in this particular example. I'm not entirely certain if this kludge produces the correct behaviour, as I haven't wrapped my head around what the expected behaviour of the two sequential C-.'s is.

A slightly less kludgy fix might be to not short-circuit Emacs' undo system in evil-with-undo, and just let Emacs do its thing. But I'm not sure if this is viable, as I don't fully understand what evil-with-undo is supposed to do. In fact, at the moment, I don't see how that macro does anything useful at all. The docstring says "If undo is disabled in the current buffer, the undo information is stored in evil-temporary-undo instead of buffer-undo-list." But evil-temporary-undo is only set after the macro body has already been called. And then evil-temporary-undo gets set to null at the end of the macro, before any other code gets to see the value. The only effect of evil-with-undo I can see is to temporarily enable undo in the buffer if it's disabled (i.e. if buffer-undo-list is t), and then discard any undo information in that case. Probably I'm missing something here. I tried redefining evil-with-undo to be a no-op (i.e. just call body and return), and again in very rudimentary testing this at least seems to prevent the error in this particular example. However, it produces different behaviour to the other kludgy fix, above, so presumably one (or both!) fixes aren't producing the correct behaviour.

However, both of these "fixes" paper over the underlying problem. I strongly suspect there will be other cases that neither change will fix. The real problem this is that every evil-mode function and macro that directly manipulates buffer-undo-list is probably doing the wrong thing in undo-tree-mode. In undo-tree-mode, those functions should be interfacing with buffer-undo-tree. Even a cast-iron guarantee that no undo-tree commands can ever get invoked whilst they're manipulating buffer-undo-list isn't sufficient. Firstly, as @duianto's example shows, this is fragile and very difficult to guarantee in all edge cases. But, worse still, recent versions of undo-tree have a setting (not enabled by default) whereby undo history gets transferred to buffer-undo-tree by a timer function.

A proper fix may take more time and effort, as I don't understand every detail of what those evil-mode functions and macros are supposed to be doing, and the evil-mode maintainers presumably don't understand every nuance of how undo-tree-mode works. But identifying the problem is a start...

tsc25 commented 3 years ago

What about redefining evil-with-undo something like this:

(defmacro evil-with-undo (&rest body)
  "Execute BODY with enabled undo.
If undo is disabled in the current buffer, the undo information
is discarded."
  `(unwind-protect
       (let ((undo-disabled (eq buffer-undo-list t)))
         (when undo-disabled (setq buffer-undo-list nil))
         (unwind-protect
             (progn ,@body)
           (unless (null (car-safe buffer-undo-list))
             (push nil buffer-undo-list))))
     (when undo-disabled (setq buffer-undo-list t))))

(I took the opportunity to fix the docstring at the same time. The current docstring misleadingly implies body can find something useful in evil-temporary-undo, which I don't believe is true.)

Not sure yet if this fixes every evil-mode / undo-tree-mode compatibility issue. But it fixes @duianto's example by getting rid of the problematic nconc, and this ought to fix more than just this example. Does this break any evil-mode functionality? The above seems functionally equivalent to the current evil-with-undo implementation. Or am I missing something?