Skip to content
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

Pattern **/ matches everything #83

Open
MartinSavc opened this issue Jul 16, 2019 · 10 comments
Open

Pattern **/ matches everything #83

MartinSavc opened this issue Jul 16, 2019 · 10 comments

Comments

@MartinSavc
Copy link

Hi.

Using glob version 0.3.0.

The pattern **/ should match all folders recursively. However it seems to also match files. Not sure if this is as intended, however in bash **/ works for matching folders only.
My test (fails on path3):

#[test]
pub fn test_pattern_recursive_dirs() {
        let path1 = Path::new("folder/");
        let path2 = Path::new("folder/folder/");
        let path3 = Path::new("folder/folder/file");
        let pattern = Pattern::new("**/").unwrap();
        assert!(pattern.matches_path(path1));
        assert!(pattern.matches_path(path2));
        assert!(!pattern.matches_path(path3));
}

If this is something to be fixed, I can look into it.

@KodrAus
Copy link
Contributor

KodrAus commented Jul 21, 2019

It looks like globset makes the **/ pattern only match directories.

I’m a little uneasy about making behavioural changes to this library though at this stage, even though this seems like the way you’d first expect this to work.

There’s appetite in #59 to replace the internals of this crate with globset which would fix this and many other issues together.

@MartinSavc
Copy link
Author

That makes sense. I have been thinking about further issues with my proposed solution but I haven't put them to the test yet. I will close the issue and pull request.

@KodrAus
Copy link
Contributor

KodrAus commented Jul 24, 2019

How about we keep the issue open because I think it's a legitimate concern, but whether and how we go about solving it is an open question?

@KodrAus KodrAus reopened this Jul 24, 2019
@MartinSavc
Copy link
Author

Ok.
Could tests be added ahead of time? While failures in testing do look disorderly, they would be of practical value. If any major changes or revisions fix this problem, it will quickly be noticed. And the issue it will not fall of the radar (event though the issue is not high priority).

@KodrAus
Copy link
Contributor

KodrAus commented Aug 2, 2019

Ah I think that would end up masking any regressions the tests catch.

@brmmm3
Copy link
Contributor

brmmm3 commented Jan 27, 2020

What about performance? I've seen that globset is using regex. I assume that regular expressions are slower than the matchers in glob.

@BurntSushi
Copy link
Member

I assume that regular expressions are slower than the matchers in glob.

Why? Have you measured it?

@brmmm3
Copy link
Contributor

brmmm3 commented Jan 27, 2020

I have no benchmarks yet for glob Pattern matching and globset. But my experience with other languages is that regex compared to simple string search is much slower. This is clear because regex can be very complex. The matching algorithm in glob seems to be simple. So I would assume that it is fast.

@BurntSushi
Copy link
Member

@brmmm3 In many cases, regex is complex precisely because it is built to be fast. globset should not ever be much slower than glob (although it may be marginally slower in some cases), but in other cases, it can be much faster. That's one of the reasons why I wrote it.

(globset does have some benchmarks comparing this crate and globset, but they aren't comprehensive.)

@brmmm3
Copy link
Contributor

brmmm3 commented Jan 27, 2020

I've run some benchmarks using the recursive wildcards tests with glob and globset. globset was only a bit slower (<4%). I'm impressed. Thumbs up for globset :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants