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.

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

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

Open
mklement0 opened this issue Aug 4, 2019 · 9 comments

Comments

@mklement0
Copy link
Collaborator

commented Aug 4, 2019

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.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Thanks for the report. The code does check case insensitively if a file written to temp can be retrieved case insensitively:

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

private static readonly bool s_isCaseSensitive = GetIsCaseSensitive();
/// <summary>Returns a comparison that can be used to compare file and directory names for equality.</summary>
internal static StringComparison StringComparison
{
get
{
return s_isCaseSensitive ?
StringComparison.Ordinal :
StringComparison.OrdinalIgnoreCase;
}
}
/// <summary>Gets whether the system is case-sensitive.</summary>
internal static bool IsCaseSensitive { get { return s_isCaseSensitive; } }
/// <summary>
/// Determines whether the file system is case sensitive.
/// </summary>
/// <remarks>
/// Ideally we'd use something like pathconf with _PC_CASE_SENSITIVE, but that is non-portable,
/// not supported on Windows or Linux, etc. For now, this function creates a tmp file with capital letters
/// and then tests for its existence with lower-case letters. This could return invalid results in corner
/// cases where, for example, different file systems are mounted with differing sensitivities.
/// </remarks>
private static bool GetIsCaseSensitive()
{
try
{
string pathWithUpperCase = Path.Combine(Path.GetTempPath(), "CASESENSITIVETEST" + Guid.NewGuid().ToString("N"));
using (new FileStream(pathWithUpperCase, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, 0x1000, FileOptions.DeleteOnClose))
{
string lowerCased = pathWithUpperCase.ToLowerInvariant();
return !File.Exists(lowerCased);
}
}
catch (Exception exc)

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

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2019

@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.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

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

@JeremyKuhne perhaps we should remove this?

@pinkfloydx33

This comment has been minimized.

Copy link

commented Aug 5, 2019

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 JeremyKuhne added this to the Future milestone Aug 5, 2019

@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2019

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

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.