DirectoryInfo.MoveTo and Directory.Move mistakenly prevent renaming directories to case variations of themselves on Windows, macOS

[mklement0]

The default filesystems on macOS and Windows are case-insensitive, yet case-preserving.

Therefore, renaming a filesystem item to a case variation of its current name - e.g., foo to FOO - should be supported.

• This is properly supported for files.

• It is not supported for directories, due to an explicit, conceptually flawed check that tests the new path for being the same case-insensitively, which throws a spurious exception stating that Source and destination path must be different.

You can paste following snippet directly into a dotnet script REPL and press Enter or paste it into a *.csx file and execute it with dotnet script /path/to/*.csx:

using System.IO;

Environment.CurrentDirectory = Path.GetTempPath();

Directory.CreateDirectory("foo");

// This should succeed even on macOS and Windows, but throws an exception.
// System.DirectoryInfo.MoveTo() exhibits the same symptom.
Directory.Move("foo", "FOO");

Additionally: Even if the source and destination are truly identical, it is arguably more sensible to apply desired-state logic and perform a quiet no-op rather than throwing an exception.

[danmoseley]

Thanks for the report. The code does check case insensitively if a file written to temp can be retrieved case insensitively:
https://github.com/dotnet/corefx/blob/6d86975666ecd2e2b4f5ab808dcfeff94fd18789/src/System.IO.FileSystem/src/System/IO/Directory.cs#L277-L278
https://github.com/dotnet/corefx/blob/6d86975666ecd2e2b4f5ab808dcfeff94fd18789/src/Common/src/System/IO/PathInternal.CaseSensitivity.cs#L16-L52

Is there a bug here? Of course, it will go wrong if the file system you are using has a different case sensitivity to the filesystem containing the temp folder.

Possibly this is another case where we should let the OS do the checks, similar to how we removed checks for invalid characters in the path.

[mklement0]

@danmosemsft: Yes, there is a bug:

On a case-preserving filesystem - even if access to files is case-insensitive - I should still be able to rename my foo directory to FOO, for instance.

Deferring to the OS seems like a good idea (cmd.exe's ren command works as intended, for instance, as does macOS' /bin/mv).

(Whether a static, process-global check for case-sensitivity based on a single location is sufficient, is a separate matter - I haven't looked into this, and I don't know if a given OS can access a mix of case-sensitive and case-insensitive volumes.)

As stated, If the paths are truly identical, to me the most sensible behavior is a quiet no-op - and if cmd.exe's ren and macOS' /bin/mv are any indication, deferring to the OS does just that.

In short: simply deferring to the OS may solve the problem in more than one way:

• It'll allow renaming to a case variation of the original name.

• It'll make an attempt to truly rename to the very same path a quiet no-op - though, I suppose, no longer complaining about a no-op renaming operation is technically a breaking change.

[danmoseley]

Oh - yes of course, even when working correctly, it prevents making a casing change on insensitive volume.

@JeremyKuhne perhaps we should remove this?

[pinkfloydx33]

I don't know if a given OS can access a mix of case-sensitive and case-insensitive volumes.

In Windows10 it is possible to toggle the case sensitivity of a directory. So it seems the static check isn't really sufficient

[JeremyKuhne]

I'm fine with allowing this. I think there may already be an issue on this actually- I'll try and see if I can find it. I'd rather not remove the failure case if the strings are ordinally equal.

It is a breaking change, so we'll have to document it as such.

Marking as up-for-grabs if anyone wants to take it on. We would want tests to validate that this works correctly with just a casing change. There should be tests with directory content as well.

[mklement0]

Interesting, @pinkfloydx33; could you please create a separate issue for that, perhaps with more background information?

[mklement0]

Glad to hear it, @JeremyKuhne.

Re:

I'd rather not remove the failure case if the strings are ordinally equal.

Understood, but note that this means preserving the existing asymmetry between files and directories, given that ordinally equal file names result in a quiet no op, on all platforms.

It is a breaking change, so we'll have to document it as such.

If you retain the behavior of failing with ordinally equal strings with directories, I would consider this a mere bug fix, but it is certainly worth documenting either way.

[JeremyKuhne]

note that this means preserving the existing asymmetry

Yeah, and I don't like that. If there was any extra value beyond that and it wasn't breaking I'd be more inclined to do it. If someone wants to investigate how this change impacts PowerShell that data might help make this part of the decision clearer (i.e. do they do any validation before calling this method, particularly from their "move" commands)?

[mklement0]

The filesystem-item move functionality is in FileystemProvider.cs.

The short of it is: Aside from preparatory path normalization and translating PS-drive-based paths into "native ones":

• moving files delegates to FileInfo.MoveTo()

  • Custom code, activated via the -Force switch, allows replacement of the target file if it exists (which could now be replaced with the new-in-3.0 FileInfo.MoveTo() overload that offers the same functionality via the overwrite parameter).

• moving directories delegates to DirectoryInfo.MoveTo(), except if the source and destinations are on different volumes, in which case a custom copy-and-delete implementation is used (which recurses on the directory content and in the process calls

That is, changing the CoreFx behavior to making ordinally equal directory paths result in a quiet no-op would surface in PowerShell's Rename-Item cmdlet too.

The Move-Item cmdlet, by contrast, expects the parent path of the directory's new location as the -Destination argument, so specifying the same paths has different semantics: It effectively requests moving a directory into a subdirectory of itself, which fails at the level of the system calls on all platforms, from what I can tell.

[Jlalond]

So I started looking into this, and I'm at somewhat of an impass.

To fix our fix exception of directory case-sensitivity we can go from:

            StringComparison pathComparison = PathInternal.StringComparison;

            if (string.Equals(sourcePath, destPath, pathComparison))
                throw new IOException(SR.IO_SourceDestMustBeDifferent);

To

            // As we're moving a directory, case sensitivty should be ordinal
            // even on a case-sensitive/case-preserving file system moving foo to FOO should still work.
            StringComparison pathComparison = StringComparison.Ordinal;

            if (string.Equals(sourcePath, destPath, pathComparison))
                throw new IOException(SR.IO_SourceDestMustBeDifferent);

However, the call will still fail and I think our issue runs a bit deeper.

Beneath the above mentioned path comparison we have our call to DirectoryExists.

            if (FileSystem.DirectoryExists(fulldestDirName))
                throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, fulldestDirName));

This calls our Kernel32 interop

            path = Path.TrimEndingDirectorySeparator(path);

            using (DisableMediaInsertionPrompt.Create())
            {
                if (!Interop.Kernel32.GetFileAttributesEx(path, Interop.Kernel32.GET_FILEEX_INFO_LEVELS.GetFileExInfoStandard, ref data))

Now, the interop API's are not my forte, and I looked around corert and checked out the used types, but as far as I see there is no way to interop a specifically case-insensitive directory exists.

So I suggest we omit this call, and do an error check in FileSystem.MoveDirectory for Interop.Errors.ERROR_ALREADY_EXISTS.

As I have limited insight into the viability of my solution, I wanted to get some feedback, (@danmosemsft @JeremyKuhne)

[Jlalond]

@mklement0 Question:

Do we want to support something like

I have a directory foo,
and another directory bar, with a directory named /fOO which is also empty.

do we want to support moving it there, or just moving ./foo to ./FOO?

[carlossanlop]

I have a directory foo,
and another directory bar, with a directory named /fOO which is also empty.
do we want to support moving it there, or just moving ./foo to ./FOO?

@mklement0 @Jlalond if you are still interested in supporting this scenario, feel free to open a new issue. This issue has already been fixed with dotnet/corefx#40570, so let's close it.

[mklement0]

@Jlalond Sorry I forgot to respond to your original question.

I think that on case-insensitive filesystems attempting to move ./foo to a preexisting ./bar/fOO should simply fail, because, from the perspective of whether the target already exists, foo and fOO should be considered identical.

Therefore, I don't think that further changes needed.