Merge pull request #6119 from hamflx/fix/invalid-backup-numbered-path

mv: fix invalid numbered backup path

Author: sylvestre

src/uucore/src/lib/features/backup_control.rs

@@ -421,25 +421,29 @@ pub fn get_backup_path(
 }
 
 fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf {
-    let mut p = path.to_string_lossy().into_owned();
-    p.push_str(suffix);
-    PathBuf::from(p)
+    let mut file_name = path.file_name().unwrap_or_default().to_os_string();
+    file_name.push(suffix);
+    path.with_file_name(file_name)
 }
 
 fn numbered_backup_path(path: &Path) -> PathBuf {
+    let file_name = path.file_name().unwrap_or_default();
     for i in 1_u64.. {
-        let path_str = &format!("{}.~{}~", path.to_string_lossy(), i);
-        let path = Path::new(path_str);
+        let mut numbered_file_name = file_name.to_os_string();
+        numbered_file_name.push(format!(".~{}~", i));
+        let path = path.with_file_name(numbered_file_name);
         if !path.exists() {
-            return path.to_path_buf();
+            return path;
         }
     }
     panic!("cannot create backup")
 }
 
 fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
-    let test_path_str = &format!("{}.~1~", path.to_string_lossy());
-    let test_path = Path::new(test_path_str);
+    let file_name = path.file_name().unwrap_or_default();
+    let mut numbered_file_name = file_name.to_os_string();
+    numbered_file_name.push(".~1~");
+    let test_path = path.with_file_name(numbered_file_name);
     if test_path.exists() {
         numbered_backup_path(path)
     } else {
@@ -660,6 +664,44 @@ mod tests {
         let result = determine_backup_suffix(&matches);
         assert_eq!(result, "-v");
     }
+
+    #[test]
+    fn test_numbered_backup_path() {
+        assert_eq!(numbered_backup_path(&Path::new("")), PathBuf::from(".~1~"));
+        assert_eq!(
+            numbered_backup_path(&Path::new("/")),
+            PathBuf::from("/.~1~")
+        );
+        assert_eq!(
+            numbered_backup_path(&Path::new("/hello/world")),
+            PathBuf::from("/hello/world.~1~")
+        );
+        assert_eq!(
+            numbered_backup_path(&Path::new("/hello/world/")),
+            PathBuf::from("/hello/world.~1~")
+        );
+    }
+
+    #[test]
+    fn test_simple_backup_path() {
+        assert_eq!(
+            simple_backup_path(&Path::new(""), ".bak"),
+            PathBuf::from(".bak")
+        );
+        assert_eq!(
+            simple_backup_path(&Path::new("/"), ".bak"),
+            PathBuf::from("/.bak")
+        );
+        assert_eq!(
+            simple_backup_path(&Path::new("/hello/world"), ".bak"),
+            PathBuf::from("/hello/world.bak")
+        );
+        assert_eq!(
+            simple_backup_path(&Path::new("/hello/world/"), ".bak"),
+            PathBuf::from("/hello/world.bak")
+        );
+    }
+
     #[test]
     fn test_source_is_target_backup() {
         let source = Path::new("data.txt.bak");

tests/by-util/test_mv.rs

@@ -571,6 +571,30 @@ fn test_mv_simple_backup() {
     assert!(at.file_exists(format!("{file_b}~")));
 }
 
+#[test]
+fn test_mv_simple_backup_for_directory() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir_a = "test_mv_simple_backup_dir_a";
+    let dir_b = "test_mv_simple_backup_dir_b";
+
+    at.mkdir(dir_a);
+    at.mkdir(dir_b);
+    at.touch(format!("{dir_a}/file_a"));
+    at.touch(format!("{dir_b}/file_b"));
+    ucmd.arg("-T")
+        .arg("-b")
+        .arg(dir_a)
+        .arg(dir_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(!at.dir_exists(dir_a));
+    assert!(at.dir_exists(dir_b));
+    assert!(at.dir_exists(&format!("{dir_b}~")));
+    assert!(at.file_exists(format!("{dir_b}/file_a")));
+    assert!(at.file_exists(format!("{dir_b}~/file_b")));
+}
+
 #[test]
 fn test_mv_simple_backup_with_file_extension() {
     let (at, mut ucmd) = at_and_ucmd!();