Should custom mkdirp take into account process.umask()

[phated]

The behavior of node's fs.mkdir() method is that process.umask() is applied to the mode being set on the directory. However, on Darwin and some other platforms, this doesn't allow us to set the directories setgid permissions. To solve this, we can use fs.chmod() on the directories after they are created but fs.chmod() ignores the process.umask() altogether. We can applied the process.umask() to the custom mode like var mode = (customMode || DEFAULT_DIR_MODE) & ~process.umask(); is this the correct/least surprising behavior or should we leave the custom mode alone?

/cc @erikkemperman

[erikkemperman]

Ah, that's tricky. The umask is only usually applied for newly created files and directories. Chmod on extant files and directories is not affected by it.

But I guess we're wanting to do a combination of mkdir+chmod to get around platform differences...

Does it make sense to apply the umask on the chmod call only when the directory did not exist prior to the mkdir?

[erikkemperman]

Then again we're applying chmod unconditionally for files at the moment; maybe that should follow a similar pattern?

[phated]

This question is specifically for mkdirp so it should probably account for the umask.

[erikkemperman]

But wasn't this new mkdirp stuff also supposed to update the mode on existing dirs? Maybe umask shouldn't be applied in that case.

And actually maybe that distinction should be made for files, as well?

[phated]

I believe it is applied in the case of the *nix command, even when updating the final directory's mode.

[erikkemperman]

Is it? I didn't think so but can't check just now.

Reading back your message at the top, though: I don't think umask is the reason why sticky et al are not applied on mac -- the man page for the mkdir syscall says that behavior is undefined for mode bits other than rwx.

This is why, I guess, the external mkdirp module from before not only applies the umask but also restricts the mode argument to 777.

[phated]

@erikkemperman I took a look through https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/mkdir.1.html (which seems to be clearer than http://man7.org/linux/man-pages/man1/mkdir.1.html) and I believe I followed all the guidelines for the custom implementation of mkdir; specifically, intermediate directories are created with the default permissions (777 - umask), custom modes aren't umasked & custom mode updates the final directory even if it exists. If you have a chance, please compare the mkdirp implementation and tests as of a6c1f05 and confirm that it matches that manpage. Thanks!

[phated]

Had to force push some amendments. Latest SHA is 5ffb005

[erikkemperman]

Nice! Good to have some reference as to what it should do. I don't have a lot of time, but with any luck I can look it over more thoroughly on Sunday. At a glance I notice that the isDarwin var is still in the test though, is it still needed?

[phated]

@erikkemperman it's no longer used. I'm going to be doing a separate pass at linting warnings today.

[erikkemperman]

@phated Oh yeah, I see. While looking at linting warnings, I'm curious if the skewed indentation in lib/dest/writeContents/writeDir.js is actually being flagged?

I'll get back to you as soon as I can on this mkdirp stuff!