Skip to content

Save extmark undo information when writing to undofile#13973

Merged
bfredl merged 1 commit intoneovim:masterfrom
chentoast:on_bytes_undofile
Mar 3, 2021
Merged

Save extmark undo information when writing to undofile#13973
bfredl merged 1 commit intoneovim:masterfrom
chentoast:on_bytes_undofile

Conversation

@chentoast
Copy link
Contributor

@chentoast chentoast commented Feb 20, 2021

Fixes #13048.

@bfredl I keep getting a ton of detected leaks from Leak Sanitizer whenever using treesitter - most of these are from nlua_regex in executor.c. Is this an actual leak, or is this just because memory ownership is taken by lua and LSAN doesn't know that?

@bfredl
Copy link
Member

bfredl commented Feb 20, 2021

@chentau not sure. I frequently get leaks with ASAN build locally but only a few objects. though I think we close lua state on exit (with ASAN build).

@bfredl
Copy link
Member

bfredl commented Feb 20, 2021

we should probably bump UF_VERSION . We might end up doing incompatible changes in 0.6 (extmark undo refactor), and then we can bump it again.

@chentoast
Copy link
Contributor Author

@bfredl Ready for review :)

@chentoast chentoast changed the title [WIP] Save extmark undo information when writing to undofile Save extmark undo information when writing to undofile Feb 21, 2021
@chentoast chentoast force-pushed the on_bytes_undofile branch 2 times, most recently from 530e29e to bf8aa8b Compare February 21, 2021 23:59
@chentoast
Copy link
Contributor Author

@bfredl Should I update the following head wiki page after this is merged? I don’t see a way for this change to not break previous undofiles. Hopefully that won’t be too big of an issue.

@bfredl
Copy link
Member

bfredl commented Feb 24, 2021

@chentau I think so. I think we consider undofile serialization of runtime state (for crash recovery etc), rather than long time storage. In case there is a mutiny we can add back compatibility of previous version (which shouldn't be too hard, but not worth it if no one uses it).

@chentoast
Copy link
Contributor Author

@bfredl Is there anything else I need to do to help get this merged?

@bfredl
Copy link
Member

bfredl commented Feb 28, 2021

I think this is pretty much done. But we need to communicate clearly with master-followers so it doesn't come as a surprise :]

@akinsho
Copy link
Contributor

akinsho commented Mar 2, 2021

I'm looking forward to this change but just curious what a user will have to be warned of? will the old undo files error, will they need to be all deleted or will they continue to work but still reproduce the original bug and to get the benefits of this PR they will need to removed so new compatible ones can be created?

@chentoast chentoast force-pushed the on_bytes_undofile branch from bf8aa8b to f42e932 Compare March 2, 2021 20:46
@chentoast
Copy link
Contributor Author

@bfredl Should be ready for merge, unless you spot anything else.

@chentoast
Copy link
Contributor Author

chentoast commented Mar 2, 2021

@akinsho As of right now, the behavior will be the former; since this format of the undofile has changed, any undofiles before this pr will be outdated and won't work. If re-creating all undofiles proves to be too inconvenient for people, then I'll submit another PR implementing backwards compatibility, although then the old undofiles will still errror out with treesitter.

@akinsho
Copy link
Contributor

akinsho commented Mar 2, 2021

@chentau this particular bug is easily the most annoying thing I face on a daily basis whilst editing so really don't mind trading off against losing undofiles they are a nice to have vs. error free highlighting 😄 , was more just curious about what to expect.

@bfredl bfredl merged commit 6995fad into neovim:master Mar 3, 2021
@bfredl
Copy link
Member

bfredl commented Mar 3, 2021

Merged. New versions of neovim will not read undofiles written by nvim before this merge (there will be an error message about incopmatible version). Nvim 0.4 (or an master up to bda1292 ) can be used to recover older undofiles, so if you worried about unsaved changes lurking around in undofiles it would make sense to keep such version around somewhere to recover them.

This is a necessary change to keep tree-sitter and plugins dependent on byte-level buffer change events fully working with undo states from a undofile. If there is a clear demand we might implement reading of the old format. Such recovered buffers will not be be fully functional with plugins relying on buffer updates or tree-sitter, however.

@chentoast chentoast deleted the on_bytes_undofile branch March 3, 2021 21:20
@jamessan
Copy link
Member

jamessan commented Mar 4, 2021

If there is a clear demand we might implement reading of the old format.

It's a versioned file format. Why wouldn't we keep the ability to read the old version?

Such recovered buffers will not be be fully functional with plugins relying on buffer updates or tree-sitter, however.

There's not a logical way to transform the old version to the new version?

@bfredl
Copy link
Member

bfredl commented Mar 4, 2021

It's a versioned file format. Why wouldn't we keep the ability to read the old version?

IIUC undofiles is intended to be more of a crash recovery or session suspend-restore thing than permanent storage of history. for instance: if the file contents doesn't match the current state in the undofile exactly the entire undofile already becomes unusable. So I think the version is more for avoid reading incompatible file by mistake, rather than version multiple files in a permanent storage.

There's not a logical way to transform the old version to the new version?

Not really. undo headers in vim operate on entire changed lines only, while we are adding byte-level change resolution. we could, in principle, construct faked byte edits that replace the entire line region, though this will cause performance regressions with tree-sitter and unexpected behaviours with plugins using byte resolution updates (which includes extmarks).

@lervag
Copy link
Contributor

lervag commented Mar 5, 2021

I'm sorry about the silly question, but: It seems this change creates a definite incompatibility with Vim. I know there is no goal of staying compatible, but I've actually been able to keep compatibility until this moment. Do I understand correctly, or am I missing something simple that can establish compatibility again?

@chentoast
Copy link
Contributor Author

@lervag As far as I'm aware, there's no easy way to establish compatibility going forward. This is because we are serializing additional byte level information about edits, and Vim has no analogue of this. I guess the reason I made this PR is because without this extra information, undofiles will not work with any plugins that leverage the on_bytes mechanism, which include plugins like tree-sitter and basically anything that uses extmark highlighting/virtual text.

That being said, if you have an idea for how to implement compatibility going forward, I'd be happy to submit another PR, since I agree that the breaking of compatibility is unfortunate.

@bfredl
Copy link
Member

bfredl commented Mar 6, 2021

@lervag if you mean mutual compatibility i e open the same file back and forward both in vim and neovim, then that will require also vim storing detailed byte tracking in undo headers. The thing is tho is that the current format (this PR) is not really what we want (it works for tree-sitter but is not detailed enough for LSP for instance) but for 0.6 I hope to implement more fully-featured undo tracking and then we should seriously discuss with vim-dev about a new shared format.

@lervag
Copy link
Contributor

lervag commented Mar 6, 2021

Thanks @chentau and @bfredl, I appreciate your responses and clarifications. My main "concern" here is that, as a plugin author, there will be slightly more of a hazzle to keep my plugins compatible with both Vim and neovim (since I will now probably decouple my Vim and neovim configs).

As a happy user of neovim, I strongly respect and acknowledge all of the very good work you and all the other devs are doing. And I don't mind that your development of neovim will require me both to learn new stuff (e.g., I will probably have to learn Lua at some time :p) and to make some choices (such as this: Vim vs neovim).

@Bluesboy
Copy link

Bluesboy commented Mar 6, 2021

My old undo-files aren't working anymore doesn't it breaks compatibility?

@bfredl
Copy link
Member

bfredl commented Mar 6, 2021

@blueyed
Copy link
Contributor

blueyed commented Apr 13, 2021

I think it might be worth reading the old format, and writing/converting it to the new format. This should get enabled explicitly though however.

As for me I'm using the following for now, i.e. a separate dir for nvim-0.5, which results in missing undo information now however..!

if has('persistent_undo')
  if has('nvim-0.5')
    " New format in https://github.com/neovim/neovim/pull/13973 (f42e932,
    " 2021-04-13).
    let &undodir = s:vimsharedir . '/undo2'
  else
    let &undodir = s:vimsharedir . '/undo'
  endifendif

@MRAAGH
Copy link

MRAAGH commented Jul 4, 2021

Undofile incompatibility should probably be mentioned in :h vim-differences

@davidchisnall
Copy link

IIUC undofiles is intended to be more of a crash recovery or session suspend-restore thing than permanent storage of history

I strongly disagree with this. The fact that I have undo history for files in source trees that I have not yet committed, that is preserved across reboots and isn't lost when I go away from the tree for a few months, is by far my favourite vim feature: it respects Raskin's first law.

This was my first interaction with neovim: It could not read any of the history that I have in my undofiles directory (~/.cache/vim/undo/ for me) and so lost state. The state was recoverable by using vim, but because the filename is not changed I have to modify my vimrc to configure vim and neovim to use different undo locations.

This could have avoided violating the principle of least astonishment by changing the filename. It could have avoided sharp data loss by reading the old format but writing only the new format. If the new format had a different name then this could have been transparent: the old file would have the undo history up to the point neovim 0.5 was used and so you could undo right back to the start, but new changes would be visible only with the new editor.

@bfredl
Copy link
Member

bfredl commented Jul 13, 2021

it respects Raskin's first law.

No idea what this law says, but vim has already broken it twice in the past. :)

This was my first interaction with neovim: It could not read any of the history that I have in my undofiles directory (~/.cache/vim/undo/ for me) and so lost state. [...] This could have avoided violating the principle of least astonishment by changing the filename.

Hmm, but the filename is different already, neovim uses ~/.cache/nvim/undo/. The issue you mention would occur when switching from neovim 0.4 to 0.5 but not with first usage of neovim.

. It could have avoided sharp data loss by reading the old format but writing only the new format.

It shouldn't overwrite an unreadable file tho, unless I have completely misread the code. Seems when you say "sharp data loss" and "lost state" you actually mean unreadable by new version but still recoverable by using old version? These conditions differ quite a lot in severity so best to not confuse them :)

Reading old files in a fallback mode have already been suggested several times. As said before it is doable in principle but the open question is what happen when using such a buffer with byte state dependent plugins (such as all tree-sitter plugins).

@davidchisnall
Copy link

No idea what this law says, but vim has already broken it twice in the past. :)

A program must not harm a user's data or through inaction allow a user's data to come to harm.

Hmm, but the filename is different already, neovim uses ~/.cache/nvim/undo/.

Neovim reads $VIMINIT, which for me is set to source ${XDG_CONFIG_HOME}/vimrc. It therefore reads my vimrc file, which sets the undo location. I had to modify my vimrc with an if has("nvim") block to set these locations.

The issue you mention would occur when switching from neovim 0.4 to 0.5 but not with first usage of neovim.

Or, as in my case, with first usage of neovim with a vimrc that sets undodir.

It shouldn't overwrite an unreadable file tho, unless I have completely misread the code.

It doesn't, but because it reads the vim-specific config file from the vim-specified location it then writes a file in that location with the name that vim expects, so files that I first edited in neovim ended up with undo files that vim couldn't read and vice versa. Both programs complained.

I've heard great things about neovim, but this caused enough frustration in my first 10 minutes of trying to use it that I ended up uninstalling it and reverting back to vim.

@MRAAGH
Copy link

MRAAGH commented Jul 13, 2021

I ended up uninstalling it and reverting back to vim.

Personally I've elected to stick with neovim 0.4.4, it's a great program and Vim-compatible. Just my 2 cents.

@EdmundsEcho
Copy link

@blueyed Thank you for posting the temporary fix. Your suggestion that the v0.6 convert the old undo format to the new is a good one. Clearly the effort required is likely non-trivial because the desired outcome (non-breaking change), is clearly the dominant choice. The suggestion to fix this, is it something to expect in the iteration or so?

The error 824 suggests the cause is that the undo was written using a newer version... not as is the case where the new version (0.6) cannot read the old version.

- E

@dkiyatkin
Copy link

When I want to see old undo history, before lose it, I use this docker-command with nvim 0.4.4. Maybe someone find it usefull:

docker run -it -v `pwd`:/mnt/volume --workdir=/mnt/volume anatolelucet/neovim:0.4.4 nvim --cmd "set undodir=." --cmd "set undofile" my_file_with_old_undo

@Arnie97
Copy link
Contributor

Arnie97 commented Aug 26, 2022

I didn't have enough patience to read undo.c and build a real .un~ parser.
However I wrote a crappy script that simulates the changes in this patch and just works in my (insufficient) tests: ufbump

@EricDuminil
Copy link

@Arnie97 : Thanks a lot for your script! It seems to have worked fine, and upgraded my precious undofiles from 0.4.4 to 0.8.2!

@neovim neovim locked as resolved and limited conversation to collaborators Jan 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[treesitter] errors with undofile