bug#22295: viper-mode undo bug introduced between Nov 10 and Nov 14

[Michael Kifer <kifer@cs.stonybrook.edu>]

On 05/14/2016 09:57 AM, Phillip Lord wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Date: Sat, 14 May 2016 12:25:13 +0300
>>> From: Eli Zaretskii <eliz@gnu.org>
>>> Cc: 22295@debbugs.gnu.org, Jim Meyering <jim@meyering.net>
>>>
>>>> From: Jim Meyering <jim@meyering.net>
>>>> Date: Sat, 2 Jan 2016 20:01:36 -0800
>>>>
>>>> Hello,
>>>> I noticed that viper-mode's "undo" ('u') command began to undo too much
>>>> and was able quickly to determine that it worked fine with my snapshot
>>>> built from git master some time on Nov 10, yet that it began to undo
>>>> too much four days later.
>>>>
>>>> To demonstrate the problem (without risking changing anything in your
>>>> home directory), run this:
>>>>
>>>>    mkdir /tmp/x && HOME=/tmp/x emacs -Q -f viper-mode -nw
>>>> ~/previously-nonexistent-file
>>>>
>>>> then respond "y", "y", "5" to get past the "viperize" setup questions.
>>>> To reproduce the error, insert two lines, terminating each "insertion" with ESC,
>>>> so that each is recorded as a separate undo'able operation. I.e., type this
>>>>
>>>>     a 1 ESC
>>>>
>>>> to create the first line, then
>>>>
>>>>    o 2 ESC
>>>>
>>>> to create the second.
>>>> Finally, hit "u" to undo creation of the second and you'll see that it undoes
>>>> both operations, erasing both lines.  This is rather disruptive when that first
>>>> bit of text was a long paragraph or two -- the novice may think that it's lost,
>>>> because redo does not restore it -- however, it is available in emacs's
>>>> yank buffer.
>>> Phillip, could you please look into this?  This sounds like a annoying
>>> problem for users of viper-mode, and AFAIU it happens on the release
>>> branch as well.
>> (Adding Michael to the addressees.)
>>
>> I took a short look, and it sounds like we need more experts here.
>> Undo in viper has its own implementation, which tries to do something
>> that is not immediately clear to me, and is not really documented
>> anywhere.  I guess vi users will know that, but I'm not one of them.
>>
>> The viper-undo command and related functions manipulate the Emacs undo
>> data structures directly, see viper-adjust-undo.  I guess the recent
>> changes in low-level undo implementation run afoul of what viper-mode
>> tries to do.
>>
>> I hope the above provides enough hints to find the reason for this
>> problem and solve it.
>>
>
> Sorry for slow response -- was travelling.
>
> Yep, viper is doing strange things to undo -- it adds a symbol ('viper)
> to the undo list, then removes it later, amalgamating everything upto
> 'viper.

I don't remember much myself, but the issue is this (and it is documented):

In VI, the granularity of undoing is much coarser than in Emacs. Several 
ops that Emacs undoes in multiple steps are supposed to be undone with
just one "undo" in VI. Viper simulates this by inserting 
viper-buffer-undo-list-mark onto buffer-undo-list to mark the point to 
which the Emacs undo's
are to be run in order to accomplish one VI-style undo. In Emacs, as I 
vaguely remember, this role is played by nil(?), but VI-style undos are 
coarser
than that, so viper requires its own marker on the unfo list.

Hope this helps.

Sorry that I don't have much time these days to work on viper.  :-(

--

        --- michael




>
> I've got a complete test case (below in case anyone is interested --
> I'll make a proper unit test of it on master eventually).
>
>
> On e0f64e7b4f9c3bbc12c4909ca8c8aa751f1fca4a (a random commit around
> the time of the error). This produced a undo list like so:
>
> (nil
>   (2 . 4)
>   nil
>   (2 . 3)
>   (1 . 2)
>   (t . -1))
>
> While on emacs-25 it gives (my comments):
>
>
> ((3 . 4) ;; insertion from 3 to 4
>   (2 . 3) ;; insertion from 2 to 3
>   nil     ;; boundary
>   (2 . 3) ;; insertion from 2 to 3
>   (1 . 2) ;; insertion from 1 to 2
>   (t . -1) ;; buffer created with file that does not exist
> )
>
>
> So it looks like the amalgamation that Emacs is supposed to be doing is
> failing. No idea at all where the head "nil" has gone.
>
> Will work on this more.
>
>
>
> (setq viper-inhibit-startup-message 't)
> (setq viper-expert-level '5)
>
> (find-file "/tmp/file.txt")
> (setq viper-mode t)
> (require 'viper)
>
> ;; leave emacs lisp in Emacs mode or edebug becomes impossible
> (setq viper-vi-state-mode-list (delq
>                                  'emacs-lisp-mode
>                                  viper-vi-state-mode-list))
>
> (setq viper-emacs-state-mode-list (cons
>                                  'emacs-lisp-mode
>                                  viper-vi-state-mode-list))
> ;; alas edebug "print last eval" is still broken
>
>
> (require 'edebug)
> (edebug-instrument-function 'viper-adjust-undo)
>
> ;; a 1
> (execute-kbd-macro
>     [?a ?1 escape ?o ?2 escape])