Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mseminatore
Copy link

This should address issue #17

@avik-das
Copy link
Contributor

avik-das commented Dec 2, 2016

@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?

@mseminatore
Copy link
Author

@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
Copy link
Author

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

mseminatore and others added 7 commits December 5, 2016 22:26
As per the node.js documentation for `fs.utimes`:

> 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.

I'm not sure why this wasn't causing a problem for most people, but it
was reported by @mseminatore as a problem on Windows, on node.js
v0.12.2. In any case, this fix doesn't cause a problem on Linux, on
node.js v6.9.1, so I'm happy to make the fix.
- Added myself.
- Added @delwyn, who had made a contribution earlier.
- Added @mseminatore, who contributed recently.
@mseminatore
Copy link
Author

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

@aneilbaboo
Copy link

aneilbaboo commented Mar 24, 2017

@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?

@mseminatore
Copy link
Author

@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?

@aneilbaboo
Copy link

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants