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

docs: fs: stat.isDirectory: added clarification #27413

Closed
wants to merge 2 commits into from
Closed

docs: fs: stat.isDirectory: added clarification #27413

wants to merge 2 commits into from

Conversation

coderaiser
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Added clarification to documentation, similar to one describes stat.isSymbolicLink which tells that
This method is only valid when using fs.lstat().

When use fs.lstat for a symbolic link to a directory it will return stat which isDirectory will always be false. This is not really obvious, and because of this both methods stat and lstat should be called to get true information about symbolic link to a directory.

Let's create symbolic link to root directory.

ln -s / 123
node

And then run in REPL:

> fs.statSync('123').isDirectory()
true
> fs.statSync('123').isSymbolicLink()
false
> fs.lstatSync('123').isDirectory()
false
> fs.lstatSync('123').isSymbolicLink()
true

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Apr 25, 2019
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Sorry, this documentation isn't correct.

I'm not clear why you think it's not valid when using lstat(). lstat gets information for the link itself, and the link is not a directory (it's a link). From your code, I could equally (wrongly) say that isDirectory() is invalid when using stat(), because when you call stat('123'), 123 is not a directory, its a link, so isDirectory() should be false (but I wouldn't say that! it ignores the stat docs, which says it follows links).

You are also missing a test in the code you posted: call lstat() on /, and then call isDirectory, it will return true, which is valid, in contradiction of the sentence you added.

Is it possible you are just confused about the difference between stat and lstat? Perhaps some additional text is needed there? Or did you miss this sentence?

lstat() is identical to stat(), except that if path is a symbolic link, then the link itself is stat-ed, not the file that it refers to.

@coderaiser
Copy link
Contributor Author

coderaiser commented Apr 25, 2019

The thing is I'm working on a file manager, and use readify to get file list with attributes, such as size, type, mode, date.
There is regular files, and directories.
There is symlinks to files, and symlinks to a directories.
How do you think the last two should be distinguished if stat returns information about type (file/directory) and do not return information about is it symbolic link. and lstat do this vice versa.

I have stepped on this rake a lot times, so I propose to clarify this thing in documentation because it is not obvious. I understand the difference, but I don't know why do I should always remember this :). If everyone else always remember the difference, we can close the issue, I just thought that it's a good idea for improvement and that it can help people who see this stat, lstat stuff for the first time.

Also superstat can help not think at all about this.

@sam-github
Copy link
Contributor

Sorry, still not quite understanding.

There is regular files, and directories.
There is symlinks to files, and symlinks to a directories.
How do you think the last two should be distinguished if stat returns information about type (file/directory) and do not return information about is it symbolic link. and lstat do this vice versa.

Most users don't want to know a path is a symlink, they want whatever they do to happen to the thing pointed to. In other words, whether they open (get stat, whatever) usgin a path to a file or to a symlink to a file, they don't want to know. fs.stat() (and fs.open()), etc., are all built for this kind of user.

If you are writing code that actually needs to know both that a path is a symlink, and what the link points to, then you need to make two stat calls. Always use fs.lstat() first, and if fs.isSymbolicLink() is true, (and you care about the file it links to), then call fs.stat() on the same path, and now you know everything there is to know about the two entities.

If I understand you correctly, you are hoping for some kind of "fs.Stats" structure to be returned that tells you both about the entity pointed to (its size, its inode, the device it is on, its creation time, owner, etc), and whether the path used to get that info went through symlink resolution. That's just not possible. Not structurally, because each property can have a different value for the symlink vs the entity it points to, so the structure doesn't work. Also, not possible because the POSIX system call to do a one-shot "get info on two things" don't exist.

In terms of naming, "lstat" gets the stat for the link itself ("l"), "stat" gets the stat for what the link points to, as most people want. I'm not going to say the names couldn't be more memorable, but they exactly mirror the UNIX system API names, so using the traditional name seems better than inventing a new one, and is consistent with the rest of the fs API.

I'm also not going to say the docs couldn't get better, I'm sure they can, but this particular change is inaccurate. The lstat vs stat distinction applies to ALL of fs.Stat, but you have singled out isDirectory as if its somehow special (it is not).

Perhaps a note right at the top of the fs.Stat class docs that refers to the two APIs that return it (stat and lstat), and mention that depending on the API used, Stat will either refer to the link itself, or the entity it points to, would be a more helpful change.

doc/api/fs.md Outdated Show resolved Hide resolved
@coderaiser
Copy link
Contributor Author

@sam-github I updated commit in accordance to changes you requested.

@sam-github
Copy link
Contributor

The lstat vs stat distinction applies to ALL of fs.Stat, but you have singled out isDirectory as if its somehow special (it is not).

Your text is more clear, thank you, but the above problem is not fixed.

I suggest moving the text, as I requested:

Perhaps a note right at the top of the fs.Stat class docs that refers to the two APIs that return it (stat and lstat), and mention that depending on the API used, Stat will either refer to the link itself, or the entity it points to, would be a more helpful change.

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@HarshithaKP
Copy link
Member

What are the next steps on this PR ? and this needs a rebase.

@coderaiser
Copy link
Contributor Author

What are the next steps on this PR ? and this needs a rebase.

Rebase is done :).

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jul 3, 2020

@coderaiser ... the "merge commit" needs to be dropped. We do not use merge commits. Instead we use git rebase instead. Let us know if you need some help with that.

@coderaiser
Copy link
Contributor Author

coderaiser commented Jul 4, 2020

@jasnell rebase is done, now without merge commit :)

@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

Ping @sam-github ... does this look good to you now?

doc/api/fs.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 9, 2020
aduh95 pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #27413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

Landed in 6c3326c

@aduh95 aduh95 closed this Nov 9, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #27413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@coderaiser coderaiser deleted the patch-1 branch November 9, 2020 19:47
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #27413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #27413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #27413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants