tpope / vim-repeat

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

Rework error return to prevent off-by-one in Ex mode #85

Closed inkarkat closed 3 years ago

inkarkat commented 4 years ago

My automated tests run Vim in silent batch mode (:help -s-ex), these also cover repeat of mappings via repeat.vim. Unfortunately, the combination of feedkeys() and :execute used in the repeat.vim implementation to return a potential caught error cause the line where the repeat mapping is executed to be increased, so for me the tests fail in a really strange way. The same problem can occur if someone uses repeats in silent batch mode (e.g. through a recorded macro) - though the source of the problem would be even harder to track down there - in my test run it at least was easily reproducible.

I raised this as a potential Vim bug in https://github.com/vim/vim/issues/7153, but Bram explained that it's a side effect of Ex mode (where empty lines make the cursor go to the next line, and as repeat#run() returns the empty String on the happy path, that's an empty line being executed). Recommendation from Bram is to pass the "x" flag for immediate execution to feedkeys() (which avoids the problem). However, in the context of repeat.vim this would mean that any errors resulting from the repeat mapping invocation would have to be caught inside repeat#run(), because they would then execute within the function's context, and not after it. Also, the "x" flag would not be supported by old Vim 7.3 and earlier.

Instead, I chose to avoid the problem by replacing the :execute with a more straightforward :call (actually an :if that tests a returned Boolean success flag), so instead of the clever direct execution of the returned :echoerr command, the error message instead is retrieved from a script-local variable via a new repeat#errmsg() getter. I use this idiom in all of my plugins through a set of utility functions (https://github.com/inkarkat/vim-ingo-library/blob/5bb32fd27aa37a767d92ed29eb76dd48a7b0c7d2/autoload/ingo/err.vim#L38-L41), too. So by adding a little (well encapsulated) variable and getter, this problem can be avoided without touching the sensitive feedkeys() and try...catch logic within the plugin.

tpope commented 3 years ago

Haven't gotten around to auditing this so I'll merge now and ask questions later.