Save extmark undo information when writing to undofile#13973
Save extmark undo information when writing to undofile#13973bfredl merged 1 commit intoneovim:masterfrom
Conversation
|
@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). |
|
we should probably bump |
d5125a0 to
3ea0541
Compare
|
@bfredl Ready for review :) |
530e29e to
bf8aa8b
Compare
|
@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. |
|
@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). |
|
@bfredl Is there anything else I need to do to help get this merged? |
|
I think this is pretty much done. But we need to communicate clearly with master-followers so it doesn't come as a surprise :] |
|
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? |
bf8aa8b to
f42e932
Compare
|
@bfredl Should be ready for merge, unless you spot anything else. |
|
@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. |
|
@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. |
|
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. |
It's a versioned file format. Why wouldn't we keep the ability to read the old version?
There's not a logical way to transform the old version to the new 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.
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). |
|
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? |
|
@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 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. |
|
@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. |
|
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). |
|
My old undo-files aren't working anymore doesn't it breaks compatibility? |
|
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'
endif
…
endif |
|
Undofile incompatibility should probably be mentioned in :h vim-differences |
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. |
No idea what this law says, but vim has already broken it twice in the past. :)
Hmm, but the filename is different already, neovim uses
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). |
A program must not harm a user's data or through inaction allow a user's data to come to harm.
Neovim reads
Or, as in my case, with first usage of neovim with a vimrc that sets
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. |
Personally I've elected to stick with neovim 0.4.4, it's a great program and Vim-compatible. Just my 2 cents. |
|
@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 |
|
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: |
|
I didn't have enough patience to read |
|
@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! |
Fixes #13048.
@bfredl I keep getting a ton of detected leaks from Leak Sanitizer whenever using treesitter - most of these are from
nlua_regexin executor.c. Is this an actual leak, or is this just because memory ownership is taken by lua and LSAN doesn't know that?