Skip to content

Reduce allocations in fs::copy_files_except_ext #2355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Reduce allocations in fs::copy_files_except_ext #2355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 19 additions & 52 deletions src/utils/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,12 @@ pub fn create_file(path: &Path) -> Result<File> {

/// Removes all the content of a directory but not the directory itself
pub fn remove_dir_content(dir: &Path) -> Result<()> {
for item in fs::read_dir(dir)? {
if let Ok(item) = item {
let item = item.path();
if item.is_dir() {
fs::remove_dir_all(item)?;
} else {
fs::remove_file(item)?;
}
for item in fs::read_dir(dir)?.flatten() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, this is off-topic and just resolves a clippy warning.

let item = item.path();
if item.is_dir() {
fs::remove_dir_all(item)?;
} else {
fs::remove_file(item)?;
}
}
Ok(())
Expand Down Expand Up @@ -108,72 +106,41 @@ pub fn copy_files_except_ext(
}

for entry in fs::read_dir(from)? {
let entry = entry?;
let entry = entry?.path();
let metadata = entry
.path()
.metadata()
.with_context(|| format!("Failed to read {:?}", entry.path()))?;
.with_context(|| format!("Failed to read {entry:?}"))?;

let entry_file_name = entry.file_name().unwrap();
let target_file_path = to.join(entry_file_name);

// If the entry is a dir and the recursive option is enabled, call itself
if metadata.is_dir() && recursive {
if entry.path() == to.to_path_buf() {
if entry == to.as_os_str() {
continue;
}

if let Some(avoid) = avoid_dir {
if entry.path() == *avoid {
if entry == *avoid {
continue;
}
}

// check if output dir already exists
if !to.join(entry.file_name()).exists() {
fs::create_dir(&to.join(entry.file_name()))?;
if !target_file_path.exists() {
fs::create_dir(&target_file_path)?;
}

copy_files_except_ext(
&from.join(entry.file_name()),
&to.join(entry.file_name()),
true,
avoid_dir,
ext_blacklist,
)?;
copy_files_except_ext(&entry, &target_file_path, true, avoid_dir, ext_blacklist)?;
} else if metadata.is_file() {
// Check if it is in the blacklist
if let Some(ext) = entry.path().extension() {
if let Some(ext) = entry.extension() {
if ext_blacklist.contains(&ext.to_str().unwrap()) {
continue;
}
}
debug!(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this trace as it did not really add value, in my opinion. The target file path is traced also below.

"creating path for file: {:?}",
&to.join(
entry
.path()
.file_name()
.expect("a file should have a file name...")
)
);

debug!(
"Copying {:?} to {:?}",
entry.path(),
&to.join(
entry
.path()
.file_name()
.expect("a file should have a file name...")
)
);
copy(
entry.path(),
&to.join(
entry
.path()
.file_name()
.expect("a file should have a file name..."),
),
)?;
debug!("Copying {entry:?} to {target_file_path:?}");
copy(&entry, &target_file_path)?;
}
}
Ok(())
Expand Down