ex_undojoin() sets b_u_curhead for no reason (which causes bugs)

[hardenedapple]

As far as I can see, setting curbuf->b_u_curhead in ex_undojoin() provides no benefit and introduces some bugs.

ex_undojoin() sets curbuf->b_u_curhead at the same time as setting curbuf->b_u_synced to FALSE. When the undo state is synced with u_sync(), both b_u_curhead is set to NULL and b_u_synced is set to TRUE (unless the undo list is corrupt and u_get_headentry returns NULL when called in u_getbot()).

Throughout the source code there are 3 places where b_u_curhead is used without resetting b_u_synced to TRUE, these are bugs 1 and 2 below, and what causes using undojoin twice in a function to give the error E790 undojoin is not allowed after undo.

There is only one place where b_u_synced is set to TRUE without setting b_u_curhead back to NULL, this is bug 3 below.

Overall I think not setting b_u_curhead in ex_undojoin() would fix these bugs (though they could be fixed in other ways) and would not cause any regressions.

The three bugs this introduces are:

1 :undojoin | redo

Essentially swaps the order of things

Test

With the file
test.txt

first line
second line

Run the following:
vim -N -u NONE test.txt

ixx<esc>
:undojoin | redo<CR>

Observe

leaves the buffer as it was opened
running undo puts the x's back into the buffer.
cannot go backwards in time from there -- undo history is false

2 :undojoin | write breaks the :earlier 1f command

Test

open a new file
vim -N -u NONE test.txt

first write to the file<esc>
:w<CR>
osecond write to the file<esc>
osecond line not written<esc>
:undojoin | w<CR>
:earlier 1f<CR>

Observe in the buffer

first write to the file
second write to the file

But this text was never written to disk.

3 Interrupt after :undojoin

Artificially send an interrupt during u_savecommon() after calling
ex_undojoin().
This leaves the undo tree with a superfluous branch, that hasn't
actually been undone but is there as if it had been.

With the file
orig_vim_debug_this.txt

Test line for actions

yy
:put
    attach gdb to nvim here -- run session provided below
:undojoin | delete
    artificial Ctrl-c in gdb in u_savecommon() for delete

Observe in the buffer

The second line was not removed
Pressing 'u' does not remove the line -- vim thinks the end of the
changelist has been reached.
Pressing 'Ctrl-r' removes the line -- vim now thinks it is a change we
have already undone.

If we make another change, that percieved available redo is moved to
an alternate branch.

vim [19:36:50] % gdb src/vim
Reading symbols from src/vim...(no debugging symbols found)...done.
(gdb) set pagination off
(gdb) python
>import subprocess as sp
>matching_processes = []
>for pid in [val.decode('utf-8') for val in
>            sp.check_output(['pgrep', 'vim']).splitlines()]:
>    with open('/proc/{}/cmdline'.format(pid), 'r') as infile:
>        cur_cmdline = infile.read()
>    if 'orig_vim_debug_this' in cur_cmdline:
>        matching_processes.append(pid)
>
>if len(matching_processes) == 1:
>    gdb.execute('attach {}'.format(matching_processes[0]))
>else:
>    print(matching_processes)
>end
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
0x00007fe462e37093 in __select_nocancel () from /usr/lib/libc.so.6
(gdb) break ex_undojoin
Breakpoint 1 at 0x58d9f0
(gdb) command 1
Type commands for breakpoint(s) 1, one per line.
End with a line saying just "end".
>silent
>set variable $savecommon_shouldbreak = 1
>cont
>end
(gdb) break u_savecommon if $savecommon_shouldbreak
Breakpoint 2 at 0x58b840
(gdb) command 2
Type commands for breakpoint(s) 2, one per line.
End with a line saying just "end".
>set variable $savecommon_shouldbreak = 0
>end
(gdb) cont
Continuing.

Breakpoint 2, 0x000000000058b840 in u_savecommon ()
(gdb) set variable got_int = 1
(gdb) cont
Continuing.

[brammool]

Thanks. You are right, b_u_curhead should only be set after undo.