cp: efficient permission fixup

src/uu/cp/src/copydir.rs

@@ -79,6 +79,12 @@ fn get_local_to_root_parent(
     }
 }
 
+/// Given an iterator, return all its items except the last.
+fn skip_last<T>(mut iter: impl Iterator<Item = T>) -> impl Iterator<Item = T> {
+    let last = iter.next();
+    iter.scan(last, |state, item| std::mem::replace(state, Some(item)))
+}
+
 /// Paths that are invariant throughout the traversal when copying a directory.
 struct Context<'a> {
     /// The current working directory at the time of starting the traversal.
@@ -162,17 +168,18 @@ struct Entry {
 }
 
 impl Entry {
-    fn new(
+    fn new<A: AsRef<Path>>(
         context: &Context,
-        direntry: &DirEntry,
+        source: A,
         no_target_dir: bool,
     ) -> Result<Self, StripPrefixError> {
-        let source_relative = direntry.path().to_path_buf();
+        let source = source.as_ref();
+        let source_relative = source.to_path_buf();
         let source_absolute = context.current_dir.join(&source_relative);
         let mut descendant =
             get_local_to_root_parent(&source_absolute, context.root_parent.as_deref())?;
         if no_target_dir {
-            let source_is_dir = direntry.path().is_dir();
+            let source_is_dir = source.is_dir();
             if path_ends_with_terminator(context.target) && source_is_dir {
                 if let Err(e) = std::fs::create_dir_all(context.target) {
                     eprintln!("Failed to create directory: {e}");
@@ -213,6 +220,7 @@ where
     // `path.ends_with(".")` does not seem to work
     path.as_ref().display().to_string().ends_with("/.")
 }
+
 #[allow(clippy::too_many_arguments)]
 /// Copy a single entry during a directory traversal.
 fn copy_direntry(
@@ -223,7 +231,6 @@ fn copy_direntry(
     preserve_hard_links: bool,
     copied_destinations: &HashSet<PathBuf>,
     copied_files: &mut HashMap<FileInformation, PathBuf>,
-    dirs_with_attrs_to_fix: &mut Vec<(PathBuf, PathBuf)>,
 ) -> CopyResult<()> {
     let Entry {
         source_absolute,
@@ -251,10 +258,6 @@ fn copy_direntry(
             if options.verbose {
                 println!("{}", context_for(&source_relative, &local_to_target));
             }
-
-            // `build_dir` doesn't set fully set attributes,
-            // we'll need to fix them later.
-            dirs_with_attrs_to_fix.push((source_absolute, local_to_target));
             return Ok(());
         }
     }
@@ -408,15 +411,8 @@ pub(crate) fn copy_directory(
         Err(e) => return Err(format!("failed to get current directory {e}").into()),
     };
 
-    // We omit certain permissions when creating dirs
-    // to prevent other uses from accessing them before they're done
-    // (race condition).
-    //
-    // As such, we need to go back through the dirs we copied and
-    // fix these permissions.
-    //
-    // This is a vec of (old_path, new_path)
-    let mut dirs_with_attrs_to_fix: Vec<(PathBuf, PathBuf)> = Vec::new();
+    // The directory we were in during the previous iteration
+    let mut last_iter: Option<DirEntry> = None;
 
     // Traverse the contents of the directory, copying each one.
     for direntry_result in WalkDir::new(root)
@@ -425,7 +421,8 @@ pub(crate) fn copy_directory(
     {
         match direntry_result {
             Ok(direntry) => {
-                let entry = Entry::new(&context, &direntry, options.no_target_dir)?;
+                let entry = Entry::new(&context, direntry.path(), options.no_target_dir)?;
+
                 copy_direntry(
                     progress_bar,
                     entry,
@@ -434,20 +431,87 @@ pub(crate) fn copy_directory(
                     preserve_hard_links,
                     copied_destinations,
                     copied_files,
-                    &mut dirs_with_attrs_to_fix,
                 )?;
+
+                // We omit certain permissions when creating directories
+                // to prevent other users from accessing them before they're done.
+                // We thus need to fix the permissions of each directory we copy
+                // once it's contents are ready.
+                // This "fixup" is implemented here in a memory-efficient manner.
+                //
+                // We detect iterations where we "walk up" the directory tree,
+                // and fix permissions on all the directories we exited.
+                // (Note that there can be more than one! We might step out of
+                // `./a/b/c` into `./a/`, in which case we'll need to fix the
+                // permissions of both `./a/b/c` and `./a/b`, in that order.)
+                if direntry.file_type().is_dir() {
+                    // If true, last_iter is not a parent of this iter.
+                    // The means we just exited a directory.
+                    let went_up = if let Some(last_iter) = &last_iter {
+                        direntry.path().strip_prefix(last_iter.path()).is_err()
+                    } else {
+                        false
+                    };
+
+                    if went_up {
+                        // Compute the "difference" between `last_iter` and `direntry`.
+                        // For example, if...
+                        // - last_iter = `a/b/c/d`
+                        // - direntry = `a/b`
+                        // then diff = `c/d`
+                        //
+                        // All the unwraps() here are unreachable.
+                        let last_iter = last_iter.as_ref().unwrap();
+                        let diff = last_iter.path().strip_prefix(direntry.path()).unwrap();
+
+                        // Fix permissions for every entry in `diff`, inside-out.
+                        // We skip the last directory (which will be `.`) because
+                        // its permissions will be fixed when we walk _out_ of it.
+                        // (at this point, we might not be done copying `.`!)
+                        for p in skip_last(diff.ancestors()) {
+                            let src = direntry.path().join(p);
+                            let entry = Entry::new(&context, &src, options.no_target_dir)?;
+
+                            copy_attributes(
+                                &entry.source_absolute,
+                                &entry.local_to_target,
+                                &options.attributes,
+                            )?;
+                        }
+                    }
+
+                    last_iter = Some(direntry);
+                }
             }
+
             // Print an error message, but continue traversing the directory.
             Err(e) => show_error!("{}", e),
         }
     }
 
-    // Fix permissions for all directories we created
-    for (src, tgt) in dirs_with_attrs_to_fix {
-        copy_attributes(&src, &tgt, &options.attributes)?;
+    // Handle final directory permission fixes.
+    // This is almost the same as the permission-fixing code above,
+    // with minor differences (commented)
+    if let Some(last_iter) = last_iter {
+        let diff = last_iter.path().strip_prefix(root).unwrap();
+
+        // Do _not_ skip `.` this time, since we know we're done.
+        // This is where we fix the permissions of the top-level
+        // directory we just copied.
+        for p in diff.ancestors() {
+            let src = root.join(p);
+            let entry = Entry::new(&context, &src, options.no_target_dir)?;
+
+            copy_attributes(
+                &entry.source_absolute,
+                &entry.local_to_target,
+                &options.attributes,
+            )?;
+        }
     }
 
-    // Copy the attributes from the root directory to the target directory.
+    // Also fix permissions for parent directories,
+    // if we were asked to create them.
     if options.parents {
         let dest = target.join(root.file_name().unwrap());
         for (x, y) in aligned_ancestors(root, dest.as_path()) {