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.

3 files changed, 79 insertions, 17 deletions

diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs
index 2c1047da..0640de3c 100644
--- a/helix-term/src/application.rs
+++ b/helix-term/src/application.rs
@@ -3,6 +3,7 @@ use futures_util::Stream;
 use helix_core::{
     config::{default_syntax_loader, user_syntax_loader},
     diagnostic::{DiagnosticTag, NumberOrString},
+    path::get_relative_path,
     pos_at_coords, syntax, Selection,
 };
 use helix_lsp::{lsp, util::lsp_pos_to_pos, LspProgressMap};
@@ -489,17 +490,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<CrosstermEvent, crossterm::ErrorKind>) {
diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs
index 375e7b4f..35c84601 100644
--- a/helix-term/src/commands/typed.rs
+++ b/helix-term/src/commands/typed.rs
@@ -268,11 +268,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");
     }
@@ -292,7 +287,7 @@ fn write_impl(
         None
     };
 
-    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();
@@ -607,7 +602,7 @@ fn write_all_impl(
             None
         };
 
-        doc.format_and_save(fmt, force)?;
+        doc.format_and_save::<_, PathBuf>(fmt, None, force)?;
     }
 
     if quit {
diff --git a/helix-term/tests/test/write.rs b/helix-term/tests/test/write.rs
index 4ac850c1..d2e6922f 100644
--- a/helix-term/tests/test/write.rs
+++ b/helix-term/tests/test/write.rs
@@ -35,7 +35,7 @@ async fn test_write() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[tokio::test]
+#[tokio::test(flavor = "multi_thread")]
 async fn test_write_quit() -> anyhow::Result<()> {
     let mut file = tempfile::NamedTempFile::new()?;
 
@@ -129,7 +129,64 @@ 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());
+                }),
+            ),
+        ],
+        false,
+    )
+    .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!(
+        helpers::platform_line("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!(
+        helpers::platform_line("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()?;