GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — allow * to be used in registry path for remove-item by SteveL-MSFT · Pull Request #4866 · PowerShell/PowerShell

Conversation

Steve Lee (SteveL-MSFT) commented Sep 19, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan)

Sep 19, 2017

Steve Lee (SteveL-MSFT) requested a review from Andrew (anmenaga)

September 19, 2017 20:26

Steve Lee (SteveL-MSFT) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners

September 19, 2017 20:26

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Sep 19, 2017

Andrew (anmenaga) suggested changes

Sep 19, 2017

View reviewed changes

Comment thread

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

Andrew (anmenaga) Sep 19, 2017

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew (anmenaga) Sep 19, 2017

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

... that section altogether ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew (anmenaga) Sep 19, 2017

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Sep 19, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will make the change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Andrew (anmenaga) Sep 19, 2017

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

remove-item with -Path and filesystem path that contains *
remove-item with -LiteralPath and filesystem path that contains *

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adityapatwardhan reacted with thumbs up emoji

All reactions

👍 1 reaction

Steve Lee (SteveL-MSFT) Sep 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) requested changes

Sep 19, 2017

View reviewed changes

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Management/Registry.Tests.ps1 Outdated

Aditya Patwardhan (adityapatwardhan) Sep 19, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add -ErrorAction SilentlyContinue -Force

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will add

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Management/Registry.Tests.ps1 Outdated

Aditya Patwardhan (adityapatwardhan) Sep 19, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please assign to null.

$null = New-Item -Force $testPath

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Aditya Patwardhan (adityapatwardhan) Sep 19, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) added Breaking-Change

breaking change that may affect users

Review - Committee

The PR/Issue needs a review from the PowerShell Committee

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

Sep 20, 2017

Steve Lee (SteveL-MSFT) commented Sep 20, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) approved these changes

Sep 22, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) commented Sep 22, 2017

Copy link

Copy Markdown

Member

Andrew (@anmenaga) Can you have a look again?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew (anmenaga) approved these changes

Sep 22, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) commented Sep 22, 2017

Copy link

Copy Markdown

Member

Steve Lee (@SteveL-MSFT) Can you update the PR description.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) added 2 commits

September 22, 2017 16:12


          [feature]

a3c8508

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


          [feature]

bc5c902

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

Steve Lee (SteveL-MSFT) force-pushed the registry-removeitem branch from 2f4ba34 to bc5c902 Compare

September 22, 2017 23:12

Steve Lee (SteveL-MSFT) mentioned this pull request

Sep 23, 2017

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

Closed

Steve Lee (SteveL-MSFT) closed this

Sep 23, 2017

Steve Lee (SteveL-MSFT) reopened this

Sep 23, 2017

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Sep 23, 2017

Steve Lee (SteveL-MSFT) force-pushed the registry-removeitem branch 2 times, most recently from a81e6f3 to 4dc5013 Compare

September 24, 2017 14:16


          [feature]

5a16dc4

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

Steve Lee (SteveL-MSFT) force-pushed the registry-removeitem branch from 4dc5013 to 5a16dc4 Compare

September 24, 2017 15:15

Aditya Patwardhan (adityapatwardhan) merged commit 65415c3 into PowerShell:master

Sep 25, 2017

Steve Lee (SteveL-MSFT) mentioned this pull request

Oct 4, 2017

beta.8 changelog draft #5006

Merged

Roman Kuzmin 1967-2026 (nightroman) mentioned this pull request

Oct 6, 2017

v6-beta.8: Potential regression of Remove-Item with wildcards and missing items #5031

Closed

Roman Kuzmin 1967-2026 (nightroman) mentioned this pull request

Oct 8, 2017

Get-Item -LiteralPath with FileSystem should fail on paths with wildcards #5057

Closed

Steve Lee (SteveL-MSFT) deleted the registry-removeitem branch

October 26, 2018 21:33

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


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

0ddb126

* [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

Reviewers

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

+1 more reviewer

Andrew (anmenaga)

anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Aditya Patwardhan (adityapatwardhan)

Labels

Breaking-Change

breaking change that may affect users

Committee-Reviewed

PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Steve Lee (SteveL-MSFT) commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Steve Lee (SteveL-MSFT) commented Sep 20, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Sep 22, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Sep 22, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Steve Lee (SteveL-MSFT) commented Sep 19, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.