-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add parse_glob
module and update du
to use parse_glob
#3754
Conversation
Any tips on how to debug the new test failure in I did
on my machine (linux) and it was fine. |
@ackerleytng That's very curious. Maybe you could download the logs from the CI and look at those? If it's a CI only issue we might have to ignore it or fix the CI somehow. Edit: Oh I think that test just has some intermittent failures. I'll rerun it. |
I diffed the this test run with an earlier working one and nothing stood out, other than
This test
Do we know if these tests are sufficiently independent of each other? |
I just noticed this: https://github.com/uutils/coreutils/blob/main/util/build-gnu.sh#L171 Since this test uses more than one process and is timing dependent, is it something to do with the hardware (performance) of the CI machine? |
Yeah that looks like something we should fix outside this PR |
Feel free to merge this if you're okay with fixing this separately! |
Yeah!
|
let mut inodes: HashSet<FileInfo> = HashSet::new(); | ||
if let Some(inode) = stat.inode { | ||
inodes.insert(inode); | ||
if let Ok(stat) = Stat::new(path, &options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment explaining what this block is doing? It isn't super obvious reading the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see newest changes!
@@ -745,6 +745,27 @@ fn test_du_exclude_mix() { | |||
assert!(!result.stdout_str().contains("qzerty")); | |||
assert!(!result.stdout_str().contains("azeaze")); | |||
assert!(result.stdout_str().contains("xcwww")); | |||
|
|||
// Negation in glob should work with both ^ and ! | |||
let result = ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe create a new test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
de02ebb
to
6aa9aa1
Compare
Fixes #3628
The
parse_glob
module uses the same logic implemented inrust-lang/glob
: https://github.com/rust-lang/glob/blob/master/src/lib.rs#L613 to identify negation in globs. When this pattern is identified, the^
in the pattern is replaced with!
and glob'sPattern
is built from the string.A disadvantage of this approach is that inconsistencies may result, if this pattern identification method diverges, when rust-lang/glob's parsing of character set negation changes.
However, I think this approach avoids having to maintain or rebuild glob parsing logic, and is the best option I could think of given that
rust-lang/glob
's implementation is not open to extension to support multiple character set negation markers.With the
parse_glob
module, fixingdu
is just a matter of using the new module.GNU
ls
anddircolors
also usefnmatch
. Those two utilities will be updated to useparse_glob
in separate PRs.