Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove undo-tree dependency #1074

Closed
alienbogart opened this issue Jul 21, 2018 · 66 comments
Closed

Remove undo-tree dependency #1074

alienbogart opened this issue Jul 21, 2018 · 66 comments

Comments

@alienbogart
Copy link

alienbogart commented Jul 21, 2018

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

  • Start Emacs
  • Make 30 changes on a files an save each step
  • Restart Emacs
  • Try undoing and redoing those changes

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
syl20bnr/spacemacs#298
syl20bnr/spacemacs#9903

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

@npostavs
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

noctuid commented Oct 15, 2018

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
Copy link

lawlist commented Oct 16, 2018

(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
Copy link
Contributor

noctuid commented Oct 16, 2018

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
Copy link

lawlist commented Oct 16, 2018

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
Copy link

tgbugs commented Nov 12, 2018

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
Copy link

AitBits commented Nov 29, 2018

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
Copy link

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
Copy link
Member

TheBB commented Jan 23, 2019

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

@lawlist
Copy link

lawlist commented Jan 23, 2019

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

@dolorsitatem
Copy link

dolorsitatem commented Feb 14, 2019

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' and goto-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
Copy link
Contributor

Hi-Angel commented Mar 6, 2019

;; 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
Copy link

dolorsitatem commented Mar 7, 2019

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.

mbakke pushed a commit to guix-mirror/guix that referenced this issue Oct 29, 2019
Because it is buggy and unnecessary.  See
<emacs-evil/evil#1074>.

* gnu/packages/emacs-xyz.scm (emacs-evil)[propagated-inputs]: Remove
emacs-undo-tree and emacs-goto-chg.
@TheBB
Copy link
Member

TheBB commented Jan 8, 2020

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
Copy link
Contributor

Hi-Angel commented Jan 8, 2020

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
Copy link
Member

TheBB commented Jan 8, 2020

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

@Hi-Angel
Copy link
Contributor

Hi-Angel commented Jan 8, 2020

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
Copy link

npostavs commented Jan 8, 2020

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
Copy link

tgbugs commented Jan 8, 2020

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

@Alexander-Shukaev
Copy link
Contributor

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
Copy link
Author

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

@ideasman42
Copy link

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
Copy link

lockie commented Jan 13, 2020

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
Copy link

ideasman42 commented Jan 13, 2020

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.

@tsc25
Copy link
Contributor

tsc25 commented Jan 27, 2021

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
Copy link
Contributor

duianto commented Feb 14, 2021

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))
  • switch buffer: C-x b
    a prompt appears: Switch to buffer (default *scratch*):
    Confirm: RET
  • enter insert state: i
  • type: a
  • exit to normal state: ESC
  • copy the line: yy
  • paste: p
  • undo: u
  • paste: p
  • repeat: . (press period)
  • repeat pop: C-. (press Ctrl and period)
  • repeat pop: C-.

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.

  • C-h v
  • type: buffer undo list (Emacs fills in the dashes between the words automatically)
  • RET
  • switch to the *Help* buffer: C-w j (or C-x o)
  • Scroll down: C-f (repeat 4 times to scroll to the bottom)

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
Copy link
Contributor

tsc25 commented Feb 15, 2021

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

omajid added a commit to omajid/config that referenced this issue Feb 16, 2021
undo-tree has lots of subtle issues:
emacs-evil/evil#1074. Try and work around
that by using the simpler undo-fu package. There's no tree
visualization here, though.
@tsc25
Copy link
Contributor

tsc25 commented Feb 16, 2021

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?

@duianto
Copy link
Contributor

duianto commented Feb 16, 2021

I haven't wrapped my head around what the expected behaviour of the two sequential C-.'s is.

I'm not sure what's expected either.

The reproduction steps were just reduced to those, after reading your comment that evil-repeat-pop could be a possible source, and encountering the error message while testing it.


The last line in the fix is outside the let.

(when undo-disabled (setq buffer-undo-list t))

Because pasting shows:

unwind-protect: Symbol’s value as variable is void: undo-disabled

Moving it inside the let fixes that void issue, but the reproduction steps error remains:

primitive-undo: Unrecognized entry in undo list undo-tree-canary

However now the buffer-undo-list says:

Its value is (nil undo-tree-canary)

@tsc25
Copy link
Contributor

tsc25 commented Feb 16, 2021

The last line is intentionally outside the let, so it gets called from the unwind-protect. If body throws an error, we still want it to clean up and reset buffer-undo-tree correctly. It's the let that should be outside the unwind-protect, I think:

(defmacro evil-with-undo (&rest body)
  "Execute BODY with enabled undo.
If undo is disabled in the current buffer, the undo information
is discarded."
  `(let ((undo-disabled (eq buffer-undo-list t)))
     (unwind-protect
         (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))))

This fixes the error in your example for me. I wonder if the new code is being called in your testing? Because evil-with-undo is a macro that gets invoked by a whole chain of macros, you need to eval or compile the new macro, then re-eval (in this order) evil-with-single-undo and evil-repeat to ensure the new version of evil-with-undo gets used. (Forking evil, updating evil-with-undo and recompiling everything might be simpler!)

@duianto
Copy link
Contributor

duianto commented Feb 16, 2021

The last line is intentionally outside the let, so it gets called from the unwind-protect.

👍

The latest version doesn't show the void message.


I replaced the evil-with-undo macro in:

(defmacro evil-with-undo (&rest body)

Removed all .elc files from:
C:\Users\username\AppData\Roaming\.custom.emacs.d\elpa\evil-20210109.807

And restarted Emacs.

The message remains:

primitive-undo: Unrecognized entry in undo list undo-tree-canary

It didn't help to recompile the evil directory:
C-u M-x byte-recompile-directory

Note:

Adding C-u 0 before M-x byte-recompile-directory,
recompiles all files without asking for confirmation on every file.

source:
the comment: https://stackoverflow.com/a/1217249
in: https://stackoverflow.com/questions/1217180/how-do-i-byte-compile-everything-in-my-emacs-d-directory

@duianto
Copy link
Contributor

duianto commented Feb 16, 2021

It doesn't seem to be related to my custom setup.
The Unrecognized message also appears with the new macro, in Manjaro with emacs in the default location: /home/username/.emacs.d/

And without the call to the local mode hook.
(add-hook 'evil-local-mode-hook 'turn-on-undo-tree-mode)

It isn't needed in the scratch buffer.

But undo-tree doesn't become enabled in a new buffer without it:
C-x b test RET

It's suggested in this comment: #1382 (comment)
in the issue:
u and C-r doesn't work in new buffer with undo-tree as evil-undo-system #1382

.emacs

(require 'package)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/") t)
;; Comment/uncomment this line to enable MELPA Stable if desired.  See `package-archive-priorities`
;; and `package-pinned-packages`. Most users will not need or want to do this.
;;(add-to-list 'package-archives '("melpa-stable" . "https://stable.melpa.org/packages/") t)
(package-initialize)


(require 'undo-tree)
(global-undo-tree-mode)

(require 'evil)
(evil-mode 1)
(evil-set-undo-system 'undo-tree)

GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3) of 2020-08-28

@tsc25
Copy link
Contributor

tsc25 commented Feb 16, 2021

Thanks for your efforts @duianto!

Would be good to double-check the replacement macro is being called. I think just recompiling everything won't work, because when evil-common gets recompiled, its version of evil-with-undo will clobber the replacement version, and evil-repeat will use that one when it gets compiled.

As a more fail-safe test, try saving the following in a file, say, "evil-with-undo-test.el", then byte-compile and load it:

(defmacro evil-with-undo (&rest body)
  "Execute BODY with enabled undo.
    If undo is disabled in the current buffer, the undo information
    is discarded."
  `(let ((undo-disabled (eq buffer-undo-list t)))
     (message "In replacement `evil-with-undo'")
     (unwind-protect
         (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))))

(defmacro evil-with-single-undo (&rest body)
  "Execute BODY as a single undo step."
  (declare (indent defun)
           (debug t))
  `(let (evil-undo-list-pointer)
     (evil-with-undo
       (unwind-protect
           (progn
             (evil-start-undo-step)
             (let ((evil-in-single-undo t))
               ,@body))
         (evil-end-undo-step)))))

(defun evil-repeat (count &optional save-point)
  "Repeat the last editing command with count replaced by COUNT.
If SAVE-POINT is non-nil, do not move point."
  (interactive (list current-prefix-arg
                     (not evil-repeat-move-cursor)))
  (cond
   ((null evil-repeat-ring)
    (error "Already executing repeat"))
   (save-point
    (save-excursion
      (evil-repeat count)))
   (t
    (unwind-protect
        (let ((confirm-kill-emacs t)
              (kill-buffer-hook
               (cons #'(lambda ()
                         (user-error "Cannot delete buffer in repeat command"))
                     kill-buffer-hook))
              (undo-pointer buffer-undo-list))
          (evil-with-single-undo
            (setq evil-last-repeat (list (point) count undo-pointer))
            (evil-execute-repeat-info-with-count
             count (ring-ref evil-repeat-ring 0))))
      (evil-normal-state)))))

When you run your recipe for reproducing the error, you should see the "In replacement `evil-with-undo'" message in the *Messages* buffer. If it's not there, it means the new macro isn't being used.

If it is there, confirming the new macro is being used, and you still see the error, then I'll have to figure out how to reproduce this new case. Just tested again following the above procedure, and the error is definitely gone on my system.

@tsc25
Copy link
Contributor

tsc25 commented Feb 16, 2021

Ah, occurs to me that I had also made some local changes to undo-tree.el whilst testing yesterday. I just pushed these to Gitlab, in case they make a difference here. (undo-tree.el version number 0.8.1).

@duianto
Copy link
Contributor

duianto commented Feb 16, 2021

I copied the contents of the current file: undo-tree.el (0.8.1)
https://gitlab.com/tsc25/undo-tree/-/blob/cbe0c708d8a71a521199bd8e3e605c760ecdb021/undo-tree.el

Overwrote the local file:
C:\Users\username\AppData\Roaming\.custom.emacs.d\elpa\undo-tree-0.7.5\undo-tree.el

Removed: undo-tree.elc

Created a new file in:
C:\Users\username\AppData\Roaming\.custom.emacs.d\evil-with-undo-test.el
with the versions of the three functions above #1074 (comment):

evil-with-undo
evil-with-single-undo
evil-repeat

Started Emacs.

It complained that it couldn't find queue.
Checked the difference between the previous and current undo-tree.el

Saw that it now says:

;; Package-Requires: ((queue "0.2"))

Installed the package: M-x package-install queue RET

Restarted Emacs.

M-x load-file C:\Users\username\AppData\Roaming\.custom.emacs.d\evil-with-undo-test.el RET

C-h f evil-with-undo RET
shows:

evil-with-undo is a Lisp macro in ‘../../evil-with-undo-test.el’.

(evil-with-undo &rest BODY)

Execute BODY with enabled undo.
    If undo is disabled in the current buffer, the undo information
    is discarded.

[back]

When following the reproduction steps:
Pressing . (period) shows:

In replacement ‘evil-with-undo’

The first C-. shows:

Undo
In replacement ‘evil-with-undo’
Undo branch point!

The second C-. shows:

primitive-undo: Unrecognized entry in undo list undo-tree-canary

The buffer-undo-list has the value:

Its value is (nil undo-tree-canary)

The same thing is observed with or without calling byte-compile-file for the files:
undo-tree.el
evil-with-undo-test.el

@tsc25
Copy link
Contributor

tsc25 commented Feb 16, 2021

Thanks @duianto - I can reproduce this now.

The macro change fixed one bug: buffer-undo-list isn't getting corrupted anymore. Now it hits the second bug I described in my long message above: evil-undo-pop (which gets called during evil-repeat-pop) tries to undo the last buffer modification by calling the vanilla Emacs undo function. Which is simply wrong in undo-tree-mode, since that undo history has been transferred to buffer-undo-tree.

To fix this one, I'll have to figure out what evil-undo-pop is doing, and undo the appropriate changes using undo-tree-mode commands instead of vanilla Emacs undo...

@Hi-Angel
Copy link
Contributor

The macro change fixed one bug: buffer-undo-list isn't getting corrupted anymore. Now it hits the second bug I described in my long message above: evil-undo-pop (which gets called during evil-repeat-pop) tries to undo the last buffer modification by calling the vanilla Emacs undo function. Which is simply wrong in undo-tree-mode, since that undo history has been transferred to buffer-undo-tree.

Hmm, on a side note, I think it might be worth making undo-tree warn about that or something… The thing is, I don't think evil-mode is the only one who might try to "vanilla-undo". In fact, right now as I'm writing the text, I made use of C+/ which calls vanilla undo, to remove last word with a typo without entering "insert-mode".

I realize I gotta rebind the combination to the appropriate function of undo-tree, but my point is that there might be other users of the vanilla undo who wouldn't know about it, and they will likely get hit by such conflict between the two undo systems.

@Hi-Angel
Copy link
Contributor

I realize I gotta rebind the combination to the appropriate function of undo-tree, but my point is that there might be other users of the vanilla undo who wouldn't know about it, and they will likely get hit by such conflict between the two undo systems.

Perhaps undo-tree mode could override the undo function?

@tsc25
Copy link
Contributor

tsc25 commented Feb 17, 2021

I realize I gotta rebind the combination to the appropriate function of undo-tree, but my point is that there might be other users of the vanilla undo who wouldn't know about it, and they will likely get hit by such conflict between the two undo systems.

You don't have to rebind anything to use undo-tree: it installs a command remapping from undo (and various others) to the appropriate undo-tree functions. Any interactive calls to undo (and related commands) in undo-tree-mode will do the right thing. (See command remapping in the Elisp manual. Emacs has had this mechanism since before the undo-tree package existed.)

Perhaps undo-tree mode could override the undo function?

It already does override it, and always has done, using the appropriate Emacs mechanism (command remapping). The undo-tree package can't redefine the undo function itself, even if that was a sensible thing to do in a package (it isn't). The undo function is still needed in buffers where undo-tree-mode is not enabled, and potentially by low-level Emacs code.

