Repository: gulpjs/vinyl-fs
Pull request #144 (2016-01-14 06:33:05)

Feature metadata
Squashed as [requested](https://github.com/gulpjs/vinyl-fs/pull/128#issuecomment-171477651).

phated: @erikkemperman it seems that `fchmod` can't be used on a file that you don't own.  Can you add the same check as `getTimes` to compare the `uid` and a test to confirm it is working?
erikkemperman: At a glance, there is no test for the check in `getTimes` yet, either? I will have some time for this tomorrow.
erikkemperman: Several changes, including the guard against calling `fchmod` when we don't own the file. I have no idea how to unit test this though, we're not allowed to chown a file in order to verify this guard works. Ideas?

The last couple of commits are smaller changes I think are improvements, but for the moment I'll leave them unsquashed for easier review.
erikkemperman: Note, there are of course still those two tests that fail against the version of `vinyl` with the contents getter/setter changing `stat.xtime`. Because that change is already in `master`, I propose to get that sorted out first -- i.e. in the corresponding [PR](https://github.com/gulpjs/vinyl-fs/pull/143) -- before contemplating this one.
phated: @erikkemperman we don't rely on the master branch of vinyl so I don't know why they would be failing
phated: @erikkemperman can you revert some of the changes that were added to handle the vinyl changes I've now removed.  I'd like to get this in but it got a lot more complicated with those workarounds.
erikkemperman: @phated I've tried to keep the changes related to the now removed `vinyl` changes in a separate PR, [here](https://github.com/gulpjs/vinyl-fs/pull/143). If there's anything in here you'd rather not have I can of course remove those, but I believe these commits are orthogonal to the `vinyl` stuff.
phated: @erikkemperman ah, I had thought the last 4-5 commits were addressing things from the vinyl changes.
erikkemperman: @phated No they're just tweaks on @piranna and my efforts that I kept separate for the moment. I think they are slight improvements, especially the commit that makes the `stat` post-dest reflect the file on disk, which ought to keep piping onto `dest` equivalent to having a new `src` on that file.

Note however that the other PR, which is now mostly pointless until the `vinyl` issue is resolved, does contain as requested a more or less unrelated commit to address the intermittently failing tests, [here](https://github.com/erikkemperman/vinyl-fs/commit/b70bed0c7c488c9de6a530e1668aba9a852f2e4e).

I didn't have any time the last week or so, but still think the `vinyl` issue deserves some thought, would love some feedback on that issue.
phated: @erikkemperman it definitely needs more thought and discussion.  However, can you submit a separate PR with the fix to the failing tests?  I'd like to get that merged.
erikkemperman: Sure thing, will do that when I get on campus in an hour or so.

As to the commits here, I kept them separate for easier review but should probably squash at some point.
erikkemperman: @piranna The fix for the intermittently failing tests would have to be altered again if this PR were merged. So I would propose getting this out the way first and then addressing those?

Let me know if I should remove any commits from this PR, or rebase/squash.
piranna: > EDIT: Sorry @piranna, auto-complete got me. I meant to address @phated.

Ok, don't worry :-)

Patch:
diff --git a/lib/dest/writeContents/index.js b/lib/dest/writeContents/index.js
index 81ef50c..220acb4 100644
--- a/lib/dest/writeContents/index.js
+++ b/lib/dest/writeContents/index.js
@@ -1,11 +1,23 @@
 'use strict';
 
-var fs = require('fs');
+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) {
   // If directory then mkdirp it
   if (file.isDirectory()) {
@@ -29,49 +41,124 @@ function writeContents(writePath, file, cb) {
 
   // If no contents then do nothing
   if (file.isNull()) {
-    return complete();
+    return finish();
   }
 
-  function complete(err) {
-    cb(err, file);
-  }
 
-  function written(err) {
+  // 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);
+    }
 
-    if (isErrorFatal(err)) {
-      return complete(err);
+    // TODO handle symlinks properly
+    if (!file.stat || file.symlink) {
+      return close(null, finish);
     }
 
-    if (!file.stat || typeof file.stat.mode !== 'number' || file.symlink) {
-      return complete();
+    if (typeof fd === 'number') {
+      return stat(fd, close);
     }
 
-    fs.stat(writePath, function(err, st) {
+    // No file descriptor given (writeDir or writeSymbolicLink) so create one.
+    fs.open(writePath, 'r', function(err, fd) {
       if (err) {
-        return complete(err);
+        return close(err, finish);
       }
-      var currentMode = (st.mode & parseInt('0777', 8));
-      var expectedMode = (file.stat.mode & parseInt('0777', 8));
-      if (currentMode === expectedMode) {
-        return complete();
-      }
-      fs.chmod(writePath, expectedMode, complete);
+
+      stat(fd, function(err1) {
+        fs.close(fd, function(err2) {
+          finish(err1 || err2);
+        });
+      });
     });
   }
 
-  function isErrorFatal(err) {
-    if (!err) {
-      return false;
-    }
 
-    if (err.code === 'EEXIST' && file.flag === 'wx') {
-      // Handle scenario for file overwrite failures.
-      return false;   // "These aren't the droids you're looking for"
-    }
+  // 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);
+        });
+      }
 
-    // Otherwise, this is a fatal error
-    return true;
+      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);
+        });
+      }
+    });
   }
