Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FileSystem: MoveDirectory: some improvements. #65132

Merged
merged 6 commits into from
Apr 8, 2022
Merged

FileSystem: MoveDirectory: some improvements. #65132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f53220a
FileSystem: MoveDirectory: some improvements.
tmds Feb 10, 2022
3a0147c
Fix case-sensitive rename.
tmds Feb 10, 2022
bfb938e
Refactor case sensitive renaming.
tmds Feb 10, 2022
a00cf28
Fix tests
tmds Feb 11, 2022
c0fc66c
Small code cleanup.
tmds Feb 11, 2022
51e7a7c
Merge branch 'main' into movedir
tmds Mar 7, 2022
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
14 changes: 2 additions & 12 deletions src/libraries/System.IO.FileSystem/tests/Directory/Move.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ public void CaseVariantDirectoryNameWithCaseVariantPaths_CaseInsensitiveFileSyst
}

[Fact]
[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.FreeBSD | TestPlatforms.NetBSD)]
[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX | TestPlatforms.FreeBSD | TestPlatforms.NetBSD)]
Copy link
Member

Choose a reason for hiding this comment

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

probably not related but I'm wondering why the *BSD is here:

furt@d13:/tmp $ mkdir test
furt@d13:/tmp $ cd test
furt@d13:/tmp/test $ mkdir FOO
furt@d13:/tmp/test $ mkdir -p bar/foo
furt@d13:/tmp/test $ mv FOO bar
furt@d13:/tmp/test $ ls -al bar/
total 2
drwxr-xr-x  4 furt  wheel  4 Feb 23 22:37 .
drwxr-xr-x  3 furt  wheel  3 Feb 23 22:37 ..
drwxr-xr-x  2 furt  wheel  2 Feb 23 22:36 FOO
drwxr-xr-x  2 furt  wheel  2 Feb 23 22:36 foo

As far as I understand this should fail on case-insensitive filesystem but it should work on case-sensitive one.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to your other comment:

Once again this may be beyond intend but it seems like this should be something like IsCaseInsensitive(TestDirectory)

I agree, it would be better to capture what is assumed and express it via a ConditionalFact rather than hard code a list of OSes.

Should I create a ticket to clean this up?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, it would be better to capture what is assumed and express it via a ConditionalFact rather than hard code a list of OSes.

Should I create a ticket to clean this up?

Sure; thank you. I don't expect we'd get to that any time soon, but it could be filed as https://github.com/dotnet/runtime/labels/up-for-grabs.

public void MoveDirectory_FailToMoveDirectoryWithUpperCaseToOtherDirectoryWithLowerCase()
{
Directory.CreateDirectory($"{TestDirectory}/FOO");
Expand All @@ -333,17 +333,7 @@ public void MoveDirectory_FailToMoveDirectoryWithUpperCaseToOtherDirectoryWithLo
}

[Fact]
[PlatformSpecific(TestPlatforms.OSX)]
public void MoveDirectory_NoOpWhenMovingDirectoryWithUpperCaseToOtherDirectoryWithLowerCase()
jeffhandley marked this conversation as resolved.
Show resolved Hide resolved
{
Directory.CreateDirectory($"{TestDirectory}/FOO");
Directory.CreateDirectory($"{TestDirectory}/bar/foo");
Move($"{TestDirectory}/FOO", $"{TestDirectory}/bar/foo");
Assert.True(Directory.Exists(Path.Combine(TestDirectory, "bar", "foo")));
}

[Fact]
[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)]
[PlatformSpecific(TestPlatforms.Windows)]
public void MoveDirectory_FailToMoveLowerCaseDirectoryWhenUpperCaseDirectoryExists()
{
Directory.CreateDirectory($"{TestDirectory}/bar/FOO");
Expand Down
61 changes: 36 additions & 25 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,40 +390,54 @@ private static void CreateParentsAndDirectory(string fullPath)
}
}

