diff --git a/Cargo.lock b/Cargo.lock index a6faec4ca..3807b03f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1441,6 +1441,7 @@ dependencies = [ "chardetng", "clipboard-win", "crossterm", + "filetime", "futures-util", "helix-core", "helix-dap", diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index c3be68ac1..ad2ffd608 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -1,6 +1,5 @@ //! From -use std::fs::File; use std::io; use std::path::Path; @@ -25,10 +24,7 @@ bitflags! { mod imp { use super::*; - use rustix::{ - fd::AsFd, - fs::{Access, OpenOptionsExt}, - }; + use rustix::fs::Access; 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 { - 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 { - 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 { -// 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 read = rustix::fs::listxattr(from, &mut buf)?; - - 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()) } - } + let mut buf = vec![0; size]; + let read = rustix::fs::listxattr(from, buf.as_mut_slice())?; // 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) { - let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + for attr_byte in buf[..read].split(|&b| b == 0) { + // 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) { - let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + for attr_byte in buf[..read].split(|&b| b == 0) { + // 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 -*/ diff --git a/helix-stdx/src/path.rs b/helix-stdx/src/path.rs index fd10e7d22..47cca2e9d 100644 --- a/helix-stdx/src/path.rs +++ b/helix-stdx/src/path.rs @@ -224,7 +224,7 @@ fn path_from_bytes(slice: &[u8]) -> Result { 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'%'; diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index a567815fc..f981fe822 100644 --- a/helix-term/src/application.rs +++ 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)] diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index aba101e9f..336ab2ab8 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/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() diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 353d7a78a..8ab3985a1 100644 --- a/helix-view/Cargo.toml +++ b/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" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 3cd2b3264..97015fbbe 100644 --- a/helix-view/src/document.rs +++ b/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() + builder.permissions(perms); 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); - let Some(n) = n.checked_add(1) else { + backup = bck_base_path.with_extension(format!("{n}.{ext}")); + 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!("err"); + bail!("Could not write into a backup directory"); } } @@ -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 { - bck = Some(Backup::from(write_path.clone(), &bck_config).await?); - let bck = bck.unwrap(); - - // SECURITY: Ensure that the created file has the same perms as the original file - let dst = if !bck.copy { - 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 write_result: Result<(), Error> = async { + if path.exists() && bck_config.kind != crate::editor::BackupKind::None { + match Backup::from(path.clone(), &bck_config).await { + Ok(b) => bck = Some(b), + Err(_) if force => {} + Err(e) => bail!("Could not create backup: {e}"), } + } - let file = open_opt.open(&bck.path).await?; - let to_meta = file.metadata().await?; + if let Some(ref bck) = bck { + // 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)] - { - // TODO: set gid/uid via fchown - } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = meta.permissions().mode(); + open_opt.mode(mode); + } - #[cfg(windows)] - { - 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? - }; + let file = open_opt.open(&path).await?; - to_writer(&mut dst, encoding_with_bom_info, &text).await?; - dst.sync_all().await?; - Ok(()) - } else { - let dst = tokio::fs::File::create(&write_path).await?; - to_writer(&mut dst, encoding_with_bom_info, &text).await?; - dst.sync_all().await?; - Ok(()) - }; + #[cfg(unix)] + { + use std::os::fd::AsFd; + use std::os::unix::fs::MetadataExt; + helix_stdx::faccess::fchown( + file.as_fd(), + Some(meta.uid()), + 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 !tokio::fs::rename(&bck.path, &write_path) + if !path.exists() { + 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) - .await - .map_err(|e| { - delete_bck = false; - log::error!("Failed to restore backup on write failure: {e}") - }); + let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|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?; diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 1fa1e442c..d43e6f5ba 100644 --- a/helix-view/src/editor.rs +++ b/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, + #[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, D::Error> +where + D: Deserializer<'de>, + T: Deserialize<'de>, +{ + use serde::de::Error; + + let vec = Vec::::deserialize(deserializer)?; + if vec.is_empty() { + return Err(::custom("vector cannot be empty!")); + } + Ok(vec) +} + +pub fn deserialize_non_empty_str<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + use serde::de::Error; + + let s = String::deserialize(deserializer)?; + if s.is_empty() { + return Err(::custom("string cannot be empty")); + } + Ok(s) +} + #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)] #[serde(rename_all = "kebab-case")] pub enum BackupKind {