-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix error message during symlink deletion #11331
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.
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
Conversation
|
after looking at those linked items, I agree with @iSazonov . We need more info before we can safely merge this. Specifically:
|
I'm happy with that.
Well the answer to that seems to be "they require recurse set to delete". But I would like to know the full answer as well. |
|
But agreed. I think the bottom line is that if different symlinks behave differently, we need a way to differentiate them and get the right behaviour for each and this PR doesn't do that. I'll reduce it back to the error message fix. |
| // TODO: | ||
| // Different symlinks seem to vary by behavior. | ||
| // In particular, OneDrive symlinks won't remove without recurse, | ||
| // 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the comment too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just put it in in the last commit, since the current behaviour won't work for OneDrive symlinks (why I opened the PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to open a tracking issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about both? Comments like this often help me when I find them in the code, because I usually hit them when I'm trying to fix something that I didn't realise was related.
|
🎉 Handy links: |
PR Summary
I hit an issue trying to delete a directory under OneDrive like this:
Expanding the error revealed this:
The directory looked like this:
I've tried writing tests that look like this:
However, they don't fail like OneDrive symlinks do and I'm not sure how to emulate this behaviour.
This is a very simple fix though: the recurse parameter needed to be passed to
Delete()and the error message needed its second argument passed through.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.