-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix FS moves across Unix mounts #13044
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
Changes from all commits
771c0dd
d7cdc00
ceedf27
c7cba1f
32932ed
5827bda
f5f30c8
618aea9
7a2c1fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,12 @@ public sealed partial class FileSystemProvider : NavigationCmdletProvider, | |
| ISecurityDescriptorCmdletProvider, | ||
| ICmdletProviderSupportsHelp | ||
| { | ||
| #if UNIX | ||
| // This is the errno returned by the rename() syscall | ||
| // when an item is attempted to be renamed across filesystem mount boundaries. | ||
| private const int UNIX_ERRNO_EXDEV = 18; | ||
| #endif | ||
|
|
||
| // 4MB gives the best results without spiking the resources on the remote connection for file transfers between pssessions. | ||
| // NOTE: The script used to copy file data from session (PSCopyFromSessionHelper) has a | ||
| // maximum fragment size value for security. If FILETRANSFERSIZE changes make sure the | ||
|
|
@@ -6001,15 +6007,7 @@ private void MoveDirectoryInfoItem( | |
|
|
||
| try | ||
| { | ||
| if (!IsSameVolume(directory.FullName, destination)) | ||
| { | ||
| CopyAndDelete(directory, destination, force); | ||
| } | ||
| else | ||
| { | ||
| // Move the file | ||
| directory.MoveTo(destination); | ||
| } | ||
| MoveDirectoryInfoUnchecked(directory, destination, force); | ||
|
|
||
| WriteItemObject( | ||
| directory, | ||
|
|
@@ -6027,14 +6025,7 @@ private void MoveDirectoryInfoItem( | |
| directory.Attributes = | ||
| directory.Attributes & ~(FileAttributes.ReadOnly | FileAttributes.Hidden); | ||
|
|
||
| if (!IsSameVolume(directory.FullName, destination)) | ||
| { | ||
| CopyAndDelete(directory, destination, force); | ||
| } | ||
| else | ||
| { | ||
| directory.MoveTo(destination); | ||
| } | ||
| MoveDirectoryInfoUnchecked(directory, destination, force); | ||
|
|
||
| WriteItemObject(directory, directory.FullName, true); | ||
| } | ||
|
|
@@ -6072,6 +6063,47 @@ private void MoveDirectoryInfoItem( | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Implements the file move operation for directories without handling any error scenarios. | ||
| /// In particular, this attempts to rename or copy+delete the file, | ||
| /// but passes any exceptional behavior through to the caller. | ||
| /// </summary> | ||
| /// <param name="directory">The directory to move.</param> | ||
| /// <param name="destinationPath">The destination path to move the directory to.</param> | ||
| /// <param name="force">If true, force move the directory, overwriting anything at the destination.</param> | ||
| private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinationPath, bool force) | ||
| { | ||
| #if UNIX | ||
| try | ||
| { | ||
| if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) | ||
| { | ||
| throw new IOException("Invalid cross-device link", hresult: UNIX_ERRNO_EXDEV); | ||
| } | ||
|
|
||
| directory.MoveTo(destinationPath); | ||
rjmholt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| catch (IOException e) when (e.HResult == UNIX_ERRNO_EXDEV) | ||
| { | ||
| // Rather than try to ascertain whether we can rename a directory ahead of time, | ||
| // it's both faster and more correct to try to rename it and fall back to copy/deleting it | ||
| // See also: https://github.com/coreutils/coreutils/blob/439741053256618eb651e6d43919df29625b8714/src/mv.c#L212-L216 | ||
| CopyAndDelete(directory, destinationPath, force); | ||
| } | ||
| #else | ||
| // On Windows, being able to rename vs copy/delete a file | ||
| // is just a question of the drive | ||
| if (IsSameWindowsVolume(directory.FullName, destinationPath)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .Net uses MOVEFILE_COPY_ALLOWED flag for MoveFileEx() on Windows. Another thought is that we can a concern about performance (rename on the same volume is very fast but cross-volume one is a coping that can take many time.) and then we need to add a new parameter
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's very interesting. I did a small experiment with PS 7.1 and got this result though:
Personally I think user expectation will be for
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember correctly Windows Explorer on XP warned about cross-volume coping but later (Windows 7?) the behavior was changed. Perhaps we could follow this.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "user expectation will be for Move-Item to transparently deal with cross-volume moves without needing to specify a flag" I can't speak for all users but I can confirm that this is what I would expect as a user. :-) Regarding *nix specific flags, a bit more context of my scenario....The real value of running PowerShell (my opinion of course) on Linux is no longer having to maintain two separate shell script versions (bash and command prompt). If we start setting a precedence of having to use special flags for Linux or Microsoft or even differently named cmdlets, (Move-LinuxFile Move-MSFile) then as consumers, we likely have to add in if ($Linux) {#Linux ways} else {#Microsoft ways} at which point we might as maintain separate shell scripts. For the record, throwing errors should always be avoided from a user frustration standpoint but I would be ok if an error was displayed by default and the CopyAndDelete would happen when -Force is provided. With that said I like @rjmholt's solution and from one dev to another, I appreciate everyone's effort on this one!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjmholt Can you remove the check on Windows and check cross-volume move? I expect it is work. |
||
| { | ||
| directory.MoveTo(destinationPath); | ||
| } | ||
| else | ||
| { | ||
| CopyAndDelete(directory, destinationPath, force); | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| private void CopyAndDelete(DirectoryInfo directory, string destination, bool force) | ||
| { | ||
| if (!ItemExists(destination)) | ||
|
|
@@ -6107,13 +6139,15 @@ private void CopyAndDelete(DirectoryInfo directory, string destination, bool for | |
| } | ||
| } | ||
|
|
||
| private bool IsSameVolume(string source, string destination) | ||
| #if !UNIX | ||
| private bool IsSameWindowsVolume(string source, string destination) | ||
| { | ||
| FileInfo src = new FileInfo(source); | ||
| FileInfo dest = new FileInfo(destination); | ||
|
|
||
| return (src.Directory.Root.Name == dest.Directory.Root.Name); | ||
| } | ||
| #endif | ||
|
|
||
| #endregion MoveItem | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
18 appears to be correct for Linux, macOS, and FreeBSD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check the error code is 18 for all the variants we support especially Raspbian and Alpine, which could have it a bit different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed on RPi:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed on Alpine: