Replace std::os::glob with extra::glob, which is written in rust ... #8201

Dretch · 2013-08-01T22:54:04Z

... fixing issue #6100. This also addresses #6101.

huonw · 2013-08-01T23:01:22Z

How does one match a literal ]?

lilyball · 2013-08-01T23:40:46Z

src/libextra/glob.rs

+            '[' => {
+                let mut chars = ~[];
+                let is_except = match pattern_iter.next() {
+                    None => false, // let the following loop fail with a message


Unless things have changed in the past 2 weeks, the iterator contract does not require that an iterator that returns None once continue to return None forever. In fact, the iterator contract specifies nothing at all about the behavior after None has been returned once.

What you state appears to be true. Maybe the Iterator docs should be more explicit about this situation. I can rework the glob code to avoid the issue.

@Dretch After discussion with @thestinger it sounds like he wants the iterator protocol to actually state that once None is returned, future calls to next() will continue to return None.

lilyball · 2013-08-01T23:47:51Z

I'd love to see a public function equivalent to fnmatch(), not just globbing. This would basically mean making a public named type for ~[PatternToken] and making pattern_matches() public (and probably renaming it too).

lilyball · 2013-08-01T23:50:53Z

Also, your implementation of the glob is inefficient when it hits AnySequence. It loops until the end of the string, trying to match at each position. This is fine for patterns that only have a single *, but any pattern that has multiple * will perform badly.

Instead, you can split your pattern on all instances of * (and coalesce them too.. foo**bar is equivalent to foo*bar) and match each sub-pattern in sequence. Once you find a match for a subpattern, never backtrack. Because * matches anything, if a subpattern matches at one location, but the overall pattern does not, trying to re-match that subpattern at a later location will never cause the overall pattern to match.

omasanori · 2013-08-02T00:04:21Z

src/libextra/glob.rs

+    pattern
+}
+
+fn pattern_matches(mut file: &str, pattern: &[PatternToken]) -> bool {


It seems that pattern_matches lacks some considerations required by Patterns Used for Filename Expansion in SUSv4.
For example, foo/.bar shall not be matched by the pattern foo/*.

Fair enough. Special behaviour for dot-files would at least be consistent with the python glob module.

Dretch · 2013-08-02T19:27:29Z

@huonw

Literal ] can be matched by []]

@kballard

Regarding fnmatch(): how about introducing a Pattern struct with a constructor that takes &str and a match(pattern: &str) method, and hiding the ~[PatternToken] inside of it?

I will also try and have a look at making * matching more efficient.

lilyball · 2013-08-03T04:37:46Z

@Dretch: fnmatch() in C has a few options. The ones that I think would be useful to provide here are FNM_PATHNAME (don't match / with * or ?, the pattern needs an explicit /), FNM_PERIOD (a leading period in a path component (if FNM_PATHNAME is set) or in the string must be matched by an explicit period in the pattern), and FNM_CASEFOLD (case insensitivity).

If you can make a match(pattern: &str, opts: something) function that provides those options in some fashion, that would be ideal.

omasanori · 2013-08-03T06:27:53Z

Note that FNM_PATHNAME | FNM_PERIOD is the glob's behavior.

@kballard there is also FNM_NOESCAPE.

lilyball · 2013-08-03T06:32:38Z

@omasanori There's also FNM_LEADING_DIR, but I don't think those two options are particularly useful.

there was no way to match NOT ']'). Also do some refactoring to avoid relying on the vector iterator always returning None after returning None for the first time (as requested by @kballard).

omasanori · 2013-08-05T08:50:03Z

@kballard I agree with you. (FNM_NOESCAPE might be used in Windows which uses \ as path separator, but I'm unsure.)

method that acts something like the C fnmatch function (albeit without any options... yet).

…code.

and Pattern::matches, and provide a case_sensitive option that defaults to true.

to the FNM_PATHNAME flag in the libc fnmatch function

catamorphism · 2013-08-17T04:05:55Z

@Dretch Needs a rebase.

catamorphism · 2013-08-21T21:07:47Z

Closing due to lack of activity. Please reopen if you have time to rebase it!

Dretch · 2013-08-21T21:27:13Z

I am still working on this (albeit slowly)! I have some more commits coming (including a rebase).

I can't see any way to reopen this pull request - should I start a new one?

This is #8201 with a bunch of amendments to address the comments (and re-based).

Change `unnecessary_to_owned` `into_iter` suggestions to `MaybeIncorrect` I am having a hard time finding a good solution for rust-lang#8148, so I am wondering if is enough to just change the suggestion's applicability to `MaybeIncorrect`? I apologize, as I realize this is a bit of a cop out. changelog: none

Replace os::glob with extra::glob, which is written in rust,

8a5a08e

fixing issue rust-lang#6100.

lilyball reviewed Aug 1, 2013
View reviewed changes

omasanori reviewed Aug 2, 2013
View reviewed changes

Allow NOT ']' to be matched with the pattern '[!]]' (prior to this

60bc719

there was no way to match NOT ']'). Also do some refactoring to avoid relying on the vector iterator always returning None after returning None for the first time (as requested by @kballard).

Dretch added 5 commits August 5, 2013 19:20

Refactor extra::glob so that it now exposes a Pattern::matches

bbabc8d

method that acts something like the C fnmatch function (albeit without any options... yet).

Add Pattern::escape(&str) to help constructing dynamic pattern strings.

b0648a0

Use util::unreachable() instead of fail!() in aid of self-describing …

149ab38

…code.

Allow matching options to be specified when calling glob

b75e005

and Pattern::matches, and provide a case_sensitive option that defaults to true.

Add a require_literal_separator match option that is equivalent

006b5e0

to the FNM_PATHNAME flag in the libc fnmatch function

catamorphism closed this Aug 21, 2013

Dretch mentioned this pull request Sep 1, 2013

Replace std::os::glob with extra::glob, which is written in rust #8914

Closed

bors added a commit that referenced this pull request Sep 6, 2013

auto merge of #8914 : Dretch/rust/native-glob, r=alexcrichton

5efe1e5

This is #8201 with a bunch of amendments to address the comments (and re-based).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace std::os::glob with extra::glob, which is written in rust ... #8201

Replace std::os::glob with extra::glob, which is written in rust ... #8201

Dretch commented Aug 1, 2013

huonw commented Aug 1, 2013

lilyball Aug 1, 2013

Dretch Aug 2, 2013

lilyball Aug 3, 2013

lilyball commented Aug 1, 2013

lilyball commented Aug 1, 2013

omasanori Aug 2, 2013

Dretch Aug 2, 2013

Dretch commented Aug 2, 2013

lilyball commented Aug 3, 2013

omasanori commented Aug 3, 2013

lilyball commented Aug 3, 2013

omasanori commented Aug 5, 2013

catamorphism commented Aug 17, 2013

catamorphism commented Aug 21, 2013

Dretch commented Aug 21, 2013

Replace std::os::glob with extra::glob, which is written in rust ... #8201

Replace std::os::glob with extra::glob, which is written in rust ... #8201

Conversation

Dretch commented Aug 1, 2013

huonw commented Aug 1, 2013

lilyball Aug 1, 2013

Choose a reason for hiding this comment

Dretch Aug 2, 2013

Choose a reason for hiding this comment

lilyball Aug 3, 2013

Choose a reason for hiding this comment

lilyball commented Aug 1, 2013

lilyball commented Aug 1, 2013

omasanori Aug 2, 2013

Choose a reason for hiding this comment

Dretch Aug 2, 2013

Choose a reason for hiding this comment

Dretch commented Aug 2, 2013

lilyball commented Aug 3, 2013

omasanori commented Aug 3, 2013

lilyball commented Aug 3, 2013

omasanori commented Aug 5, 2013

catamorphism commented Aug 17, 2013

catamorphism commented Aug 21, 2013

Dretch commented Aug 21, 2013