-
Notifications
You must be signed in to change notification settings - Fork 722
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
refactor(mkdir): consistent error handling #854
base: master
Are you sure you want to change the base?
Changes from all commits
dac1a48
17b10b9
03d08f3
c28319f
95bfdd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,16 @@ common.register('mkdir', _mkdir, { | |
}, | ||
}); | ||
|
||
// See mkdir(2) for details on each error case | ||
// http://man7.org/linux/man-pages/man2/mkdir.2.html | ||
var codeToMessage = { | ||
EACCES: 'Permission denied', | ||
EEXIST: 'File exists', | ||
ENAMETOOLONG: 'File name too long', | ||
ENOENT: 'No such file or directory', | ||
ENOTDIR: 'Not a directory', | ||
}; | ||
|
||
// Recursively creates `dir` | ||
function mkdirSyncRecursive(dir) { | ||
var baseDir = path.dirname(dir); | ||
|
@@ -51,29 +61,13 @@ function mkdirSyncRecursive(dir) { | |
function _mkdir(options, dirs) { | ||
if (!dirs) common.error('no paths given'); | ||
|
||
if (typeof dirs === 'string') { | ||
dirs = [].slice.call(arguments, 1); | ||
} | ||
// if it's array leave it as it is | ||
dirs = [].slice.call(arguments, 1); | ||
|
||
dirs.forEach(function (dir) { | ||
try { | ||
var stat = common.statNoFollowLinks(dir); | ||
if (!options.fullpath) { | ||
common.error('path already exists: ' + dir, { continue: true }); | ||
} else if (stat.isFile()) { | ||
common.error('cannot create directory ' + dir + ': File exists', { continue: true }); | ||
} | ||
return; // skip dir | ||
} catch (e) { | ||
// do nothing | ||
} | ||
|
||
// Base dir does not exist, and no -p option given | ||
var baseDir = path.dirname(dir); | ||
if (!fs.existsSync(baseDir) && !options.fullpath) { | ||
common.error('no such file or directory: ' + baseDir, { continue: true }); | ||
return; // skip dir | ||
// Skip mkdir if -p option is given and directory already exists | ||
if (fs.existsSync(dir) && common.statNoFollowLinks(dir).isDirectory() && | ||
options.fullpath) { | ||
return; | ||
} | ||
|
||
try { | ||
|
@@ -83,16 +77,14 @@ function _mkdir(options, dirs) { | |
fs.mkdirSync(dir, parseInt('0777', 8)); | ||
} | ||
} catch (e) { | ||
var reason; | ||
if (e.code === 'EACCES') { | ||
reason = 'Permission denied'; | ||
} else if (e.code === 'ENOTDIR' || e.code === 'ENOENT') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm this is weird. On unix, ENNOTDIR and ENOENT are for different reasons. On appveyor, I see ENOENT when part of the path is a regular file. Not sure yet how we should handle this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still causing the appveyor CI to fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I haven't fixed this yet. |
||
reason = 'Not a directory'; | ||
} else { | ||
/* istanbul ignore next */ | ||
throw e; | ||
} | ||
common.error('cannot create directory ' + dir + ': ' + reason, { continue: true }); | ||
var reason = codeToMessage[e.code]; | ||
|
||
/* istanbul ignore if */ | ||
if (!reason) throw e; | ||
|
||
common.error('cannot create directory \'' + dir + '\': ' + reason, { | ||
continue: true, | ||
}); | ||
} | ||
}); | ||
return ''; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
"no-param-reassign": "off", | ||
"no-console": "off", | ||
"curly": "off", | ||
"quotes": ["error", "single", "avoid-escape"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if you'd like to promote this to the top-level config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any escaped quotes outside of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guess there's not. Let's keep it here then. |
||
"no-var": "error", | ||
"prefer-const": "error", | ||
"prefer-template": "off", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ test('dir already exists', t => { | |
const result = shell.mkdir(t.context.tmp); // dir already exists | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, `mkdir: path already exists: ${t.context.tmp}`); | ||
t.is(result.stderr, `mkdir: cannot create directory '${t.context.tmp}': File exists`); | ||
t.is(common.statFollowLinks(t.context.tmp).mtime.toString(), mtime); // didn't mess with dir | ||
}); | ||
|
||
|
@@ -42,7 +42,7 @@ test('Can\'t overwrite a broken link', t => { | |
const result = shell.mkdir('test/resources/badlink'); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, 'mkdir: path already exists: test/resources/badlink'); | ||
t.is(result.stderr, "mkdir: cannot create directory 'test/resources/badlink': File exists"); | ||
t.is(common.statNoFollowLinks('test/resources/badlink').mtime.toString(), mtime); // didn't mess with file | ||
}); | ||
|
||
|
@@ -51,7 +51,7 @@ test('root path does not exist', t => { | |
const result = shell.mkdir('/asdfasdf/foobar'); // root path does not exist | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, 'mkdir: no such file or directory: /asdfasdf'); | ||
t.is(result.stderr, "mkdir: cannot create directory '/asdfasdf/foobar': No such file or directory"); | ||
t.falsy(fs.existsSync('/asdfasdf')); | ||
t.falsy(fs.existsSync('/asdfasdf/foobar')); | ||
}); | ||
|
@@ -61,7 +61,7 @@ test('try to overwrite file', t => { | |
const result = shell.mkdir('test/resources/file1'); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, 'mkdir: path already exists: test/resources/file1'); | ||
t.is(result.stderr, "mkdir: cannot create directory 'test/resources/file1': File exists"); | ||
t.truthy(common.statFollowLinks('test/resources/file1').isFile()); | ||
}); | ||
|
||
|
@@ -70,7 +70,7 @@ test('try to overwrite file, with -p', t => { | |
const result = shell.mkdir('-p', 'test/resources/file1'); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, 'mkdir: cannot create directory test/resources/file1: File exists'); | ||
t.is(result.stderr, "mkdir: cannot create directory 'test/resources/file1': File exists"); | ||
t.truthy(common.statFollowLinks('test/resources/file1').isFile()); | ||
}); | ||
|
||
|
@@ -79,7 +79,7 @@ test('try to make a subdirectory of a file', t => { | |
const result = shell.mkdir('test/resources/file1/subdir'); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, 'mkdir: cannot create directory test/resources/file1/subdir: Not a directory'); | ||
t.is(result.stderr, "mkdir: cannot create directory 'test/resources/file1/subdir': Not a directory"); | ||
t.truthy(common.statFollowLinks('test/resources/file1').isFile()); | ||
t.falsy(fs.existsSync('test/resources/file1/subdir')); | ||
}); | ||
|
@@ -95,14 +95,29 @@ test('Check for invalid permissions', t => { | |
t.is(result.code, 1); | ||
t.is( | ||
result.stderr, | ||
'mkdir: cannot create directory nowritedir/foo: Permission denied' | ||
"mkdir: cannot create directory 'nowritedir/foo': Permission denied" | ||
); | ||
t.truthy(shell.error()); | ||
t.falsy(fs.existsSync(dirName + '/foo')); | ||
shell.rm('-rf', dirName); // clean up | ||
}); | ||
}); | ||
|
||
test('name too long', t => { | ||
// Generate a directory name which is more than 255 characters. Luckily, all | ||
// major OS's share approximately the same limit (we bump this up to 260 to | ||
// be cautious). | ||
const longName = new Array(260 + 1).join('a'); | ||
const dirName = `${t.context.tmp}/${longName}`; | ||
|
||
const result = shell.mkdir(dirName); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.is(result.stderr, | ||
`mkdir: cannot create directory '${dirName}': File name too long`); | ||
t.falsy(fs.existsSync(longName)); | ||
}); | ||
|
||
// | ||
// Valids | ||
// | ||
|
@@ -145,6 +160,21 @@ test('-p flag', t => { | |
shell.rm('-Rf', `${t.context.tmp}/a`); // revert | ||
}); | ||
|
||
test('create same dir twice with -p flag', t => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is implicitly covered in the "globbed dir" test, but this merits explicit coverage. |
||
t.falsy(fs.existsSync(`${t.context.tmp}/a`)); | ||
const result = shell.mkdir('-p', `${t.context.tmp}/a/b/c`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/a/b/c`)); | ||
|
||
const result2 = shell.mkdir('-p', `${t.context.tmp}/a/b/c`); | ||
t.falsy(shell.error()); | ||
t.is(result2.code, 0); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/a/b/c`)); | ||
|
||
shell.rm('-Rf', `${t.context.tmp}/a`); // revert | ||
}); | ||
|
||
test('multiple dirs', t => { | ||
const result = shell.mkdir('-p', `${t.context.tmp}/zzza`, | ||
`${t.context.tmp}/zzzb`, `${t.context.tmp}/zzzc`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the type is always string. We introduced some code to flatten out arrays. Even in the case of ShellStrings, I think we also convert those into bare strings.