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

feat(tests): Test that mtime is the same after move or rename #43689

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

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Feb 20, 2024

Summary

There is no clear consensus on whether copy should preserve mtime, but rename or move should definetly preserve it.
So adding some tests for this, and if failure are found I may fix some of them in the PR as well.

Checklist

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added the 2. developing Work in progress label Feb 20, 2024
@come-nc come-nc added this to the Nextcloud 29 milestone Feb 20, 2024
@come-nc come-nc self-assigned this Feb 20, 2024
…erved

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
$result = copy($this->getSourcePath($source), $this->getSourcePath($target));
$sourceInternalPath = $this->getSourcePath($source);
$targetInternalPath = $this->getSourcePath($target);
$result = copy($sourceInternalPath, $targetInternalPath);

Check failure

Code scanning / Psalm

TaintedFile Error

Detected tainted file handling
$result = copy($this->getSourcePath($source), $this->getSourcePath($target));
$sourceInternalPath = $this->getSourcePath($source);
$targetInternalPath = $this->getSourcePath($target);
$result = copy($sourceInternalPath, $targetInternalPath);

Check failure

Code scanning / Psalm

TaintedFile Error

Detected tainted file handling
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the enh/test-mtime-after-move branch from 53a8d98 to d37436a Compare February 22, 2024 11:08
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@come-nc come-nc modified the milestones: Nextcloud 29, Nextcloud 30 Mar 12, 2024
@trendco
Copy link

trendco commented May 14, 2024

At the moment I find it terrible! If you sync with the Windows client, the date is retained. If you add files via WebDav or upload them via the browser, the date is changed to the current date. If you copy within the NC via the browser, it is also set to the current date. If you move it in the browser, it remains the same, but only if you move it to the internal memory! If you move from an externally mounted storage (e.g. smb/cifs), it is also changed to the current date. What a mess

Some want it this way, others that way. Why don't you make this variable, i.e. customisable?

You could set a default via the admin settings, but users could customise this in their own settings.

And you could, for example, insert a checkbox ‘Keep file date’ when copying/moving via the browser, then everyone can decide for themselves what suits them.

It would then be very flexible and everyone could set it as it suits them.

This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@come-nc come-nc removed this from the Nextcloud 30 milestone Aug 13, 2024
@skjnldsv skjnldsv marked this pull request as draft November 15, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants