From 315e4abd7e88c428e78c006ccf2dfdb499911a05 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 17 Jan 2019 23:39:06 -0500 Subject: [PATCH] mkdir should not be recursive by default (#1530) It should return an error if a file with the given path exists and recursive isn't specified. Because mode is not used on windows and rarely used in unix, it is made to the last parameter. In collaboration with Stefan Dombrowski --- js/mkdir.ts | 29 ++++++++++++++++++++++------- js/mkdir_test.ts | 33 +++++++++++++++++++++++++++++---- js/read_link_test.ts | 4 ++-- js/rename_test.ts | 4 ++-- js/symlink_test.ts | 4 ++-- src/deno_dir.rs | 10 ++++++---- src/fs.rs | 9 +++------ src/msg.fbs | 7 +++---- src/ops.rs | 6 ++++-- 9 files changed, 73 insertions(+), 33 deletions(-) diff --git a/js/mkdir.ts b/js/mkdir.ts index 8040bd51b8..2981976ab3 100644 --- a/js/mkdir.ts +++ b/js/mkdir.ts @@ -3,33 +3,48 @@ import * as msg from "gen/msg_generated"; import * as flatbuffers from "./flatbuffers"; import * as dispatch from "./dispatch"; -/** Creates a new directory with the specified path and permission - * synchronously. +/** Creates a new directory with the specified path synchronously. + * If `recursive` is set to true, nested directories will be created (also known + * as "mkdir -p"). + * `mode` sets permission bits (before umask) on UNIX and does nothing on + * Windows. * * import { mkdirSync } from "deno"; * mkdirSync("new_dir"); + * mkdirSync("nested/directories", true); */ -export function mkdirSync(path: string, mode = 0o777): void { - dispatch.sendSync(...req(path, mode)); +export function mkdirSync(path: string, recursive = false, mode = 0o777): void { + dispatch.sendSync(...req(path, recursive, mode)); } -/** Creates a new directory with the specified path and permission. +/** Creates a new directory with the specified path. + * If `recursive` is set to true, nested directories will be created (also known + * as "mkdir -p"). + * `mode` sets permission bits (before umask) on UNIX and does nothing on + * Windows. * * import { mkdir } from "deno"; * await mkdir("new_dir"); + * await mkdir("nested/directories", true); */ -export async function mkdir(path: string, mode = 0o777): Promise { - await dispatch.sendAsync(...req(path, mode)); +export async function mkdir( + path: string, + recursive = false, + mode = 0o777 +): Promise { + await dispatch.sendAsync(...req(path, recursive, mode)); } function req( path: string, + recursive: boolean, mode: number ): [flatbuffers.Builder, msg.Any, flatbuffers.Offset] { const builder = flatbuffers.createBuilder(); const path_ = builder.createString(path); msg.Mkdir.startMkdir(builder); msg.Mkdir.addPath(builder, path_); + msg.Mkdir.addRecursive(builder, recursive); msg.Mkdir.addMode(builder, mode); const inner = msg.Mkdir.endMkdir(builder); return [builder, msg.Any.Mkdir, inner]; diff --git a/js/mkdir_test.ts b/js/mkdir_test.ts index ea6028c9ed..963f738067 100644 --- a/js/mkdir_test.ts +++ b/js/mkdir_test.ts @@ -3,15 +3,15 @@ import { testPerm, assert, assertEqual } from "./test_util.ts"; import * as deno from "deno"; testPerm({ write: true }, function mkdirSyncSuccess() { - const path = deno.makeTempDirSync() + "/dir/subdir"; + const path = deno.makeTempDirSync() + "/dir"; deno.mkdirSync(path); const pathInfo = deno.statSync(path); assert(pathInfo.isDirectory()); }); testPerm({ write: true }, function mkdirSyncMode() { - const path = deno.makeTempDirSync() + "/dir/subdir"; - deno.mkdirSync(path, 0o755); // no perm for x + const path = deno.makeTempDirSync() + "/dir"; + deno.mkdirSync(path, false, 0o755); // no perm for x const pathInfo = deno.statSync(path); if (pathInfo.mode !== null) { // Skip windows @@ -31,8 +31,33 @@ testPerm({ write: false }, function mkdirSyncPerm() { }); testPerm({ write: true }, async function mkdirSuccess() { - const path = deno.makeTempDirSync() + "/dir/subdir"; + const path = deno.makeTempDirSync() + "/dir"; await deno.mkdir(path); const pathInfo = deno.statSync(path); assert(pathInfo.isDirectory()); }); + +testPerm({ write: true }, function mkdirErrIfExists() { + let err; + try { + deno.mkdirSync("."); + } catch (e) { + err = e; + } + assertEqual(err.kind, deno.ErrorKind.AlreadyExists); + assertEqual(err.name, "AlreadyExists"); +}); + +testPerm({ write: true }, function mkdirSyncRecursive() { + const path = deno.makeTempDirSync() + "/nested/directory"; + deno.mkdirSync(path, true); + const pathInfo = deno.statSync(path); + assert(pathInfo.isDirectory()); +}); + +testPerm({ write: true }, async function mkdirRecursive() { + const path = deno.makeTempDirSync() + "/nested/directory"; + await deno.mkdir(path, true); + const pathInfo = deno.statSync(path); + assert(pathInfo.isDirectory()); +}); diff --git a/js/read_link_test.ts b/js/read_link_test.ts index 5838476546..bf27707c43 100644 --- a/js/read_link_test.ts +++ b/js/read_link_test.ts @@ -3,7 +3,7 @@ import { test, testPerm, assert, assertEqual } from "./test_util.ts"; import * as deno from "deno"; testPerm({ write: true }, function readlinkSyncSuccess() { - const testDir = deno.makeTempDirSync() + "/test-readlink-sync"; + const testDir = deno.makeTempDirSync(); const target = testDir + "/target"; const symlink = testDir + "/symln"; deno.mkdirSync(target); @@ -30,7 +30,7 @@ test(function readlinkSyncNotFound() { }); testPerm({ write: true }, async function readlinkSuccess() { - const testDir = deno.makeTempDirSync() + "/test-readlink"; + const testDir = deno.makeTempDirSync(); const target = testDir + "/target"; const symlink = testDir + "/symln"; deno.mkdirSync(target); diff --git a/js/rename_test.ts b/js/rename_test.ts index d82f9f2c72..99bca67dc3 100644 --- a/js/rename_test.ts +++ b/js/rename_test.ts @@ -3,7 +3,7 @@ import { testPerm, assert, assertEqual } from "./test_util.ts"; import * as deno from "deno"; testPerm({ write: true }, function renameSyncSuccess() { - const testDir = deno.makeTempDirSync() + "/test-rename-sync"; + const testDir = deno.makeTempDirSync(); const oldpath = testDir + "/oldpath"; const newpath = testDir + "/newpath"; deno.mkdirSync(oldpath); @@ -38,7 +38,7 @@ testPerm({ write: false }, function renameSyncPerm() { }); testPerm({ write: true }, async function renameSuccess() { - const testDir = deno.makeTempDirSync() + "/test-rename"; + const testDir = deno.makeTempDirSync(); const oldpath = testDir + "/oldpath"; const newpath = testDir + "/newpath"; deno.mkdirSync(oldpath); diff --git a/js/symlink_test.ts b/js/symlink_test.ts index 84dc452794..f039334422 100644 --- a/js/symlink_test.ts +++ b/js/symlink_test.ts @@ -3,7 +3,7 @@ import { testPerm, assert, assertEqual } from "./test_util.ts"; import * as deno from "deno"; testPerm({ write: true }, function symlinkSyncSuccess() { - const testDir = deno.makeTempDirSync() + "/test-symlink-sync"; + const testDir = deno.makeTempDirSync(); const oldname = testDir + "/oldname"; const newname = testDir + "/newname"; deno.mkdirSync(oldname); @@ -48,7 +48,7 @@ testPerm({ write: true }, function symlinkSyncNotImplemented() { }); testPerm({ write: true }, async function symlinkSuccess() { - const testDir = deno.makeTempDirSync() + "/test-symlink"; + const testDir = deno.makeTempDirSync(); const oldname = testDir + "/oldname"; const newname = testDir + "/newname"; deno.mkdirSync(oldname); diff --git a/src/deno_dir.rs b/src/deno_dir.rs index fa1bff9553..5c7e2f9e8d 100644 --- a/src/deno_dir.rs +++ b/src/deno_dir.rs @@ -73,10 +73,12 @@ impl DenoDir { deps_https, reload, }; - deno_fs::mkdir(deno_dir.gen.as_ref(), 0o755)?; - deno_fs::mkdir(deno_dir.deps.as_ref(), 0o755)?; - deno_fs::mkdir(deno_dir.deps_http.as_ref(), 0o755)?; - deno_fs::mkdir(deno_dir.deps_https.as_ref(), 0o755)?; + + // TODO Lazily create these directories. + deno_fs::mkdir(deno_dir.gen.as_ref(), 0o755, true)?; + deno_fs::mkdir(deno_dir.deps.as_ref(), 0o755, true)?; + deno_fs::mkdir(deno_dir.deps_http.as_ref(), 0o755, true)?; + deno_fs::mkdir(deno_dir.deps_https.as_ref(), 0o755, true)?; debug!("root {}", deno_dir.root.display()); debug!("gen {}", deno_dir.gen.display()); diff --git a/src/fs.rs b/src/fs.rs index 1b860b0578..9748cffab3 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -67,15 +67,12 @@ pub fn make_temp_dir( } } -pub fn mkdir(path: &Path, perm: u32) -> std::io::Result<()> { +pub fn mkdir(path: &Path, perm: u32, recursive: bool) -> std::io::Result<()> { debug!("mkdir -p {}", path.display()); let mut builder = DirBuilder::new(); - builder.recursive(true); + builder.recursive(recursive); set_dir_permission(&mut builder, perm); - builder.create(path).or_else(|err| match err.kind() { - std::io::ErrorKind::AlreadyExists => Ok(()), - _ => Err(err), - }) + builder.create(path) } #[cfg(any(unix))] diff --git a/src/msg.fbs b/src/msg.fbs index 57f8a7f2c6..4b303535fa 100644 --- a/src/msg.fbs +++ b/src/msg.fbs @@ -260,14 +260,13 @@ table MakeTempDirRes { table Mkdir { path: string; - mode: uint; - // mode specified by https://godoc.org/os#FileMode + recursive: bool; + mode: uint; // Specified by https://godoc.org/os#FileMode } table Chmod { path: string; - mode: uint; - // mode specified by https://godoc.org/os#FileMode + mode: uint; // Specified by https://godoc.org/os#FileMode } table Remove { diff --git a/src/ops.rs b/src/ops.rs index abc7b8d34e..f3c80e1ac4 100644 --- a/src/ops.rs +++ b/src/ops.rs @@ -540,15 +540,17 @@ fn op_mkdir( ) -> Box { assert_eq!(data.len(), 0); let inner = base.inner_as_mkdir().unwrap(); - let mode = inner.mode(); let path = String::from(inner.path().unwrap()); + let recursive = inner.recursive(); + let mode = inner.mode(); if let Err(e) = state.check_write(&path) { return odd_future(e); } + blocking(base.sync(), move || { debug!("op_mkdir {}", path); - deno_fs::mkdir(Path::new(&path), mode)?; + deno_fs::mkdir(Path::new(&path), mode, recursive)?; Ok(empty_buf()) }) }