Enhance Remove-Item to work with OneDrive (Second)

[iSazonov]

PR Summary

Better to review commit by commit:

1. First commit comes from Enhance Remove-Item to work with OneDrive #14902
2. Second commit fixOn 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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[TravisEz13]

/azp run PowerShell-CI-Windows-daily

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[TravisEz13]

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

[ghost]

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]

/azp run PowerShell-CI-Windows-daily

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rjmholt]

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

[TravisEz13]

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

[rjmholt]

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
	intlastError=Marshal.GetLastWin32Error();
	thrownewWin32Exception(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/PowerShell

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

[iSazonov]

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]

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

Handy links:

• Release Notes

[rkeithhill]

Thanks for adding this regression test!

[iSazonov]

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

[rjmholt]

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.

[ghost]

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

Handy links:

• Release Notes