From 1f086bad9e75b74ca5f75a8dccd92a5cd9ad0f7c Mon Sep 17 00:00:00 2001
From: Sylvestre Ledru <sylvestre@debian.org>
Date: Sat, 7 Feb 2026 16:43:35 +0100
Subject: [PATCH] cp: improve code clarity and remove redundant filesystem
 checks

---
 src/uu/cp/src/copydir.rs | 33 ++++++++++++++++++++++++---------
 src/uu/cp/src/cp.rs      |  6 +++---
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs
index 614bd597d18..7f63b5e7e05 100644
--- a/src/uu/cp/src/copydir.rs
+++ b/src/uu/cp/src/copydir.rs
@@ -31,6 +31,16 @@ use crate::{
     copy_file,
 };
 
+/// Represents a directory that needs permission fixup after copying its contents.
+struct DirNeedingPermissions {
+    /// Absolute path to the source directory
+    source: PathBuf,
+    /// Path to the destination directory
+    dest: PathBuf,
+    /// Whether this directory was freshly created by the copy operation
+    was_created: bool,
+}
+
 /// Ensure a Windows path starts with a `\\?`.
 #[cfg(target_os = "windows")]
 fn adjust_canonicalization(p: &Path) -> Cow<'_, Path> {
@@ -237,7 +247,12 @@ impl Entry {
 
 #[allow(clippy::too_many_arguments)]
 /// Copy a single entry during a directory traversal.
-/// Returns a bool value indicating whether this function created a directory or not
+///
+/// # Returns
+///
+/// Returns `Ok(true)` if this function created a new directory, `Ok(false)` otherwise.
+/// This information is used to determine whether default directory permissions should
+/// be preserved during attribute copying.
 fn copy_direntry(
     progress_bar: Option<&ProgressBar>,
     entry: &Entry,
@@ -422,7 +437,7 @@ pub(crate) fn copy_directory(
     let mut last_iter: Option<DirEntry> = None;
 
     // Keep track of all directories we've created that need permission fixes
-    let mut dirs_needing_permissions: Vec<(PathBuf, PathBuf, bool)> = Vec::new();
+    let mut dirs_needing_permissions: Vec<DirNeedingPermissions> = Vec::new();
 
     // Traverse the contents of the directory, copying each one.
     for direntry_result in WalkDir::new(root)
@@ -481,11 +496,11 @@ pub(crate) fn copy_directory(
                         continue;
                     }
                     // Add this directory to our list for permission fixing later
-                    dirs_needing_permissions.push((
-                        entry.source_absolute.clone(),
-                        entry.local_to_target.clone(),
-                        created,
-                    ));
+                    dirs_needing_permissions.push(DirNeedingPermissions {
+                        source: entry.source_absolute.clone(),
+                        dest: entry.local_to_target.clone(),
+                        was_created: created,
+                    });
 
                     // If true, last_iter is not a parent of this iter.
                     // The means we just exited a directory.
@@ -534,8 +549,8 @@ 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, created) in dirs_needing_permissions {
-        copy_attributes(&source_path, &dest_path, &options.attributes, created)?;
+    for dir in dirs_needing_permissions {
+        copy_attributes(&dir.source, &dir.dest, &options.attributes, dir.was_created)?;
     }
 
     // Also fix permissions for parent directories,
diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs
index c57d962d0eb..db38d5c3e5b 100644
--- a/src/uu/cp/src/cp.rs
+++ b/src/uu/cp/src/cp.rs
@@ -1717,16 +1717,16 @@ pub(crate) fn copy_attributes(
     source: &Path,
     dest: &Path,
     attributes: &Attributes,
-    created: bool,
+    dest_is_freshly_created_dir: bool,
 ) -> CopyResult<()> {
     let context = &*format!("{} -> {}", source.quote(), dest.quote());
     let source_metadata =
         fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
 
-    let is_explicit = matches!(attributes.mode, Preserve::No { explicit: true });
+    let mode_explicitly_disabled = matches!(attributes.mode, Preserve::No { explicit: true });
 
     // preserve is true by default if the destination is created by us and it's a directory
-    let mode = if !is_explicit && dest.is_dir() && created {
+    let mode = if !mode_explicitly_disabled && dest_is_freshly_created_dir {
         Preserve::Yes { required: false }
     } else {
         attributes.mode