tpope / vim-repeat

repeat.vim: enable repeating supported plugin maps with "."
http://www.vim.org/scripts/script.php?script_id=2136
2.58k stars 81 forks source link

(Vim bug?) b:changedtick incremented after undo #63

Closed justinmk closed 2 weeks ago

justinmk commented 6 years ago

This issue is for reference, I don't have a solution.

At some point Vim changed its b:changedtick behavior after u, to increment more than vim-repeat expects. This can be reproduced with vim-surround, though it also affects vim-sneak.

Steps to reproduce:

$ vim -u NORC -N +":set runtimepath+=~/.vim/bundle/vim-repeat/" +":runtime plugin/repeat.vim" +":set runtimepath+=~/.vim/bundle/vim-surround/" +":runtime plugin/surround.vim"

ifooboo<Esc>0
ystb)
..
u
.

The . after u does not redo the surround operation.

Even though repeat#wrap() preserves g:repeat_tick, b:changedtick is incremented sometime after, so the next . does not work correctly.

tpope commented 6 years ago

No desire to implement it but one fix could be to cache the getchar() result inside opfunc, and clear that cache beforehand when the map is invoked. Then we can ditch the repeat#set() entirely and let . call g@ directly.

justinmk commented 6 years ago

Relaxing the tolerance might be good enough:

diff --git a/autoload/repeat.vim b/autoload/repeat.vim
index ce2141b8753d..7cebc992bbf4 100644
--- a/autoload/repeat.vim
+++ b/autoload/repeat.vim
@@ -75,7 +75,7 @@ endfunction
-
 function! repeat#run(count)
     try
-        if g:repeat_tick == b:changedtick
+        if (b:changedtick - g:repeat_tick <= 2)
             let r = ''
             if g:repeat_reg[0] ==# g:repeat_sequence && !empty(g:repeat_reg[1])
                 if g:repeat_reg[1] ==# '='
@@ -110,7 +110,7 @@ function! repeat#run(count)
 endfunction
-
 function! repeat#wrap(command,count)
-    let preserve = (g:repeat_tick == b:changedtick)
+    let preserve = (b:changedtick - g:repeat_tick <= 2)
     call feedkeys((a:count ? a:count : '').a:command, 'n')
     exe (&foldopen =~# 'undo\|all' ? 'norm! zv' : '')
     if preserve
tpope commented 6 years ago

Is this specific to surround.vim or can it be reproduced with other plugins?

justinmk commented 6 years ago

https://github.com/justinmk/vim-sneak/ is another plugin that is affected by it. My glance at repeat#wrap() leads me to believe this would affect any plugin that uses vim-repeat, since the problem occurs when u is invoked.

justinmk commented 6 years ago

I checked some more plugins which use repeat#set, and confirmed that this issue affects them:

The hack that I suggested above is too brittle, because the u step may increment b:changedtick by much more than 2 or 3 ticks, e.g. after vim-lion glipa , u increments b:changedtick by 5.

tpope commented 6 years ago

I'll also throw into the mix that a false negative breaking a plugin repeat is probably better than a false positive breaking all built-in repeats.

The direction I am moving is avoid repeat.vim wherever I can in favor of an operator func, even if I have to shoehorn it in. I'm pretty sure I can turn [e into an operator func, for example, and I'll probably do that at some point. Lion can probably use the same trick I proposed for Surround. I'm not sure about operator pending motions though. I don't really have a lot of experience there.

alexanderak commented 5 years ago

Hi! This solution solves issue:

diff --git a/autoload/repeat.vim b/autoload/repeat.vim
index ce2141b..1a4ab92 100644
--- a/autoload/repeat.vim
+++ b/autoload/repeat.vim
@@ -111,7 +111,7 @@ endfunction

 function! repeat#wrap(command,count)
     let preserve = (g:repeat_tick == b:changedtick)
-    call feedkeys((a:count ? a:count : '').a:command, 'n')
+    exe 'norm!' (a:count ? a:count : '').a:command
     exe (&foldopen =~# 'undo\|all' ? 'norm! zv' : '')
     if preserve
         let g:repeat_tick = b:changedtick
svermeulen commented 5 years ago

A plugin I'm working on also suffers from this issue. @alexanderak's patch solves it though. Is there a reason feedkeys needs to be used there instead? If not I hope that can be merged at some point

svermeulen commented 5 years ago

Here's another (possibly lower risk) solution for consideration

diff --git a/autoload/repeat.vim b/autoload/repeat.vim
index ce2141b..c24c872 100644
--- a/autoload/repeat.vim
+++ b/autoload/repeat.vim
@@ -114,7 +114,7 @@ function! repeat#wrap(command,count)
     call feedkeys((a:count ? a:count : '').a:command, 'n')
     exe (&foldopen =~# 'undo\|all' ? 'norm! zv' : '')
     if preserve
-        let g:repeat_tick = b:changedtick
+        call feedkeys(":let g:repeat_tick = b:changedtick\<cr>", 'n')
     endif
 endfunction
lacygoill commented 4 years ago

The issue is not due to a Vim bug but to a regression introduced by 1b82cad74cd5d5caf3c4092365cda54dc1fb853e.

Before this commit, the undo command was executed by :norm. The latter inserts the command in the typeahead and executes it immediately. After this commit, the command is written into the typeahead via feedkeys() which does not execute the command immediately.

As a result, this assignment is processed too early: https://github.com/tpope/vim-repeat/blob/c947ad2b6a16983724a0153bdf7f66d7a80a32ca/autoload/repeat.vim#L137

The undo command has not been executed yet. Which is why later, when you press ., the ticks are no longer synchronized, and the wrapper falls back on the native dot command which is unable to repeat a custom command.

There are 3 ways to fix the issue:

  1. execute the command via :norm again
  2. pass the x flag to feedkeys() to force Vim to execute the undo command immediately
  3. install a fire-once autocmd listening to TextChanged which updates g:repeat_tick only after the undo command has been executed

1. is not possible because it would hide the undo messages, which is why :norm was replaced by feedkeys() in the first place.

I don't like 2. because it forces Vim to execute the entire typeahead, and I wonder whether this could have unexpected side-effects when the latter contains more than the undo command (like when replaying a macro).

3. seems like the safest fix, but it's also the most verbose. I've been using it in my reimplementation, and so far it seems to work as expected.


I don't think the fact that :norm inserts keys in the typeahead (instead of appending them), and executes them immediately is documented. However, watch this:

" run this shell command
vim -Nu NONE <(cat <<'EOF'
    " a
    " b
    " c
    " some folded text {{{
    " some folded text
    " some folded text }}}
EOF
) +'set fdm=marker noro' +1d +'$'

" run this Ex command
:call feedkeys('3gsu', 'n') | norm! zv

Notice how the fold is immediately opened, but the undo command is not run before 3s. That's because Vim has executed the keys in this order:

zv3gsu
^^
inserted; not appended

Now, watch this:

vim -es -Nu NONE -i NONE +"pu='some text'" \
  +'set vbs=1 | echom b:changedtick | call feedkeys("dd", "n") | echom b:changedtick | qa!'
                                      ^^^^^^^^^^^^^^^^^^^^^^^^
3
3

vim -es -Nu NONE -i NONE +"pu='some text'" \
  +'set vbs=1 | echom b:changedtick | exe "norm! dd" | echom b:changedtick | qa!'
                                      ^^^^^^^^^^^^^^
3
4

Both shell commands make Vim execute the normal command dd. One via feedkeys(), the other via :norm. But notice how b:changedtick is only incremented immediately for :norm, not for feedkeys(). That's because :norm executes the keys immediately, but not feedkeys() (unless you pass it the x flag).


In any case, I've submitted a PR to fix the issue.

tomtomjhj commented 5 months ago

I don't like 2. because it forces Vim to execute the entire typeahead, and I wonder whether this could have unexpected side-effects when the latter contains more than the undo command (like when replaying a macro).

It seems that feedkeys() with 'ix' and :normal behave similarly in that regard: they both call exec_normal() to execute the entire typeahead.

So, assuming that :normal works well with replaying, this would work:

 function! repeat#wrap(command,count)
     let preserve = (g:repeat_tick == b:changedtick)
-    call feedkeys((a:count ? a:count : '').a:command, 'n')
-    exe (&foldopen =~# 'undo\|all' ? 'norm! zv' : '')
+    call feedkeys((a:count ? a:count : '').a:command, 'ntix')
     if preserve
         let g:repeat_tick = b:changedtick
     endif

Note that I also added 't' flag to so that folds are automatically opened as if the command was typed by the user.

tpope commented 2 weeks ago

I've gone with @tomtomjhj's solution as I find it a bit easier to reason about than the autocommand.

tomtomjhj commented 2 weeks ago

FYI, I ended up wrapping that in try-catch to avoid hit-enter due to error with stack trace when accidentally undo/redoing in nomodifiable buffer. https://github.com/tomtomjhj/vim-repeat/commit/782271546ca490e825b732cbc200765c5b6c6557

tpope commented 2 weeks ago

Fucking hell, it also impacts 'readonly', and that's only a warning so catch doesn't help.

tomtomjhj commented 2 weeks ago

FYI, the latest solution (a89a4d0760240e8b9ef6fd272b1f3f9da769d530) doesn't unfold folds.