Skip to content

Commit 013a385

Browse files
authored
feat(cp): optimize directory copy by caching file checks and refactoring calls (#8805)
* feat: optimize directory copy by caching file checks and refactoring calls - Add `target_is_file` field to Context to avoid repeated `stat` calls on target - Refactor `copy_direntry` to accept `&Entry` and additional bool params for symlink/directory handling - Use cached value and local variables for cleaner access to entry properties - Pass `created_parent_dirs` to `copy_file` for improved directory creation tracking This reduces filesystem overhead in cp operations when copying directories.
1 parent 94b6544 commit 013a385

File tree

2 files changed

+115
-52
lines changed

2 files changed

+115
-52
lines changed

src/uu/cp/src/copydir.rs

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ struct Context<'a> {
9999
/// The target path to which the directory will be copied.
100100
target: &'a Path,
101101

102+
/// Whether the target is an existing file. Cached to avoid repeated `stat` calls.
103+
target_is_file: bool,
104+
102105
/// The source path from which the directory will be copied.
103106
root: &'a Path,
104107
}
@@ -107,6 +110,7 @@ impl<'a> Context<'a> {
107110
fn new(root: &'a Path, target: &'a Path) -> io::Result<Self> {
108111
let current_dir = env::current_dir()?;
109112
let root_path = current_dir.join(root);
113+
let target_is_file = target.is_file();
110114
let root_parent = if target.exists() && !root.to_str().unwrap().ends_with("/.") {
111115
root_path.parent().map(|p| p.to_path_buf())
112116
} else if root == Path::new(".") && target.is_dir() {
@@ -122,6 +126,7 @@ impl<'a> Context<'a> {
122126
current_dir,
123127
root_parent,
124128
target,
129+
target_is_file,
125130
root,
126131
})
127132
}
@@ -213,7 +218,7 @@ impl Entry {
213218
}
214219

215220
let local_to_target = context.target.join(descendant);
216-
let target_is_file = context.target.is_file();
221+
let target_is_file = context.target_is_file;
217222
Ok(Self {
218223
source_absolute,
219224
source_relative,
@@ -227,54 +232,73 @@ impl Entry {
227232
/// Copy a single entry during a directory traversal.
228233
fn copy_direntry(
229234
progress_bar: Option<&ProgressBar>,
230-
entry: Entry,
235+
entry: &Entry,
236+
entry_is_symlink: bool,
237+
entry_is_dir_no_follow: bool,
231238
options: &Options,
232239
symlinked_files: &mut HashSet<FileInformation>,
233240
preserve_hard_links: bool,
234241
copied_destinations: &HashSet<PathBuf>,
235242
copied_files: &mut HashMap<FileInformation, PathBuf>,
243+
created_parent_dirs: &mut HashSet<PathBuf>,
236244
) -> CopyResult<()> {
237-
let Entry {
238-
source_absolute,
239-
source_relative,
240-
local_to_target,
241-
target_is_file,
242-
} = entry;
245+
let source_is_symlink = entry_is_symlink;
246+
let source_is_dir = if source_is_symlink && !options.dereference {
247+
false
248+
} else if source_is_symlink {
249+
entry.source_absolute.is_dir()
250+
} else {
251+
entry_is_dir_no_follow
252+
};
243253

244254
// If the source is a symbolic link and the options tell us not to
245255
// dereference the link, then copy the link object itself.
246-
if source_absolute.is_symlink() && !options.dereference {
247-
return copy_link(&source_absolute, &local_to_target, symlinked_files, options);
256+
if source_is_symlink && !options.dereference {
257+
return copy_link(
258+
&entry.source_absolute,
259+
&entry.local_to_target,
260+
symlinked_files,
261+
options,
262+
);
248263
}
249264

250265
// If the source is a directory and the destination does not
251266
// exist, ...
252-
if source_absolute.is_dir() && !local_to_target.exists() {
253-
return if target_is_file {
267+
if source_is_dir && !entry.local_to_target.exists() {
268+
return if entry.target_is_file {
254269
Err(translate!("cp-error-cannot-overwrite-non-directory-with-directory").into())
255270
} else {
256-
build_dir(&local_to_target, false, options, Some(&source_absolute))?;
271+
build_dir(
272+
&entry.local_to_target,
273+
false,
274+
options,
275+
Some(&entry.source_absolute),
276+
)?;
257277
if options.verbose {
258-
println!("{}", context_for(&source_relative, &local_to_target));
278+
println!(
279+
"{}",
280+
context_for(&entry.source_relative, &entry.local_to_target)
281+
);
259282
}
260283
Ok(())
261284
};
262285
}
263286

264287
// If the source is not a directory, then we need to copy the file.
265-
if !source_absolute.is_dir() {
288+
if !source_is_dir {
266289
if let Err(err) = copy_file(
267290
progress_bar,
268-
&source_absolute,
269-
local_to_target.as_path(),
291+
&entry.source_absolute,
292+
entry.local_to_target.as_path(),
270293
options,
271294
symlinked_files,
272295
copied_destinations,
273296
copied_files,
297+
created_parent_dirs,
274298
false,
275299
) {
276300
if preserve_hard_links {
277-
if !source_absolute.is_symlink() {
301+
if !source_is_symlink {
278302
return Err(err);
279303
}
280304
// silent the error with a symlink
@@ -292,7 +316,10 @@ fn copy_direntry(
292316
show!(uio_error!(
293317
e,
294318
"{}",
295-
translate!("cp-error-cannot-open-for-reading", "source" => source_relative.quote()),
319+
translate!(
320+
"cp-error-cannot-open-for-reading",
321+
"source" => entry.source_relative.quote()
322+
),
296323
));
297324
}
298325
e => return Err(e),
@@ -320,6 +347,7 @@ pub(crate) fn copy_directory(
320347
symlinked_files: &mut HashSet<FileInformation>,
321348
copied_destinations: &HashSet<PathBuf>,
322349
copied_files: &mut HashMap<FileInformation, PathBuf>,
350+
created_parent_dirs: &mut HashSet<PathBuf>,
323351
source_in_command_line: bool,
324352
) -> CopyResult<()> {
325353
// if no-dereference is enabled and this is a symlink, copy it as a file
@@ -332,6 +360,7 @@ pub(crate) fn copy_directory(
332360
symlinked_files,
333361
copied_destinations,
334362
copied_files,
363+
created_parent_dirs,
335364
source_in_command_line,
336365
);
337366
}
@@ -405,16 +434,29 @@ pub(crate) fn copy_directory(
405434
{
406435
match direntry_result {
407436
Ok(direntry) => {
408-
let entry = Entry::new(&context, direntry.path(), options.no_target_dir)?;
437+
let direntry_type = direntry.file_type();
438+
let direntry_path = direntry.path();
439+
let (entry_is_symlink, entry_is_dir_no_follow) =
440+
match direntry_path.symlink_metadata() {
441+
Ok(metadata) => {
442+
let file_type = metadata.file_type();
443+
(file_type.is_symlink(), file_type.is_dir())
444+
}
445+
Err(_) => (direntry_type.is_symlink(), direntry_type.is_dir()),
446+
};
447+
let entry = Entry::new(&context, direntry_path, options.no_target_dir)?;
409448

410449
copy_direntry(
411450
progress_bar,
412-
entry,
451+
&entry,
452+
entry_is_symlink,
453+
entry_is_dir_no_follow,
413454
options,
414455
symlinked_files,
415456
preserve_hard_links,
416457
copied_destinations,
417458
copied_files,
459+
created_parent_dirs,
418460
)?;
419461

420462
// We omit certain permissions when creating directories
@@ -428,19 +470,17 @@ pub(crate) fn copy_directory(
428470
// (Note that there can be more than one! We might step out of
429471
// `./a/b/c` into `./a/`, in which case we'll need to fix the
430472
// permissions of both `./a/b/c` and `./a/b`, in that order.)
431-
if direntry.file_type().is_dir() {
473+
let is_dir_for_permissions =
474+
entry_is_dir_no_follow || (options.dereference && direntry_path.is_dir());
475+
if is_dir_for_permissions {
432476
// Add this directory to our list for permission fixing later
433-
let entry_for_tracking =
434-
Entry::new(&context, direntry.path(), options.no_target_dir)?;
435-
dirs_needing_permissions.push((
436-
entry_for_tracking.source_absolute,
437-
entry_for_tracking.local_to_target,
438-
));
477+
dirs_needing_permissions
478+
.push((entry.source_absolute.clone(), entry.local_to_target.clone()));
439479

440480
// If true, last_iter is not a parent of this iter.
441481
// The means we just exited a directory.
442482
let went_up = if let Some(last_iter) = &last_iter {
443-
last_iter.path().strip_prefix(direntry.path()).is_ok()
483+
last_iter.path().strip_prefix(direntry_path).is_ok()
444484
} else {
445485
false
446486
};
@@ -454,14 +494,14 @@ pub(crate) fn copy_directory(
454494
//
455495
// All the unwraps() here are unreachable.
456496
let last_iter = last_iter.as_ref().unwrap();
457-
let diff = last_iter.path().strip_prefix(direntry.path()).unwrap();
497+
let diff = last_iter.path().strip_prefix(direntry_path).unwrap();
458498

459499
// Fix permissions for every entry in `diff`, inside-out.
460500
// We skip the last directory (which will be `.`) because
461501
// its permissions will be fixed when we walk _out_ of it.
462502
// (at this point, we might not be done copying `.`!)
463503
for p in skip_last(diff.ancestors()) {
464-
let src = direntry.path().join(p);
504+
let src = direntry_path.join(p);
465505
let entry = Entry::new(&context, &src, options.no_target_dir)?;
466506

467507
copy_attributes(

0 commit comments

Comments
 (0)