Reduce aggressive correcting casing of file system paths #14469

iSazonov · 2020-12-20T18:29:27Z

PR Summary

While modern (SSD) disks are fast, this does not mean that disk operations have become so insignificant that they can be neglected. For case correction we use additional disk operations (enumerations).

It is very weird that we do case corrections for file paths which are excluded in the PR:

on Linux
for operations without output like IsItemContainer()

PR Context

#9250 Return correct casing of filesystem path during normalization

PR Checklist

xavierd · 2020-12-21T02:10:16Z

I'm wondering if this would also help to fix #13432.

iSazonov · 2020-12-21T05:30:19Z

I'm wondering if this would also help to fix #13432.

@xavierd I hope. I replaced full enumeration with getting first element from IEnumerable. Could you please download compiled artifact from the PR and analyze PerfView traces to confirm.

ghost · 2020-12-28T14:00:07Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

vexx32 · 2021-05-20T13:21:25Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+            if (Platform.IsLinux)
+            {
+                return path;
+            }


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

By default MacOS uses a case-insensitive file system.

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.

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?

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.

vexx32 · 2021-05-20T13:25:25Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

        private FileSystemInfo GetFileSystemItem(string path, ref bool isContainer, bool showHidden)
        {
-            path = NormalizePath(path);
+            path = GetCorrectCasedPath(path);


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?

After the PR:

GetCorrectCasedPath - 16 uses.

NormalizePath - 8 uses.

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

ghost · 2021-05-28T14:00:09Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

JamesWTruher · 2021-08-23T21:02:45Z

While the WG agrees that something might be done, we have an overriding concern for compatibility and being comprehensive in the light that some unix file systems may be case insensitive. We have decided to reject this PR for the following reasons:

high level of risk due to complexity in current code base
we need to have expected behavior well documented
we need to have proper, comprehensive validation for new behaviors

iSazonov · 2021-08-24T09:19:30Z

While the WG agrees that something might be done, we have an overriding concern for compatibility and being comprehensive in the light that some unix file systems may be case insensitive. We have decided to reject this PR for the following reasons:

high level of risk due to complexity in current code base

we need to have expected behavior well documented

we need to have proper, comprehensive validation for new behaviors

Interesting! :-) Could please the WG point me RFC, public discussion and important business scenarios for adding the feature? Did the WG explicitly measure and approve 5x, 10x and more extra P/Invokes for every path in processing? Is the opinion of the WG unapologetically strict that the main thing is that I should see in the console once a day "Abc" instead of "abc", although I do not care about it and I need my scripts run every minute to work as fast as possible?

ghost · 2021-08-31T14:00:39Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

daxian-dbw · 2021-11-02T05:41:39Z

Given that the engine WG has rejected this PR, I will close it for housekeeping purpose.

iSazonov · 2021-11-02T11:58:20Z

I can't agree with that. It makes absolutely no sense to do this in scripts. For cosmetic effect we make file operations slow. PowerShell is already bad with files and this makes it unusable at all. Of course this is not a problem if you just type cd /var/log, but I have to say goodbye to PowerShell every time for large disks.
It's a shame that PowerShell remains a toy not suitable for serious work in a corporate environment.

mi-hol · 2021-12-01T11:14:34Z

@SteveL-MSFT As a consequence of this WG decision the slogan "PowerShell for every system!" should be removed from github´s About page!

Reduce aggressive correcting casing of file system paths

715a95f

iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Dec 20, 2020

iSazonov requested review from SteveL-MSFT and daxian-dbw December 20, 2020 18:29

iSazonov requested a review from anmenaga as a code owner December 20, 2020 18:29

iSazonov assigned daxian-dbw Dec 20, 2020

Fix test

a9719b0

Fix test 2

025765f

iSazonov mentioned this pull request Dec 25, 2020

Tracking FileSystem provider performance issues #14497

Closed

ghost added the Review - Needed The PR is being reviewed label Dec 28, 2020

iSazonov closed this May 20, 2021

ghost removed the Review - Needed The PR is being reviewed label May 20, 2021

iSazonov reopened this May 20, 2021

vexx32 reviewed May 20, 2021

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label May 28, 2021

iSazonov mentioned this pull request Aug 5, 2021

Fix FileSystemProvider to work with volume and pipe paths #15873

Merged

22 tasks

SteveL-MSFT added the WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc. label Aug 6, 2021

ghost removed the Review - Needed The PR is being reviewed label Aug 24, 2021

ghost added the Review - Needed The PR is being reviewed label Aug 31, 2021

daxian-dbw removed the Review - Needed The PR is being reviewed label Nov 2, 2021

daxian-dbw closed this Nov 2, 2021

iSazonov mentioned this pull request Nov 5, 2021

Windows (NTFS): Inconsistent support for the long-path prefix (\\?\) in filesystem paths #10805

Open

Reduce aggressive correcting casing of file system paths #14469

Reduce aggressive correcting casing of file system paths #14469

Uh oh!

Conversation

iSazonov commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

xavierd commented Dec 21, 2020

Uh oh!

iSazonov commented Dec 21, 2020

Uh oh!

ghost commented Dec 28, 2020

Uh oh!

vexx32 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

iSazonov May 21, 2021

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

vexx32 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

iSazonov May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 28, 2021

Uh oh!

JamesWTruher commented Aug 23, 2021

Uh oh!

iSazonov commented Aug 24, 2021

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

daxian-dbw commented Nov 2, 2021

Uh oh!

iSazonov commented Nov 2, 2021

Uh oh!

mi-hol commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

iSazonov commented Dec 20, 2020 •

edited

Loading

iSazonov May 21, 2021 •

edited

Loading