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

solved: Regression from v3: matches that used to work no longer do #133

Closed
Vinnl opened this Issue Jul 11, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@Vinnl
Copy link

Vinnl commented Jul 11, 2018

The changelog says

There should be no breaking changes. Please report any regressions.

Hence this issue :)

Please describe the minimum necessary steps to reproduce this issue:

Matches that used to work in micromatch@2 no longer do in micromatch@3. I'm not sure exactly in what cases, but it would be good to mention them in the CHANGELOG.

It's explained in more detail at facebook/jest#6563 (comment), but e.g. src/**/*.{js} should now be src/**/*.js, and src/**.js should now be src/**/*.js. There might be more changed matches that I don't know of.

What is happening (but shouldn't):

The CHANGELOG mentions no breaking changes.

What should be happening instead?

The CHANGELOG could mention which globs no longer work, and how they could be changed to work again.

@jonschlinkert

This comment has been minimized.

Copy link
Member

jonschlinkert commented Jul 11, 2018

Hi @Vinnl, thanks for the issue.

Matches that used to work in micromatch@2 no longer do in micromatch@3. I'm not sure exactly in what cases, but it would be good to mention them in the CHANGELOG.

Indeed, the changelog is wrong. This is my reminder to update our very neglected changelog, I'm sorry for the inconvenience that caused.

Regarding the actual issue, since I haven't seen any details that lend insight into where and how the patterns are handled etc. (only that it's "not matching"), I'm going to make a leap and assume that it's related to the most "obvious" thing that comes to mind for me: handling of windows backslashes. My hunch is that path.resolve() or path.join() is being used on globs to make them absolute, which will cause issues if backslashes are in the patterns.

TLDR; This looks like a regression, but we made an intentional necessary change in v3.0 that unfortunately was left out of the changelog but is well documented in the readme.

Here is the bottom line of the changes:

  • before: In previous versions of micromatch, backslashes were converted to forward slashes. Since globs are effectively regular expressions, this caused serious issues for users that needed to escape special characters so they would match as literals. We felt it was necessary to address this, to ensure that users didn't inadvertently write glob patterns with unintended side effects. Like deleting files that shouldn't have been matched.
  • now: In v3.0.0 of micromatch, backslashes are respected as escape characters. This is also the correct behavior per bash spec.
  • advice: use forward slashes when joining globs to file paths

If it turns out that the problem was related to something else, we'll pretend I didn't write all of this and I'll just move it over to our neglected changelog so that I can just start calling it our "changelog" ;)

@Vinnl

This comment has been minimized.

Copy link
Author

Vinnl commented Jul 11, 2018

It's a bit hard for me to diagnose as well since it happened inside on of my dependencies (i.e. Jest), but if by "window backslashes" you meant "Windows backslashes" - if that's the case, then Jest should have been doing something with my glob pattern. I'm running on Linux, and did not escape any paths. If helpful, my glob patterns were defined here.

Of course, addressing issues is perfectly understandable :) In any case, thanks for the project :)

@jonschlinkert

This comment has been minimized.

Copy link
Member

jonschlinkert commented Jul 11, 2018

window backslashes

typo, fixed.

if that's the case, then Jest should have been doing something with my glob pattern. I'm running on Linux, and did not escape any paths. If helpful, my glob patterns were defined here.

Hmm, something else is going on then. I'm still thinking it's related to joining globs, but I'm not sure what specifically. I'll try to look into it. Thanks for the additional info.

@jonschlinkert

This comment has been minimized.

Copy link
Member

jonschlinkert commented Jul 11, 2018

I'm running on Linux, and did not escape any paths.

Fwiw, to clarify (even though it doesn't really pertain to you since you're not using Windows) it's more related to something like this: path.join(cwd, '**/*.js').

On Windows, that would result in something like '\\foo\\bar\\**/*.js', where the first * is now escaped and will no longer be respected as a special character. I know this might seem inconvenient, but the alternative is the inverse scenario, where we might convert a literal string into a special character that matches things that it shouldn't. That also leaves the door open for attackers to create malicious globs.