private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase)
private static void MoveDirectory(string sourceFullPath, string destFullPath, bool isCaseSensitiveRename)
{
// isCaseSensitiveRename is only set for case-insensitive systems (like macOS).
Debug.Assert(!isCaseSensitiveRename || !PathInternal.IsCaseSensitive);

ReadOnlySpan<char> srcNoDirectorySeparator = Path.TrimEndingDirectorySeparator(sourceFullPath.AsSpan());
ReadOnlySpan<char> destNoDirectorySeparator = Path.TrimEndingDirectorySeparator(destFullPath.AsSpan());

// When the path ends with a directory separator, it must not be a file.
// On Unix 'rename' fails with ENOTDIR, on wasm we need to manually check.
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
// The destination must not exist (unless it is a case-sensitive rename).
// On Unix 'rename' will overwrite the destination file if it already exists, we need to manually check.
if (!isCaseSensitiveRename && Interop.Sys.LStat(destNoDirectorySeparator, out Interop.Sys.FileStatus destFileStatus) >= 0)
{
if (Interop.Sys.Stat(destNoDirectorySeparator, out _) >= 0)
{
// destination exists, but before we throw we need to check whether source exists or not
// Maintain order of exceptions as on Windows.

// 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 if the source doesn't exist.
if (Interop.Sys.LStat(srcNoDirectorySeparator, out Interop.Sys.FileStatus sourceFileStatus) < 0)
{
throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath));
}
// Source and destination must not be the same file unless it is a case-sensitive rename.
else if (sourceFileStatus.Dev == destFileStatus.Dev &&
sourceFileStatus.Ino == destFileStatus.Ino)
{
Copy link
Member Author

@tmds tmds Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is where we now allow renaming from /a/foo to /a/Foo on case-sensitive systems like Linux and FreeBSD when we see both paths are the same file (dev and ino), and differ only in case.

// isCaseSensitiveRename is only true when the system is case-insensitive (like macOS).
// On a case-sensitive system (like Linux), there can stil be case-insensitive filesystems mounted.
// When both paths refer to the same file and they differ only in casing, we fall through to Rename.
if (!PathInternal.IsCaseSensitive && // handled by isCaseSensitiveRename.
!srcNoDirectorySeparator.Equals(destNoDirectorySeparator, StringComparison.OrdinalIgnoreCase) || // different paths.
Path.GetFileName(srcNoDirectorySeparator).SequenceEqual(Path.GetFileName(destNoDirectorySeparator))) // same names.
{
throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath));
throw new IOException(SR.IO_SourceDestMustBeDifferent);
}

// Some Unix distros will overwrite the destination file if it already exists.
// Throwing IOException to match Windows behavior.
}
// When the path ends with a directory separator, it must be a directory.
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));
}
else
{
throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, destFullPath));
}
}
Expand All @@ -436,10 +450,7 @@ private static void MoveDirectory(string sourceFullPath, string destFullPath, bo
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));
throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath));
case Interop.Error.ENOTDIR: // sourceFullPath exists and it's not a directory
throw new IOException(SR.Format(SR.IO_PathNotFound_Path, sourceFullPath));
default:
Expand Down
10 changes: 9 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,16 @@ public static DateTimeOffset GetLastWriteTime(string fullPath)
return data.ftLastWriteTime.ToDateTimeOffset();
}

private static void MoveDirectory(string sourceFullPath, string destFullPath, bool sameDirectoryDifferentCase)
private static void MoveDirectory(string sourceFullPath, string destFullPath, bool isCaseSensitiveRename)
{
// Source and destination must have the same root.
ReadOnlySpan<char> sourceRoot = Path.GetPathRoot(sourceFullPath);
ReadOnlySpan<char> destinationRoot = Path.GetPathRoot(destFullPath);
if (!sourceRoot.Equals(destinationRoot, StringComparison.OrdinalIgnoreCase))
{
throw new IOException(SR.IO_SourceDestMustHaveSameRoot);
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}

if (!Interop.Kernel32.MoveFile(sourceFullPath, destFullPath, overwrite: false))
{
int errorCode = Marshal.GetLastWin32Error();
Expand Down
34 changes: 12 additions & 22 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,19 @@ internal static void MoveDirectory(string sourceFullPath, string destFullPath)
ReadOnlySpan<char> srcNoDirectorySeparator = Path.TrimEndingDirectorySeparator(sourceFullPath.AsSpan());
ReadOnlySpan<char> destNoDirectorySeparator = Path.TrimEndingDirectorySeparator(destFullPath.AsSpan());

ReadOnlySpan<char> sourceDirNameFromFullPath = Path.GetFileName(sourceFullPath.AsSpan());
ReadOnlySpan<char> 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 && srcNoDirectorySeparator.Equals(destNoDirectorySeparator, fileSystemSensitivity))
throw new IOException(SR.IO_SourceDestMustBeDifferent);

ReadOnlySpan<char> sourceRoot = Path.GetPathRoot(srcNoDirectorySeparator);
ReadOnlySpan<char> 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);
// Don't allow the same path, except for changing the casing of the filename.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where we prevent moving from /a/foo to /A/foo on case-insensitive systems like Windows and macOS because /a and /A are assumed to be the same parent.
This kept the behavior as-is.

bool isCaseSensitiveRename = false;
if (srcNoDirectorySeparator.Equals(destNoDirectorySeparator, PathInternal.StringComparison))
{
if (PathInternal.IsCaseSensitive || // FileNames will be equal because paths are equal.
Path.GetFileName(srcNoDirectorySeparator).SequenceEqual(Path.GetFileName(destNoDirectorySeparator)))
{
throw new IOException(SR.IO_SourceDestMustBeDifferent);
}
isCaseSensitiveRename = true;
}

MoveDirectory(sourceFullPath, destFullPath, sameDirectoryDifferentCase);
MoveDirectory(sourceFullPath, destFullPath, isCaseSensitiveRename);
}
}
}