fix tests

Cargo.lock

@@ -1441,6 +1441,7 @@ dependencies = [
  "chardetng",
  "clipboard-win",
  "crossterm",
+ "filetime",
  "futures-util",
  "helix-core",
  "helix-dap",

helix-stdx/src/faccess.rs

@@ -1,6 +1,5 @@
 //! From <https://github.com/Freaky/faccess>
 
-use std::fs::File;
 use std::io;
 use std::path::Path;
 
@@ -25,10 +24,7 @@ bitflags! {
 mod imp {
     use super::*;
 
-    use rustix::{
+    use rustix::fs::Access;
-        fd::AsFd,
-        fs::{Access, OpenOptionsExt},
-    };
     use std::os::unix::fs::{MetadataExt, PermissionsExt};
 
     pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> {
@@ -76,24 +72,6 @@ mod imp {
         let metadata = p.metadata()?;
         Ok(metadata.nlink())
     }
-
-    pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result<File> {
-        let from_meta = std::fs::metadata(from)?;
-        let mode = from_meta.permissions().mode();
-        let file = std::fs::OpenOptions::new()
-            .mode(mode)
-            .read(true)
-            .write(true)
-            .create_new(true)
-            .open(to)?;
-
-        // Change ownership
-        let from_meta = std::fs::metadata(from)?;
-        let uid = from_meta.uid();
-        let gid = from_meta.gid();
-        fchown(file.as_fd(), Some(uid), Some(gid))?;
-        Ok(file)
-    }
 }
 
 #[cfg(windows)]
@@ -125,6 +103,7 @@ mod imp {
 
     use std::ffi::c_void;
 
+    use std::fs::File;
     use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt, io::AsRawHandle};
 
     // Licensed under MIT from faccess
@@ -460,26 +439,6 @@ mod imp {
         let n = file_info(p)?.nNumberOfLinks as u64;
         Ok(n)
     }
-
-    pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result<File> {
-        let sd = SecurityDescriptor::for_path(from)?;
-
-        // read/write still need to be set to true or `create_new` returns an error
-        let to_file = std::fs::OpenOptions::new()
-            .read(true)
-            .write(true)
-            .access_mode(GENERIC_READ | GENERIC_WRITE | WRITE_OWNER | WRITE_DAC)
-            .create_new(true)
-            .open(to)?;
-
-        // Necessary because `security_attributes` is not exposed: https://github.com/rust-lang/libs-team/issues/314
-        chown(to_file.as_raw_handle(), sd)?;
-
-        let meta = std::fs::metadata(from)?;
-        let perms = meta.permissions();
-        std::fs::set_permissions(to, perms)?;
-        Ok(to_file)
-    }
 }
 
 // Licensed under MIT from faccess except for `copy_metadata`
@@ -523,11 +482,6 @@ pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> {
     imp::copy_metadata(from, to)
 }
 
-// /// Create a file copying permissions, uid, and gid of `from` at the the target destination `to`
-// pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result<File> {
-//     imp::create_copy_mode(from, to)
-// }
-
 #[cfg(windows)]
 pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> {
     imp::copy_ownership(from, to)
@@ -556,30 +510,33 @@ pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> {
         len => len,
     };
 
-    let mut buf = vec![0i8; size];
+    let mut buf = vec![0; size];
-    let read = rustix::fs::listxattr(from, &mut buf)?;
+    let read = rustix::fs::listxattr(from, buf.as_mut_slice())?;
-
-    fn i8_to_u8_slice(input: &[i8]) -> &[u8] {
-        // SAFETY: Simply reinterprets bytes
-        unsafe { std::slice::from_raw_parts(input.as_ptr() as *const u8, input.len()) }
-    }
 
     // Iterate over null-terminated C-style strings
     // Two loops to avoid multiple allocations
     // Find max-size for attributes
     let mut max_attr_len = 0;
-    for attr_byte in buf.split(|&b| b == 0) {
+    for attr_byte in buf[..read].split(|&b| b == 0) {
-        let name = std::str::from_utf8(i8_to_u8_slice(attr_byte))
+        // handle platforms where c_char is i8
+        #[allow(clippy::unnecessary_cast)]
+        let conv =
+            unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) };
+        let name = std::str::from_utf8(conv)
             .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?;
         let attr_len = rustix::fs::getxattr(from, name, &mut [])?;
         max_attr_len = max_attr_len.max(attr_len);
     }
 
     let mut attr_buf = vec![0u8; max_attr_len];
-    for attr_byte in buf.split(|&b| b == 0) {
+    for attr_byte in buf[..read].split(|&b| b == 0) {
-        let name = std::str::from_utf8(i8_to_u8_slice(attr_byte))
+        // handle platforms where c_char is i8
+        #[allow(clippy::unnecessary_cast)]
+        let conv =
+            unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) };
+        let name = std::str::from_utf8(conv)
             .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?;
-        let read = rustix::fs::getxattr(from, name, &mut attr_buf)?;
+        let read = rustix::fs::getxattr(from, name, attr_buf.as_mut_slice())?;
 
         // If we can't set xattr because it already exists, try to replace it
         if read != 0 {
@@ -599,69 +556,3 @@ pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> {
 
     Ok(())
 }
-
-/*
-Neovim backup path function:
-- If a backup is desired (would be disabled by a user if using a file watcher):
-    - Checks if user explicitly requested a copy
-    - Or automatically choose whether to copy or rename
-    - Offers options for:
-        - Breaking symlinks or hardlinks (not offered in Helix)
-    - Offers the ability to have a list of directories where the backup file is written:
-        - Default is: ".,$XDG_STATE_HOME/nvim/backup//"
-    - Offers ability to set backup extension
-- For copy backup:
-    - If the file is a link, then the backup will have the name of the link
-- Auto backup:
-    - Immediately set copy if:
-        - Is hardlink or symlink
-    - Then, tries to:
-        - Create a temporary file with the same permissions as the file to test if its ok to rename later
-            - If it fails, then set copy
-        - fchown created file
-            - If it fails or perms weren't copied, then set copy
-        - Delete test file
-    - Otherwise, will rename
-- Break symlink/hardlink if requested
-- Copy backup:
-    - If there is an error while creating the file, it will be propogated unless force write is true
-    - Try to create backup path in bdir:
-        - Tries first directory where this is possible
-        - If no directory exists, the last directory is created
-        - Filename is escaped and extension applied
-    - Check if backup already exists:
-        - Check if pre-existing file is a symlink to the original file (and don't attempt to create one)
-        - Dunno what p_bk is, but if false, it tries to create a different backup file path where each character before the extension is changed (if all attempts fail, then error)
-    - Copies file with UV_FS_COPYFILE_FICLONE
-    - Sets perm as os_setperm(*backupp, perm & 0777);
-    - On Unix:
-        - Attempts to set gid via chown:
-            - os_setperm(*backupp, (perm & 0707) | ((perm & 07) << 3) if fails
-        - Sets file time:
-            os_file_settime(*backupp,
-                        (double)file_info_old->stat.st_atim.tv_sec,
-                        (double)file_info_old->stat.st_mtim.tv_sec);
-    - On Windows, sets ACL
-    - Attempts to copy xattr if exists
-- Rename backup:
-    - Backup is created by renaming original file:
-        - Don't if file is read-only and cpoptions has "W" flag
-    - Tries to find backup file name w/ bdir (similar to copy)
-    - Checks if a file with that name already exists:
-        - Attempts same method as copy backup to create a different filename
-
-Neovim write:
-- On Unix:
-    - If using :w! and file was read-only, make it writable (if process uid is same as file):
-- Reset read-only flag if overwriting
-- Executes fsync (will not propogate error if storage does not support op)
-- Copies xattr for non-copy backups
-- If a rename backup is being performed:
-    - Check if uid and gid are same as original file, and set if they aren't
-    - Set perms
-- Either way, copy perms from old file to new file
-- Either way, if not a backup copy, also set ACL (method seems to not do anything?)
-- On failure:
-    - If a copy, copy contents from copy to original file
-    - Otherwise, rename backup back to original path
-*/

helix-stdx/src/path.rs

@@ -224,7 +224,7 @@ fn path_from_bytes(slice: &[u8]) -> Result<PathBuf, Utf8Error> {
 
 fn is_sep_byte(b: u8) -> bool {
     if cfg!(windows) {
-        b == b'/' || b == b'\\'
+        b == b'/' || b == b'\\' || b == b':'
     } else {
         b == b'/'
     }
@@ -233,7 +233,7 @@ fn is_sep_byte(b: u8) -> bool {
 /// Replaces all path separators in a path with %
 pub fn escape_path(path: &Path) -> PathBuf {
     let s = path.as_os_str().to_os_string();
-    let mut bytes = os_str_as_bytes(&s);
+    let mut bytes = os_str_as_bytes(s);
     for b in bytes.iter_mut() {
         if is_sep_byte(*b) {
             *b = b'%';

helix-term/src/application.rs

@@ -234,6 +234,14 @@ impl Application {
                 .unwrap_or_else(|_| editor.new_file(Action::VerticalSplit));
         }
 
+        let bck_config = &config.load().editor.backup;
+        if bck_config.kind != helix_view::editor::BackupKind::None
+            && !bck_config.directories.iter().any(|p| p.exists())
+        {
+            // Initialize last directory for backups
+            std::fs::create_dir_all(bck_config.directories.last().unwrap())?;
+        }
+
         editor.set_theme(theme);
 
         #[cfg(windows)]

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

@@ -539,6 +539,7 @@ async fn test_symlink_write() -> anyhow::Result<()> {
     let dir = tempfile::tempdir()?;
 
     let mut file = tempfile::NamedTempFile::new_in(&dir)?;
+    // NOTE: This will fail on Windows unless ran in administrator
     let symlink_path = dir.path().join("linked");
     symlink(file.path(), &symlink_path)?;
 
@@ -578,6 +579,7 @@ async fn test_symlink_write_fail() -> anyhow::Result<()> {
 
     let file = helpers::new_readonly_tempfile_in_dir(&dir)?;
     let symlink_path = dir.path().join("linked");
+    // NOTE: This will fail on Windows unless ran in administrator
     symlink(file.path(), &symlink_path)?;
 
     let mut app = helpers::AppBuilder::new()
@@ -622,6 +624,7 @@ async fn test_symlink_write_relative() -> anyhow::Result<()> {
     let mut file = tempfile::NamedTempFile::new_in(&inner_dir)?;
     let symlink_path = dir.path().join("linked");
     let relative_path = std::path::PathBuf::from("b").join(file.path().file_name().unwrap());
+    // NOTE: This will fail on Windows unless ran in administrator
     symlink(relative_path, &symlink_path)?;
 
     let mut app = helpers::AppBuilder::new()

helix-view/Cargo.toml

@@ -30,6 +30,7 @@ crossterm = { version = "0.28", optional = true }
 
 tempfile = "3.14"
 same-file = "1.0.1"
+filetime = "0.2"
 
 # Conversion traits
 once_cell = "1.20"

helix-view/src/document.rs

@@ -1,6 +1,7 @@
 use anyhow::{anyhow, bail, Error};
-use arc_swap::access::{Access, DynAccess};
+use arc_swap::access::DynAccess;
 use arc_swap::ArcSwap;
+use filetime::FileTime;
 use futures_util::future::BoxFuture;
 use futures_util::FutureExt;
 use helix_core::auto_pairs::AutoPairs;
@@ -23,7 +24,6 @@ use std::collections::HashMap;
 use std::fmt::Display;
 use std::future::Future;
 use std::io;
-use std::os::unix::fs::MetadataExt;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::sync::{Arc, Weak};
@@ -153,7 +153,6 @@ impl Backup {
         // - it is a hardlink
         // - it is a symlink
         // - we don't have file create perms for the dir
-        // TODO: also set copy when perms can't be set or metadata can't be read
         if !copy {
             // Conservatively assume it is a hardlink if we can't read metadata
             let is_hardlink = {
@@ -165,16 +164,16 @@ impl Backup {
                 copy = true;
             } else {
                 // Check if we have write permissions by creating a temporary file
+                let from_meta = tokio::fs::metadata(&p).await?;
+                let perms = from_meta.permissions();
                 let mut builder = tempfile::Builder::new();
-                // TODO: Need a way to create cross-platform perms with more granularity
+                builder.permissions(perms);
-                // builder.permissions()
                 if let Ok(f) = builder.tempfile() {
                     // Check if we have perms to set perms
                     #[cfg(unix)]
                     {
                         use std::os::{fd::AsFd, unix::fs::MetadataExt};
 
-                        let from_meta = tokio::fs::metadata(&p).await?;
                         let to_meta = tokio::fs::metadata(&f.path()).await?;
                         let _ = fchown(
                             f.as_file().as_fd(),
@@ -205,12 +204,14 @@ impl Backup {
         // - directory is not writable
         // - path is a directory
         // - path exists
-        let mut dir_exists = false;
         let escaped_p = helix_stdx::path::escape_path(&p);
+        // `.join` on absolute path replaces instead of append
+        debug_assert!(escaped_p.is_relative());
+
         'outer: for dir in config.directories.iter().filter(|p| p.is_dir()) {
             let ext = config.extension.as_str();
             let bck_base_path = &dir.join(&escaped_p);
-            let mut backup = bck_base_path.join(&ext);
+            let mut backup = bck_base_path.with_extension(&ext);
 
             // NOTE: Should we just overwrite regardless?
             // If the backup file already exists, we'll try to add a number before the extension
@@ -218,10 +219,11 @@ impl Backup {
             // NOTE: u8 since if we need more than 256, there might be an issue
             let mut n: u8 = 1;
             while backup.exists() {
-                backup = bck_base_path.join(n.to_string()).join(&ext);
+                backup = bck_base_path.with_extension(format!("{n}.{ext}"));
-                let Some(n) = n.checked_add(1) else {
+                if n == u8::MAX {
                     continue 'outer;
-                };
+                }
+                n = n + 1;
             }
 
             if copy {
@@ -233,7 +235,7 @@ impl Backup {
                 {
                     use std::os::unix::fs::{MetadataExt, PermissionsExt};
 
-                    let mut from_meta = tokio::fs::metadata(&p).await?;
+                    let from_meta = tokio::fs::metadata(&p).await?;
                     let mut perms = from_meta.permissions();
 
                     // Strip s-bit
@@ -249,7 +251,11 @@ impl Backup {
                         perms.set_mode(new_perms);
                     }
                     std::fs::set_permissions(&backup, perms)?;
-                    // TODO: Set time
+
+                    let atime = FileTime::from_last_access_time(&from_meta);
+                    let mtime = FileTime::from_last_modification_time(&from_meta);
+                    filetime::set_file_times(&backup, atime, mtime)?;
+
                     copy_xattr(&p, &backup)?;
                 }
 
@@ -260,25 +266,15 @@ impl Backup {
                         copy_ownership(&p, &backup_)?;
                         Ok(())
                     })
-                    .await?;
+                    .await??;
                 }
-
-                return Ok(Self {
-                    copy: true,
-                    path: backup,
-                });
             } else {
                 tokio::fs::rename(p, &backup).await?;
-                return Ok(Self {
-                    copy: false,
-                    path: backup,
-                });
             }
+            return Ok(Self { copy, path: backup });
         }
-        // TODO: Try to initialize last dir if none of the dirs exist
 
-        // TODO
+        bail!("Could not write into a backup directory");
-        bail!("err");
     }
 }
 
@@ -1070,19 +1066,8 @@ impl Document {
                     }
                 }
             }
-            let write_path = tokio::fs::read_link(&path)
-                .await
-                .ok()
-                .and_then(|p| {
-                    if p.is_relative() {
-                        path.parent().map(|parent| parent.join(p))
-                    } else {
-                        Some(p)
-                    }
-                })
-                .unwrap_or_else(|| path.clone());
 
-            if readonly(&write_path) {
+            if readonly(&path) {
                 bail!(std::io::Error::new(
                     std::io::ErrorKind::PermissionDenied,
                     "Path is read only"
@@ -1090,61 +1075,72 @@ impl Document {
             }
 
             // Use a backup file
+            let meta = tokio::fs::metadata(&path).await?;
             let mut bck = None;
-            let write_result = if bck_config.kind != crate::editor::BackupKind::None {
+            let write_result: Result<(), Error> = async {
-                bck = Some(Backup::from(write_path.clone(), &bck_config).await?);
+                if path.exists() && bck_config.kind != crate::editor::BackupKind::None {
-                let bck = bck.unwrap();
+                    match Backup::from(path.clone(), &bck_config).await {
-
+                        Ok(b) => bck = Some(b),
-                // SECURITY: Ensure that the created file has the same perms as the original file
+                        Err(_) if force => {}
-                let dst = if !bck.copy {
+                        Err(e) => bail!("Could not create backup: {e}"),
-                    let from_meta = tokio::fs::metadata(&write_path).await?;
-                    let mut open_opt = tokio::fs::OpenOptions::new()
-                        .read(true)
-                        .write(true)
-                        .create_new(true);
-
-                    #[cfg(unix)]
-                    {
-                        use std::os::unix::fs::PermissionsExt;
-                        let mode = from_meta.permissions().mode();
-                        open_opt.mode(mode);
                     }
+                }
 
-                    let file = open_opt.open(&bck.path).await?;
+                if let Some(ref bck) = bck {
-                    let to_meta = file.metadata().await?;
+                    // SECURITY: Ensure that the created file has the same perms as the original file
+                    let mut dst = if !bck.copy {
+                        let mut open_opt = tokio::fs::OpenOptions::new();
+                        open_opt.read(true).write(true).create_new(true);
 
-                    #[cfg(unix)]
+                        #[cfg(unix)]
-                    {
+                        {
-                        // TODO: set gid/uid via fchown
+                            use std::os::unix::fs::PermissionsExt;
-                    }
+                            let mode = meta.permissions().mode();
+                            open_opt.mode(mode);
+                        }
 
-                    #[cfg(windows)]
+                        let file = open_opt.open(&path).await?;
-                    {
-                        let from = write_path.clone();
-                        let to = bck.path.clone();
-                        tokio::task::spawn_blocking(move || {
-                            helix_stdx::faccess::copy_ownership(&from, &to)?;
-                            Ok(())
-                        })
-                        .await?;
-                    }
-                    file
-                } else {
-                    // SECURITY: Backup copy already exists
-                    tokio::fs::File::create(&write_path).await?
-                };
 
-                to_writer(&mut dst, encoding_with_bom_info, &text).await?;
+                        #[cfg(unix)]
-                dst.sync_all().await?;
+                        {
-                Ok(())
+                            use std::os::fd::AsFd;
-            } else {
+                            use std::os::unix::fs::MetadataExt;
-                let dst = tokio::fs::File::create(&write_path).await?;
+                            helix_stdx::faccess::fchown(
-                to_writer(&mut dst, encoding_with_bom_info, &text).await?;
+                                file.as_fd(),
-                dst.sync_all().await?;
+                                Some(meta.uid()),
-                Ok(())
+                                Some(meta.gid()),
-            };
+                            )?;
+                        }
+
+                        #[cfg(windows)]
+                        {
+                            let from = path.clone();
+                            let to = bck.path.clone();
+                            tokio::task::spawn_blocking(move || -> Result<(), Error> {
+                                helix_stdx::faccess::copy_ownership(&from, &to)?;
+                                Ok(())
+                            })
+                            .await??;
+                        }
+                        file
+                    } else {
+                        // SECURITY: Backup copy already exists
+                        tokio::fs::File::create(&path).await?
+                    };
 
-            let save_time = match fs::metadata(&write_path).await {
+                    to_writer(&mut dst, encoding_with_bom_info, &text).await?;
+                    dst.sync_all().await?;
+                    Ok(())
+                } else {
+                    let mut dst = tokio::fs::File::create(&path).await?;
+                    to_writer(&mut dst, encoding_with_bom_info, &text).await?;
+                    dst.sync_all().await?;
+                    Ok(())
+                }
+            }
+            .await;
+
+            let save_time = match fs::metadata(&path).await {
                 Ok(metadata) => metadata.modified().map_or(SystemTime::now(), |mtime| mtime),
                 Err(_) => SystemTime::now(),
             };
@@ -1158,35 +1154,41 @@ impl Document {
                 let mut delete_bck = true;
                 if write_result.is_err() {
                     // If original file no longer exists, then backup is renamed to original file
-                    if !write_path.exists() {
+                    if !path.exists() {
-                        if !tokio::fs::rename(&bck.path, &write_path)
+                        delete_bck = false;
+                        if tokio::fs::rename(&bck.path, &path)
                             .await
                             .map_err(|e| {
-                                delete_bck = false;
                                 log::error!("Failed to restore backup on write failure: {e}")
                             })
-                            .is_err()
+                            .is_ok()
                         {
-                            // TODO: Set timestamps to prior to write
+                            // Reset timestamps
+                            let atime = FileTime::from_last_access_time(&meta);
+                            let mtime = FileTime::from_last_modification_time(&meta);
+                            filetime::set_file_times(&path, atime, mtime)?;
                         }
                     } else {
                         if bck.copy {
                             // Restore backup from copy
-                            let _ = tokio::fs::copy(&bck.path, &write_path).await.map_err(|e| {
+                            let _ = tokio::fs::copy(&bck.path, &path).await.map_err(|e| {
                                 delete_bck = false;
                                 log::error!("Failed to restore backup on write failure: {e}")
                             });
                         } else {
                             // restore backup
-                            let _ = tokio::fs::rename(&bck.path, &write_path)
+                            let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|e| {
-                                .await
+                                delete_bck = false;
-                                .map_err(|e| {
+                                log::error!("Failed to restore backup on write failure: {e}")
-                                    delete_bck = false;
+                            });
-                                    log::error!("Failed to restore backup on write failure: {e}")
-                                });
                         }
                     }
                 }
+
+                // Delete backup if we're done with it
+                if delete_bck {
+                    tokio::fs::remove_file(bck.path).await?;
+                }
             }
 
             write_result?;

helix-view/src/editor.rs

@@ -352,7 +352,9 @@ pub struct Config {
 #[serde(rename_all = "kebab-case", default)]
 pub struct BackupConfig {
     pub kind: BackupKind,
+    #[serde(deserialize_with = "deserialize_non_empty_vec")]
     pub directories: Vec<PathBuf>,
+    #[serde(deserialize_with = "deserialize_non_empty_str")]
     pub extension: String,
 }
 
@@ -360,14 +362,39 @@ impl Default for BackupConfig {
     fn default() -> Self {
         Self {
             kind: BackupKind::Auto,
-            // TODO: Prevent empty vector
             directories: vec![helix_loader::state_dir().join("backup")],
-            // TODO: Prevent empty strings
             extension: String::from("bck"),
         }
     }
 }
 
+fn deserialize_non_empty_vec<'de, D, T>(deserializer: D) -> Result<Vec<T>, D::Error>
+where
+    D: Deserializer<'de>,
+    T: Deserialize<'de>,
+{
+    use serde::de::Error;
+
+    let vec = Vec::<T>::deserialize(deserializer)?;
+    if vec.is_empty() {
+        return Err(<D::Error as Error>::custom("vector cannot be empty!"));
+    }
+    Ok(vec)
+}
+
+pub fn deserialize_non_empty_str<'de, D>(deserializer: D) -> Result<String, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    use serde::de::Error;
+
+    let s = String::deserialize(deserializer)?;
+    if s.is_empty() {
+        return Err(<D::Error as Error>::custom("string cannot be empty"));
+    }
+    Ok(s)
+}
+
 #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)]
 #[serde(rename_all = "kebab-case")]
 pub enum BackupKind {