-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
See #19710. NetFX behavior is that you can use Directory to move files. This was broken trying to match other legacy behavior in dotnet#17913. Tested on NetFX and CoreFX.
|
||
// Windows will throw if the source file/directory doesn't exist, we preemptively check | ||
// to make sure our cross platform behavior matches NetFX behavior. | ||
if (!FileSystem.Current.DirectoryExists(fullsourceDirName) && !FileSystem.Current.FileExists(fullsourceDirName)) |
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.
It's unfortunate we're now making multiple calls to FillAttributeInfo on Windows and FileExists on Unix: each one appears to do the same thing but then checks whether the result is a directory or a file. Should we add a DirectoryOrFileExists to the pal so that the implementation can do these checks once rather than twice?
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.
Yes, we should. Created #19717 to track doing this post 2.0.
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.
Ok
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 dont need API approval to add FileSystem.PathExists
right now to make it efficient, but I'm fine of course with this as is.
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.
@danmosemsft I do want to make it better- I'm just a little worried about unnecessary changes this late. This stuff is pretty subtle. :)
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.
Yep makes sense to me
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.
Once comment, otherwise LGTM.
Unix impl doesn't tolerate a trailing separator. Public api explicitly checks for this.
Unix won't move the file with a trailing slash. Add more tests for different combinations of trailing slashes. Validated against NetFX.
Trying to handle trailing separator on a source file cases in the same way as Windows.
fullDestDirName = fullDestDirName + PathHelpers.DirectorySeparatorCharAsString; | ||
string destination = Path.GetFullPath(destDirName); | ||
string destinationWithSeparator = destination; | ||
if (destinationWithSeparator[destinationWithSeparator.Length - 1] != Path.DirectorySeparatorChar) |
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.
On Window, what if it's the AltDirectorySeparatorChar?
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.
I'll add that case explicitly
@@ -413,7 +407,9 @@ private static bool DirectoryExists(string fullPath, out Interop.ErrorInfo error | |||
public override bool FileExists(string fullPath) | |||
{ | |||
Interop.ErrorInfo ignored; | |||
return FileExists(fullPath, Interop.Sys.FileTypes.S_IFREG, out ignored); | |||
|
|||
// Windows doesn't care about the trailing separator, str |
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.
"str"?
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.
Whoops- fixing
See #19710. NetFX behavior is that you can use Directory
to move files. This was broken trying to match other legacy
behavior in #17913. Tested on NetFX and CoreFX.