VIM undo: Why does the cursor jump to the wrong position when undoing `undojoin`?

Question

EDITs:

• I've simplified the function and clarified the question.
  Original question is still available further down the page.

• Crossposted onto vim_dev mailing list: https://groups.google.com/forum/#!topic/vim_dev/_Rz3uVXbwsQ

• Reported as a bug to Neovim:
  https://github.com/neovim/neovim/issues/6276

---

Why is the cursor positioned differently in the following two examples:

1. [CORRECT CURSOR POSITION] The result of the substitution is joined to the previous change in the buffer (addition of line 3), the cursor position is correctly restored to the second line in the buffer.

   normal ggiline one is full of aaaa
   set undolevels=10 " splits the change into separate undo blocks
   
   normal Goline two is full of bbbb
   set undolevels=10
   
   normal Goline three is full of cccc
   set undolevels=10
   
   undojoin
   keepjumps %s/aaaa/zzzz/
   normal u
   

2. [INCORRECT CURSOR POSITION] The result of the substitution is joined to the previous change in the buffer (addition of line 4), the cursor position is incorrectly restored to the first line in the buffer (should be line 3).

   normal ggiline one is bull of aaaa
   set undolevels=10 " splits the change into separate undo blocks
   
   normal Goline two is full of bbbb
   set undolevels=10 
   
   normal Goline three is full of cccc        
   set undolevels=10
   
   normal Goline four is full of aaaa's again
   set undolevels=10
   
   undojoin
   keepjumps %s/aaaa/zzzz/
   normal u
   

---

Original Question

The way my VIM is set up, saving a buffer to a file triggers a custom StripTrailingSpaces() function (attached at the end of the question):

autocmd BufWritePre,FileWritePre,FileAppendPre,FilterWritePre <buffer>
        \ :keepjumps call StripTrailingSpaces(0)

After seeing Restore the cursor position after undoing text change made by a script, I got an idea to exclude the changes made by my StripTrailingSpaces() function from the undo history by merging undo record created by the function onto the end of the previous change in the buffer.

This way, when undoing changes, it would appear that the function didn't create it's own undo record at all.

To validate my idea I've used a simple test case: create a clean buffer and enter the following commands manually, or save the following block as a file and source it via:

vim +"source <saved-filename-here>"

normal ggiline one is full of aaaa
set undolevels=10 " splits the change into separate undo blocks

normal Goline two is full of bbbb
set undolevels=10

normal Goline three is full of cccc
set undolevels=10

undojoin
keepjumps %s/aaaa/zzzz/
normal u

As you can see, after undoing the last change in the buffer, that is creating the third line, the cursor is correctly returned to the second line in the file.

Since my test worked, I implemented an almost identical undojoin in my StripTrailingSpaces(). However, when I undo the last change after the function has run, the cursor is returned to the top most change in the file. This is often a stripped space and is not the position of the change I undojoin-ed to.

Can anyone think of why this would be? Better yet, can anyone suggest a fix?

function! StripTrailingSpaces(number_of_allowed_spaces)
    " Match all trailing spaces in a file
    let l:regex = [
                \ '\^\zs\s\{1,\}\$',
                \ '\S\s\{' . a:number_of_allowed_spaces . '\}\zs\s\{1,\}\$',
                \ ]

    " Join trailing spaces regex into a single, non-magic string
    let l:regex_str = '\V\(' . join(l:regex, '\|') . '\)'

    " Save current window state
    let l:last_search=@/
    let l:winview = winsaveview()

    try
        " Append the comming change onto the end of the previous change
        " NOTE: Fails if previous change doesn't exist
        undojoin
    catch
    endtry

    " Substitute all trailing spaces
    if v:version > 704 || v:version == 704 && has('patch155')
        execute 'keepjumps keeppatterns %s/' . l:regex_str . '//e'
    else
        execute 'keepjumps %s/' . l:regex_str . '//e'
        call histdel('search', -1)
    endif

    " Restore current window state
    call winrestview(l:winview)
    let @/=l:last_search
