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

Ignore option does not work with lowercase drive letter (Windows) #527

Closed
frankkulak opened this issue May 14, 2023 · 3 comments
Closed

Comments

@frankkulak
Copy link

frankkulak commented May 14, 2023

My VS Code extension accepts user-defined include/exclude patterns. I resolve relative paths with path.resolve(...), then convert all \\ to / with absPath.replace(/\\/g, "/") (I've been burned by using \\ in glob pattern on Windows in the past, figured out that glob requires / everywhere from this S/O post).

I then find matches like so:

// include/exclude are string arrays
const matches = glob.sync(include, {
  ignore: exclude,
});

This mostly works as expected (no issues on macOS), and even works on Windows if the user's excluded path is absolute, with an uppercase drive letter. The issue comes up when the user's path is relative, because resolving (which includes normalizing) the paths returns lowercase drive letters on Windows (EDIT: Just discovered that resolving does not lowercase the drive name; the lowercase is from how VS Code returns the root directory path).

This lowercase drive letter works fine for included patterns, it returns all of the paths I'd expect to see. However, the lowercase letter causes the ignored patterns to not work as I would expect, given that it does work with the included paths.

For example, I would expect the following to match all non-XML files within the source folder:

"include": [
  "**/*"
],
"exclude": [
  "**/*.xml"
]

These relative paths resolve to "include": ["c:/blah/**/*"] and "exclude": ["c:/blah/**/*.xml"], and using these paths as illustrated by my first code snippet returns every single file within blah, including *.xml ones.

However, if I instead use:

"include": [
  "**/*"
],
"exclude": [
  "C:/blah/**/*.xml"
]

It works as expected, and all XML files are excluded from the results.

For the time being, I suppose I can just add a hack that forces the drive letter to uppercase on Windows, but since the lowercase letter works in included patterns, I would expect it to also work in ignored ones. Is there a technical reason why this is not the case, or is this a bug? If it's intentional, do you have any best practices for using relative paths in the way I've described?

@isaacs
Copy link
Owner

isaacs commented May 14, 2023

Yeah, I'd expect all those same things to work, so this feels like a bug to me. My guess is that the settings used for Minipass in the Ignore class aren't quite right. (Ignores are just a minipass match, not a glob walk, so there's a bit of a semantic difference which is easy to not get exactly correct.)

Just to confirm, you're using the latest version of node-glob? I'll take a look at this tonight.

@frankkulak
Copy link
Author

frankkulak commented May 14, 2023

Just to confirm, you're using the latest version of node-glob? I'll take a look at this tonight.

Looks like I am one minor version behind, I'm on 10.2.2 and 10.2.3 was released 5 days ago. I just updated and tested again, and the issue is still there.

isaacs added a commit that referenced this issue May 20, 2023
@isaacs
Copy link
Owner

isaacs commented May 20, 2023

The logic used to case-insensitively compare UNC paths like //?/C:/... to their drive letter equivalents c:/... was forced to be case-insensitive on windows, but not if neither was a UNC path. Fixed that on the latest release, now any drive/unc pattern that is a literal drive letter will be compared against a corresponding literal drive letter in the file, in a case-insensitive way.

This issue was closed.
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

2 participants