Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9046454
Made `remove_dir_all` work as documented when directory is a symlink
tailhook Oct 27, 2015
d957c94
The `remove_dir_all` fixes recommended in pull request
tailhook Oct 28, 2015
6f467c7
Add unit tests for remove_dir_all on windows
tailhook Oct 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
The remove_dir_all fixes recommended in pull request 
* Renamed internal method `*_unchecked` -> `*_recursive`
* Better windows support (untested)
  • Loading branch information
@tailhook
tailhook committed Oct 28, 2015
commit d957c94b77ea7baaab287e13b27bed4e4fdc0a20
34 changes: 27 additions & 7 deletions src/libstd/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,21 +1071,41 @@ 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)
if cfg!(windows) {
// On Windows symlinks to files and directories removed differently
// we should remove only directories here and have error on file
remove_dir(path)
} else {
// On unix symlinks are always files
remove_file(path)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be good to extract into a helper function to be shared with below, something like:

fn remove(path: &Path, meta: &fs::Metadata) -> io::Result<()> {
    // ...
}

} else {
_remove_dir_all_unchecked(path)
remove_dir_all_recursive(path)
}
}
fn _remove_dir_all_unchecked(path: &Path) -> io::Result<()> {
fn remove_dir_all_recursive(path: &Path) -> io::Result<()> {
for child in try!(read_dir(path)) {
let child = try!(child);
let child_path = child.path();
let mut child_type = try!(child.file_type());
let child_type = try!(child.file_type());
if child_type.is_dir() {
try!(_remove_dir_all_unchecked(&*child_path));
try!(remove_dir_all_recursive(&*child_path));
} else {
// The FileType::is_dir() is false for symlinks too
try!(remove_file(&*child_path));
if cfg!(windows) {
if child_type.is_symlink() {
let target_type = try!(metadata(&*child_path)).file_type();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If the symlink is broken (e.g. the destination is gone), I don't think this will work.
  2. 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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if target_type.is_dir() {
try!(remove_dir(&*child_path));
} else {
try!(remove_file(&*child_path));
}
} else {
try!(remove_file(&*child_path));
}
} else {
// The FileType::is_dir() is false for symlinks too
try!(remove_file(&*child_path));
}
}
}
remove_dir(path)
Expand Down