Made `remove_dir_all` work as documented when directory is a symlink #29412

tailhook · 2015-10-27T21:19:07Z

Previously the function worked correctly only when symlink is inside of
a directory, not when path is a symlink itself.

This patch also eliminates the lstat() call on most systems because
DirEntry::file_type is enough both to detect if entry is a dir and if
entry is a symlink.

I'm not sure if the #[cfg(not(windows))] and the comment above is still actual. Can't test patch on windows.

This supercedes #29324

Previously the function worked correctly only when symlink is inside of a directory, not when path is a symlink itself. This patch also eliminates the `lstat()` call on most systems because `DirEntry::file_type` is enough both to detect if entry is a dir and if entry is a symlink.

rust-highfive · 2015-10-27T21:19:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eefriedman · 2015-10-27T21:58:41Z

src/libstd/fs.rs

@@ -1069,13 +1069,23 @@ pub fn remove_dir_all<P: AsRef<Path>>(path: P) -> io::Result<()> {
 }

 fn _remove_dir_all(path: &Path) -> io::Result<()> {
+    let filetype = try!(symlink_metadata(path)).file_type();
+    if filetype.is_symlink() {
+        remove_file(path)


Not sure this will work correctly on Windows; a symlink isn't always a file on Windows.

Well, the code that removed a symlink inside the directory used remove_file too, AFAICS. So at least it's not a regression.

Anyway, it would be helpful if someone can test it on windows.

I've confirmed that directory symlinks on windows cannot be removed by remove_file but can be removed via remove_dir.

I think that this may want to also use is_dir to choose whether to recurse or not (like below). Junctions on Windows (e.g. hard links to directories) don't report is_dir, but to behave the same as unix we should delete their contents.

I think that this may want to also use is_dir to choose whether to recurse or not (like below). Junctions on Windows (e.g. hard links to directories) don't report is_dir, but to behave the same as unix we should delete their contents.

Isn't it a contradiction? If they don't report is_dir how should we know to recurse?

The question probably is: is_dir() && is_symlink() (impossible on unix) should recurse or remove_dir() ?

Okay, it looks like impossible combination on windows too:
https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/fs.rs#L429

So probably I must stat symlink on windows to find out whether it's a directory. And then if it's a directory do (non-recursive) remove_dir. Right? According to wikipedia, junctions are removed with rmdir (i.e. non-recursive)

remove_dir() seems like the answer which is most consistent with other forms of symbolic links.

Hm yes I think I may have gotten a little confused, I'll have to think on this a bit.

nagisa · 2015-10-27T23:22:14Z

I’d like to see a test which tests whether function behaviour is as expected. A “make-run” test probably would be the least painful one to do.

alexcrichton · 2015-10-27T23:25:31Z

src/libstd/fs.rs

+        _remove_dir_all_unchecked(path)
+    }
+}
+fn _remove_dir_all_unchecked(path: &Path) -> io::Result<()> {


It's fine to remove the leading underscore here, the convention above is just because it's paired with a sibling remove_dir_all function.

Also this may be best with a _recursive prefix here instead of _unchecked

tailhook · 2015-10-27T23:30:44Z

I’d like to see a test which tests whether function behaviour is as expected.

There is a test, function called "recursive_rmdir_of_symlink"

A “make-run” test probably would be the least painful one to do.

Well, what is a "make-run" test?

nagisa · 2015-10-27T23:33:40Z

There is a test, function called "recursive_rmdir_of_symlink"

Ah, ignore me then. I should go to sleep instead of reviewing more PRs, I guess.

alexcrichton · 2015-10-27T23:45:38Z

src/libstd/fs.rs

-            try!(remove_dir_all(&*child));
+        let child = try!(child);
+        let child_path = child.path();
+        let mut child_type = try!(child.file_type());


This should fall back to calling symlink_metadata in the case file_type() failed (as it's not guaranteed to work across all platforms right now)

AFAICS, fallback is already inside DirEntry: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L186 (unix), on windows it looks like it always work.

Oh, right! I forgot about that, carry on!

* Renamed internal method `*_unchecked` -> `*_recursive` * Better windows support (untested)

tailhook · 2015-10-28T20:33:04Z

Okay, I've renamed the method and added some windows support. Unfortunately, windows support is untested. But I've included few provisional tests for windows. Hopefully @bors will run the tests for me.

Speaking about windows: I do metadata() for symlinks, to find out which function to use for delete. Unit tests are for symlinked files and directories. Junctions should use remove_dir, so should work too in theory.

retep998 · 2015-10-28T21:19:55Z

Just so you know, you cannot actually create any symbolic links in the tests because that requires administrator privileges on Windows for some obscure reason. Directory junctions and hard links are fine, just not symbolic links.

tailhook · 2015-10-28T21:30:11Z

Well, so I should remove the tests back again. @retep998, does std::fs::hard_link create a junction when destination is a directory? I may probably just replace symlinks into hard_links/junctions in tests.

retep998 · 2015-10-28T21:31:47Z

Hard links are for files only while junctions are for directories only. Also hard links are not reparse points.

tailhook · 2015-10-28T21:33:20Z

@retep998, any way to create a junction in rust?

retep998 · 2015-10-28T21:44:23Z

https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/fs.rs#L652-L690

alexcrichton · 2015-10-29T22:03:53Z

src/libstd/fs.rs

-            try!(remove_file(&*child));
+            if cfg!(windows) {
+                if child_type.is_symlink() {
+                    let target_type = try!(metadata(&*child_path)).file_type();


I tested this out a bit, and oddly enough I'm not sure we can use this function to determine this. Using metadata will resolve all intermediate symlinks, showing the file type of the actual destination file. This ends up having what I think are two problems:

If the symlink is broken (e.g. the destination is gone), I don't think this will work.

Removal of the symlink depends on what kind of symlink was created, not the destination itself.

I found that I could create a symlink to a file with symlink_dir, but I could only remove the symlink with remove_dir. I think that we may have enough information in the DirEntry itself to figure this out (via a private API for now).

I found that I could create a symlink to a file with symlink_dir,

That's because Windows doesn't check the target of symbolic links when you create them. You still created a directory symbolic link. It just happens to be pointing at a file.

steveklabnik · 2015-12-31T17:07:16Z

What's the status of this PR?

tailhook · 2015-12-31T17:38:20Z

Well, it works on linux. But it seems I don't have a time to work on windows stuff. And as I've already written I can't test anyway.

bors · 2016-02-04T20:57:47Z

☔ The latest upstream changes (presumably #31360) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2016-02-07T04:43:09Z

cc @pitdicker

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made `remove_dir_all` work as documented when directory is a symlink #29412

Made `remove_dir_all` work as documented when directory is a symlink #29412

tailhook commented Oct 27, 2015

rust-highfive commented Oct 27, 2015

eefriedman Oct 27, 2015

tailhook Oct 27, 2015

alexcrichton Oct 27, 2015

alexcrichton Oct 27, 2015

tailhook Oct 28, 2015

tailhook Oct 28, 2015

eefriedman Oct 28, 2015

alexcrichton Oct 29, 2015

nagisa commented Oct 27, 2015

alexcrichton Oct 27, 2015

alexcrichton Oct 27, 2015

tailhook commented Oct 27, 2015

nagisa commented Oct 27, 2015

alexcrichton Oct 27, 2015

tailhook Oct 28, 2015

alexcrichton Oct 28, 2015

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

alexcrichton Oct 29, 2015

retep998 Oct 29, 2015

steveklabnik commented Dec 31, 2015

tailhook commented Dec 31, 2015

bors commented Feb 4, 2016

alexcrichton commented Feb 7, 2016

pitdicker commented Feb 7, 2016

pitdicker commented Feb 7, 2016

tailhook commented Feb 7, 2016

Made remove_dir_all work as documented when directory is a symlink #29412

Made remove_dir_all work as documented when directory is a symlink #29412

Conversation

tailhook commented Oct 27, 2015

rust-highfive commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tailhook commented Oct 27, 2015

nagisa commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

tailhook commented Oct 28, 2015

retep998 commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Dec 31, 2015

tailhook commented Dec 31, 2015

bors commented Feb 4, 2016

alexcrichton commented Feb 7, 2016

pitdicker commented Feb 7, 2016

pitdicker commented Feb 7, 2016

tailhook commented Feb 7, 2016

Made `remove_dir_all` work as documented when directory is a symlink #29412

Made `remove_dir_all` work as documented when directory is a symlink #29412