After dealing with annoying failures to remove directories on windows with std.file.rmdirRecurse for too long, I finally decided to look into it.

It looks like rmdirRecurse fails because I'm trying to remove a directory that contains a git repository in it.  It turns out that git marks some files as READONLY to indicate that they shouldn't be modified.  However, this will cause the DeleteFile function (which is what rmdirRecurse will eventually call) to fail.  The docs say that you must remove the READONLY attribute before the file can be deleted.

Note that deleting a directory/file from Windows Explorer or calling `del` from the command-line will remove READONLY files no problem.  I could create a pull request to modify std.file to handle this, however, I'm not sure what the right solution is.  We could modify std.file.remove to be able to remove readonly files, but maybe there's use cases where people want the removal to fail if it is marked as readonly?  Although, the counter-argument to this would be that the standard mechanisms to delete files ignore the READONLY attribute and delete them anyway, so maybe D's default behavior should match?

We could also enhance the API to allow the user to specify the behavior.  By either defining new functions like "removeForce/rmdirForceRecurse", or adding an options argument remove(file, options), rmdirRecurse(dir, options).

What do people think is the right solution here?

On Saturday, 7 March 2020 at 20:01:07 UTC, Jonathan Marler wrote:
> We could modify std.file.remove to be able to remove readonly files, but maybe there's use cases where people want the removal to fail if it is marked as readonly?

I always thought "readonly" was intended to protect from "modification" not from "deletion", so simply making the function consistent with the explorer would be the right thing to do.
It's like a "const" variable: you cannot change its value, but of course you should be able to free the memory it occupies if it goes out of scope or is otherwise detectable not used anymore.

On Saturday, 7 March 2020 at 20:01:07 UTC, Jonathan Marler wrote:
> After dealing with annoying failures to remove directories on windows with std.file.rmdirRecurse for too long, I finally decided to look into it.
>
> It looks like rmdirRecurse fails because I'm trying to remove a directory that contains a git repository in it.  It turns out that git marks some files as READONLY to indicate that they shouldn't be modified.  However, this will cause the DeleteFile function (which is what rmdirRecurse will eventually call) to fail.  The docs say that you must remove the READONLY attribute before the file can be deleted.

This sounds right to me.

> Note that deleting a directory/file from Windows Explorer or calling `del` from the command-line will remove READONLY files no problem.

I don't think this should affect what Phobos does, as the above are interfaces for humans and Phobos is an OS API wrapper. (Though, in the case of rmdirRecurse, it's not as clear because it is an utility function.)

>  I could create a pull request to modify std.file to handle this, however, I'm not sure what the right solution is.  We could modify std.file.remove to be able to remove readonly files, but maybe there's use cases where people want the removal to fail if it is marked as readonly?

I think the function should do no more than the OS primitive API.

As far as I can see, the steps to create such a file from with in D would be:

1. Create the file.
2. Mark it read-only.

The inverse operation, again from D, would then be:

2. Unmark it as read-only.
1. Delete the file.

No problem here.

If you begin to do "magic" and "do what I mean" things, you open yourself up to questions like "why only the readonly attribute? why not also the ACL lists? and what about other platforms?" and it goes downhill from there. One similar example of this is that Tango's initial directory iteration primitive skipped hidden and system files (because that's what the shell does, duh), which as you can imagine caused a lot of pain should one such file ever come across your program.

I think the correct solution is to make sure that it is easy to recursively delete a directory (while clearing the readonly attribute if that's what the user wants) using only Phobos directory iteration primitives (dirEntries). If it's not then that is where the improvements ought to be done.

The current dirEntries API is definitely suboptimal in that error handling is difficult if you want to skip parts of the tree that fail to iterate, and that you can't selectively choose which directories to recurse into. I tried to address these with an alternative approach in my library (https://github.com/CyberShadow/ae/blob/6177cd442d9737b8e8141e30197fe124c5aaba18/sys/file.d#L176), which is also many times faster than dirEntries in many cases due to allocating less and not needing to stat unless absolutely necessary.

I also have a few "improved" rmdirRecurse variants which solve this problem as well:

https://github.com/CyberShadow/ae/blob/6177cd442d9737b8e8141e30197fe124c5aaba18/sys/file.d#L1002

However, I don't think they are suitable for Phobos, because there is just too much variation in what exactly "recursively delete a directory" should mean - so, in your case, a variant of rmdirRecurse which now works on git directories will fail due to ACLs, or not do what the user expects if the given path is a symlink, or other of many such questions.
Therefore, I think we should ensure that the primitives are there and easily accessible and programs should build their own utility functions to delete directory trees with whatever properties they need to care about.

On Saturday, 7 March 2020 at 20:01:07 UTC, Jonathan Marler wrote:
> Note that deleting a directory/file from Windows Explorer or calling `del` from the command-line will remove READONLY files no problem.

del gives me "Access denied" error.

On Saturday, 7 March 2020 at 22:51:46 UTC, Dominikus Dittes Scherkl wrote:
> On Saturday, 7 March 2020 at 20:01:07 UTC, Jonathan Marler wrote:
>> We could modify std.file.remove to be able to remove readonly files, but maybe there's use cases where people want the removal to fail if it is marked as readonly?
>
> I always thought "readonly" was intended to protect from "modification" not from "deletion", so simply making the function consistent with the explorer would be the right thing to do.
> It's like a "const" variable: you cannot change its value, but of course you should be able to free the memory it occupies if it goes out of scope or is otherwise detectable not used anymore.

Microsoft says [1]
FILE_ATTRIBUTE_READONLY
A file that is read-only. Applications can read the file, but cannot write to it or delete it. This attribute is not honored on directories.

So the behaviour of del or the explorer is violating theur own definition.


[1]: https://docs.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants

On Sunday, 8 March 2020 at 09:04:34 UTC, Patrick Schluter wrote:
> [...]
>
> So the behaviour of del or the explorer is violating theur own definition.

Such inconsistencies are sadly fairly common in Win32 and other
Microsoft docs.

On Saturday, 7 March 2020 at 20:01:07 UTC, Jonathan Marler wrote:
> After dealing with annoying failures to remove directories on windows with std.file.rmdirRecurse for too long, I finally decided to look into it.
>
> It looks like rmdirRecurse fails because I'm trying to remove a directory that contains a git repository in it.  It turns out that git marks some files as READONLY to indicate that they shouldn't be modified.  However, this will cause the DeleteFile function (which is what rmdirRecurse will eventually call) to fail.  The docs say that you must remove the READONLY attribute before the file can be deleted.
>
> Note that deleting a directory/file from Windows Explorer or calling `del` from the command-line will remove READONLY files no problem.  I could create a pull request to modify std.file to handle this, however, I'm not sure what the right solution is.  We could modify std.file.remove to be able to remove readonly files, but maybe there's use cases where people want the removal to fail if it is marked as readonly?  Although, the counter-argument to this would be that the standard mechanisms to delete files ignore the READONLY attribute and delete them anyway, so maybe D's default behavior should match?
>
> We could also enhance the API to allow the user to specify the behavior.  By either defining new functions like "removeForce/rmdirForceRecurse", or adding an options argument remove(file, options), rmdirRecurse(dir, options).
>
> What do people think is the right solution here?

I made a package for this solving this exact issue I had too with the .git folder!

https://code.dlang.org/packages/rm-rf
https://github.com/WebFreak001/rm-rf/blob/master/source/rm/rf.d

It's public domain so you can just look over the few lines it has and decide if you want to copy paste the file into your project or depend on it on dub :p

On Sunday, 8 March 2020 at 16:38:37 UTC, WebFreak001 wrote:
.
.
.

>
> I made a package for this solving this exact issue I had too with the .git folder!
>
> https://code.dlang.org/packages/rm-rf
> https://github.com/WebFreak001/rm-rf/blob/master/source/rm/rf.d
>
> It's public domain so you can just look over the few lines it has and decide if you want to copy paste the file into your project or depend on it on dub :p

Hmm... How about https://code.dlang.org/packages/scriptlike? That's were I would have looked for such an "extension"..

On Sunday, 8 March 2020 at 09:04:34 UTC, Patrick Schluter wrote:
> Microsoft says [1]
> FILE_ATTRIBUTE_READONLY
> A file that is read-only. Applications can read the file, but cannot write to it or delete it. This attribute is not honored on directories.
>
> So the behaviour of del or the explorer is violating theur own definition.
>
>
> [1]: https://docs.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants

Perhaps a small addendum to rmdirRecurse's documentation concerning this behavior would be appropriate?

On 3/7/2020 12:01 PM, Jonathan Marler wrote:
> We could modify std.file.remove to be able to remove readonly files,

This is definitely the wrong approach for the default behavior.

It should fail and the app then remove the readonly attribute and try again, or the remove() should have a "force" non-default option.