Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(mkdir): consistent error handling #854

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

refactor(mkdir): consistent error handling #854

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dac1a48
refactor(mkdir): consistent error handling
nfischer Jun 3, 2018
17b10b9
move dir into temp dir
nfischer Jun 3, 2018
03d08f3
try to join paths differently in test
nfischer Jun 3, 2018
c28319f
Revert "try to join paths differently in test"
nfischer Jun 3, 2018
95bfdd1
move object to module scope
nfischer Jun 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 22 additions & 31 deletions src/mkdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,29 +51,13 @@ function mkdirSyncRecursive(dir) {
function _mkdir(options, dirs) {
if (!dirs) common.error('no paths given');

if (typeof dirs === 'string') {
Copy link
Member Author

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.

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 {
Expand All @@ -83,16 +67,23 @@ 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') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ENOENT should be handled distinctly from ENOTDIR, not sure why they're in the same branch here.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still causing the appveyor CI to fail?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 });
// 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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I limited this to the list of error codes I could repro locally. I think that's fine for now, and we can add more later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this enum something that should be declared globally? Perhaps in src/common.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the error codes can be shared (e.g., EEXIST). However, there's no real advantage to moving these to common.js. It's the difference between common.codes.EEXIST and 'EEXIST'.

When it comes to user-facing error messages, I think each command wants a different message as it sees fit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. This enum could at least be declared globally in this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

};
var reason = codeToMessage[e.code];

/* istanbul ignore if */
if (!reason) throw e;

common.error('cannot create directory \'' + dir + '\': ' + reason, {
continue: true,
});
}
});
return '';
Expand Down
1 change: 1 addition & 0 deletions test/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"no-param-reassign": "off",
"no-console": "off",
"curly": "off",
"quotes": ["error", "single", "avoid-escape"],
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any escaped quotes outside of test/? I can't find any in src/.

Copy link
Member Author

Choose a reason for hiding this comment

The 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",
Expand Down
43 changes: 36 additions & 7 deletions test/mkdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});

Expand All @@ -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
});

Expand All @@ -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'));
});
Expand All @@ -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());
});

Expand All @@ -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());
});

Expand All @@ -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'));
});
Expand All @@ -95,14 +95,28 @@ 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 result = shell.mkdir(longName);
t.truthy(shell.error());
t.is(result.code, 1);
t.is(result.stderr,
`mkdir: cannot create directory '${longName}': File name too long`);
t.falsy(fs.existsSync(longName));
});

//
// Valids
//
Expand Down Expand Up @@ -145,6 +159,21 @@ test('-p flag', t => {
shell.rm('-Rf', `${t.context.tmp}/a`); // revert
});

test('create same dir twice with -p flag', t => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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`);
Expand Down