neovim
feat(undo): re-sync undo files on external change
Problem: External changes cause the hashes of a file and its undo file to de-sync, leading to loss of undo history.
Solution: On hash mismatch, overwrite the buffer with the backup file. Underlying implementation requires that every :write operation happen twice to keep the original and backup files in sync. Provides 'undosync' option, reliant on 'backup'.
fix #33666
Instead of manually reading and writing buffers, I opted to call the built-in readfile and buf_write functions to do the heavy lifting. This should be much more stable.
It doesn't crash if binary files are overwritten by plaintext anymore.
This can yet be improved. Instead of having write calls happen twice, we can have buf_write_make_backup be called again in buf_write. I'll look into this later today
Failing tests all concern changed behavior, because the backups aren't the same anymore due to the copy operation at the end of the function. How can I help with this?
the backups aren't the same anymore due to the copy operation at the end of the function. How can I help with this?
Can the tests not be updated in a way so they are still meaningful but maybe are checking different conditions than before?
Is it important that "the backups are the same"?
Let's go ahead and plow through and enable this behavior as a new feature of 'backup'.
It's not called "new feature" if it cannot coexist with the existing behavior of 'backup'. It's called "breaking change".
'backup' is supposed to keep a copy of the previous version of the written file, not a copy of the current version.
Hence the suggestion for the 'undosync' option. The way I see it it's either put it back in or delete the failing tests, and I'm not too keen on the latter.
Don't want to delete the tests, but also don't see why this requires a new option. My suggestion about 'backup' was not intended to break any behavior of backup. https://github.com/neovim/neovim/issues/33666 describes the current behavior as:
result in empty undo history when later open the file in vim.
No one has explained why that behavior would ever be wanted or why we should continue to support it.
Is the new proposed option simply a side-effect of the current implementation in this PR?
result in empty undo history when later open the file in vim.
This is not a behavior of backup. It's a behavior of undo.
Yes, it does, because no one is has argued about preserving this behavior of undo yet, and undo and backup have been two independent features.
However, I do think there is a problem with the new undo syncing behavior: it can cause the size of the undo file to grow rapidly when switching between two branches of version control, since each time editing the file after a branch switch it'll add the entire difference between the two branches' versions to the undo history as a big single entry.
size of the undo file to grow rapidly when switching between two branches of version control
And this is common, so could be another hidden source of "unexplained performance issues". Even if it's "optional" (distros will enable it, people will copy it to their config).
So that needs to be addressed as part of this change. Possible ideas:
- show a warning when undofile gets very big
- add a healthcheck