Fix Rename-Item -Path with wildcard char by kwkam · Pull Request #7398 · PowerShell/PowerShell

kwkam · 2018-07-29T04:12:32Z

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

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.

Ilya (iSazonov)

Leave a comment

Ilya (iSazonov) · 2018-07-30T07:16:41Z

        /// </summary>
        protected override void ProcessRecord()
+        {
+            do


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.

Ilya (iSazonov) · 2018-07-30T07:21:15Z

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


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

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}'

I believe the enhancement is out of the PR and we should discuss it in a new tracking Issue (please open).

Ilya (iSazonov) · 2018-07-31T03:53:30Z

+                }
+            }
+            finally {
+            }


We can remove the empty finally block.

Ilya (iSazonov) · 2018-08-01T08:49:37Z

        protected override void ProcessRecord()
+        {
+            try
+            {


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

This reverts commit be59b3d.

Ilya (iSazonov) · 2018-08-02T03:57:35Z

kwkam Please rebase to fix CI Travis failure.

Ilya (iSazonov)

Please look tests - I don't see test related the PR.

Ilya (iSazonov) · 2018-08-02T04:09:55Z


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


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

Updated test.

Ilya (iSazonov) · 2018-08-02T04:11:11Z

+                    break;
+
+                default:
+                    RenameItem(Path, literalPath: true);


Is this really working?

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.

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.

Ilya (iSazonov) · 2018-08-02T04:17:45Z

        /// </summary>
        protected override void ProcessRecord()
+        {
+            if (base.SuppressWildcardExpansion)


Do we really need base prefix?

kwkam · 2018-08-12T04:16:07Z

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

Ilya (iSazonov) · 2018-08-12T14:05:32Z

Why do you need to rebase?

kwkam · 2018-08-12T15:42:51Z

Ilya (@iSazonov) #7398 (comment)

Ilya (iSazonov) · 2018-08-13T04:32:12Z

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

kwkam · 2018-08-13T12:47:04Z

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

Do not escape CWD when LiteralPath is used. This causes issues when both -LiteralPath and CWD contains special characters.

stale · 2018-09-30T23:44:30Z

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.

Ilya (iSazonov) · 2018-10-02T09:53:36Z

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

Steve Lee (SteveL-MSFT) · 2018-10-05T00:04:40Z

Ilya (@iSazonov) yes, interested in this change

Ilya (iSazonov) · 2018-10-05T06:32:58Z

kwkam Please rebase to pass CIs.

kwkam · 2018-10-06T08:04:26Z

Ilya (@iSazonov) OK, rebased to latest master.

stale · 2018-11-05T08:27:55Z

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.

Steve Lee (SteveL-MSFT)

LGTM

Aditya Patwardhan (adityapatwardhan) · 2018-11-09T19:08:43Z

kwkam Thank you for your contribution!

kwkam requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners July 29, 2018 04:12

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

[Feature] Fix FunctionProvider test

70327e0

fixup! [Feature] Fix FunctionProvider test

61d10dd

Ilya (iSazonov) reviewed Jul 30, 2018

View reviewed changes

[Feature] Treat Path as literal

09b5d78

Ilya (iSazonov) reviewed Jul 31, 2018

View reviewed changes

kwkam changed the title ~~[Feature] Fix Rename-Item -Path with wildcard char~~ Fix Rename-Item -Path with wildcard char Jul 31, 2018

kwkam changed the title ~~Fix Rename-Item -Path with wildcard char~~ Fix Rename-Item -Path with wildcard char Jul 31, 2018

[Feature] Remove unnecessary finally

be59b3d

Ilya (iSazonov) reviewed Aug 1, 2018

View reviewed changes

kwkam added 2 commits August 1, 2018 21:50

Revert "[Feature] Remove unnecessary finally"

ba96ba3

This reverts commit be59b3d.

[Feature] Remove try-finally

a2c29ad

Ilya (iSazonov) reviewed Aug 2, 2018

View reviewed changes

kwkam added 5 commits August 12, 2018 10:01

Move Remove-Item test to PowerShell#7404

3c9b0a1

Fix commented items

2e0615f

[Feature] Add test

8ac5d9e

fixup! Fix commented items

5a019f1

fixup! [Feature] Add test

4bed5fe

fixup! fixup! [Feature] Add test

b3adcf1

kwkam added 2 commits August 13, 2018 21:23

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.

[Feature] Add test for -LiteralPath

3356c91

kwkam requested review from Bruce Payette (BrucePay) and Andrew (anmenaga) as code owners August 13, 2018 13:28

kwkam mentioned this pull request Aug 13, 2018

LocationGlobber: Fix issue when CWD contains wildcard char #7406

Closed

4 of 11 tasks

stale Bot added the Stale label Sep 30, 2018

stale Bot removed the Stale label Oct 2, 2018

Merge branch 'master' into wc-renameitem

68ff0df

stale Bot added the Stale label Nov 5, 2018

Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) November 5, 2018 10:48

stale Bot removed the Stale label Nov 5, 2018

Steve Lee (SteveL-MSFT) reviewed Nov 5, 2018

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Resolved

Steve Lee (SteveL-MSFT) approved these changes Nov 8, 2018

View reviewed changes

Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Nov 8, 2018

Aditya Patwardhan (adityapatwardhan) approved these changes Nov 9, 2018

View reviewed changes

Aditya Patwardhan (adityapatwardhan) merged commit caad7ed into PowerShell:master Nov 9, 2018

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

Fix Rename-Item -Path with wildcard char (PowerShell#7398)

390fc03

Conversation

kwkam commented Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Ilya (iSazonov) left a comment

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!

Ilya (iSazonov) commented Aug 2, 2018

Uh oh!

Ilya (iSazonov) left a comment

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!

kwkam commented Aug 12, 2018

Uh oh!

Ilya (iSazonov) commented Aug 12, 2018

Uh oh!

kwkam commented Aug 12, 2018

Uh oh!

Ilya (iSazonov) commented Aug 13, 2018

Uh oh!

kwkam commented Aug 13, 2018

Uh oh!

stale Bot commented Sep 30, 2018

Uh oh!

Ilya (iSazonov) commented Oct 2, 2018

Uh oh!

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

Uh oh!

Ilya (iSazonov) commented Oct 5, 2018

Uh oh!

kwkam commented Oct 6, 2018

Uh oh!

stale Bot commented Nov 5, 2018

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Nov 9, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwkam commented Jul 29, 2018 •

edited

Loading