Refactor fs.rs and ops/fs.rs #4410

dubiousjim · 2020-03-17T18:18:30Z

This a complex boring PR that shifts around code (primarily) in cli/fs.rs and cli/ops/fs.rs for no change in functionality. (Though see the commit messages, especially on the last commit re the spelling of "not implemented" error messages.)

The gain of this refactoring is to ease the way for #4188 and #4017, and also to avoid some future development pain. Also, we move everything from cli/fs.rs that's only called by cli/ops/fs.rs to the latter location, and we drop the need for one external crate (remove_dir_all).

Highly recommend reviewing this one commit at a time, I organized them so as to make the reviewer's job easier.

A PR for #4188 and more PRs for the series #4017 are waiting to be pushed as soon as this one is merged.

@ry

Instead of having these use declarations at the top, I follow the lead of @ry in denoland#4270 (commit b4a6fbb) and move them to the sites where they're used. This helps immensel with refactoring, and also makes it easier to see when/why they're needed.

This helps refactoring, especially when std::fs calls are replaced with tokio::fs calls (denoland#4188)

Updated cli/Cargo.toml and Cargo.lock to remove dependency on external remove_dir_all crate. Had to add an await to the use of the (async) function remove in a finally clause in std/fs/walk_test: testWalk, else some of those tests would fail on Windows when we switched from remove_dir_all::remove_dir_all to std::fs::remove_dir_all.

When we shift from std::fs::read_dir to the tokio::fs version (denoland#4188), the processing of entries will be an async function, and Rust doesn't currently have a stable implementation of async closures. So instead of using `filter_map` with a closure, we iterate and build the Vec by hand. This makes the shift to tokio::fs more straightforward.

@ry

…ented" error Following the lead of @ry in denoland#4270 (commit b4a6fbb), I wanted to shift from OpError::other("Not implemented") in op_symlink to OpError::not_implemented. But this triggered an awkward cascade having to do with our unsystematic spelling of errors generated in different places. The OpError::not_implemented (following our other Rust errors) is uncapitalized. But then some other errors for Deno.symlink come from cli/js/util.ts: notImplemented(), where the error was capitalized. Changing that to lower case for consistency in the tests (and to keep from having to guess the capitalization depending on the error) required changing the capitalization of checks against the error in cli/js/tests/remove_test.ts and std/fs/ensure_symlink_test.ts. That's enough to stop this cascade. But really we ought to either: (1) settle on a consistent formatting/spelling scheme for our error messages, whether they come from Rust, or cli/js (or presumably std too). And/or: (2) make the checks against the errors in our tests case-insensitive.

dubiousjim · 2020-03-17T23:12:26Z

@cknight, maybe keep an eye on how things end up with the capitalization of "not implemented" errors (last commit in this PR, at least atm) and you may want to adjust notImplemented in std/node/_utils.ts to correspond.

dubiousjim · 2020-03-18T00:05:17Z

This also helped me spot two bugs (will push a commit shortly that fixes them). In op_utime, the path wasn't being passed through resolve_from_cwd. In op_chown, the path is being passed through that function, but then the result is discarded and the original value is used instead.

Current master:

fn op_chown(
  state: &State,
  args: Value,
  _zero_copy: Option<ZeroCopyBuf>,
) -> Result<JsonOp, OpError> {
  let args: ChownArgs = serde_json::from_value(args)?;
  let path = deno_fs::resolve_from_cwd(Path::new(&args.path))?;

  state.check_write(&path)?;

  let is_sync = args.promise_id.is_none();
  blocking_json(is_sync, move || {
    debug!("op_chown {}", path.display());
    deno_fs::chown(args.path.as_ref(), args.uid, args.gid)?;
    Ok(json!({}))
  })
}

When invoked on Windows, this op now throws (see denoland#4306)

This reverts commit 618aaeb.

dubiousjim · 2020-03-18T10:50:47Z

Tests seem now to be passing, except for the dreaded #4373.

ry

Generally this PR looks fine. I'm glad it removes a crate dep, I'm glad it has net negative code, and I'm glad that it removes unnecessary indirection to cli/fs.rs. So thank you for that.

But it's quite difficult to review a bunch of unrelated changes - especially when it contains unnecessary bits code moving around.

The spelling of "Not implemented" is not related to fs changes.
The rewriting of the read_dir loop apparently has no effect
This changes how chmod (and chown) work on windows?

I don't mind if one sneaks in a few lines of unrelated changes in a PR, if it's an obvious change and faster than submitting a new one.
Each PR in Deno gets squashed into one commit. I can't squash this into one commit because it's doing too many different things. I can't even rebase it, because it's 26 commits. For that reason I'm closing without merge.

ry · 2020-03-18T18:51:42Z

cli/ops/fs.rs

+        // entries.push(get_stat_json(metadata, filename).unwrap());
+        entries.push(get_stat_json(metadata, filename)?);
+      }
+    }


What's the purpose of this change?

Isn't it the case that if there is a failed return (Err(_) or None, forget what the type is here), that unwrap() will panic? But ? will coerce it to the Err type of the containing function.

dubiousjim · 2020-03-18T19:24:56Z

Ok, am happy to break it into smaller pieces.

The spelling of "Not implemented" is not related to fs changes.

Will make that a separate commit.

The rewriting of the read_dir loop apparently has no effect

Will reserve that for the PR that moves from std::fs to tokio::fs. (The latter has to have the structure of the rewritten read_dir loop, and then just changes std::fs::blah(...)?; to tokio::fs::blah(...).await?;

This changes how chmod (and chown) work on windows?

I don't think it changed the behavior of chown, only of chmod. But I can appreciate this should be a separate commit.

dubiousjim · 2020-03-18T19:33:15Z

@ry, apart from those three bulleted items (and anything else that feels similar), would it be okay to have something like the rest of this in a single PR? I'm imagining pruning this back a bit so that it should make no difference in behavior whatsoever (and aiming to reserve the stuff whose usefulness is only demonstrated in later PRs until then). But it will still be big and complex. It's hard to separate out a lot of inter-related refactoring into independent PRs. (And then doubly hard to keep it in your head while waiting for it to be reviewed, revised, etc.)

dubiousjim added 14 commits March 17, 2020 12:58

Move make_temp, mkdir, set_dir_permission from fs.rs to ops/fs.rs

c5ba81f

ops/fs.rs: inline set_dir_permission into mkdir, make_temp

7c21e5a

fs.rs: inline set_permissions, improve debug!

abfeb18

ops/fs.rs: refactor symlink

a9c9421

fs.rs, ops/fs.rs: use std::env::{current_dir, set_current_dir, temp_dir}

6dbc318

Move chown from fs.rs to ops/fs.rs

5f0b978

ops/fs.rs: inline chown, improve debug!

45d3842

ops/fs.rs: use deno_fs::resolve_from_cwd

e46389d

fs.rs, ops/fs.rs: drop use std, use std::fs

1df12ff

This helps refactoring, especially when std::fs calls are replaced with tokio::fs calls (denoland#4188)

ops/fs.rs: refactor chmod

833e17a

dubiousjim added 10 commits March 17, 2020 22:01

ops/fs.rs: utime: apply resolve_from_cwd to path

532a6d2

ops/fs.rs: chmod: refactor, query metadata only on Windows (then throw)

49619ad

When invoked on Windows, this op now throws (see denoland#4306)

ops/fs.rs: chown: refactor to parallel chmod, query metadata on Windows

fcf4c9a

symlink,link: oldname->oldpath, newname->newpath

ca31562

ops/fs.rs: symlink: refactor to parallel chmod/chown

6a4730c

leftover

618aaeb

Revert "leftover"

d1875e6

This reverts commit 618aaeb.

ops/fs.rs: fix unreachable code on windows

3d82bc5

fix tests that fail on Windows when chmod throws

cdfbcb8

fixup

b5bede8

dubiousjim added 2 commits March 18, 2020 06:51

Merge branch 'master' into middle1

2e3d33e

mkdir: stray comment

61d21d9

ry reviewed Mar 18, 2020

View reviewed changes

ry closed this Mar 18, 2020

dubiousjim mentioned this pull request Mar 19, 2020

Refactoring fs.rs and ops/fs.rs, try 2 #4428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fs.rs and ops/fs.rs #4410

Refactor fs.rs and ops/fs.rs #4410

dubiousjim commented Mar 17, 2020 •

edited

Loading

dubiousjim commented Mar 17, 2020

dubiousjim commented Mar 18, 2020

dubiousjim commented Mar 18, 2020

ry left a comment •

edited

Loading

ry Mar 18, 2020

dubiousjim Mar 18, 2020

dubiousjim commented Mar 18, 2020

dubiousjim commented Mar 18, 2020

Refactor fs.rs and ops/fs.rs #4410

Refactor fs.rs and ops/fs.rs #4410

Conversation

dubiousjim commented Mar 17, 2020 • edited Loading

dubiousjim commented Mar 17, 2020

dubiousjim commented Mar 18, 2020

dubiousjim commented Mar 18, 2020

ry left a comment • edited Loading

Choose a reason for hiding this comment

ry Mar 18, 2020

Choose a reason for hiding this comment

dubiousjim Mar 18, 2020

Choose a reason for hiding this comment

dubiousjim commented Mar 18, 2020

dubiousjim commented Mar 18, 2020

dubiousjim commented Mar 17, 2020 •

edited

Loading

ry left a comment •

edited

Loading