+
+
+  // This is invoked by the close callbacks; we're all done.
+  function finish(err) {
+    cb(err, file);
+  }
+
+
+
 }
 
 module.exports = writeContents;
diff --git a/lib/dest/writeContents/writeBuffer.js b/lib/dest/writeContents/writeBuffer.js
index daf0321..d58c666 100644
--- a/lib/dest/writeContents/writeBuffer.js
+++ b/lib/dest/writeContents/writeBuffer.js
@@ -2,29 +2,21 @@
 
 var fs = require('graceful-fs');
 
-var futimes = require('../../futimes');
-
-function writeBuffer(writePath, file, cb) {
+function writeBuffer(writePath, file, written) {
   var stat = file.stat;
 
   fs.open(writePath, file.flag, stat.mode, function(err, fd) {
     if (err) {
-      return cb(err);
+      return written(err);
     }
 
-    fs.write(fd, file.contents, 0, file.contents.length, 0, function(error) {
-      if (error) {
-        return complete(error);
-      }
-      futimes(fd, stat, complete);
-    });
-
-    // Cleanup
-    function complete(err1) {
-      fs.close(fd, function(err2) {
-        cb(err1 || err2);
+    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);
+        });
       });
-    }
+    });
   });
 }
 
diff --git a/lib/dest/writeContents/writeDir.js b/lib/dest/writeContents/writeDir.js
index d0b46a4..81b75de 100644
--- a/lib/dest/writeContents/writeDir.js
+++ b/lib/dest/writeContents/writeDir.js
@@ -2,8 +2,8 @@
 
 var mkdirp = require('mkdirp');
 
