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.

Sign up for GitHub

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

Jump to bottom

fs: consistently return symlink type from readdir #22808

Closed
wants to merge 1 commit into from
Closed

fs: consistently return symlink type from readdir #22808

wants to merge 1 commit into from

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Sep 11, 2018

Use 'lstat' to determine type of directory entry.
This is more consistent with the type returned from the readdir binding.
Also use 'path.join' over 'path.resolve' because 'name' is not absolute.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 11, 2018
@ajafff
fs: consistently return symlink type from readdir
6197ca2 
Use 'lstat' to determine type of directory entry.
This is more consistent with the type returned from the readdir binding.
Also use 'path.join' over 'path.resolve' because 'name' is not absolute.
@danbev
Copy link
Contributor

danbev commented Sep 19, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17315/

@addaleax addaleax requested a review from bengl September 24, 2018 20:45
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 24, 2018
@addaleax
Copy link
Member

I’m wondering whether this should be semver-major and/or an optional feature? There’s definitely a valid use case for having the non-lstat version also available…

@jasnell
Copy link
Member

jasnell commented Sep 25, 2018

good question... I have no idea.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 25, 2018

We could keep the existing behavior, and add an option for the lstat behavior. Then, in the next major release, potentially adjust which one is the default behavior.

@ajafff
Copy link
Contributor Author

ajafff commented Sep 25, 2018

I don't want to introduce any new behavior in this PR. This tries to correct an inconsistency:
AFAICT the native binding returns the symlink's type as if lstat was used behind the scenes. Only if it returns an unknown kind it uses stat (in JS) to retrieve the missing information.

IMO this qualifies as semver-patch

@bengl
Copy link
Member

bengl commented Sep 25, 2018

I think lstat is the correct behavior, since the Dirent objects provide isSymbolicLink() and those need to be correct. This is semver-patch, since the previous behavior was inconsistent across platforms when directories contain symlinks.

If stat behavior is preferred, then the Dirent class should be changed to behave consistently. Either way, that would be a separate change from this one.

@addaleax
Copy link
Member

Thanks for the explanations, semver-patch sounds right then!

mhdawson
mhdawson approved these changes Sep 27, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

CI run: https://ci.nodejs.org/job/node-test-pull-request/17471/

@mhdawson
Copy link
Member

Opened issue for failure on Windows: #23127

@mhdawson
Copy link
Member

mhdawson commented Sep 27, 2018 •
edited
Loading

Resumed build and looks good except for one test that was yellow. Going to land. Link for resumed job https://ci.nodejs.org/job/node-test-pull-request/17473/

@mhdawson
Copy link
Member

Landed as 83864b3

@mhdawson mhdawson closed this Sep 27, 2018
mhdawson pushed a commit that referenced this pull request Sep 27, 2018
@ajafff @mhdawson
fs: consistently return symlink type from readdir
83864b3 
Use 'lstat' to determine type of directory entry.
This is more consistent with the type returned from the readdir binding.
Also use 'path.join' over 'path.resolve' because 'name' is not absolute.

PR-URL: #22808
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Sep 27, 2018
@ajafff @targos
fs: consistently return symlink type from readdir
58490d5 
Use 'lstat' to determine type of directory entry.
This is more consistent with the type returned from the readdir binding.
Also use 'path.join' over 'path.resolve' because 'name' is not absolute.

PR-URL: #22808
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
@ajafff @targos
fs: consistently return symlink type from readdir
bb6530b 
Use 'lstat' to determine type of directory entry.
This is more consistent with the type returned from the readdir binding.
Also use 'path.join' over 'path.resolve' because 'name' is not absolute.

PR-URL: #22808
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
This was referenced Oct 11, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@bpasero bpasero mentioned this pull request Aug 10, 2019
Closed
bpasero referenced this pull request in microsoft/vscode Sep 10, 2019
@bpasero
Revert "debt - adopt new fs.readdir with stat info"
956208b 
This reverts commit da5fb7d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bengl bengl bengl approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ajafff @danbev @addaleax @jasnell @cjihrig @bengl @mhdawson @nodejs-github-bot