Skip to content

Address Set-Location/Get-Item Issue With ?/* Characters#12294

Open
NoMoreFood wants to merge 4 commits intoPowerShell:masterfrom
NoMoreFood:setlocation_wildcard
Open

Address Set-Location/Get-Item Issue With ?/* Characters#12294
NoMoreFood wants to merge 4 commits intoPowerShell:masterfrom
NoMoreFood:setlocation_wildcard

Conversation

@NoMoreFood
Copy link
Copy Markdown
Contributor

@NoMoreFood NoMoreFood commented Apr 10, 2020 •
edited
Loading

PR Summary

  • Adjusted behavior of GetCorrectCasedPath() which attempts to search for the file on the file system in order to normalize its case. Since some file systems support wildcard-type characters (e.g. MacOS) in path names so these need to be skipped to avoid erroneous resolution.
  • Added tests to detect this behavior.

PR Context

This corrects the behavior where Set-Location fails to change directory or changes to an entirely different child directory when the target directory contains characters such as '*'. Similarly, Get-Item also returns errant information.

Resolves: #12299

PR Checklist

@NoMoreFood NoMoreFood requested a review from anmenaga as a code owner April 10, 2020 14:42
@ghost ghost assigned TravisEz13 Apr 10, 2020
@NoMoreFood NoMoreFood changed the title WIP: Address Set-Location Issue With ?/* Characters Address Set-Location/Get-Item Issue With ?/* Characters Apr 10, 2020
@NoMoreFood NoMoreFood mentioned this pull request Apr 10, 2020
Merged
13 tasks
@NoMoreFood NoMoreFood changed the title Address Set-Location/Get-Item Issue With ?/* Characters WIP: Address Set-Location/Get-Item Issue With ?/* Characters Apr 11, 2020
@NoMoreFood NoMoreFood force-pushed the setlocation_wildcard branch 2 times, most recently from a1cb895 to 8b887f2 Compare April 11, 2020 08:57
@NoMoreFood
Address Set-Location & Get-Item Issue With ?/* Characters
02d5492 
- Adjusted behavior of GetCorrectCasedPath() which attempts to search for the file on the file system in order to normalize its case. Since some file systems support wildcard characters in path names; processing needs to be skipped for these.
- Added tests to detect this behavior.
@NoMoreFood NoMoreFood force-pushed the setlocation_wildcard branch from 8b887f2 to 02d5492 Compare April 11, 2020 11:08
@NoMoreFood NoMoreFood changed the title WIP: Address Set-Location/Get-Item Issue With ?/* Characters Address Set-Location/Get-Item Issue With ?/* Characters Apr 11, 2020
@TravisEz13 TravisEz13 added the Breaking-Change breaking change that may affect users label Apr 11, 2020
TravisEz13
TravisEz13 requested changes Apr 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

This look good. Thanks.

I've requested a coup of changes in the tests to make sure it doesn't regress things now or in the future.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 11, 2020
@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Apr 11, 2020
@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented Apr 11, 2020

@PowerShell/powershell-committee This is a breaking change. Please review. We made a similar change for matching commands. Where we prefer literal matches over wildcard matches.

@NoMoreFood
Copy link
Copy Markdown
Contributor Author

NoMoreFood commented Apr 11, 2020

@TravisEz13 So I can categorize these correctly in the future, can you educate me as to why this would be considered a breaking change?

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 11, 2020
iSazonov
iSazonov reviewed Apr 14, 2020
View reviewed changes
@SteveL-MSFT
Copy link
Copy Markdown
Member

SteveL-MSFT commented Apr 15, 2020

@PowerShell/powershell-committee agrees that the current behavior is broken where:

set-location -literalpath ?

must treat ? as a literal value and not a wildcard. We don't believe this is a breaking change that causes concern, however, we are concerned with the location of the code change in a helper that is used by many code paths which itself may introduce a regression somewhere else.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Apr 15, 2020
@NoMoreFood
Address Set-Location & Get-Item Issue With ?/* Characters - Revisions
7230f08 
- Mark backslash test on MacOS / Linux as 'Pending' vice 'Skipped'.
- Use Utils.Separators.StarOrQuestion instead of hard-coded constants.
@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented May 20, 2020

I'm not sure your change addressed the committee's concern about the location of the change.

@TravisEz13 TravisEz13 removed the Breaking-Change breaking change that may affect users label May 20, 2020
@NoMoreFood
Copy link
Copy Markdown
Contributor Author

NoMoreFood commented May 21, 2020 •
edited
Loading

@TravisEz13 Sorry, the day job has distracted me from following through on this and one other PR. I honestly thought the "location of the code change" comment was sort of meant to be rhetorical. The bug itself can be scoped to that helper function: In what scenario would I possibly pass a literal, existing directory path to GetCorrectCasedPath() and it would be acceptable to get back a reference to an existing different directory? The function exists solely to normalize case within a path --- it should NEVER be returning a path to a different file.

@ghost ghost added the Review - Needed The PR is being reviewed label May 28, 2020
@ghost
Copy link
Copy Markdown

ghost commented May 28, 2020

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

@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels May 28, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 9, 2020
@NoMoreFood NoMoreFood force-pushed the setlocation_wildcard branch 2 times, most recently from 65bee7b to 7c4a4f9 Compare June 9, 2020 02:51
@NoMoreFood
Address Set-Location & Get-Item Issue With ?/* Characters - Revisions
fed014a 
- Added failure test for a directory that contains wildcard characters.
@NoMoreFood NoMoreFood force-pushed the setlocation_wildcard branch from 7c4a4f9 to fed014a Compare June 9, 2020 03:18
@NoMoreFood NoMoreFood requested a review from TravisEz13 June 9, 2020 03:43
iSazonov
iSazonov reviewed Jun 9, 2020
View reviewed changes
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jun 10, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Jun 17, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jun 17, 2020

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

@NoMoreFood
Copy link
Copy Markdown
Contributor Author

NoMoreFood commented Jun 25, 2020 •
edited
Loading

Is this ready for merge or are we still waiting for reviews? @TravisEz13

NoMoreFood
NoMoreFood commented Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@NoMoreFood NoMoreFood left a comment

Choose a reason for hiding this comment

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

I believe I have resolved these.

@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented Oct 27, 2020

@TravisEz13 Sorry, the day job has distracted me from following through on this and one other PR. I honestly thought the "location of the code change" comment was sort of meant to be rhetorical. The bug itself can be scoped to that helper function: In what scenario would I possibly pass a literal, existing directory path to GetCorrectCasedPath() and it would be acceptable to get back a reference to an existing different directory? The function exists solely to normalize case within a path --- it should NEVER be returning a path to a different file.

Here is the direct call:
https://sourcegraph.com/search?q=repo:powershell/powershell+GetCorrectCasedPath&patternType=literal

Then, NormalizePath is used in these places:
https://sourcegraph.com/search?q=repo:powershell/powershell+content:%22Microsoft.PowerShell.Commands%22+content:%22NormalizePath%28%22+lang:c%23++-file:%22src/System.Management.Automation/namespaces/RegistryProvider.cs%22&patternType=literal#1

We need to make sure we didn't break any of these calls.

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 27, 2020
@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc. labels Oct 27, 2020
@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented Oct 27, 2020

I'll let the Provider working group review from here.

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Oct 28, 2020

The GetCorrectCasedPath() is not in right place. This would only have to be used in Formatting System and completors.

@NoMoreFood
Copy link
Copy Markdown
Contributor Author

NoMoreFood commented Nov 4, 2020

@TravisEz13 All of the non-file system paths you reference end up calling a base class version or provider-specific version of NormalizePath() so this change luckily does not affect them.

@iSazonov I think that might be splitting hairs to some degree. When somebody does a Set-Location to a specific path, do you want the resultant path at the prompt to be case-accurate or not? I think it might be hard to address all the "what if" sort of cases. Regardless, this PR was authored to address the "bug" within GetCorrectCasedPath() itself so I was hoping not to boil the ocean to get this committed. :-)

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 4, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Nov 11, 2020
@ghost
Copy link
Copy Markdown

ghost commented Nov 11, 2020

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

@TravisEz13 TravisEz13 added Needs-Triage The issue is new and needs to be triaged by a work group. and removed Review - Needed The PR is being reviewed labels May 8, 2023
@ghost ghost added the Review - Needed The PR is being reviewed label May 16, 2023
@ghost
Copy link
Copy Markdown

ghost commented May 16, 2023

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

@iSazonov iSazonov mentioned this pull request May 13, 2025
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

Requested changes must be addressed to merge this pull request.

Assignees

@TravisEz13 TravisEz13

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Needs-Triage The issue is new and needs to be triaged by a work group. Review - Needed The PR is being reviewed WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Set-Location & Get-Item -LiteralPath Inconsistent On Files/Dirs With ?/* Characters

4 participants

@NoMoreFood @TravisEz13 @SteveL-MSFT @iSazonov