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

Check path for hassyspath and use shutil.rmdir if True #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geoffjukes
Copy link
Contributor

@geoffjukes geoffjukes commented Sep 21, 2020

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review :)

Description

Minor enhancement to removetree when the path has a syspath and so can be removed via shutil.rmtree

@althonos althonos self-assigned this Sep 22, 2020
self.removedir(_path)
else:
self.remove(_path)
if _dir_path != "/":
Copy link
Member

Choose a reason for hiding this comment

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

I think you also need to check that _dir_path is not "/" with the shutil.rmtree branch too, otherwise you may end up deleting the root of the filesystem and potentially preventing new files/directories from being created in that filesystem afterwards.

if _dir_path != "/":
self.removedir(dir_path)
if self.hassyspath(path):
shutil.rmtree(self.getsyspath(path))
Copy link
Member

Choose a reason for hiding this comment

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

shutil.rmtree can potentially raise OSError, we would want to wrap it using the onerror callback, probably turning it into an fs.errors.OperationFailed.

@althonos
Copy link
Member

I think this is a good optimization, but we just need a few tests to make sure it is consistent with the current FS behaviour.

By the way, WrapReadOnly should be updated so that it makes the removetree method readonly, since this implementation does not use remove or removedir and could result in files being deleted even on a wrapped filesystem.

@althonos althonos mentioned this pull request Mar 26, 2021
4 tasks
@althonos althonos added this to the v2.5.0 milestone Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants