Bug with partial wildcards

[es128]

> mm.isMatch('../test-fixtures/add.txt', '../test-fixtures/**/a*.txt')
true
> mm.isMatch('../test-fixtures/add.txt', '../test-*/**/a*.txt')
false
> mm.isMatch('../test-fixtures/subdir/subsub/a.txt', '../test-*/**/a*.txt')
false
> mm.isMatch('../test-fixtures/subdir/subsub/ab.txt', '../test-*/**/a*.txt')
true

Realized this is the cause of a few newly failing tests in chokidar

[es128]

Looks like it's specific to the globstar occurring right after the partial (tailing) wildard - the */** part.

> mm.makeRe('../test-*/**/a*.txt')
/^(?:..\/test-(?!\.)(?=.)[^\/]*?\/.*\/a(?!\.)(?=.)[^\/]*?\.txt)$/
> mm.makeRe('../test-fixtures/**/a*.txt')
/^(?:(?:..\/test-fixtures\/a(?!\.)(?=.)[^\/]*?\.txt|..\/test-fixtures\/.*\/a(?!\.)(?=.)[^\/]*?\.txt))$/
> mm.makeRe('../test-*tures/**/a*.txt')
/^(?:(?:..\/test-(?!\.)(?=.)[^\/]*?tures\/a(?!\.)(?=.)[^\/]*?\.txt|..\/test-(?!\.)(?=.)[^\/]*?tures\/.*\/a(?!\.)(?=.)[^\/]*?\.txt))$/
> mm.isMatch('../test-fixtures/add.txt', '../test-*/**/a*.txt')
false
> mm.isMatch('../test-fixtures/add.txt', '../test-*tures/**/a*.txt')
true

[es128]

Been digging into this. Focusing on this line atm https://github.com/jonschlinkert/micromatch/blob/c007899ae1e288310cc0c43d5d57faf6d7ec671a/lib/expand.js#L138

Appears to be where the globstar handling fails, probably because of the \w

[es128]

Ok it's actually the \ws down in https://github.com/jonschlinkert/micromatch/blob/c007899ae1e288310cc0c43d5d57faf6d7ec671a/lib/expand.js#L284-L294

Replacing them with [^\/] fixes the issue. Now to try it against the rest of the test suite and write a new test...

[es128]

The fix described in the last comment did not fully resolve the issue. There seems to be a secondary problem with partial wildcard matches after a globstar as well.

> mm.isMatch('a/b.txt', '**/*.txt')
true
> mm.isMatch('a/b.txt', '**/b.txt')
true
> mm.isMatch('a/b.txt', '**/b*.txt')
false
> mm.isMatch('a/bc.txt', '**/b*.txt')
true

[jonschlinkert]

sorry I've been offline all day. just reading over this now

[es128]

Was just beginning to prepare a branch and PR

[jonschlinkert]

did you figure out which regex was causing it?

[jonschlinkert]

crazy that this pattern wouldn't be anywhere in the unit tests...

[es128]

I have figured out half of the issue so far - with the optionalGlobstar function. Although it points to an assumption that may need to be adjusted elsewhere too regarding using \w to indicate path part boundaries

[jonschlinkert]

Ok, I can get it the rest of the way. Do you want to do the pr still?

[jonschlinkert]

Sorry you had to deal with this.

[es128]

Yeah I'll get the PR started in a minute, but on a branch here so you'll be able to keep pushing commits to it and finish up.

Sorry you had to deal with this

No worries. It was an opportunity to get acquainted with how this thing works

[jonschlinkert]

ah, wait, this is the one that's causing the issue, right?

> mm.isMatch('a/b.txt', '**/b*.txt')
false

If so, I can fix that, but it was intentional, since that's how the specification is written, but it's not how minimatch works. I like how minimatch works for this - so we can change it

[jonschlinkert]

Yeah I'll get the PR started in a minute, but on a branch here so you'll be able to keep pushing commits to it and finish up.

great, thanks!

[jonschlinkert]

I'm looking for that spec, it was either wildmatch (used by git) or bash. no matter though since we're changing it

[es128]

That's the part I hadn't solved yet.

This is a separate problem as well (which I have solved)

> mm.isMatch('a/b-c/z.js', 'a/b-*/**/z.js')
false

[jonschlinkert]

ok, great.

[jonschlinkert]

@es128 would you mind pushing up some failing unit tests that describe all the failing patterns? Maybe just create a section right here and name it something like issue #15?

[jonschlinkert]

ha, well you might have already done it. and that's a better place too probably

[es128]

Yeah I tried to isolate them to expose the observed issues in the simplest possible form (and potential related ones).

But I was mainly focusing on the use case that I started with - I suspect there are more holes that can be poked. Would take a close look at any regex that uses \w

[jonschlinkert]

looks like they're all they same issue.

Would take a close look at any regex that uses \w

If it makes sense to change all boundaries to use slashes instead of word characters. I'll try to get this one fixed then we can go from there and decide where else we want to depart from spec

[jonschlinkert]

btw, one thing that might not be immediately obvious in expand.js is how I'm using glob.repl versus replace. they're they same except replace also does escaping on the patterns so we don't continue to replace ? and * characters throughout parsing

[es128]

That did confuse me briefly when I was monitoring the value of pattern instead of glob.pattern

[jonschlinkert]

quick glance, there are only four places using \w.

• one is for drive letters \w:
• two are dotfile related

but the other we might be able to get rid of

[jonschlinkert]

did you notice the track option?

[jonschlinkert]

it might not be documented...

[es128]

Sorry AFK at this point. But no, didn't notice it
On Thu, Mar 26, 2015 at 5:36 PM Jon Schlinkert notifications@github.com
wrote:

it might not be documented...

—
Reply to this email directly or view it on GitHub
#15 (comment)
.

[jonschlinkert]

no prob. I'm on it

[jonschlinkert]

I don't see that as at odds with path/to/**/*.js matching path/to/this.js

no you're right. that would be a match.

[es128]

oh interesting, turns out github comments drops a * when there are two in a quote

[jonschlinkert]

yeah I just saw that too lol. I had to edit and add backticks

[es128]

Why are you getting different results?

> mm.isMatch('z.js', '**/z*.js')
true
> mm.makeRe('**/z*.js').test('z.js')
true

[jonschlinkert]

not with micromatch. minimatch produces a different answer between the two methods

[es128]

you show false above? just a copypasta mistake maybe?

[jonschlinkert]

oh, wait. I think I missed the point. yeah one sec

[jonschlinkert]

wanted to double check. yes you're correct. I just didn't update the result in the comments before I pasted. seems important to do lol

[jonschlinkert]

ok, corrected

[jonschlinkert]

I'm thinking we're in good shape for now. so far, everything seems to be consistent with the spec.

[es128]

ok yeah... we have beat this to death

for every example that's been discussed, micromatch's current behavior is correct, and the tests are appropriate for keeping it that way

@tunnckoCore I hope satisfactory proof has been provided for you. If not, I'm sorry. But I, for one, am done here.

[tunnckoCore]

haha, okey thanks guys. I dont like the results, but okey.

[es128]

For reference, if you want a pattern that says one or more subdirs (instead of 0 or more), use /*/**

[tunnckoCore]

but .. doh. I cant understand why this is true (as shown in examples)

console.log(micromatch.isMatch('z.js', '**/z*.js'));
//=> true

and this is false

console.log(micromatch.isMatch('a/z.js', 'a/z*.js'));
//=> false

[es128]

ah you're right on this one, you've spotted a bug. Second example should be true

[tunnckoCore]

doh, noooo, opposite :D

[tunnckoCore]

first is absolutely wrong for me. and think it, if it have ability to talk it would say "a want dir and something starting with z"

[es128]

nope, sorry - that's just is not how it's supposed to be. If you want that it would be */z*.js. Or if you want any number (1 or more) of subdirs it's */**/z*.js

[tunnckoCore]

If you want that it would be */z.js

what?

[es128]

"a want dir and something starting with z"

to express that as a glob: */z*

or if you meant a javascript file */z*.js

one star instead of two. ** means zero or more subdirs. This is very well established at this point.

[tunnckoCore]

so one star means one or more? lol

[es128]

one star means zero or more characters. In the context of being the only thing between path separators, it means exactly one dir with any name.

[tunnckoCore]

var micromatch = require('micromatch');

var paths = [
  'd.js',
  'e.php',
  'z.js',
  'a/b.cs',
  'a/k.php',
  'a/z.js',
  'a/b/h.js',
  'a/b/c/z.js'
];

console.log(micromatch.isMatch('z.js', '**/z*.js'));
//=> true
console.log(micromatch.isMatch('a/z.js', 'a/z*.js'));
//=> false

console.log(micromatch(paths, '**/z*.js'));
//=> [ 'z.js', 'a/z.js', 'a/b/c/z.js' ]
console.log(micromatch(paths, '*/z*.js'));
//=> []
console.log(micromatch(paths, '*/*.js'));
//=> [ 'a/z.js' ]
console.log(micromatch(paths, '**/*.js'));
//=> [ 'd.js', 'z.js', 'a/z.js', 'a/b/h.js', 'a/b/c/z.js' ]
console.log(micromatch(paths, '*z*.js'));
//=> [ 'z.js' ]
console.log(micromatch(paths, '**z*.js'));
//=> [ 'z.js', 'a/z.js', 'a/b/c/z.js' ]

[es128]

You've identified one bug and represented it twice in these examples:

console.log(micromatch.isMatch('a/z.js', 'a/z*.js'));
//=> should be true
console.log(micromatch(paths, '*/z*.js'));
//=> should be [ 'a/z.js' ]

And then potentially one more with that last example, although it's a more academic case that wouldn't be found among anyone's legitimate use cases.

[tunnckoCore]

and multimatch

// multimatch

console.log(multimatch(paths, '**/z*.js'));
//=> [ 'z.js', 'a/z.js', 'a/b/c/z.js' ]
console.log(multimatch(paths, '*/z*.js'));
//=> [ 'a/z.js' ]
console.log(multimatch(paths, '*/*.js'));
//=> [ 'a/z.js' ]
console.log(multimatch(paths, '**/*.js'));
//=> [ 'd.js', 'z.js', 'a/z.js', 'a/b/h.js', 'a/b/c/z.js' ]
console.log(multimatch(paths, '*z*.js'));
//=> [ 'z.js' ]
console.log(multimatch(paths, '**z*.js'));
//=> [ 'z.js' ]

[jonschlinkert]

@tunnckoCore can I just ask why you're doing this? you said you don't use this, but you're trying your best to find bugs - which IMO is fine b/c it helps us, but I don't get the impression you're pointing out flaws to try to be helpful here.

[tunnckoCore]

b/c I will use it, but it looks wrong to me.

nevermind

[es128]

Yeah we're just going to have to move on.

@tunnckoCore you've made your point that you disagree with how globstar is intended to work as agreed upon in every spec and implementation we can find, but that's how it is (and it isn't an arbitrary decision by @jonschlinkert nor I that you can talk us out of). I'd suggest you either adjust your understanding or just refrain from using globstar functionality in your glob patterns.

Your (constructive) feedback continues to be welcome (at least as far as I'm concerned), and thank you for helping to uncover two new bugs.