Fix Dir.glob ../ and cases#8542
Conversation
|
I am reluctant to revert that fix without some input from @edipofederle on what exactly it fixed. Punting to 9.4.11.0 so we can discuss further. |
|
Looking at the logic they added, it just queried the filesystem for the actual path and replaced the glob result with that, taking into account paths starting with The idea wasn't to merely just revert the change, though. I'm also still trying to find a way to do it on the fly, however it's proving to be quite tricky since the glob logic is so convoluted. I may still just try rework the original fix to better take into account |
danini-the-panini
force-pushed
the
ds-8426-fix-glob
branch
from
1b4eb94 to
cf0e5a7
Compare
danini-the-panini
force-pushed
the
ds-8426-fix-glob
branch
from
9956c9c to
a190b15
Compare
danini-the-panini
force-pushed
the
ds-8426-fix-glob
branch
from
155302a to
ea49a09
Compare
| final int end = begin + path.getRealSize(); | ||
| final int end = begin + path.length(); |
There was a problem hiding this comment.
i'm not sure if this changes anything. looks like length just returns realSize :/
There was a problem hiding this comment.
@danini-the-panini length() is there to satisfy Java CharSequence. It is fine to leave it since it will always match byte length but it is a weird method since it implies it should return char length.
There was a problem hiding this comment.
We have discussed in the past modifying the CharSequence methods of ByteList to actually work against the contained characters rather than just the bytes but at this point it's so entrenched we'd have to break the world to do it. I think this is fine.
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
I think it is impossible to trace through this and know you are prepending scheme in all cases but that is the nature of this code. It is very complicated.
I see no new tests but I do wonder if you noticed any cases which we are lacking coverage?
|
@enebo there aren't really any new test cases, the case I sensitive one was enabled by the previous fix which I reverted. The only other thing to test would be the ../ case but it's quite niche. |
headius
left a comment
headius
left a comment
There was a problem hiding this comment.
Complicated logic being patched here but I like the changes and the move to an enum for the three-mode magic glob behavior. Minor recommendations to address but otherwise this is good to go.
| final int end = begin + path.getRealSize(); | ||
| final int end = begin + path.length(); |
There was a problem hiding this comment.
We have discussed in the past modifying the CharSequence methods of ByteList to actually work against the contained characters rather than just the bytes but at this point it's so entrenched we'd have to break the world to do it. I think this is fine.
|
I approve of merging this for 9.4.11.0, or punting to 9.4.12.0. |
3 participants
FNM_CASEFOLDoption to Dir.glob, to match MRI.has_magicto return an enum instead of a boolean.This distinguishes paths with "alpha" chars, which get treated as magical in case-insensitive filesystems.
Forces each path segment to be fetched from the filesystem in order to do a
fnmatch, even if it doesn't contain any magic glob patterns.FNM_DOTMATCHfunctionality matches MRI.