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

Return correct casing of filesystem path during normalization #9250

Merged
merged 8 commits into from Apr 23, 2019

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Mar 29, 2019

PR Summary

On a case-insensitive filesystem, if the path exists, we propagate the user typed casing of the path and reuse it. This causes problems with some native tools like git where it expects correct casing of filesystem paths. The change is to step through the path and retrieve the actual casing of the items in the path and return that during the path normalization method.

PR Context

Fix #5678

PR Checklist

@SteveL-MSFT
Copy link
Member Author

@PoshChan Please get failures

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@PoshChan
Copy link
Collaborator

@SteveL-MSFT, test results were not published for PowerShell-CI-static-analysis at vstfs:///Build/Build/18523

@PoshChan

This comment has been minimized.

@SteveL-MSFT
Copy link
Member Author

@PoshChan Please get failures

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@PoshChan
Copy link
Collaborator

@SteveL-MSFT, test results were not published for PowerShell-CI-static-analysis at vstfs:///Build/Build/18594

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@PoshChan

This comment has been minimized.

@SteveL-MSFT
Copy link
Member Author

@PoshChan Please retry windows

@PoshChan
Copy link
Collaborator

PoshChan commented Apr 1, 2019

@SteveL-MSFT, successfully started retry of PowerShell-CI-Windows

@PoshChan

This comment has been minimized.

@SteveL-MSFT SteveL-MSFT changed the title Return correct casing of filesystem path during normalization WIP: Return correct casing of filesystem path during normalization Apr 1, 2019
@PoshChan

This comment has been minimized.

@SteveL-MSFT
Copy link
Member Author

Hmmm, looks like the failures are due to handling of long path names which explains why it didn't fail on my machine. Might have to add some long path tests first.

@PoshChan

This comment has been minimized.

@SteveL-MSFT SteveL-MSFT changed the title WIP: Return correct casing of filesystem path during normalization Return correct casing of filesystem path during normalization Apr 3, 2019
@anmenaga anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 11, 2019
@anmenaga anmenaga merged commit 095be46 into PowerShell:master Apr 23, 2019
@TravisEz13 TravisEz13 added this to the 7.0.0-preview.1 milestone Apr 23, 2019
@daxian-dbw
Copy link
Member

@anmenaga Please merge by "Squash and Merge" in future unless we really want to preserve the PR history (should be rare).

@iSazonov
Copy link
Collaborator

@SteveL-MSFT @daxian-dbw
The change is performance expensive.
Version 6.2.3 vs latest local build:
image
image

PerfView:
image

We see a performance decreasing although overall performance of 7.0 builds is 2x better for file operations (like dir -Recurse)!

Also the trace shows that we call the method from some places several times. So the change slow down all file system operations without the need.

From original issue:

If I use cmd.exe instead, it gives the expected/desired results.

So I hope there is another way to fix the issue without perf degradation. I look cmd.exe and don't see that it does a path case normalization but works well. It seems we did not find a root of the issue and the fix is not in right place.

@daxian-dbw
Copy link
Member

@iSazonov I see different results -- it's even slightly faster in 7.0.0-preview.5 comparing with 6.2.3.

image

@iSazonov
Copy link
Collaborator

@daxian-dbw I guess we use different cwd - my one is very long.
And obviously many folder enumeration (current fix) is slower then replacing slashes (before the fix).

@daxian-dbw
Copy link
Member

@iSazonov I still cannot reproduce it using a long current working directory
Can you please provide a consistent repro using preview.5?

image

@iSazonov
Copy link
Collaborator

@daxian-dbw I tested on another system and I guess it is Kaspersky AV that slow down the CreateFileW P/Invoke. I still don't like these extra P/Invokes taking in account how cmd.exe and Windows PowerShell works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set-Location preserves case instead of matching filesystem on case-insensitive/preserving system
6 participants