-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
On filesystems which do not support birthtime, stats.birthtime can be greater than stats.mtime #2222
Comments
When the operating system does not support birthtime for stat, previously behaviour was to use the ctime (last changed) time instead. This change instead enforces that uv_fs_stat will use the oldest available time (of ctime, atime, and mtime) rather than arbitrarily picking ctime. Fixes: nodejs/node#2222
When the operating system does not support birthtime for stat, previously behaviour was to use the ctime (last changed) time instead. This change instead enforces that uv_fs_stat will use the oldest available time (of ctime, atime, and mtime) rather than arbitrarily picking ctime. Fixes: nodejs/node#2222
@jorangreef thanks for reporting this. I've submitted a patch to our upstream dependency, libuv, which will hopefully make its way into the node codebase and fix the problem for you! |
When the operating system does not support birthtime for stat, previously behaviour was to use the ctime (last changed) time instead. This change instead enforces that uv_fs_stat will use the oldest available time (of ctime, atime, and mtime) rather than arbitrarily picking ctime. Fixes: nodejs/node#2222
Thanks for the patch @brendanashworth! |
Why is it node's business to fabricate a |
We don't have to close it just yet - it is still pertinent! :)
Not quite node, but libuv wants to provide a consistent API on all platforms. You could open an issue there if you'd like. |
The upstream patch was not accepted because it broke backwards compatibility (and because it fabricated a birthtime when one wasn't available, wink @ivan). @jorangreef I think the best way to solve this on the node.js side is a note in the documentation of |
@brendanashworth sure, I will be away till next week. |
@jorangreef still planing on sending a PR for this? |
@kthelgason if you would like to that would be appreciated, otherwise I will. |
Done. Thanks @jorangreef for raising the issue. You can view the change in #5479 and comment on whether you think this clarifies things. |
If this is going to be accepted as expected result, then I would recommend including a test as well. Perhaps the code in this issue would do? |
Thanks @kthelgason ! @thefourtheye this is only a docs change for clarification, and it's possible in all environments for |
stats.birthtime tracks ctime on filesystems which do not support birthtime, even if stats.ctime > stats.mtime or stats.ctime > stats.atime.
It would be better in this case if stats.birthtime be set to the earliest of all available timestamps.
Here is a test to reproduce, which should pass on OS X and fail on Ubuntu:
The text was updated successfully, but these errors were encountered: