-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Check for external file modifications when writing #5805
Merged
the-mikedavis
merged 4 commits into
helix-editor:master
from
divarvel:protect-against-overwrites
Feb 8, 2023
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a17e53d
Protect against overwriting external changes
kov db1fbcf
Merge remote-tracking branch 'origin/master' into overwrite-new
divarvel 0343d92
Make overwrite protection configurable + small fixes
divarvel f64ff10
remove config field for overwrite protection
divarvel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit hesitant about adding a config option here. We should check for a nonsensical
mtime
value, but a user should also be able to manually bump the file viatouch
or just use:w!
to bypass the checkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added at the request of @the-mikedavis: #4734 (comment)
:w! already works, so removing the configuration bit would make things more annoying on systems with unreliable mtime, but not impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also give the setting a scary name and a scarier description if it makes sense to keep it (personally, i would rather use
:w!
than disabling the check entirely, but that's just me)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a config option was originally requested by @the-mikedavis: #4734 (comment). He thought thought that fighting the editor (
w!
) for every write would be annoying in those nieche cases.Detecting non-sensival mtime is a bit hard. We xould vhrck that the mtime is between the last saved mtime and now but here is usually a certain amount of racing going with tien/fs events so that could lead to false positives. That also still doesn't necessarily cstvh all false positives.
We could changes this option to fallback to sha1 hashes instead. Mtime is preferable bbecause it js much faster to retrieve and usually reliable. We don't usually want to compute hashes because it requires reading the file again and sha1 runs at around 250MB/sec so there would be some delay for multi gig files (although @cessen has developed a non-cryptographic alternative hash that is much faster but it's not quite mature enough yet but we could switch to that in the future)
I thaugh just adding an option to toggle this on/off would be easiest. What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of thoughts about using hashes:
I think where a hash is really going to come in handy is for things like persistent undo, where the hash can be stored with the on-disk undo data so that it can be appropriately invalidated if the file has been changed the next time it's loaded. Because you probably don't want to be storing an entire copy of the file just for undo stack invalidation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to get that fancy. If you take a look at Ropey's
PartialEq
impl forRopeSlice
, it solves basically the same problem (chunks not aligning) without any allocations. (In fact, IIRC you fixed a bug in one of those impls.) I think you can do the same thing here, which would let you read the file in fixed-size chunks, no additional allocations needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the sense of actually duplicating data in memory, yeah. And it does that deferred cloning incrementally as well (a single edit will only duplicate
log N
of the tree's data).In benchmarks post-clone edits are notably slower (by several times, I think). But given that Ropey already does 1-3 million edits a second (e.g. you could have well over 10,000 cursors and still edit at 60fps), in practice I doubt being a handful of times slower is going to have any real impact. Also, after the first edit in an area of the text, more edits to the same area won't have the post-cloning performance overhead anymore.
Then again, hashing instead of cloning could still make sense. The benefit of the hashing approach is that it only happens during syncing with disk, so even if it's a bit slower than a direct comparison, it probably won't impact usability in any meaningful way. Whereas holding on to a clone impacts the performance of all initial edits after a sync. Additionally, if we're eventually going to use hashing for persistent undo invalidation, then we'll be computing the hash on sync anyway.
Dunno. Both approaches have tradeoffs. I mainly just wanted to put the idea out there as a viable alternative to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right about cloning the rope here instead of hashing.
Just to clarify with diff gutters we currently always pay the overhead of cloning each chuck when it's edited.
The diff worker holds a clone of the rope in a different thread which is usually only replaced by the new rope shortly after the edit is finished.
I definitely want to remove this cost at some-point. That will likely require the addition of a
WeakRope
to ropey (just a wrapper aroundstd::arc::Weak
which can be upgraded back to a full Rope).But since this not a big problem in practice I think this simpler approach would be better suited than storing a hash since for this particular case this problem would only appear once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be ok with leaving out the option from this PR. We can revisit it if we see reports of the mtime check failing. I don't really remember the details of when I was fighting with mtime - I doubt that I will run into it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've removed the option