-function writeDir(writePath, file, cb) {
-  mkdirp(writePath, file.stat.mode, cb);
+function writeDir(writePath, file, written) {
+  mkdirp(writePath, file.stat.mode, written);
 }
 
 module.exports = writeDir;
diff --git a/lib/dest/writeContents/writeSymbolicLink.js b/lib/dest/writeContents/writeSymbolicLink.js
index fdd229a..04a8f75 100644
--- a/lib/dest/writeContents/writeSymbolicLink.js
+++ b/lib/dest/writeContents/writeSymbolicLink.js
@@ -2,13 +2,13 @@
 
 var fs = require('graceful-fs');
 
-function writeSymbolicLink(writePath, file, cb) {
+function writeSymbolicLink(writePath, file, written) {
   fs.symlink(file.symlink, writePath, function(err) {
     if (err && err.code !== 'EEXIST') {
       return cb(err);
     }
 
-    cb(null, file);
+    written();
   });
 }
 
diff --git a/lib/futimes.js b/lib/futimes.js
deleted file mode 100644
index 53ebece..0000000
--- a/lib/futimes.js
+++ /dev/null
@@ -1,27 +0,0 @@
-'use strict';
-
-var fs = require('graceful-fs');
-
-// http://stackoverflow.com/a/10589791/586382
-function validDate(date) {
-  return date instanceof Date && !isNaN(date.valueOf());
-}
-
-function futimes(fd, stat, cb) {
-  // Given `mtime` is not valid, do nothing
-  var mtime = stat.mtime;
-  if (!validDate(mtime)) {
-    return cb();
-  }
-
-  // Given `atime` is not valid, assign a new one with current time
-  var atime = stat.atime;
-  if (!validDate(atime)) {
-    atime = new Date();
-  }
-
-  // Set file `atime` and `mtime` fields
-  fs.futimes(fd, atime, mtime, cb);
-}
-
-module.exports = futimes;


Test patch:
diff --git a/lib/dest/writeContents/writeStream.js b/lib/dest/writeContents/writeStream.js
index 2cfda5f..10b3daa 100644
--- a/lib/dest/writeContents/writeStream.js
+++ b/lib/dest/writeContents/writeStream.js
@@ -3,20 +3,28 @@
 var fs = require('graceful-fs');
 
 var streamFile = require('../../src/getContents/streamFile');
-var futimes    = require('../../futimes');
 
-function writeStream(writePath, file, cb) {
+function writeStream(writePath, file, written) {
   var stat = file.stat;
+  var mode = stat.mode;
+
   var outStream;
   var outFD;
 
-  fs.open(writePath, 'w', file.stat.mode, function(err, fd) {
+  fs.open(writePath, 'w', mode, function(err, fd) {
     if (err) {
-      cb(err);
+      written(err);
     }
 
+    var opt = {
+      mode: mode,
+      flag: file.flag,
+      autoClose: false,
+      fd: fd,
+    };
+
     outFD = fd;
-    outStream = fs.createWriteStream(null, { fd: fd });
+    outStream = fs.createWriteStream(null, opt);
 
     file.contents.once('error', complete);
     file.contents.once('end', readStreamEnd);
@@ -35,14 +43,7 @@ function writeStream(writePath, file, cb) {
         return complete(error);
       }
 
-      futimes(outFD, stat, function(error) {
-        if (error) {
-          return complete(error);
-        }
-
-        // All finished with WriteStream, close and clean up
-        outStream.end();
-      });
+      complete();
     });
   }
 
@@ -54,7 +55,11 @@ function writeStream(writePath, file, cb) {
       outStream.removeListener('error', complete);
       outStream.removeListener('finish', complete);
     }
-    cb(err);
+    written(err, outFD, function(err1, finish) {
+      outStream.end(function(err2) {
+        finish(err1 || err2);
+      });
+    });
   }
 }
 
diff --git a/test/dest.js b/test/dest.js
index 6300f0b..e90e350 100644
--- a/test/dest.js
+++ b/test/dest.js
@@ -2,7 +2,9 @@
 
 var spies = require('./spy');
 var chmodSpy = spies.chmodSpy;
-var statSpy = spies.statSpy;
+var fchmodSpy = spies.fchmodSpy;
+var futimesSpy = spies.futimesSpy;
+var fstatSpy = spies.fstatSpy;
 
 var vfs = require('../');
 
@@ -21,8 +23,10 @@ require('mocha');
 var wipeOut = function() {
   this.timeout(20000);
   spies.setError('false');
-  statSpy.reset();
+  fstatSpy.reset();
   chmodSpy.reset();
+  fchmodSpy.reset();
+  futimesSpy.reset();
   del.sync(path.join(__dirname, './fixtures/highwatermark'));
   del.sync(path.join(__dirname, './out-fixtures/'));
 };
@@ -613,7 +617,7 @@ describe('dest stream', function() {
     stream.end();
   });
 
-  it('should not modify file mtime and atime when not provided on the vinyl stat', function(done) {
+  it('should not call futimes when no mtime is provided on the vinyl stat', function(done) {
     var inputPath = path.join(__dirname, './fixtures/test.coffee');
     var inputBase = path.join(__dirname, './fixtures/');
     var expectedPath = path.join(__dirname, './out-fixtures/test.coffee');
@@ -632,13 +636,12 @@ describe('dest stream', function() {
     var buffered = [];
 
     var onEnd = function() {
+      should(futimesSpy.called).be.not.ok;
       buffered.length.should.equal(1);
       buffered[0].should.equal(expectedFile);
       fs.existsSync(expectedPath).should.equal(true);
       fs.lstatSync(expectedPath).atime.setMilliseconds(0).should.equal(expectedAtime.setMilliseconds(0));
       fs.lstatSync(expectedPath).mtime.setMilliseconds(0).should.equal(expectedMtime.setMilliseconds(0));
-      expectedFile.stat.should.not.have.property('mtime');
-      expectedFile.stat.should.not.have.property('atime');
       done();
     };
 
@@ -651,7 +654,7 @@ describe('dest stream', function() {
     stream.end();
   });
 
-  it('should write file mtime using the vinyl mtime', function(done) {
+  it('should call futimes when an mtime is provided on the vinyl stat', function(done) {
     var inputPath = path.join(__dirname, './fixtures/test.coffee');
     var inputBase = path.join(__dirname, './fixtures/');
     var expectedPath = path.join(__dirname, './out-fixtures/test.coffee');
@@ -671,13 +674,13 @@ describe('dest stream', function() {
     var buffered = [];
 
     var onEnd = function() {
+      should(futimesSpy.called).be.ok;
       buffered.length.should.equal(1);
       buffered[0].should.equal(expectedFile);
       fs.existsSync(expectedPath).should.equal(true);
       fs.lstatSync(expectedPath).mtime.getTime().should.equal(expectedMtime.getTime());
       expectedFile.stat.should.have.property('mtime');
       expectedFile.stat.mtime.should.equal(expectedMtime);
-      expectedFile.stat.should.not.have.property('atime');
       done();
     };
 
@@ -690,7 +693,7 @@ describe('dest stream', function() {
     stream.end();
   });
 
-  it('should not modify file mtime and atime when provided mtime on the vinyl stat is invalid', function(done) {
+  it('should not call futimes when provided mtime on the vinyl stat is invalid', function(done) {
     var inputPath = path.join(__dirname, './fixtures/test.coffee');
     var inputBase = path.join(__dirname, './fixtures/');
     var expectedPath = path.join(__dirname, './out-fixtures/test.coffee');
@@ -711,6 +714,7 @@ describe('dest stream', function() {
     var buffered = [];
 
     var onEnd = function() {
+      should(futimesSpy.called).be.not.ok;
       buffered.length.should.equal(1);
       buffered[0].should.equal(expectedFile);
       fs.existsSync(expectedPath).should.equal(true);
@@ -727,7 +731,7 @@ describe('dest stream', function() {
     stream.end();
   });
 
-  it('should write file mtime when provided mtime on the vinyl stat is valid but provided atime is invalid', function(done) {
+  it('should call futimes when provided mtime on the vinyl stat is valid but provided atime is invalid', function(done) {
     var inputPath = path.join(__dirname, './fixtures/test.coffee');
     var inputBase = path.join(__dirname, './fixtures/');
     var expectedPath = path.join(__dirname, './out-fixtures/test.coffee');
@@ -749,6 +753,7 @@ describe('dest stream', function() {
     var buffered = [];
 
     var onEnd = function() {
+      should(futimesSpy.called).be.ok;
       buffered.length.should.equal(1);
       buffered[0].should.equal(expectedFile);
       fs.existsSync(expectedPath).should.equal(true);
@@ -784,10 +789,6 @@ describe('dest stream', function() {
       },
     });
 
-    // Node.js uses `utime()`, so `fs.utimes()` has a resolution of 1 second
-    expectedAtime.setMilliseconds(0);
-    expectedMtime.setMilliseconds(0);
-
     var buffered = [];
 
     var onEnd = function() {
@@ -926,7 +927,7 @@ describe('dest stream', function() {
     fs.closeSync(fs.openSync(expectedPath, 'w'));
 
     spies.setError(function(mod, fn) {
-      if (fn === 'stat' && arguments[2] === expectedPath) {
+      if (fn === 'fstat' && typeof arguments[2] === 'number') {
         return new Error('stat error');
       }
     });
@@ -961,7 +962,7 @@ describe('dest stream', function() {
     fs.closeSync(fs.openSync(expectedPath, 'w'));
 
     spies.setError(function(mod, fn) {
-      if (fn === 'chmod' && arguments[2] === expectedPath) {
+      if (fn === 'fchmod' && typeof arguments[2] === 'number') {
         return new Error('chmod error');
       }
     });
@@ -994,14 +995,14 @@ describe('dest stream', function() {
 
     var expectedCount = 0;
     spies.setError(function(mod, fn) {
-      if (fn === 'stat' && arguments[2] === expectedPath) {
+      if (fn === 'fstat' && typeof arguments[2] === 'number') {
         expectedCount++;
       }
     });
 
     var onEnd = function() {
       expectedCount.should.equal(1);
-      should(chmodSpy.called).be.not.ok;
+      should(fchmodSpy.called).be.not.ok;
       realMode(fs.lstatSync(expectedPath).mode).should.equal(expectedMode);
       done();
     };
@@ -1010,8 +1011,8 @@ describe('dest stream', function() {
     fs.closeSync(fs.openSync(expectedPath, 'w'));
     fs.chmodSync(expectedPath, expectedMode);
 
-    statSpy.reset();
-    chmodSpy.reset();
+    fstatSpy.reset();
+    fchmodSpy.reset();
     var stream = vfs.dest('./out-fixtures/', { cwd: __dirname });
 
     var buffered = [];
@@ -1043,14 +1044,14 @@ describe('dest stream', function() {
 
     var expectedCount = 0;
     spies.setError(function(mod, fn) {
-      if (fn === 'stat' && arguments[2] === expectedPath) {
+      if (fn === 'fstat' && typeof arguments[2] === 'number') {
         expectedCount++;
       }
     });
 
     var onEnd = function() {
       expectedCount.should.equal(1);
-      should(chmodSpy.called).be.not.ok;
+      should(fchmodSpy.called).be.not.ok;
       done();
     };
 
@@ -1058,8 +1059,8 @@ describe('dest stream', function() {
     fs.closeSync(fs.openSync(expectedPath, 'w'));
     fs.chmodSync(expectedPath, expectedMode);
 
-    statSpy.reset();
-    chmodSpy.reset();
+    fstatSpy.reset();
+    fchmodSpy.reset();
     var stream = vfs.dest('./out-fixtures/', { cwd: __dirname });
 
     var buffered = [];
@@ -1248,6 +1249,8 @@ describe('dest stream', function() {
     var stream = vfs.dest('./out-fixtures/', { cwd: __dirname });
 
     var bufferStream = through.obj(dataWrap(buffered.push.bind(buffered)), onEnd);
+
+    stream.on('error', done);
     stream.pipe(bufferStream);
     stream.write(inputFile);
     stream.end();
diff --git a/test/spy.js b/test/spy.js
index e25c10a..f64b6ce 100644
--- a/test/spy.js
+++ b/test/spy.js
@@ -1,6 +1,6 @@
 'use strict';
 
-var fs = require('fs');
+var fs = require('graceful-fs');
 var sinon = require('sinon');
 
 var errorfn = false;
@@ -25,5 +25,8 @@ module.exports = {
     errorfn = fn;
   },
   chmodSpy: maybeCallAsync(fs, 'chmod'),
+  fchmodSpy: maybeCallAsync(fs, 'fchmod'),
+  futimesSpy: maybeCallAsync(fs, 'futimes'),
   statSpy: maybeCallAsync(fs, 'stat'),
+  fstatSpy: maybeCallAsync(fs, 'fstat'),
 };