Skip to content

fix(files): preserve mtimes when copying and moving - #62429

Open
mh0pe wants to merge 6 commits into
nextcloud:masterfrom
mh0pe:codex/issue-15192-preserve-mtime
Open

fix(files): preserve mtimes when copying and moving#62429
mh0pe wants to merge 6 commits into
nextcloud:masterfrom
mh0pe:codex/issue-15192-preserve-mtime

Conversation

@mh0pe

@mh0pe mh0pe commented Jul 22, 2026

Copy link
Copy Markdown

Summary

Preserve file and directory modification times consistently when copying and moving files, including same-storage operations, cross-storage fallbacks, and recursive directory copies.

What changed

  • Preserve file and directory mtimes for same-storage and cross-storage copies.
  • Preserve mtimes recursively when moves fall back to copy and delete.
  • Retain known mtimes across storage-wrapper renames and moves.
  • Keep View::copy(..., false) as an explicit opt-out.

Why

Nextcloud already preserves the logical mtime in the file cache while tracking the backing storage timestamp separately as storage_mtime. The storage operations did not apply that model consistently: View::copy() ignored its preservation flag, recursive storage copies dropped the flag, the local fallback dropped it, and the known-mtime wrapper read timestamps after paths had moved.

The copied or moved node now retains its content chronology while the destination parent's mtime and etag still record the operation. Directory copies restore mtimes bottom-up in a post-copy traversal, avoiding a public IStorage signature change for third-party storage implementations.

Validation

  • Affected PHPUnit suites: 622 tests, 2,259 assertions, 3 skips; all passing.
  • PHP syntax checks: passing.
  • XML validation: passing.
  • PHP CS Fixer dry run: passing.
  • git diff --check: passing.
  • Psalm reported 16 missing Imagick symbols outside the changed files; no Psalm errors were reported in the changed production files.
  • On the previous equivalent PR head, 195 of 227 check runs passed. The nine failed jobs were Cypress/Playwright policy gates, their downstream summaries, or runner-level npm permission failures; no backend, storage, PHPUnit, lint, or static-analysis test failed.

TODO

  • No remaining implementation TODOs.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Screenshots before/after for front-end changes — not applicable; this is a backend-only change
  • Documentation (manuals or wiki) has been updated or is not required — documentation is not required
  • Backports requested where applicable (ex: critical bugfixes) — not requested; the target is master
  • Labels added where applicable (ex: bug/enhancement, 3. to review, feature component) — maintainer action required
  • Milestone added for target branch/version (ex: 32.x for stable32) — maintainer action required

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@mh0pe
mh0pe marked this pull request as ready for review July 23, 2026 00:30
@mh0pe
mh0pe requested a review from a team as a code owner July 23, 2026 00:30
@mh0pe
mh0pe requested review from Altahrim, come-nc, leftybournes and salmart-dev and removed request for a team July 23, 2026 00:30

mh0pe commented Jul 29, 2026

Copy link
Copy Markdown
Author

Friendly maintainer ping @Altahrim @salmart-dev @leftybournes @come-nc — this is ready for review. It preserves mtimes across same-storage and cross-storage copy/move paths, including recursive directories, with focused regression coverage. Please let me know if you’d like any additional validation or changes to the scope.

@come-nc come-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks close to what I did in #43689

If I remember correctly @icewind1991 told me at the time that this was not always wanted (for performance reasons I think).

Still, some of what’s in there looks quite straightforward and shoud be merged I think. But maybe not all, especially the recursion on all copied files in a folder.

When comparing with my version it looks like you missed encryption wrapper.

}
}

private function preserveCopyMtime(Storage $sourceStorage, Storage $targetStorage, string $sourceInternalPath, string $targetInternalPath): void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is recursively checking all files in a copied folder, was that tested for performance? Pretty sure it will add a big overhead.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistent handling of last modified dates when copying/moving files

2 participants