fix(write): do not set new path on document until write succeeds

If a document is written with a new path, currently, in the event that
the write fails, the document still gets its path changed. This fixes
it so that the path is not updated unless the write succeeds.

Author: dead10ck

helix-term/src/application.rs

@@ -2,6 +2,7 @@ use arc_swap::{access::Map, ArcSwap};
 use futures_util::Stream;
 use helix_core::{
     config::{default_syntax_loader, user_syntax_loader},
+    path::get_relative_path,
     pos_at_coords, syntax, Selection,
 };
 use helix_lsp::{lsp, util::lsp_pos_to_pos, LspProgressMap};
@@ -452,17 +453,26 @@ impl Application {
         );
 
         doc.set_last_saved_revision(doc_save_event.revision);
+
         let lines = doc.text().len_lines();
         let bytes = doc.text().len_bytes();
 
-        let path_str = doc
-            .path()
-            .expect("document written without path")
-            .to_string_lossy()
-            .into_owned();
-
-        self.editor
-            .set_status(format!("'{}' written, {}L {}B", path_str, lines, bytes));
+        if let Err(err) = doc.set_path(Some(&doc_save_event.path)) {
+            log::error!(
+                "error setting path for doc '{:?}': {}",
+                doc.path(),
+                err.to_string(),
+            );
+            self.editor.set_error(err.to_string());
+        } else {
+            // TODO: fix being overwritten by lsp
+            self.editor.set_status(format!(
+                "'{}' written, {}L {}B",
+                get_relative_path(&doc_save_event.path).to_string_lossy(),
+                lines,
+                bytes
+            ));
+        }
     }
 
     pub fn handle_terminal_events(&mut self, event: Result<Event, crossterm::ErrorKind>) {

helix-term/src/commands/typed.rs

@@ -199,11 +199,6 @@ fn write_impl(
     let jobs = &mut cx.jobs;
     let doc = doc_mut!(cx.editor);
 
-    if let Some(ref path) = path {
-        doc.set_path(Some(path.as_ref().as_ref()))
-            .context("invalid filepath")?;
-    }
-
     if doc.path().is_none() {
         bail!("cannot write a buffer without a filename");
     }
@@ -220,7 +215,7 @@ fn write_impl(
         shared
     });
 
-    doc.format_and_save(fmt, force)?;
+    doc.format_and_save(fmt, path.map(AsRef::as_ref), force)?;
 
     if path.is_some() {
         let id = doc.id();
@@ -483,7 +478,7 @@ fn write_all_impl(
             shared
         });
 
-        doc.format_and_save(fmt, force)?;
+        doc.format_and_save::<_, PathBuf>(fmt, None, force)?;
     }
 
     if quit {

helix-term/tests/test/write.rs

@@ -117,7 +117,57 @@ async fn test_write_fail_mod_flag() -> anyhow::Result<()> {
 }
 
 #[tokio::test]
-#[ignore]
+async fn test_write_new_path() -> anyhow::Result<()> {
+    let mut file1 = tempfile::NamedTempFile::new().unwrap();
+    let mut file2 = tempfile::NamedTempFile::new().unwrap();
+
+    test_key_sequences(
+        &mut Application::new(
+            Args {
+                files: vec![(file1.path().to_path_buf(), Position::default())],
+                ..Default::default()
+            },
+            Config::default(),
+        )?,
+        vec![
+            (
+                Some("ii can eat glass, it will not hurt me<ret><esc>:w<ret>"),
+                Some(&|app| {
+                    let doc = doc!(app.editor);
+                    assert!(!app.editor.is_err());
+                    assert_eq!(file1.path(), doc.path().unwrap());
+                }),
+            ),
+            (
+                Some(&format!(":w {}<ret>", file2.path().to_string_lossy())),
+                Some(&|app| {
+                    let doc = doc!(app.editor);
+                    assert!(!app.editor.is_err());
+                    assert_eq!(file2.path(), doc.path().unwrap());
+                    assert!(app.editor.document_by_path(file1.path()).is_none());
+                }),
+            ),
+        ],
+    )
+    .await?;
+
+    file1.as_file_mut().flush()?;
+    file1.as_file_mut().sync_all()?;
+    file2.as_file_mut().flush()?;
+    file2.as_file_mut().sync_all()?;
+
+    let mut file1_content = String::new();
+    file1.as_file_mut().read_to_string(&mut file1_content)?;
+    assert_eq!("i can eat glass, it will not hurt me\n", file1_content);
+
+    let mut file2_content = String::new();
+    file2.as_file_mut().read_to_string(&mut file2_content)?;
+    assert_eq!("i can eat glass, it will not hurt me\n", file2_content);
+
+    Ok(())
+}
+
+#[tokio::test]
 async fn test_write_fail_new_path() -> anyhow::Result<()> {
     let file = helpers::new_readonly_tempfile()?;
 

helix-view/src/document.rs

@@ -90,6 +90,7 @@ impl Serialize for Mode {
 pub struct DocumentSaveEvent {
     pub revision: usize,
     pub doc_id: DocumentId,
+    pub path: PathBuf,
 }
 
 pub type DocumentSaveEventResult = Result<DocumentSaveEvent, anyhow::Error>;
@@ -456,39 +457,61 @@ impl Document {
         Some(fut)
     }
 
-    pub fn save(&mut self, force: bool) -> Result<(), anyhow::Error> {
-        self.save_impl::<futures_util::future::Ready<_>>(None, force)
+    pub fn save<P: Into<PathBuf>>(
+        &mut self,
+        path: Option<P>,
+        force: bool,
+    ) -> Result<(), anyhow::Error> {
+        self.save_impl::<futures_util::future::Ready<_>, _>(None, path, force)
     }
 
-    pub fn format_and_save(
+    pub fn format_and_save<F, P>(
         &mut self,
-        formatting: Option<impl Future<Output = LspFormatting> + 'static + Send>,
+        formatting: Option<F>,
+        path: Option<P>,
         force: bool,
-    ) -> anyhow::Result<()> {
-        self.save_impl(formatting, force)
+    ) -> anyhow::Result<()>
+    where
+        F: Future<Output = LspFormatting> + 'static + Send,
+        P: Into<PathBuf>,
+    {
+        self.save_impl(formatting, path, force)
     }
 
-    // TODO: impl Drop to handle ensuring writes when closed
     /// The `Document`'s text is encoded according to its encoding and written to the file located
     /// at its `path()`.
     ///
     /// If `formatting` is present, it supplies some changes that we apply to the text before saving.
-    fn save_impl<F: Future<Output = LspFormatting> + 'static + Send>(
+    fn save_impl<F, P>(
         &mut self,
         formatting: Option<F>,
+        path: Option<P>,
         force: bool,
-    ) -> Result<(), anyhow::Error> {
+    ) -> Result<(), anyhow::Error>
+    where
+        F: Future<Output = LspFormatting> + 'static + Send,
+        P: Into<PathBuf>,
+    {
         if self.save_sender.is_none() {
             bail!("saves are closed for this document!");
         }
 
         // we clone and move text + path into the future so that we asynchronously save the current
         // state without blocking any further edits.
-
         let mut text = self.text().clone();
-        let path = self.path.clone().expect("Can't save with no path set!");
-        let identifier = self.identifier();
 
+        let path = match path {
+            Some(path) => helix_core::path::get_canonicalized_path(&path.into())?,
+            None => {
+                if self.path.is_none() {
+                    bail!("Can't save with no path set!");
+                }
+
+                self.path.as_ref().unwrap().clone()
+            }
+        };
+
+        let identifier = self.identifier();
         let language_server = self.language_server.clone();
 
         // mark changes up to now as saved
@@ -520,12 +543,13 @@ impl Document {
                 }
             }
 
-            let mut file = File::create(path).await?;
+            let mut file = File::create(&path).await?;
             to_writer(&mut file, encoding, &text).await?;
 
             let event = DocumentSaveEvent {
                 revision: current_rev,
                 doc_id,
+                path,
             };
 
             if let Some(language_server) = language_server {