Skip to content

Refactor path validation and optimize performance #18

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

Closed
ppmpreetham wants to merge 3 commits into from
Closed

Refactor path validation and optimize performance #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f127de8
refactor: remove redundant dest path validation
ppmpreetham Jun 4, 2026
cf7e468
perf: remove vec allocation
ppmpreetham Jun 4, 2026
e014d6b
perf: matches!() is a bit faster
ppmpreetham Jun 4, 2026
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
80 changes: 44 additions & 36 deletions crates/core/src/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,47 +6,55 @@ pub(crate) struct CopyFilter;

impl CopyFilter {
pub(crate) fn excludes(self, path: &Path) -> bool {
let parts = path
.components()
.filter_map(|component| match component {
Component::Normal(part) => Some(part),
_ => None,
})
.collect::<Vec<_>>();
let mut parts = path.components().filter_map(|component| match component {
Component::Normal(part) => Some(part),
_ => None,
});

parts.iter().any(|part| excludes_component(part))
|| parts
.windows(2)
.any(|parts| matches_yarn_artifact(parts[0], parts[1]))
let first = match parts.next() {
Some(p) => p,
None => return false,
};

if excludes_component(first) {
return true;
}

let mut prev = first;
parts.any(|part| {
let result = excludes_component(part) || matches_yarn_artifact(prev, part);
prev = part;
result
})
Comment on lines +9 to +28

@nexxeln nexxeln Jun 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prolly not meaningful difference in perf so i'd prefer to keep the current pretty impl

}
}

fn excludes_component(part: &OsStr) -> bool {
[
"node_modules",
".pnpm-store",
"target",
".venv",
"venv",
".tox",
".nox",
"__pycache__",
".pytest_cache",
".mypy_cache",
".ruff_cache",
".next",
".nuxt",
".svelte-kit",
".turbo",
".vite",
".parcel-cache",
".cache",
"dist",
"build",
"coverage",
]
.into_iter()
.any(|excluded| part == excluded)
let Some(s) = part.to_str() else { return false };
matches!(
s,
"node_modules"
| "target"
| ".pnpm-store"
| ".venv"
| "venv"
| ".tox"
| ".nox"
| "__pycache__"
| ".pytest_cache"
| ".mypy_cache"
| ".ruff_cache"
| ".next"
| ".nuxt"
| ".svelte-kit"
| ".turbo"
| ".vite"
| ".parcel-cache"
| ".cache"
| "dist"
| "build"
| "coverage"
)
Comment on lines +33 to +57

@nexxeln nexxeln Jun 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here

}

fn matches_yarn_artifact(first: &OsStr, second: &OsStr) -> bool {
Expand Down
3 changes: 0 additions & 3 deletions crates/core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,6 @@ impl Manager {
None => default_storage(&root.path)?,
};
let name = RiftName::from_optional(input.name)?;
if destination_parent.join(name.as_str()).starts_with(&from) {

@nexxeln nexxeln Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should keep the early check that stops copying into the source itself
the later check still blocks these destinations, so removing this doesn’t let us copy anything new. it only means we create the destination folder first and fail after that, which can leave a new folder behind inside the source when the request is invalid.

return Err(Error::InsideSource(destination_parent.join(name.as_str())));
}
fs::create_dir_all(&destination_parent)?;
let destination_parent = fs::canonicalize(destination_parent)?;
let destination = destination_parent.join(name.as_str());
Expand Down
Loading