Skip to content

Commit

Permalink
Update: Avoid passing fd to updateMetadata callback (#174)
Browse files Browse the repository at this point in the history
  • Loading branch information
@erikkemperman @phated
erikkemperman authored and phated committed Nov 28, 2017
1 parent 488a7b6 commit f682573
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 38 deletions.
7 changes: 4 additions & 3 deletions lib/dest/write-contents/write-buffer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ function writeBuffer(file, written) {
}

fo.updateMetadata(fd, file, onUpdate);
}

function onUpdate(statErr, fd) {
fo.closeFd(statErr, fd, written);
function onUpdate(statErr) {
fo.closeFd(statErr, fd, written);
}
}

}

module.exports = writeBuffer;
7 changes: 4 additions & 3 deletions lib/dest/write-contents/write-dir.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ function writeDir(file, written) {
}

fo.updateMetadata(fd, file, onUpdate);
}

function onUpdate(statErr, fd) {
fo.closeFd(statErr, fd, written);
function onUpdate(statErr) {
fo.closeFd(statErr, fd, written);
}
}

}

function isInaccessible(err) {
Expand Down
10 changes: 5 additions & 5 deletions lib/file-operations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function updateMetadata(fd, file, callback) {

function onStat(err, stat) {
if (err) {
return callback(err, fd);
return callback(err);
}

// Check if mode needs to be updated
Expand All @@ -111,13 +111,13 @@ function updateMetadata(fd, file, callback) {

// Nothing to do
if (!modeDiff && !timesDiff) {
return callback(null, fd);
return callback(null);
}

// Check access, `futimes` and `fchmod` only work if we own the file,
// or if we are effectively root.
if (!isOwner(stat)) {
return callback(null, fd);
return callback(null);
}

if (modeDiff) {
Expand All @@ -137,7 +137,7 @@ function updateMetadata(fd, file, callback) {
if (timesDiff) {
return times(fchmodErr);
}
callback(fchmodErr, fd);
callback(fchmodErr);
}
}

Expand All @@ -149,7 +149,7 @@ function updateMetadata(fd, file, callback) {
file.stat.atime = timesDiff.atime;
file.stat.mtime = timesDiff.mtime;
}
callback(fchmodErr || futimesErr, fd);
callback(fchmodErr || futimesErr);
}
}
}
Expand Down
50 changes: 23 additions & 27 deletions test/file-operations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ describe('updateMetadata', function() {
done();
});

it('passes the error and file descriptor if fstat fails', function(done) {
it('passes the error if fstat fails', function(done) {
if (isWindows) {
console.log('Changing the time of a directory errors in Windows.');
console.log('Changing the mode of a file is not supported by node.js in Windows.');
Expand All @@ -643,10 +643,8 @@ describe('updateMetadata', function() {

var fd = 9001;

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(err).toExist();
expect(typeof fd === 'number').toEqual(true);
expect(fd2).toEqual(fd);

done();
});
Expand All @@ -661,13 +659,13 @@ describe('updateMetadata', function() {
var fd = fs.openSync(inputPath, 'w+');
var stats = fs.fstatSync(fd);

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
// Not sure why .toEqual doesn't match these
Object.keys(file.stat).forEach(function(key) {
expect(file.stat[key]).toEqual(stats[key]);
});

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -682,11 +680,11 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(fchmodSpy.calls.length).toEqual(0);
expect(futimesSpy.calls.length).toEqual(0);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -708,11 +706,11 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(fchmodSpy.calls.length).toEqual(0);
expect(futimesSpy.calls.length).toEqual(0);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -731,7 +729,7 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(futimesSpy.calls.length).toEqual(1);
var stats = fs.fstatSync(fd);
var mtimeMs = Date.parse(file.stat.mtime);
Expand All @@ -743,7 +741,7 @@ describe('updateMetadata', function() {
expect(file.stat.atime).toEqual(new Date(then));
expect(atime).toEqual(Date.parse(stats.atime));

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -763,13 +761,13 @@ describe('updateMetadata', function() {
file.stat.atime = new Date(then);

var fd = fs.openSync(inputPath, 'w+');
expect(typeof fd === 'number').toEqual(true);

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(err).toExist();
expect(futimesSpy.calls.length).toEqual(1);
expect(typeof fd2 === 'number').toEqual(true);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -786,12 +784,12 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(fchmodSpy.calls.length).toEqual(1);
var stats = fs.fstatSync(fd);
expect(file.stat.mode).toEqual(stats.mode);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -809,12 +807,12 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(fchmodSpy.calls.length).toEqual(1);
var stats = fs.fstatSync(fd);
expect(file.stat.mode).toEqual(stats.mode);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -833,12 +831,11 @@ describe('updateMetadata', function() {
cb(new Error('mocked error'));
});

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(err).toExist();
expect(fchmodSpy.calls.length).toEqual(1);
expect(typeof fd2 === 'number').toEqual(true);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -861,7 +858,7 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w+');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(fchmodSpy.calls.length).toEqual(1);
expect(futimesSpy.calls.length).toEqual(1);

Expand All @@ -877,7 +874,7 @@ describe('updateMetadata', function() {
expect(atime).toEqual(Date.parse(stats.atime));
expect(file.stat.mode).toEqual(stats.mode);

fs.close(fd2, done);
fs.close(fd, done);
});
});

Expand All @@ -904,14 +901,13 @@ describe('updateMetadata', function() {

var fd = fs.openSync(inputPath, 'w');

updateMetadata(fd, file, function(err, fd2) {
updateMetadata(fd, file, function(err) {
expect(err).toExist();
expect(err).toEqual(expectedErr);
expect(fchmodSpy.calls.length).toEqual(1);
expect(futimesSpy.calls.length).toEqual(1);
expect(typeof fd2 === 'number').toEqual(true);

fs.close(fd2, done);
fs.close(fd, done);
});
});
});

0 comments on commit f682573

Please sign in to comment.