fs: add followSymlinks option to glob and globSync#61317
fs: add followSymlinks option to glob and globSync#61317MatricalDefunkt wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
fs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61317 +/- ##
==========================================
- Coverage 88.54% 88.52% -0.03%
==========================================
Files 704 704
Lines 208753 208796 +43
Branches 40283 40285 +2
==========================================
- Hits 184849 184835 -14
- Misses 15907 15979 +72
+ Partials 7997 7982 -15
🚀 New features to boost your workflow:
|
|
|
||
| describe('glob follow', () => { | ||
| test('should return matched files in symlinked directory when follow is true', async () => { | ||
| if (common.isWindows) return; |
There was a problem hiding this comment.
can you explain why we completely bail on this test if we're on Windows?
There was a problem hiding this comment.
The setup function excluded it specifically as well. I did not question why.
From what I am understanding, symlinks need admin permissions or developer mode enabled on Windows which is perhaps why the test does not exist for it.
| * `followSymlinks` {boolean} `true` to traverse matching symbolic links to directories, | ||
| `false` otherwise. **Default:** `false`. |
There was a problem hiding this comment.
Make sure to update the changes entry above
There was a problem hiding this comment.
What should be the version tag for the change?
| const { lstatSync, readdirSync, statSync: fsStatSync } = require('fs'); | ||
| const { lstat, readdir, stat: fsStat } = require('fs/promises'); |
There was a problem hiding this comment.
Oh that was for my reference. I will rename and push. Sorry!
I won't be pushing. Many locations in the file refer to a variable named stat. keeping the function name as the same would reduce readability.
If you believe that I should make the change, do let me know, I'll commit and push ASAP :)
lib/internal/fs/glob.js
Outdated
| try { | ||
| const stat = fsStatSync(join(fullpath, entry.name)); | ||
| isDirectory = stat.isDirectory(); | ||
| } catch { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Shouldn't an error here be bubbled up? If the user explicitly asked for us to follow symlinks, and we can't do that, wouldn't they expect some kind of indication?
There was a problem hiding this comment.
I agree, we should inform the user. Sorry for the oversight
There was a problem hiding this comment.
Do you think we should emit a warning or an error would be more appropriate? I believe an error is better.
MatricalDefunkt
force-pushed
the
feature/glob-recurse-symlinks
branch
from
e611924 to
7b0c22d
Compare
| @@ -0,0 +1,34 @@ | |||
| // test-stat-identity.mjs | |||
| isDirectory = stat.isDirectory(); | ||
| if (isDirectory) { | ||
| // Check for cycles by looking at visited inodes | ||
| let node = pattern.visited; |
There was a problem hiding this comment.
I'm a bit lost in the implementation, why are you attaching visited to patterns while you mention inodes in the comment?
|
@MatricalDefunkt any updates? |
|
Hey, sorry for the late response, @mcollina I would like to close this for now. This is a pretty poor PR, absolutely not my best work, and I think the project deserves more than this slop. I will retry with a more thorough and well thought out PR. Thank you for the reviews and the time you gave. |
5 participants
This PR adds a
followSymlinksoption tofs.globandfs.globSync, allowing recursive directory traversal into symbolic links when enabled.Changes
lib/internal/fs/glob.js:followSymlinkstoGlobconstructor options.Globclass to handlefollowSymlinks: true.statwhenfollowSymlinksis enabled. If the symlink points to a directory, it is now traversed.doc/api/fs.md.test/parallel/test-fs-glob.mjsto verifyfollowSymlinks: truebehavior.Fixes: #61033