sepia icon indicating copy to clipboard operation
sepia copied to clipboard

Fixed EINVAL error with fs.utimesSync() due to incorrect Unix timestamp

Open mseminatore opened this issue 8 years ago • 8 comments

This should address issue #17

mseminatore avatar Dec 01 '16 20:12 mseminatore

@mseminatore: Thanks for the PR. Two things:

  • Since I don't work at LinkedIn anymore, we'll have to track down someone who can merge into this repositor. Or, you can open up a PR on my fork of this project.

  • I'm not sure I understand why this problem just became apparent now. Can you provide a test case that demonstrates the problem by failing before your change and passing afterwards?

avik-das avatar Dec 02 '16 07:12 avik-das

@avik-das I can reproduce this using the following code:

// utimes-test.js
var fs = require('fs');
var now = Date.now();

fs.utimesSync('a.txt', now, now);
console.log("Success!");

c:\sepia-test>echo hello > a.txt c:\sepia-test>node utimes-test.js

I am not certain why this wasn't an issue either. Maybe the prior code works on other OS's or different Node versions. In any case the current NodeJS documentation is clear that Date.now() can't be used as is for atime and mtime.

Snipped:

Note: the arguments atime and mtime of the following related functions follow these rules:

- The value should be a Unix timestamp in seconds. For example, Date.now() returns milliseconds, so it should be divided by 1000 before passing it in.
- If the value is a numeric string like '123456789', the value will get converted to the corresponding number.
- If the value is NaN or Infinity, the value will get converted to Date.now() / 1000.

mseminatore avatar Dec 02 '16 16:12 mseminatore

@avik-das OK, PR to your fork sent as well.

mseminatore avatar Dec 02 '16 18:12 mseminatore

@avik-das Do you have any contact with anyone who can accept this PR? It would be nice to have this fix integrated.

mseminatore avatar Dec 26 '16 00:12 mseminatore

@mseminatore - This repo appears to be abandoned. A couple of us created a fork called "replayer" to start accepting commits. You'll need to rename sepia=>replayer.

Could you submit your PR there?

aneilbaboo avatar Mar 24 '17 11:03 aneilbaboo

@aneilbaboo I've not done this before. Can you share the specific steps that I would need to take? I assume it is more than simply renaming my forked repository. That won't connect my fork to your repo though will it?

@avik-das Have you had any luck contacting the LinkedIn owners?

mseminatore avatar Mar 24 '17 17:03 mseminatore

@mseminatore - That won't work.

I think this is right, but haven't checked it:

  1. fork replayer & clone replayer

  2. add a remote pointing at your sepia repo git remote add mysepia [email protected]:msemiatore/sepia.git

  3. checkout your sepia master branch to a new local branch in your local replayer repo, giving it a new name, to avoid confusing it with the replayer master branch. E.g., call it the einval branch. git fetch mysepia && git checkout -b einval --track mysepia/master

  4. checkout the replayer master branch git checkout master

  5. merge your branch into master and enjoy fixing the merge conflicts git merge einval This should mostly be renamings of sepia => replayer in your code.

  6. issue a pull request

aneilbaboo avatar Mar 25 '17 00:03 aneilbaboo

If you're having trouble or would just prefer me to do it, that's fine too.

I took a look at the changes. They seem fine.

aneilbaboo avatar Mar 25 '17 00:03 aneilbaboo