Add support of RemoveDirectoryOptions::RemoveReadOnlyDirectory which remove read-only directories

[dimhotepus]

Closes #565

[dunhor]

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

[dimhotepus]

Followed the similar approach for file deletion:

wil/include/wil/filesystem.h

Line 325 in 2edb790

RETURN_LAST_ERROR_IF(!WI_IsFlagSet(fileAttr, FILE_ATTRIBUTE_READONLY));

[dunhor]

IMO that seems like a bug for that code path as well. potentiallyFixableReadOnlyProblem should logically be something more like:

                auto err = ::GetLastError();
                bool potentiallyFixableReadOnlyProblem =
                    (WI_IsFlagSet(options, RemoveDirectoryOptions::RemoveReadOnlyDirectory)) &&
                    (err == ERROR_ACCESS_DENIED) &&
                    WI_IsFlagSet(::GetFileAttributesW(path.get()), FILE_ATTRIBUTE_READONLY);
                if (!potentiallyFixableReadOnlyProblem)
                {
                    RETURN_WIN32(err);
                }

[dunhor]

Is this necessary? Presumably the attributes have FILE_ATTRIBUTE_DIRECTORY set at a minimum.

[dimhotepus]

Followed similar approach as when file is deleted:

wil/include/wil/filesystem.h

Line 328 in 2edb790

WI_ClearFlag(fileAttr, FILE_ATTRIBUTE_READONLY);

Set FILE_ATTRIBUTE_DIRECTORY instead, looks like SetFileAttributesW accepts it for directory.

[dunhor]

The difference here is that this is a directory - it must have FILE_ATTRIBUTE_DIRECTORY set. In contrast, FILE_ATTRIBUTE_NORMAL is only valid when used alone. If a file has FILE_ATTRIBUTE_READONLY set, it's valid for that to be the only flag set.

That said, this isn't harmful, just an unnecessary check.

[dunhor]

@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 - RemoveDirectoryOptions::RemoveReadOnlyDirectory - and possibly add a new RemoveDirectoryOptions::RemoveReadOnlyFile that aliases RemoveDirectoryOptions::RemoveReadOnly.

That said, looking around at existing uses of RemoveDirectoryOptions::RemoveReadOnly, there doesn't appear to be that many instances outside of test code, so this might just be okay.

[dunhor]

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.

[jonwis]

Yeah, I like that additional flag for "... and your little readonly dirs, too!" mode. Thanks for the change.