Skip to content
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.

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

Avoid unwrap() if non UTF-8 in the require_literal_leading_dot flow #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jongy
Copy link

@Jongy Jongy commented Feb 3, 2025

Drop the path if UTF-8 validation fails and is required for string comparison, instead of crashing.
Add a test exhibiting the problem (fixed with this change).

Note that with require_literal_leading_dot: false, I could get results that have non utf-8 path components; with require_literal_leading_dot: true it panics without the patch, and filters those paths with it (I think that it makes more sense to filter out paths that we're unsure if should be filtered in).

An alternative is to use bytes comparison:

diff --git a/src/lib.rs b/src/lib.rs
index 8428a9a..ded971e 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -943,8 +943,13 @@ fn fill_todo(
             match dirs {
                 Ok(mut children) => {
                     if options.require_literal_leading_dot {
-                        children
-                            .retain(|x| !x.file_name().unwrap().to_str().unwrap().starts_with('.'));
+                        children.retain(|x| {
+                            !x.file_name()
+                                .unwrap()
+                                .as_encoded_bytes()
+                                .first()
+                                .is_some_and(|&b| b == b'.')
+                        });
                     }
                     children.sort_by(|p1, p2| p2.file_name().cmp(&p1.file_name()));
                     todo.extend(children.into_iter().map(|x| Ok((x, idx))));

but I think that it's not a good idea to add half-done utf-8 handling. Instead, let's wait for proper support across the crate.

Btw, the rust issue rust-lang/rust#9639 is referenced around here - perhaps we should create a tracking issue in this repository and update the references?

Drop the path if UTF-8 validation fails and is required for string comparison,
instead of panicing.
Add a test exhibiting the problem (fixed with this change).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant