Custom mkdirp (Issue #165) #169
Conversation
Custom mkdir implementation that include the ability to update the permissions mode on a directory that already exists.
New tests will skip on Windows.
Make sure directory is properly deleted before next test.
| var cb = callback || function() {}; | ||
| dirpath = path.resolve(dirpath); | ||
|
|
||
| fs.mkdir(dirpath, m, function(er) { |
There was a problem hiding this comment.
why does this attempt to use fs.mkdir before mkdirp (only if ENOENT)?
There was a problem hiding this comment.
The initial fs.mkdir call ends the recursion on success. If a parent directory doesn't exist, it recursively calls 'mkdirp' up the path until fs.mkdir succeeds. Then calls mkdir back down the path.
There was a problem hiding this comment.
It doesn't seem to call mkdir back down the path (cb is being called, ending the cycle). This probably shouldn't be using mkdirp at all because it has to chmod each path segment
There was a problem hiding this comment.
The mkdir back down the path is on line 221. It's actually another mkdirp call.
One clarifying question on mode:
When mode is provided, all newly created directories will be created with that mode. And currently, if the final directory on the path already exists, then chmod will be called to change the mode.
Do you want ALL existing directories in the provided dirpath parameter to change permission?
There was a problem hiding this comment.
Yeah, I believe the desire is to have every directory updated.
There was a problem hiding this comment.
Also, I was thinking it was still using the mkdirp module, totally forgot that the function was named mkdirp (ignore my comments about that).
There was a problem hiding this comment.
Gotcha. I will try to add that mode detail in the next day or two.
|
@virtuakazib @phated I like this idea -- it removes the need to subvert the setuid/setgid/sticky tests on MacOSX, as currently done here. Unfortunately I discovered too late (after this PR was merged) what was actually up with that:
EDIT Hm, actually it is more complicated still... The current master branch fails |
|
@virtuakazib @phated Any news on this? No pressure, I was just looking forward to not having to hack this By the way, I was curious, why are the new tests in this PR skipped entirely for Windows? Wouldn't it be better to test the creation of dirs and only skip testing their modes? |
|
@erikkemperman thanks for following up but it looks like changes are still to be made based on my comments. |
|
@virtuakazib will you have anytime soon to make the changes? |
|
This needs a few basic tests for Windows (that just check the directories are created). |
Custom mkdir implementation that include the ability to update the
permissions mode on a directory that already exists.
This is an initial cut at this feature that works with passing unit tests on OS X. Haven't been able to verify Windows and Linux compatibility yet.
There are also probably coding convention and style preferences to fix.