Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vim-like Persistent Undo #9154

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Vim-like Persistent Undo #9154

wants to merge 31 commits into from

Conversation

kirawi
Copy link
Member

@kirawi kirawi commented Dec 23, 2023

Resolves persistent undo as part of #401
Depends upon #9236

Differences from Vim's implementation

  • Only one magic value is used instead of one for the file, two for the header, and two for the list of undo points.
  • Undofiles are appended to and are only overwritten if they are corrupted or the corresponding file was changed by some other process.
  • Because the client-server model hasn't yet been implemented, I added a :history-reload command that will merge the undofile's history to the instance's history. That way, it is possible for two separate clients to have worked on the same file and not totally erase the undofile. This command can be removed once it is implemented.

Remaining Tasks

  • Write tests
  • Write tests for expected errors
  • Write integration tests
  • Apply permissions to files the same as Vim
  • Add config option
  • Better error handling
  • Include a command to prune the history. A command to delete the undofile history might be a good enough alternative too (if someone accidentally deletes it, they can just :w to restore it).

Notes

  • tenthash could potentially be used in the future instead of blake3

@kirawi

This comment was marked as outdated.

@kirawi
Copy link
Member Author

kirawi commented Jan 1, 2024

I am rethinking utilizing blake3 as the hasher. We don't need it to be cryptographically secure and sha256's speed has worked well enough for Vim. The issue arises in that I intend to use the hash as a checksum to verify the undofile's integrity. Vim does not check after the fact. If, as I suspect, these undofiles grow big enough, then the hashing will take noticably more time. This could end up not being a problem at all in practice or there may be better solutions (such as pruning old branches).

Thoughts on using blake3, reverting to sha1, or waiting on benchmarks and real-world testing?

Edit: I found out about xxhash which seems to target this niche. xxhash-rust adds no further dependencies and its author is the same as clipboard-win.

Edit2: On second thought, I think it would be better to just not verify the undofile's integrity. The cost is not worth the rare case that it becomes subtly corrupted. I'll revert to sha1.

@kirawi kirawi force-pushed the undo branch 2 times, most recently from 6178527 to e46d4e8 Compare January 1, 2024 23:27
@intarga intarga mentioned this pull request Jan 4, 2024
12 tasks
@kirawi kirawi marked this pull request as ready for review January 5, 2024 00:10
@kirawi kirawi added A-helix-term Area: Helix term improvements A-command Area: Commands labels Jan 5, 2024
@kirawi kirawi added the S-waiting-on-pr Status: This is waiting on another PR to be merged first label Jan 13, 2024
@cessen
Copy link
Contributor

cessen commented Jan 16, 2024

Regarding hashes and persistent undo: I agree that the undo file getting corrupted is unlikely, and probably not worth considering.

However, as you've noted in the PR description, there is a high probability of the text file getting modified outside of Helix (checking out git branches, running auto formatters, making quick edits in other editors, etc.). So I think storing the hash for the text contents that the head of the undo file corresponds to does make sense, since time stamps can be fickle on some systems.

And with that in mind, I think using a faster hash still makes sense, since you'll be hashing text file contents on load/save. IIRC blake3 is notably faster than any of the SHA hashes, even single-threaded.

You might also consider tagging the hash value with what hash it is (Blake2, SHA1, whatever) so that it's easier to change it in the future if desired.

@kirawi
Copy link
Member Author

kirawi commented Jan 16, 2024

Yeah it already was using a hash (sha1), but I've switched it to xxhash since it seems more geared towards this kind of thing (e.g. rsync). I don't think it's necessary to store the hasher since we already have a VERSION tag in the header. I also got rid of mtime now since it's not really useful (and Vim seemed to be using it for its history rather than any validation).

@kirawi kirawi force-pushed the undo branch 2 times, most recently from 79476d1 to 6efad7b Compare January 17, 2024 00:48
@David-Else
Copy link
Contributor

David-Else commented Jan 17, 2024

I had a quick look at the code and can't find the config options, I assume I can turn this feature off? Also, I personally would like to see it off by default.

@kirawi
Copy link
Member Author

kirawi commented Jan 17, 2024

Ah, I forgot to document it. It's editor.undofile and it's off by default.

@kirawi kirawi changed the title Persistent Undo Vim-like Persistent Undo Jan 29, 2024
@kirawi kirawi added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Apr 9, 2024
@cole-h cole-h mentioned this pull request May 13, 2024
@fastfading
Copy link

can we proritize this ? a new version released . still no persistent undo
many people use helix to replace simple vim tool on server .
not many people will use this as a serious ide. we don't need any other fancy feature.
we just need the basic function like this feature.

@kirawi
Copy link
Member Author

kirawi commented Jul 16, 2024

I'm not currently working on major features for Helix for at least a few months. For the past few years, almost all my contributions have been to Helix, so right now I'm looking to focus on my own projects. This PR will be completed eventually, it'll just take longer.

@EricHenry
Copy link
Contributor

@kirawi want some help finishing this one up?

@kirawi
Copy link
Member Author

kirawi commented Sep 19, 2024

I restarted work on this on a private branch already, and I'll try to get that over the finish line. If I'm not available for it, then I'll make that branch public and allow someone to adopt the PR.

@kirawi
Copy link
Member Author

kirawi commented Oct 30, 2024

Just a minor status update: We'll be using an SQLite DB to host the persisted file histories instead of a bespoke file format. The underlying architecture will have to be significantly changed since revisions won't be serialized in the same way. Otherwise, I have addressed some bugs with the architecture that would undesirably overwrite valid undofiles or incorrectly merge to other clients.

@kirawi
Copy link
Member Author

kirawi commented Nov 14, 2024

I'm fairly busy right now, so if anyone is interested and has the experience to tackle this, please message me on Matrix (I am djo_kirawi). Otherwise, I will continue to work on it as I am available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants