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

doc: document opening hidden files on Windows #15409

Closed
wants to merge 5 commits into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Sep 14, 2017

Fixes: #14553

Checklist
Affected core subsystem(s)

doc

@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 Sep 14, 2017
doc/api/fs.md Outdated
@@ -102,6 +102,11 @@ example `fs.readdirSync('c:\\')` can potentially return a different result than
`fs.readdirSync('c:')`. For more information, see
[this MSDN page][MSDN-Rel-Path].

*Note:* On Windows opening existing hidden file using the `w` flag (either
through `fs.open` or `fs.writeFile`) will fail with `EPERM`. You can work
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: fs.open(), fs.writeFile() and fs.ftruncate()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be either opening an existing hidden file or opening existing hidden files.

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid the use of informal pronouns like you in the docs

doc/api/fs.md Outdated
*Note:* On Windows opening existing hidden file using the `w` flag (either
through `fs.open` or `fs.writeFile`) will fail with `EPERM`. You can work
around this by opening the file with the `a` flag and then using `fs.ftruncate`
to clear the file contents.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does ftruncate rewind the current file position to the beginning?

doc/api/fs.md Outdated
@@ -102,6 +102,11 @@ example `fs.readdirSync('c:\\')` can potentially return a different result than
`fs.readdirSync('c:')`. For more information, see
[this MSDN page][MSDN-Rel-Path].

*Note:* On Windows opening existing hidden file using the `w` flag (either
Copy link
Member

Choose a reason for hiding this comment

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

nit: comma after On Windows

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there. Left a couple comments.

@bzoz
Copy link
Contributor Author

bzoz commented Sep 18, 2017

Updated, PTAL

@bnoordhuis
Copy link
Member

Ping @jasnell. I think your comments have been addressed.

Copy link
Contributor

@seishun seishun left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

doc/api/fs.md Outdated
*Note:* On Windows, opening an existing hidden file using the `w` flag (either
through `fs.open` or `fs.writeFile`) will fail with `EPERM`. Hidden files
can be opened for writing with either `a` or `r+` flag. A call to
`fs.truncate` can be used to reset the file contents.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't truncate a file opened in 'a' mode.

> var fd = fs.openSync('test.txt', 'a')
undefined
> fs.truncateSync(fd)
Error: EPERM: operation not permitted, ftruncate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, looks like on Windows you need to open the file with r+ for truncate to work. That is unfortunate, there is no easy way to reproduce Linux behavior. One would need to call truncate with filename, ignore the error if the file does not exists, and then open the file with a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the best thing is to just document that?

@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

@bzoz would you be so kind and address the comment? This is only blocked by that.

@bzoz
Copy link
Contributor Author

bzoz commented Oct 2, 2017

Updated, PTAL

@seishun
Copy link
Contributor

seishun commented Oct 2, 2017

Rather that opening the file twice, I think we should instead suggest opening in r+ once, then using ftruncate on the fd.

@bzoz
Copy link
Contributor Author

bzoz commented Oct 2, 2017

@seishun r+ will fail if the file does not exists.

@seishun
Copy link
Contributor

seishun commented Oct 2, 2017

@bzoz fs.truncate will also fail if the file does not exist.

@bzoz
Copy link
Contributor Author

bzoz commented Oct 2, 2017

Sure, but then it will be created by fs.open. In the end, after truncate and open with a you will have a empty file opened for writing, even if it existed and was hidden.

@seishun
Copy link
Contributor

seishun commented Oct 2, 2017

If we want to suggest only the approach that works for both existing and non-existing files, then "r+" shouldn't be suggested at all. But since people often know beforehand whether the file exists, I would prefer describing both approaches.

@bzoz
Copy link
Contributor Author

bzoz commented Oct 2, 2017

Hm, file needs to exist to be "hidden". So maybe we should only describe r+?

@bzoz
Copy link
Contributor Author

bzoz commented Oct 3, 2017

Updated. PTAL

@seishun
Copy link
Contributor

seishun commented Oct 3, 2017

Since the suggested approach now only works on existing files, perhaps it should say fs.ftruncate instead of fs.truncate, since the fact that the latter works with fds is notpoorly documented. (I was actually surprised by this behavior, I'm going to submit an issue about it)

@BridgeAR
Copy link
Member

Landed in 4f1d548

@BridgeAR BridgeAR closed this Oct 19, 2017
BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
PR-URL: #15409
Fixes: #14553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15409
Fixes: #14553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15409
Fixes: nodejs/node#14553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15409
Fixes: nodejs/node#14553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Can't open hidden files in 'w' mode on Windows - document, fix, or do nothing?
9 participants