Add option to preserve timestamps

[Narigo]

Another attempt to fix #138.
Hope this is better for you than PR #139

[jprichardson]

This looks really good, thank you!

1. Yesterday, the copy tests were moved from ./test/copy to ./lib/copy/__tests__. Not a big deal, but would be cool if you could rebase. If you don't want to hassle with it, don't worry about it. I can patch that part up.
2. It's interesting these pass on Node.js v0.10 on both Windows/Linux but fail on Node 0.12 / io.js. It looks like a millisecond precision difference. Any idea what's going on there?

[Narigo]

I'm not sure if I check the dates correctly - I haven't really used mocha... probably the date check via assert.deepEqual doesn't work. It didn't pass the tests for me when using assert.equals, but with deepEqual it worked here locally...

I cloned the repo today and used the master branch. I didn't see the copy tests, so I just created one like move. Will move that though...

[jprichardson]

The formatting in my last comment butchered the path names, the copy tests were moved to ./lib/copy/__tests__. You can see the failed output on both Travis (Linux) and Appveyor (Windows). Both pass on Node v0.10, but fail in the rest due to the millisecond differences.

[Narigo]

Any preference on where I should put the fixtures? Or shall I only test the copy of a single file?

[jprichardson]

Just be sure to reference or rebase against the latest master. If you need to create new fixtures, that's fine as well. Just put them in lib/copy/__tests__/fixtures/.

[Narigo]

Okay, I hope this will work out now. I've changed the equality check of the modification time / access time. I'm testing it on Node 0.12.4 on a Mac OSX, not sure about other setups, sorry...

[jprichardson]

The millisecond precision issue still exists. You can see exactly what I'm talking about here:

• (original PR): https://travis-ci.org/jprichardson/node-fs-extra/jobs/67825340 (scroll all the way down)
• (your new modification): https://travis-ci.org/jprichardson/node-fs-extra/jobs/67842466 (scroll all the way down)... notice the 460 millisecond difference?

It seems that on Windows/Linux Node v0.12 / io.js platform issue? All tests pass on Node v0.10 on every platform. Any ideas?

[jprichardson]

Perhaps a Node.js/io.js bug introduced in fs.utimes and fs.utimesSync?

[jprichardson]

Ah, here we go:

• Node 0.12.0: fs.utimes does not set sub-second time on windows nodejs/node-v0.x-archive#9369
• File timestamp resolution is inconsistent with fs.stat / fs.utimes nodejs/node-v0.x-archive#7000

[jprichardson]

Ok, so it looks like fs.lstat returned only second precision in Node v0.10, hence why tests are passing. In Node v0.11 (hence 0.12 / io.js), fs.lstat returns millisecond precision and fs.utimes only supports setting second precision. So as I see it, we have two solutions:

1. Reduce our timestamps to second precision on return from fs.lstat. (Easy: Math.floor(timestamp / 1000) * 1000)
2. Use fs.futimes. But that would require us to call fs.open and fs.close as well but we have the benefit of maintaining millisecond precision.

My sense is to say that let's keep the code simple, and go with option 1. Does anyone really care that much about millisecond precision? If it becomes an issue, we could always choose option 2 in the future.

What are your thoughts?

[jprichardson]

@goloroden what are your thoughts on the two options?

[goloroden]

Well, the point is that this feature is only useful if the timestamps are exactly the same. Our initial problem was that docker build regarded files as changed even if nothing had changed except the timestamp.

Hence, I'd vote for option 2: Otherwise it's not really preserving the timestamp, but only almost doing so (which in the end is the same as not preserving it at all).

So, I'd vote for option 2 to really be correct (although I have to admit that it's not nice, but at least it's correct).

[jprichardson]

@goloroden yeah, good point. @Narigo we could create two methods fs.utimesMillis (sync too) and place them in ./lib/util/, these methods would then do the fs.open, fs.futimes, fs.close craziness. This would help us to keep the fs.copy and fs.copySync code as it now without introducing any additional complexity.

[goloroden]

@jprichardson Thanks :-)

And @Narigo: Thank you very much for taking the time to fix / implement this :-)!

[Narigo]

@jprichardson can you merge and implement that? :) I won't be able to do it today anymore and am gone for almost a week. Not sure where I can squeeze that in... :(

[Narigo]

@jprichardson are you working on it right now? I've seen your commits for the utimes util file. I'd have about 30 minutes to try and see how far I can get now (sleep's overrated)

[jprichardson]

Go for it. I'm probably done for the day on it :) Most of it should be working other than the utimesMillisSync function.

[Narigo]

Looks like AppVeyor doesn't like me. Travis works now, though... no time to debug/fix the appveyor part anymore :(

[jprichardson]

I've been unable to get this to work on Windows. I believe it to be a bug in Node.js/io.js. Submitted bug report: nodejs/node#2069

[Mey1212]

res.on('data', function(chunkBuffer) {
body += chunkBuffer.toString('utf8');
});