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

PICARD-972: Avoid save if file removed #609

Merged

Conversation

Sophist-UK
Copy link
Contributor

Handles both removal before save starts and removal whilst file is being
saved.

Resolves PICARD-972.

Handles both removal before save starts and removal whilst file is being
saved.
picard/file.py Outdated
@@ -222,6 +225,9 @@ def _rmdir(dir):
raise OSError

def _saving_finished(self, result=None, error=None):
# Handle file removed
if self.state == File.REMOVED and not result:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the previous comment, please explain and not result

@@ -247,8 +253,9 @@ def _saving_finished(self, result=None, error=None):
self.clear_pending()
self._add_path_to_metadata(self.orig_metadata)

del self.tagger.files[old_filename]
self.tagger.files[new_filename] = self
if self.state != File.REMOVED:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we are checking here again? Didn't we already return earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - we get here if file was removed during the save - and we only add the file to the tagger if it wasn't removed.

@Sophist-UK
Copy link
Contributor Author

I still need to test this latest incarnation, but providing it is working OK it will be ready to merge.

@Sophist-UK
Copy link
Contributor Author

I still need to test this - so please wait on the merge until I do so later today.

@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Feb 2, 2017

Ok - I have tested this - and the functionality works,fine. So it is ready for merge.

But it has identified another bug which is that the log.debug messages are not flushed to stderr before the executable quits.

@zas zas merged commit 12f4aed into metabrainz:master Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants