-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Properly match the rules "**/" and "!**/" #652
Conversation
I think the build failure here on macOS is spurious. The same test passes on my own machine (running 10.12.6, while the build machine is 10.11.6), and the test that failed is one that is supposed to panic but failed with a |
When processing a rule that ends in a slash, we strip it off and set the `is_only_dir` flag. We then apply the rule that paths that aren't absolute should be given an implicit `**/` prefix, while avoiding adding that prefix if it already exists. However, this means that we miss the case in which we had already stripped off the trailing slash and set `is_only_dir`. Correct this by also explicitly checking for that case. Fixes BurntSushi#649
f2ba972
to
fa4dd34
Compare
Huh, failed twice the exact same way, which I can't reproduce here with the exact same nightly compiler, though a different kernel. That's strange. |
That's... strange. I just kicked it again. Looks like a great fix, thank you. :-) |
Damn, defeated again. I'm not sure what's going on. I'm at work so I can't really dig into this at the moment, but I would at least like to try to get to the bottom of this before merging. (I can't imagine how this PR is causing this error though!) |
Yeah, it's really strange, I also don't see how this change could have caused this error, but it makes sense to get it worked out before merging since you do need to keep the CI working happily. I might try spinning up a 10.11 VM this evening and see if I can repro the issue. |
The same CI problem is happening with other PRs, so it's clearly not provoked by this specific PR. Therefore, I shall merge! Thanks again @lambda! |
Wonder if the CI problem was rust-lang/rust#45866? |
@lambda Ah yup, I believe it is! That particular test is also a |
When processing a rule that ends in a slash, we strip it off and set the
is_only_dir
flag. We then apply the rule that paths that aren'tabsolute should be given an implicit
**/
prefix, while avoidingadding that prefix if it already exists.
However, this means that we miss the case in which we had already
stripped off the trailing slash and set
is_only_dir
. Correct thisby also explicitly checking for that case.
Fixes #649