Separate st_mode into FileInfo.type/mode #4448
Conversation
We add a FileInfo.type enum, that tracks the hi bits of st_mode. FileInfo.mode is now masked to 0o7777.
|
I don't want to pester anyone, because I know we're all busy right now. But I did want to record that this PR is blocking a number of other, more substantial and important ones that I've got waiting, wrt #4017 and #4188. If it's decided to pass on this PR for now, I can rebase around that, but so long as this is in limbo I can't push the others. #4337 is another PR in the #4017 series that's now still open, but it's fine for that one to wait; it doesn't interact much with other things. |
* denoland/master: Ignore flaky test cafile_info (denoland#4517) fix(inspector): proper error message on port collision (denoland#4514) feat: Added colors to doc output (denoland#4518) v0.38.0 feat: Add "deno doc" subcommand (denoland#4500) Update to Prettier 2 and use ES Private Fields (denoland#4498) upgrade: dprint 0.9.6 (denoland#4509) upgrade: rusty_v8 to v0.3.9 (denoland#4505) feat: Support Inspector / Chrome Devtools (denoland#4484) Improve isatty and kill API docs; Deno.kill() - throw on Windows (denoland#4497) refactor: rename ConsoleOptions to InspectOptions (denoland#4493) upgrade: dprint 0.9.5 (denoland#4491) feat: window.close() (denoland#4474) errors: replace .lines with explicit .split newline (denoland#4483) doc: improve various API docs and include examples (denoland#4486) hide source line if error message longer than 150 chars (denoland#4487) fix: add fsEvent notify::Error casts (denoland#4488)
| @@ -1229,8 +1229,10 @@ declare namespace Deno { | |||
| /** **UNSTABLE**: Match behavior with Go on Windows for `mode`. | |||
| * | |||
| * The underlying raw `st_mode` bits that contain the standard Unix | |||
| * permissions for this file/directory. */ | |||
| * permissions for this file/directory (masked to `0o7777`). */ | |||
There was a problem hiding this comment.
No, we don't mask out the suid/guid/sticky bits.
| TYPE_CHARDEV = 2 << 12, | ||
| TYPE_BLKDEV = 6 << 12, | ||
| TYPE_SOCKET = 12 << 12, | ||
| } |
There was a problem hiding this comment.
These are platform dependent (?) and largely covered by FileInfo.isDirectory() , isSymlink(), isFile()... It's not clear to me it's desirable to expose such APIs.
There was a problem hiding this comment.
I can't speak authoritatively, but from the best I've been able to research, they don't vary (at least not between Linux and Mac).
I guess this PR does three things: (a) stop assuming that isDirectory, isSymlink, and isFile exhaust all the options (so don't define one of them as meaning "neither of the other two"); (b) expose a version of st_mode which is masked to have only the permission bits, so that we don't have to write & 0o777 everytime we work with a FileInfo.mode; (c) expose the high bits of st_mode as an enum.
(c) concerns me the least, we could always add that back in later. Or I could create isCharacterDevice etc booleans on the StatResponse (in a way that uses the platform's own headers, in case these do vary). But I'm happy to take the enum out and leave this info unexposed, at least for now, if you think that's better.
I would still advocate for (b) though, and especially for (a).
There was a problem hiding this comment.
at least not between Linux and Mac
It's windows I'm concerned about.
There was a problem hiding this comment.
I handled that by manually assigning the enum values when the StatResponse.mode field is null, based on the isFile etc fields.
There was a problem hiding this comment.
Sorry @dubiousjim for the delay - this one slipped through the cracks.
With these changes are you in need of discovering if a file is a block device, or socket, etc? Generally we're trying to expose platform independent APIs - which often take a bit more thought. In particular I'd like to research how the APIs look on other platforms...
| ? FileType.TYPE_DIRECTORY | ||
| : res.isSymlink | ||
| ? FileType.TYPE_SYMLINK | ||
| : null; |
There was a problem hiding this comment.
In POSIX and Go these flags are left in the same field (st_mode). I think we ought to just do the same.
There was a problem hiding this comment.
Ok, how about I evolve this PR to suppress all the parts except what I called (a) before (don't treat the three options as being exclusive).
There was a problem hiding this comment.
I'm not sure what that means. Those methods, isDirectory(), etc are in the Go API - I think they should stay. Their existence doesn't imply they are exhaustive.
There was a problem hiding this comment.
cli/js/file_info.ts now has:
isDirectory(): boolean {
return !this.#isFile && !this.#isSymlink;
}
which is wrong --- it counts fifos etc as directories. And to fix this, the Rust side has to provide more info in the StatResponse.
There was a problem hiding this comment.
We need the StatResponse to also have a isDirectory boolean, and then to stash that as a private var on the FileInfo too.
There was a problem hiding this comment.
Indeed, that is wrong. Your change, adding isDir to the StatResponse is better.
I just don't agree with the further change of adding this new "type" member.
There was a problem hiding this comment.
Ok, I'll push more commits to this PR Monday evening that roll back most of the changes besides those. Then I have to rework the rest of my queue to cohere with that. But once a later form of this PR is merged, I should be able to push 5 more substantive PRs (adding create/createNew in more places). After that, the highlights are: the fstat etc ops, broader nofollow functionality (as in lstat vs stat), and openat etc. functions. So many of the patches are so interdependent, they have to keep coming in steps.
|
Closing in favor of #4541 |
We add a FileInfo.type enum, that tracks the hi bits of st_mode.
FileInfo.mode is now masked to 0o7777.