Fix bugs in `evil-with-undo' and `evil-undo-pop' with undo-tree.

[tsc25]

trafficstars

Addresses https://github.com/emacs-evil/evil/issues/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.

[tomdl89]

@tsc25 do you intend to fix the broken tests on this? I'll probably close the PR in a few weeks if not. Thanks

[tsc25]

@tomdl89 I was waiting on someone who understands evil-mode better than I do to give some input.

The evil ert tests aren't all that clearly documented (at least to an outsider). And they're particularly hard to step through to figure out what they're supposed to be testing, because they're pre-canned sequences of characters, rather than calls to the corresponding evil-mode functions. I put some time and effort into rewriting this patch to get all but that one final ert test to pass. Unfortunately, I have no idea why that final test is failing, or what it's supposed to be testing. This needs input from someone who knows the evil-mode implementation better than me. I'm not an evil-mode developer, I'm just the undo-tree author and maintainer trying to help you out here.

Closing this PR seems...short-sighted. This is a pretty significant bug in evil-mode that has been complained about for many years by quite a few users (see e.g. #1074 and myriad reddit posts and similar reports around the web). This issue can't be fixed in undo-tree, because the bug is in evil-mode code (see #1074 for a fuller discussion). There simply isn't anywhere I could put a fix for this in the undo-tree package.

Wouldn't it be better to find an evil-mode developer who can help out with figuring out what that final test is about and refactoring this PR to get it to pass? I'm happy to help explain the undo-tree side, but I hit the limit of what I could figure out myself about the buggy evil-mode implementation at that final ert test.

[tomdl89]

Hey @tsc25 thanks for your lengthy and quick reply. Worry not - I won't close this as long as someone is engaged with the problem. I'll take a look to see if I can understand the bug and your approach, and if not I may leave you a question or two here. We'll get the tests passing - they're actually quite intuitive once you get to know them. Cheers

[axelf4]

I made some changes to this commit to fix the failing tests + a couple other things (Remove nested unwind-protect, ensure evil does not use undo-tree when undo info is going into evil-temporary-undo since undo-tree should be disabled when buffer-undo-list is t, or the undo-list-transfer-to-tree timer would error AFAICS (?).) and pushed it to my fork. I think the issue was that a duplicate undo boundary was added to evil-temporary-undo unlike what was expected by evil-undo-pop/etc.