Read symlink when writing files (#10339)

Co-authored-by: Pascal Kuthe <pascalkuthe@pm.me>

helix-term/tests/test/commands/write.rs

@@ -529,6 +529,81 @@ async fn test_write_all_insert_final_newline_do_not_add_if_unmodified() -> anyho
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread")]
+async fn test_symlink_write() -> anyhow::Result<()> {
+    #[cfg(unix)]
+    use std::os::unix::fs::symlink;
+    #[cfg(not(unix))]
+    use std::os::windows::fs::symlink_file as symlink;
+
+    let dir = tempfile::tempdir()?;
+
+    let mut file = tempfile::NamedTempFile::new_in(&dir)?;
+    let symlink_path = dir.path().join("linked");
+    symlink(file.path(), &symlink_path)?;
+
+    let mut app = helpers::AppBuilder::new()
+        .with_file(&symlink_path, None)
+        .build()?;
+
+    test_key_sequence(
+        &mut app,
+        Some("ithe gostak distims the doshes<ret><esc>:w<ret>"),
+        None,
+        false,
+    )
+    .await?;
+
+    reload_file(&mut file).unwrap();
+    let mut file_content = String::new();
+    file.as_file_mut().read_to_string(&mut file_content)?;
+
+    assert_eq!(
+        LineFeedHandling::Native.apply("the gostak distims the doshes"),
+        file_content
+    );
+    assert!(symlink_path.is_symlink());
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_symlink_write_fail() -> anyhow::Result<()> {
+    #[cfg(unix)]
+    use std::os::unix::fs::symlink;
+    #[cfg(not(unix))]
+    use std::os::windows::fs::symlink_file as symlink;
+
+    let dir = tempfile::tempdir()?;
+
+    let file = helpers::new_readonly_tempfile_in_dir(&dir)?;
+    let symlink_path = dir.path().join("linked");
+    symlink(file.path(), &symlink_path)?;
+
+    let mut app = helpers::AppBuilder::new()
+        .with_file(&symlink_path, None)
+        .build()?;
+
+    test_key_sequence(
+        &mut app,
+        Some("ihello<esc>:wq<ret>"),
+        Some(&|app| {
+            let mut docs: Vec<_> = app.editor.documents().collect();
+            assert_eq!(1, docs.len());
+
+            let doc = docs.pop().unwrap();
+            assert_eq!(Some(&path::normalize(&symlink_path)), doc.path());
+            assert_eq!(&Severity::Error, app.editor.get_status().unwrap().1);
+        }),
+        false,
+    )
+    .await?;
+
+    assert!(symlink_path.is_symlink());
+
+    Ok(())
+}
+
 async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> {
     let mut file = tempfile::NamedTempFile::new()?;
 

helix-term/tests/test/helpers.rs

@@ -305,6 +305,18 @@ pub fn new_readonly_tempfile() -> anyhow::Result<NamedTempFile> {
     Ok(file)
 }
 
+/// Creates a new temporary file in the directory that is set to read only. Useful for
+/// testing write failures.
+pub fn new_readonly_tempfile_in_dir(
+    dir: impl AsRef<std::path::Path>,
+) -> anyhow::Result<NamedTempFile> {
+    let mut file = tempfile::NamedTempFile::new_in(dir)?;
+    let metadata = file.as_file().metadata()?;
+    let mut perms = metadata.permissions();
+    perms.set_readonly(true);
+    file.as_file_mut().set_permissions(perms)?;
+    Ok(file)
+}
 pub struct AppBuilder {
     args: Args,
     config: Config,

helix-view/src/document.rs

@@ -893,15 +893,18 @@ impl Future<Output = Result<DocumentSavedEvent, anyhow::Error>> + 'static + Send
                     }
                 }
             }
+            let write_path = tokio::fs::read_link(&path)
+                .await
+                .unwrap_or_else(|_| path.clone());
 
-            if readonly(&path) {
+            if readonly(&write_path) {
                 bail!(std::io::Error::new(
                     std::io::ErrorKind::PermissionDenied,
                     "Path is read only"
                 ));
             }
             let backup = if path.exists() {
-                let path_ = path.clone();
+                let path_ = write_path.clone();
                 // hacks: we use tempfile to handle the complex task of creating
                 // non clobbered temporary path for us we don't want
                 // the whole automatically delete path on drop thing
@@ -925,7 +928,7 @@ impl Future<Output = Result<DocumentSavedEvent, anyhow::Error>> + 'static + Send
             };
 
             let write_result: anyhow::Result<_> = async {
-                let mut dst = tokio::fs::File::create(&path).await?;
+                let mut dst = tokio::fs::File::create(&write_path).await?;
                 to_writer(&mut dst, encoding_with_bom_info, &text).await?;
                 Ok(())
             }
@@ -934,14 +937,13 @@ impl Future<Output = Result<DocumentSavedEvent, anyhow::Error>> + 'static + Send
             if let Some(backup) = backup {
                 if write_result.is_err() {
                     // restore backup
-                    let _ = tokio::fs::rename(&backup, &path)
+                    let _ = tokio::fs::rename(&backup, &write_path)
                         .await
                         .map_err(|e| log::error!("Failed to restore backup on write failure: {e}"));
                 } else {
                     // copy metadata and delete backup
-                    let path_ = path.clone();
                     let _ = tokio::task::spawn_blocking(move || {
-                        let _ = copy_metadata(&backup, &path_)
+                        let _ = copy_metadata(&backup, &write_path)
                             .map_err(|e| log::error!("Failed to copy metadata on write: {e}"));
                         let _ = std::fs::remove_file(backup)
                             .map_err(|e| log::error!("Failed to remove backup file on write: {e}"));