-
Notifications
You must be signed in to change notification settings - Fork 8.1k
allow * to be used in registry path for remove-item #4866
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
allow * to be used in registry path for remove-item #4866
Conversation
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)
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 ...
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 ...
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.
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
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 *
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 *
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*.xmlI'll fix it so it returns error when using -literalpath, but it's a breaking change
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
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
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 $testPathThere 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
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.
|
@PowerShell/powershell-committee reviewed this and is ok with the minor |
adityapatwardhan
left a comment
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
|
@anmenaga Can you have a look again? |
|
@SteveL-MSFT Can you update the PR description. |
2f4ba34 to
bc5c902
Compare
a81e6f3 to
4dc5013
Compare
4dc5013 to
5a16dc4
Compare
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-literalpathwith an asterisk and the filesystem provider will now return an error rather than returning quietly.Fix #4704
Fix #4330