cp: fix cp throwing error when dest is symlink and options backup and --rem is given

Author: matrixhead

src/uu/cp/src/cp.rs

@@ -1669,34 +1669,33 @@ fn handle_existing_dest(
             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_dest_removed
-                && (is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly())
-            {
+    if !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() {
+                    fs::remove_file(dest)?;
+                }
+            }
+            OverwriteMode::Clobber(ClobberMode::RemoveDestination) => {
                 fs::remove_file(dest)?;
             }
-        }
-        OverwriteMode::Clobber(ClobberMode::RemoveDestination) => {
-            fs::remove_file(dest)?;
-        }
-        OverwriteMode::Clobber(ClobberMode::Standard) => {
-            // Consider the following files:
-            //
-            // * `src/f` - a regular file
-            // * `src/link` - a hard link to `src/f`
-            // * `dest/src/f` - a different regular file
-            //
-            // In this scenario, if we do `cp -a src/ dest/`, it is
-            // possible that the order of traversal causes `src/link`
-            // to get copied first (to `dest/src/link`). In that case,
-            // in order to make sure `dest/src/link` is a hard link to
-            // `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()
+            OverwriteMode::Clobber(ClobberMode::Standard) => {
+                // Consider the following files:
+                //
+                // * `src/f` - a regular file
+                // * `src/link` - a hard link to `src/f`
+                // * `dest/src/f` - a different regular file
+                //
+                // In this scenario, if we do `cp -a src/ dest/`, it is
+                // possible that the order of traversal causes `src/link`
+                // to get copied first (to `dest/src/link`). In that case,
+                // in order to make sure `dest/src/link` is a hard link to
+                // `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()
             // only try to remove dest file only if the current source
             // is hardlink to a file that is already copied
                 && copied_files.contains_key(
@@ -1705,14 +1704,13 @@ fn handle_existing_dest(
                         options.dereference(source_in_command_line),
                     )
                     .context(format!("cannot stat {}", source.quote()))?,
-                )
-                && !is_dest_removed
-            {
-                fs::remove_file(dest)?;
+                ) {
+                    fs::remove_file(dest)?;
+                }
             }
-        }
-        _ => (),
-    };
+            _ => (),
+        };
+    }
 
     Ok(())
 }
@@ -2043,6 +2041,7 @@ fn copy_file(
                 options.overwrite,
                 OverwriteMode::Clobber(ClobberMode::RemoveDestination)
             )
+            && options.backup == BackupMode::NoBackup
         {
             fs::remove_file(dest)?;
         }

tests/by-util/test_cp.rs

@@ -5696,3 +5696,23 @@ fn test_cp_parents_absolute_path() {
     let res = format!("dest{}/a/b/f", at.root_dir_resolved());
     at.file_exists(res);
 }
+
+// make sure that cp backup dest symlink before removing it.
+#[test]
+fn test_cp_with_options_backup_and_rem_when_dest_is_symlink() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.write("file", "xyz");
+    at.mkdir("inner_dir");
+    at.write("inner_dir/inner_file", "abc");
+    at.relative_symlink_file("inner_file", "inner_dir/sl");
+    scene
+        .ucmd()
+        .args(&["-b", "--rem", "file", "inner_dir/sl"])
+        .succeeds();
+    assert!(at.file_exists("inner_dir/inner_file"));
+    assert_eq!(at.read("inner_dir/inner_file"), "abc");
+    assert!(at.symlink_exists("inner_dir/sl~"));
+    assert!(!at.symlink_exists("inner_dir/sl"));
+    assert_eq!(at.read("inner_dir/sl"), "xyz");
+}