GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix Rename-Item -Path with wildcard char by kwkam · Pull Request #7398 · PowerShell/PowerShell

Conversation

@kwkam

@kwkam kwkam commented Jul 29, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

PR Summary

*Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs:
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 characters.

*System.Management.Automation/namespaces/LocationGlobber.cs:
Do not escape CWD when LiteralPath is used.
This causes issues when both -LiteralPath and CWD contains special characters. For example,

Rename-Item -LiteralPath [test]file.txt [test]file2.txt

will complain that [test]file.txt does not exist when CWD contains special characters.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • 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.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kwkam
[Feature] Fix Rename-Item -Path with wildcard char
0be2d49 
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 characters that forms an invalid pattern.
@kwkam
kwkam requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners July 29, 2018 04:12
@kwkam kwkam mentioned this pull request Jul 29, 2018
Fix handling of -Path when it literally contains wildcard char #5896
Closed
7 of 9 tasks
kwkam added 2 commits July 29, 2018 19:22
@kwkam
[Feature] Fix FunctionProvider test
70327e0
@kwkam
fixup! [Feature] Fix FunctionProvider test
61d10dd
iSazonov
Ilya (iSazonov) reviewed Jul 30, 2018
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

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.

Leave a comment

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
/// </summary>
protected override void ProcessRecord()
{
do

@iSazonov Ilya (iSazonov) Jul 30, 2018

Copy link
Copy Markdown
Collaborator

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.

Why do we use cycle here? We have single input string.
I would try to make things easier - just try to rename in try-catch.

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/resources/NavigationResources.resx Outdated
<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>

@iSazonov Ilya (iSazonov) Jul 30, 2018

Copy link
Copy Markdown
Collaborator

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.

It makes no sense to tell the user that we have tried to resolve wildcards - in fact, we consider the Path parameter as literal.

Sorry, something went wrong.

Uh oh!

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

@kwkam kwkam Jul 30, 2018

Copy link
Copy Markdown
Contributor 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.

I wonder if we would add support for renaming multiple items like the ren command in cmd?
eg. Rename-Item -Path '*.txt' -NewExtension '.bak'
or, Rename-Item -Path '*.txt' -Format 'renamed_{0}'

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jul 30, 2018

Copy link
Copy Markdown
Collaborator

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 believe the enhancement is out of the PR and we should discuss it in a new tracking Issue (please open).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kwkam
[Feature] Treat Path as literal
09b5d78
iSazonov
Ilya (iSazonov) reviewed Jul 31, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
}
}
finally {
}

@iSazonov Ilya (iSazonov) Jul 31, 2018

Copy link
Copy Markdown
Collaborator

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.

We can remove the empty finally block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kwkam kwkam changed the title [Feature] Fix Rename-Item -Path with wildcard char Fix Rename-Item -Path with wildcard char Jul 31, 2018
@kwkam kwkam changed the title Fix Rename-Item -Path with wildcard char Fix Rename-Item -Path with wildcard char Jul 31, 2018
@kwkam
[Feature] Remove unnecessary finally
be59b3d
iSazonov
Ilya (iSazonov) reviewed Aug 1, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
protected override void ProcessRecord()
{
try
{

@iSazonov Ilya (iSazonov) Aug 1, 2018

Copy link
Copy Markdown
Collaborator

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.

Seem all used methods use try-catch and we can remove the try.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kwkam added 2 commits August 1, 2018 21:50
@kwkam
Revert "[Feature] Remove unnecessary finally"
ba96ba3 
This reverts commit be59b3d.
@kwkam
[Feature] Remove try-finally
a2c29ad
@iSazonov

Ilya (iSazonov) commented Aug 2, 2018

Copy link
Copy Markdown
Collaborator

kwkam Please rebase to fix CI Travis failure.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Aug 2, 2018
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

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 look tests - I don't see test related the PR.

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

private Collection<PathInfo> GetResolvedPaths(string path)
{
Collection<PathInfo> results = new Collection<PathInfo>();

@iSazonov Ilya (iSazonov) Aug 2, 2018

Copy link
Copy Markdown
Collaborator

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.

It is extra allocation. I think we should remove this and check null in calling code.

Sorry, something went wrong.

Uh oh!

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

@kwkam kwkam Aug 31, 2018

Copy link
Copy Markdown
Contributor 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.

Updated test.

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
break;

default:
RenameItem(Path, literalPath: true);

@iSazonov Ilya (iSazonov) Aug 2, 2018

Copy link
Copy Markdown
Collaborator

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.

Is this really working?

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Aug 2, 2018

Copy link
Copy Markdown
Collaborator

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'd expect that we unescape path, check existance (or try rename), if fail then resolve globs (write an error if cpunt > 1) and try rename again.

Sorry, something went wrong.

Uh oh!

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

@kwkam kwkam Aug 22, 2018

Copy link
Copy Markdown
Contributor 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.

I believe -Path should try to resolve globs first, since it's the expected behaviour, and users can escape the wildcard character, for example

Rename-Item -Path '`[A-B`]' -NewName 'A-B'

will prevent globbing.

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
/// </summary>
protected override void ProcessRecord()
{
if (base.SuppressWildcardExpansion)

@iSazonov Ilya (iSazonov) Aug 2, 2018

Copy link
Copy Markdown
Collaborator

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.

Do we really need base prefix?

Sorry, something went wrong.

Uh oh!

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

@kwkam kwkam Aug 31, 2018

Copy link
Copy Markdown
Contributor 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.

Removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kwkam added 5 commits August 12, 2018 10:01
@kwkam
Move Remove-Item test to PowerShell#7404
3c9b0a1
@kwkam
Fix commented items
2e0615f
@kwkam
[Feature] Add test
8ac5d9e
@kwkam
fixup! Fix commented items
5a019f1
@kwkam
fixup! [Feature] Add test
4bed5fe
@kwkam

kwkam commented Aug 12, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) Should I run git rebase master?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kwkam
fixup! fixup! [Feature] Add test
b3adcf1
@iSazonov

Ilya (iSazonov) commented Aug 12, 2018

Copy link
Copy Markdown
Collaborator

Why do you need to rebase?
All reactions

Sorry, something went wrong.

Uh oh!

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

kwkam commented Aug 12, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) #7398 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Aug 13, 2018

Copy link
Copy Markdown
Collaborator

kwkam Ok, I understand. You can rebase to temporary skip broken tests.
All reactions

Sorry, something went wrong.

Uh oh!

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

kwkam commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) I see. I would not rebase if it is not necessary.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kwkam added 2 commits August 13, 2018 21:23
@kwkam
LocationGlobber: Fix issue when CWD contains wildcard char
431f015 
Do not escape CWD when LiteralPath is used.
This causes issues when both -LiteralPath and CWD contains special characters.
@kwkam
[Feature] Add test for -LiteralPath
3356c91
@kwkam
kwkam requested review from Bruce Payette (BrucePay) and Andrew (anmenaga) as code owners August 13, 2018 13:28
@kwkam kwkam mentioned this pull request Aug 13, 2018
LocationGlobber: Fix issue when CWD contains wildcard char #7406
Closed
4 of 11 tasks
@stale

stale Bot commented Sep 30, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stale stale Bot added the Stale label Sep 30, 2018
@iSazonov

Ilya (iSazonov) commented Oct 2, 2018

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) do we have interest in the change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stale stale Bot removed the Stale label Oct 2, 2018
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 5, 2018

Copy link
Copy Markdown
Member

Ilya (@iSazonov) yes, interested in this change
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Oct 5, 2018

Copy link
Copy Markdown
Collaborator

kwkam Please rebase to pass CIs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kwkam
Merge branch 'master' into wc-renameitem
68ff0df
@kwkam

kwkam commented Oct 6, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) OK, rebased to latest master.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

stale Bot commented Nov 5, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stale stale Bot added the Stale label Nov 5, 2018
@iSazonov
Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) November 5, 2018 10:48
@stale stale Bot removed the Stale label Nov 5, 2018
SteveL-MSFT
Steve Lee (SteveL-MSFT) reviewed Nov 5, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Nov 8, 2018
View reviewed changes

@SteveL-MSFT Steve Lee (SteveL-MSFT) 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
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Nov 8, 2018
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 9, 2018
View reviewed changes
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit caad7ed into PowerShell:master Nov 9, 2018
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Nov 9, 2018

Copy link
Copy Markdown
Member

kwkam Thank you for your contribution!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kwkam
kwkam deleted the wc-renameitem branch January 11, 2019 15:05
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@kwkam @adityapatwardhan
Fix Rename-Item -Path with wildcard char (PowerShell#7398)
390fc03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay

Uh oh!

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

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

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

@kwkam @iSazonov @SteveL-MSFT @adityapatwardhan
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 01402520 — 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.