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

Fix using gitignore and absolute options at the same time on Windows #137

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Oct 16, 2019

When globby is invoked with globby('*', { absolute: true, gitignore: true }) on Windows systems, the assertion added in 4af038e always triggers. This is because when we default the cwd in normalizeOptions, we didn't convert from C:\\ style paths to forward slashes.

A simple work around for users hitting this issue, is to provide a cwd option that has already been converted:

globby('*', { absolute: true, gitignore: true, cwd: slash(process.cwd()) })

The test added here was failing on Windows, example output prior to the fixes:

  test » gitignore option with absolute option
  C:\Users\travis\build\rwjblue\globby\gitignore.js:52
   51:
   52:     throw new Error(`Path ${p} is not in cwd ${cwd}`);
   53:   }
  Rejected promise returned by test. Reason:
  Error {
    message: 'Path C:/Users/travis/build/rwjblue/globby/a.tmp is not in cwd C:\\Users\\travis\\build\\rwjblue\\globby',
  }

Related to #133 (though it isn't clear that this fixes all of the scenarios that #133 is exploring).

When `globby` is invoked with `globby('*', { absolute: true, gitignore: true
})` on Windows systems, the assertion added in
4af038e **always** triggers. This is
because when we default the `cwd` in `normalizeOptions`, we didn't
convert from `C:\\` style paths to forward slashes.

The test added here was failing on Windows, example output prior to the
fixes:

```
  test » gitignore option with absolute option
  C:\Users\travis\build\rwjblue\globby\gitignore.js:52
   51:
   52:     throw new Error(`Path ${p} is not in cwd ${cwd}`);
   53:   }
  Rejected promise returned by test. Reason:
  Error {
    message: 'Path C:/Users/travis/build/rwjblue/globby/a.tmp is not in cwd C:\\Users\\travis\\build\\rwjblue\\globby',
  }
```
@jamiekyle-eb
Copy link
Contributor

I found a library path-is-inside that is supposed to do a better job than the filePath.startsWith(cwd) check I wrote on https://github.com/sindresorhus/globby/blob/master/gitignore.js#L48

@rwjblue
Copy link
Contributor Author

rwjblue commented Oct 16, 2019

Ya, looks like that one deals with case sensitivity by platform and normalizing trailing slashes, but still effectively is doing .startsWith (it uses .lastIndexOf(needle, 0) === 0). I think it would fail in the same way described in my description above (because it would not convert the backslashes in process.cwd() to forward slashes which is what fast-glob is giving us).

I'm happy to pull in that package and use it here, I just think we'll still need to normalize the process.cwd() before comparison.

@sindresorhus
Copy link
Owner

sindresorhus commented Jan 6, 2020

I have a similar package we could use: https://github.com/sindresorhus/is-path-inside It fixes a couple of shortcomings of path-is-inside.

Moving this to a new issue: #140

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

Successfully merging this pull request may close these issues.

3 participants