Change behavior of Remove-Item on symbolic links (#621) #3637
Conversation
When Remove-Item is used to remove a symbolic link, only the link itself is removed. The -Recurse switch, if given, is ignored.
|
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. |
|
Close/Reopen the PR to trigger the stuck CI build. |
| @@ -2863,15 +2863,6 @@ private void RemoveDirectoryInfoItem(DirectoryInfo directory, bool recurse, bool | |||
| continueRemoval = ShouldProcess(directory.FullName, action); | |||
| } | |||
|
|
|||
| //if this is a reparse point and force is not specified then warn user but dont remove the directory. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've updated the PR description and removed the unused DirectoryReparsePoint message string.
| try | ||
| { | ||
| DirectoryInfo directory = new DirectoryInfo(path); | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -246,6 +246,171 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { | |||
| } | |||
| } | |||
|
|
|||
| Describe "Hard link and symbolic link tests" -Tags "CI", "RequireAdminOnWindows" { | |||
There was a problem hiding this comment.
Your description mentions that the -Recurse parameter switch was ignored. But I don't see any tests to verify that it is fixed.
There was a problem hiding this comment.
Added a test using the -Recurse parameter when removing a symlink to a directory
| $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 |
There was a problem hiding this comment.
Please use common template:
$childB.Count | Should BeExactly $childA.Count
$childB.Name | Should BeExactly $childA.Name | 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 |
There was a problem hiding this comment.
Should we use -Force and -ErrorAction SilentlyContinue or maybe -ErrorAction Continue?
There was a problem hiding this comment.
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.
| Test-Path $hardLinkToFile | Should Be $true | ||
| $link = Get-Item -Path $hardLinkToFile | ||
| $link.LinkType | Should Be "HardLink" | ||
| Get-Content -Path $hardLinkToFile | Should be $fileContent |
There was a problem hiding this comment.
I would prefer BeExactly for strings in all test in the file.
| # 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" |
There was a problem hiding this comment.
Plaese add "on Unix" in titles.
| # Junctions and directory symbolic links are Windows and NTFS only | ||
| $windowsTestCases = @( | ||
| @{ | ||
| Name = "Remove-Item can remove a symbolic link to a directory" |
There was a problem hiding this comment.
Plaese add "on Windows" in titles.
|
@PaulHigin @iSazonov Can I get an updated review, please? |
|
@jeffbi The tests are beautiful. Thanks! |
|
Thanks @iSazonov and @PaulHigin for the review! |
|
@jeffbi Great work! Thanks! |
| Target = $realDir | ||
| } | ||
| @{ | ||
| Name = "Remove-Item can remove a junction to a directory" |
When
Remove-Itemis used to remove a symbolic link in Windows, only the link itself is removed. The-Forceswitch 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
-Recurseswitch, if given, is ignored.This brings
Remove-Item more in line with the behavior of the 'rm'command on Unix.Partially fix #621