Assertion with undo steps that only move the cursor

[ideasman42]

Very ocasionally vundo is asserting:

cl--assertion-failed: Assertion failed: (not (and (consp buffer-undo-list) (null (car buffer-undo-list))))

I traced the problem down to undo steps that only move the cursor, (an undo step that contains a single integer representing the cursor position).

Here is a script to reproduce the issue:

(defun undo-test-only-positional-changes ()
  "Use this"
  (interactive)
  (dotimes (n 10)
    (push n buffer-undo-list)
    (push nil buffer-undo-list)))

(global-set-key (kbd "<f12>") 'undo-test-only-positional-changes)

Run this on a buffer with 11 characters or more, then undo (the cursor moves). Then try use vundo, it instantly asserts.

---

Having undo steps that only move the cursor position seems strange, it's possible this occurs because of logic in undo-fu-session that flattens the undo-history. Although attempting to create branching steps that are flattened - I can't force this to happen, so I'm not sure why this occurs. Nevertheless, since it seems like valid undo data that emacs undo supports, I think vundo could support it too.

[casouri]

Thanks for debugging, much appreciated! I figured out why this happens. Turns out Emacs doesn't add new undo records when undoing this sort of position-only records. Normally buffer modifications generate undo records that are pushed to buffer-undo-list, and undo operations themselves are modifications so they push undo records too. Therefore after vundo undo a record, it checks that the head of buffer-undo-list is not nil (because there should be a new record). But since undoing a position-only record doesn't push new records to buffer-undo-list, the head of buffer-undo-list remains nil, and assertion fails.

So my guess is that position undo records are really meant to be ancillary records that accompany other types of records. It makes sense that goto-char doesn't generate undo records so I don't thinkEmacs is doing anything unexpected.

I'll add some code that detects position-only records and not assert in that situation, many thanks!

[ideasman42]

Good to here this seems like something that can be supported.

Come to think of it I should have mentioned I disable undo amalgamation: (fset 'undo-auto-amalgamate 'ignore) which I suspect is the cause of these undo steps.

[casouri]

Actually, I spoke too soon. Vundo relies on the fact that undo pushes new records to buffer-undo-list to construct the tree, and when the undo operation doesn't push records, we are stuck on the node, unable to move forward or back. (I'm happy to explain the detail why if you want to know.) So right now there is no easy fix for this. We either 1) patch up buffer-undo-list by removing position-only records, or 2) pretend these records don't exist in buffer-undo-list, or 3) push dummy records into buffer-undo-list ourselves. 1) and 3) touches buffer-undo-list in ways I don't know if are safe, 2) requires changes in so many places scattered around the code base.

In terms of hygiene, I think 3) is the least bad. I can probably make it an optional feature. But before I move forward, is there anyway to prevent these position-only records from occurring in the first place? I'll also ask on emacs-devel. Thanks.

[ideasman42]

I think 2 would be fine, for example, primitive-undo could be wrapped by a version that detects these steps and performs multiple undoes in that case (stepping over them). I would guess this would resolve the issue in most cases. I'm not sure if there remains complication if vundo begins on one of these undo steps since it should be possible to restore the initial state.

[ideasman42]

This is a temporary workaround until the issue is resolved, all motion only undo steps are removed before running vundo:

(defun vundo-with-clear-impl (undo-list)
  "Remove motion only undo steps."
  (setq undo-list (cons nil undo-list))
  (let ((step-cdr undo-list))
    (while (and step-cdr (not (eq t step-cdr)))
      (let
        (
          (only-int t)
          (undo-elt t)
          (prev-cdr step-cdr))
        (while undo-elt
          (setq undo-elt (pop step-cdr))
          (when (and only-int undo-elt)
            (unless (integerp undo-elt)
              (setq only-int nil))))
        (when only-int
          (setcdr prev-cdr step-cdr)))))
  (cdr undo-list))

(defun vundo-with-clean (old-fn &rest args)
  "Clean up any motion only undo steps and open vundo."
  (setq buffer-undo-list (vundo-with-clear-impl buffer-undo-list))
  (call-interactively old-fn args))

(advice-add 'vundo :around #'vundo-with-clean)

[casouri]

I pushed a fix, could you give it a try? As you suggested, vundo now ignores position-only records, and these records are not remove from buffer-undo-list.

[ideasman42]

Great, I can confirm this works properly now.