Fix FileSystemProvider to not treat all reparsepoints as files #14860
Conversation
SteveL-MSFT
requested review from
adityapatwardhan,
anmenaga and
daxian-dbw
as code owners
ghost
assigned
…ified is still treated as a file
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
The PR description does more confuse then explain. :-)
| { | ||
| System.IO.DirectoryInfo di = new(providerPath); | ||
| if (di != null && (di.Attributes & System.IO.FileAttributes.ReparsePoint) != 0) | ||
| if (di != null && di.Attributes.HasFlag(System.IO.FileAttributes.ReparsePoint) && (di.Attributes.HasFlag(System.IO.FileAttributes.Directory) && !shouldRecurse)) |
There was a problem hiding this comment.
I wonder why do we need the code at all if now FileSystemProvider correctly process all cases? (We can remove the code block from line 2696 to line 2713.)
There was a problem hiding this comment.
The code was added to make remove-item -literalpath * work for Registry provider. I'll add to the comment.
There was a problem hiding this comment.
Registry provider? In line 2697 we check FileSystemProvider.
There was a problem hiding this comment.
The code is relying on DirectoryInfo class to determine if the path is a directory and this would only work with FileSystemProvider so this code is skipped for other providers (like Registry Provider where it would fail). The way to think about this is if the path is FileSystemProvider path and a Directory, we do additional checks like reparse point to determine if the item should be treated as a file or not. For other providers, these API calls would fail so they are skipped.
| // but the .NET API here does not allow us to distinguish them. | ||
| // We may need to revisit using p/Invokes here to get the right behavior | ||
| directory.Delete(); | ||
| directory.Delete(recursive: recurse); |
There was a problem hiding this comment.
I'd add a comment that the API correctly works with non named surrogates like a directory on OneDrive. This could help code readers to understand the code path more deeply.
There was a problem hiding this comment.
Good point, will add a comment why this is needed.
iSazonov
added
the
CL-General
SteveL-MSFT
added
Breaking-Change
|
@PowerShell/powershell-committee reviewed this, there is a concern regarding breaking existing confirm behavior which prompts for every item, but ok with a minor break if |
SteveL-MSFT
added
Committee-Reviewed
|
This is that rare case when several problems intersected in one place and everyone was confused. :-) Compare: The screenshots explicitly say that there was a bug in .Net 3. Now the bug is fixed in .Net 5.0. How we work with OneDrive it is another issue. I think current fix is not right - we should change a behavior only for non-named surrogates - and this can not be a breaking change - it should be classified as new behavior or bug fix. Right fix is to use IsReparsePointWithTarget() and get the same behavior for OneDrive as for regular directory. |
|
@iSazonov do you want to submit a new PR based on your proposal and I'll close this one? |
|
@SteveL-MSFT Will do. |
3 participants
PR Summary
The FileSystemProvider treats all reparsepoints as symlinks (files) so that trying to recursively remove a directory from OneDrive fails as it won't recurse and thus fails because the directory is not empty. Fix is to don't treat reparse points as files if it is also a directory and also pass the recurse flag to the Directory.Remove() overload only if
-Forceis specified.The breaking change behavior is that if you use
-Recurse -Force -Confirmthen it will now only confirm the top level folder before removing compared to the previous behavior (for a non-Onedrive folder) which prompts to confirm for every item within the folder.Manually validated that removing nested non-empty folder on OneDrive works and also removing a directory symlink removes the symlink and not the target.
PR Context
Fix #9246
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.PSForceRemoveReparsePoint(which runs in a different PS Host).