Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Sep 19, 2017

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

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)

Choose a reason for hiding this comment

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

... that section altogether ...

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 ...

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

will make the change

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:

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

Copy link
Member Author

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 *

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will add

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix

Copy link
Member

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.

@SteveL-MSFT 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
@SteveL-MSFT
Copy link
Member Author

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

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan
Copy link
Member

@anmenaga Can you have a look again?

@adityapatwardhan
Copy link
Member

@SteveL-MSFT Can you update the PR description.

allow * to be used in registry path for remove-item
address PR feedback
have remove-item return error if -literalpath doesn't resolve to path
fix in navigation exposed an issue with WSMan Config Provider tests
that require -Recurse to be used otherwise a confirmation prompt shows
up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants