Reduce aggressive correcting casing of file system paths by iSazonov · Pull Request #14469 · PowerShell/PowerShell

Ilya (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

Xavier Deguillard (xavierd) · 2020-12-21T02:10:16Z

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

Ilya (iSazonov) · 2020-12-21T05:30:19Z

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

Xavier Deguillard (@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.

Deleted user (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

Rain Sallow (/u/ta11ow) (vexx32) · 2021-05-20T13:21:25Z

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

Rain Sallow (/u/ta11ow) (vexx32) · 2021-05-20T13:25:25Z

        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.

Deleted user (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

James Truher (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

Ilya (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?

Deleted user (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

Dongbo Wang (daxian-dbw) · 2021-11-02T05:41:39Z

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

Ilya (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

Steve Lee (@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

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

Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Dongbo Wang (daxian-dbw) December 20, 2020 18:29

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

Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Dec 20, 2020

Fix test

a9719b0

Fix test 2

025765f

Ilya (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

Ilya (iSazonov) closed this May 20, 2021

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

Ilya (iSazonov) reopened this May 20, 2021

Rain Sallow (/u/ta11ow) (vexx32) reviewed May 20, 2021

View reviewed changes

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

Ilya (iSazonov) mentioned this pull request Aug 5, 2021

Fix FileSystemProvider to work with volume and pipe paths #15873

Merged

8 of 22 tasks

Steve Lee (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

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

Dongbo Wang (daxian-dbw) closed this Nov 2, 2021

Ilya (iSazonov) mentioned this pull request Nov 5, 2021

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

Open

Conversation

Ilya (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!

Xavier Deguillard (xavierd) commented Dec 21, 2020

Uh oh!

Ilya (iSazonov) commented Dec 21, 2020

Uh oh!

Deleted user (ghost) commented Dec 28, 2020

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) May 20, 2021

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) May 20, 2021

Choose a reason for hiding this comment

Uh oh!

Ilya (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!

Deleted user (ghost) commented May 28, 2021

Uh oh!

James Truher (JamesWTruher) commented Aug 23, 2021

Uh oh!

Ilya (iSazonov) commented Aug 24, 2021

Uh oh!

Deleted user (ghost) commented Aug 31, 2021

Uh oh!

Dongbo Wang (daxian-dbw) commented Nov 2, 2021

Uh oh!

Ilya (iSazonov) commented Nov 2, 2021

Uh oh!

mi-hol commented Dec 1, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Ilya (iSazonov) commented Dec 20, 2020 •

edited

Loading

Ilya (iSazonov) May 21, 2021 •

edited

Loading