Merge pull request #5699 from sylvestre/mv-cp-seen

cp/mv/ln: add support for the "will not overwrite just-created"

src/uu/cp/src/cp.rs

@@ -1171,6 +1171,9 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult
     //
     // key is the source file's information and the value is the destination filepath.
     let mut copied_files: HashMap<FileInformation, PathBuf> = HashMap::with_capacity(sources.len());
+    // remember the copied destinations for further usage.
+    // we can't use copied_files as it is because the key is the source file's information.
+    let mut copied_destinations: HashSet<PathBuf> = HashSet::with_capacity(sources.len());
 
     let progress_bar = if options.progress_bar {
         let pb = ProgressBar::new(disk_usage(sources, options.recursive)?)
@@ -1191,17 +1194,38 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult
         if seen_sources.contains(source) {
             // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases)
             show_warning!("source {} specified more than once", source.quote());
-        } else if let Err(error) = copy_source(
+        } else {
-            &progress_bar,
+            let dest = construct_dest_path(source, target, target_type, options)
-            source,
+                .unwrap_or_else(|_| target.to_path_buf());
-            target,
+
-            target_type,
+            if fs::metadata(&dest).is_ok() && !fs::symlink_metadata(&dest)?.file_type().is_symlink()
-            options,
+            {
-            &mut symlinked_files,
+                // There is already a file and it isn't a symlink (managed in a different place)
-            &mut copied_files,
+                if copied_destinations.contains(&dest)
-        ) {
+                    && options.backup != BackupMode::NumberedBackup
-            show_error_if_needed(&error);
+                {
-            non_fatal_errors = true;
+                    // If the target file was already created in this cp call, do not overwrite
+                    return Err(Error::Error(format!(
+                        "will not overwrite just-created '{}' with '{}'",
+                        dest.display(),
+                        source.display()
+                    )));
+                }
+            }
+
+            if let Err(error) = copy_source(
+                &progress_bar,
+                source,
+                target,
+                target_type,
+                options,
+                &mut symlinked_files,
+                &mut copied_files,
+            ) {
+                show_error_if_needed(&error);
+                non_fatal_errors = true;
+            }
+            copied_destinations.insert(dest.clone());
         }
         seen_sources.insert(source);
     }

src/uu/ln/src/ln.rs

@@ -12,6 +12,7 @@ use uucore::fs::{make_path_relative_to, paths_refer_to_same_file};
 use uucore::{format_usage, help_about, help_section, help_usage, prompt_yes, show_error};
 
 use std::borrow::Cow;
+use std::collections::HashSet;
 use std::error::Error;
 use std::ffi::OsString;
 use std::fmt::Display;
@@ -295,6 +296,8 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings)
     if !target_dir.is_dir() {
         return Err(LnError::TargetIsDirectory(target_dir.to_owned()).into());
     }
+    // remember the linked destinations for further usage
+    let mut linked_destinations: HashSet<PathBuf> = HashSet::with_capacity(files.len());
 
     let mut all_successful = true;
     for srcpath in files {
@@ -338,10 +341,20 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings)
                 }
             };
 
-        if let Err(e) = link(srcpath, &targetpath, settings) {
+        if linked_destinations.contains(&targetpath) {
+            // If the target file was already created in this ln call, do not overwrite
+            show_error!(
+                "will not overwrite just-created '{}' with '{}'",
+                targetpath.display(),
+                srcpath.display()
+            );
+            all_successful = false;
+        } else if let Err(e) = link(srcpath, &targetpath, settings) {
             show_error!("{}", e);
             all_successful = false;
         }
+
+        linked_destinations.insert(targetpath.clone());
     }
     if all_successful {
         Ok(())

src/uu/mv/src/mv.rs

@@ -10,6 +10,7 @@ mod error;
 use clap::builder::ValueParser;
 use clap::{crate_version, error::ErrorKind, Arg, ArgAction, ArgMatches, Command};
 use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
+use std::collections::HashSet;
 use std::env;
 use std::ffi::OsString;
 use std::fs;
@@ -433,7 +434,10 @@ pub fn mv(files: &[OsString], opts: &Options) -> UResult<()> {
 }
 
 #[allow(clippy::cognitive_complexity)]
-fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, opts: &Options) -> UResult<()> {
+fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options) -> UResult<()> {
+    // remember the moved destinations for further usage
+    let mut moved_destinations: HashSet<PathBuf> = HashSet::with_capacity(files.len());
+
     if !target_dir.is_dir() {
         return Err(MvError::NotADirectory(target_dir.quote().to_string()).into());
     }
@@ -442,7 +446,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, opts: &Options) ->
         .canonicalize()
         .unwrap_or_else(|_| target_dir.to_path_buf());
 
-    let multi_progress = opts.progress_bar.then(MultiProgress::new);
+    let multi_progress = options.progress_bar.then(MultiProgress::new);
 
     let count_progress = if let Some(ref multi_progress) = multi_progress {
         if files.len() > 1 {
@@ -471,6 +475,20 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, opts: &Options) ->
             }
         };
 
+        if moved_destinations.contains(&targetpath) && options.backup != BackupMode::NumberedBackup
+        {
+            // If the target file was already created in this mv call, do not overwrite
+            show!(USimpleError::new(
+                1,
+                format!(
+                    "will not overwrite just-created '{}' with '{}'",
+                    targetpath.display(),
+                    sourcepath.display()
+                ),
+            ));
+            continue;
+        }
+
         // Check if we have mv dir1 dir2 dir2
         // And generate an error if this is the case
         if let Ok(canonicalized_source) = sourcepath.canonicalize() {
@@ -493,7 +511,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, opts: &Options) ->
             }
         }
 
-        match rename(sourcepath, &targetpath, opts, multi_progress.as_ref()) {
+        match rename(sourcepath, &targetpath, options, multi_progress.as_ref()) {
             Err(e) if e.to_string().is_empty() => set_exit_code(1),
             Err(e) => {
                 let e = e.map_err_context(|| {
@@ -513,6 +531,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, opts: &Options) ->
         if let Some(ref pb) = count_progress {
             pb.inc(1);
         }
+        moved_destinations.insert(targetpath.clone());
     }
     Ok(())
 }

tests/by-util/test_cp.rs

@@ -3592,3 +3592,36 @@ fn test_cp_attributes_only() {
     assert_eq!(mode_a, at.metadata(a).mode());
     assert_eq!(mode_b, at.metadata(b).mode());
 }
+
+#[test]
+fn test_cp_seen_file() {
+    let ts = TestScenario::new(util_name!());
+    let at = &ts.fixtures;
+
+    at.mkdir("a");
+    at.mkdir("b");
+    at.mkdir("c");
+    at.write("a/f", "a");
+    at.write("b/f", "b");
+
+    let result = ts.ucmd().arg("a/f").arg("b/f").arg("c").fails();
+    #[cfg(not(unix))]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c\\f' with 'b/f'"));
+    #[cfg(unix)]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c/f' with 'b/f'"));
+
+    assert!(at.plus("c").join("f").exists());
+
+    ts.ucmd()
+        .arg("--backup=numbered")
+        .arg("a/f")
+        .arg("b/f")
+        .arg("c")
+        .succeeds();
+    assert!(at.plus("c").join("f").exists());
+    assert!(at.plus("c").join("f.~1~").exists());
+}

tests/by-util/test_ln.rs

@@ -3,6 +3,8 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 use crate::common::util::TestScenario;
+#[cfg(unix)]
+use std::os::unix::fs::MetadataExt;
 use std::path::PathBuf;
 
 #[test]
@@ -719,3 +721,49 @@ fn test_symlink_remove_existing_same_src_and_dest() {
     assert!(at.file_exists("a") && !at.symlink_exists("a"));
     assert_eq!(at.read("a"), "sample");
 }
+
+#[test]
+#[cfg(not(target_os = "android"))]
+fn test_ln_seen_file() {
+    let ts = TestScenario::new(util_name!());
+    let at = &ts.fixtures;
+
+    at.mkdir("a");
+    at.mkdir("b");
+    at.mkdir("c");
+    at.write("a/f", "a");
+    at.write("b/f", "b");
+
+    let result = ts.ucmd().arg("a/f").arg("b/f").arg("c").fails();
+
+    #[cfg(not(unix))]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c\\f' with 'b/f'"));
+    #[cfg(unix)]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c/f' with 'b/f'"));
+
+    assert!(at.plus("c").join("f").exists());
+    // b/f still exists
+    assert!(at.plus("b").join("f").exists());
+    // a/f still exists
+    assert!(at.plus("a").join("f").exists());
+    #[cfg(unix)]
+    {
+        // Check inode numbers
+        let inode_a_f = at.plus("a").join("f").metadata().unwrap().ino();
+        let inode_b_f = at.plus("b").join("f").metadata().unwrap().ino();
+        let inode_c_f = at.plus("c").join("f").metadata().unwrap().ino();
+
+        assert_eq!(
+            inode_a_f, inode_c_f,
+            "Inode numbers of a/f and c/f should be equal"
+        );
+        assert_ne!(
+            inode_b_f, inode_c_f,
+            "Inode numbers of b/f and c/f should not be equal"
+        );
+    }
+}

tests/by-util/test_mv.rs

@@ -1469,6 +1469,65 @@ fn test_mv_file_into_dir_where_both_are_files() {
         .stderr_contains("mv: failed to access 'b/': Not a directory");
 }
 
+#[test]
+fn test_mv_seen_file() {
+    let ts = TestScenario::new(util_name!());
+    let at = &ts.fixtures;
+
+    at.mkdir("a");
+    at.mkdir("b");
+    at.mkdir("c");
+    at.write("a/f", "a");
+    at.write("b/f", "b");
+
+    let result = ts.ucmd().arg("a/f").arg("b/f").arg("c").fails();
+
+    #[cfg(not(unix))]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c\\f' with 'b/f'"));
+    #[cfg(unix)]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c/f' with 'b/f'"));
+
+    // a/f has been moved into c/f
+    assert!(at.plus("c").join("f").exists());
+    // b/f still exists
+    assert!(at.plus("b").join("f").exists());
+    // a/f no longer exists
+    assert!(!at.plus("a").join("f").exists());
+}
+
+#[test]
+fn test_mv_seen_multiple_files_to_directory() {
+    let ts = TestScenario::new(util_name!());
+    let at = &ts.fixtures;
+
+    at.mkdir("a");
+    at.mkdir("b");
+    at.mkdir("c");
+    at.write("a/f", "a");
+    at.write("b/f", "b");
+    at.write("b/g", "g");
+
+    let result = ts.ucmd().arg("a/f").arg("b/f").arg("b/g").arg("c").fails();
+    #[cfg(not(unix))]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c\\f' with 'b/f'"));
+    #[cfg(unix)]
+    assert!(result
+        .stderr_str()
+        .contains("will not overwrite just-created 'c/f' with 'b/f'"));
+
+    assert!(!at.plus("a").join("f").exists());
+    assert!(at.plus("b").join("f").exists());
+    assert!(!at.plus("b").join("g").exists());
+    assert!(at.plus("c").join("f").exists());
+    assert!(at.plus("c").join("g").exists());
+}
+
 #[test]
 fn test_mv_dir_into_file_where_both_are_files() {
     let scene = TestScenario::new(util_name!());