-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Protect against overwriting external changes #4734
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ use std::future::Future; | |
use std::path::{Path, PathBuf}; | ||
use std::str::FromStr; | ||
use std::sync::Arc; | ||
use std::time::SystemTime; | ||
|
||
use helix_core::{ | ||
encoding, | ||
|
@@ -127,6 +128,10 @@ pub struct Document { | |
|
||
pub savepoint: Option<Transaction>, | ||
|
||
// Last time we wrote to the file. This will carry the time the file was last opened if there | ||
// were no saves. | ||
last_saved_time: SystemTime, | ||
|
||
last_saved_revision: usize, | ||
version: i32, // should be usize? | ||
pub(crate) modified_since_accessed: bool, | ||
|
@@ -368,6 +373,7 @@ impl Document { | |
version: 0, | ||
history: Cell::new(History::default()), | ||
savepoint: None, | ||
last_saved_time: SystemTime::now(), | ||
last_saved_revision: 0, | ||
modified_since_accessed: false, | ||
language_server: None, | ||
|
@@ -557,9 +563,11 @@ impl Document { | |
|
||
let encoding = self.encoding; | ||
|
||
let last_saved_time = self.last_saved_time; | ||
|
||
// We encode the file according to the `Document`'s encoding. | ||
let future = async move { | ||
use tokio::fs::File; | ||
use tokio::{fs, fs::File}; | ||
if let Some(parent) = path.parent() { | ||
// TODO: display a prompt asking the user if the directories should be created | ||
if !parent.exists() { | ||
|
@@ -571,6 +579,17 @@ impl Document { | |
} | ||
} | ||
|
||
// Protect against overwriting changes made externally | ||
if !force { | ||
if let Ok(metadata) = fs::metadata(&path).await { | ||
if let Ok(mtime) = metadata.modified() { | ||
if last_saved_time < mtime { | ||
bail!("file modified by an external process, use :w! to force"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
let mut file = File::create(&path).await?; | ||
to_writer(&mut file, encoding, &text).await?; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should set save time here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(). |
||
|
@@ -644,6 +663,8 @@ impl Document { | |
self.append_changes_to_history(view.id); | ||
self.reset_modified(); | ||
|
||
self.last_saved_time = SystemTime::now(); | ||
|
||
self.detect_indent_and_line_ending(); | ||
|
||
Ok(()) | ||
|
@@ -979,6 +1000,7 @@ impl Document { | |
rev | ||
); | ||
self.last_saved_revision = rev; | ||
self.last_saved_time = SystemTime::now(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the file isn't saved, I don't think the last_saved_time should be set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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<>. |
||
} | ||
|
||
/// Get the document's latest saved revision. | ||
|
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.
matching
app.event_loop_until_idle(...)