Change behavior of Remove-Item on symbolic links (#621) by jeffbi · Pull Request #3637 · PowerShell/PowerShell

jeffbi · 2017-04-24T18:42:44Z

When Remove-Item is used to remove a symbolic link in Windows, only the link itself is removed. The -Force switch is no longer required.

If the directory pointed to by the link has child items, the cmdlet no longer prompts the user to remove the child items---those child items are not removed. The -Recurse switch, if given, is ignored.

This brings Remove-Item more in line with the behavior of the 'rm' command on Unix.

Partially fix #621

When Remove-Item is used to remove a symbolic link, only the link itself is removed. The -Recurse switch, if given, is ignored.

PaulHigin · 2017-04-25T16:31:26Z

Please provide more information on the issue along with example of current behavior and example of expected behavior. What do you expect the recursive switch parameter to do, remove all files as well? This sounds like a breaking change (Remove-Item no longer just removes a symbolic link) and if so we should mark this PR accordingly. Also please discuss the fix. It is not clear to me how these changes solve the problem.

daxian-dbw · 2017-04-25T16:35:05Z

Close/Reopen the PR to trigger the stuck CI build.

PaulHigin · 2017-04-25T16:40:31Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                continueRemoval = ShouldProcess(directory.FullName, action);
            }

-            //if this is a reparse point and force is not specified then warn user but dont remove the directory.


Why is this removed. It seems like the -Force parameter was required before removing and was Windows only. So I assume removing this requirement makes Windows and Linux behavior the same. Please make this clear in the PR description.

Also if we remove this code we should also remove the associated FileSystemProviderStrings.DirectoryReparsePoint message string since it is no longer used.

I've updated the PR description and removed the unused DirectoryReparsePoint message string.

PaulHigin · 2017-04-25T16:42:23Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs


                // If the above didn't throw an exception, check to
-                // see if it contains any directories
+                // see if we should proceed and if it contains any children


This is a bit surprising. What case does this cover where an exception is not thrown but Attributes is not Directory? Please add a comment to clarify.

The DirectoryInfo constructor will succeed if the given path does not exist or if it actually refers to a file. In the former case the Exists property will be false, in the latter the Attributes property will lack the Directory attribute.

The myriad other ways the constructor will fail are explicitly called out in the various catch blocks.

PaulHigin · 2017-04-25T16:54:31Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

    }
 }

+Describe "Hard link and symbolic link tests" -Tags "CI", "RequireAdminOnWindows" {


Your description mentions that the -Recurse parameter switch was ignored. But I don't see any tests to verify that it is fixed.

Added a test using the -Recurse parameter when removing a symlink to a directory

…ing a symlink.

iSazonov · 2017-04-27T05:53:35Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            $childB = Get-ChildItem folder
+            $childB.Count | Should Be 1
+            $childB.Count -eq $childA.Count | Should Be $true
+            $childB.Name -eq $childA.Name | Should Be $true


Please use common template:

$childB.Count | Should BeExactly $childA.Count $childB.Name | Should BeExactly $childA.Name

iSazonov · 2017-04-27T05:55:28Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            New-Item -ItemType File -Path $file -Value "some content" >$null
+            New-Item -ItemType SymbolicLink -Path $link -value $folder >$null
+            $childA = Get-Childitem $folder
+            Remove-Item -Path $link -Recurse


Should we use -Force and -ErrorAction SilentlyContinue or maybe -ErrorAction Continue?

Not quite clear on this one. This test shows that when we use -Recurse when deleting a symlink, it is ignored. Remove-Item should not fail in this circumstance, it should just remove the symlink without even trying to recurse into the linked-to directory.

iSazonov · 2017-04-27T05:59:56Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            Test-Path $hardLinkToFile | Should Be $true
+            $link = Get-Item -Path $hardLinkToFile
+            $link.LinkType | Should Be "HardLink"
+            Get-Content -Path $hardLinkToFile | Should be $fileContent


I would prefer BeExactly for strings in all test in the file.

iSazonov · 2017-04-27T06:02:51Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            # New-Item on Windows will not create a "plain" symlink to a directory
+            $unixTestCases = @(
+                @{
+                    Name = "Remove-Item can remove a symbolic link to a directory"


Plaese add "on Unix" in titles.

iSazonov · 2017-04-27T06:03:04Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            # Junctions and directory symbolic links are Windows and NTFS only
+            $windowsTestCases = @(
+                @{
+                    Name = "Remove-Item can remove a symbolic link to a directory"


Plaese add "on Windows" in titles.

jeffbi · 2017-04-27T22:53:36Z

@PaulHigin @iSazonov Can I get an updated review, please?

PaulHigin

LGTM

daxian-dbw · 2017-04-28T00:44:52Z

@jeffbi The tests are beautiful. Thanks!

daxian-dbw · 2017-04-28T00:48:33Z

Thanks @iSazonov and @PaulHigin for the review!

iSazonov · 2017-04-28T02:54:58Z

@jeffbi Great work! Thanks!

iSazonov · 2017-04-28T03:18:01Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+                    Target = $realDir
+                }
+                @{
+                    Name = "Remove-Item can remove a junction to a directory"


"on Windows" skipped 😄

Change behavior of Remove-Item on symbolic links (#621)

ad1a63e

When Remove-Item is used to remove a symbolic link, only the link itself is removed. The -Recurse switch, if given, is ignored.

msftclas added the cla-already-signed label Apr 24, 2017

Fix a test issue causing Travis CI to fail on OS X

be795b0

daxian-dbw closed this Apr 25, 2017

daxian-dbw reopened this Apr 25, 2017

msftclas added the cla-already-signed label Apr 25, 2017

daxian-dbw self-assigned this Apr 25, 2017

daxian-dbw requested a review from PaulHigin April 25, 2017 16:53

PaulHigin suggested changes Apr 25, 2017

View reviewed changes

Remove unused message string. Added test of using -Recurse when remov…

f988991

…ing a symlink.

iSazonov reviewed Apr 27, 2017

View reviewed changes

Jeff Bienstadt added 2 commits April 27, 2017 09:22

Address issues in code review. Fix additional bug it test.

b5808fb

Address one more code review issue.

ed4e77d

PaulHigin approved these changes Apr 27, 2017

View reviewed changes

daxian-dbw merged commit f1769fe into PowerShell:master Apr 28, 2017

daxian-dbw mentioned this pull request Apr 28, 2017

Fix Remove-Item <symbolic link to directory> #621

Closed

iSazonov reviewed Apr 28, 2017

View reviewed changes

jeffbi deleted the remove-item-621 branch May 8, 2017 05:05

iSazonov mentioned this pull request Dec 13, 2019

Fix error message during symlink deletion #11331

Merged

14 tasks

Conversation

jeffbi commented Apr 24, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulHigin commented Apr 25, 2017

Uh oh!

daxian-dbw commented Apr 25, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

jeffbi commented Apr 27, 2017

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Apr 28, 2017

Uh oh!

daxian-dbw commented Apr 28, 2017

Uh oh!

iSazonov commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jeffbi commented Apr 24, 2017 •

edited by daxian-dbw

Loading