fix symlink detection & extension naming

helix-stdx/src/path.rs

@@ -214,12 +214,12 @@ fn os_str_as_bytes<P: AsRef<std::ffi::OsStr>>(path: P) -> Vec<u8> {
 
 fn path_from_bytes(slice: &[u8]) -> Result<PathBuf, Utf8Error> {
     #[cfg(windows)]
-    return Ok(PathBuf::from(std::str::from_utf8(slice)?));
+    let res = PathBuf::from(std::str::from_utf8(slice)?);
 
     #[cfg(unix)]
-    return Ok(PathBuf::from(
-        <std::ffi::OsStr as std::os::unix::ffi::OsStrExt>::from_bytes(slice),
-    ));
+    let res = PathBuf::from(<std::ffi::OsStr as std::os::unix::ffi::OsStrExt>::from_bytes(slice));
+
+    Ok(res)
 }
 
 fn is_sep_byte(b: u8) -> bool {
@@ -242,6 +242,20 @@ pub fn escape_path(path: &Path) -> PathBuf {
     path_from_bytes(&bytes).unwrap()
 }
 
+pub fn add_extension<'a, S: AsRef<std::ffi::OsStr>>(p: &'a Path, extension: S) -> Cow<'a, Path> {
+    let new = extension.as_ref();
+    if new.is_empty() {
+        Cow::Borrowed(p)
+    } else {
+        let Some(mut ext) = p.extension().map(std::ffi::OsStr::to_owned) else {
+            return Cow::Borrowed(p);
+        };
+        ext.push(".");
+        ext.push(new);
+        Cow::Owned(p.with_extension(ext))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use std::{

helix-stdx/tests/path.rs

@@ -1,10 +1,9 @@
 #![cfg(windows)]
 
-use std::{
-    env::set_current_dir,
-    error::Error,
-    path::{Component, Path, PathBuf},
-};
+use std::{env::set_current_dir, error::Error, path::Component};
+
+#[cfg(unix)]
+use std::path::{Path, PathBuf};
 
 use helix_stdx::path;
 use tempfile::Builder;

helix-view/src/document.rs

@@ -159,7 +159,7 @@ impl Backup {
                 let p_ = p.clone();
                 tokio::task::spawn_blocking(move || hardlink_count(&p_).unwrap_or(2)).await? > 1
             };
-            let is_symlink = tokio::fs::metadata(&p).await?.is_symlink();
+            let is_symlink = tokio::fs::symlink_metadata(&p).await?.is_symlink();
             if is_hardlink || is_symlink {
                 copy = true;
             } else {
@@ -211,7 +211,9 @@ impl Backup {
         '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.with_extension(&ext);
+
+            // NOTE: `escaped_p` will make dot files appear to be extensions, so we need to append
+            let mut backup = helix_stdx::path::add_extension(&bck_base_path, ext).into_owned();
 
             // NOTE: Should we just overwrite regardless?
             // If the backup file already exists, we'll try to add a number before the extension
@@ -219,11 +221,12 @@ 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.with_extension(format!("{n}.{ext}"));
+                backup = helix_stdx::path::add_extension(&bck_base_path, format!("{n}.{ext}"))
+                    .into_owned();
                 if n == u8::MAX {
                     continue 'outer;
                 }
-                n = n + 1;
+                n += 1;
             }
 
             if copy {
@@ -231,11 +234,11 @@ impl Backup {
                 // TODO: How handle error?
                 tokio::fs::copy(&p, &backup).await?;
 
+                let from_meta = tokio::fs::metadata(&p).await?;
                 #[cfg(unix)]
                 {
                     use std::os::unix::fs::{MetadataExt, PermissionsExt};
 
-                    let from_meta = tokio::fs::metadata(&p).await?;
                     let mut perms = from_meta.permissions();
 
                     // Strip s-bit
@@ -252,12 +255,11 @@ impl Backup {
                     }
                     std::fs::set_permissions(&backup, perms)?;
 
-                    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)?;
                 }
+                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)?;
 
                 #[cfg(windows)]
                 {
@@ -1075,7 +1077,11 @@ impl Document {
             }
 
             // Use a backup file
-            let meta = tokio::fs::metadata(&path).await?;
+            let meta = if path.exists() {
+                Some(tokio::fs::metadata(&path).await?)
+            } else {
+                None
+            };
             let mut bck = None;
             let write_result: Result<(), Error> = async {
                 if path.exists() && bck_config.kind != crate::editor::BackupKind::None {
@@ -1089,6 +1095,9 @@ impl Document {
                 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 {
+                        #[cfg(unix)]
+                        let meta = meta.as_ref().unwrap();
+
                         let mut open_opt = tokio::fs::OpenOptions::new();
                         open_opt.read(true).write(true).create_new(true);
 
@@ -1164,30 +1173,29 @@ impl Document {
                             .is_ok()
                         {
                             // Reset timestamps
+                            let meta = meta.as_ref().unwrap();
                             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, &path).await.map_err(|e| {
+                            delete_bck = false;
+                            log::error!("Failed to restore backup on write failure: {e}")
+                        });
                     } else {
-                        if bck.copy {
-                            // Restore backup from copy
-                            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, &path).await.map_err(|e| {
-                                delete_bck = false;
-                                log::error!("Failed to restore backup on write failure: {e}")
-                            });
-                        }
+                        delete_bck = false;
+                        // restore backup
+                        let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|e| {
+                            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?;
+                    let _ = tokio::fs::remove_file(bck.path).await;
                 }
             }