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

Bugfix: File deleted / emptied when moved / copied onto itself (#546) #547

Merged

Conversation

tfeldmann
Copy link
Contributor

@tfeldmann tfeldmann commented Jul 22, 2022

Type of changes

  • Bug fix
  • Tests

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 @PyFilesystem/maintainers may be pedantic in the code review.

Description

Draft PR for fixing #546.

@tfeldmann
Copy link
Contributor Author

tfeldmann commented Jul 22, 2022

My git fu is not so good. Only the latest commit is meant to be in this branch. I'll try to rebase.

EDIT: Should be fine now.

@tfeldmann tfeldmann force-pushed the fix/file-deleted-when-moved-onto-itself branch from c68718f to ec7c68d Compare July 22, 2022 10:28
@lurch
Copy link
Contributor

lurch commented Jul 22, 2022

Yes, it successfully fails 😆

@lurch lurch changed the title Bugfix: File deleted when moved onto itself (#546) Bugfix: File deleted / emptied when moved / copied onto itself (#546) Jul 22, 2022
fs/osfs.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 22, 2022

Coverage Status

Coverage increased (+0.02%) to 94.814% when pulling 420998d on tfeldmann:fix/file-deleted-when-moved-onto-itself into cd545b2 on PyFilesystem:master.

@tfeldmann tfeldmann marked this pull request as ready for review July 25, 2022 10:07
@tfeldmann
Copy link
Contributor Author

tfeldmann commented Jul 25, 2022

The github actions probably should have a timeout 😅 They need 6h to expire when there's a loop going wrong.
At least this passes locally but can be cleaned up a bit more.

@tfeldmann tfeldmann marked this pull request as draft July 25, 2022 15:48
@lurch
Copy link
Contributor

lurch commented Jul 26, 2022

The github actions probably should have a timeout

I'm sure nobody would mind if you set https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes to something sensible? 😃

(and I suspect #520 is probably why the whole test-suite takes so long to fail?)

@althonos
Copy link
Member

@lurch : Fine by me 👍

@tfeldmann tfeldmann mentioned this pull request Jul 27, 2022
4 tasks
@tfeldmann tfeldmann marked this pull request as ready for review July 27, 2022 09:40
fs/base.py Outdated Show resolved Hide resolved
@tfeldmann
Copy link
Contributor Author

Caveat: This only fixes moving / copying on the same FS instance. I'll try to tackle the problem with the same resources on different FSs in a separate PR.

@althonos
Copy link
Member

althonos commented Aug 2, 2022

Good job again @tfeldmann , apart from my one comment this is pretty solid and helps covering some undefined behaviour 👍 Looking forward to see how the plugins (fs.sshfs, fs-s3fs, etc.) fare with this change !

@althonos althonos merged commit 11ad1ec into PyFilesystem:master Aug 2, 2022
@althonos althonos added the bug label Aug 2, 2022
Comment on lines +1180 to +1182
if _src_path == _dst_path:
# early exit when moving a file onto itself
return
Copy link
Contributor

@lurch lurch Aug 2, 2022

Choose a reason for hiding this comment

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

This PR has been merged now, but I'm still not convinced that doing an early-exit here, instead of raising an exception, is the correct thing to do? Since trying to move a file onto itself is probably an indication of an application bug? Same comment applies to movedir too.

Copy link
Member

Choose a reason for hiding this comment

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

Now that you point it out, I agree. In UNIX filesystems, moving a file onto itself raises an error, so I think it's fair to expect an IllegalDestination there as well.

@tfeldmann tfeldmann deleted the fix/file-deleted-when-moved-onto-itself branch August 19, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants