glob icon indicating copy to clipboard operation
glob copied to clipboard

Pattern **/ matches everything

Open MartinSavc opened this issue 6 years ago • 10 comments

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.

MartinSavc avatar Jul 16 '19 18:07 MartinSavc

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.

KodrAus avatar Jul 21 '19 21:07 KodrAus

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.

MartinSavc avatar Jul 22 '19 08:07 MartinSavc

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 avatar Jul 24 '19 23:07 KodrAus

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).

MartinSavc avatar Jul 25 '19 06:07 MartinSavc

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

KodrAus avatar Aug 02 '19 07:08 KodrAus

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

brmmm3 avatar Jan 27 '20 07:01 brmmm3

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

Why? Have you measured it?

BurntSushi avatar Jan 27 '20 12:01 BurntSushi

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.

brmmm3 avatar Jan 27 '20 21:01 brmmm3

@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.)

BurntSushi avatar Jan 27 '20 21:01 BurntSushi

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 :-)

brmmm3 avatar Jan 27 '20 22:01 brmmm3