Add support of RemoveDirectoryOptions::RemoveReadOnlyDirectory which remove read-only directories#617
Conversation
|
|
||
| // Fail if the directory does not have read-only set, likely just an ACL problem | ||
| DWORD directoryAttr = ::GetFileAttributesW(path.get()); | ||
| RETURN_LAST_ERROR_IF(!WI_IsFlagSet(directoryAttr, FILE_ATTRIBUTE_READONLY)); |
There was a problem hiding this comment.
Presumably GetLastError won't contain failure because you just called GetFileAttributesW right before this (which probably succeeded). And even if it did fail, you probably want the original error from the call to RemoveDirectoryW
There was a problem hiding this comment.
Followed the similar approach for file deletion:
Line 325 in 2edb790
| if (directoryAttr == 0) | ||
| { | ||
| directoryAttr = FILE_ATTRIBUTE_NORMAL; | ||
| } |
There was a problem hiding this comment.
Is this necessary? Presumably the attributes have FILE_ATTRIBUTE_DIRECTORY set at a minimum.
There was a problem hiding this comment.
Followed similar approach as when file is deleted:
Line 328 in 2edb790
Set FILE_ATTRIBUTE_DIRECTORY instead, looks like SetFileAttributesW accepts it for directory.
|
@ChrisGuzak and @jonwis for their thoughts. This jumps out as potentially concerning because it's changing behavior of something that an existing application could theoretically be depending on. The safest route would be to introduce a new flag - That said, looking around at existing uses of |
|
The impact is presumably limited to just the directories themselves, so all of the directory contents would still get deleted, so the impact from, say, a security perspective, seems low. |
|
Yeah, I like that additional flag for "... and your little readonly dirs, too!" mode. Thanks for the change. |
90f6268 to
03136d5
Compare
Closes #565