The only(*) way to call the vanilla undo function in undo-tree-mode is from Elisp code. And Elisp code is expected to know what it's doing: there are a gazillion ways to shoot oneself in the foot from Elisp!

(*) Unless you go to strenuous efforts to bind undo in a keymap that overrides minor-mode maps. And that also requires Elisp.

To be clear, Evil's evil-undo and evil-redo commands already call the correct functions, depending on the setting of evil-undo-system. The bug is due to Evil calling undo directly from a low-level, auxiliary function (evil-undo-pop). That will only work correctly with the vanilla Emacs undo system; doing it in undo-tree-mode is a bug, and the Evil code ought to call the appropriate function here depending on the setting of evil-undo-system, as it does with the evil-undo command.

But the buggy implementation will work OK most of the time. It takes a subtle combination of circumstances to trigger the bug: you need an undo-tree command to get called just before evil-undo-pop is invoked. Normally, undo-tree commands only get called when the user calls them interactively (via evil-undo or evil-redo), and evil-undo-pop doesn't get called in that case. @duianto's recipe gets evil-undo into the Evil repeat ring, which sets it up to get called implicitly in the middle of processing the evil-repeat-pop action.

It's "just" a bug that needs fixing in evil-mode.

@Hi-Angel
Copy link
Contributor

It already does override it, and always has done, using the appropriate Emacs mechanism (command remapping). The undo-tree package can't redefine the undo function itself, even if that was a sensible thing to do in a package (it isn't). The undo function is still needed in buffers where undo-tree-mode is not enabled, and potentially by low-level Emacs code.

The only(*) way to call the vanilla undo function in undo-tree-mode is from Elisp code, as Evil is doing. And Elisp code is expected to know what it's doing: there are a gazillion ways to shoot oneself in the foot from Elisp!

Ah, I see, thank you for elaboration

@tsc25
Copy link
Contributor

tsc25 commented Feb 17, 2021

To fix this one, I'll have to figure out what evil-undo-pop is doing, and undo the appropriate changes using undo-tree-mode commands instead of vanilla Emacs undo.

OK, I think this might be as simple as making undo-tree-pop just call undo-tree-undo when undo-tree is used (together with the evil-with-undo macro fix from above).

Same proviso as before: I'm not certain what the expected behaviour should be. But loading the following code at least makes the "Unrecognized entry" error go away for me in @duianto's example:

(defmacro evil-with-undo (&rest body)
  "Execute BODY with enabled undo.
        If undo is disabled in the current buffer, the undo information
        is discarded."
  `(let ((undo-disabled (eq buffer-undo-list t)))
     (unwind-protect
         (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))))

(defmacro evil-with-single-undo (&rest body)
  "Execute BODY as a single undo step."
  (declare (indent defun)
           (debug t))
  `(let (evil-undo-list-pointer)
     (evil-with-undo
       (unwind-protect
           (progn
             (evil-start-undo-step)
             (let ((evil-in-single-undo t))
               ,@body))
         (evil-end-undo-step)))))

(defun evil-repeat (count &optional save-point)
  "Repeat the last editing command with count replaced by COUNT.
    If SAVE-POINT is non-nil, do not move point."
  (interactive (list current-prefix-arg
                     (not evil-repeat-move-cursor)))
  (cond
   ((null evil-repeat-ring)
    (error "Already executing repeat"))
   (save-point
    (save-excursion
      (evil-repeat count)))
   (t
    (unwind-protect
        (let ((confirm-kill-emacs t)
              (kill-buffer-hook
               (cons #'(lambda ()
                         (user-error "Cannot delete buffer in repeat command"))
                     kill-buffer-hook))
              (undo-pointer buffer-undo-list))
          (evil-with-single-undo
            (setq evil-last-repeat (list (point) count undo-pointer))
            (evil-execute-repeat-info-with-count
             count (ring-ref evil-repeat-ring 0))))
      (evil-normal-state)))))

(defun evil-repeat-pop (count &optional save-point)
  "Replace the just repeated command with a previously executed command.
Only allowed after `evil-repeat', `evil-repeat-pop' or
`evil-repeat-pop-next'. Uses the same repeat count that
was used for the first repeat.

The COUNT argument inserts the COUNT-th previous kill.
If COUNT is negative, this is a more recent kill."
  (interactive (list (prefix-numeric-value current-prefix-arg)
                     (not evil-repeat-move-cursor)))
  (cond
   ((not (and (eq last-command #'evil-repeat)
              evil-last-repeat))
    (user-error "Previous command was not evil-repeat: %s" last-command))
   (save-point
    (save-excursion
      (evil-repeat-pop count)))
   (t
    (unless (eq buffer-undo-list (nth 2 evil-last-repeat))
      (evil-undo-pop))
    (goto-char (car evil-last-repeat))
    ;; rotate the repeat-ring
    (while (> count 0)
      (when evil-repeat-ring
        (ring-insert-at-beginning evil-repeat-ring
                                  (ring-remove evil-repeat-ring 0)))
      (setq count (1- count)))
    (while (< count 0)
      (when evil-repeat-ring
        (ring-insert evil-repeat-ring
                     (ring-remove evil-repeat-ring)))
      (setq count (1+ count)))
    (setq this-command #'evil-repeat)
    (evil-repeat (cadr evil-last-repeat)))))


(defun evil-undo-pop ()
  "Undo the last buffer change.
Removes the last undo information from `buffer-undo-list'.
If undo is disabled in the current buffer, use the information
in `evil-temporary-undo' instead."
  (if (and (eq evil-undo-system 'undo-tree)
           (not (eq buffer-undo-list t)))
      (undo-tree-undo)
    (let ((paste-undo (list nil)))
      (let ((undo-list (if (eq buffer-undo-list t)
                           evil-temporary-undo
                         buffer-undo-list)))
        (when (or (not undo-list) (car undo-list))
          (user-error "Can't undo previous change"))
        (while (and undo-list (null (car undo-list)))
          (pop undo-list)) ; remove nil
        (while (and undo-list (car undo-list))
          (push (pop undo-list) paste-undo))
        (let ((buffer-undo-list (nreverse paste-undo)))
          (evil-save-echo-area
            (undo)))
        (if (eq buffer-undo-list t)
            (setq evil-temporary-undo nil)
          (setq buffer-undo-list undo-list))))))

@duianto
Copy link
Contributor

duianto commented Feb 19, 2021

I'm still getting the Unrecognized message after the second C-.
In both Manjaro and Windows.
With the latest code block.

buffer-undo-list is a variable defined in ‘C source code’.
Its value is (nil undo-tree-canary)
Local in buffer *scratch*; global value is nil

The contents of evil-with-undo-test.el was replaced, with the latest code block above:
#1074 (comment)

C:\Users\username\AppData\Roaming\.custom.emacs.d\elpa\undo-tree-0.7.5\undo-tree.el
is still overwritten with the contents of:
https://gitlab.com/tsc25/undo-tree/-/blob/master/undo-tree.el
;; Version: 0.8.1

The file was loaded on Emacs startup:
M-x load-file C:\Users\username\AppData\Roaming\.custom.emacs.d\evil-with-undo-test.el RET

C-h f evil-with-undo shows:

evil-with-undo is a Lisp macro in ‘../../evil-with-undo-test.el’.

(evil-with-undo &rest BODY)

Execute BODY with enabled undo.
        If undo is disabled in the current buffer, the undo information
        is discarded.

[back]

@tsc25
Copy link
Contributor

tsc25 commented Feb 20, 2021

@duianto I think this is because you're not setting the evil-undo-system variable in your test setup. Normally this variable is set via customize, which calls evil-set-undo-system to set the undo/redo functions to undo-tree's. Calling the evil-set-undo-system function directly doesn't set the evil-undo-system variable itself.

Could you try again, adding

(setq evil-undo-system 'undo-tree)

immediately before the evil-set-undo-system line in your .custom-init.el?

@duianto
Copy link
Contributor

duianto commented Feb 20, 2021

Confirmed.

With only: (evil-set-undo-system 'undo-tree) in the init.el file.
The variable evil-undo-system had the value nil on startup.

With:

(setq evil-undo-system 'undo-tree)
(evil-set-undo-system 'undo-tree)

or just this single line:

(custom-set-variables '(evil-undo-system 'undo-tree))

source:
u and C-r doesn't work in new buffer with undo-tree as evil-undo-system #1382

Now there is no Unrecognized entry message when pressing C-..

Continuing to press C-. removes the initial scratch buffer text

;; This buffer is for text that is not saved, ...

And after the fourth time C-. is pressed (six in total).

Then the minibuffer shows:

No further undo information

Which most likely is expected.

tsc25 added a commit to tsc25/evil that referenced this issue Mar 6, 2021
Addresses emacs-evil#1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue emacs-evil#1074 referenced
  above). Since evil-temporary-undo was always nil when executing @Body
  anyway, and reset to nil at end, other elisp code could never see a non-nil
  value for it anyway here. So we fix the nconc bug by simplifying the code,
  and getting rid of the redundant evil-temporary-undo juggling in the
  process.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is almost always a bug
  in undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.
tsc25 added a commit to tsc25/evil that referenced this issue Mar 6, 2021
Addresses emacs-evil#1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue emacs-evil#1074). Leave
  standard undo machinery to work as usual when undo is enabled. Deal with
  disabled undo by temporarily enabling then disabling undo, and transferring
  any undo changes to evil-temporary-undo.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is wrong when in
  undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.
axelf4 added a commit to axelf4/evil that referenced this issue Jan 5, 2023
Addresses emacs-evil#1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue emacs-evil#1074). Leave
  standard undo machinery to work as usual when undo is enabled. Deal with
  disabled undo by temporarily enabling then disabling undo, and transferring
  any undo changes to evil-temporary-undo.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is wrong when in
  undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.

Co-authored-by: Axel Forsman <axelsfor@gmail.com>
axelf4 added a commit to tsc25/evil that referenced this issue Jan 7, 2023
Addresses emacs-evil#1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue emacs-evil#1074). Leave
  standard undo machinery to work as usual when undo is enabled. Deal with
  disabled undo by temporarily enabling then disabling undo, and transferring
  any undo changes to evil-temporary-undo.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is wrong when in
  undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.

Co-authored-by: Axel Forsman <axelsfor@gmail.com>
axelf4 added a commit to tsc25/evil that referenced this issue Jan 7, 2023
Addresses emacs-evil#1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue emacs-evil#1074). Leave
  standard undo machinery to work as usual when undo is enabled. Deal with
  disabled undo by temporarily enabling then disabling undo, and transferring
  any undo changes to evil-temporary-undo.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is wrong when in
  undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.

Co-authored-by: Axel Forsman <axelsfor@gmail.com>
axelf4 added a commit that referenced this issue Jan 7, 2023
Addresses #1074

- evil-with-undo:
  nconc'ing onto front of buffer-undo-list here can corrupt buffer-undo-list
  when in undo-tree-mode in rare circumstances (see issue #1074). Leave
  standard undo machinery to work as usual when undo is enabled. Deal with
  disabled undo by temporarily enabling then disabling undo, and transferring
  any undo changes to evil-temporary-undo.

- evil-undo-pop:
  This function called `undo' directly from Elisp, which is wrong when in
  undo-tree-mode. Fix this by calling undo-tree-undo instead when in
  undo-tree-mode.

Co-authored-by: Axel Forsman <axelsfor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests