Fix handling of -Path when it literally contains wildcard char #5896

kwkam · 2018-01-13T08:20:53Z

Fix handling of -Path argument that contains wildcard char (eg. ./`[file`].txt) on some Cmdlet.
Fix tab completion when path contains wildcard char (eg. ./`[fi[TAB] did not get completed).
Make Get-Command try to resolve -Path as literal when it contains invalid wildcard pattern.

I believe this also solve/work around the following issues:
#3724 #3725 #4726 #6232

PR Summary

*Microsoft.PowerShell.Commands.Management/commands/management/Navigation:
Move-Item: Set the context.SuppressWildcardExpansion in MoveItem instead of escaping the path every time. Also solve the issue where Move-Item complains -Path wildcard pattern is not valid when -Path contains special chars that forms an invalid pattern.
Rename-Item: Unescape non-literal, non-glob path in ProccessRecord and set the context.SuppressWildcardExpansion in RenameItem. This solve the issue where Rename-Item complains -Path does not exist when both -Path and CWD contains special char.

*System.Management.Automation/engine/CommandCompletion/CompletionCompleters:
Use WildcardPattern::Escape to escape completion text of filename.

*System.Management.Automation/engine/CommandSearcher:
Do not resolve path as glob when it contains invalid wildcard pattern (eg. ./[.ps1).
Merge the path resolving code from GetNextFromPath into ResolvePSPath and use that function.

*System.Management.Automation/engine/GetCommandCommand:
Do not treat path as pattern when it contains invalid wildcard pattern (eg. ./[.ps1).

*System.Management.Automation/engine/SessionStateContainer:
Get-ChildItem: Unescape non-literal, non-glob path before existence checking. This solve the issue where Get-GhildItem does not behave correctly when -Path contains special char. For example, Get-ChildItem -Path './`[dir`]' will complain Cannot find path ... while Get-ChildItem -Path './`[dir`]' -Depth 0 will work fine.
New-Item: Unescape non-literal path when -Name is not set. This works around for New-Item treating -Path as literal path while it can also be globbable. For example, assuming there is [file]1 in current directory, and tab completion suggests ./`[file`]1 for the -Path argument, but New-Item -Path './`[file`]2' will create a file named `[file`]2 instead of [file]2.

*System.Management.Automation/engine/regex:
WildcardPattern::Escape should also escape `, since WildcardPattern::Unescape would unescape it, and the matcher parse it as escape char.

*System.Management.Automation/namespaces/LocationGlobber:
Do not escape CWD when LiteralPath is used. This causes issues when both -LiteralPath and CWD contains special char.
Do not pass regex escaped string to WildcardPattern::Get. This fails tab completion when doing ./path/to/`[f[TAB].

*System.Management.Automation/utils/PathUtils:
Unescape non-literal, non-glob path before calling GetUnresolvedProviderPathFromPSPath since it only accepts literal path. This solve the issue where some commands depending on that method will treat -Path as literal unintentionally. For example, Out-File -Path './`[out`].txt' -Append will create a new file named `[out`].txt instead of writing to [out].txt.

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing Documentation needed
- Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

*engine/CommandCompletion/CompletionCompleters: Use WildcardPattern::Escape to escape completion text *engine/SessionStateContainer: GetChildItem: Unescape non-literal, non-glob path before existence checking NewItem: Unescape non-literal path when -Name is not set *engine/regex: WildcardPattern::Escape should also escape '`', since WildcardPattern::Unescape would unescape it, and the matcher parse it as escape char *namespaces/LocationGlobber: Do not pass regex escaped string to WildcardPattern::Get *utils/PathUtils: Unescape non-literal path, non-glob path before calling GetUnresolvedProviderPathFromPSPath, it only accepts literal path

adityapatwardhan · 2018-01-22T17:19:05Z

@kwkam Please add tests. Marking the PR as WIP

iSazonov · 2018-01-27T17:22:13Z

@kwkam Could you continue with the PR?

kwkam · 2018-02-13T16:46:18Z

@adityapatwardhan Done. I am not sure whether they are in the proper file though

adityapatwardhan · 2018-02-22T19:16:44Z

@daxian-dbw @lzybkr @anmenaga Can you have a look?

adityapatwardhan · 2018-03-02T19:17:11Z

@anmenaga @daxian-dbw ping ...

*namespaces/LocationGlobber: Do not escape CWD when LiteralPath is used

Rename-Item will fail if -Path is relative and contains special char, and CWD also contains special char, complaining -Path does not exist because IsCurrentLocationOrAncestor can only handle literal path.

This reverts commit 2f22264.

kwkam · 2018-04-12T15:43:04Z

@adityapatwardhan Test failures related to the changes are solved (I have no idea why the remote session test is failing). Please let me know if anything were missing/wrong.

adityapatwardhan · 2018-05-10T20:32:08Z

@anmenaga @daxian-dbw ping..

iSazonov · 2018-05-19T19:32:13Z

test/powershell/Modules/Microsoft.PowerShell.Management/Remove-Item.Tests.ps1

+
+            Remove-Item -Path $testfilepathSp
+            $testfilepathSp | Should -Not -Exist
+        }


Please correct a indentation of the It block.

I believe my indentation is correct.
What I notice is, there are 93 lines of code that is mixing tab/space for indentation.
Should I fix them here?

Sorry, GitHub show a shift.
Closed.

Feel free to fix if it is only one string.

I guess I should leave it since the PR is not related to code formatting.
Perhaps open an issue for replacing tabs to spaces in the project (whole/some modules/only the misused)?

I hope we already fixed most of tabs in test .ps1 files.
Feel free to fix remains in new PR.

iSazonov

@kwkam The PR is very large. Can we split it on some new PRs?

iSazonov · 2018-05-20T15:49:29Z

src/Microsoft.PowerShell.Commands.Management/resources/NavigationResources.resx

    <value>Cannot move item because the item at '{0}' is in use.</value>
  </data>
+  <data name="RenameMultipleItemError" xml:space="preserve">
+      <value>Cannot rename item because the path '{0}' resolved to multiple items. Only one item can be renamed at a time.</value>


With long path names the message will be unreadable. Can we reword and place path '{0}' at end?

I am not sure. I just copied that from another resx file.
How about this?
Cannot rename item because only one item can be renamed at a time, but multiple items resolved from the path '{0}'.

@SteveL-MSFT Could you please review the resource string?

iSazonov · 2018-05-20T16:02:12Z

test/powershell/Modules/Microsoft.PowerShell.Core/Pester.Commands.Cmdlets.GetCommand.Tests.ps1

+        # This should find the file [.exe
+        $result = Get-Command -Name .\[.exe -Type Application
+        $result | Should -Not -BeNullOrEmpty
+        $result | Should -Be [.exe


Please use -BeExactly for string.

iSazonov · 2018-05-20T16:17:19Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

        } // ProcessRecord

-        private void MoveItem(string path)
+        private void MoveItem(string path, bool literalPath)


Should we assign default bool literalPath = false?

iSazonov · 2018-05-20T16:22:38Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

+                switch (resolvedPaths.Count)
+                {
+                    case 0:
+                        // Do nothing


Please remove obvious comment.

iSazonov · 2018-05-20T16:23:28Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

+                }
+            } while (false);
+
+        } // ProcessRecord


Please remove the comment.

iSazonov · 2018-05-20T16:25:54Z

src/System.Management.Automation/engine/CommandSearcher.cs

-                    CommandDiscovery.discoveryTracer.WriteLine(
-                        "Path resolved to: {0}",
-                        path);
+            // Find the match if it is.


Seems previous tracer comment is more useful. Please remove comment in the line or reword.

iSazonov · 2018-05-20T16:27:28Z

src/System.Management.Automation/engine/CommandSearcher.cs

-                    result = GetInfoFromPath(path);
-                }
-            } while (false);
+            // If the path was resolved, and it exists


Please remove obvious comment.

Should use BeExactly instead

iSazonov · 2018-05-22T03:52:22Z

@kwkam The PR is very large. Could you split it on some small PRs for more fast review?

kwkam · 2018-05-22T06:00:11Z

@iSazonov Sorry, I am afraid I would not have free time get it done very soon. Feel free to fork this if you are in hurry.
P.S. Appending ?w=1 to the URL (or --ignore-white-space on command line) could help a bit when viewing the diff.

stale · 2018-06-21T06:15:48Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

kwkam · 2018-07-29T04:28:25Z

Split into #7396 #7397 #7398 #7399

@iSazonov Please let me know if anything is incorrect

kwkam requested review from BrucePay, anmenaga, daxian-dbw and lzybkr as code owners January 13, 2018 08:20

daxian-dbw assigned adityapatwardhan Jan 18, 2018

daxian-dbw removed the request for review from BrucePay January 18, 2018 22:07

adityapatwardhan changed the title ~~Fix path handling when contains wildcard char~~ WIP: Fix path handling when contains wildcard char Jan 22, 2018

marckassay mentioned this pull request Jan 29, 2018

Get-ChildItem -Recurse treats literals passed to -Path like wildcard expressions #5699

Closed

kwkam added 4 commits February 12, 2018 23:07

add test

d7a0ee6

remove unnecessary cases

396c899

some more fix

694d2b2

Merge branch 'master' into fix-wcpath

8cd42a5

use existing item for test

b52c7bb

kwkam changed the title ~~WIP: Fix path handling when contains wildcard char~~ Fix path handling when contains wildcard char Feb 15, 2018

oops

91280d7

kwkam mentioned this pull request Feb 26, 2018

Creating symbolic links or junctions with New-Item doesn't work if target contains '[' or ']' #6232

Closed

kwkam added 7 commits March 3, 2018 17:55

update test

e8e3bca

add 2 more tests

b7f0794

Do not escape CWD when LiteralPath is used

29f6086

*namespaces/LocationGlobber: Do not escape CWD when LiteralPath is used

Merge branch 'master' into fix-wcpath

5a806a6

fixup! add 2 more tests

b46ecfb

try1

f13b7ae

Fix Rename-Item when CWD/Path contains special char

2f22264

Rename-Item will fail if -Path is relative and contains special char, and CWD also contains special char, complaining -Path does not exist because IsCurrentLocationOrAncestor can only handle literal path.

kwkam requested a review from adityapatwardhan as a code owner March 5, 2018 09:55

kwkam added 7 commits April 10, 2018 23:30

[Feature] minor fix

c05cc58

Revert "Fix Rename-Item when CWD/Path contains special char"

b841979

This reverts commit 2f22264.

Redo fix Rename-Item

3eee43b

[Feature] fix Move-Item

572c914

[Feature] update Move-Item test

c2c777d

[Feature] fix test

a07d177

[Feature] minimise CommandSearcher changes

b2fb902

kwkam added 4 commits April 13, 2018 15:37

fixup! Redo fix Rename-Item

9a18f57

fixup! fixup! Redo fix Rename-Item

93d53d3

[Feature] Fix FunctionProvider test

72212f5

fixup! fixup! fixup! Redo fix Rename-Item

31b1e39

kwkam changed the title ~~Fix path handling when contains wildcard char~~ Fix handling of -Path when it literally contains wildcard char May 12, 2018

[Feature] Merge branch 'master' into fix-wcpath

0877108

iSazonov reviewed May 19, 2018

View reviewed changes

iSazonov reviewed May 20, 2018

View reviewed changes

kwkam added 3 commits May 22, 2018 02:12

remove comments

de834aa

fix test

05bcd1f

Should use BeExactly instead

[Feature] set literalPath default value

e6dccd3

stale bot added the Stale label Jun 21, 2018

Merge branch 'master' into fix-wcpath

d43df71

stale bot removed the Stale label Jun 30, 2018

kwkam closed this Jul 29, 2018

kwkam deleted the fix-wcpath branch January 11, 2019 15:02

kwkam mentioned this pull request Mar 21, 2020

"Path is not parsed correctly if '[' is present in the path" #12170

Closed

14 tasks

Fix handling of -Path when it literally contains wildcard char #5896

Fix handling of -Path when it literally contains wildcard char #5896

Uh oh!

Conversation

kwkam commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

adityapatwardhan commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 27, 2018

Uh oh!

kwkam commented Feb 13, 2018

Uh oh!

adityapatwardhan commented Feb 22, 2018

Uh oh!

adityapatwardhan commented Mar 2, 2018

Uh oh!

kwkam commented Apr 12, 2018

Uh oh!

adityapatwardhan commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov May 20, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented May 22, 2018

Uh oh!

kwkam commented May 22, 2018

Uh oh!

stale bot commented Jun 21, 2018

Uh oh!

kwkam commented Jul 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwkam commented Jan 13, 2018 •

edited

Loading

adityapatwardhan commented Jan 22, 2018 •

edited

Loading

iSazonov May 20, 2018 •

edited

Loading