-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
Respect gitignore files from parent directories #107
Conversation
I just came here to work on this issue and here's a PR already! This would really help out my jsx-info project. |
@dflupu Hey, sorry. I totally missed this PR... If you're still interested, would you be able to fix the merge conflict and add some more tests? |
Sure, I'll have it fixed up in the next few days. |
8159ac0
to
ac501b6
Compare
@sindresorhus I've updated the PR. Let me know what you think. |
gitignore.js
Outdated
const getIsIgnoredPredecate = (ignores, cwd) => { | ||
return p => ignores.ignores(slash(path.relative(cwd, ensureAbsolutePathForCwd(cwd, p)))); | ||
const getIsIgnoredPredecate = (ignores, gitRoot, cwd) => { | ||
return p => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a descriptive variable name.
gitignore.js
Outdated
return getIsIgnoredPredecate(ignores, options.cwd); | ||
const {cwd} = options; | ||
|
||
const gitDir = await findUp('.git', {cwd}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const gitDir = await findUp('.git', {cwd}); | |
const gitDirectory = await findUp('.git', {cwd}); |
]); | ||
|
||
const gitIgnoreFileContents = await Promise.all( | ||
[].concat(...gitIgnoreFilePaths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a TODO comment about using flatMap here when we target Node.js 12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we do [].concat(...gitIgnoreFilePaths)
because it's nested arrays?
]); | ||
|
||
const gitIgnoreFileContents = await Promise.all( | ||
[].concat(...gitIgnoreFilePaths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we do [].concat(...gitIgnoreFilePaths)
because it's nested arrays?
The new behavior needs to be documented in the readme and index.d.ts. And it would be good with some integrations tests that uses |
I'm also concerned that this will further worsen the performance, see: #50 (comment) |
I've done a bit of testing, and that may happen in some circumstances. Globbing small directory trees (eg <5 directories, <100 files) using the sync function may be up to 3x slower. It does however become less measurable when globbing larger directory trees. The async function is pretty much unaffected when it comes to performance - unless there is a huge gitignore file in a parent directory. Note that the main reason as to why the gitignore option is slow in the first place seems to be the globbing for gitignore files in subdirectories. Maybe that could be avoided in some way? How about checking for gitignore files only in directories and parent directories of files that were matched by the users' glob expression? Some kind of callback from fast-glob whenever it wants to descend into a directory that tells it whether or not it should do that could also work. |
Yes, I think that would be smart. |
Could you open an issue on |
@dflupu Bump |
@sindresorhus I've opened an issue on |
Can you share a link to the issue? |
How do we move forward with this PR? |
Here's the fast-glob issue. I'll mention that I've tried using I see a few options for this PR:
|
I'm going to close as it doesn't look like something will happen with this anytime soon. |
Fixes #86