Protect against overwriting external changes #4734

kov · 2022-11-13T22:00:53Z

When saving, check that the file has not changed before our last known save before writing the file. Only overwrite external changes if asked to force.

I've almost lost a bunch of work because I did it on a separate instance of helix in another terminal window, but had the same file opened on an earlier instance and saved the file without realizing. Thankfully I had a backup (a cat in another tab xD)

helix-view/src/document.rs

When saving, check that the file has not changed before our last known save before writing the file. Only overwrite external changes if asked to force.

kirawi · 2022-11-29T03:25:25Z

helix-view/src/document.rs

@@ -979,6 +1000,7 @@ impl Document {
            rev
        );
        self.last_saved_revision = rev;
+        self.last_saved_time = SystemTime::now();


Since the file isn't saved, I don't think the last_saved_time should be set.

My thinking here is to mimic the way History handles revisions and last_saved_revision here. They have an initial value as if the file had been saved at the time it was opened, this avoids adding a lot of unnecessary handling for an Option<>.

kirawi · 2022-11-29T03:27:15Z

helix-view/src/document.rs

+                    }
+                }
+            }
+
            let mut file = File::create(&path).await?;
            to_writer(&mut file, encoding, &text).await?;



We should set save time here.

I don't think this would be correct, or possible due to this happening inside the future, self would need to be captured. The state is only committed when the DocumentSavedEvent that is created here gets processed by the caller of the future. I decided to set the time in set_last_saved_revision() because there is no reason I can think of you'd want to call that without updating the time, and that ensures we do not forget to do it if more call sites are added for set_last_saved_revision().

pascalkuthe

I recently got bitten pretty badly by helix habing nor protection fkr this case and lost about a days work. I looked into a watchmen based solution (which is more accurate as it uses files to synchronize os events and also offers a lot for other features so it's something I want to look into in the future) but I concluded that we want mtime based overwrite protection for a couple of reasons:

watchmen is an external program, this functionality is basic/important enough that it should work without watchmen
watchmen is designed to watch a directories. We can use the root dir but this doesn't work when helix opens other random files with :open or with the LSP.

For these reasons I was looking to implement mtime based overwrite protection as a fallback. I got sidetracked and just found that there is an existing PR for this so it's a good thing I didn't already get started with this.

Implementation wise this is pretty simple and LGTM. I would have implemented this the same way. I think it makes sense to treat last_saved_time the same as last_saved_rev.

The only nitpick: last_saved_time should be added to the Debug implementation of Document

the-mikedavis · 2023-01-17T15:58:17Z

I think this should be configurable: I don't quite remember the context where this has happened to me previously (maybe it involved containers, virtual machines or external formatters) but if you're in a situation where mtime isn't reliable then you have to fight the editor to save files every time.

kirawi · 2023-01-17T16:10:32Z

Perhaps we could check if the buffer's content differs from the file's if the file was written at a later mtime?

pascalkuthe · 2023-01-17T16:10:42Z

I think this should be configurable: I don't quite remember the context where this has happened to me previously (maybe it involved containers, virtual machines or external formatters) but if you're in a situation where mtime isn't reliable then you have to fight the editor to save files every time.

That sounds reasonable and something I didn't think about. Adding a simple configuration flag like editor.enable-overwrite-protection should be pretty trivial. I think it would be good to enable that by default tough. mtime being unreliable is probably an exception rather than the rule.

pascalkuthe · 2023-01-17T16:20:37Z

Perhaps we could check if the buffer's content differs from the file's if the file was written at a later mtime?

That would work but I think that would add a bunch of extra overhead. Especially when writing large files so probably not ideal.
I think a config option as @the-mikedavis suggested should be good enough as mtime usually works fine and if mtime doesn't work it usually doesn't ever work so you would want to just want to totally disable the check in the config then.

I can't really conceive how you would get a false positive here (false negatives are possible because any fs operation is fundamentally racy but there is no way around that) but in the unlikely case they occur you can simply use :w! which is not too bad.

LeoniePhiline · 2023-01-19T21:18:56Z

helix-term/tests/test/helpers.rs

@@ -322,6 +322,12 @@ impl AppBuilder {
    }
 }

