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.

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

FileUtils#rm_r removes a symlink itself #2928

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

mosop
Copy link
Contributor

@mosop mosop commented Jun 29, 2016

This is for fix and improvement both.

If a directory have a symbolic link, FileUtils#rm_r fails with an error message like:

Unable to remove directory '/path/to/the/symlink': Not a directory

And if the symlink also points to a directory, rm_r also deletes all files under the directory. I feel it's a bit dangerous. The rm command and Ruby's FileUtils#rm_r remove a symlink itself instead.

Thank you!

@mosop mosop closed this Jun 30, 2016
@mosop mosop reopened this Jun 30, 2016
@ysbaddaden
Copy link
Contributor

Good catch! What about a distinct it "doesn't follow symlinks" spec?

@mosop mosop force-pushed the rm_r_removes_symlink_itself branch from 27f8744 to cf89ce4 Compare June 30, 2016 10:59
@mosop
Copy link
Contributor Author

mosop commented Jun 30, 2016

@ysbaddaden Thank you. How about my new spec?

@asterite asterite added this to the 0.18.7 milestone Jun 30, 2016
File.write(file_path, "")

FileUtils.rm_r(removed_path)
Dir.exists?(removed_path).should be_false
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to test that it removed a directory (the test above did that), just that the link itself got removed but not the destination folder nor its contents.

@mosop
Copy link
Contributor Author

mosop commented Jun 30, 2016

@ysbaddaden I can remove the line, but I think it is still needed to assert that rm_r successfully removes a directory even if it has symlinks. Because the older test is not asserting that. Confirm this again. Thanks.

@ysbaddaden
Copy link
Contributor

@mosop ok

@ysbaddaden ysbaddaden merged commit d440d97 into crystal-lang:master Jun 30, 2016
@mosop mosop deleted the rm_r_removes_symlink_itself branch July 1, 2016 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants