Going to change this to try Move() first, then if AccessDenied, then try CopyDelete().

Making this a draft PR for now, the customer tried a private and still had initial problem after latest change. Trying to determine if they got a different HResult.

Looking Directory.MoveTo() implementation https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs#L119-L137
I think we can throw for ERROR_PATH_NOT_FOUND and ERROR_ALREADY_EXISTS but fall back for other exceptions.

@iSazonov Looks like there can be different HResult for DFS, so can't rely on that. Based on https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs#L133, I think we can just try copydelete on an IOException.

If we look GetExceptionForWin32Error()
https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/Common/src/System/IO/Win32Marshal.cs#L25
I think we should fall back for IOException and do not fall back for FileNotFoundException, DirectoryNotFoundException, PathTooLongException which is IOException too.

GitHub

dotnet/runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps. - dotnet/runtime

@iSazonov I see, that makes sense, will update.

Need to think about this some more, the FQErrorID is now different on Linux vs Windows which is not acceptable. Maybe I can find out if there is a limited set of HResult we can check against.

We already discussed this with @rjmholt . See https://github.com/PowerShell/PowerShell/pull/13044/files#r446770600

I guess .Net unify the behavior (exceptions) and we can have one code path for all platforms.

I see the test It "Verify Move-Item across devices for directory" is not correct too. It came from from #13044 . I suggest to re-review #13044 now since the code doesn't work and isn't tested correctly neither Windows nor Unix.

GitHub

Fix FS moves across Unix mounts by rjmholt · Pull Request #13044 · PowerShell/PowerShell

PR Summary Fixes #13039. Currently Move-Item does not support cross-mount moves on Unix, because it doesn't check properly whether given paths lie on the same filesystem. This PR fixes the chec...

@iSazonov can you be specific on your concerns about that test?

@iSazonov can you be specific on your concerns about that test?

I'd expect Move-Item -Path $dir -Destination $destination -ErrorAction Stop throws in the test.

https://github.com/rjmholt/PowerShell/blob/7a2c1fb06cfddff1bb9f2535197154ff143aeefa/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1#L134-L152

And I guess .Net team unified exceptions in the API for all platforms and we could have one code path here. Both return codes fall in IOException and we need only to ignore FileNotFoundException, DirectoryNotFoundException, PathTooLongException.

We must take into account that there are many ways to mount a different file system / volume and they may have different return codes.

@iSazonov so that test case relies on a testhook to simulate doing a move across devices/volumes. It succeeds because the test hook forces it to throw the exception that results in CopyAndDelete() to be used.

In one of my commits, I tried the approach of ignoring FileNotFoundException, DirectoryNotFoundException, PathTooLongException but that resulted in another FullyQualifiedErrorId to change whereas the current implementation checking for specific HResult which is specific to the Unix error of moving across devices and Windows error of moving across volumes has less impact. I was also concerned that other specific IOExceptions beyond those ignored types would fallback to CopyAndDelete() when it didn't in the past and might cause unintended side effects.

@iSazonov so that test case relies on a testhook to simulate doing a move across devices/volumes. It succeeds because the test hook forces it to throw the exception that results in CopyAndDelete() to be used.

Sorry, my bad. With -Stop parameter I expect -Should -Not -Throw -Because ""

In one of my commits, I tried the approach of ignoring FileNotFoundException, DirectoryNotFoundException, PathTooLongException but that resulted in another FullyQualifiedErrorId to change whereas the current implementation checking for specific HResult which is specific to the Unix error of moving across devices and Windows error of moving across volumes has less impact. I was also concerned that other specific IOExceptions beyond those ignored types would fallback to CopyAndDelete() when it didn't in the past and might cause unintended side effects.

We had similar discussion in #13634 (comment)
I tried to make minimal changes. But after some conversations we concluded that driver error codes is not reliable way in that context and I made more large refactoring.
So here I also expect that using driver codes is the last thing we should do.
We could think about moving to OneDrive but this works. WebDav? Other Cloud services? Etc.?
It is good if a driver supports moving, but if not? What code will it return? We do not know. I suppose you could always try copying.

My proposal:

        private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinationPath, bool force)
        {
            try
            {
                if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory)
                {
                    throw new IOException("Invalid cross-device link", hresult: TEST_CODE);
                }

                directory.MoveTo(destinationPath);
            }
            catch (Exception exception)
            {
                switch (exception)
                {
                    case FileNotFoundException:
                    case DirectoryNotFoundException:
                    case PathTooLongException:
                        throw;
                    case IOException:
                        CopyAndDelete(directory, destinationPath, force);
                        break;
                    default:
                        throw;
                }
            }
        }

🎉v7.2.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes