cp: backup, hardlink fixes

Author: matrixhead

src/uu/cp/src/cp.rs

@@ -1499,10 +1499,11 @@ fn context_for(src: &Path, dest: &Path) -> String {
     format!("{} -> {}", src.quote(), dest.quote())
 }
 
-/// Implements a simple backup copy for the destination file.
+/// Implements a simple backup copy for the destination file .
+/// if is_dest_symlink flag is set to true dest will be renamed to backup_path
 /// TODO: for the backup, should this function be replaced by `copy_file(...)`?
-fn backup_dest(dest: &Path, backup_path: &Path) -> CopyResult<PathBuf> {
-    if dest.is_symlink() {
+fn backup_dest(dest: &Path, backup_path: &Path, is_dest_symlink: bool) -> CopyResult<PathBuf> {
+    if is_dest_symlink {
         fs::rename(dest, backup_path)?;
     } else {
         fs::copy(dest, backup_path)?;
@@ -1536,6 +1537,7 @@ fn handle_existing_dest(
     dest: &Path,
     options: &Options,
     source_in_command_line: bool,
+    copied_files: &mut HashMap<FileInformation, PathBuf>,
 ) -> CopyResult<()> {
     // Disallow copying a file to itself, unless `--force` and
     // `--backup` are both specified.
@@ -1545,6 +1547,7 @@ fn handle_existing_dest(
 
     options.overwrite.verify(dest)?;
 
+    let mut is_dest_removed = false;
     let backup_path = backup_control::get_backup_path(options.backup, dest, &options.backup_suffix);
     if let Some(backup_path) = backup_path {
         if paths_refer_to_same_file(source, &backup_path, true) {
@@ -1555,13 +1558,16 @@ fn handle_existing_dest(
             )
             .into());
         } else {
-            backup_dest(dest, &backup_path)?;
+            is_dest_removed = dest.is_symlink();
+            backup_dest(dest, &backup_path, is_dest_removed)?;
         }
     }
     match options.overwrite {
         // FIXME: print that the file was removed if --verbose is enabled
         OverwriteMode::Clobber(ClobberMode::Force) => {
-            if is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly() {
+            if !is_dest_removed
+                && (is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly())
+            {
                 fs::remove_file(dest)?;
             }
         }
@@ -1582,7 +1588,18 @@ fn handle_existing_dest(
             // `dest/src/f` and `dest/src/f` has the contents of
             // `src/f`, we delete the existing file to allow the hard
             // linking.
-            if options.preserve_hard_links() {
+
+            if options.preserve_hard_links()
+            // only try to remove dest file only if the current source is hardlink to a file that is already copied  
+                && copied_files.contains_key(
+                    &FileInformation::from_path(
+                        source,
+                        options.dereference(source_in_command_line),
+                    )
+                    .context(format!("cannot stat {}", source.quote()))?,
+                )
+                && !is_dest_removed
+            {
                 fs::remove_file(dest)?;
             }
         }
@@ -1708,7 +1725,7 @@ fn handle_copy_mode(
                 let backup_path =
                     backup_control::get_backup_path(options.backup, dest, &options.backup_suffix);
                 if let Some(backup_path) = backup_path {
-                    backup_dest(dest, &backup_path)?;
+                    backup_dest(dest, &backup_path, dest.is_symlink())?;
                     fs::remove_file(dest)?;
                 }
                 if options.overwrite == OverwriteMode::Clobber(ClobberMode::Force) {
@@ -1935,7 +1952,7 @@ fn copy_file(
         {
             return Ok(());
         }
-        handle_existing_dest(source, dest, options, source_in_command_line)?;
+        handle_existing_dest(source, dest, options, source_in_command_line, copied_files)?;
     }
 
     if options.attributes_only

tests/by-util/test_cp.rs

@@ -3855,3 +3855,71 @@ fn test_cp_no_dereference_attributes_only_with_symlink() {
         "file2 content does not match expected"
     );
 }
+
+#[cfg(all(unix, not(target_os = "macos")))]
+#[test]
+fn test_cp_deref_dest_before_copying_with_preserve_links() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.write("file1", "1");
+    at.write("file2", "2");
+    at.symlink_file("file1", "sym1");
+
+    scene.ucmd().args(&["-d", "file2", "sym1"]).succeeds();
+
+    assert!(at.symlink_exists("sym1"));
+
+    assert_eq!(at.read("file1"), "2",);
+}
+
+#[cfg(all(unix, not(target_os = "macos")))]
+#[test]
+fn test_cp_backup_when_dest_is_symlink_to_same_file_with_preserve_links() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.write("file1", "1");
+    at.symlink_file("file1", "sym1");
+
+    scene.ucmd().args(&["-bd", "file1", "sym1"]).succeeds();
+
+    assert!(at.symlink_exists("sym1~"));
+    assert!(at.file_exists("sym1"));
+
+    assert_eq!(at.read("sym1"), "1",);
+}
+
+#[cfg(all(unix, not(target_os = "macos")))]
+#[test]
+fn test_cp_backup_when_dest_is_symlink_to_same_file_with_force() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.write("file1", "1");
+    at.symlink_file("file1", "sym1");
+
+    scene.ucmd().args(&["-bf", "file1", "sym1"]).succeeds();
+
+    assert!(at.symlink_exists("sym1~"));
+    assert!(at.file_exists("sym1"));
+
+    assert_eq!(at.read("sym1"), "1",);
+}
+
+#[cfg(all(unix, not(target_os = "macos")))]
+#[test]
+fn test_cp_backup_when_dest_is_symlink_to_same_file_with_force_and_preserve_links() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.write("file1", "1");
+    at.symlink_file("file1", "sym1");
+
+    scene.ucmd().args(&["-bdf", "file1", "sym1"]).succeeds();
+
+    assert!(at.symlink_exists("sym1~"));
+    assert!(at.file_exists("sym1"));
+
+    assert_eq!(at.read("sym1"), "1",);
+}