endfunction

Answer 1

This definitely looks like a bug with the substitute command to me. From what I can tell the substitute command will sporadically take over the change location to jump to for the undo block when it is included. I can't isolate the pattern - sometimes it will do this when the substitution happens > some number of times. Other times, the location of the substitution seems to affect when this happens. It seems to be very unreliable. I don't think it actually has anything to do with the undojoin command either as I have been able to reproduce this effect for other functions that don't make use of that. If you're interested, try the following:

 function! Test()
    normal ciwfoo
    normal ciwbar
    %s/one/two/
 endfunction

Try it on some different texts with different numbers of "ones" included and placed at different locations. You'll notice that afterwards sometimes undo will jump to the line where the first substitution occurred and other times it will jump to the place where the first normal command makes its change.

I think the solution here for you is going to be to do something like this:

undo
normal ma
redo

at the top of your function and then bind u to something like u'a in your function so that after the undo it will jump back to the place where the actual first change occurred as opposed to whatever randomness :s forces on you. Of course, it can't be quite that simple because you will have to unmap u once you've done your jump, etc., etc. but this pattern in general should give you a way to save the correct location and then jump back to it. Of course, you'll probably want to do all of this with some global variable instead of hijacking marks but you get the idea.

EDIT: After spending some time digging through the source code, it actually looks like the behavior that you're after is the bug. This is the chunk of code that determines where the cursor should be placed after an undo:

if (top < newlnum)
{
    /* If the saved cursor is somewhere in this undo block, move it to
     * the remembered position.  Makes "gwap" put the cursor back
     * where it was. */
    lnum = curhead->uh_cursor.lnum;
    if (lnum >= top && lnum <= top + newsize + 1)
    {
    MSG("Remembered Position.\n");
    curwin->w_cursor = curhead->uh_cursor;
    newlnum = curwin->w_cursor.lnum - 1;
    }
    else
    {
    char msg_buf[1000];
    MSG("First change\n");
    sprintf(msg_buf, "lnum: %d, top: %d, newsize: %d", lnum, top, newsize);
    MSG(msg_buf);
    /* Use the first line that actually changed.  Avoids that
     * undoing auto-formatting puts the cursor in the previous
     * line. */
    for (i = 0; i < newsize && i < oldsize; ++i)
        if (STRCMP(uep->ue_array[i], ml_get(top + 1 + i)) != 0)
        break;
    if (i == newsize && newlnum == MAXLNUM && uep->ue_next == NULL)
    {
        newlnum = top;
        curwin->w_cursor.lnum = newlnum + 1;
    }
    else if (i < newsize)
    {
        newlnum = top + i;
        curwin->w_cursor.lnum = newlnum + 1;
    }
    }
}

It's rather involved but basically what this does is check where the cursor was when the change was made and then if it's inside the change block for the undo then reset the cursor to that position for the gw command. Otherwise, it skips to the top most changed line and puts you there. What happens with substitute is that it activates this logic for each line that is substituted and so if one of those substitutions is in the undo block then it jumps to the position of the cursor before the undo (your desired behavior). Other times, none of the changes will be in that block so it will jump to the topmost changed line (probably what it should do). Therefore, I think the answer to your question is that your desired behavior (make a change but merge it with the previous change except for in determining where to place the cursor when the change is undone) is not currently supported by vim.

EDIT: This particular chunk of code resides in undo.c on line 2711 inside the undoredo function. Inside of u_savecommon is where the whole thing gets setup before undo is actually called and that's where the cursor position that ends up being used for the gw command exception is saved (undo.c line 385 and saved on line 548 when called on a synced buffer). The logic for the substitution command is located in ex_cmds.c on line 4268 which calls u_savecommon indirectly on line 5208 (calls u_savesub which calls u_savecommon).