fs: preserve directory timestamps in cp - #65540
Conversation
This fixes an issue where fs.cp and fs.cpSync (when a filter is provided) correctly restore mtime and atime on copied files when preserveTimestamps: true is passed, but skip restoring them for directories. The fix applies the existing setDestTimestamps helper to directories, ensuring it is called after directory creation/copying but before any modes are restored (since restoring a read-only mode would prevent timestamp modification). Note: This fix covers fs.cp() (async, all cases) and fs.cpSync() when a filter is provided. The native fast path used by cpSync() without a filter (CpSyncCopyDir in src/node_file.cc) has the identical gap and is left for a separate follow-up contribution, since it requires native code changes and a different review path. Signed-off-by: Abhinandan Kumar <abhi128618@gmail.com>
70b223c to
321146b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Could someone please check the result of Jenkins CI job 76579? I’m unable to access the job because Jenkins shows “abhi128nandan is missing the Overall/Read permission”, so I can’t see whether there’s an actual test failure or an infrastructure issue. Thanks! |
I woldn't normally see a request like this but I happened to be looking at the job (randomly) for something else so I'll reply. The jobs is still running. The message with the link was only posted 40 minutes ago and it takes longer than that to run through the tests. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65540 +/- ##
==========================================
+ Coverage 90.06% 90.08% +0.01%
==========================================
Files 751 751
Lines 254917 254926 +9
Branches 48133 48136 +3
==========================================
+ Hits 229580 229638 +58
+ Misses 16493 16463 -30
+ Partials 8844 8825 -19
🚀 New features to boost your workflow:
|
|
Could a collaborator please approve/re-run the GitHub Actions and start Jenkins CI for the updated HEAD |
Was there a reason to sync? AFAICT there were no conflicts with current |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
“I’m unable to inspect the Jenkins failure because Jenkins says I’m missing the Overall/Read permission. Could someone check the failing job logs?” |
This comment was marked as outdated.
This comment was marked as outdated.
|
The commit queue failed because my branch contains the 6661a99 merge commit from syncing main. The original PR commit itself was applied successfully. Should I rebase/drop the merge commit and push the updated branch? |
|
Drop the merge commit :-) |
6661a99 to
321146b
Compare
This fixes an issue where fs.cp and fs.cpSync (when a filter is provided) correctly restore mtime and atime on copied files when preserveTimestamps: true is passed, but skip restoring them for directories. The fix applies the existing setDestTimestamps helper to directories, ensuring it is called after directory creation/copying but before any modes are restored (since restoring a read-only mode would prevent timestamp modification). Note: This fix covers fs.cp() (async, all cases) and fs.cpSync() when a filter is provided. The native fast path used by cpSync() without a filter (CpSyncCopyDir in src/node_file.cc) has the identical gap and is left for a separate follow-up contribution, since it requires native code changes and a different review path. Signed-off-by: Abhinandan Kumar <abhi128618@gmail.com> PR-URL: #65540 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
|
@abhi128nandan thank you! Regarding the previous CI runs showing failures, it's caused by random flakes not related to this PR. Nothing to worry about. https://ci.nodejs.org/job/node-test-pull-request/76600/ is green and it's based on |
|
Landed in 8fe4b64 |
This completes the work started in PR nodejs#65540. While the previous PR fixed directory timestamp preservation for the JavaScript paths, the native C++ fast path (CpSyncCopyDir in src/node_file.cc) used by fs.cpSync when no filter is provided still failed to preserve directory timestamps. This commit invokes the existing CopyUtimes helper for both the root destination directory and all subdirectories after their contents are recursively copied. PR-URL: nodejs#65678 Signed-off-by: Abhinandan Kumar <abhi128618@gmail.com>
This completes the work started in PR nodejs#65540. The previous PR fixed directory timestamp preservation for the JavaScript paths, but the native C++ fast path (CpSyncCopyDir in src/node_file.cc) used by fs.cpSync when no filter is provided still failed to preserve directory timestamps. This commit invokes the existing CopyUtimes helper for both the root destination directory and all subdirectories after their contents are recursively copied, aligning the native behavior with the JavaScript fallback. PR-URL: nodejs#65678 Signed-off-by: Abhinandan Kumar <abhi128618@gmail.com>
fs: preserve directory timestamps in cp
This fixes an issue where fs.cp and fs.cpSync (when a filter is
provided) correctly restore mtime and atime on copied files when
preserveTimestamps: true is passed, but skip restoring them for
directories.
The fix applies the existing setDestTimestamps helper to directories,
ensuring it is called after directory creation/copying but before any
modes are restored (since restoring a read-only mode would prevent
timestamp modification).
Note: This fix covers fs.cp() (async, all cases) and fs.cpSync()
when a filter is provided. The native fast path used by cpSync()
without a filter (CpSyncCopyDir in src/node_file.cc) has the
identical gap and is left for a separate follow-up contribution, since
it requires native code changes and a different review path.