Skip to content

Remove ignore option for isGitIgnored and isGitIgnoredSync#225

Merged
sindresorhus merged 2 commits intosindresorhus:mainfrom
fisker:is-git-ignored-ignore
Jan 21, 2022
Merged

Remove ignore option for isGitIgnored and isGitIgnoredSync#225
sindresorhus merged 2 commits intosindresorhus:mainfrom
fisker:is-git-ignored-ignore

Conversation

@fisker
Copy link
Copy Markdown
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
@fisker fisker force-pushed the is-git-ignored-ignore branch from a5113f3 to bad86bb Compare January 21, 2022 04:57
'**/flow-typed/**',
'**/coverage/**',
'**/.git',
];
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants