Bug with partial wildcards about micromatch HOT 103 CLOSED

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

Was just beginning to prepare a branch and PR

from micromatch.

did you figure out which regex was causing it?

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

Sorry you had to deal with this.

from micromatch.

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

from micromatch.

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

from micromatch.

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!

from micromatch.

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

from micromatch.

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

from micromatch.

ok, great.

from micromatch.

@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?

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

did you notice the track option?

from micromatch.

it might not be documented...

from micromatch.

Sorry AFK at this point. But no, didn't notice it
On Thu, Mar 26, 2015 at 5:36 PM Jon Schlinkert [email protected]
wrote:

it might not be documented...

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

from micromatch.

no prob. I'm on it

from micromatch.

I have a fix. I'll be pushing it up in a bit after testing more

from micromatch.

I'm still working on the fix. It's not the regex or getting to work that's a problem, it's the spec and inconsistencies with minimatch (or rather, minimatch's inconsistencies), and I want to make sure I'm not missing something before I push this up. In the meantime, I pushed up a patch with your fix.

from micromatch.

meant to just comment....

from micromatch.

Alright. The publish wasn't necessary - the patch I had committed wasn't enough to make the chokidar tests pass. But no worries, looking forward to your the fixes you're polishing up.

from micromatch.

k, thx. just wanted to show this, here is one of the many examples of inconsistencies with minimatch:

var minimatch = require('minimatch');
console.log(minimatch('foo/baz/bar', 'foo/**/**/bar'))
//=> true
console.log(minimatch.makeRe('foo/**/**/bar').test('foo/baz/bar'))
//=> false

This, and gaps in the spec, are what made this a pita in the beginning

from micromatch.

for the heck of it, here's another one for you. Run the following:

var minimatch = require('minimatch');
var micromatch = require('./');


console.log('micromatch:')
console.log(micromatch.makeRe('**/z*.js'))
console.log(micromatch.isMatch('z.js', '**/z*.js'))
console.log(micromatch.makeRe('**/z*.js').test('z.js'))

console.log()
console.log('----------')
console.log()

console.log('minimatch:')
console.log(minimatch.makeRe('**/z*.js'))
console.log(minimatch('z.js', '**/z*.js'))
console.log(minimatch.makeRe('**/z*.js').test('z.js'))

Outputs:

micromatch:
/^(?:(^|.+\/)(?=.)z[^/]*?\.js)$/
true
true

----------

minimatch:
/^(?:(?:(?!(?:\/|^)\.).)*?\/(?=.)z[^/]*?\.js)$/
true
false

from micromatch.

console.log(micromatch.makeRe('*/z.js').test('z.js'))

damn, this is dramatically bad.

from micromatch.

good grief, please lighten up the tone a little. that doesn't help. Try posting a proposed solution instead to encourage discussion and collaboration to solve the problem.

None of these patterns is too difficult to implement. of course we'll have some things that didn't get tested, including some seemingly obvious ones at first. but at the end of the day:

• we would have to make 150 patches like the one we did this past week for normalizing file paths to make micromatch as slow as minimatch.
• the code here isn't perfect (yet) but it passes more tests than minimatch
• the code is far more maintainable than minimatch, lending itself to be more reliable over the long run

why focus on a snippet of code?

from micromatch.

The code in the quote is modified from what was posted earlier in the thread. That version evaluates as false, which makes sense. The original version started with a globstar, and it evaluates as true (console.log(micromatch.makeRe('**/z*.js').test('z.js'))). Both cases currently behave exactly as I'd expect them to.

from micromatch.

yeah I was just going to say, I just tested it and the code produces what I would expect:

console.log(micromatch.makeRe('*/z.js').test('z.js'))
//=> false

@tunnckoCore is there a reason that should evaluate to true?

from micromatch.

is there a reason that should evaluate to true?

no, exactly what I mean. it should stay false.

from micromatch.

there's no logic to be true and also here

 mm.isMatch('a/b-c/z.js', 'a/b-*/**/z.js').should.be.true;

from micromatch.

that result is correct. /**/ may also be evaluated as /

from micromatch.

and /**/**/**/**/**/**/**/**/**/**/**/**/**/** may also be evaluated as /

from micromatch.

@tunnckoCore is it possible that you misread the example above where minimatch fails, thinking it was this library?

from micromatch.

this one? no.

idk, sorry for the tone around, but Im just not okey with the added tests and think that they should be false. plus this one on line 73

 mm.isMatch('z.js', '**/*.js').should.be.true;

from micromatch.

okey if we back to this comment I understand that the problem was that this one

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

returns false? @es128

from micromatch.

Seems like you simply disagree with how globstar is meant to work.

Except this one already existed before and you didn't take issue with it.

The glob spec and minimatch both also agree that globstar means 0 or more subdirs, not 1 or more

from micromatch.

Except this one already existed before and you didn't take issue with it.

yea I notice that.

from micromatch.

the only one that looks wrong to me is console.log(micromatch.isMatch('z.js', '**/z*.js')); I can't see how that would be correct.

from micromatch.

That's right.

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

should be true.

from micromatch.

but that's different since the path to match against has a slash. technically you should have to define matchBase to only match on the basename

from micromatch.

hmmm

from micromatch.

matchBase to only match on the basename

exactly

from micromatch.

can't see how that would be correct.

Im not saying that.

from micromatch.

wait a sec

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

looks right to me. The globstar is 0 or more subdirs, and the wildcard adjoined with a (non-separator) char is 0 or more chars.

from micromatch.

should be true.

I dont know why it should be, but this is wrong behavior for me.

from micromatch.

just... intuition?

the globstar part or the zero-width wildcard part - which seems wrong to you?

from micromatch.

the thing that makes this seemingly straightforward is that there are use cases where you are both correct. but it's not so "exact". the specification differs based on accompanying patterns.

from micromatch.

like this. the following result is correct. I think we would all agree with this, no?

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

from micromatch.

hm, I dont thinks so. But want to mention that I didn't read the spec. Im not spec specialist.

from micromatch.

Puts this comment in a very different light

from micromatch.

I didn't memorize the spec(s) yet, but I did read them. Bash 4.3 and wildmatch (used by git). They follow the same specifications for most cases, but there are holes in both, and differences.

How about if I come back later with some spec details so that if we continue this discussion we can do so around some hard data?

from micromatch.

http://man7.org/linux/man-pages/man7/glob.7.html is far from comprehensive - but it is very clear about a wildcard * character being able to match 0 (empty string) or more characters

from micromatch.

its simple.. on bash 4.1

GNU bash, version 4.1.2(1)-release (i386-redhat-linux-gnu)

i did some tests on the file system

just created following structure

a/b.cs
a/k.php
a/z.js
d.js
e.php
z.js

and this ls -al **/*.js matches only a/z.js
and this ls -al *.js matches only d.js and z.js

from micromatch.

so this one

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

should be false

from micromatch.

bash 4.1

This follows Bash 4.3.

See:

• https://github.com/jonschlinkert/micromatch/blob/master/test/bash.js#L20 (this is complete, except for unit tests that had bash variables or patterns that are irrelevant in node. minimatch does not pass all of these, micromatch does)
• https://github.com/jonschlinkert/micromatch/blob/master/test/braces.js#L65 (even though the attribution comment is on line 65, virtually all of those tests are derived from wildmatch)

from micromatch.

add more subdirs: a/b/c/d/e.js

does it match?

I suspect the globstar option wasn't even enabled here.

from micromatch.

should be false

You're making falsey assumptions about the spec.

from micromatch.

and don't point out the extglob wildmatch tests lol, that's not even officially supported here yet (nor by minimatch), but this still passes most of them.

from micromatch.

Here's another reference explaining how globstars are supposed to work, which is consistent with how micromatch currently behaves:

http://www.gnu.org/software/bash/manual/bashref.html#The-Shopt-Builtin-1

from micromatch.

that's what I was just reading over, thx.

from micromatch.

does it match?

actually nope, keep matching only a/z.js

from micromatch.

eeeexactly

from micromatch.

If the pattern is followed by a ‘/’, only directories and subdirectories match.

This is the part of the spec I was focused on with example above. Minimatch does this, and micromatch does as well. However, like every specification, there are exceptions to this rule. It doesn't apply to every pattern in the specification.

from micromatch.

okay, here is micromatch vs minimatch for all of the patterns in the section we've been discussing.

Both libraries yield the same result for all of the following:

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

console.log(micromatch.isMatch('a/b-c/d/e/z.js', 'a/b-*/**/z.js'));
//=> true
console.log(micromatch.isMatch('a/b-c/z.js', 'a/b-*/**/z.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/d/e/z.js', 'a/b/**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/d/z.js', 'a/b/**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/z.js', '**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/z.js', 'a/b/**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/z.js', 'a/b/c**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/c/z.js', 'a/b/c/**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/z.js', '**/*.js'));
//=> true
console.log(micromatch.isMatch('a/b/z.js', 'a/b/**/*.js'));
//=> true
console.log(micromatch.isMatch('a/z.js', '**/*.js'));
//=> true
console.log(micromatch.isMatch('z.js', '**/*.js'));
//=> true
console.log(micromatch.isMatch('z.js', '**/z*'));
//=> true
console.log(micromatch.isMatch('z.js', '*.js'));
//=> true

However, results differ on the following:

// minimatch
console.log(minimatch.makeRe('**/z*.js').test('z.js'));
//=> false
console.log(minimatch('z.js', '**/z*.js'));
//=> true

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

from micromatch.

I interpret that to mean the difference between path/to/** and path/to/**/.

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

from micromatch.

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.

from micromatch.

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

from micromatch.

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

from micromatch.

Why are you getting different results?

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

from micromatch.

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

from micromatch.

you show false above? just a copypasta mistake maybe?

from micromatch.

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

from micromatch.

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

from micromatch.

ok, corrected

from micromatch.

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

from micromatch.

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.

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

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

from micromatch.

doh, noooo, opposite :D

from micromatch.

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"

from micromatch.

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

from micromatch.

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

what?

from micromatch.

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

from micromatch.

so one star means one or more? lol

from micromatch.

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.

from micromatch.

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' ]

from micromatch.

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.

from micromatch.

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' ]

from micromatch.