8000 Enhance Remove-Item to work with OneDrive (Second) by iSazonov · Pull Request #15260 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Enhance Remove-Item to work with OneDrive (Second) #15260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Apr 27, 2021
Merged

Enhance Remove-Item to work with OneDrive (Second) #15260

merged 7 commits into from
Apr 27, 2021

Conversation

iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Apr 17, 2021 •
edited
Loading

PR Summary

Better to review commit by commit:

  1. First commit comes from Enhance Remove-Item to work with OneDrive #14902
  2. Second commit fix On Windows, Remove-Item unexpectedly fails if the target directory path happens to end in a backslash #15248 (see also Revert "Enhance Remove-Item to work with OneDrive (#14902)" #15253)
  3. Third commit reformat old tests
  4. Fourth commit add new test

About second commit.

Partial regression in #14902. Partial because there are other code paths (a path comes from public with trailing backslash) which can raise an issue in the place.

Docs say that FindFirstFileEx does not accept a path with a trailing backslash (\).

The fix is always removing a trailing backslash from path string before calling FindFirstFileEx.

Before the fix only enumeration (Get-ChildItem) worked on OneDrive.
Now Remove-Item correctly removes folders and files on OneDrive too.

  1. Enhance IsReparsePointLikeSymlink() method so that exclude non named surrogates from reparse points. As result PowerShell recurse into such reparse points which is a OneDrive entities.
  2. Use updated IsReparsePointLikeSymlink() method in RemoveDirectoryInfoItem() and NameString() methods
  3. Add new tests and a test hook to emulate old PowerShell behavior and test new PowerShell behavior on OneDrive.
  4. Add small perf improvement in Dir() and IsReparsePointLikeSymlink() methods to exclude extra p/invoke in most scenarios.

PR Context

Replace #14860 and #14902 (reverted in #15253)

Related #9246

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 17, 2021
@iSazonov iSazonov requested a review from rjmholt April 17, 2021 14:48
@iSazonov iSazonov force-pushed the fix-isreparsepointlikesymlink2 branch from a1d15d6 to 1bc1de8 Compare April 17, 2021 15:37
@TravisEz13

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@TravisEz13
Copy link
Member

/azp run PowerShell-CI-Windows-daily

@azure-pipelines
Copy link
Azure Pipelines successfully started running 1 pipeline(s).

@TravisEz13
Copy link
Member

@rjmholt See how to run the CI that tests the issue here: #15260 (comment)

@iSazonov iSazonov mentioned this pull request Apr 20, 2021
Closed
@ghost ghost added the Review - Needed The PR is being reviewed label Apr 27, 2021
@ghost
Copy link
ghost commented Apr 27, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@rjmholt
Copy link
Collaborator
rjmholt commented Apr 27, 2021

/azp run PowerShell-CI-Windows-daily

@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 27, 2021
@azure-pipelines
Copy link
Azure Pipelines successfully started running 1 pipeline(s).

iSazonov and others added 3 commits April 27, 2021 22:06
@rjmholt
Copy link
Collaborator
rjmholt commented Apr 27, 2021

Ok this seems to be passing CI so I'm going to merge, but we'll just need to keep an eye out for CI failures later on

@rjmholt rjmholt merged commit 08e7313 into PowerShell:master Apr 27, 2021
@iSazonov iSazonov deleted the fix-isreparsepointlikesymlink2 branch April 28, 2021 04:28
@TravisEz13
Copy link
Member

Verified this passed CI after merge:
https://dev.azure.com/powershell/PowerShell/_build/results?buildId=77087&view=results

rkeithhill pushed a commit to rkeithhill/PowerShell that referenced this pull request May 3, 2021
@iSazonov @rkeithhill
Enhance Remove-Item to work with OneDrive (Second) (PowerShell#15260)
5e12a84
@rjmholt
Copy link
Collaborator
rjmholt commented May 26, 2021 •
edited
Loading

We've picked up a test failure during release testing that we think is caused by this change.

The failing tests are these:

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

Lines 960 to 999 in dd5cf86

Describe "Handling long paths" -Tags "CI" {
BeforeAll {
$longDir = 'a' * 250
$longSubDir = 'b' * 250
$fileName = "file1.txt"
$topPath = Join-Path $TestDrive $longDir
$longDirPath = Join-Path $topPath $longSubDir
$longFilePath = Join-Path $longDirPath $fileName
$cwd = Get-Location
}
BeforeEach {
New-Item -ItemType File -Path $longFilePath -Force | Out-Null
}
AfterEach {
Remove-Item -Path $topPath -Force -Recurse -ErrorAction SilentlyContinue
Set-Location $cwd
}
It "Can remove a file via a long path" {
Remove-Item -Path $longFilePath -ErrorVariable e -ErrorAction SilentlyContinue
$e | Should -BeNullOrEmpty
$longFilePath | Should -Not -Exist
}
It "Can rename a file via a long path" {
$newFileName = "new-file.txt"
$newPath = Join-Path $longDirPath $newFileName
Rename-Item -Path $longFilePath -NewName $newFileName
$longFilePath | Should -Not -Exist
$newPath | Should -Exist
}
It "Can change into a directory via a long path" {
Set-Location -Path $longDirPath -ErrorVariable e -ErrorAction SilentlyContinue
$e | Should -BeNullOrEmpty
$c = Get-Location
$fileName | Should -Exist
}
It "Can use Test-Path to check for a file via a long path" {
Test-Path $longFilePath | Should -BeTrue
}
}

And the error message is this:

PS:31> Remove-Item -Path $longFilePath
Remove-Item: The system cannot find the path specified.

Exception:

NativeErrorCode : 3
ErrorCode       : -2147467259
TargetSite      : Boolean IsReparsePointLikeSymlink(System.IO.FileSystemInfo)
StackTrace      :    at Microsoft.PowerShell.Commands.InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(Fil
                  eSystemInfo fileInfo) in System.Management.Automation.dll:token 0x60016f5+0x97
                     at Microsoft.PowerShell.Commands.RemoveItemCommand.ProcessRecord() in
                  Microsoft.PowerShell.Commands.Management.dll:token 0x60004dd+0x2e2
                     at System.Management.Automation.CommandProcessor.ProcessRecord() in
                  System.Management.Automation.dll:token 0x60020f6+0x1ae
Message         : The system cannot find the path specified.
Data            : {}
InnerException  :
HelpLink        :
Source          : System.Management.Automation
HResult         : -2147467259

The error is thrown here:

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

Lines 8248 to 8254 in dd5cf86

if (handle.IsInvalid)
{
// Our handle could be invalidated by something else touching the filesystem,
// so ensure we deal with that possibility here
int lastError = Marshal.GetLastWin32Error();
throw new Win32Exception(lastError);
}

It appears that using FindFirstFileExW, we need to:

  • Ensure the path exists before calling the Windows API
  • Ensure that long paths are converted to the long path format appropriately

It also seems that the long path tests to catch this did not in our daily builds and we need to investigate why.

GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@rjmholt rjmholt mentioned this pull request May 26, 2021
Closed
@iSazonov
Copy link
Collaborator Author

We've picked up a test failure during release testing that we think is caused by this change.

I wonder CIs was passed successfully.

@ghost
Copy link
ghost commented May 27, 2021

🎉v7.2.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 27, 2021
Closed
@iSazonov iSazonov mentioned this pull request May 31, 2021
Closed
rkeithhill
rkeithhill reviewed May 31, 2021
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Management/Remove-Item.Tests.ps1

Test-Path $testdirectory | Should -BeFalse
}
It "Should be able to recursively delete a directory with a trailing backslash" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this regression test!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really we have very small code coverage. I hope WGs start work and we add more tests to exclude regressions.

@rjmholt rjmholt added the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Jun 8, 2021
@rjmholt
Copy link
Collaborator
rjmholt commented Jun 8, 2021 •
edited
Loading

I discussed this with @daxian-dbw, @adityapatwardhan and @anmenaga in a maintainer review and rather than trying to fix this as is, we think the previous bug is easier to live with than the new bug and that we need to revert it. We can accept a full solution later.

rjmholt added a commit that referenced this pull request Jun 8, 2021
@rjmholt
Revert "Enhance Remove-Item to work with OneDrive (Second) (#15260)"
692c6f4 
This reverts commit 08e7313.
@rjmholt rjmholt mentioned this pull request Jun 8, 2021
Merged
anmenaga pushed a commit that referenced this pull request Jun 11, 2021
@rjmholt
Revert "Enhance Remove-Item to work with OneDrive (Second) (#15260)" (#…
0ae0b65 
…15546)
@iSazonov iSazonov mentioned this pull request Jun 13, 2021
Merged
22 tasks
@ghost
Copy link
ghost commented Jun 17, 2021

🎉v7.2.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

iSazonov added a commit to iSazonov/PowerShell that referenced this pull request Jul 27, 2021
@iSazonov
Enhance Remove-Item to work with OneDrive (Second) (PowerShell#15260)
622bd24
@adityapatwardhan adityapatwardhan removed the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkeithhill rkeithhill rkeithhill left review comments

@rjmholt rjmholt rjmholt approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

Assignees

@rjmholt rjmholt

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

On Windows, Remove-Item unexpectedly fails if the target directory path happens to end in a backslash
5 participants
@iSazonov @TravisEz13 @rjmholt @rkeithhill @adityapatwardhan
0