/PowerShell

allow * to be used in registry path for remove-item #4866

Merged
merged 3 commits into from Sep 25, 2017

Conversation

Reviewers

@anmenaga anmenaga

@adityapatwardhan adityapatwardhan

@daxian-dbw daxian-dbw

Assignees

@adityapatwardhan adityapatwardhan

Labels
Breaking-Change Committee-Reviewed
Projects
None yet
Milestone
No milestone
4 participants
@SteveL-MSFT @adityapatwardhan @anmenaga @msftclas
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 19, 2017 •
Edited 1 time

Code is creating a DirectoryInfo instance to determine if the path is a directory or file, but it fails because * is not allowed, however, * is a valid character in a registry path. Fix is to catch this exception and ignore it. This means that using -literalpath with an asterisk and the filesystem provider will now return an error rather than returning quietly.

Fix #4704
Fix #4330

@SteveL-MSFT SteveL-MSFT assigned adityapatwardhan Sep 19, 2017

@SteveL-MSFT SteveL-MSFT requested a review from anmenaga Sep 19, 2017

@SteveL-MSFT SteveL-MSFT requested review from adityapatwardhan and daxian-dbw as code owners Sep 19, 2017

@msftclas msftclas added the cla-not-required label Sep 19, 2017

...crosoft.PowerShell.Commands.Management/commands/management/Navigation.cs
@@ -3266,6 +3266,10 @@ protected override void ProcessRecord()
{
// not a directory
}
+ catch (System.ArgumentException)
@anmenaga

anmenaga Sep 19, 2017

Contributor

Maybe it is better to skip that altogether if provider is not FileSystem provider?
e.g.: resolvedPath.Provider.Name.Equals(FileSystemProvider.ProviderName, StringComparison.OrdinalIgnoreCase)

@anmenaga

anmenaga Sep 19, 2017

Contributor

... that section altogether ...

@anmenaga

anmenaga Sep 19, 2017

Contributor

... that "DirectoryInfo code section" section altogether ...

@adityapatwardhan

adityapatwardhan Sep 19, 2017

Member

I agree checking for the provider seems to be a better option.

@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

will make the change

...crosoft.PowerShell.Commands.Management/commands/management/Navigation.cs
@@ -3266,6 +3266,10 @@ protected override void ProcessRecord()
{
// not a directory
}
+ catch (System.ArgumentException)
+ {
@anmenaga

anmenaga Sep 19, 2017

Contributor

Need to make sure that FileSystem paths continue to work as they were before this new catch handler; 2 tests:

  1. remove-item with -Path and filesystem path that contains *
  2. remove-item with -LiteralPath and filesystem path that contains *
@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

Looks like there's tests for -Path, I'll add a test for -LiteralPath with *

@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

Looks like the current code does nothing if you use a * with -literalpath:

remove-item -literalpath .\a*.xml

I'll fix it so it returns error when using -literalpath, but it's a breaking change

...crosoft.PowerShell.Commands.Management/commands/management/Navigation.cs
@@ -3266,6 +3266,10 @@ protected override void ProcessRecord()
{
// not a directory
}
+ catch (System.ArgumentException)
@anmenaga

anmenaga Sep 19, 2017

Contributor

Maybe it is better to skip that altogether if provider is not FileSystem provider?
e.g.: resolvedPath.Provider.Name.Equals(FileSystemProvider.ProviderName, StringComparison.OrdinalIgnoreCase)

@anmenaga

anmenaga Sep 19, 2017

Contributor

... that section altogether ...

@anmenaga

anmenaga Sep 19, 2017

Contributor

... that "DirectoryInfo code section" section altogether ...

@adityapatwardhan

adityapatwardhan Sep 19, 2017

Member

I agree checking for the provider seems to be a better option.

@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

will make the change

test/powershell/Modules/Microsoft.PowerShell.Management/Registry.Tests.ps1
+ try {
+ $tempPath = "HKCU:\_tmp"
+ $testPath = "$tempPath\*\sub"
+ New-Item -Force $testPath
@adityapatwardhan

adityapatwardhan Sep 19, 2017

Member

Please assign to null.

$null = New-Item -Force $testPath
@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

will fix

test/powershell/Modules/Microsoft.PowerShell.Management/Registry.Tests.ps1
+ $testPath | Should Not Exist
+ }
+ finally {
+ Remove-Item -Recurse $tempPath
@adityapatwardhan

adityapatwardhan Sep 19, 2017

Member

Please add -ErrorAction SilentlyContinue -Force

@SteveL-MSFT

SteveL-MSFT Sep 20, 2017

Owner

will add

@SteveL-MSFT SteveL-MSFT added Breaking-Change Review - Committee Committee-Reviewed and removed Review - Committee labels Sep 20, 2017

@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 20, 2017

@PowerShell/powershell-committee reviewed this and is ok with the minor breaking change
@adityapatwardhan
Member

adityapatwardhan commented Sep 22, 2017

@anmenaga Can you have a look again?
@adityapatwardhan
Member

adityapatwardhan commented Sep 22, 2017

@SteveL-MSFT Can you update the PR description.

SteveL-MSFT added some commits Sep 19, 2017

@SteveL-MSFT
[feature] 
allow * to be used in registry path for remove-item
a3c8508
@SteveL-MSFT
[feature] 
address PR feedback
have remove-item return error if -literalpath doesn't resolve to path

Some checks were not successful

1 failing and 1 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
bc5c902

@SteveL-MSFT SteveL-MSFT referenced this pull request Sep 23, 2017

Closed

Remove-Item: potential v6 regression on path like Function:\Global:* #4330

@SteveL-MSFT SteveL-MSFT closed this Sep 23, 2017

@SteveL-MSFT SteveL-MSFT reopened this Sep 23, 2017

@msftclas msftclas added the cla-not-required label Sep 23, 2017

@SteveL-MSFT
[feature] 
fix in navigation exposed an issue with WSMan Config Provider tests
that require -Recurse to be used otherwise a confirmation prompt shows
up

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
5a16dc4

@adityapatwardhan adityapatwardhan merged commit 65415c3 into PowerShell:master Sep 25, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Oct 4, 2017

Merged
Closed
Closed

powercode pushed a commit to powercode/PowerShell that referenced this pull request Oct 19, 2017

@SteveL-MSFT
allow * to be used in registry path for remove-item (#4866) 
* [feature]
allow * to be used in registry path for remove-item

* [feature]
address PR feedback
have remove-item return error if -literalpath doesn't resolve to path

* [feature]
fix in navigation exposed an issue with WSMan Config Provider tests
that require -Recurse to be used otherwise a confirmation prompt shows
up
82a9d38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment