Skip to content

Don't completely skip files whose basenames aren't UTF-8#21

Merged
cgwalters merged 1 commit intocoreos:mainfrom
nabijaczleweli:utf9
Feb 3, 2023
Merged

Don't completely skip files whose basenames aren't UTF-8#21
cgwalters merged 1 commit intocoreos:mainfrom
nabijaczleweli:utf9

Conversation

@nabijaczleweli
Copy link
Collaborator

@nabijaczleweli nabijaczleweli commented Feb 3, 2023

It's not as beautiful as it could be if Rust had string manipulation of
OsStr but it's still zero-alloc in the vast majority of cases

API change: scan() -> BTreeMap<String, PathBuf> to scan() -> BTreeMap<OsString, PathBuf>

Draft because it's on top of #20, patch 2 is the interesting one

@cgwalters cgwalters added the semver-break Change which requires a semver bump label Feb 3, 2023
@nabijaczleweli nabijaczleweli force-pushed the utf9 branch 2 times, most recently from dacf9c3 to d3d99f2 Compare February 3, 2023 17:51
It's not as beautiful as it could be if Rust had string manipulation of
OsStr but it's still zero-alloc in the vast majority of cases

API change: scan() -> BTreeMap<String, PathBuf>
            to
            scan() -> BTreeMap<OsString, PathBuf>
@nabijaczleweli nabijaczleweli marked this pull request as ready for review February 3, 2023 17:56
@nabijaczleweli
Copy link
Collaborator Author

And so is this


// If hidden files not allowed, ignore dotfiles.
if ignore_dotfiles && fname.starts_with('.') {
// Rust RFC 900 &c.: there's no way to check if a Path/OsStr starts with a prefix;
Copy link
Member

Choose a reason for hiding this comment

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

We can on Unix at least just compare the first bytes using https://doc.rust-lang.org/std/os/unix/ffi/trait.OsStrExt.html#tymethod.as_bytes, i.e. under some #cfg[(unix)] or so. I'd honestly be a bit surprised if we ever had users on non-Unix platforms (but, I wouldn't say we should gratuitously not compile there - at least with Rust it's ergonomic to do conditional compilation and most high level abstractions peel away at compile time too).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered this, but nothing else in the crate uses a system-specific API, so I went with this which decays graciously I think.

@cgwalters cgwalters merged commit c2b8ca2 into coreos:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-break Change which requires a semver bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants