Skip to content
New issue

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

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

Already on GitHub? # to your account

mv: fallback to copy only if source and destination are on a different device #7152

Closed
wants to merge 2 commits into from

Conversation

jfinkels
Copy link
Collaborator

Make mv command fallback to copy only if the src and dst are on different device.

Fixes #6029

This is the same pull request #6040, I've just updated the Cargo.lock file.

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/stdbuf is no longer failing!

Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/perm-1. tests/mv/perm-1 is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/stdbuf is no longer failing!

@jfinkels
Copy link
Collaborator Author

Hm the test is failing because std::fs::rename is unexpectedly returning error

 Os {
    code: 5,
    kind: PermissionDenied,
    message: "Access is denied.",
}

on Windows. I don't work on Windows so this will be too hard for me to debug. I'll close this for now.

@jfinkels jfinkels closed this Jan 18, 2025
@sylvestre
Copy link
Contributor

@jfinkels you can just ignore the test on Windows with a comment :)

@jfinkels
Copy link
Collaborator Author

Okay, I can do that and open a new issue for it

@jfinkels jfinkels reopened this Jan 18, 2025
Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/perm-1. tests/mv/perm-1 is passing on 'main'. Maybe you have to rebase?

@jfinkels jfinkels force-pushed the mv-copy-if-not-same-device branch 6 times, most recently from 00e3085 to 64879e2 Compare January 18, 2025 18:50
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@jfinkels jfinkels force-pushed the mv-copy-if-not-same-device branch from 6b2477d to c242b86 Compare January 18, 2025 20:20
Copy link

GNU testsuite comparison:

GNU test failed: tests/misc/stdbuf. tests/misc/stdbuf is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@jfinkels jfinkels marked this pull request as ready for review January 18, 2025 20:49
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/cksum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-base64 is no longer failing!
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@jfinkels jfinkels force-pushed the mv-copy-if-not-same-device branch from 17efece to f1f2be9 Compare February 1, 2025 14:45
Copy link

github-actions bot commented Feb 1, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/usage_vs_getopt (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@hamflx
Copy link
Contributor

hamflx commented Feb 4, 2025

Hm the test is failing because std::fs::rename is unexpectedly returning error

 Os {
    code: 5,
    kind: PermissionDenied,
    message: "Access is denied.",
}

on Windows. I don't work on Windows so this will be too hard for me to debug. I'll close this for now.

@sylvestre

I think this error should not be ignored. It occurs because the file is opened, leading to this error. However, if we fallback to the copy-and-delete logic, the move can succeed. In other words, on Windows, when a file is opened, MoveFile will fail with the error "Access is denied," but DeleteFile can still be used. Determining whether deletion is possible under "Access is denied" scenarios will be challenging, which makes it difficult to proceed with this PR.

@hamflx
Copy link
Contributor

hamflx commented Feb 4, 2025

A file on Windows can exist in the following two states:

  1. The file is open but can still be deleted. In this case, using MoveFile will fail, and we should fallback to the copy-and-delete logic.

  2. The file is open and cannot be deleted. In this scenario, we should not fallback to the copy-and-delete logic.

My previous PR overlooked the first case. Before merging the PR, we need to find a way to distinguish between these two scenarios.

@jfinkels
Copy link
Collaborator Author

jfinkels commented Feb 5, 2025

you can just ignore the test on Windows with a comment :)

The changes here cause the existing test test_mv_arg_update_short_overwrite to fail on Windows, so it's not just as simple as ignoring the new test on Windows. So I'm going to close this, as this is more @hamflx 's code than mine and I can't really get my head around Windows code.

My previous PR overlooked the first case. Before merging the PR, we need to find a way to distinguish between these two scenarios.

Better to just update the pull request from your branch. Thanks

@jfinkels jfinkels closed this Feb 6, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the mv command takes a long time even if the src and dest on same device.
3 participants