From b6ca0d95254d4c9acd5463c38012d72de135c24a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 10:35:19 +0100 Subject: [PATCH 1/7] fix the tests: call the virtual Move method, which is overriden in DirectoryInfo_MoveTo class and allows for testing DirectoryInfo.MoveTo as well --- .../tests/Directory/Move.cs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Move.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Move.cs index c5a145c686cff..167c474ce40e6 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Move.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Move.cs @@ -180,7 +180,7 @@ public void TrailingDirectorySeparators() string testDirDest = Path.Combine(TestDirectory, GetTestFileName()); Directory.CreateDirectory(testDirSource); - Directory.Move(testDirSource + Path.DirectorySeparatorChar, testDirDest + Path.DirectorySeparatorChar); + Move(testDirSource + Path.DirectorySeparatorChar, testDirDest + Path.DirectorySeparatorChar); Assert.True(Directory.Exists(testDirDest)); } @@ -216,7 +216,7 @@ public void Path_Longer_Than_MaxLongPath_Throws_Exception() public void ThrowIOExceptionWhenMovingDirectoryToItself() { Directory.CreateDirectory(Path.Combine(TestDirectory, "foo")); - Assert.Throws(() => Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "foo"))); + Assert.Throws(() => Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "foo"))); } [Fact] @@ -224,7 +224,7 @@ public void ThrowIOExceptionWhenMovingToExistingDirectoryWithSameCase() { Directory.CreateDirectory(Path.Combine(TestDirectory, "foo")); Directory.CreateDirectory(Path.Combine(TestDirectory, "bar", "foo")); - Assert.Throws(() => Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "foo"))); + Assert.Throws(() => Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "foo"))); } [Fact] @@ -233,7 +233,7 @@ public void ToNewDirectoryButWithDifferentCasing() Directory.CreateDirectory(Path.Combine(TestDirectory, "foo")); var otherDirectory = Path.Combine(TestDirectory, "bar"); Directory.CreateDirectory(Path.Combine(otherDirectory)); - Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(otherDirectory, "FOO")); + Move(Path.Combine(TestDirectory, "foo"), Path.Combine(otherDirectory, "FOO")); Assert.True(Directory.Exists(Path.Combine(otherDirectory, "FOO"))); Assert.False(Directory.Exists(Path.Combine(TestDirectory, "foo"))); } @@ -245,7 +245,7 @@ public void SameDirectoryWithDifferentCasing_WithFileContent() var fooDirectoryUppercase = Path.Combine(TestDirectory, "FOO"); Directory.CreateDirectory(fooDirectory); File.WriteAllText(Path.Combine(fooDirectory, "bar.txt"), string.Empty); - Directory.Move(fooDirectory, fooDirectoryUppercase); + Move(fooDirectory, fooDirectoryUppercase); var firstFile = Directory.GetFiles(fooDirectoryUppercase); Assert.Equal("bar.txt", Path.GetFileName(firstFile[0])); } @@ -255,7 +255,7 @@ public void WithDifferentRootCase() { Directory.CreateDirectory($"{TestDirectory}/bar"); var root = Path.GetPathRoot(TestDirectory); - Directory.Move($"{TestDirectory}/bar".Replace(root, root.ToLower()), $"{TestDirectory}/foo"); + Move($"{TestDirectory}/bar".Replace(root, root.ToLower()), $"{TestDirectory}/foo"); Assert.True(Directory.Exists($"{TestDirectory}/foo")); Assert.False(Directory.Exists($"{TestDirectory}/bar")); } @@ -267,7 +267,7 @@ public void SameDirectoryWithDifferentCasing_WithDirectoryContent() var fooDirectoryPathUpperCase = Path.Combine(TestDirectory, "FOO"); Directory.CreateDirectory(fooDirectoryPath); Directory.CreateDirectory(Path.Combine(fooDirectoryPath, "bar")); - Directory.Move(fooDirectoryPath, fooDirectoryPathUpperCase); + Move(fooDirectoryPath, fooDirectoryPathUpperCase); var firstFile = Directory.GetDirectories(fooDirectoryPathUpperCase); Assert.Equal("bar", Path.GetFileName(firstFile[0])); } @@ -282,7 +282,7 @@ public void DirectoryWithDifferentCasingThanFileSystem_ToAnotherDirectory() { Directory.CreateDirectory(Path.Combine(TestDirectory, "FOO")); Directory.CreateDirectory(Path.Combine(TestDirectory, "bar")); - Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "FOO")); + Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "FOO")); } [Fact] @@ -290,7 +290,7 @@ public void DirectoryWithDifferentCasingThanFileSystem_ToAnotherDirectory() public void DirectoryWithDifferentCasingThanFileSystem_ToItself() { Directory.CreateDirectory(Path.Combine(TestDirectory, "FOO")); - Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "FOO")); + Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "FOO")); Assert.True(Directory.Exists(Path.Combine(TestDirectory, "FOO"))); } @@ -300,7 +300,7 @@ public void DirectoryWithDifferentCasingThanFileSystem_ToAnotherDirectory_CaseSe { Directory.CreateDirectory(Path.Combine(TestDirectory, "FOO")); Directory.CreateDirectory(Path.Combine(TestDirectory, "bar")); - Assert.Throws(() => Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "FOO"))); + Assert.Throws(() => Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "bar", "FOO"))); } [Fact] @@ -308,7 +308,7 @@ public void DirectoryWithDifferentCasingThanFileSystem_ToAnotherDirectory_CaseSe public void DirectoryWithDifferentCasingThanFileSystem_ToItself_CaseSensitiveOS() { Directory.CreateDirectory(Path.Combine(TestDirectory, "FOO")); - Assert.Throws(() => Directory.Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "FOO"))); + Assert.Throws(() => Move(Path.Combine(TestDirectory, "foo"), Path.Combine(TestDirectory, "FOO"))); } [Fact] @@ -320,7 +320,7 @@ public void CaseVariantDirectoryNameWithCaseVariantPaths_CaseInsensitiveFileSyst Directory.CreateDirectory(Path.Combine(TestDirectory, "FOO", "bar")); Directory.CreateDirectory(Path.Combine(TestDirectory, "foo")); - Assert.Throws(() => Directory.Move(directoryToBeMoved, Path.Combine(newPath, "bar"))); + Assert.Throws(() => Move(directoryToBeMoved, Path.Combine(newPath, "bar"))); } [Fact] @@ -329,7 +329,7 @@ public void MoveDirectory_FailToMoveDirectoryWithUpperCaseToOtherDirectoryWithLo { Directory.CreateDirectory($"{TestDirectory}/FOO"); Directory.CreateDirectory($"{TestDirectory}/bar/foo"); - Assert.Throws(() => Directory.Move($"{TestDirectory}/FOO", $"{TestDirectory}/bar/foo")); + Assert.Throws(() => Move($"{TestDirectory}/FOO", $"{TestDirectory}/bar/foo")); } [Fact] @@ -338,7 +338,7 @@ public void MoveDirectory_NoOpWhenMovingDirectoryWithUpperCaseToOtherDirectoryWi { Directory.CreateDirectory($"{TestDirectory}/FOO"); Directory.CreateDirectory($"{TestDirectory}/bar/foo"); - Directory.Move($"{TestDirectory}/FOO", $"{TestDirectory}/bar/foo"); + Move($"{TestDirectory}/FOO", $"{TestDirectory}/bar/foo"); Assert.True(Directory.Exists(Path.Combine(TestDirectory, "bar", "foo"))); } @@ -348,7 +348,7 @@ public void MoveDirectory_FailToMoveLowerCaseDirectoryWhenUpperCaseDirectoryExis { Directory.CreateDirectory($"{TestDirectory}/bar/FOO"); Directory.CreateDirectory($"{TestDirectory}/foo"); - Assert.Throws(() => Directory.Move($"{TestDirectory}/foo", $"{TestDirectory}/bar/foo")); + Assert.Throws(() => Move($"{TestDirectory}/foo", $"{TestDirectory}/bar/foo")); } [ConditionalFact(nameof(AreAllLongPathsAvailable))] From 43509b6242f1461eed324a9aca1833c9e51694d4 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 11:17:08 +0100 Subject: [PATCH 2/7] unify the Directory.Move and DirectoryInfo.MoveTo logic, move the common part to FileSystem.MoveDirectory --- .../src/System/IO/Directory.cs | 44 ++--------------- .../src/System/IO/DirectoryInfo.cs | 21 +-------- .../src/System/IO/FileSystem.Unix.cs | 2 +- .../src/System/IO/FileSystem.Windows.cs | 2 +- .../src/System/IO/FileSystem.cs | 47 +++++++++++++++++++ 5 files changed, 53 insertions(+), 63 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs index 2bfcd01604047..2e2a4b8d72dd3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs @@ -248,48 +248,10 @@ public static void Move(string sourceDirName, string destDirName) if (destDirName.Length == 0) throw new ArgumentException(SR.Argument_EmptyFileName, nameof(destDirName)); - string fullsourceDirName = Path.GetFullPath(sourceDirName); - string sourcePath = PathInternal.EnsureTrailingSeparator(fullsourceDirName); + string destinationFullPath = Path.GetFullPath(destDirName); + string destinationWithSeparator = PathInternal.EnsureTrailingSeparator(destinationFullPath); - string fulldestDirName = Path.GetFullPath(destDirName); - string destPath = PathInternal.EnsureTrailingSeparator(fulldestDirName); - - ReadOnlySpan sourceDirNameFromFullPath = Path.GetFileName(fullsourceDirName.AsSpan()); - ReadOnlySpan destDirNameFromFullPath = Path.GetFileName(fulldestDirName.AsSpan()); - - StringComparison fileSystemSensitivity = PathInternal.StringComparison; - bool directoriesAreCaseVariants = - !sourceDirNameFromFullPath.SequenceEqual(destDirNameFromFullPath) && - sourceDirNameFromFullPath.Equals(destDirNameFromFullPath, StringComparison.OrdinalIgnoreCase); - bool sameDirectoryDifferentCase = - directoriesAreCaseVariants && - destDirNameFromFullPath.Equals(sourceDirNameFromFullPath, fileSystemSensitivity); - - // If the destination directories are the exact same name - if (!sameDirectoryDifferentCase && string.Equals(sourcePath, destPath, fileSystemSensitivity)) - throw new IOException(SR.IO_SourceDestMustBeDifferent); - - ReadOnlySpan sourceRoot = Path.GetPathRoot(sourcePath.AsSpan()); - ReadOnlySpan destinationRoot = Path.GetPathRoot(destPath.AsSpan()); - - // Compare paths for the same, skip this step if we already know the paths are identical. - if (!sourceRoot.Equals(destinationRoot, StringComparison.OrdinalIgnoreCase)) - throw new IOException(SR.IO_SourceDestMustHaveSameRoot); - - // Windows will throw if the source file/directory doesn't exist, we preemptively check - // to make sure our cross platform behavior matches .NET Framework behavior. - if (!FileSystem.DirectoryExists(fullsourceDirName) && !FileSystem.FileExists(fullsourceDirName)) - throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, fullsourceDirName)); - - if (!sameDirectoryDifferentCase // This check is to allow renaming of directories - && FileSystem.DirectoryExists(fulldestDirName)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, fulldestDirName)); - - // If the directories aren't the same and the OS says the directory exists already, fail. - if (!sameDirectoryDifferentCase && Directory.Exists(fulldestDirName)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, fulldestDirName)); - - FileSystem.MoveDirectory(fullsourceDirName, fulldestDirName); + FileSystem.MoveDirectory(Path.GetFullPath(sourceDirName), destinationFullPath, destinationWithSeparator); } public static void Delete(string path) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs index 1aeef2f7501a8..ea584a907b462 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs @@ -194,28 +194,9 @@ public void MoveTo(string destDirName) throw new ArgumentException(SR.Argument_EmptyFileName, nameof(destDirName)); string destination = Path.GetFullPath(destDirName); - string destinationWithSeparator = PathInternal.EnsureTrailingSeparator(destination); - string sourceWithSeparator = PathInternal.EnsureTrailingSeparator(FullPath); - - if (string.Equals(sourceWithSeparator, destinationWithSeparator, PathInternal.StringComparison)) - throw new IOException(SR.IO_SourceDestMustBeDifferent); - - string? sourceRoot = Path.GetPathRoot(sourceWithSeparator); - string? destinationRoot = Path.GetPathRoot(destinationWithSeparator); - - if (!string.Equals(sourceRoot, destinationRoot, PathInternal.StringComparison)) - throw new IOException(SR.IO_SourceDestMustHaveSameRoot); - - // Windows will throw if the source file/directory doesn't exist, we preemptively check - // to make sure our cross platform behavior matches .NET Framework behavior. - if (!Exists && !FileSystem.FileExists(FullPath)) - throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, FullPath)); - - if (FileSystem.DirectoryExists(destination)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destinationWithSeparator)); - FileSystem.MoveDirectory(FullPath, destination); + FileSystem.MoveDirectory(FullPath, destination, destinationWithSeparator, Exists); Init(originalPath: destDirName, fullPath: destinationWithSeparator, diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 566d3686ce5e1..749e500ab0298 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -387,7 +387,7 @@ private static void CreateParentsAndDirectory(string fullPath) } } - public static void MoveDirectory(string sourceFullPath, string destFullPath) + private static void MoveDirectory(string sourceFullPath, string destFullPath) { // Windows doesn't care if you try and copy a file via "MoveDirectory"... if (FileExists(sourceFullPath)) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index 3436a7f1428f6..2952099df734c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -154,7 +154,7 @@ public static DateTimeOffset GetLastWriteTime(string fullPath) return data.ftLastWriteTime.ToDateTimeOffset(); } - public static void MoveDirectory(string sourceFullPath, string destFullPath) + private static void MoveDirectory(string sourceFullPath, string destFullPath) { if (!Interop.Kernel32.MoveFile(sourceFullPath, destFullPath, overwrite: false)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs index 09f6000326111..d460689a281d0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs @@ -20,5 +20,52 @@ internal static void VerifyValidPath(string path, string argName) throw new ArgumentException(SR.Argument_InvalidPathChars, argName); } } + + internal static void MoveDirectory(string sourceFullPath, string destFullPath, string destinationWithSeparator, bool? sourceDirectoryExists = default) + { + string sourcePath = PathInternal.EnsureTrailingSeparator(sourceFullPath); + + ReadOnlySpan sourceDirNameFromFullPath = Path.GetFileName(sourceFullPath.AsSpan()); + ReadOnlySpan destDirNameFromFullPath = Path.GetFileName(destFullPath.AsSpan()); + + StringComparison fileSystemSensitivity = PathInternal.StringComparison; + bool directoriesAreCaseVariants = + !sourceDirNameFromFullPath.SequenceEqual(destDirNameFromFullPath) && + sourceDirNameFromFullPath.Equals(destDirNameFromFullPath, StringComparison.OrdinalIgnoreCase); + bool sameDirectoryDifferentCase = + directoriesAreCaseVariants && + destDirNameFromFullPath.Equals(sourceDirNameFromFullPath, fileSystemSensitivity); + + // If the destination directories are the exact same name + if (!sameDirectoryDifferentCase && string.Equals(sourcePath, destinationWithSeparator, fileSystemSensitivity)) + throw new IOException(SR.IO_SourceDestMustBeDifferent); + + ReadOnlySpan sourceRoot = Path.GetPathRoot(sourcePath.AsSpan()); + ReadOnlySpan destinationRoot = Path.GetPathRoot(destinationWithSeparator.AsSpan()); + + // Compare paths for the same, skip this step if we already know the paths are identical. + if (!sourceRoot.Equals(destinationRoot, StringComparison.OrdinalIgnoreCase)) + throw new IOException(SR.IO_SourceDestMustHaveSameRoot); + + if (sourceDirectoryExists is null) + { + sourceDirectoryExists = DirectoryExists(sourceFullPath); + } + + // Windows will throw if the source file/directory doesn't exist, we preemptively check + // to make sure our cross platform behavior matches .NET Framework behavior. + if (!sourceDirectoryExists.Value && !FileExists(sourceFullPath)) + throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + + if (!sameDirectoryDifferentCase // This check is to allow renaming of directories + && DirectoryExists(destFullPath)) + throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + + // If the directories aren't the same and the OS says the directory exists already, fail. + if (!sameDirectoryDifferentCase && Directory.Exists(destFullPath)) + throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + + MoveDirectory(sourceFullPath, destFullPath); + } } } From 896245dd4ac6de913cdbfa9a8ff9ada8803cb9ba Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 11:19:16 +0100 Subject: [PATCH 3/7] remove duplicated check --- .../System.Private.CoreLib/src/System/IO/FileSystem.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs index d460689a281d0..d337a86025074 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs @@ -61,10 +61,6 @@ internal static void MoveDirectory(string sourceFullPath, string destFullPath, s && DirectoryExists(destFullPath)) throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); - // If the directories aren't the same and the OS says the directory exists already, fail. - if (!sameDirectoryDifferentCase && Directory.Exists(destFullPath)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); - MoveDirectory(sourceFullPath, destFullPath); } } From af1cd9b4925bfe7d1c5aafd05aea26eb4f4781a8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 11:58:15 +0100 Subject: [PATCH 4/7] there is no need to perform the extra checks on Windows --- .../src/System/IO/DirectoryInfo.cs | 3 ++- .../src/System/IO/FileSystem.Unix.cs | 16 +++++++++++++++- .../src/System/IO/FileSystem.Windows.cs | 2 +- .../src/System/IO/FileSystem.cs | 16 +--------------- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs index ea584a907b462..3ca339fc40eab 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs @@ -196,7 +196,8 @@ public void MoveTo(string destDirName) string destination = Path.GetFullPath(destDirName); string destinationWithSeparator = PathInternal.EnsureTrailingSeparator(destination); - FileSystem.MoveDirectory(FullPath, destination, destinationWithSeparator, Exists); + FileSystem.MoveDirectory(FullPath, destination, destinationWithSeparator, + OperatingSystem.IsWindows() ? null : Exists); // on Windows we don't need to perform the extra check Init(originalPath: destDirName, fullPath: destinationWithSeparator, diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 749e500ab0298..aa70f27ced20f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -387,8 +387,22 @@ private static void CreateParentsAndDirectory(string fullPath) } } - private static void MoveDirectory(string sourceFullPath, string destFullPath) + private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase, bool? sourceDirectoryExists) { + if (sourceDirectoryExists is null) + { + sourceDirectoryExists = DirectoryExists(sourceFullPath); + } + + // Windows will throw if the source file/directory doesn't exist, we preemptively check + // to make sure our cross platform behavior matches .NET Framework behavior. + if (!sourceDirectoryExists.Value && !FileExists(sourceFullPath)) + throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + + if (!sameDirectoryDifferentCase // This check is to allow renaming of directories + && DirectoryExists(destFullPath)) + throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + // Windows doesn't care if you try and copy a file via "MoveDirectory"... if (FileExists(sourceFullPath)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index 2952099df734c..eb7ad9b0d0b0b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -154,7 +154,7 @@ public static DateTimeOffset GetLastWriteTime(string fullPath) return data.ftLastWriteTime.ToDateTimeOffset(); } - private static void MoveDirectory(string sourceFullPath, string destFullPath) + private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase, bool? sourceDirectoryExists) { if (!Interop.Kernel32.MoveFile(sourceFullPath, destFullPath, overwrite: false)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs index d337a86025074..1eb5971a3a056 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs @@ -47,21 +47,7 @@ internal static void MoveDirectory(string sourceFullPath, string destFullPath, s if (!sourceRoot.Equals(destinationRoot, StringComparison.OrdinalIgnoreCase)) throw new IOException(SR.IO_SourceDestMustHaveSameRoot); - if (sourceDirectoryExists is null) - { - sourceDirectoryExists = DirectoryExists(sourceFullPath); - } - - // Windows will throw if the source file/directory doesn't exist, we preemptively check - // to make sure our cross platform behavior matches .NET Framework behavior. - if (!sourceDirectoryExists.Value && !FileExists(sourceFullPath)) - throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); - - if (!sameDirectoryDifferentCase // This check is to allow renaming of directories - && DirectoryExists(destFullPath)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); - - MoveDirectory(sourceFullPath, destFullPath); + MoveDirectory(sourceFullPath, destFullPath, sameDirectoryDifferentCase, sourceDirectoryExists); } } } From 373a4a29ca3cfa3d47cc5c0f13d982bd5ceaf9d5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 12:05:53 +0100 Subject: [PATCH 5/7] check for source file existience only once --- .../src/System/IO/FileSystem.Unix.cs | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index aa70f27ced20f..32eedaa50d06d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -389,23 +389,18 @@ private static void CreateParentsAndDirectory(string fullPath) private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase, bool? sourceDirectoryExists) { - if (sourceDirectoryExists is null) - { - sourceDirectoryExists = DirectoryExists(sourceFullPath); - } + sourceDirectoryExists ??= DirectoryExists(sourceFullPath); // Windows will throw if the source file/directory doesn't exist, we preemptively check // to make sure our cross platform behavior matches .NET Framework behavior. - if (!sourceDirectoryExists.Value && !FileExists(sourceFullPath)) - throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); - - if (!sameDirectoryDifferentCase // This check is to allow renaming of directories - && DirectoryExists(destFullPath)) - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); - - // Windows doesn't care if you try and copy a file via "MoveDirectory"... - if (FileExists(sourceFullPath)) + if (!sourceDirectoryExists.Value) { + if (!FileExists(sourceFullPath)) + { + throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + } + + // Windows doesn't care if you try and copy a file via "MoveDirectory"... // ... but it doesn't like the source to have a trailing slash ... // On Windows we end up with ERROR_INVALID_NAME, which is @@ -415,17 +410,26 @@ private static void MoveDirectory(string sourceFullPath, string destFullPath, bo // give DirectoryNotFound. if (Path.EndsInDirectorySeparator(sourceFullPath)) + { throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + } // ... but it doesn't care if the destination has a trailing separator. destFullPath = Path.TrimEndingDirectorySeparator(destFullPath); } - if (FileExists(destFullPath)) + if (!sameDirectoryDifferentCase) // This check is to allow renaming of directories { - // Some Unix distros will overwrite the destination file if it already exists. - // Throwing IOException to match Windows behavior. - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + if (DirectoryExists(destFullPath)) + { + throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + } + else if (FileExists(destFullPath)) + { + // Some Unix distros will overwrite the destination file if it already exists. + // Throwing IOException to match Windows behavior. + throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); + } } if (Interop.Sys.Rename(sourceFullPath, destFullPath) < 0) From 02a86644a51883696786aed202e784116e15e1de Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 12 Jan 2022 16:23:46 +0100 Subject: [PATCH 6/7] improve performance --- .../Unix/System.Native/Interop.Rename.cs | 16 +++++ .../src/System/IO/Directory.cs | 5 +- .../src/System/IO/DirectoryInfo.cs | 6 +- .../src/System/IO/FileSystem.Unix.cs | 59 ++++++++----------- .../src/System/IO/FileSystem.Windows.cs | 2 +- .../src/System/IO/FileSystem.cs | 13 ++-- 6 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Rename.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Rename.cs index 8670f15a7ba40..741f4000627c0 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Rename.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Rename.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.InteropServices; +using System.Text; internal static partial class Interop { @@ -18,5 +19,20 @@ internal static partial class Sys /// [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Rename", CharSet = CharSet.Ansi, SetLastError = true)] internal static partial int Rename(string oldPath, string newPath); + + [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Rename", SetLastError = true)] + internal static partial int Rename(ref byte oldPath, ref byte newPath); + + internal static int Rename(ReadOnlySpan oldPath, ReadOnlySpan newPath) + { + ValueUtf8Converter converterNewPath = new(stackalloc byte[DefaultPathBufferSize]); + ValueUtf8Converter converterOldPath = new(stackalloc byte[DefaultPathBufferSize]); + int result = Rename( + ref MemoryMarshal.GetReference(converterOldPath.ConvertAndTerminateString(oldPath)), + ref MemoryMarshal.GetReference(converterNewPath.ConvertAndTerminateString(newPath))); + converterNewPath.Dispose(); + converterOldPath.Dispose(); + return result; + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs index 2e2a4b8d72dd3..b434c2f8ba512 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs @@ -248,10 +248,7 @@ public static void Move(string sourceDirName, string destDirName) if (destDirName.Length == 0) throw new ArgumentException(SR.Argument_EmptyFileName, nameof(destDirName)); - string destinationFullPath = Path.GetFullPath(destDirName); - string destinationWithSeparator = PathInternal.EnsureTrailingSeparator(destinationFullPath); - - FileSystem.MoveDirectory(Path.GetFullPath(sourceDirName), destinationFullPath, destinationWithSeparator); + FileSystem.MoveDirectory(Path.GetFullPath(sourceDirName), Path.GetFullPath(destDirName)); } public static void Delete(string path) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs index 3ca339fc40eab..0f0d72cefe840 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/DirectoryInfo.cs @@ -194,13 +194,11 @@ public void MoveTo(string destDirName) throw new ArgumentException(SR.Argument_EmptyFileName, nameof(destDirName)); string destination = Path.GetFullPath(destDirName); - string destinationWithSeparator = PathInternal.EnsureTrailingSeparator(destination); - FileSystem.MoveDirectory(FullPath, destination, destinationWithSeparator, - OperatingSystem.IsWindows() ? null : Exists); // on Windows we don't need to perform the extra check + FileSystem.MoveDirectory(FullPath, destination); Init(originalPath: destDirName, - fullPath: destinationWithSeparator, + fullPath: PathInternal.EnsureTrailingSeparator(destination), fileName: null, isNormalized: true); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 32eedaa50d06d..8f4775d09e418 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -387,58 +387,49 @@ private static void CreateParentsAndDirectory(string fullPath) } } - private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase, bool? sourceDirectoryExists) + private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase) { - sourceDirectoryExists ??= DirectoryExists(sourceFullPath); + ReadOnlySpan srcNoDirectorySeparator = Path.TrimEndingDirectorySeparator(sourceFullPath.AsSpan()); + ReadOnlySpan destNoDirectorySeparator = Path.TrimEndingDirectorySeparator(destFullPath.AsSpan()); - // Windows will throw if the source file/directory doesn't exist, we preemptively check - // to make sure our cross platform behavior matches .NET Framework behavior. - if (!sourceDirectoryExists.Value) + if (!sameDirectoryDifferentCase) // This check is to allow renaming of directories { - if (!FileExists(sourceFullPath)) - { - throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); - } - - // Windows doesn't care if you try and copy a file via "MoveDirectory"... - // ... but it doesn't like the source to have a trailing slash ... - - // On Windows we end up with ERROR_INVALID_NAME, which is - // "The filename, directory name, or volume label syntax is incorrect." - // - // This surfaces as an IOException, if we let it go beyond here it would - // give DirectoryNotFound. - - if (Path.EndsInDirectorySeparator(sourceFullPath)) + if (Interop.Sys.Stat(destNoDirectorySeparator, out _) >= 0) { - throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); - } + // destination exists, but before we throw we need to check whether source exists or not - // ... but it doesn't care if the destination has a trailing separator. - destFullPath = Path.TrimEndingDirectorySeparator(destFullPath); - } + // Windows will throw if the source file/directory doesn't exist, we preemptively check + // to make sure our cross platform behavior matches .NET Framework behavior. + if (Interop.Sys.Stat(srcNoDirectorySeparator, out Interop.Sys.FileStatus sourceFileStatus) < 0) + { + throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + } + else if ((sourceFileStatus.Mode & Interop.Sys.FileTypes.S_IFMT) != Interop.Sys.FileTypes.S_IFDIR + && Path.EndsInDirectorySeparator(sourceFullPath)) + { + throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + } - if (!sameDirectoryDifferentCase) // This check is to allow renaming of directories - { - if (DirectoryExists(destFullPath)) - { - throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); - } - else if (FileExists(destFullPath)) - { // Some Unix distros will overwrite the destination file if it already exists. // Throwing IOException to match Windows behavior. throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath)); } } - if (Interop.Sys.Rename(sourceFullPath, destFullPath) < 0) + if (Interop.Sys.Rename(sourceFullPath, destNoDirectorySeparator) < 0) { Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo(); switch (errorInfo.Error) { case Interop.Error.EACCES: // match Win32 exception throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, sourceFullPath), errorInfo.RawErrno); + case Interop.Error.ENOENT: + throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, + Interop.Sys.Stat(srcNoDirectorySeparator, out _) >= 0 + ? destFullPath // the source directory exists, so destination does not. Example: Move("/tmp/existing/", "/tmp/nonExisting1/nonExisting2/") + : sourceFullPath)); + case Interop.Error.ENOTDIR: // sourceFullPath exists and it's not a directory + throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); default: throw Interop.GetExceptionForIoErrno(errorInfo, isDirectory: true); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index eb7ad9b0d0b0b..a64f82b5ed498 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -154,7 +154,7 @@ public static DateTimeOffset GetLastWriteTime(string fullPath) return data.ftLastWriteTime.ToDateTimeOffset(); } - private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase, bool? sourceDirectoryExists) + private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase) { if (!Interop.Kernel32.MoveFile(sourceFullPath, destFullPath, overwrite: false)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs index 1eb5971a3a056..99821e0f9e120 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs @@ -21,9 +21,10 @@ internal static void VerifyValidPath(string path, string argName) } } - internal static void MoveDirectory(string sourceFullPath, string destFullPath, string destinationWithSeparator, bool? sourceDirectoryExists = default) + internal static void MoveDirectory(string sourceFullPath, string destFullPath) { - string sourcePath = PathInternal.EnsureTrailingSeparator(sourceFullPath); + ReadOnlySpan srcNoDirectorySeparator = Path.TrimEndingDirectorySeparator(sourceFullPath.AsSpan()); + ReadOnlySpan destNoDirectorySeparator = Path.TrimEndingDirectorySeparator(destFullPath.AsSpan()); ReadOnlySpan sourceDirNameFromFullPath = Path.GetFileName(sourceFullPath.AsSpan()); ReadOnlySpan destDirNameFromFullPath = Path.GetFileName(destFullPath.AsSpan()); @@ -37,17 +38,17 @@ internal static void MoveDirectory(string sourceFullPath, string destFullPath, s destDirNameFromFullPath.Equals(sourceDirNameFromFullPath, fileSystemSensitivity); // If the destination directories are the exact same name - if (!sameDirectoryDifferentCase && string.Equals(sourcePath, destinationWithSeparator, fileSystemSensitivity)) + if (!sameDirectoryDifferentCase && srcNoDirectorySeparator.Equals(destNoDirectorySeparator, fileSystemSensitivity)) throw new IOException(SR.IO_SourceDestMustBeDifferent); - ReadOnlySpan sourceRoot = Path.GetPathRoot(sourcePath.AsSpan()); - ReadOnlySpan destinationRoot = Path.GetPathRoot(destinationWithSeparator.AsSpan()); + ReadOnlySpan sourceRoot = Path.GetPathRoot(srcNoDirectorySeparator); + ReadOnlySpan destinationRoot = Path.GetPathRoot(destNoDirectorySeparator); // Compare paths for the same, skip this step if we already know the paths are identical. if (!sourceRoot.Equals(destinationRoot, StringComparison.OrdinalIgnoreCase)) throw new IOException(SR.IO_SourceDestMustHaveSameRoot); - MoveDirectory(sourceFullPath, destFullPath, sameDirectoryDifferentCase, sourceDirectoryExists); + MoveDirectory(sourceFullPath, destFullPath, sameDirectoryDifferentCase); } } } From 753e3d5c4113cf122b33fea9b81ea3bb7fb23fb3 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 13 Jan 2022 10:35:40 +0100 Subject: [PATCH 7/7] on WASM rename does not fail with ENOTDIR for files that end with / so we need to handle it before rename is called --- .../src/System/IO/FileSystem.Unix.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 8f4775d09e418..c961346524027 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -392,6 +392,15 @@ private static void MoveDirectory(string sourceFullPath, string destFullPath, bo ReadOnlySpan srcNoDirectorySeparator = Path.TrimEndingDirectorySeparator(sourceFullPath.AsSpan()); ReadOnlySpan destNoDirectorySeparator = Path.TrimEndingDirectorySeparator(destFullPath.AsSpan()); + if (OperatingSystem.IsBrowser() && Path.EndsInDirectorySeparator(sourceFullPath) && FileExists(sourceFullPath)) + { + // On Windows we end up with ERROR_INVALID_NAME, which is + // "The filename, directory name, or volume label syntax is incorrect." + // On Unix, rename fails with ENOTDIR, but on WASM it does not. + // So if the path ends with directory separator, but it's a file, we just throw. + throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath)); + } + if (!sameDirectoryDifferentCase) // This check is to allow renaming of directories { if (Interop.Sys.Stat(destNoDirectorySeparator, out _) >= 0)