Fix: Additional refactor & tons of tests for metadata update

.travis.yml

@@ -1,4 +1,4 @@
-sudo: false
+sudo: required
 language: node_js
 node_js:
   - 'stable'
@@ -8,5 +8,7 @@ node_js:
   - '0.10'
 before_script:
   - find test -type d -exec chmod g+s {} \;
+  - sudo chown root test/not-owned/
+  - sudo chown root test/not-owned/not-owned.txt
 after_script:
   - npm run coveralls

lib/dest/writeContents/index.js

@@ -1,24 +1,11 @@
 'use strict';
 
-var fs = require('graceful-fs');
-var assign = require('object-assign');
-
 var writeDir = require('./writeDir');
 var writeStream = require('./writeStream');
 var writeBuffer = require('./writeBuffer');
 var writeSymbolicLink = require('./writeSymbolicLink');
 
-// TODO include sticky/setuid/setgid, i.e. 7777?
-var MASK_MODE = parseInt('0777', 8);
-
-
-// http://stackoverflow.com/a/10589791/586382
-function validDate(date) {
-  return date instanceof Date && !isNaN(date.valueOf());
-}
-
-
-function writeContents(writePath, file, cb) {
+function writeContents(writePath, file, callback) {
   // If directory then mkdirp it
   if (file.isDirectory()) {
     return writeDir(writePath, file, written);
@@ -41,124 +28,32 @@ function writeContents(writePath, file, cb) {
 
   // If no contents then do nothing
   if (file.isNull()) {
-    return finish();
+    return written();
   }
 
-
   // This is invoked by the various writeXxx modules when they've finished
   // writing the contents.
-  // The third argument, if present, should be invoked to indicate we're done
-  // with the file descriptor.
-  function written(err, fd, close) {
-    close = close || function(err, cb) {
-      cb(err);
-    };
-
-    if (err && !(err.code === 'EEXIST' && file.flag === 'wx')) {
-      return close(err, finish);
-    }
-
-    // TODO handle symlinks properly
-    if (!file.stat || file.symlink) {
-      return close(null, finish);
+  function written(err) {
+    if (isErrorFatal(err)) {
+      return callback(err);
     }
 
-    if (typeof fd === 'number') {
-      return stat(fd, close);
-    }
-
-    // No file descriptor given (writeDir or writeSymbolicLink) so create one.
-    fs.open(writePath, 'r', function(err, fd) {
-      if (err) {
-        return close(err, finish);
-      }
-
-      stat(fd, function(err1) {
-        fs.close(fd, function(err2) {
-          finish(err1 || err2);
-        });
-      });
-    });
+    callback(null, file);
   }
 
+  function isErrorFatal(err) {
+    if (!err) {
+      return false;
+    }
 
-  // Set mode and/or atime, mtime.
-  function stat(fd, close) {
-    fs.fstat(fd, function(err, stat) {
-      if (err) {
-        return close(err, finish);
-      }
-
-      // Check if mode needs to be updated
-      var modeDiff = 0;
-      if (typeof file.stat.mode === 'number') {
-        modeDiff = (file.stat.mode ^ stat.mode) & MASK_MODE;
-      }
-
-      // Check if atime/mtime need to be updated
-      var timesDiff = null;
-      if (validDate(file.stat.mtime)) {
-        timesDiff = {
-          mtime: file.stat.mtime,
-          atime: validDate(file.stat.atime) ? file.stat.atime : stat.atime,
-        };
-      }
-
-      // Set file.stat to the reflect current state on disk
-      assign(file.stat, stat);
-
-      // Nothing to do
-      if (!modeDiff && !timesDiff) {
-        return close(null, finish);
-      }
-
-      // Check access, `futimes` and `fchmod` only work if we own the file,
-      // or if we are effectively root.
-      var uid = process.geteuid ? process.geteuid() : process.getuid();
-      if (stat.uid !== uid && uid !== 0) {
-        return close(null, finish);
-      }
-
-      if (modeDiff) {
-        return mode();
-      }
-      times();
-
-      function mode() {
-        var mode = stat.mode ^ modeDiff;
-        fs.fchmod(fd, mode, function(err) {
-          if (!err) {
-            file.stat.mode = mode;
-            file.stat.ctime.setTime(Date.now());
-          }
-          if (timesDiff) {
-            return times(err);
-          }
-          close(err, finish);
-        });
-      }
-
-      function times(err1) {
-        fs.futimes(fd, timesDiff.atime, timesDiff.mtime, function(err2) {
-          if (!err2) {
-            file.stat.atime = timesDiff.atime;
-            file.stat.mtime = timesDiff.mtime;
-            file.stat.ctime.setTime(Date.now());
-          }
-          close(err1 || err2, finish);
-        });
-      }
-    });
-  }
-
+    if (err.code === 'EEXIST' && file.flag === 'wx') {
+      // Handle scenario for file overwrite failures.
+      return false;   // "These aren't the droids you're looking for"
+    }
 
-  // This is invoked by the close callbacks; we're all done.
-  function finish(err) {
-    cb(err, file);
+    // Otherwise, this is a fatal error
+    return true;
   }
-
-
-
 }
 
 module.exports = writeContents;

lib/dest/writeContents/writeBuffer.js

@@ -1,23 +1,26 @@
 'use strict';
 
-var fs = require('graceful-fs');
+var fo = require('../../fileOperations');
 
 function writeBuffer(writePath, file, written) {
-  var stat = file.stat;
+  var opt = {
+    mode: file.stat.mode,
+    flag: file.flag,
+  };
 
-  fs.open(writePath, file.flag, stat.mode, function(err, fd) {
-    if (err) {
-      return written(err);
+  fo.writeFile(writePath, file.contents, opt, onWriteFile);
+
+  function onWriteFile(writeErr, fd) {
+    if (writeErr) {
+      return fo.closeFd(writeErr, fd, written);
     }
 
-    fs.write(fd, file.contents, 0, file.contents.length, 0, function(err) {
-      written(err, fd, function(err1, finish) {
-        fs.close(fd, function(err2) {
-          finish(err1 || err2);
-        });
-      });
-    });
-  });
+    fo.updateMetadata(fd, file, onUpdate);
+  }
+
+  function onUpdate(statErr, fd) {
+    fo.closeFd(statErr, fd, written);
+  }
 }
 
 module.exports = writeBuffer;

lib/dest/writeContents/writeDir.js

@@ -1,9 +1,49 @@
 'use strict';
 
+var fs = require('graceful-fs');
 var mkdirp = require('mkdirp');
 
+var fo = require('../../fileOperations');
+
 function writeDir(writePath, file, written) {
-  mkdirp(writePath, file.stat.mode, written);
+  mkdirp(writePath, file.stat.mode, onMkdirp);
+
+  function onMkdirp(mkdirpErr) {
+    if (mkdirpErr) {
+      return written(mkdirpErr);
+    }
+
+    fs.open(writePath, 'r', onOpen);
+  }
+
+  function onOpen(openErr, fd) {
+    // If we don't have access, just move along
+    if (isInaccessible(openErr)) {
+      return fo.closeFd(null, fd, written);
+    }
+
+    if (openErr) {
+      return fo.closeFd(openErr, fd, written);
+    }
+
+    fo.updateMetadata(fd, file, onUpdate);
+  }
+
+  function onUpdate(statErr, fd) {
+    fo.closeFd(statErr, fd, written);
+  }
 }
 
+function isInaccessible(err) {
+    if (!err) {
+      return false;
+    }
+
+    if (err.code === 'EACCES') {
+      return true;
+    }
+
+    return false;
+  }
+
 module.exports = writeDir;

lib/dest/writeContents/writeStream.js

@@ -2,64 +2,56 @@
 
 var fs = require('graceful-fs');
 
+var fo = require('../../fileOperations');
 var streamFile = require('../../src/getContents/streamFile');
 
 function writeStream(writePath, file, written) {
-  var stat = file.stat;
-  var mode = stat.mode;
+  var opt = {
+    mode: file.stat.mode,
+    flag: file.flag,
+    autoClose: false,
+  };
 
-  var outStream;
-  var outFD;
+  var outStream = fs.createWriteStream(writePath, opt);
 
-  fs.open(writePath, 'w', mode, function(err, fd) {
-    if (err) {
-      written(err);
-    }
-
-    var opt = {
-      mode: mode,
-      flag: file.flag,
-      autoClose: false,
-      fd: fd,
-    };
-
-    outFD = fd;
-    outStream = fs.createWriteStream(null, opt);
+  file.contents.once('error', complete);
+  file.contents.once('end', readStreamEnd);
+  outStream.once('error', complete);
+  outStream.once('finish', complete);
 
-    file.contents.once('error', complete);
-    file.contents.once('end', readStreamEnd);
-    outStream.once('error', complete);
-    outStream.once('finish', complete);
-
-    // Streams are piped with end disabled, this prevents the
-    // WriteStream from closing the file descriptor after all
-    // data is written.
-    file.contents.pipe(outStream, { end: false });
-  });
+  // Streams are piped with end disabled, this prevents the
+  // WriteStream from closing the file descriptor after all
+  // data is written.
+  file.contents.pipe(outStream, { end: false });
 
   function readStreamEnd() {
-    streamFile(file, {}, function(error) {
-      if (error) {
-        return complete(error);
-      }
+    streamFile(file, complete);
+  }
+
+  function end(propagatedErr) {
+    outStream.end(onEnd);
 
-      complete();
-    });
+    function onEnd(endErr) {
+      written(propagatedErr || endErr);
+    }
   }
 
   // Cleanup
-  function complete(err) {
+  function complete(streamErr) {
     file.contents.removeListener('error', complete);
     file.contents.removeListener('end', readStreamEnd);
-    if (outStream) {
-      outStream.removeListener('error', complete);
-      outStream.removeListener('finish', complete);
+    outStream.removeListener('error', complete);
+    outStream.removeListener('finish', complete);
+
+    if (streamErr) {
+      return end(streamErr);
     }
-    written(err, outFD, function(err1, finish) {
-      outStream.end(function(err2) {
-        finish(err1 || err2);
-      });
-    });
+
+    if (typeof outStream.fd !== 'number') {
+      return end();
+    }
+
+    fo.updateMetadata(outStream.fd, file, end);
   }
 }
 

lib/dest/writeContents/writeSymbolicLink.js

@@ -3,13 +3,18 @@
 var fs = require('graceful-fs');
 
 function writeSymbolicLink(writePath, file, written) {
+  // TODO handle symlinks properly
   fs.symlink(file.symlink, writePath, function(err) {
-    if (err && err.code !== 'EEXIST') {
-      return cb(err);
+    if (isFatalError(err)) {
+      return written(err);
     }
 
     written();
   });
 }
 
+function isFatalError(err) {
+  return (err && err.code !== 'EEXIST');
+}
+
 module.exports = writeSymbolicLink;