fix chmod function arguments

[thefourtheye]

*chmod* functions accept only path and mode. As we were using
chownFix, it misinterprets callback function passed to chmod as gid.

Possible Fix for nodejs/node#7846

[MylesBorins]

@thefourtheye there are still four failures in npm with this patch

test/tap/install-link-scripts.js ...................... 1/4
  plain install
  not ok TypeError: cb is not a function
    at:
      line: 210
      column: 7
      file: node_modules/graceful-fs/polyfills.js
    stack: |
      node_modules/graceful-fs/polyfills.js:210:7
      FSReqWrap.oncomplete (fs.js:123:15)
    test: plain install
    message: 'TypeError: cb is not a function'

  link
  not ok TypeError: cb is not a function
    at:
      line: 210
      column: 7
      file: node_modules/graceful-fs/polyfills.js
    stack: |
      node_modules/graceful-fs/polyfills.js:210:7
      FSReqWrap.oncomplete (fs.js:123:15)
    test: link
    message: 'TypeError: cb is not a function'

  install --link
  not ok TypeError: cb is not a function
    at:
      line: 210
      column: 7
      file: node_modules/graceful-fs/polyfills.js
    stack: |
      node_modules/graceful-fs/polyfills.js:210:7
      FSReqWrap.oncomplete (fs.js:123:15)
    test: install --link
    message: 'TypeError: cb is not a function'

test/tap/update-symlink.js .......................... 14/15 6s
  when update is called linked packages should be excluded
  not ok updated ok
    found: >+
      my-local-package@1.0.0
      /Users/thealphanerd/code/node/master/test-npm/test/tap/update-symlink

      `-- async@2.0.1

    pattern: /async@1.5.2/
    at:
      line: 64
      column: 9
      file: test/tap/update-symlink.js
    stack: |
      test/tap/update-symlink.js:64:9
      f (node_modules/once/once.js:17:25)
      ChildProcess.<anonymous> (test/common-tap.js:68:5)
      Pipe._handle.close [as _onclose] (net.js:492:12)

[ChALkeR]

@thefourtheye Note that this patch actually makes callback argument mandratory for chmod even for older Node versions (e.g. v4 or v6), where it's not mandratory. Strictly speaking, this is a major change in graceful-fs.

For this to be not major, it should check if callback is being passed.

[MylesBorins]

@@ -205,6 +205,7 @@ function patchLutimes (fs) {
 function chmodFix (orig) {
   if (!orig) return orig
   return function (target, mode, cb) {
+    if (!cb) cb = function () {}
     return orig.call(fs, target, mode, function (er, res) {
       if (chownErOk(er)) er = null
       cb(er, res)

This diff will make this a semverpatch change. I think it would likely be in the spirit of graceful-fs to "gracefully" handle the case where to callback is given.

@isaacs do you consider a no-op graceful or should there be some sort of error / warning on platforms that require the callback?

[thefourtheye]

@thealphanerd Thanks. I included a fix to the problem now. PTAL.

[isaacs]

This patch doesn't seem offensive to me at all, but I'm not quite getting exactly what the problem is that it's addressing.

Can someone provide an example program or test that shows the specific behavior that this is addressing? Is it a node core change that needs to be taken into consideration? The link to nodejs/node#7846 is a bit confusing for me to follow.

[isaacs]

Basically, if it's "chownFix doesn't work for chmod", well... ok... how come it was working all this time, then? Was it always broken? If so, how was it broken?

I'm not trying to be difficult, I just have an excessive abundance of caution where it concerns this module, because it is so widely depended upon. Thanks for understanding :)

[MylesBorins]

@isaacs the TLDR on nodejs/node#7846 is that all fs async calls in a future major version will make the callback mandatory. This change is likely getting reverted right now as we assess ecosystem breakage

As for the the chownFix not working... it seems to me like it was working but my accident.

fs.chown(path, uid, gid, callback)
fs.chmod(path, mode, callback)

So the way it was being done before the callback was being passed as the gid and it just happened to work.

The unfortunate side effect of this was that the callback wrapper that checks the error and wraps it in chownErOk was never called. Our tests on the enforced callback revealed this bug. We can do a citgm run on this patch if it would help.

[isaacs]

Oh! I see, so if the chmod syscall raised EINVAL, ENOSYS, or EPERM, it'd fail, instead of silently being a no-op. Yeah, that's a bug, and I see how it would have been working by accident.

[isaacs]

Thanks! Landed with a test and published to 4.1.5.