Filesystem2 #4193
Closed
Filesystem2 #4193
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sometimes for clarity, other times for consistency of formatting and/or wording. (Examples: after periods there were sometimes two spaces, sometimes one. Sometimes asynch functions "resolved to" a value, other times "resolved with"...) Aimed also to copy JSDoc changes introduced by denoland#4170 over to the corresponding TypeScript modules. Many of these comments will be changed again by later commits in this sequence of PRs that refine/extend the interface or function being described. Not sure how the `// @url js/...` directives work, but these seemed to be inconsistently present, so I added them where I noticed. Didn't try to clean up everything, just the filesystem modules I was looking at. Note: The comment on the `FileInfo.created` field replaces `Unix` with `Mac/BSD`. The Rust libs do handle this correctly for Linux, but the story about how they do so there is more complicated (see https://doc.rust-lang.org/std/fs/struct.Metadata.html#method.created).
More comment cleanup, in same spirit as previous commit. Separated because these touch parts of lib.deno.ns.d.ts that I wasn't working on, but got introduced just because of search/replacing in attempt to make things consistent.
Various supposed-to-be innocuous changes. Shouldn't have semantic effects (that anyone cares about). For example: `#[cfg(any(unix))]` attributes -- the `any` does nothing here.
Changed one use of std::fs to just fs, and uses of tokio::fs to tokio_fs.
The point of cli/rs.rs seemed to be to collect all the implementations for filesystem operations that depend on third-party modules. So I moved remove_dir_all, set_file_times there. Also unix::fs::symlink.
Comments in the code proposed this might be coming. readdir is the spelling used by the underlying libc call, and by Node.
It was hard to remember which Options interfaces were spelled in the singular and which in the plural (and anyway this one contained two options). Also added MkdirOptions to cli/js/deno.ts. All the analogous interfaces were exported there.
This was the last remaining Option interface spelled in the singular. Easier to remember if they're all Option**s**; plus we may want to add extra options here later.
There's a lot of variation in doc comments and internal code about whether the first parameter to file system calls is `path` or `name` or `filename`. For consistency, have made it always be `path`. Changes to: files.ts, readLink, utime, stat, truncate, writeFile, readFile
The previous implementation would clobber anything Rust calls `permissions` that aren't visible to Unix chmod (for example, `metadata.permissions().readonly()`). Replaces with the method recommended at https://doc.rust-lang.org/std/os/unix/fs/trait.PermissionsExt.html#tymethod.set_mode.
There's a lot of variation in doc comments and internal code about whether chmod/0o777-style permissions are called `mode` or `perm`. (For example, mkdir and writeFile choose differently.) `mode` is also used in other ways, for example to label whether a file is being opened as `r+`, `w`, etc. I've made the first things consistently be called `perm`. I left the field in the (internal) interface StatResponse named `mode`, as it contains more than just file permissions. (It has all the bits from st_mode.) Changes to: mkdir, chmod, FileInfo, and tests/other modules that depend on them.
These changes are speculative: since the core API `FileInfo.mode` was renamed to `perm` in a previous commit, perhaps we want to change the `fileMode` field in tar headers to `filePerm`? And rename the `EntryInfo.mode` field in std/http/file_server to `perm` too? (Also make corresponding changes to labels and errors in that server's output.)
This still needs tests (they'd have to be integration tests to be very effective). In later commits we change permission tests for some other operations to check that the process's umask is being applied. There we assume the test machine is running with the usual default umask 0o022 (as Github's CI machines do); we could also query it using this function. On Windows, `umask(value)` currently throws, but `umask()` just returns 0. Should we make the latter throw too?
Verify that the `perm` supplied to mkdir is subject to the process's umask (here just assumed to be the default 0o022 on Unix, or 0 on Windows).
Add `perm` option for makeTempDir and makeTempFile. As with mkdir, this argument is only used when a new dir or file is created. Defaults to 0o700 for directories and 0o600 for files. We make the tests verify that the `perm` used is subject to the process's umask (here just assumed to be the default 0o022 on Unix, or 0 on Windows).
Add `perm` option for truncate. As with mkdir and the makeTemp* functions, this argument is only used when a new file is created. Defaults to 0o666. We make the tests verify that the `perm` used is subject to the process's umask (here just assumed to be the default 0o022 on Unix, or 0 on Windows). The comments say "error to specify perm when create is set to `false`"; but the create option is only introduced in a later commit.
Add `perm` option for open and create. As with mkdir, makeTemp*, and truncate, this argument is only used when a new file is created. Defaults to 0o666 (the process's umask will then be applied). Currently we throw if `perm` is specified but a read-only opening mode was requested. Need to decide how to handle `perm` on Windows: NOOP? or throw? The codebase doesn't seem to be consistent on this.
This is the first in a series of commits adding a `clobber` option to
filesystem operations. The term `clobber` comes from the shell's
`noclobber` option.
Mostly, `clobber: false` is just another way to write `createNew: true`.
There are some cases though where this functionality could be useful but
it'd be unnatural to use `createNew` to express it. For example,
`rename(oldpath, newpath, { clobber: false })` (not yet implemented)
could fail if there's already a file at `newpath`, instead of the
current (default) behavior of overwriting newpath.
Implement createNew for writeFile (also the variant spelling `clobber`, with inverse boolean value). This introduces a change of functionality. To make the operation more atomic, and conform better to other parts of the API, writeFile will now only apply `perm` if it creates a new file, not if it overwrites or appends to an existing file. Previously, writeFile did a chmod on the target file no matter what. If that's the behavior a user wants, they can do it easily enough with chmod. (And it never was atomic.)
We verify that the `perm` applied by writeFile are only used when creating a new file; also that they are subject to the process's umask (here just assumed to be the default 0o022 on Unix, or 0 on Windows). Perhaps this test should be moved from stat_test.ts to write_file_test.ts?
Add create and createNew (and clobber) options for copyFile. These work
as for writeFile. The fastest implementation is used when { create:
true, createNew: false } (this is the default, and matches the older
implementation).
To implement createNew atomically, we have to fall back to what I think
is a slower implementation, where the data gets copied from kernel space
to user space, and back.
Add create and createNew (and clobber) options for truncate. These work as for writeFile and copyFile.
This was referenced Feb 29, 2020
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Filesystem Permission commits for #4017 (PR 2 of 4).
This is based upon PR #4192.