library: skip saving files with unchanged tags#6834
Conversation
There was a problem hiding this comment.
Pull request overview
grug see PR try stop useless file save when tags not really change. this help library sync tools not re-copy big files just because mtime bump, and make modify --write not touch disk when only DB-only field change.
Changes:
- Item.write now read relevant tags from file and skip
mediafile.save()when file already hold same values (unlessforce,id3v23, or images involved). - CLI
beet write --forcenow plumb toItem.try_sync(..., force_write=True)so force still rewrite. - add tests and docs/changelog notes for new “skip save” behavior and event semantics.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| beets/library/models.py | implement tag readback + compare in Item.write(), add force/force_write plumbing |
| beets/ui/commands/write.py | pass CLI --force through to force writing even when unchanged |
| test/test_library.py | add regression tests for “no write when same tags”, list-empty removal, unreadable image case |
| test/ui/commands/test_modify.py | add regression test that DB-only field change does not change file mtime |
| docs/reference/config.rst | document id3v23 implies always-saving behavior |
| docs/reference/cli.rst | document modify does not touch file mtime for DB-only changes |
| docs/dev/plugins/events.rst | update write event semantics (but one doc mismatch noted in comment) |
| docs/dev/library.rst | clarify mtime policy when Item.write() skips saving |
| docs/changelog.rst | add bugfix note + plugin-dev note about new behavior and force |
| :Description: Called before beets decides whether to write a file's | ||
| metadata to disk. Handlers may modify ``tags`` or raise | ||
| ``library.FileOperationError`` to abort. ``after_write`` is skipped | ||
| when the file being written is the item's own and already holds the | ||
| tags. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0ff492192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| compare = ( | ||
| not force | ||
| # Saving is what converts the tags to ID3v2.3, and the conversion | ||
| # loses what the older version cannot hold, so a file written that | ||
| # way is saved every time. `MediaFile` sets this for MP3s only. | ||
| and not mediafile.id3v23 | ||
| and path == self.path |
There was a problem hiding this comment.
When the config is left at the default id3v23: false, an MP3 that currently has ID3v2.3 tags is opened with its tags translated to v2.4 in memory, but save() is still the step that writes that v2.4 tag back to disk. With this condition, if the tag values already match the database, Item.write() returns before saving, so beet write no longer converts existing v2.3 files to beets' default v2.4 output unless the user happens to pass --force. The same conversion exception added for requested v2.3 writes needs to account for older MP3s being upgraded to v2.4 as well.
Useful? React with 👍 / 👎.
|
Check the CI failure and address existing comments, and I'll review after that! |
Item.write() saved the file whether or not the tags it writes would change anything, so `beet modify --write` gave a file a new modification time even when the field that changed was one only the database keeps, such as data_source. Compare the tags read back from the file with the ones about to be written, and skip the save when they match. The images are left out of the comparison, since a write that carries one replaces the whole list of them. A file written as ID3v2.3 is still saved every time, since saving is what converts its tags. Fixes beetbox#6529.
Reading an MP3 translates its tags to ID3v2.4 in memory, so a v2.3 file already holding the values to be written read back as unchanged, and the write skipped the save that would have converted it on disk. Check the on-disk tag version too, and skip the save only when no conversion is pending. Also cover the `write -f` wiring, the `id3v23` branch, and the `after_write` event with tests, list `path` among the `after_write` event parameters, which the event has been sending all along, and format the docs the way `docstrfmt` wants them.
A list tag whose values are all empty strings read back as no tag at all, the same as an absent one, so removing the field compared as unchanged and the file kept the tag. Only an empty list reads as no tag now. Empty values keep counting as values.
The tag comparison only applies to the item's own file. A write to any other path saves whether or not that file holds the tags, and no test held that in place.
`preserve_write_mtimes` pins a file's mtime from the `after_write` event, which a write that finds the tags already in the file no longer sends.
The skipped save recorded the file's mtime as of after the comparison, so a change landing in between looked already written and a later update would not read it. Record the mtime from before the tags are read instead, which leaves any later change newer than the database.
Also cover the race above with a test, register the event listener the way the other plugin tests do, and fix a test comment that described the image guard rather than the field selection the test proves.
f0ff492 to
1bfbb22
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6834 +/- ##
==========================================
+ Coverage 75.63% 75.65% +0.02%
==========================================
Files 163 163
Lines 21312 21333 +21
Branches 3360 3364 +4
==========================================
+ Hits 16119 16140 +21
Misses 4401 4401
Partials 792 792
🚀 New features to boost your workflow:
|
|
Rebased onto master and the docs check is fixed. Both bot comments were right, fixed in the new commits along with two more cases they led me to. |
The line ran a few characters short of the width the formatter uses, which the docs check caught.
Description
Fixes #6529.
Item.write()calledmediafile.save()whether or not the tags it was about to write differed from the ones already in the file.beet modify --writeon a field that only the database keeps,data_sourcefor instance, gave the file a new modification time without changing a single tag, and tools that sync the library copied it again for nothing.The tags the write is about to touch are now read back from the file, compared with the ones already there, and the save is skipped when they match.
Comparing what the file reads back, rather than what the database holds, keeps a file that cannot store a value exactly, such as one carrying a replaygain peak, from looking changed on every write. Images stay out of the comparison, since a write that carries one replaces the whole list of them.
beet write --forcestill rewrites. So does a file written with theid3v23option, and so does one whose tags still need converting to the default ID3v2.4, since saving is what converts them either way.To Do
modifycommand, theid3v23option, the mtime policy in the library docs, thewriteandafter_writeevents, and the importadded plugin.)