Repository: uutils/coreutils (language: rust)

Issue:
cp: don't show msg with `cp -i -v --update=older a b` if `a` is older than `b`
While reviewing https://github.com/uutils/coreutils/pull/7287 I noticed that we show an `'a' -> 'b'
`-message with `cp -i -v --update=older a b` if `a` is older than `b` and thus `b` is not overwritten:
```bash
$ echo "a" > a
$ echo "b" > b
$ cargo run cp -i -v --update=older a b
'a' -> 'b'
$ cat b
b
```
With GNU `cp`, there is no such message:
```bash
$ echo "a" > a
$ echo "b" > b
$ cp -i -v --update=older a b
$ cat b
b
```


Discussion / hints:
Hmm looking at the code it seams that also the filtered "not copied" files are post processed like all others. That means permissions and attributes are copied for filtered files too. Is that intended behavior? 

There could be some return value on `handle_copy_mode()` to indicate if something has been copied and thus suppress verbose output. The question is if it should also suppress the permission and attribute handling?

Fix patch:
diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs
index 0f1a6967b04..acd714a3a8e 100644
--- a/src/uu/cp/src/cp.rs
+++ b/src/uu/cp/src/cp.rs
@@ -287,6 +287,13 @@ pub struct Options {
     pub progress_bar: bool,
 }
 
+/// Enum representing if a file has been skipped.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+enum PerformedAction {
+    Copied,
+    Skipped,
+}
+
 /// Enum representing various debug states of the offload and reflink actions.
 #[derive(Debug)]
 #[allow(dead_code)] // All of them are used on Linux
@@ -1974,7 +1981,7 @@ fn handle_copy_mode(
     source_in_command_line: bool,
     source_is_fifo: bool,
     #[cfg(unix)] source_is_stream: bool,
-) -> CopyResult<()> {
+) -> CopyResult<PerformedAction> {
     let source_is_symlink = source_metadata.is_symlink();
 
     match options.copy_mode {
@@ -2043,7 +2050,7 @@ fn handle_copy_mode(
                             println!("skipped {}", dest.quote());
                         }
 
-                        return Ok(());
+                        return Ok(PerformedAction::Skipped);
                     }
                     update_control::UpdateMode::ReplaceNoneFail => {
                         return Err(Error::Error(format!("not replacing '{}'", dest.display())));
@@ -2054,7 +2061,7 @@ fn handle_copy_mode(
                         let src_time = source_metadata.modified()?;
                         let dest_time = dest_metadata.modified()?;
                         if src_time <= dest_time {
-                            return Ok(());
+                            return Ok(PerformedAction::Skipped);
                         } else {
                             options.overwrite.verify(dest, options.debug)?;
 
@@ -2096,7 +2103,7 @@ fn handle_copy_mode(
         }
     };
 
-    Ok(())
+    Ok(PerformedAction::Copied)
 }
 
 /// Calculates the permissions for the destination file in a copy operation.
@@ -2322,7 +2329,7 @@ fn copy_file(
     #[cfg(not(unix))]
     let source_is_stream = false;
 
-    handle_copy_mode(
+    let performed_action = handle_copy_mode(
         source,
         dest,
         options,
@@ -2335,7 +2342,7 @@ fn copy_file(
         source_is_stream,
     )?;
 
-    if options.verbose {
+    if options.verbose && performed_action != PerformedAction::Skipped {
         print_verbose_output(options.parents, progress_bar, source, dest);
     }
 


Test patch:
diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs
index bb57406628e..694b0153d19 100644
--- a/tests/by-util/test_cp.rs
+++ b/tests/by-util/test_cp.rs
@@ -439,6 +439,29 @@ fn test_cp_arg_update_older_dest_not_older_than_src() {
     assert_eq!(at.read(new), "new content\n");
 }
 
+#[test]
+fn test_cp_arg_update_older_dest_not_older_than_src_no_verbose_output() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    let old = "test_cp_arg_update_dest_not_older_file1";
+    let new = "test_cp_arg_update_dest_not_older_file2";
+    let old_content = "old content\n";
+    let new_content = "new content\n";
+
+    at.write(old, old_content);
+    at.write(new, new_content);
+
+    ucmd.arg(old)
+        .arg(new)
+        .arg("--verbose")
+        .arg("--update=older")
+        .succeeds()
+        .no_stderr()
+        .no_stdout();
+
+    assert_eq!(at.read(new), "new content\n");
+}
+
 #[test]
 fn test_cp_arg_update_older_dest_older_than_src() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -464,6 +487,32 @@ fn test_cp_arg_update_older_dest_older_than_src() {
     assert_eq!(at.read(old), "new content\n");
 }
 
+#[test]
+fn test_cp_arg_update_older_dest_older_than_src_with_verbose_output() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    let old = "test_cp_arg_update_dest_older_file1";
+    let new = "test_cp_arg_update_dest_older_file2";
+    let old_content = "old content\n";
+    let new_content = "new content\n";
+
+    let mut f = at.make_file(old);
+    f.write_all(old_content.as_bytes()).unwrap();
+    f.set_modified(std::time::UNIX_EPOCH).unwrap();
+
+    at.write(new, new_content);
+
+    ucmd.arg(new)
+        .arg(old)
+        .arg("--verbose")
+        .arg("--update=older")
+        .succeeds()
+        .no_stderr()
+        .stdout_is(format!("'{new}' -> '{old}'\n"));
+
+    assert_eq!(at.read(old), "new content\n");
+}
+
 #[test]
 fn test_cp_arg_update_short_no_overwrite() {
     // same as --update=older