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

Be deterministic when saving to file #329

Merged
merged 3 commits into from Nov 13, 2017
Merged

Be deterministic when saving to file #329

merged 3 commits into from Nov 13, 2017

Conversation

@cushy007
Copy link
Contributor

cushy007 commented Nov 12, 2017

I am using Unison as a backup tool for my music files and every time I update my library's tags with Mutagen some of them are found modified although the audio part and the tags had not been modified. I found out that APEV2 tags are sorted by lengths before saving to file but this is not sufficient. Tags that have the exact same length are saved in random orders because of Python's dictionary implementation. This patch aims to correct this because it adds tags keys to the comparing function and since tag's keys are unique, this ensures that each sorting key is unique too.
The problem is that this is very difficult to test because of its randomness :(

@@ -459,7 +459,7 @@ def save(self, filething):
# "APE tags items should be sorted ascending by size... This is
# not a MUST, but STRONGLY recommended. Actually the items should
# be sorted by importance/byte, but this is not feasible."
tags.sort(key=len)
tags.sort(key=lambda tag: (len(tag), tag[8:256 + 8]))

This comment has been minimized.

Copy link
@lazka

lazka Nov 12, 2017

Member

Shouldn't (len(tag), tag) also work?

This comment has been minimized.

Copy link
@cushy007

cushy007 Nov 12, 2017

Author Contributor

Yes it works too. I have chosen the slice because :

  1. the tags will be ordered alphanumerically into the file (most people won't care anyway :)
  2. this avoids having to parse a possibly huge tag value
    But this may be less readable so it's up to you whether we should use the slice or not...

This comment has been minimized.

Copy link
@lazka

lazka Nov 12, 2017

Member

I'd prefer it without the slice

@lazka

This comment has been minimized.

Copy link
Member

lazka commented Nov 12, 2017

The problem is that this is very difficult to test because of its randomness :(

fyi, you can start python with the "-R" switch, which enables "hash randomization" and should change the order. (we use it on travis-ci by default)

@cushy007

This comment has been minimized.

Copy link
Contributor Author

cushy007 commented Nov 12, 2017

Great ! I did not know this option, I'l give it a try...

@lazka lazka merged commit 300c250 into quodlibet:master Nov 13, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka

This comment has been minimized.

Copy link
Member

lazka commented Nov 13, 2017

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.