Skip to content

Reduce aggressive correcting casing of file system paths #14469

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

Closed
iSazonov wants to merge 3 commits into from
Closed

Reduce aggressive correcting casing of file system paths #14469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
715a95f
Reduce aggressive correcting casing of file system paths
iSazonov Dec 20, 2020
a9719b0
Fix test
iSazonov Dec 21, 2020
025765f
Fix test 2
iSazonov Dec 21, 2020
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
46 changes: 26 additions & 20 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,11 @@ public FileSystemProvider()
/// </returns>
private static string NormalizePath(string path)
{
return GetCorrectCasedPath(path.Replace(StringLiterals.AlternatePathSeparator, StringLiterals.DefaultPathSeparator));
return path.Replace(StringLiterals.AlternatePathSeparator, StringLiterals.DefaultPathSeparator);
}

/// <summary>
/// Get the correct casing for a path. This method assumes it's being called by NormalizePath()
/// so that the path is already normalized.
/// Normalize and get the correct casing for a path.
/// </summary>
/// <param name="path">
/// The path to retrieve.
Expand All @@ -123,6 +122,13 @@ private static string NormalizePath(string path)
/// </returns>
private static string GetCorrectCasedPath(string path)
{
path = NormalizePath(path);

if (Platform.IsLinux)
{
return path;
}
Comment on lines +127 to +130
Copy link
Collaborator

@vexx32 vexx32 May 20, 2021

Choose a reason for hiding this comment

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

Aren't MacOS paths also case sensitive? Is there another reason we'd have this behaviour for Linux but not MacOS?

Copy link
Collaborator Author

@iSazonov iSazonov May 21, 2021

Choose a reason for hiding this comment

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

By default MacOS uses a case-insensitive file system.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Aug 5, 2021

Choose a reason for hiding this comment

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

The primary scenario where we hit a problem is with git where the remote repo is on Linux and case-sensitive and someone adds a file with a directory that they manually typed and then you have two directories in git that only differ by case. I think that affects Windows and macOS clients even if their local filesystems are case insensitive.

Copy link
Collaborator Author

@iSazonov iSazonov Aug 5, 2021

Choose a reason for hiding this comment

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

If git is case-sensetive locally on all platforms then users should follow the application design why should PowerShell do expensive work for all paths and for other scenarios?

Copy link
Collaborator Author

@iSazonov iSazonov Aug 6, 2021

Choose a reason for hiding this comment

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

they manually typed

the remote repo is on Linux and case-sensitive

Wow! If repo/application is case-sensitive and user obviously know this shouldn't the user take care of compliance himself?

If different platforms have different behavior, this is a fundamental and unsolvable problem. We constantly get requests about slashes because they are different in local (Windows) and remote session (Linux). I haven't seen as many requests about case, but the root of the problem is the same.
It should be clearly written in the documentation that this is the user's concern: if a scenario is - scripts works on Windows client and remote sessions are on Linux-s (and vise-versa) a recommendation for users should be to prefer forward slashes (backslashs) and be careful about cases.


// Only apply to directories where there are issues with some tools if the casing
// doesn't match the source like git
if (Directory.Exists(path))
Expand Down Expand Up @@ -167,13 +173,13 @@ private static string GetCorrectCasedPath(string path)
}
else
{
// Use GetFileSystemEntries to get the correct casing of this element
// Use enumeration to get the correct casing of this element
try
{
var entries = Directory.GetFileSystemEntries(exactPath, item);
if (entries.Length > 0)
var entry = Directory.EnumerateFileSystemEntries(exactPath, item).FirstOrDefault();
if (entry is not null)
{
exactPath = entries[0];
exactPath = entry;
}
else
{
Expand Down Expand Up @@ -1376,7 +1382,7 @@ protected override void GetItem(string path)

private FileSystemInfo GetFileSystemItem(string path, ref bool isContainer, bool showHidden)
{
path = NormalizePath(path);
path = GetCorrectCasedPath(path);
Copy link
Collaborator

@vexx32 vexx32 May 20, 2021

Choose a reason for hiding this comment

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

I see we've swapped a bunch of calls to use GetCorrectCasedPath -- are there cases where NormalizePath is still called on its own? Or is it only used in the GetCorrectCasedPath method now?

Copy link
Collaborator Author

@iSazonov iSazonov May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After the PR:

  • GetCorrectCasedPath - 16 uses.
  • NormalizePath - 8 uses.

The PR does mostly obvious changes. Later we could remove more GetCorrectCasedPath uses.

FileInfo result = new FileInfo(path);

// FileInfo.Exists is always false for a directory path, so we check the attribute for existence.
Expand Down Expand Up @@ -1639,7 +1645,7 @@ private void GetPathItems(
throw PSTraceSource.NewArgumentException(nameof(path));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

var fsinfo = GetFileSystemInfo(path, out bool isDirectory);

Expand Down Expand Up @@ -2125,7 +2131,7 @@ protected override void RenameItem(
throw PSTraceSource.NewArgumentException(nameof(path));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

if (string.IsNullOrEmpty(newName))
{
Expand Down Expand Up @@ -2271,7 +2277,7 @@ protected override void NewItem(
type = "file";
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

if (Force)
{
Expand Down Expand Up @@ -3616,8 +3622,8 @@ protected override void CopyItem(
throw PSTraceSource.NewArgumentException(nameof(destinationPath));
}

path = NormalizePath(path);
destinationPath = NormalizePath(destinationPath);
path = GetCorrectCasedPath(path);
destinationPath = GetCorrectCasedPath(destinationPath);

PSSession fromSession = null;
PSSession toSession = null;
Expand Down Expand Up @@ -5083,13 +5089,13 @@ protected override string NormalizeRelativePath(

do // false loop
{
path = NormalizePath(path);
path = GetCorrectCasedPath(path);
path = EnsureDriveIsRooted(path);

// If it's not fully normalized, normalize it.
path = NormalizeRelativePathHelper(path, basePath);

basePath = NormalizePath(basePath);
basePath = GetCorrectCasedPath(basePath);
basePath = EnsureDriveIsRooted(basePath);

result = path;
Expand Down Expand Up @@ -5785,8 +5791,8 @@ protected override void MoveItem(
throw PSTraceSource.NewArgumentException(nameof(destination));
}

path = NormalizePath(path);
destination = NormalizePath(destination);
path = GetCorrectCasedPath(path);
destination = GetCorrectCasedPath(destination);

// Verify that the target doesn't represent a device name
if (PathIsReservedDeviceName(destination, "MoveError"))
Expand Down Expand Up @@ -6170,7 +6176,7 @@ public void GetProperty(string path, Collection<string> providerSpecificPickList
throw PSTraceSource.NewArgumentException(nameof(path));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

PSObject result = null;

Expand Down Expand Up @@ -6308,7 +6314,7 @@ public void SetProperty(string path, PSObject propertyToSet)
throw PSTraceSource.NewArgumentNullException(nameof(propertyToSet));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

PSObject results = new PSObject();

Expand Down Expand Up @@ -6474,7 +6480,7 @@ public void ClearProperty(
throw PSTraceSource.NewArgumentException(nameof(path));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

if (propertiesToClear == null ||
propertiesToClear.Count == 0)
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/namespaces/FileSystemSecurity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void GetSecurityDescriptor(string path,
AccessControlSections sections)
{
ObjectSecurity sd = null;
path = NormalizePath(path);
path = GetCorrectCasedPath(path);

if (string.IsNullOrEmpty(path))
{
Expand Down Expand Up @@ -106,7 +106,7 @@ public void SetSecurityDescriptor(
throw PSTraceSource.NewArgumentException(nameof(path));
}

path = NormalizePath(path);
path = GetCorrectCasedPath(path);

if (securityDescriptor == null)
{
Expand Down