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

Mtime fixes #2667

Merged
merged 7 commits into from
Aug 25, 2017
Merged

Mtime fixes #2667

merged 7 commits into from
Aug 25, 2017

Conversation

wisp3rwind
Copy link
Member

As per #2664, the Item.mtime is reset for any assignment to MediaFile-backed fields, even if the Items fields and the file's tags do not go out of sync (i.e. if the previous value is reassigned). This happens frequently in the edit plugin. While the effects are not devastating, it's certainly not ideal when using the plugin with coarse queries and only changing few items in the editor.

In fact, test.test_edit already had a method to check that these superfluous tag writes didn't happen. It didn't really check this, though, since the mtime for test fixtures would always be 0 to begin with. An "mtime reset" would therefore not _dirty the Item, and the plugin would in turn not write the tags out: In a real-world setting with non-zero mtime, these writes did happen.

Some additional details can be found in the commit messages. Thoughts?

NOTE: Forgot the changelog, will add it later.

wordofglass added 5 commits August 22, 2017 23:24
mtime == 0 is the "this Item contains newer metadata than the file's
tags" marker. Setting this to something else than 0 emulates the state
of Items freshly read from the database.

Breaks 4 of the edit plugin tests.
…ehaviour

I was midway through writing a test when realizing that it's best
the way it is...
This didn't manifest as a testing failure since the plugin (mostly
silently) drops id changes.
… changes

Unbreaks the remaining edit plugin tests.
@mention-bot
Copy link

@wordofglass, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @wlof and @geigerzaehler to be potential reviewers.

@sampsyo
Copy link
Member

sampsyo commented Aug 23, 2017

Nice work! These test fixes look good as well. Fighting through the harnesses there wasn’t easy—you deserve some kind of a badge. 🏅

This looks ready to merge to me.

@wisp3rwind
Copy link
Member Author

Ok. Would you usually write a changelog entry for this? As far as I can see, there's no user-visible change (well, file access times will be updated less frequently which might affect some rsync, but this is pretty minor IMO).

@sampsyo
Copy link
Member

sampsyo commented Aug 24, 2017

In general, I do usually tend to write a changelog for even these esoteric things. I'm not sure anyone actually reads them, but it makes me happy to have a complete summary of what happened in a given release. 😃

Maybe something like this would work?

  • Fixed a problem where "no-op" modifications would reset files' mtimes, resulting in unnecessary writes. This most prominently affected the :doc:`/plugins/edit` when saving the text file without making changes to some music.

@wisp3rwind
Copy link
Member Author

I couldn't have worded that better, I'm going to blatantly copy that to the changelog 😆

@wisp3rwind wisp3rwind merged commit aa19577 into beetbox:master Aug 25, 2017
@wisp3rwind wisp3rwind deleted the mtime_fixes branch August 25, 2017 13:39
@sampsyo
Copy link
Member

sampsyo commented Aug 25, 2017

Fantastic; thank you!

@wisp3rwind
Copy link
Member Author

Ok, resolving merge conflicts on Github is apparently not the best idea. Sorry for the extra commit...

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.

3 participants