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 read permission error on ignore files search #259

Merged
merged 2 commits into from
Feb 11, 2024
Merged

Fix read permission error on ignore files search #259

merged 2 commits into from
Feb 11, 2024

Conversation

nicolas-goudry
Copy link
Contributor

@nicolas-goudry nicolas-goudry commented Nov 12, 2023

This PR should fix #254 and fix #258. As well as xo’s #737 when globby’s dependency gets upgraded.

It allows to forward the ignore option field passed to globby’s isIgnoredByIgnoreFiles to fast-glob.

To give some more context, this function is used to find all .gitignore files relative to the current working directory. If any subdirectory of the current working directory is not readable, fast-glob exits with an error.

By forwarding the ignore option to fast-glob when searching for ignore files, we prevent this behavior.

A few things that should be considered before this gets merged:

Documentation

There are currently no docs about this, but since the readme file mentions that the options object extends the fast-glob options, I’m not sure if it’s worth to document.

Tests

When a directory is not readable, the behavior of fast-glob is to exit, not to throw.
So, if we remove the ignore option of the test, ava will fail all tests run with the following error:

Exiting due to process.exit() when running tests/ignore.js

Some side nodes about the test:

  • .notThrowsAsync is used, but as said above: fast-glob doesn’t throw, it exits ; so this might be wrong. You tell me
  • .serial is used to prevent other tests to fail because of the bad directory permissions
  • .teardown is used to revert the directory permissions to something readable

@nicolas-goudry
Copy link
Contributor Author

nicolas-goudry commented Nov 12, 2023

To fix #254 with the provided steps to reproduce, would give the following:

mkdir someproject
cd someproject
npm init -y && npm i globby
mkdir testdir
chmod -r testdir # remove read access
# Notice the addition of "ignore: ['testdir']" option
node --input-type=module -e "const { globby } = await import('globby'); console.log(await globby('test', { gitignore: true, ignore: ['testdir'] }))"

@nicolas-goudry
Copy link
Contributor Author

@sindresorhus could you please take a look at this?

@sindresorhus sindresorhus merged commit 3a28601 into sindresorhus:main Feb 11, 2024
6 checks passed
azu added a commit to secretlint/secretlint that referenced this pull request Feb 17, 2024
- update globby@14.0.1 which  Fix read permission error on ignore files search

refs sindresorhus/globby#259
closes #460
azu added a commit to secretlint/secretlint that referenced this pull request Feb 17, 2024
- update globby@14.0.1 which  Fix read permission error on ignore files search

refs sindresorhus/globby#259
closes #460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants