Move file, instead of copy/delete

[malthe]

See https://docs.microsoft.com/en-us/dotnet/api/system.io.file.move?view=net-5.0.

[malthe]

@jborean93 I don't really understand these XML diff test case errors; can you grok what's wrong here?

[jborean93]

Unfortunately the test suite is pretty rudimentary and a few tests are based on the message expectations which needs to match up on what is sent. I can update the expectations myself as I know it's painful for someone to do themselves but first we would need to consider the changes in the PR itself.

Using Move is fine as a first option but the code will need to be able to handle dest paths that exist on a different volume. .NET will raise an exception if you try to move something between volumes so either there needs to be a pre-check beforehand or a fallback on an IOException from this call back to the Copy method from before. Also if the Move suceeds then the Delete action in the finally block will fail and potentially cause a false positive on the client side so that would need updating to check if it is exists before deleting.

[malthe]

@jborean93 I checked the documentation on Delete and it claims that no exception is raised in case the file does not exist. So if the temporary file has a unique name (not sure if it does), then this should be safe.

My PowerShell skills aren't really good enough to know if one might try and catch an exception that specifically targets this situation – moves between volumes.

[jborean93]

I checked the documentation on Delete and it claims that no exception is raised in case the file does not exist

Good catch, looks it does fail if the directory the file is in doesn't exist but in this case I don't think that will ever happen so we can leave it as it is.

My PowerShell skills aren't really good enough to know if one might try and catch an exception that specifically targets this situation – moves between volumes.

I haven't tested it but I would think something like this should work

try {
    [System.IO.File]::Move($path, $output_path, $true)
} catch [IOException] {
    # If the output_path is on a different volume Move will fail, fallback to trying Copy.
    [System.IO.File]::Copy($path, $output_path, $true)
}

This will catch the IOException that Move is meant to throw when trying to move across volumes and fallback to the Copy method which should work. You will need to validate this does actually work though.

[malthe]

From the Move-Item documentation, there should be no problem moving a file between drives since it's the same provider (filesystem).

I did a little test on my PC which has a mapped drive to a UNC location (i.e. fileshare) and moving files to and from this drive using Move-Item works fine.

[jborean93]

From the Move-Item documentation, there should be no problem moving a file between drives since it's the same provider (filesystem).

You are talking about a PowerShell specific feature and it internally would handle the Move with the Copy fallback implementation. This call is the .NET Move function where it explicitly states an IOException is raised when moving across volumes.

An I/O error has occurred, e.g. while copying the file across disk volumes.

The .NET function is basically a thin wrapper (see this and this) around the Win32 MoveFIle function where it explicitly states it will not work across volumes

The one caveat is that the MoveFile function will fail on directory moves when the destination is on a different volume.

[malthe]

@jborean93 when I read that documentation I understand something a little different:

This method works across disk volumes, and it does not throw an exception if the source and destination are the same. Note that if you attempt to replace a file by moving a file of the same name into that directory, an IOException is thrown.

So to get the behavior of overwrite, we'd need to first Delete the target file (which is always okay as long as the target is a file, or doesn't exist) and only then use Move:

[System.IO.File]::Delete($output_path)
[System.IO.File]::Move($path, $output_path)

(The $true argument for overwrite doesn't work on all .NET versions apparently, and even did not work on my own PC. It did work on the remote computer that I'm testing out the library on though.)

Note that I believe with this approach, we'd never have to fall back to Copy since Move internally will do that itself.

[jborean93]

(The $true argument for overwrite doesn't work on all .NET versions apparently, and even did not work on my own PC. It did work on the remote computer that I'm testing out the library on though.)

Ah yes I should have picked up on that, it was only added in .NET Core 3.0 which is no Windows PowerShell version. Good pickup.

Looking a bit closer MoveFile does work across volumes for files, it only calls out directories as a case where it will fail. So I think the Delete and then Move should be fine.

[malthe]

@jborean93 I have pushed an update with this change now.

[jborean93]

Thanks, I'll try and push a commit that updates the test expectations sometime today or tomorrow.

[codecov[bot]]

Codecov Report

Merging #121 (d9f64e0) into master (e4e5130) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files          13       13           
  Lines        3290     3290           
=======================================
  Hits         3238     3238           
  Misses         52       52           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e4e5130...d9f64e0. Read the comment docs.

[jborean93]

Looks like there might be a bug with CI and installing requirements on Windows Python 3.6 but I won't let that hold up the PR. The expectations have been updated and a changelog entry added. Thanks for your work here.