cp: Added additional gnu test which *has* to work in combination with needed changes for the added test

Author: nikolalukovic

src/uu/cp/src/copydir.rs

@@ -503,7 +503,7 @@ pub(crate) fn copy_directory(
                             copy_attributes(
                                 &entry.source_absolute,
                                 &entry.local_to_target,
-                                &options.attributes,
+                                options,
                             )?;
                         }
                     }
@@ -520,7 +520,7 @@ pub(crate) fn copy_directory(
     // Fix permissions for all directories we created
     // This ensures that even sibling directories get their permissions fixed
     for (source_path, dest_path) in dirs_needing_permissions {
-        copy_attributes(&source_path, &dest_path, &options.attributes)?;
+        copy_attributes(&source_path, &dest_path, options)?;
     }
 
     // Also fix permissions for parent directories,
@@ -529,7 +529,7 @@ pub(crate) fn copy_directory(
         let dest = target.join(root.file_name().unwrap());
         for (x, y) in aligned_ancestors(root, dest.as_path()) {
             if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) {
-                copy_attributes(&src, y, &options.attributes)?;
+                copy_attributes(&src, y, options)?;
             }
         }
     }

src/uu/cp/src/cp.rs

@@ -1506,7 +1506,7 @@ fn copy_source(
         if options.parents {
             for (x, y) in aligned_ancestors(source, dest.as_path()) {
                 if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) {
-                    copy_attributes(&src, y, &options.attributes)?;
+                    copy_attributes(&src, y, options)?;
                 }
             }
         }
@@ -1655,25 +1655,14 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> {
 }
 
 /// Copy the specified attributes from one path to another.
-pub(crate) fn copy_attributes(
-    source: &Path,
-    dest: &Path,
-    attributes: &Attributes,
-) -> CopyResult<()> {
+pub(crate) fn copy_attributes(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> {
     let context = &*format!("{} -> {}", source.quote(), dest.quote());
     let source_metadata =
         fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
 
-    // if --no-preserve wasn't explicitly passed and we're copying a directory by default we should preserve mode
-    let mode = if dest.is_dir() && attributes.mode == (Preserve::No { explicit: false }) {
-        Preserve::Yes { required: false }
-    } else {
-        attributes.mode
-    };
-
     // Ownership must be changed first to avoid interfering with mode change.
     #[cfg(unix)]
-    handle_preserve(&attributes.ownership, || -> CopyResult<()> {
+    handle_preserve(&options.attributes.ownership, || -> CopyResult<()> {
         use std::os::unix::prelude::MetadataExt;
         use uucore::perms::Verbosity;
         use uucore::perms::VerbosityLevel;
@@ -1708,14 +1697,22 @@ pub(crate) fn copy_attributes(
         Ok(())
     })?;
 
-    handle_preserve(&mode, || -> CopyResult<()> {
+    handle_preserve(&options.attributes.mode, || -> CopyResult<()> {
         // The `chmod()` system call that underlies the
         // `fs::set_permissions()` call is unable to change the
         // permissions of a symbolic link. In that case, we just
         // do nothing, since every symbolic link has the same
         // permissions.
         if !dest.is_symlink() {
-            fs::set_permissions(dest, source_metadata.permissions())
+            // let dest_permissions = calculate_dest_permissions(
+            //     fs::metadata(dest).ok().as_ref(),
+            //     dest,
+            //     &source_metadata,
+            //     options,
+            //     context,
+            // )?;
+            let dest_permissions = source_metadata.permissions();
+            fs::set_permissions(dest, dest_permissions)
                 .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
             // FIXME: Implement this for windows as well
             #[cfg(feature = "feat_acl")]
@@ -1727,7 +1724,7 @@ pub(crate) fn copy_attributes(
         Ok(())
     })?;
 
-    handle_preserve(&attributes.timestamps, || -> CopyResult<()> {
+    handle_preserve(&options.attributes.timestamps, || -> CopyResult<()> {
         let atime = FileTime::from_last_access_time(&source_metadata);
         let mtime = FileTime::from_last_modification_time(&source_metadata);
         if dest.is_symlink() {
@@ -1740,7 +1737,7 @@ pub(crate) fn copy_attributes(
     })?;
 
     #[cfg(feature = "selinux")]
-    handle_preserve(&attributes.context, || -> CopyResult<()> {
+    handle_preserve(&options.attributes.context, || -> CopyResult<()> {
         // Get the source context and apply it to the destination
         if let Ok(context) = selinux::SecurityContext::of_path(source, false, false) {
             if let Some(context) = context {
@@ -1758,7 +1755,7 @@ pub(crate) fn copy_attributes(
         Ok(())
     })?;
 
-    handle_preserve(&attributes.xattr, || -> CopyResult<()> {
+    handle_preserve(&options.attributes.xattr, || -> CopyResult<()> {
         #[cfg(all(unix, not(target_os = "android")))]
         {
             copy_extended_attrs(source, dest)?;
@@ -2519,7 +2516,7 @@ fn copy_file(
     if options.dereference(source_in_command_line) {
         if let Ok(src) = canonicalize(source, MissingHandling::Normal, ResolveMode::Physical) {
             if src.exists() {
-                copy_attributes(&src, dest, &options.attributes)?;
+                copy_attributes(&src, dest, options)?;
             }
         }
     } else if source_is_stream && !source.exists() {
@@ -2528,7 +2525,7 @@ fn copy_file(
         // attributes. However, this is already handled in the stream
         // copy function (see `copy_stream` under platform/linux.rs).
     } else {
-        copy_attributes(source, dest, &options.attributes)?;
+        copy_attributes(source, dest, options)?;
     }
 
     #[cfg(feature = "selinux")]
@@ -2704,7 +2701,7 @@ fn copy_link(
         delete_path(dest, options)?;
     }
     symlink_file(&link, dest, symlinked_files)?;
-    copy_attributes(source, dest, &options.attributes)
+    copy_attributes(source, dest, options)
 }
 
 /// Generate an error message if `target` is not the correct `target_type`

tests/by-util/test_cp.rs

@@ -7415,6 +7415,8 @@ fn test_cp_recurse_verbose_output_with_symlink_already_exists() {
 #[test]
 #[cfg(not(target_os = "windows"))]
 fn test_cp_preserve_directory_permissions_by_default() {
+    use std::io;
+
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
 
@@ -7430,7 +7432,13 @@ fn test_cp_preserve_directory_permissions_by_default() {
     scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds();
     scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds();
 
-    scene.ucmd().arg("-r").arg("a").arg("c").succeeds();
+    scene
+        .ucmd()
+        .arg("-r")
+        .arg("a")
+        .arg("c")
+        .set_stdout(io::stdout())
+        .succeeds();
 
     assert_eq!(get_mode(at.plus("b")), 0o40555);
     assert_eq!(get_mode(at.plus("b/b")), 0o40555);
@@ -7445,37 +7453,38 @@ fn test_cp_preserve_directory_permissions_by_default() {
 
 #[test]
 #[cfg(not(target_os = "windows"))]
-fn test_cp_no_preserve_directory_permissions() {
+fn test_cp_existing_perm_dir() {
+    use std::io;
+
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
 
-    let dir = "a/b/c/d";
-    let file = "foo.txt";
-
-    at.mkdir_all(dir);
-
-    let file_path = format!("{dir}/{file}");
-
-    at.touch(file_path);
-
-    scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds();
-    scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds();
+    scene
+        .cmd("mkdir")
+        .arg("-p")
+        .arg("-m")
+        .arg("ug-s,u=rwx,g=rwx,o=rx")
+        .arg("src/dir")
+        .umask(0o022)
+        .succeeds();
+    scene
+        .cmd("mkdir")
+        .arg("-p")
+        .arg("-m")
+        .arg("ug-s,u=rwx,g=,o=")
+        .arg("dst/dir")
+        .umask(0o022)
+        .succeeds();
 
     scene
         .ucmd()
         .arg("-r")
-        .arg("--no-preserve=mode")
-        .arg("a")
-        .arg("c")
+        .arg("src/.")
+        .arg("dst/")
+        .set_stdout(io::stdout())
         .succeeds();
 
-    assert_eq!(get_mode(at.plus("b")), 0o40555);
-    assert_eq!(get_mode(at.plus("b/b")), 0o40555);
-    assert_eq!(get_mode(at.plus("b/b/c")), 0o40555);
-    assert_eq!(get_mode(at.plus("b/b/c/d")), 0o40555);
+    let mode = get_mode(at.plus("dst/dir"));
 
-    assert_eq!(get_mode(at.plus("c")), 0o40755);
-    assert_eq!(get_mode(at.plus("c/b")), 0o40755);
-    assert_eq!(get_mode(at.plus("c/b/c")), 0o40755);
-    assert_eq!(get_mode(at.plus("c/b/c/d")), 0o40755);
+    assert_eq!(mode, 0o40700);
 }