+pub async fn run_event_loop_to_idle(app: &mut Application) {


Suggested change

pub async fn run_event_loop_to_idle(app: &mut Application) {

pub async fn run_event_loop_until_idle(app: &mut Application) {

matching app.event_loop_until_idle(...)

divarvel · 2023-02-03T14:32:38Z

Since a lot of people are waiting on this feature, i took the liberty to apply requested feedback and merge with the latest master: https://github.com/divarvel/helix/tree/protect-against-overwrites

@kov I have opened a PR against your branch, but the merge from master makes it a bit hard to read. You can also cherry-pick commits if you want and rebase everything on top of a recent master (the conflicts are trivial to solve).

@pascalkuthe i can also open a new PR and rebase everything on top of the latest master, if that's simpler for maintainers.

pascalkuthe · 2023-02-03T14:38:42Z

Since a lot of people are waiting on this feature, i took the liberty to apply requested feedback and merge with the latest master: divarvel/helix@protect-against-overwrites

@kov I have opened a PR against your branch, but the merge from master makes it a bit hard to read. You can also cherry-pick commits if you want and rebase everything on top of a recent master (the conflicts are trivial to solve).

@pascalkuthe i can also open a new PR and rebase everything on top of the latest master, if that's simpler for maintainers.

Since there wasn't repose here in a while you can just open a new PR that supersedes. Aslong as you keep the original commits around to give credit and give credit in the PR description that is fine.

divarvel · 2023-02-03T14:47:19Z

Alright, done in #5805

kov · 2023-02-05T01:11:21Z

I'm closing this in favour of #5805 thank you =)

kov force-pushed the protect-against-overwrites branch from 3735792 to 6a392b3 Compare November 13, 2022 22:47

kirawi reviewed Nov 14, 2022

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

kov force-pushed the protect-against-overwrites branch from 11fd349 to 43cbb1a Compare November 14, 2022 11:54

Protect against overwriting external changes

a17e53d

When saving, check that the file has not changed before our last known save before writing the file. Only overwrite external changes if asked to force.

kov force-pushed the protect-against-overwrites branch from 43cbb1a to a17e53d Compare November 14, 2022 11:55

kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 14, 2022

kirawi reviewed Nov 29, 2022

View reviewed changes

pascalkuthe approved these changes Jan 17, 2023

View reviewed changes

pascalkuthe 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-review Status: Awaiting review from a maintainer. labels Jan 17, 2023

LeoniePhiline reviewed Jan 19, 2023

View reviewed changes

David-Else mentioned this pull request Jan 25, 2023

:reload should fail if unsaved changes #5680

Open

divarvel mentioned this pull request Feb 3, 2023

Protect against overwrites kov/helix#1

Open

divarvel mentioned this pull request Feb 3, 2023

Check for external file modifications when writing #5805

Merged

kov closed this Feb 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect against overwriting external changes #4734

Protect against overwriting external changes #4734

kov commented Nov 13, 2022

kirawi Nov 29, 2022

kov Dec 9, 2022

kirawi Nov 29, 2022

kov Dec 9, 2022

pascalkuthe left a comment

the-mikedavis commented Jan 17, 2023

kirawi commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

LeoniePhiline Jan 19, 2023

divarvel commented Feb 3, 2023

pascalkuthe commented Feb 3, 2023 •

edited

Loading

divarvel commented Feb 3, 2023

kov commented Feb 5, 2023

	pub async fn run_event_loop_to_idle(app: &mut Application) {
	pub async fn run_event_loop_until_idle(app: &mut Application) {

Protect against overwriting external changes #4734

Protect against overwriting external changes #4734

Conversation

kov commented Nov 13, 2022

kirawi Nov 29, 2022

Choose a reason for hiding this comment

kov Dec 9, 2022

Choose a reason for hiding this comment

kirawi Nov 29, 2022

Choose a reason for hiding this comment

kov Dec 9, 2022

Choose a reason for hiding this comment

pascalkuthe left a comment

Choose a reason for hiding this comment

the-mikedavis commented Jan 17, 2023

kirawi commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

LeoniePhiline Jan 19, 2023

Choose a reason for hiding this comment

divarvel commented Feb 3, 2023

pascalkuthe commented Feb 3, 2023 • edited Loading

divarvel commented Feb 3, 2023

kov commented Feb 5, 2023

pascalkuthe commented Feb 3, 2023 •

edited

Loading