Use a temporary file for writes (#9236)

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

Cargo.lock

@@ -1405,12 +1405,15 @@ version = "24.3.0"
 name = "helix-stdx"
 version = "24.3.0"
 dependencies = [
+ "bitflags 2.5.0",
  "dunce",
  "etcetera",
  "regex-cursor",
  "ropey",
+ "rustix",
  "tempfile",
  "which",
+ "windows-sys 0.52.0",
 ]
 
 [[package]]
@@ -1515,6 +1518,7 @@ dependencies = [
  "serde",
  "serde_json",
  "slotmap",
+ "tempfile",
  "tokio",
  "tokio-stream",
  "toml",

helix-stdx/Cargo.toml

@@ -17,6 +17,13 @@ etcetera = "0.8"
 ropey = { version = "1.6.1", default-features = false }
 which = "6.0"
 regex-cursor = "0.1.4"
+bitflags = "2.4"
+
+[target.'cfg(windows)'.dependencies]
+windows-sys = { version = "0.52", features = ["Win32_Security", "Win32_Security_Authorization", "Win32_System_Threading"] }
+
+[target.'cfg(unix)'.dependencies]
+rustix = { version = "0.38", features = ["fs"] }
 
 [dev-dependencies]
 tempfile = "3.10"

helix-stdx/src/faccess.rs

@@ -0,0 +1,459 @@
+//! From <https://github.com/Freaky/faccess>
+
+use std::io;
+use std::path::Path;
+
+use bitflags::bitflags;
+
+// Licensed under MIT from faccess
+bitflags! {
+    /// Access mode flags for `access` function to test for.
+    pub struct AccessMode: u8 {
+        /// Path exists
+        const EXISTS  = 0b0001;
+        /// Path can likely be read
+        const READ    = 0b0010;
+        /// Path can likely be written to
+        const WRITE   = 0b0100;
+        /// Path can likely be executed
+        const EXECUTE = 0b1000;
+    }
+}
+
+#[cfg(unix)]
+mod imp {
+    use super::*;
+
+    use rustix::fs::Access;
+    use std::os::unix::fs::{MetadataExt, PermissionsExt};
+
+    pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> {
+        let mut imode = Access::empty();
+
+        if mode.contains(AccessMode::EXISTS) {
+            imode |= Access::EXISTS;
+        }
+
+        if mode.contains(AccessMode::READ) {
+            imode |= Access::READ_OK;
+        }
+
+        if mode.contains(AccessMode::WRITE) {
+            imode |= Access::WRITE_OK;
+        }
+
+        if mode.contains(AccessMode::EXECUTE) {
+            imode |= Access::EXEC_OK;
+        }
+
+        rustix::fs::access(p, imode)?;
+        Ok(())
+    }
+
+    fn chown(p: &Path, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {
+        let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) });
+        let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) });
+        rustix::fs::chown(p, uid, gid)?;
+        Ok(())
+    }
+
+    pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> {
+        let from_meta = std::fs::metadata(from)?;
+        let to_meta = std::fs::metadata(to)?;
+        let from_gid = from_meta.gid();
+        let to_gid = to_meta.gid();
+
+        let mut perms = from_meta.permissions();
+        perms.set_mode(perms.mode() & 0o0777);
+        if from_gid != to_gid && chown(to, None, Some(from_gid)).is_err() {
+            let new_perms = (perms.mode() & 0o0707) | ((perms.mode() & 0o07) << 3);
+            perms.set_mode(new_perms);
+        }
+
+        std::fs::set_permissions(to, perms)?;
+
+        Ok(())
+    }
+}
+
+// Licensed under MIT from faccess except for `chown`, `copy_metadata` and `is_acl_inherited`
+#[cfg(windows)]
+mod imp {
+
+    use windows_sys::Win32::Foundation::{CloseHandle, LocalFree, ERROR_SUCCESS, HANDLE, PSID};
+    use windows_sys::Win32::Security::Authorization::{
+        GetNamedSecurityInfoW, SetNamedSecurityInfoW, SE_FILE_OBJECT,
+    };
+    use windows_sys::Win32::Security::{
+        AccessCheck, AclSizeInformation, GetAce, GetAclInformation, GetSidIdentifierAuthority,
+        ImpersonateSelf, IsValidAcl, IsValidSid, MapGenericMask, RevertToSelf,
+        SecurityImpersonation, ACCESS_ALLOWED_CALLBACK_ACE, ACL, ACL_SIZE_INFORMATION,
+        DACL_SECURITY_INFORMATION, GENERIC_MAPPING, GROUP_SECURITY_INFORMATION, INHERITED_ACE,
+        LABEL_SECURITY_INFORMATION, OBJECT_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION,
+        PRIVILEGE_SET, PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR,
+        SID_IDENTIFIER_AUTHORITY, TOKEN_DUPLICATE, TOKEN_QUERY,
+    };
+    use windows_sys::Win32::Storage::FileSystem::{
+        FILE_ACCESS_RIGHTS, FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ,
+        FILE_GENERIC_WRITE,
+    };
+    use windows_sys::Win32::System::Threading::{GetCurrentThread, OpenThreadToken};
+
+    use super::*;
+
+    use std::ffi::c_void;
+
+    use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt};
+
+    struct SecurityDescriptor {
+        sd: PSECURITY_DESCRIPTOR,
+        owner: PSID,
+        group: PSID,
+        dacl: *mut ACL,
+    }
+
+    impl Drop for SecurityDescriptor {
+        fn drop(&mut self) {
+            if !self.sd.is_null() {
+                unsafe {
+                    LocalFree(self.sd);
+                }
+            }
+        }
+    }
+
+    impl SecurityDescriptor {
+        fn for_path(p: &Path) -> io::Result<SecurityDescriptor> {
+            let path = std::fs::canonicalize(p)?;
+            let pathos = path.into_os_string();
+            let mut pathw: Vec<u16> = Vec::with_capacity(pathos.len() + 1);
+            pathw.extend(pathos.encode_wide());
+            pathw.push(0);
+
+            let mut sd = std::ptr::null_mut();
+            let mut owner = std::ptr::null_mut();
+            let mut group = std::ptr::null_mut();
+            let mut dacl = std::ptr::null_mut();
+
+            let err = unsafe {
+                GetNamedSecurityInfoW(
+                    pathw.as_ptr(),
+                    SE_FILE_OBJECT,
+                    OWNER_SECURITY_INFORMATION
+                        | GROUP_SECURITY_INFORMATION
+                        | DACL_SECURITY_INFORMATION
+                        | LABEL_SECURITY_INFORMATION,
+                    &mut owner,
+                    &mut group,
+                    &mut dacl,
+                    std::ptr::null_mut(),
+                    &mut sd,
+                )
+            };
+
+            if err == ERROR_SUCCESS {
+                Ok(SecurityDescriptor {
+                    sd,
+                    owner,
+                    group,
+                    dacl,
+                })
+            } else {
+                Err(io::Error::last_os_error())
+            }
+        }
+
+        fn is_acl_inherited(&self) -> bool {
+            let mut acl_info: ACL_SIZE_INFORMATION = unsafe { ::core::mem::zeroed() };
+            let acl_info_ptr: *mut c_void = &mut acl_info as *mut _ as *mut c_void;
+            let mut ace: ACCESS_ALLOWED_CALLBACK_ACE = unsafe { ::core::mem::zeroed() };
+
+            unsafe {
+                GetAclInformation(
+                    self.dacl,
+                    acl_info_ptr,
+                    std::mem::size_of_val(&acl_info) as u32,
+                    AclSizeInformation,
+                )
+            };
+
+            for i in 0..acl_info.AceCount {
+                let mut ptr = &mut ace as *mut _ as *mut c_void;
+                unsafe { GetAce(self.dacl, i, &mut ptr) };
+                if (ace.Header.AceFlags as u32 & INHERITED_ACE) != 0 {
+                    return true;
+                }
+            }
+
+            false
+        }
+
+        fn descriptor(&self) -> &PSECURITY_DESCRIPTOR {
+            &self.sd
+        }
+
+        fn owner(&self) -> &PSID {
+            &self.owner
+        }
+    }
+
+    struct ThreadToken(HANDLE);
+    impl Drop for ThreadToken {
+        fn drop(&mut self) {
+            unsafe {
+                CloseHandle(self.0);
+            }
+        }
+    }
+
+    impl ThreadToken {
+        fn new() -> io::Result<Self> {
+            unsafe {
+                if ImpersonateSelf(SecurityImpersonation) == 0 {
+                    return Err(io::Error::last_os_error());
+                }
+
+                let token: *mut HANDLE = std::ptr::null_mut();
+                let err =
+                    OpenThreadToken(GetCurrentThread(), TOKEN_DUPLICATE | TOKEN_QUERY, 0, token);
+
+                RevertToSelf();
+
+                if err == 0 {
+                    return Err(io::Error::last_os_error());
+                }
+
+                Ok(Self(*token))
+            }
+        }
+
+        fn as_handle(&self) -> &HANDLE {
+            &self.0
+        }
+    }
+
+    // Based roughly on Tcl's NativeAccess()
+    // https://github.com/tcltk/tcl/blob/2ee77587e4dc2150deb06b48f69db948b4ab0584/win/tclWinFile.c
+    fn eaccess(p: &Path, mut mode: FILE_ACCESS_RIGHTS) -> io::Result<()> {
+        let md = p.metadata()?;
+
+        if !md.is_dir() {
+            // Read Only is ignored for directories
+            if mode & FILE_GENERIC_WRITE == FILE_GENERIC_WRITE && md.permissions().readonly() {
+                return Err(io::Error::new(
+                    io::ErrorKind::PermissionDenied,
+                    "File is read only",
+                ));
+            }
+
+            // If it doesn't have the correct extension it isn't executable
+            if mode & FILE_GENERIC_EXECUTE == FILE_GENERIC_EXECUTE {
+                if let Some(ext) = p.extension().and_then(|s| s.to_str()) {
+                    match ext {
+                        "exe" | "com" | "bat" | "cmd" => (),
+                        _ => {
+                            return Err(io::Error::new(
+                                io::ErrorKind::InvalidData,
+                                "File not executable",
+                            ))
+                        }
+                    }
+                }
+            }
+
+            return std::fs::OpenOptions::new()
+                .access_mode(mode)
+                .open(p)
+                .map(|_| ());
+        }
+
+        let sd = SecurityDescriptor::for_path(p)?;
+
+        // Unmapped Samba users are assigned a top level authority of 22
+        // ACL tests are likely to be misleading
+        const SAMBA_UNMAPPED: SID_IDENTIFIER_AUTHORITY = SID_IDENTIFIER_AUTHORITY {
+            Value: [0, 0, 0, 0, 0, 22],
+        };
+        unsafe {
+            let owner = sd.owner();
+            if IsValidSid(*owner) != 0
+                && (*GetSidIdentifierAuthority(*owner)).Value == SAMBA_UNMAPPED.Value
+            {
+                return Ok(());
+            }
+        }
+
+        let token = ThreadToken::new()?;
+
+        let mut privileges: PRIVILEGE_SET = unsafe { std::mem::zeroed() };
+        let mut granted_access: u32 = 0;
+        let mut privileges_length = std::mem::size_of::<PRIVILEGE_SET>() as u32;
+        let mut result = 0;
+
+        let mut mapping = GENERIC_MAPPING {
+            GenericRead: FILE_GENERIC_READ,
+            GenericWrite: FILE_GENERIC_WRITE,
+            GenericExecute: FILE_GENERIC_EXECUTE,
+            GenericAll: FILE_ALL_ACCESS,
+        };
+
+        unsafe { MapGenericMask(&mut mode, &mut mapping) };
+
+        if unsafe {
+            AccessCheck(
+                *sd.descriptor(),
+                *token.as_handle(),
+                mode,
+                &mut mapping,
+                &mut privileges,
+                &mut privileges_length,
+                &mut granted_access,
+                &mut result,
+            )
+        } != 0
+        {
+            if result == 0 {
+                Err(io::Error::new(
+                    io::ErrorKind::PermissionDenied,
+                    "Permission Denied",
+                ))
+            } else {
+                Ok(())
+            }
+        } else {
+            Err(io::Error::last_os_error())
+        }
+    }
+
+    pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> {
+        let mut imode = 0;
+
+        if mode.contains(AccessMode::READ) {
+            imode |= FILE_GENERIC_READ;
+        }
+
+        if mode.contains(AccessMode::WRITE) {
+            imode |= FILE_GENERIC_WRITE;
+        }
+
+        if mode.contains(AccessMode::EXECUTE) {
+            imode |= FILE_GENERIC_EXECUTE;
+        }
+
+        if imode == 0 {
+            if p.exists() {
+                Ok(())
+            } else {
+                Err(io::Error::new(io::ErrorKind::NotFound, "Not Found"))
+            }
+        } else {
+            eaccess(p, imode)
+        }
+    }
+
+    fn chown(p: &Path, sd: SecurityDescriptor) -> io::Result<()> {
+        let path = std::fs::canonicalize(p)?;
+        let pathos = path.as_os_str();
+        let mut pathw = Vec::with_capacity(pathos.len() + 1);
+        pathw.extend(pathos.encode_wide());
+        pathw.push(0);
+
+        let mut owner = std::ptr::null_mut();
+        let mut group = std::ptr::null_mut();
+        let mut dacl = std::ptr::null();
+
+        let mut si = OBJECT_SECURITY_INFORMATION::default();
+        if unsafe { IsValidSid(sd.owner) } != 0 {
+            si |= OWNER_SECURITY_INFORMATION;
+            owner = sd.owner;
+        }
+
+        if unsafe { IsValidSid(sd.group) } != 0 {
+            si |= GROUP_SECURITY_INFORMATION;
+            group = sd.group;
+        }
+
+        if unsafe { IsValidAcl(sd.dacl) } != 0 {
+            si |= DACL_SECURITY_INFORMATION;
+            if !sd.is_acl_inherited() {
+                si |= PROTECTED_DACL_SECURITY_INFORMATION;
+            }
+            dacl = sd.dacl as *const _;
+        }
+
+        let err = unsafe {
+            SetNamedSecurityInfoW(
+                pathw.as_ptr(),
+                SE_FILE_OBJECT,
+                si,
+                owner,
+                group,
+                dacl,
+                std::ptr::null(),
+            )
+        };
+
+        if err == ERROR_SUCCESS {
+            Ok(())
+        } else {
+            Err(io::Error::last_os_error())
+        }
+    }
+
+    pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> {
+        let sd = SecurityDescriptor::for_path(from)?;
+        chown(to, sd)?;
+
+        let meta = std::fs::metadata(from)?;
+        let perms = meta.permissions();
+
+        std::fs::set_permissions(to, perms)?;
+
+        Ok(())
+    }
+}
+
+// Licensed under MIT from faccess except for `copy_metadata`
+#[cfg(not(any(unix, windows)))]
+mod imp {
+    use super::*;
+
+    pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> {
+        if mode.contains(AccessMode::WRITE) {
+            if std::fs::metadata(p)?.permissions().readonly() {
+                return Err(io::Error::new(
+                    io::ErrorKind::PermissionDenied,
+                    "Path is read only",
+                ));
+            } else {
+                return Ok(());
+            }
+        }
+
+        if p.exists() {
+            Ok(())
+        } else {
+            Err(io::Error::new(io::ErrorKind::NotFound, "Path not found"))
+        }
+    }
+
+    pub fn copy_metadata(from: &path, to: &Path) -> io::Result<()> {
+        let meta = std::fs::metadata(from)?;
+        let perms = meta.permissions();
+        std::fs::set_permissions(to, perms)?;
+
+        Ok(())
+    }
+}
+
+pub fn readonly(p: &Path) -> bool {
+    match imp::access(p, AccessMode::WRITE) {
+        Ok(_) => false,
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => false,
+        Err(_) => true,
+    }
+}
+
+pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> {
+    imp::copy_metadata(from, to)
+}

helix-stdx/src/lib.rs

@@ -1,3 +1,4 @@
 pub mod env;
+pub mod faccess;
 pub mod path;
 pub mod rope;

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

@@ -95,7 +95,7 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> {
     .await?;
 
     helpers::assert_file_has_content(
-        file.as_file_mut(),
+        &mut file,
         &LineFeedHandling::Native.apply(&RANGE.end().to_string()),
     )?;
 
@@ -117,9 +117,7 @@ async fn test_write() -> anyhow::Result<()> {
     )
     .await?;
 
-    file.as_file_mut().flush()?;
+    reload_file(&mut file).unwrap();
-    file.as_file_mut().sync_all()?;
-
     let mut file_content = String::new();
     file.as_file_mut().read_to_string(&mut file_content)?;
 
@@ -148,12 +146,9 @@ async fn test_overwrite_protection() -> anyhow::Result<()> {
 
     test_key_sequence(&mut app, Some(":x<ret>"), None, false).await?;
 
-    file.as_file_mut().flush()?;
+    reload_file(&mut file).unwrap();
-    file.as_file_mut().sync_all()?;
-
-    file.rewind()?;
     let mut file_content = String::new();
-    file.as_file_mut().read_to_string(&mut file_content)?;
+    file.read_to_string(&mut file_content)?;
 
     assert_eq!("extremely important content", file_content);
 
@@ -175,11 +170,10 @@ async fn test_write_quit() -> anyhow::Result<()> {
     )
     .await?;
 
-    file.as_file_mut().flush()?;
+    reload_file(&mut file).unwrap();
-    file.as_file_mut().sync_all()?;
 
     let mut file_content = String::new();
-    file.as_file_mut().read_to_string(&mut file_content)?;
+    file.read_to_string(&mut file_content)?;
 
     assert_eq!(
         LineFeedHandling::Native.apply("the gostak distims the doshes"),
@@ -205,11 +199,9 @@ async fn test_write_concurrent() -> anyhow::Result<()> {
 
     test_key_sequence(&mut app, Some(&command), None, false).await?;
 
-    file.as_file_mut().flush()?;
+    reload_file(&mut file).unwrap();
-    file.as_file_mut().sync_all()?;
-
     let mut file_content = String::new();
-    file.as_file_mut().read_to_string(&mut file_content)?;
+    file.read_to_string(&mut file_content)?;
     assert_eq!(
         LineFeedHandling::Native.apply(&RANGE.end().to_string()),
         file_content
@@ -279,7 +271,7 @@ async fn test_write_scratch_to_new_path() -> anyhow::Result<()> {
     )
     .await?;
 
-    helpers::assert_file_has_content(file.as_file_mut(), &LineFeedHandling::Native.apply("hello"))?;
+    helpers::assert_file_has_content(&mut file, &LineFeedHandling::Native.apply("hello"))?;
 
     Ok(())
 }
@@ -324,7 +316,7 @@ async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> {
     test_key_sequences(&mut app, vec![(Some(":w<ret>"), None)], false).await?;
 
     // file still saves
-    helpers::assert_file_has_content(file.as_file_mut(), "let foo = 0;\n")?;
+    helpers::assert_file_has_content(&mut file, "let foo = 0;\n")?;
 
     Ok(())
 }
@@ -363,12 +355,12 @@ async fn test_write_new_path() -> anyhow::Result<()> {
     .await?;
 
     helpers::assert_file_has_content(
-        file1.as_file_mut(),
+        &mut file1,
         &LineFeedHandling::Native.apply("i can eat glass, it will not hurt me\n"),
     )?;
 
     helpers::assert_file_has_content(
-        file2.as_file_mut(),
+        &mut file2,
         &LineFeedHandling::Native.apply("i can eat glass, it will not hurt me\n"),
     )?;
 
@@ -439,7 +431,7 @@ async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<()
     test_key_sequence(&mut app, Some(":w<ret>"), None, false).await?;
 
     helpers::assert_file_has_content(
-        file.as_file_mut(),
+        &mut file,
         &LineFeedHandling::Native.apply("have you tried chamomile tea?\n"),
     )?;
 
@@ -457,7 +449,7 @@ async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::R
     test_key_sequence(&mut app, Some(":w<ret>"), None, false).await?;
 
     helpers::assert_file_has_content(
-        file.as_file_mut(),
+        &mut file,
         &LineFeedHandling::Native.apply("ten minutes, please\n"),
     )?;
 
@@ -481,10 +473,8 @@ async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> any
 
     test_key_sequence(&mut app, Some(":w<ret>"), None, false).await?;
 
-    helpers::assert_file_has_content(
+    reload_file(&mut file).unwrap();
-        file.as_file_mut(),
+    helpers::assert_file_has_content(&mut file, "the quiet rain continued through the night")?;
-        "the quiet rain continued through the night",
-    )?;
 
     Ok(())
 }
@@ -510,12 +500,12 @@ async fn test_write_all_insert_final_newline_add_if_missing_and_modified() -> an
     .await?;
 
     helpers::assert_file_has_content(
-        file1.as_file_mut(),
+        &mut file1,
         &LineFeedHandling::Native.apply("we don't serve time travelers here\n"),
     )?;
 
     helpers::assert_file_has_content(
-        file2.as_file_mut(),
+        &mut file2,
         &LineFeedHandling::Native.apply("a time traveler walks into a bar\n"),
     )?;
 
@@ -534,7 +524,7 @@ async fn test_write_all_insert_final_newline_do_not_add_if_unmodified() -> anyho
 
     test_key_sequence(&mut app, Some(":wa<ret>"), None, false).await?;
 
-    helpers::assert_file_has_content(file.as_file_mut(), "i lost on Jeopardy!")?;
+    helpers::assert_file_has_content(&mut file, "i lost on Jeopardy!")?;
 
     Ok(())
 }
@@ -560,7 +550,7 @@ async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> {
     )
     .await?;
 
-    file.rewind()?;
+    reload_file(&mut file).unwrap();
     let mut new_file_content: Vec<u8> = Vec::new();
     file.read_to_end(&mut new_file_content)?;
 

helix-term/tests/test/helpers.rs

@@ -1,5 +1,4 @@
 use std::{
-    fs::File,
     io::{Read, Write},
     mem::replace,
     path::PathBuf,
@@ -390,9 +389,8 @@ pub async fn run_event_loop_until_idle(app: &mut Application) {
     app.event_loop_until_idle(&mut rx_stream).await;
 }
 
-pub fn assert_file_has_content(file: &mut File, content: &str) -> anyhow::Result<()> {
+pub fn assert_file_has_content(file: &mut NamedTempFile, content: &str) -> anyhow::Result<()> {
-    file.flush()?;
+    reload_file(file)?;
-    file.sync_all()?;
 
     let mut file_content = String::new();
     file.read_to_string(&mut file_content)?;
@@ -406,3 +404,13 @@ pub fn assert_status_not_error(editor: &Editor) {
         assert_ne!(&Severity::Error, sev);
     }
 }
+
+pub fn reload_file(file: &mut NamedTempFile) -> anyhow::Result<()> {
+    let path = file.path();
+    let f = std::fs::OpenOptions::new()
+        .write(true)
+        .read(true)
+        .open(&path)?;
+    *file.as_file_mut() = f;
+    Ok(())
+}

helix-term/tests/test/splits.rs

@@ -62,18 +62,9 @@ async fn test_split_write_quit_all() -> anyhow::Result<()> {
     )
     .await?;
 
-    helpers::assert_file_has_content(
+    helpers::assert_file_has_content(&mut file1, &LineFeedHandling::Native.apply("hello1"))?;
-        file1.as_file_mut(),
+    helpers::assert_file_has_content(&mut file2, &LineFeedHandling::Native.apply("hello2"))?;
-        &LineFeedHandling::Native.apply("hello1"),
+    helpers::assert_file_has_content(&mut file3, &LineFeedHandling::Native.apply("hello3"))?;
-    )?;
-    helpers::assert_file_has_content(
-        file2.as_file_mut(),
-        &LineFeedHandling::Native.apply("hello2"),
-    )?;
-    helpers::assert_file_has_content(
-        file3.as_file_mut(),
-        &LineFeedHandling::Native.apply("hello3"),
-    )?;
 
     Ok(())
 }
@@ -131,10 +122,7 @@ async fn test_split_write_quit_same_file() -> anyhow::Result<()> {
     )
     .await?;
 
-    helpers::assert_file_has_content(
+    helpers::assert_file_has_content(&mut file, &LineFeedHandling::Native.apply("hello\ngoodbye"))?;
-        file.as_file_mut(),
-        &LineFeedHandling::Native.apply("hello\ngoodbye"),
-    )?;
 
     Ok(())
 }

helix-view/Cargo.toml

@@ -27,6 +27,8 @@ bitflags = "2.5"
 anyhow = "1"
 crossterm = { version = "0.27", optional = true }
 
+tempfile = "3.9"
+
 # Conversion traits
 once_cell = "1.19"
 url = "2.5.0"

helix-view/src/document.rs

@@ -10,6 +10,7 @@ use helix_core::encoding::Encoding;
 use helix_core::syntax::{Highlight, LanguageServerFeature};
 use helix_core::text_annotations::{InlineAnnotation, Overlay};
 use helix_lsp::util::lsp_pos_to_pos;
+use helix_stdx::faccess::{copy_metadata, readonly};
 use helix_vcs::{DiffHandle, DiffProviderRegistry};
 
 use ::parking_lot::Mutex;
@@ -870,7 +871,7 @@ impl Document {
 
         // We encode the file according to the `Document`'s encoding.
         let future = async move {
-            use tokio::{fs, fs::File};
+            use tokio::fs;
             if let Some(parent) = path.parent() {
                 // TODO: display a prompt asking the user if the directories should be created
                 if !parent.exists() {
@@ -893,8 +894,63 @@ impl Document {
                 }
             }
 
-            let mut file = File::create(&path).await?;
+            if readonly(&path) {
-            to_writer(&mut file, encoding_with_bom_info, &text).await?;
+                bail!(std::io::Error::new(
+                    std::io::ErrorKind::PermissionDenied,
+                    "Path is read only"
+                ));
+            }
+            let backup = if path.exists() {
+                let path_ = 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
+                // since the path doesn't exist yet, we just want
+                // the path
+                tokio::task::spawn_blocking(move || -> Option<PathBuf> {
+                    tempfile::Builder::new()
+                        .prefix(path_.file_name()?)
+                        .suffix(".bck")
+                        .make_in(path_.parent()?, |backup| std::fs::rename(&path_, backup))
+                        .ok()?
+                        .into_temp_path()
+                        .keep()
+                        .ok()
+                })
+                .await
+                .ok()
+                .flatten()
+            } else {
+                None
+            };
+
+            let write_result: anyhow::Result<_> = async {
+                let mut dst = tokio::fs::File::create(&path).await?;
+                to_writer(&mut dst, encoding_with_bom_info, &text).await?;
+                Ok(())
+            }
+            .await;
+
+            if let Some(backup) = backup {
+                if write_result.is_err() {
+                    // restore backup
+                    let _ = tokio::fs::rename(&backup, &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_)
+                            .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}"));
+                    })
+                    .await;
+                }
+            }
+
+            write_result?;
 
             let event = DocumentSavedEvent {
                 revision: current_rev,
@@ -955,35 +1011,12 @@ impl Document {
         }
     }
 
-    #[cfg(unix)]
     // Detect if the file is readonly and change the readonly field if necessary (unix only)
     pub fn detect_readonly(&mut self) {
-        use rustix::fs::{access, Access};
         // Allows setting the flag for files the user cannot modify, like root files
         self.readonly = match &self.path {
             None => false,
-            Some(p) => match access(p, Access::WRITE_OK) {
+            Some(p) => readonly(p),
-                Ok(_) => false,
-                Err(err) if err.kind() == std::io::ErrorKind::NotFound => false,
-                Err(_) => true,
-            },
-        };
-    }
-
-    #[cfg(not(unix))]
-    // Detect if the file is readonly and change the readonly field if necessary (non-unix os)
-    pub fn detect_readonly(&mut self) {
-        // TODO Use the Windows' function `CreateFileW` to check if a file is readonly
-        // Discussion: https://github.com/helix-editor/helix/pull/7740#issuecomment-1656806459
-        // Vim implementation: https://github.com/vim/vim/blob/4c0089d696b8d1d5dc40568f25ea5738fa5bbffb/src/os_win32.c#L7665
-        // Windows binding: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Storage/FileSystem/fn.CreateFileW.html
-        self.readonly = match &self.path {
-            None => false,
-            Some(p) => match std::fs::metadata(p) {
-                Err(err) if err.kind() == std::io::ErrorKind::NotFound => false,
-                Err(_) => false,
-                Ok(metadata) => metadata.permissions().readonly(),
-            },
         };
     }