Where/when are the errors occuring? Locally, or on a service like Appveyor?

@Vinnl

This comment has been minimized.

Copy link
Author

Vinnl commented Jul 11, 2018

For me, I encountered the errors both locally and running in GitLab CI/CD - both on Linux. That said, there were quite a few others in that bug report that encountered it too, and I don't know where they encountered it. Of course it's perfectly possible that it happens somewhere in the interaction with Jest, but I just noticed that you are present in their issue tracker already, so they'll probably be able to tell you more about that.

@Tvrqvoise

This comment has been minimized.

Copy link
Contributor

Tvrqvoise commented Jul 11, 2018

Here's a test case demonstrating the difference of behavior between versions:
https://github.com/Tvrqvoise/test-micromatch-bracket-handling

A couple opinions here:

  • The behavior in version 3 is in my eyes correct. For that reason, I don't feel this is a regression so much as a documentation issue
  • We don't know that version 3.0.0 is precisely where it broke. It's possible that a later bug fix caused this. You could try out that test case with more versions, and see if any of them cause it to start working again.

My uname:
Darwin MACC02VT0LXHTD7 16.7.0 Darwin Kernel Version 16.7.0: Fri Apr 27 17:59:46 PDT 2018; root:xnu-3789.73.13~1/RELEASE_X86_64 x86_64

@jonschlinkert

This comment has been minimized.

Copy link
Member

jonschlinkert commented Jul 11, 2018

ah, all three of those patterns are invalid globs.

  • 'foo/**/*.{js}' - since {js} does not contain either a comma-separated set ({a,b}) or a range ({a..b}) it's considered a string literal. This should have always been the case. If it worked before, that was a bug in the previous version that has been fixed.
  • 'foo/**/*.{js|ts}' - commas should be used to separate brace patterns. You can alternatively use an extglob (like foo/**/*.@(js|ts), foo/**/*.*(js|ts), or foo/**/*.+(js|ts)) or a simply a regex group (foo/**/*.(js|ts))
  • 'foo/**.js' - when two consecutive stars (so called "globstars") are not the only thing in a segment (between slashes, or beginning or ending a pattern), they are treated as a single star. This is bash spec, and it's how v2 of micromatch should have behaved as well.
@Tvrqvoise

This comment has been minimized.

Copy link
Contributor

Tvrqvoise commented Jul 11, 2018

Totally agreed -- I think the issue is just that this change in behavior isn't mentioned in the CHANGELOG. I think even: "v3 handling of glob is more strict, so some invalid expressions which worked in v2 may no longer function" would suffice, though if we know precisely what changed (which would be hard, since v3 was cut so long ago) that would be better.

@jonschlinkert

This comment has been minimized.

Copy link
Member

jonschlinkert commented Jul 13, 2018

v3 handling of glob is more strict, so some invalid expressions which worked in v2 may no longer function

If you think about it, that doesn't really make a lot of sense, since there are an infinite number of patterns that don't work, and only a finite number of patterns that do. The only patterns this library ever had a "contract" to match with was valid patterns.

IMHO, it's not a bug or a regression that the newest version of micromatch makes user errors more obvious.

edit: to be clear, v3 isn't "more strict", it's more accurate. So maybe I should add something like "Since v3 is more accurate than previous versions, it's possible that invalid patterns that worked in the past will no longer work"?

Tvrqvoise pushed a commit to Tvrqvoise/micromatch that referenced this issue Jul 13, 2018

@jonschlinkert jonschlinkert changed the title Regression from v3: matches that used to work no longer do solved: ~~Regression from v3: matches that used to work no longer do~~ Jul 30, 2018

@jonschlinkert jonschlinkert changed the title solved: ~~Regression from v3: matches that used to work no longer do~~ solved: Regression from v3: matches that used to work no longer do Jul 30, 2018

jonschlinkert added a commit that referenced this issue Dec 14, 2018

Merge pull request #134 from Tvrqvoise/v3-changelog
Add CHANGELOG for version 3, fixes #133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment