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

follow: true doesn't follow more that 1 symbolic link #579

Closed
pamelalozano16 opened this issue Feb 28, 2024 · 7 comments
Closed

follow: true doesn't follow more that 1 symbolic link #579

pamelalozano16 opened this issue Feb 28, 2024 · 7 comments

Comments

@pamelalozano16
Copy link

pamelalozano16 commented Feb 28, 2024

"By default, a ** in a pattern will follow 1 symbolic link if it is not the first item in the pattern, or none if it is the first item in the pattern, following the same behavior as Bash.". However, some dependent modules contain multiple nested symbolic links, and the option follow: true only operates for a single level of symlinks.

dir_foo

foo.txt

dir_bar

foo

dir_baz

bar

In this case
glob.sync('dir_baz/**', {follow:true, nodir:true}) returns foo instead of foo.txt.

@pamelalozano16 pamelalozano16 changed the title Follow more that 1 symbolic link Config option to follow more that 1 symbolic link Feb 28, 2024
@pamelalozano16 pamelalozano16 changed the title Config option to follow more that 1 symbolic link follow: true doesn't follow more that 1 symbolic link Feb 28, 2024
@pamelalozano16
Copy link
Author

@isaacs any thoughts on this? 🤔

@isaacs
Copy link
Owner

isaacs commented Mar 6, 2024

Seems to be working as intended to me, no?

$ tree
./
├── dir_bar/
│   └── foo -> ../dir_foo/
├── dir_baz/
│   └── bar -> ../dir_bar/
└── dir_foo/
    └── foo.txt

6 directories, 1 file

$ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> require('../').globSync('dir_baz/**', {follow:true})
[
  'dir_baz',
  'dir_baz/bar',
  'dir_baz/bar/foo',
  'dir_baz/bar/foo/foo.txt'
]
> require('../').glob('dir_baz/**', {follow:true}).then(console.log)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 143,
  [Symbol(trigger_async_id_symbol)]: 132
}
> [
  'dir_baz',
  'dir_baz/bar',
  'dir_baz/bar/foo',
  'dir_baz/bar/foo/foo.txt'
]
> require('../').glob('dir_baz/**', {follow:true, nodir:true}).then(console.log)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 257,
  [Symbol(trigger_async_id_symbol)]: 246
}
> [ 'dir_baz/bar', 'dir_baz/bar/foo', 'dir_baz/bar/foo/foo.txt' ]

> require('../').globSync('dir_baz/**', {follow:true, nodir: true})
[ 'dir_baz/bar', 'dir_baz/bar/foo', 'dir_baz/bar/foo/foo.txt' ]

@pamelalozano16
Copy link
Author

In the given example, shouldn't > require('../').globSync('dir_baz/**', {follow:true, nodir: true}) return
[ 'dir_baz/bar/foo/foo.txt' ] only and not [ 'dir_baz/bar', 'dir_baz/bar/foo', 'dir_baz/bar/foo/foo.txt' ]?

I see that if (ifDir && this.opts.nodir) return undefined uses .isDirectory() function from path_scurry, checks if the current .path, is a directory. However, when .path is a symlink that points to a directory it is not skipped.
The path is still added to this.matches, since ifDir equals false.

Repository owner deleted a comment from frontzhm Mar 27, 2024
@isaacs
Copy link
Owner

isaacs commented Mar 27, 2024

@pamelalozano16 Yeah, this is interesting. It's not actually a directory, so maybe it should be included. But you've set follow: true, so maybe we should be testing the target rather than the entry itself. I'm somewhat torn.

@frontzhm 请在此处的评论中使用英语。

@nex3
Copy link

nex3 commented Mar 27, 2024

+1 for treating symlinks to directories as directories with follow: true. I think the expectation when that option is passed is that symlinks become fully transparent representations of whatever they link to as much as possible, and doing otherwise is likely to break users who aren't thinking carefully about edge cases. This also matches the behavior of earlier versions of glob, and so is less likely to cause breakages for users upgrading.

@isaacs
Copy link
Owner

isaacs commented Mar 27, 2024

@nex3 Yeah, I've noodled on it more since posting that comment earlier today, and I think you're right. follow:true should be "symlinks are treated as their targets in every way", not "follow symlinks, but still treat them differently than directories".

@isaacs isaacs closed this as completed in ea7cf5f Mar 28, 2024
@isaacs
Copy link
Owner

isaacs commented Mar 28, 2024

Will publish once CI says it's ok

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

No branches or pull requests

3 participants