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

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

[Jim Meyering]

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.

Thanks for tending emacs,

Jim

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

[Eli Zaretskii]

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

TIA

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

[Eli Zaretskii]

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

Thanks.

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

[Phillip Lord]

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'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])

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

[Michael Kifer]

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])

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:

> 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>
>>
>> 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):

It's commented rather 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.

Yes, you are correct -- boundaries are nil.

Viper puts it's own mark in, yes, but then deletes it later on and
replaces it with nil in this bit of code.


(setq tmp2 (cdr tmp)) ; the part after mark

;; cut tail from buffer-undo-list temporarily by direct
;; manipulation with pointers in buffer-undo-list
(setcdr tmp nil)

(setq buffer-undo-list (delq nil buffer-undo-list))
(setq buffer-undo-list
      (delq viper-buffer-undo-list-mark buffer-undo-list))
;; restore tail of buffer-undo-list
(setq buffer-undo-list (nconc buffer-undo-list tmp2)))

Essentially, it just kills the standard boundary handling and does it's
own thing.


> Hope this helps.

You don't by any chance remember why viper mode appears to turns itself
off in noninteractive mode? Turns out to be rather painful for testing.


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

Too busy doing all logic and semantics?

Phil

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 4578 bytes --]

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

[Michael Albinus]

phillip.lord@russet.org.uk (Phillip Lord) writes:

> You don't by any chance remember why viper mode appears to turns itself
> off in noninteractive mode? Turns out to be rather painful for testing.

You could embed your test code like this:

(let (noninteractive)
  (run-your-tests))

> Phil

Best regards, Michael.

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

[Jim Meyering]

On Sat, May 14, 2016 at 6:57 AM, Phillip Lord
<phillip.lord@russet.org.uk> 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've got a complete test case (below in case anyone is interested --
> I'll make a proper unit test of it on master eventually).

Thank you for working on this. My fingers seem to have developed a
serious dependence on viper-mode over the last two decades.
I don't want to retrain them :-)

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:
> Right, it has to do its own marking. When something gets viper-undone,
> the latest viper-buffer-undo-list-mark is deleted and is replaced with
> a nil, so that things can be further viper-undone.
>
> Hope this helps.


It does yes. I am working on a solution; I think that the undo changes
mean that this should be easier to implement.



> You don't by any chance remember why viper mode appears to turns itself
> off in noninteractive mode? Turns out to be rather painful for testing.
>
> I am not sure what you are referring to here.

Consider this:

(defcustom viper-mode (cond (noninteractive nil)
			    (t 'ask))
                            .....
                            

And the viper-mode function...

(defun viper-mode ()
  "Turn on Viper emulation of Vi in Emacs. See Info node `(viper)Top'."
  (interactive)
  (if (not noninteractive)
      (progn
         'actually-turn-viper-mode-on.....)
  (if (eq major-mode 'viper-mode)
      (setq major-mode 'fundamental-mode))
  )

I normally run tests in batch, but viper automatically switches itself
off. It's going to make testing viper essentially impossible without
working around it (easy to do, but it would be good to understand why
it's like this).

Phil

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

[Phillip Lord]

Michael Albinus <michael.albinus@gmx.de> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> You don't by any chance remember why viper mode appears to turns itself
>> off in noninteractive mode? Turns out to be rather painful for testing.
>
> You could embed your test code like this:
>
> (let (noninteractive)
>   (run-your-tests))


Yeah, been doing that. Ugly, though, it's not necessary.

The only reason I can think is for people who do lots of "--batch" jobs
and don't want viper to interfere. But, it shouldn't, really, and my
guess is that most people do "emacs --batch -q -l blah.el" for this use.

Phil

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

[Phillip Lord]

Jim Meyering <jim@meyering.net> writes:

>> 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've got a complete test case (below in case anyone is interested --
>> I'll make a proper unit test of it on master eventually).
>
> Thank you for working on this. My fingers seem to have developed a
> serious dependence on viper-mode over the last two decades.
> I don't want to retrain them :-)

It's okay. I don't want Eli forwarding bug reports once emacs-25 comes
out!

I have pushed a first attempt at a fix to branch fix/viper-undo.
Clearly, it's not finished yet, but I'd like to get your feedback as to
whether it works; it should fix the example given, but I haven't used
viper at all, so if it breaks other things I won't know.

I haven't managed to work out exactly why the error is happening --
probably though because the timing of the undo-boundary is somewhat
different, though. With this fix, viper just disables automatic boundary
addition and adds it's own as necessary, which seems cleaner.

Phil

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

[Jim Meyering]

On Mon, May 16, 2016 at 5:41 AM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
...
> I have pushed a first attempt at a fix to branch fix/viper-undo.
> Clearly, it's not finished yet, but I'd like to get your feedback as to
> whether it works; it should fix the example given, but I haven't used
> viper at all, so if it breaks other things I won't know.
>
> I haven't managed to work out exactly why the error is happening --
> probably though because the timing of the undo-boundary is somewhat
> different, though. With this fix, viper just disables automatic boundary
> addition and adds it's own as necessary, which seems cleaner.

Thank you.
That does indeed fix the case I mentioned.
Here are some cases where it does not work as expected:

  start with an empty buffer in viper-mode
  type 'i 1 2 3 4 5ESC'
  type 'F2dw' to delete the 2 and a space.
  type 'wdw' to delete the 4 and a space.

Now, if I were to hit "u" to undo, I would expect that most recent
deletion to be undone and the 4 would reappear.
Then I would hit '.' to undo the deletion of the '2'. Finally one more
'.' would undo the creation of that first line.
However, with the current patches, that first 'u' undoes everything
and leaves me with the empty initial file.

Another example starting with an empty file:
Create some content via ':r!seq 999|fmt RETURN'
Then remove e.g., "222 " and "444 " via '/222' RET 'dw',
then '/444' RET 'dw'. Now, we expect a single 'u' to restore the '444 ',
yet it undoes everything, leaving an empty buffer.
Hmm... that's probably no different from the first example.

One more, then. Starting with this input:

  1 2 3 4 5 6

advance to the '2' with 'w', 'dw' to delete the 2, then three '.'s to
delete the 3, then 4 and 5.
Then begin to undo with 'u', then '.' to repeat it. Those first two
work, restoring the 5 and 4.
However, one more '.' restores both the 3 and the 2.

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

[Eli Zaretskii]

> From: Jim Meyering <jim@meyering.net>
> Date: Mon, 16 May 2016 08:39:15 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, Michael Kifer <kifer@cs.stonybrook.edu>, 22295@debbugs.gnu.org
> 
> Thank you.
> That does indeed fix the case I mentioned.
> Here are some cases where it does not work as expected:
> 
>   start with an empty buffer in viper-mode
>   type 'i 1 2 3 4 5ESC'
>   type 'F2dw' to delete the 2 and a space.
>   type 'wdw' to delete the 4 and a space.
> 
> Now, if I were to hit "u" to undo, I would expect that most recent
> deletion to be undone and the 4 would reappear.
> Then I would hit '.' to undo the deletion of the '2'. Finally one more
> '.' would undo the creation of that first line.
> However, with the current patches, that first 'u' undoes everything
> and leaves me with the empty initial file.
> 
> Another example starting with an empty file:
> Create some content via ':r!seq 999|fmt RETURN'
> Then remove e.g., "222 " and "444 " via '/222' RET 'dw',
> then '/444' RET 'dw'. Now, we expect a single 'u' to restore the '444 ',
> yet it undoes everything, leaving an empty buffer.
> Hmm... that's probably no different from the first example.
> 
> One more, then. Starting with this input:
> 
>   1 2 3 4 5 6
> 
> advance to the '2' with 'w', 'dw' to delete the 2, then three '.'s to
> delete the 3, then 4 and 5.
> Then begin to undo with 'u', then '.' to repeat it. Those first two
> work, restoring the 5 and 4.
> However, one more '.' restores both the 3 and the 2.

Thanks.  It now looks like your expectations are close to what Emacs
does by default, whereas Michael said the VI undo is more coarse (and
in the original recipe, it indeed seemed to be that).

Is it possible to have a more general/formal description of what
'undo' in VI is supposed to do?  E.g., it looks like it has different
granularities wrt insertions and deletions, is that correct?

You see, when I said this is undocumented, I meant precisely that: the
expected effect of 'undo' in VI is not described, so someone who is
not a VI user doesn't know what to test and how to program that.

Michael said that this _is_ documented, but the only documentation I
see is comments that describe _what_ they do in terms of Emacs undo
structures, and are full of "rationale" such as "so that things will
be undone properly".  I don't see any description of the expected
effect of undoing in different situations, let alone some formal
specification of undo-related requirements.  If I missed some
description of how undo is supposed to work in viper, please just
point me there.

Another alternative is to make viper use the default Emacs undo, and
then ask you and other users of viper to tell where the results don't
match your expectations.  It could well be that starting with a clean
slate will get us to the goal faster and with less complex code.

TIA

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 1851 bytes --]

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 4794 bytes --]

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

[Eli Zaretskii]

> Cc: phillip.lord@russet.org.uk, 22295@debbugs.gnu.org
> From: Michael Kifer <kifer@cs.stonybrook.edu>
> Date: Mon, 16 May 2016 13:14:05 -0400
> 
> >  Another alternative is to make viper use the default Emacs undo, and
> > then ask you and other users of viper to tell where the results don't
> > match your expectations.  It could well be that starting with a clean
> > slate will get us to the goal faster and with less complex code.
> 
> This would be a non-starter and would cause a mass migration to vim. The undo would also then be
> implementation dependent. If, say, "delete 2 words" is implemented differently from how it is now then it would
> be undone via a different sequence of commands. 

Then I guess the current trial-and-error method will have to be the
way.  Too bad, I have hard time believing we will be ready with a
complete solution in time for Emacs 25.1, which is what I hoped.

Thanks.

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 2866 bytes --]

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

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> One more, then. Starting with this input:
>> 
>>   1 2 3 4 5 6
>> 
>> advance to the '2' with 'w', 'dw' to delete the 2, then three '.'s to
>> delete the 3, then 4 and 5.
>> Then begin to undo with 'u', then '.' to repeat it. Those first two
>> work, restoring the 5 and 4.
>> However, one more '.' restores both the 3 and the 2.
>
> Thanks.  It now looks like your expectations are close to what Emacs
> does by default, whereas Michael said the VI undo is more coarse (and
> in the original recipe, it indeed seemed to be that).
>
> Is it possible to have a more general/formal description of what
> 'undo' in VI is supposed to do?  E.g., it looks like it has different
> granularities wrt insertions and deletions, is that correct?
>
> You see, when I said this is undocumented, I meant precisely that: the
> expected effect of 'undo' in VI is not described, so someone who is
> not a VI user doesn't know what to test and how to program that.

AFAICT, once you enter insert mode, it squashes all undo boundaries,
until you leave again. Unfortunately, outside of insert mode (i.e. in
"vi" mode), it uses the default "one per command" emacs undo.


> Michael said that this _is_ documented, but the only documentation I
> see is comments that describe _what_ they do in terms of Emacs undo
> structures, and are full of "rationale" such as "so that things will
> be undone properly".  I don't see any description of the expected
> effect of undoing in different situations, let alone some formal
> specification of undo-related requirements.  If I missed some
> description of how undo is supposed to work in viper, please just
> point me there.


I guess it's "the same as vi". Although, the undo behaves differently
from vim at least: viper has "u" == "undo" and "." == "undo more". VIM
just uses repeated "u"'s to achieve the same thing.


> Another alternative is to make viper use the default Emacs undo, and
> then ask you and other users of viper to tell where the results don't
> match your expectations.  It could well be that starting with a clean
> slate will get us to the goal faster and with less complex code.
>
> TIA


It mostly does use the default Emacs undo, with a bit of weirdness in
one mode.

Phil

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

[Phillip Lord]

Jim Meyering <jim@meyering.net> writes:

> On Mon, May 16, 2016 at 5:41 AM, Phillip Lord
> <phillip.lord@russet.org.uk> wrote:
> ...
>> I have pushed a first attempt at a fix to branch fix/viper-undo.
>> Clearly, it's not finished yet, but I'd like to get your feedback as to
>> whether it works; it should fix the example given, but I haven't used
>> viper at all, so if it breaks other things I won't know.
>>
>> I haven't managed to work out exactly why the error is happening --
>> probably though because the timing of the undo-boundary is somewhat
>> different, though. With this fix, viper just disables automatic boundary
>> addition and adds it's own as necessary, which seems cleaner.
>
> Thank you.
> That does indeed fix the case I mentioned.
> Here are some cases where it does not work as expected:
>
>   start with an empty buffer in viper-mode
>   type 'i 1 2 3 4 5ESC'
>   type 'F2dw' to delete the 2 and a space.
>   type 'wdw' to delete the 4 and a space.
>
> Now, if I were to hit "u" to undo, I would expect that most recent
> deletion to be undone and the 4 would reappear.
> Then I would hit '.' to undo the deletion of the '2'. Finally one more
> '.' would undo the creation of that first line.
> However, with the current patches, that first 'u' undoes everything
> and leaves me with the empty initial file.
>
> Another example starting with an empty file:
> Create some content via ':r!seq 999|fmt RETURN'
> Then remove e.g., "222 " and "444 " via '/222' RET 'dw',
> then '/444' RET 'dw'. Now, we expect a single 'u' to restore the '444 ',
> yet it undoes everything, leaving an empty buffer.
> Hmm... that's probably no different from the first example.
>
> One more, then. Starting with this input:
>
>   1 2 3 4 5 6
>
> advance to the '2' with 'w', 'dw' to delete the 2, then three '.'s to
> delete the 3, then 4 and 5.
> Then begin to undo with 'u', then '.' to repeat it. Those first two
> work, restoring the 5 and 4.
> However, one more '.' restores both the 3 and the 2.

How do you ever learn all of these keypresses?

I've pushed to fix/viper-undo, and AFAICT, it handles all of these
situations.

I *thought* viper was essentially switching off Emacs undo-boundary
behaviour, but, actually, it only does this very specific circumstances
(which it does by adding 'viper, then deleting all the other boundaries
upto 'viper). So, the fix I have now just disables the addition of
undo-boundaries when 'viper would have been added, and switches them
back on again when the boundaries and 'viper would have been removed.
This *should* behave the same. As an added bonus, the code is quite a
bit simpler.

Can you test a bit further for me? Any key sequences you want to send
would be gratefully recieved; I'm converting them into kbd-macros and
will use them as test cases. So:

  (execute-kbd-macro
   [
    ?i
    ?1 ?  ?2 ?  ?3 ?  ?4 ?  ?5
    escape
    ?F ?2 ?d ?w
    ?w ?d ?w
    ?u
    ])

in a clean buffer should leave "1 2 4 5".

Phil

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:

>> You see, when I said this is undocumented, I meant precisely that: the
>> expected effect of 'undo' in VI is not described, so someone who is
>> not a VI user doesn't know what to test and how to program that.
>
> In VI, an undo is supposed to undo the effect of the previous VI
> command. In Emacs terms, each such command usually means several
> inserts and deletes, which in Emacs would be undone via a series of
> undos. Such behavior is a non-no to a vi user.


Actually, by default inserts and (simple) deletes are amalgamated by
Emacs and undone in chunks of 20 rather than one at a time.


> I was referring to the insertion of a special marker into the undo
> list. Obviously, the usual Vi conventions are not documented because
> this would require to duplicate the Vi manual.

Actually, that would be a useful statement to have. Viper may replicate
"vi" behaviour, although I don't have a copy of vi to test it on. It
doesn't replicate vim's undo.


>> Another alternative is to make viper use the default Emacs undo, and
>> then ask you and other users of viper to tell where the results don't
>> match your expectations. It could well be that starting with a clean
>> slate will get us to the goal faster and with less complex code.
>
> This would be a non-starter and would cause a mass migration to vim.
> The undo would also then be implementation dependent. If, say, "delete
> 2 words" is implemented differently from how it is now then it would
> be undone via a different sequence of commands.

People will get a different sequence of commands if they migrate to vim
also!

Anyway, I have sent more code upstream. It changes the implementation,
but (hopefully) will preserve the currrent behaviour.

Phil

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

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: phillip.lord@russet.org.uk, 22295@debbugs.gnu.org
>> From: Michael Kifer <kifer@cs.stonybrook.edu>
>> Date: Mon, 16 May 2016 13:14:05 -0400
>> 
>> >  Another alternative is to make viper use the default Emacs undo, and
>> > then ask you and other users of viper to tell where the results don't
>> > match your expectations.  It could well be that starting with a clean
>> > slate will get us to the goal faster and with less complex code.
>> 
>> This would be a non-starter and would cause a mass migration to vim. The undo would also then be
>> implementation dependent. If, say, "delete 2 words" is implemented differently from how it is now then it would
>> be undone via a different sequence of commands. 
>
> Then I guess the current trial-and-error method will have to be the
> way.  Too bad, I have hard time believing we will be ready with a
> complete solution in time for Emacs 25.1, which is what I hoped.

I worry about this as well. I still do not understand why my changes
have caused this problem; I'd rather not be taking the approach to
redoing the implementation of viper during release candidate.

Still, if, as I suspect, it's because the timing of boundaries has
changed slightly, it's likely to be the only choice.

Phil

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:

> On 05/16/2016 05:50 AM, Phillip Lord wrote:
> OK. I don't remember this very clearly but there are cases (which I
> don't remember) where viper-mode gets kicked in implicitly for buffers
> where the user didn't ask it to and where it is not wanted. Maybe when
> one runs emacs in batch mode as a scripting tool.

AFAIK, noninteractive *only* happens in batch. Perhaps there are other
circumstances.

> One can defeat viper-mode in that case using emacs -q, but maybe there
> are other cases.

> Since I don't remember and since it is not good to have kludges
> around, one way to figure it out is to comment out the (not
> interactive) part and then see what happens. If and when you see
> undesired behavior, then uncomment and document :-)

I might well try that on master. We really need some unit tests for
viper; it was be a shame if Jim's examples just got archived in the bug
tracker.

Phil

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 2194 bytes --]

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 3650 bytes --]

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:
> I used to have (may still have) access to Solaris where the One True
> Vi could still be found :-) True, vim undo is slightly different. But
> I am not sure there was a vim back when viper started. Besides, the
> '.' came from a precursor of viper, introduced by someone else, so I
> am not responsible :-) After all these years using it I prefer the
> viper behavior.

Trying to preserve the old behaviour (i.e. before I broke it!) is the
best I can offer then!


> Anyway, I have sent more code upstream. It changes the implementation,
> but (hopefully) will preserve the currrent behaviour.
>
> Cool, thanks! 

Please do send me any test cases that do not work. I've got ert unit
tests working now, so I can use any set of keypresses on a clean buffer
with the expected output (preferable as a keyboard macro, but I can
convert a description!).

Phil

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

[Phillip Lord]

Jim Meyering <jim@meyering.net> writes:
> Thank you!
> I've updated and built with that. It is much better.
> So far, I have noticed only one anomaly:
>
> Starting with an empty file, add three lines of text, terminating each
> addition with ESC.
> E.g., i1ESC then o2ESC and o3ESC
> Then, "u.." should restore to the original, empty file.
> The first "u" works fine and removes the "3" line, but the following
> "." has no visual effect, and I have to hit it again to remove the
> "2". Same story for the "1".

Yeah, I was adding two undo-boundaries to rather than just one. For no
readily apparent reason I was directly changing the buffer-undo-list,
rather than calling undo-boundary.

Anyway, that should be fixed.

Phil

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

[Jim Meyering]

On Wed, May 18, 2016 at 2:15 AM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
> Yeah, I was adding two undo-boundaries to rather than just one. For no
> readily apparent reason I was directly changing the buffer-undo-list,
> rather than calling undo-boundary.
>
> Anyway, that should be fixed.

Confirmed.
Thanks again for fixing all of that.

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

[Phillip Lord]

Jim Meyering <jim@meyering.net> writes:

> On Wed, May 18, 2016 at 2:15 AM, Phillip Lord
> <phillip.lord@russet.org.uk> wrote:
>> Yeah, I was adding two undo-boundaries to rather than just one. For no
>> readily apparent reason I was directly changing the buffer-undo-list,
>> rather than calling undo-boundary.
>>
>> Anyway, that should be fixed.
>
> Confirmed.
> Thanks again for fixing all of that.


Let me know if you find anything else.

John, this is a reasonably extensive change now -- affecting both
simple.el, but also a big rework of viper undo.

To Emacs-25 or not to Emacs-25 that is the question?

Phil

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

[Jim Meyering]

On Wed, May 18, 2016 at 2:42 PM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> On Wed, May 18, 2016 at 2:15 AM, Phillip Lord
>> <phillip.lord@russet.org.uk> wrote:
>>> Yeah, I was adding two undo-boundaries to rather than just one. For no
>>> readily apparent reason I was directly changing the buffer-undo-list,
>>> rather than calling undo-boundary.
>>>
>>> Anyway, that should be fixed.
>>
>> Confirmed.
>> Thanks again for fixing all of that.
>
>
> Let me know if you find anything else.
>
> John, this is a reasonably extensive change now -- affecting both
> simple.el, but also a big rework of viper undo.
>
> To Emacs-25 or not to Emacs-25 that is the question?

Perspective of a viper-mode user: including this fix in Emacs-25 is a
must: without it, any existing viper-mode user will very quickly
notice how "undo" appears to destroy data, seemingly unrecoverably,
since "redo" does not restore it -- they'd have to go look in emacs'
yank buffer.

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

[Eli Zaretskii]

> From: Jim Meyering <jim@meyering.net>
> Date: Wed, 18 May 2016 18:09:36 -0700
> Cc: 22295@debbugs.gnu.org, John Wiegley <jwiegley@gmail.com>, Eli Zaretskii <eliz@gnu.org>
> 
> On Wed, May 18, 2016 at 2:42 PM, Phillip Lord
> <phillip.lord@russet.org.uk> wrote:
> > Jim Meyering <jim@meyering.net> writes:
> >
> >> On Wed, May 18, 2016 at 2:15 AM, Phillip Lord
> >> <phillip.lord@russet.org.uk> wrote:
> >>> Yeah, I was adding two undo-boundaries to rather than just one. For no
> >>> readily apparent reason I was directly changing the buffer-undo-list,
> >>> rather than calling undo-boundary.
> >>>
> >>> Anyway, that should be fixed.
> >>
> >> Confirmed.
> >> Thanks again for fixing all of that.
> >
> >
> > Let me know if you find anything else.
> >
> > John, this is a reasonably extensive change now -- affecting both
> > simple.el, but also a big rework of viper undo.
> >
> > To Emacs-25 or not to Emacs-25 that is the question?
> 
> Perspective of a viper-mode user: including this fix in Emacs-25 is a
> must: without it, any existing viper-mode user will very quickly
> notice how "undo" appears to destroy data, seemingly unrecoverably,
> since "redo" does not restore it -- they'd have to go look in emacs'
> yank buffer.

FWIW, I think this should go to emacs-25, since (a) the changes affect
only viper-mode, and (b) users of that mode will be most unhappy
without these changes.

However, we should add comments to viper-cmd.el that document the
special handling of 'undo' by viper-mode, and how that interacts with
the core 'undo' functionalities.  We should also comment in simple.el
that undo-auto-disable-boundaries is used by viper-mode, and document
that variable in the ELisp manual.  Phillip, can you add these bits to
the branch, please?

Thanks.

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

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

> FWIW, I think this should go to emacs-25, since (a) the changes affect
> only viper-mode, and (b) users of that mode will be most unhappy
> without these changes.

Not quite true. I've changed simple.el.

The change is minimal, and just allows downstream developers to disable
the auto-undo boundary. However, if a developer does do this in a bad
way, then Emacs does produce pretty intrusive "this is probably a bug"
warnings.


> However, we should add comments to viper-cmd.el that document the
> special handling of 'undo' by viper-mode, and how that interacts with
> the core 'undo' functionalities.  We should also comment in simple.el
> that undo-auto-disable-boundaries is used by viper-mode, and document
> that variable in the ELisp manual.  Phillip, can you add these bits to
> the branch, please?


Oh, yeah, sorry forgot the manual. I updated most of the rest last
night. I think that the branch is ready, bar squashing.

Phil

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

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Jim Meyering <jim@meyering.net>,  22295@debbugs.gnu.org,  jwiegley@gmail.com
> Date: Fri, 20 May 2016 12:46:14 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > FWIW, I think this should go to emacs-25, since (a) the changes affect
> > only viper-mode, and (b) users of that mode will be most unhappy
> > without these changes.
> 
> Not quite true. I've changed simple.el.

I've seen the change, of course.  As long as only viper-mode uses that
variable, I see no danger.

> > However, we should add comments to viper-cmd.el that document the
> > special handling of 'undo' by viper-mode, and how that interacts with
> > the core 'undo' functionalities.  We should also comment in simple.el
> > that undo-auto-disable-boundaries is used by viper-mode, and document
> > that variable in the ELisp manual.  Phillip, can you add these bits to
> > the branch, please?
> 
> Oh, yeah, sorry forgot the manual. I updated most of the rest last
> night. I think that the branch is ready, bar squashing.

Thanks.

John, we are waiting for your verdict.

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

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> FWIW, I think this should go to emacs-25, since (a) the changes affect only
> viper-mode, and (b) users of that mode will be most unhappy without these
> changes.

I agree, Eli, let's push this to emacs-25.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

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

[Phillip Lord]

John Wiegley <jwiegley@gmail.com> writes:

>>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>
>> FWIW, I think this should go to emacs-25, since (a) the changes affect only
>> viper-mode, and (b) users of that mode will be most unhappy without these
>> changes.
>
> I agree, Eli, let's push this to emacs-25.


All done.

I've added my tests as well -- should have done that in the same commit
I guess, but forgot. Hope that's okay on emacs-25.

Phil

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

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Eli Zaretskii <eliz@gnu.org>,  Jim Meyering <jim@meyering.net>,  22295@debbugs.gnu.org
> Date: Mon, 23 May 2016 14:23:44 +0100
> 
> John Wiegley <jwiegley@gmail.com> writes:
> 
> >>>>>> Eli Zaretskii <eliz@gnu.org> writes:
> >
> >> FWIW, I think this should go to emacs-25, since (a) the changes affect only
> >> viper-mode, and (b) users of that mode will be most unhappy without these
> >> changes.
> >
> > I agree, Eli, let's push this to emacs-25.
> 
> 
> All done.

Thanks.

Any reason you didn't close the bug?

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

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> >> FWIW, I think this should go to emacs-25, since (a) the changes affect only
>> >> viper-mode, and (b) users of that mode will be most unhappy without these
>> >> changes.
>> >
>> > I agree, Eli, let's push this to emacs-25.
>> 
>> 
>> All done.
>
> Thanks.
>
> Any reason you didn't close the bug?

Closed now.

(If I have done it right -- if I have done it wrong, tell me!)

Phil

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

[Eli Zaretskii]

> From: Phillip Lord <phillip.lord@russet.org.uk>
> Cc: 22295@debbugs.gnu.org,  jwiegley@gmail.com,  jim@meyering.net
> Date: Wed, 25 May 2016 13:43:22 +0100
> 
> > Any reason you didn't close the bug?
> 
> Closed now.
> 
> (If I have done it right -- if I have done it wrong, tell me!)

You did OK, thanks.

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

[Stefan Monnier]

> different, though. With this fix, viper just disables automatic boundary
> addition and adds it's own as necessary, which seems cleaner.

I see an annoying side-effect of that change, tho:

    % emacs -Q -f viper-mode
    y 5 n                   ;; To get past viper's initial questions.
    i helo C-b k C-/

This last C-/ used to (and should) just undo the insertion of the last
"k", but instead it now completely wipes out the *scratch* buffer.

So I think that completely disabling undo boundaries is not a good idea:
while in many use-cases insertion mode is very transient, it is not so
unusual to spend more time in insertion mode (which is pretty much
"emacs mode") and to expect undo boundaries to properly inserted during
this time.

Have we been able to identify the original problem?  The old code seemed
"simple" enough that the problem was probably simple to fix (once
identified).


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> different, though. With this fix, viper just disables automatic boundary
>> addition and adds it's own as necessary, which seems cleaner.
>
> I see an annoying side-effect of that change, tho:
>
>     % emacs -Q -f viper-mode
>     y 5 n                   ;; To get past viper's initial questions.
>     i helo C-b k C-/
>
> This last C-/ used to (and should) just undo the insertion of the last
> "k", but instead it now completely wipes out the *scratch* buffer.
>
> So I think that completely disabling undo boundaries is not a good idea:
> while in many use-cases insertion mode is very transient, it is not so
> unusual to spend more time in insertion mode (which is pretty much
> "emacs mode") and to expect undo boundaries to properly inserted during
> this time.

Oh dear. Yes, that is a problem. The difficulty is that viper modifies
the undo-list, removing boundaries only *after* we leave insert mode.
Hence, when you type C-/ above they are still there.

If you do

i helo C-b k escape C-/

before the undo changes, then the whole buffer is deleted also. So, undo
behaves different in insert mode and in <V> command mode.

Viper's solution of introducing a 'viper symbol is a nice one, but has
it's problems. If you do, for example

i helo C-b k C-/ C-/

We get an error from the undo system.

primitive-undo: Unrecognized entry in undo list viper

The deep problem here is that undo boundary == nil -- i.e. there is one
and only one symbol for undo-boundary. If it could carry a value, viper
could do this cleanly.

Only solution that leaps to mind is this:


1) Restore all the old viper code
2) Instead of adding a 'viper mark, copy the buffer-undo-list to
"viper-old-buffer-undo-list".
3) Instead of this....

(if (setq tmp (memq viper-buffer-undo-list-mark buffer-undo-list))

we should now already have the cons cell that represents the tail of the
buffer-undo-list.

This is also quite a big change, and I worry about buffer compaction --
viper-old-buffer-undo-list would not be open for GC.


> Have we been able to identify the original problem?

No, fraid not. Thought about it lots. Failed.

> The old code seemed "simple" enough that the problem was probably
> simple to fix (once identified).

Well, most problems are simple to fix once you actually know what they
are.

I shall look again at the old code, and see if I can figure it out.

Phil

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

[Michael Kifer]

On 06/01/2016 06:23 PM, Phillip Lord wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> different, though. With this fix, viper just disables automatic boundary
>>> addition and adds it's own as necessary, which seems cleaner.
>> I see an annoying side-effect of that change, tho:
>>
>>      % emacs -Q -f viper-mode
>>      y 5 n                   ;; To get past viper's initial questions.
>>      i helo C-b k C-/
>>
>> This last C-/ used to (and should) just undo the insertion of the last
>> "k", but instead it now completely wipes out the *scratch* buffer.
>>
>> So I think that completely disabling undo boundaries is not a good idea:
>> while in many use-cases insertion mode is very transient, it is not so
>> unusual to spend more time in insertion mode (which is pretty much
>> "emacs mode") and to expect undo boundaries to properly inserted during
>> this time.
> Oh dear. Yes, that is a problem. The difficulty is that viper modifies
> the undo-list, removing boundaries only *after* we leave insert mode.
> Hence, when you type C-/ above they are still there.
>
> If you do
>
> i helo C-b k escape C-/
>
> before the undo changes, then the whole buffer is deleted also. So, undo
> behaves different in insert mode and in <V> command mode.
>
> Viper's solution of introducing a 'viper symbol is a nice one, but has
> it's problems. If you do, for example
>
> i helo C-b k C-/ C-/
>
> We get an error from the undo system.
>
> primitive-undo: Unrecognized entry in undo list viper

not in  25.0.50
viper worked fine with this for 20 years and I'd say it is Emacs 
breakage, not Viper's.


> The deep problem here is that undo boundary == nil -- i.e. there is one
> and only one symbol for undo-boundary. If it could carry a value, viper
> could do this cleanly.
>
> Only solution that leaps to mind is this:
>
>
> 1) Restore all the old viper code
> 2) Instead of adding a 'viper mark, copy the buffer-undo-list to
> "viper-old-buffer-undo-list".
> 3) Instead of this....
>
> (if (setq tmp (memq viper-buffer-undo-list-mark buffer-undo-list))
>
> we should now already have the cons cell that represents the tail of the
> buffer-undo-list.
>
> This is also quite a big change, and I worry about buffer compaction --
> viper-old-buffer-undo-list would not be open for GC.

Do you know who made the changes to the emacs undo list? Maybe complain 
to him?



>
>> Have we been able to identify the original problem?
> No, fraid not. Thought about it lots. Failed.

could not  understand what happened either.

--

        --- michael


>
>> The old code seemed "simple" enough that the problem was probably
>> simple to fix (once identified).
> Well, most problems are simple to fix once you actually know what they
> are.
>
> I shall look again at the old code, and see if I can figure it out.
>
> Phil

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

[Phillip Lord]

Michael Kifer <kifer@cs.stonybrook.edu> writes:

> On 06/01/2016 06:23 PM, Phillip Lord wrote:
>> Viper's solution of introducing a 'viper symbol is a nice one, but has
>> it's problems. If you do, for example
>>
>> i helo C-b k C-/ C-/
>>
>> We get an error from the undo system.
>>
>> primitive-undo: Unrecognized entry in undo list viper
>
> not in  25.0.50
> viper worked fine with this for 20 years and I'd say it is Emacs breakage, not
> Viper's.

Okay, will check on that and see what has changed. I was surprised no
one had complained about it.


>> we should now already have the cons cell that represents the tail of the
>> buffer-undo-list.
>>
>> This is also quite a big change, and I worry about buffer compaction --
>> viper-old-buffer-undo-list would not be open for GC.
>
> Do you know who made the changes to the emacs undo list? Maybe complain to
> him?

Sadly, that would be me. This is the reason I am trying to fix a mode
that I don't use! Although, by the time I have finished this, maybe I
will know the keystrokes well enough that I will switch.

Phil

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

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 796 bytes --]

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

