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.

iSazonov

Leave a comment

iSazonov · 2018-07-30T07:16:41Z

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

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

iSazonov · 2018-07-30T07:21:15Z

src/Microsoft.PowerShell.Commands.Management/resources/NavigationResources.resx

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

iSazonov · 2018-07-31T03:53:30Z

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

+                }
+            }
+            finally {
+            }


We can remove the empty finally block.

iSazonov · 2018-08-01T08:49:37Z

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

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


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

This reverts commit be59b3d.

iSazonov · 2018-08-02T03:57:35Z

@kwkam Please rebase to fix CI Travis failure.

iSazonov

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

iSazonov · 2018-08-02T04:09:55Z

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


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

iSazonov · 2018-08-02T04:11:11Z

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

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

iSazonov · 2018-08-02T04:17:45Z

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

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


Do we really need base prefix?

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

@iSazonov Should I run git rebase master?

iSazonov · 2018-08-12T14:05:32Z

Why do you need to rebase?

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

@iSazonov #7398 (comment)

iSazonov · 2018-08-13T04:32:12Z

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

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

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

iSazonov · 2018-10-02T09:53:36Z

@SteveL-MSFT do we have interest in the change?

SteveL-MSFT · 2018-10-05T00:04:40Z

@iSazonov yes, interested in this change

iSazonov · 2018-10-05T06:32:58Z

@kwkam Please rebase to pass CIs.

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

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

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

SteveL-MSFT

LGTM

adityapatwardhan · 2018-11-09T19:08:43Z

@kwkam Thank you for your contribution!

kwkam requested review from adityapatwardhan and 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

9 tasks

kwkam added 2 commits July 29, 2018 19:22

[Feature] Fix FunctionProvider test

70327e0

fixup! [Feature] Fix FunctionProvider test

61d10dd

iSazonov reviewed Jul 30, 2018

View reviewed changes

[Feature] Treat Path as literal

09b5d78

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

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

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

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

iSazonov requested a review from SteveL-MSFT November 5, 2018 10:48

stale bot removed the Stale label Nov 5, 2018

SteveL-MSFT reviewed Nov 5, 2018

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Show resolved Hide resolved

SteveL-MSFT approved these changes Nov 8, 2018

View reviewed changes

SteveL-MSFT assigned adityapatwardhan Nov 8, 2018

adityapatwardhan approved these changes Nov 9, 2018

View reviewed changes

adityapatwardhan merged commit caad7ed into PowerShell:master Nov 9, 2018

kwkam deleted the wc-renameitem branch January 11, 2019 15:05

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!

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!

iSazonov commented Aug 2, 2018

Uh oh!

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!

iSazonov commented Aug 12, 2018

Uh oh!

kwkam commented Aug 12, 2018

Uh oh!

iSazonov commented Aug 13, 2018

Uh oh!

kwkam commented Aug 13, 2018

Uh oh!

stale bot commented Sep 30, 2018

Uh oh!

iSazonov commented Oct 2, 2018

Uh oh!

SteveL-MSFT commented Oct 5, 2018

Uh oh!

iSazonov commented Oct 5, 2018

Uh oh!

kwkam commented Oct 6, 2018

Uh oh!

stale bot commented Nov 5, 2018

Uh oh!

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Nov 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwkam commented Jul 29, 2018 •

edited

Loading