Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/System.Management.Automation/engine/Utils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2080,6 +2080,8 @@ public static class InternalTestHooks

internal static bool ShowMarkdownOutputBypass;

internal static bool ThrowExdevErrorOnMoveDirectory;

/// <summary>This member is used for internal test purposes.</summary>
public static void SetTestHook(string property, object value)
{
Expand Down
70 changes: 52 additions & 18 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

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.

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed on RPi:

pi@daedalus:~ $ errno -l | grep EXDEV
EXDEV 18 Invalid cross-device link

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed on Alpine:

/ # errno -l | grep EXDEV
EXDEV 18 Cross-device link

#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
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

.Net uses MOVEFILE_COPY_ALLOWED flag for MoveFileEx() on Windows.
So I think we can remove the check at all on all platforms but only check catch (IOException e) when (e.HResult == UNIX_ERRNO_EXDEV) on Unix-s.

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 Move-Item -AllowCrossVolumeCoping to explicitly allow the behavior for all platforms. It is my preference. But how we will detect cross-volume on Windows if root path can be the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.Net uses MOVEFILE_COPY_ALLOWED flag for MoveFileEx() on Windows.

That's very interesting. I did a small experiment with PS 7.1 and got this result though:

C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !]
> gi .\tools_copy\ | Sv t
C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !]
> $t.MoveTo('D:\tools')
MethodInvocationException: Exception calling "MoveTo" with "1" argument(s): "Source and destination path must have identical roots. Move will not work across volumes."
C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !]
> get-error

Exception             :
    Type           : System.Management.Automation.MethodInvocationException
    ErrorRecord    :
        Exception             :
            Type    : System.Management.Automation.ParentContainsErrorRecordException
            Message : Exception calling "MoveTo" with "1" argument(s): "Source and destination path
must have identical roots. Move will not work across volumes."
            HResult : -2146233087
        CategoryInfo          : NotSpecified: (:) [], ParentContainsErrorRecordException
        FullyQualifiedErrorId : IOException
        InvocationInfo        :
            ScriptLineNumber : 1
            OffsetInLine     : 1
            HistoryId        : -1
            Line             : $t.MoveTo('D:\tools')
            PositionMessage  : At line:1 char:1
                               + $t.MoveTo('D:\tools')
                               + ~~~~~~~~~~~~~~~~~~~~~
            CommandOrigin    : Internal
        ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
    TargetSite     :
        Name          : ConvertToMethodInvocationException
        DeclaringType : System.Management.Automation.ExceptionHandlingOps,
System.Management.Automation, Version=7.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35
        MemberType    : Method
        Module        : System.Management.Automation.dll
    StackTrace     :
   at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception
exception, Type typeToThrow, String methodName, Int32 numArgs, MemberInfo memberInfo)
   at CallSite.Target(Closure , CallSite , Object , String )
   at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
   at System.Management.Automation.Interpreter.DynamicInstruction`3.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame
frame)
    Message        : Exception calling "MoveTo" with "1" argument(s): "Source and destination path
must have identical roots. Move will not work across volumes."
    Data           : System.Collections.ListDictionaryInternal
    InnerException :
        Type       : System.IO.IOException
        TargetSite :
            Name          : MoveTo
            DeclaringType : System.IO.DirectoryInfo
            MemberType    : Method
            Module        : System.IO.FileSystem.dll
        StackTrace :
   at System.IO.DirectoryInfo.MoveTo(String destDirName)
   at CallSite.Target(Closure , CallSite , Object , String )
        Message    : Source and destination path must have identical roots. Move will not work
across volumes.
        Source     : System.IO.FileSystem
        HResult    : -2146232800
    Source         : System.Management.Automation
    HResult        : -2146233087
CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
FullyQualifiedErrorId : IOException
InvocationInfo        :
    ScriptLineNumber : 1
    OffsetInLine     : 1
    HistoryId        : -1
    Line             : $t.MoveTo('D:\tools')
    PositionMessage  : At line:1 char:1
                       + $t.MoveTo('D:\tools')
                       + ~~~~~~~~~~~~~~~~~~~~~
    CommandOrigin    : Internal
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1

Another thought is that we can a concern about performance

Personally I think user expectation will be for Move-Item to transparently deal with cross-volume moves without needing to specify a flag. mv doesn't require one and nor does .NET on Windows. Throwing an error is unhelpful and technically not performant -- it performs no useful work. I think the best way to get performance here is probably to optimise your mount layout.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Although we should take into account:

  1. Performance - coping will be slow which may be unexpected for the user in an interactive session or it is amazing to see a script slowdown.
  2. Lost security descriptor - after coping permissions are inherited from new root.
  3. Formally it is a breaking change.
  4. Reliability - it is not an atom operation.

Copy link

Choose a reason for hiding this comment

The 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!

Copy link
Collaborator

@iSazonov iSazonov Jun 30, 2020

Choose a reason for hiding this comment

The 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.
(Although the question is how this works for reparse points like OneDrive but it seems we don't detect this whatever).
Or you already did? If so it looks like a bug in .Net :-(

{
directory.MoveTo(destinationPath);
}
else
{
CopyAndDelete(directory, destinationPath, force);
}
#endif
}

private void CopyAndDelete(DirectoryInfo directory, string destination, bool force)
{
if (!ItemExists(destination))
Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,36 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" {
"$destDir/$testDir/$testFile" | Should -Exist
}

It "Verify Move-Item across devices for directory" {
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('ThrowExdevErrorOnMoveDirectory', $true)
try
{
$dir = (New-Item -Path TestDrive:/dir -ItemType Directory -ErrorAction Stop).FullName
$file = (New-Item -Path "$dir/file.txt" -Value "HELLO" -ErrorAction Stop).Name
$destination = "$TestDrive/destination"

Move-Item -Path $dir -Destination $destination -ErrorAction Stop

$dir | Should -Not -Exist
$destination | Should -Exist
"$destination/$file" | Should -Exist
}
finally
{
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('ThrowExdevErrorOnMoveDirectory', $false)
}
}

It "Verify Move-Item will not move to an existing file" {
{ Move-Item -Path $testDir -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand"
$error[0].Exception | Should -BeOfType System.IO.IOException
$testDir | Should -Exist
}

It "Verify Move-Item throws correct error for non-existent source" {
{ Move-Item -Path /does/not/exist -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId 'PathNotFound,Microsoft.PowerShell.Commands.MoveItemCommand'
}

It "Verify Move-Item as substitute for Rename-Item" {
$newFile = Move-Item -Path $testFile -Destination $newTestFile -PassThru
$fileExists = Test-Path $newTestFile
Expand Down