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

Remove ignore option for isGitIgnored and isGitIgnoredSync #225

Merged
merged 2 commits into from Jan 21, 2022

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Jan 21, 2022

@fisker fisker changed the title Remove ignore option for isGitIgnored Remove ignore option for isGitIgnored and isGitIgnoredSync Jan 21, 2022
@fisker fisker mentioned this pull request Jan 21, 2022
'**/flow-typed/**',
'**/coverage/**',
'**/.git',
];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're removing the ignore option for isGitIgnored, I think globby should still ignore these paths by default in the main globby methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were ONLY used when glob .gitignore file, changing globby() to ignore files in these directories is a new breaking change to the globby() function, it out scope of this PR.

And if we decide to ignore them by default, but how to unignore them?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were ONLY used when glob .gitignore file

Yes, that's what I'm talking about. Only for .gitignore files. It doesn't make sense to look for .gitignore files inside node_modules and doing so would create a big slowdown.

For reference, here's the original commit that added the ignores: ba08350

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you are right, misunderstood.

'**/node_modules',
'**/flow-typed',
'**/coverage',
'**/.git',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I tested, when use **/node_modules/ and **/node_modules/**/*, fast-glob will read those directories, but when use **/node_modules , fast-glob won't read them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering this question #224 (comment)

@fisker
Copy link
Collaborator Author

fisker commented Jan 21, 2022

I still have one thing want to do before you can release, the expandDirectories requires file access, though I already reduced the tasks, we still expanding ignore repeatedly, we can expand them in 0~2 steps, one for the common ignore, one for the negative patterns.

@sindresorhus sindresorhus merged commit 2e43cc4 into sindresorhus:main Jan 21, 2022
@fisker fisker deleted the is-git-ignored-ignore branch January 21, 2022 16:55
@fisker
Copy link
Collaborator Author

fisker commented Jan 24, 2022

I'm going to give up for this #225 (comment).

I have tried, the logic became more complex, but didn't see much performance improvement, maybe because the file access is fast enough. Adding this logic also only benefit cases with multiple discontinuous negative patterns, it's not very common.

@sindresorhus
Copy link
Owner

Thanks for all the work you have been doing. I did a new release: https://github.com/sindresorhus/globby/releases/tag/v13.0.0

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.

None yet

2 participants