[Stefan Monnier]

>> Oh dear. Yes, that is a problem. The difficulty is that viper modifies
>> the undo-list, removing boundaries only *after* we leave insert mode.

I don't see any difficulty in there.

>> Viper's solution of introducing a 'viper symbol is a nice one, but has
>> it's problems.

Agreed.

>> primitive-undo: Unrecognized entry in undo list viper
> not in  25.0.50 viper worked fine with this for 20 years and I'd say
> it is Emacs breakage, not Viper's.

FWIW, Viper adds an entry to buffer-undo-list which is incompatible with
the documented format of the entries, so while it worked, Viper was
doing something unkosher.

In any case it shouldn't be difficult to fix that problem.
E.g. you could use

    (defconst viper--undo-marker '(1 . 1))

instead of a symbol.

>> The deep problem here is that undo boundary == nil

I know you like to think of it as a problem ;-)

>> 1) Restore all the old viper code
>> 2) Instead of adding a 'viper mark, copy the buffer-undo-list to
>> "viper-old-buffer-undo-list".

Don't copy, just (setq viper--original-undo-list buffer-undo-list).

>> This is also quite a big change, and I worry about buffer compaction --
>> viper-old-buffer-undo-list would not be open for GC.

Why not?

This said, AFAIK the OP's problem may not be directly linked to the addition
of a special symbol in the undo-list.  We should first figure out what's
actually going wrong there.


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Oh dear. Yes, that is a problem. The difficulty is that viper modifies
>>> the undo-list, removing boundaries only *after* we leave insert mode.
>
> I don't see any difficulty in there.

Sorry, "difficulty" in the sense that this is the cause of the different
behaviour.


>
>>> Viper's solution of introducing a 'viper symbol is a nice one, but has
>>> it's problems.
>
> Agreed.
>
>>> primitive-undo: Unrecognized entry in undo list viper
>> not in  25.0.50 viper worked fine with this for 20 years and I'd say
>> it is Emacs breakage, not Viper's.
>
> FWIW, Viper adds an entry to buffer-undo-list which is incompatible with
> the documented format of the entries, so while it worked, Viper was
> doing something unkosher.
>
> In any case it shouldn't be difficult to fix that problem.
> E.g. you could use
>
>     (defconst viper--undo-marker '(1 . 1))
>
> instead of a symbol.

I think that has the advantage of fulfilling the spec by putting what is
a nop in the undo.


>>> The deep problem here is that undo boundary == nil
>
> I know you like to think of it as a problem ;-)

It is. In my poking through Emacs, I have found several places where the
code say "remove the last undo-boundary which should be..." or "don't
remove undo-boundary which are explicit". But we have to guess which is
which.

Instead we could do:

(nil . :explicit)
(nil . :auto)
(nil . :timer)


>>> 1) Restore all the old viper code
>>> 2) Instead of adding a 'viper mark, copy the buffer-undo-list to
>>> "viper-old-buffer-undo-list".
>
> Don't copy, just (setq viper--original-undo-list buffer-undo-list).

Agreed; meant "copy the reference". Poor choice of words.

>
>>> This is also quite a big change, and I worry about buffer compaction --
>>> viper-old-buffer-undo-list would not be open for GC.
>
> Why not?

Oh, yes, you are correct. I was thinking of this as some sort of weak
reference, but actually, buffer-undo-list is specifically truncated by
GC. It wouldn't matter if there were a copy elsewhere.


> This said, AFAIK the OP's problem may not be directly linked to the addition
> of a special symbol in the undo-list.  We should first figure out what's
> actually going wrong there.


I will try and investigate again, but I am running out of ideas to test.

Phil

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

[Stefan Monnier]

>> The old code seemed "simple" enough that the problem was probably
>> simple to fix (once identified).
> Well, most problems are simple to fix once you actually know what they
> are.

The problem seems to be the following: when you hit ESC, Viper will remove
boundaries from the undo-list, without making any changes to the buffer,
so the top-level loop won't add a boundary before the next command and
ends up hence "amalgamating" the next command with the previous one.

In the old code, we just always blindly added a boundary to
current-buffer before running a command, whereas now we only do so if
the buffer has been modified since the last time we pushed a boundary.

I think the patch below fixes the original problem.
Another way to fix it would be to change undo-auto--add-boundary so it
always considers (current-buffer) regardless of
undo-auto--undoably-changed-buffers.


        Stefan


diff --git a/lisp/emulation/viper-cmd.el b/lisp/emulation/viper-cmd.el
index 93cf3b0..ca52f98 100644
--- a/lisp/emulation/viper-cmd.el
+++ b/lisp/emulation/viper-cmd.el
@@ -1715,7 +1715,8 @@ viper-adjust-undo
       (let ((inhibit-quit t)
 	    tmp tmp2)
 	(setq viper-undo-needs-adjustment nil)
-	(if (listp buffer-undo-list)
+	(when (listp buffer-undo-list)
+	  (let ((had-boundary (null (car buffer-undo-list))))
 	    (if (setq tmp (memq viper-buffer-undo-list-mark buffer-undo-list))
 		(progn
 		  (setq tmp2 (cdr tmp)) ; the part after mark
@@ -1729,8 +1730,11 @@ viper-adjust-undo
 			(delq viper-buffer-undo-list-mark buffer-undo-list))
 		  ;; restore tail of buffer-undo-list
 		  (setq buffer-undo-list (nconc buffer-undo-list tmp2)))
-	      (setq buffer-undo-list (delq nil buffer-undo-list)))))
-    ))
+	      (setq buffer-undo-list (delq nil buffer-undo-list)))
+            ;; The top-level loop only adds boundaries if there has been
+            ;; modifications in the buffer, so make sure we don't accidentally
+            ;; drop the "final" boundary (bug#22295).
+	    (if had-boundary (undo-boundary)))))))
 
 
 (defun viper-set-complex-command-for-undo ()

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

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> The old code seemed "simple" enough that the problem was probably
>>> simple to fix (once identified).
>> Well, most problems are simple to fix once you actually know what they
>> are.
>
> The problem seems to be the following: when you hit ESC, Viper will remove
> boundaries from the undo-list, without making any changes to the buffer,
> so the top-level loop won't add a boundary before the next command and
> ends up hence "amalgamating" the next command with the previous one.
>
> In the old code, we just always blindly added a boundary to
> current-buffer before running a command, whereas now we only do so if
> the buffer has been modified since the last time we pushed a boundary.
>
> I think the patch below fixes the original problem.
> Another way to fix it would be to change undo-auto--add-boundary so it
> always considers (current-buffer) regardless of
> undo-auto--undoably-changed-buffers.

Stefan

Many thanks for working this out -- I was going crazy trying to track
this down, and I was looking in the wrong place all along. 

I've tried your patch against emacs-25 (after reverting c0139e32f1f3b)
and it appears to work. The current behaviour of undo-auto--add-boundary
seems sensible to me; the difference in behaviour only shows up because
viper is manipulating the undo-list. I can try modifying u-a--a-b though
-- viper is, I am sure, not the only package to fiddle with undo.

I'd propose adding this to emacs-25. Eli, are you happy with this --- my
last fix appears to not work and Stefans approach is much more discrete.


As a separate issue, I'd like to add part of c0139e32f1f3b back
though to master -- adding the variable undo-auto-disable-boundaries.
It's not immediately necessary now, but it seems a useful option to
have.

Phil

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

[Stefan Monnier]

> As a separate issue, I'd like to add part of c0139e32f1f3b back
> though to master -- adding the variable undo-auto-disable-boundaries.
> It's not immediately necessary now, but it seems a useful option to
> have.

FWIW, I'd rather not have such an option.

It's seeing this option which made me look further into your patch,
since it seemed like using such a thing would inevitably lead
to trouble.


        Stefan

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

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: 22295@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Jim Meyering <jim@meyering.net>,  Michael Kifer <kifer@cs.stonybrook.edu>
> Date: Fri, 10 Jun 2016 23:18:29 +0100
> 
> I'd propose adding this to emacs-25. Eli, are you happy with this --- my
> last fix appears to not work and Stefans approach is much more discrete.

Yes, thanks.

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

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> As a separate issue, I'd like to add part of c0139e32f1f3b back
>> though to master -- adding the variable undo-auto-disable-boundaries.
>> It's not immediately necessary now, but it seems a useful option to
>> have.
>
> FWIW, I'd rather not have such an option.
>
> It's seeing this option which made me look further into your patch,
> since it seemed like using such a thing would inevitably lead
> to trouble.

No worries, I will leave this out.

Phil

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

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: 22295@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, Jim Meyering
>> <jim@meyering.net>, Michael Kifer <kifer@cs.stonybrook.edu>
>> Date: Fri, 10 Jun 2016 23:18:29 +0100
>> 
>> I'd propose adding this to emacs-25. Eli, are you happy with this --- my
>> last fix appears to not work and Stefans approach is much more discrete.
>
> Yes, thanks.

This has been added to Emacs-25. Everyone happy for me to close?

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

[Stefan Monnier]

> This has been added to Emacs-25. Everyone happy for me to close?

Fine by me,


        Stefan

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

[Jim Meyering]

On Mon, Jun 13, 2016 at 6:06 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> This has been added to Emacs-25. Everyone happy for me to close?
>
> Fine by me,

Same here. Thanks for all the work.

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

[Michael Kifer]

On 06/13/2016 08:37 AM, Phillip Lord wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: phillip.lord@russet.org.uk (Phillip Lord)
>>> Cc: 22295@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, Jim Meyering
>>> <jim@meyering.net>, Michael Kifer <kifer@cs.stonybrook.edu>
>>> Date: Fri, 10 Jun 2016 23:18:29 +0100
>>>
>>> I'd propose adding this to emacs-25. Eli, are you happy with this --- my
>>> last fix appears to not work and Stefans approach is much more discrete.
>> Yes, thanks.
> This has been added to Emacs-25. Everyone happy for me to close?
>

Yes, thank you!

--

        --- michael