From 4d354c7799fb77c67dd8cd11b276768399efd533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Mon, 13 Jul 2026 17:50:33 +0300 Subject: [PATCH 1/8] library: skip saving files with unchanged tags 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 #6529. --- beets/library/models.py | 78 ++++++++++++++++++++++++++++++--- beets/ui/commands/write.py | 2 +- docs/changelog.rst | 15 +++++-- docs/dev/library.rst | 5 ++- docs/dev/plugins/events.rst | 8 ++-- docs/reference/cli.rst | 2 + docs/reference/config.rst | 4 ++ test/test_library.py | 48 ++++++++++++++++++++ test/ui/commands/test_modify.py | 20 +++++++++ 9 files changed, 167 insertions(+), 15 deletions(-) diff --git a/beets/library/models.py b/beets/library/models.py index 7d81720142..3f90226d52 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -931,11 +931,41 @@ def read(self, read_path: util.PathLike | None = None) -> None: self.path = read_path + @staticmethod + def _read_tags( + mediafile: MediaFile, fields: Iterable[str] + ) -> dict[str, Any]: + """Read `fields` back from `mediafile`. + + Reading the values back is what makes them comparable with the ones + about to be written, since a file only keeps what it can represent. It + rounds `rg_track_peak` to six decimal places, for instance. + """ + + def value(field: str) -> Any: + value = getattr(mediafile, field) + if isinstance(value, list): + # An empty list reads back as `[""]` from some formats. Keep + # the values themselves, so that dropping an empty one shows. + return value if any(v != "" for v in value) else None + + if isinstance(value, float): + # A gain of zero decibels is a value, not the lack of one. + return value + + # Formats differ on whether they keep a tag holding the null of its + # type, an empty string or a zero. Report it as no tag at all. + return value or None + + return {f: value(f) for f in fields} + def write( self, path: bytes | None = None, tags: Mapping[str, Any] | None = None, id3v23: bool | None = None, + *, + force: bool = False, ) -> None: """Write the item's metadata to a media file. @@ -951,6 +981,10 @@ def write( `id3v23` will override the global `id3v23` config option if it is set to something other than `None`. + Unless `force` is set, the item's own file is not saved when reading it + back already gives the tags to be written. Saving it would give it a new + mtime without changing any of its tags. + Can raise either a `ReadError` or a `WriteError`. """ if path is None: @@ -976,8 +1010,32 @@ def write( except UnreadableFileError as exc: raise ReadError(path, exc) + # The tags the update below is going to write. Writing an image + # replaces the whole list of them, which none of the fields here would + # show, so a write that carries one is saved without comparing anything. + written = set(item_tags) & set(mediafile.fields()) + 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 + and written.isdisjoint(("art", "images")) + ) + old_tags = self._read_tags(mediafile, written) if compare else None + # Write the tags to the file. mediafile.update(item_tags) + + if compare and self._read_tags(mediafile, written) == old_tags: + # The file already holds these tags. Saving it would only give it a + # new mtime, which makes tools that sync the library copy the file + # again for nothing. + log.debug("no tags to write to {.filepath}", self) + self.mtime = self.current_mtime() + return + try: mediafile.save() except UnreadableFileError as exc: @@ -993,6 +1051,8 @@ def try_write( path: bytes | None = None, tags: Mapping[str, Any] | None = None, id3v23: bool | None = None, + *, + force: bool = False, ) -> bool: """Call `write()` but catch and log `FileOperationError` exceptions. @@ -1000,28 +1060,34 @@ def try_write( Return `False` an exception was caught and `True` otherwise. """ try: - self.write(path=path, tags=tags, id3v23=id3v23) + self.write(path=path, tags=tags, id3v23=id3v23, force=force) return True except FileOperationError as exc: log.error("{}", exc) return False def try_sync( - self, write: bool, move: bool, with_album: bool = True + self, + write: bool, + move: bool, + with_album: bool = True, + *, + force_write: bool = False, ) -> None: """Synchronize the item with the database and, possibly, update its tags on disk and its path (by moving the file). - `write` indicates whether to write new tags into the file. Similarly, - `move` controls whether the path should be updated. In the - latter case, files are *only* moved when they are inside their + `write` indicates whether to write new tags into the file, and + `force_write` whether to do so even when the file already contains + them. Similarly, `move` controls whether the path should be updated. + In the latter case, files are *only* moved when they are inside their library's directory (if any). Similar to calling :meth:`write`, :meth:`move`, and :meth:`store` (conditionally). """ if write: - self.try_write() + self.try_write(force=force_write) if move: # Check whether this file is inside the library directory. if self._db and self._db.directory in util.ancestry(self.path): diff --git a/beets/ui/commands/write.py b/beets/ui/commands/write.py index 87fba8236e..e3a388fdb3 100644 --- a/beets/ui/commands/write.py +++ b/beets/ui/commands/write.py @@ -37,7 +37,7 @@ def write_items(lib, query, pretend, force): if (changed or force) and not pretend: # We use `try_sync` here to keep the mtime up to date in the # database. - item.try_sync(True, False) + item.try_sync(True, False, force_write=force) def write_func(lib, opts, args): diff --git a/docs/changelog.rst b/docs/changelog.rst index 096a35a6db..cff093194a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -95,10 +95,19 @@ Bug fixes valid date/time string" error instead of crashing with an uncaught ``KeyError``. A ``|`` was being accepted as a relative-date unit due to a regular expression character-class typo. +- ``beet modify --write`` no longer re-saves a file that already holds the tags + being written, so changing a field only the database keeps, such as + ``data_source``, no longer gives the file a new modification time. Files + written with the ``id3v23`` option are still saved every time, since saving is + what converts their tags. :bug:`6529` -.. - For plugin developers - ~~~~~~~~~~~~~~~~~~~~~ +For plugin developers +~~~~~~~~~~~~~~~~~~~~~ + +- ``Item.write()`` no longer saves the item's own file, and no longer sends the + ``after_write`` event, when the file already holds the tags being written. + Pass ``force=True`` (or ``Item.try_sync(..., force_write=True)``) to save it + regardless. Other changes ~~~~~~~~~~~~~ diff --git a/docs/dev/library.rst b/docs/dev/library.rst index 1ce22df08b..bc8cac466f 100644 --- a/docs/dev/library.rst +++ b/docs/dev/library.rst @@ -115,8 +115,9 @@ When it is possible that the metadata is out of sync, beets can then just set This leads to the following implementation policy: - - On every write of disk metadata (``Item.write()``), the DB mtime is - updated to match the post-write disk mtime. + - On every call to ``Item.write()`` for the item's own file, the DB mtime is + updated to match the disk mtime. This includes the case where the file + already holds the tags and is therefore left alone. - Same for metadata reads (``Item.read()``). - On every modification to DB metadata (``item.field = ...``), the DB mtime is reset to zero. diff --git a/docs/dev/plugins/events.rst b/docs/dev/plugins/events.rst index ace7b93874..157a80cdd1 100644 --- a/docs/dev/plugins/events.rst +++ b/docs/dev/plugins/events.rst @@ -111,9 +111,11 @@ registration process in this case: ``write`` :Parameters: ``item`` (|Item|), ``path`` (path), ``tags`` (dict) - :Description: Called just before a file's metadata is written to disk. - Handlers may modify ``tags`` or raise ``library.FileOperationError`` to - abort. + :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. ``after_write`` :Parameters: ``item`` (|Item|) diff --git a/docs/reference/cli.rst b/docs/reference/cli.rst index 5a801cdcb9..178685046c 100644 --- a/docs/reference/cli.rst +++ b/docs/reference/cli.rst @@ -309,6 +309,8 @@ Items will automatically be moved around when necessary if they're in your library directory, but you can disable that with ``-M``. Tags will be written to the files according to the settings you have for imports, but these can be overridden with ``-w`` (write tags, the default) and ``-W`` (don't write tags). +A file that already holds the tags is left untouched, so changing a field that +beets keeps only in its database does not give the file a new modification time. When you run the ``modify`` command, it prints a list of all affected items in the library and asks for your permission before making any changes. You can then diff --git a/docs/reference/config.rst b/docs/reference/config.rst index f434ba8028..e1f6ef121e 100644 --- a/docs/reference/config.rst +++ b/docs/reference/config.rst @@ -420,6 +420,10 @@ By default, beets writes MP3 tags using the ID3v2.4 standard, the latest version of ID3. Enable this option to instead use the older ID3v2.3 standard, which is preferred by certain older software such as Windows Media Player. +Saving a file is what converts its tags to ID3v2.3, so beets saves your MP3s +every time it writes them while this option is enabled, even the ones that +already hold the tags it is writing. + .. _va_name: va_name diff --git a/test/test_library.py b/test/test_library.py index b22cc45ecf..acc2e74e4f 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -10,6 +10,7 @@ import unittest from unittest.mock import patch +import mutagen import pytest from mediafile import MediaFile, UnreadableFileError @@ -1309,6 +1310,53 @@ def test_write_date_field(self): item.write() assert MediaFile(syspath(item.path)).year == clean_year + @pytest.mark.parametrize("file_format", ["MP3", "FLAC"]) + def test_no_write_when_file_has_the_tags(self, file_format): + item = self.add_item_fixture(format=file_format) + # The file keeps six decimal places of the peak, so the value read + # back from it never equals the one stored in the database. + item.rg_track_peak = 10 ** (-1.2 / 20) + item.write() + os.utime(syspath(item.path), (1000000000, 1000000000)) + + item.write() + + assert item.current_mtime() == 1000000000 + + def test_write_list_tag_that_drops_an_empty_value(self): + # An empty value the file happens to hold is still a value there, so + # the file no longer holds the tags once it is gone. + item = self.add_item_fixture(format="FLAC") + item.write() + mediafile = MediaFile(syspath(item.path)) + mediafile.artists = ["the artist", "", "another artist"] + mediafile.save() + item.artists = ["the artist", "another artist"] + os.utime(syspath(item.path), (1000000000, 1000000000)) + + item.write() + + assert item.current_mtime() != 1000000000 + assert MediaFile(syspath(item.path)).artists == [ + "the artist", + "another artist", + ] + + def test_write_file_with_an_unreadable_image(self): + # The images are not read, so a file beets cannot parse one from is + # written like any other. + path = os.path.join(self.temp_dir, b"unreadable_image.ogg") + shutil.copy(os.path.join(_common.RSRC, b"full.ogg"), path) + mediafile = mutagen.File(syspath(path)) + mediafile["metadata_block_picture"] = ["not base64"] + mediafile.save() + item = beets.library.Item.from_path(path) + item.title = "another title" + + item.write() + + assert MediaFile(syspath(path)).title == "another title" + class TestItemRead(PytestItemHelper): def test_unreadable_raise_read_error(self, item_in_db): diff --git a/test/ui/commands/test_modify.py b/test/ui/commands/test_modify.py index a74f08a44a..2ff4638be4 100644 --- a/test/ui/commands/test_modify.py +++ b/test/ui/commands/test_modify.py @@ -1,3 +1,5 @@ +import os + import pytest from mediafile import MediaFile @@ -240,6 +242,24 @@ def test_arg_parsing_equals_in_value(self): assert mods == {"title": ModifyOperation(None, "newTitle")} +class TestWriteTags(ModifyHelper, TestHelper): + @pytest.fixture + def item(self): + album = self.add_album_fixture() + [item] = album.items() + item.write() + item.store() + os.utime(syspath(item.path), (1000000000, 1000000000)) + return item + + def test_keep_file_when_only_database_field_changes(self, item): + self.modify("--nomove", "data_source=bandcamp") + + item.load() + assert item.data_source == "bandcamp" + assert item.current_mtime() == 1000000000 + + class TestMultiValue(ModifyHelper, TestHelper): @pytest.fixture def item(self): From 67b525e2985ac503203f62441adbd709b94ef7e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 17:05:29 +0300 Subject: [PATCH 2/8] library: save files whose tags still need ID3 conversion 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. --- beets/library/models.py | 8 +++++++- docs/changelog.rst | 12 +++++++----- docs/dev/plugins/events.rst | 14 +++++++------- test/test_library.py | 29 +++++++++++++++++++++++++++-- test/test_plugins.py | 20 ++++++++++++++++++++ test/ui/commands/test_write.py | 12 ++++++++++++ 6 files changed, 80 insertions(+), 15 deletions(-) diff --git a/beets/library/models.py b/beets/library/models.py index 3f90226d52..99d6517e9f 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -983,7 +983,9 @@ def write( Unless `force` is set, the item's own file is not saved when reading it back already gives the tags to be written. Saving it would give it a new - mtime without changing any of its tags. + mtime without changing any of its tags. A file whose tags still need + converting to another ID3 version is saved either way, since saving is + what converts them. Can raise either a `ReadError` or a `WriteError`. """ @@ -1020,6 +1022,10 @@ def write( # 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 + # Saving is also what converts older ID3 tags to the default + # v2.4. Reading translates them in memory, and `MediaFile` does + # not expose the version the file holds, hence mutagen's. + and getattr(mediafile.mgfile.tags, "version", (2, 4)) >= (2, 4) and path == self.path and written.isdisjoint(("art", "images")) ) diff --git a/docs/changelog.rst b/docs/changelog.rst index cff093194a..fc8ac91864 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -98,16 +98,18 @@ Bug fixes - ``beet modify --write`` no longer re-saves a file that already holds the tags being written, so changing a field only the database keeps, such as ``data_source``, no longer gives the file a new modification time. Files - written with the ``id3v23`` option are still saved every time, since saving is - what converts their tags. :bug:`6529` + written with the ``id3v23`` option are still saved every time, and so are + files whose tags have yet to be converted to the default ID3v2.4, since saving + is what converts them. :bug:`6529` For plugin developers ~~~~~~~~~~~~~~~~~~~~~ - ``Item.write()`` no longer saves the item's own file, and no longer sends the - ``after_write`` event, when the file already holds the tags being written. - Pass ``force=True`` (or ``Item.try_sync(..., force_write=True)``) to save it - regardless. + ``after_write`` event, when the file already holds the tags being written. A + file whose tags still need converting between ID3 versions is saved either + way. Pass ``force=True`` (or ``Item.try_sync(..., force_write=True)``) to save + it regardless. Other changes ~~~~~~~~~~~~~ diff --git a/docs/dev/plugins/events.rst b/docs/dev/plugins/events.rst index 157a80cdd1..752fb50f28 100644 --- a/docs/dev/plugins/events.rst +++ b/docs/dev/plugins/events.rst @@ -111,15 +111,15 @@ registration process in this case: ``write`` :Parameters: ``item`` (|Item|), ``path`` (path), ``tags`` (dict) - :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. + :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`` - :Parameters: ``item`` (|Item|) - :Description: Called after a file's metadata is written to disk. + :Parameters: ``item`` (|Item|), ``path`` (path) + :Description: Called after a file's metadata is written to disk. Not sent + when the write was skipped because the item's own file already held the + tags (see ``Item.write()``). ``import_task_created`` :Parameters: ``task`` (|ImportTask|), ``session`` (|ImportSession|) diff --git a/test/test_library.py b/test/test_library.py index acc2e74e4f..7160a68fdb 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -1323,6 +1323,31 @@ def test_no_write_when_file_has_the_tags(self, file_format): assert item.current_mtime() == 1000000000 + def test_write_converts_id3v23_file_to_v24(self): + # Saving is what converts the tags of an older MP3 to the default + # ID3v2.4, so a v2.3 file is saved even when it holds the tags. + item = self.add_item_fixture(format="MP3") + item.write() + mediafile = mutagen.File(syspath(item.path)) + mediafile.tags.update_to_v23() + mediafile.save(v2_version=3) + item.read() + + item.write() + + assert mutagen.File(syspath(item.path)).tags.version >= (2, 4, 0) + + def test_id3v23_write_saves_file_with_the_tags(self): + # Converting the tags to ID3v2.3 also happens when the file is saved, + # so a write with the option enabled never skips the save. + item = self.add_item_fixture(format="MP3") + item.write() + os.utime(syspath(item.path), (1000000000, 1000000000)) + + item.write(id3v23=True) + + assert item.current_mtime() != 1000000000 + def test_write_list_tag_that_drops_an_empty_value(self): # An empty value the file happens to hold is still a value there, so # the file no longer holds the tags once it is gone. @@ -1343,8 +1368,8 @@ def test_write_list_tag_that_drops_an_empty_value(self): ] def test_write_file_with_an_unreadable_image(self): - # The images are not read, so a file beets cannot parse one from is - # written like any other. + # Images are not compared, so an unreadable one does not stop the + # write. path = os.path.join(self.temp_dir, b"unreadable_image.ogg") shutil.copy(os.path.join(_common.RSRC, b"full.ogg"), path) mediafile = mutagen.File(syspath(path)) diff --git a/test/test_plugins.py b/test/test_plugins.py index 1fa658c61d..a129decba8 100644 --- a/test/test_plugins.py +++ b/test/test_plugins.py @@ -69,6 +69,26 @@ def test_listener_registered(self): assert MediaFile(syspath(item.path)).artist == "YYY" + def test_after_write_skipped_when_file_has_the_tags(self): + events = [] + + class EventPlugin(plugins.BeetsPlugin): + def __init__(self): + super().__init__() + self.register_listener( + "after_write", lambda item, path: events.append(path) + ) + + self.register_plugin(EventPlugin) + item = self.add_item_fixture() + item.write() + assert events + + events.clear() + item.write() + + assert not events + def test_multi_value_flex_field_type(self): item = Item(path="apath", artist="aaa") item.multi_value = ["one", "two", "three"] diff --git a/test/ui/commands/test_write.py b/test/ui/commands/test_write.py index 7197cbded0..12e80869f8 100644 --- a/test/ui/commands/test_write.py +++ b/test/ui/commands/test_write.py @@ -1,10 +1,22 @@ +import os + from beets.test.helper import BeetsTestCase, IOMixin +from beets.util import syspath class WriteTest(IOMixin, BeetsTestCase): def write_cmd(self, *args): return self.run_with_output("write", *args) + def test_force_write_file_with_the_tags(self): + item = self.add_item_fixture() + item.write() + os.utime(syspath(item.path), (1000000000, 1000000000)) + + self.write_cmd("-f") + + assert item.current_mtime() != 1000000000 + def test_update_mtime(self): item = self.add_item_fixture() item["title"] = "a new title" From 0d33f8aeaab7614d163c843e27ca0d807a9a62b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 17:43:05 +0300 Subject: [PATCH 3/8] library: save the removal of a tag holding only empty values 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. --- beets/library/models.py | 7 ++++--- test/test_library.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/beets/library/models.py b/beets/library/models.py index 99d6517e9f..ddca8cf4aa 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -945,9 +945,10 @@ def _read_tags( def value(field: str) -> Any: value = getattr(mediafile, field) if isinstance(value, list): - # An empty list reads back as `[""]` from some formats. Keep - # the values themselves, so that dropping an empty one shows. - return value if any(v != "" for v in value) else None + # An absent list of tags reads back as an empty one from some + # formats. The values themselves are kept, empty ones + # included, so that dropping one shows. + return value or None if isinstance(value, float): # A gain of zero decibels is a value, not the lack of one. diff --git a/test/test_library.py b/test/test_library.py index 7160a68fdb..e8d85f9612 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -1367,6 +1367,21 @@ def test_write_list_tag_that_drops_an_empty_value(self): "another artist", ] + def test_write_drops_a_list_tag_of_empty_values(self): + # A tag holding only empty values is still a tag the file keeps, so + # removing it is a change to save. + item = self.add_item_fixture(format="FLAC") + item.write() + mediafile = MediaFile(syspath(item.path)) + mediafile.artists = [""] + mediafile.save() + os.utime(syspath(item.path), (1000000000, 1000000000)) + + item.write() + + assert item.current_mtime() != 1000000000 + assert MediaFile(syspath(item.path)).artists is None + def test_write_file_with_an_unreadable_image(self): # Images are not compared, so an unreadable one does not stop the # write. From 345635e26ce737bead630e56605e4b9649ef79da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 17:45:27 +0300 Subject: [PATCH 4/8] library: test that a foreign-path write always saves 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. --- test/test_library.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/test_library.py b/test/test_library.py index e8d85f9612..cec1d70d26 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -1277,6 +1277,18 @@ def test_write_with_custom_path(self): assert MediaFile(syspath(custom_path)).artist == "new artist" assert MediaFile(syspath(item.path)).artist != "new artist" + def test_write_custom_path_with_the_tags(self): + # A write to a path other than the item's own file always saves. + item = self.add_item_fixture() + item.write() + custom_path = os.path.join(self.temp_dir, b"custom.mp3") + shutil.copy(syspath(item.path), syspath(custom_path)) + os.utime(syspath(custom_path), (1000000000, 1000000000)) + + item.write(custom_path) + + assert os.path.getmtime(syspath(custom_path)) != 1000000000 + def test_write_custom_tags(self): item = self.add_item_fixture(artist="old artist") item.write(tags={"artist": "new artist"}) From ce9f366f61731e12b696080a0e210cdc596b614d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 17:54:37 +0300 Subject: [PATCH 5/8] importadded: note that a skipped write leaves the mtime alone `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. --- docs/plugins/importadded.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/plugins/importadded.rst b/docs/plugins/importadded.rst index 8a6f92277a..baea85485a 100644 --- a/docs/plugins/importadded.rst +++ b/docs/plugins/importadded.rst @@ -47,7 +47,9 @@ file. There are two options available: - **preserve_mtimes**: After importing files, re-set their mtimes to their original value. Default: ``no``. - **preserve_write_mtimes**: After writing files, re-set their mtimes to their - original value. Default: ``no``. + original value. A file that already holds the tags being written is not + saved, so its mtime is not re-set either; pass ``-f`` to ``beet write`` to + force the save. Default: ``no``. Reimport -------- From 5448487a7d41432c5b43253df4f19e14be13e82e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 18:35:19 +0300 Subject: [PATCH 6/8] library: record the compared mtime when skipping a save 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. --- beets/library/models.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/beets/library/models.py b/beets/library/models.py index ddca8cf4aa..3fd726b738 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -1007,6 +1007,14 @@ def write( item_tags.update(tags) plugins.send("write", item=self, path=path, tags=item_tags) + # The mtime of the file the tags are about to be read from. A save + # skipped below records this one, so a change landing once the file + # is read stays newer than the database and still shows. + mtime = 0 + if path == self.path: + with suppress(OSError): + mtime = self.current_mtime() + # Open the file. try: mediafile = MediaFile(syspath(path), id3v23=id3v23) @@ -1040,7 +1048,7 @@ def write( # new mtime, which makes tools that sync the library copy the file # again for nothing. log.debug("no tags to write to {.filepath}", self) - self.mtime = self.current_mtime() + self.mtime = mtime return try: From 1bfbb22c6b2b2ce19b029c9b91704b1c428e3ea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 18:35:20 +0300 Subject: [PATCH 7/8] library: pin the always-save of image writes with a test 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. --- test/test_library.py | 36 +++++++++++++++++++++++++++++++++--- test/test_plugins.py | 7 ++++--- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/test/test_library.py b/test/test_library.py index cec1d70d26..0e19752eee 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -12,7 +12,7 @@ import mutagen import pytest -from mediafile import MediaFile, UnreadableFileError +from mediafile import Image, MediaFile, UnreadableFileError import beets.dbcore.query import beets.library @@ -1394,9 +1394,39 @@ def test_write_drops_a_list_tag_of_empty_values(self): assert item.current_mtime() != 1000000000 assert MediaFile(syspath(item.path)).artists is None + def test_write_image_tag_always_saves(self): + # Images are never compared, so a write that carries one saves even + # when the file already embeds the same image. + item = self.add_item_fixture(format="MP3") + with open(os.path.join(_common.RSRC, b"image-2x3.jpg"), "rb") as f: + image = Image(f.read()) + item.write(tags={"images": [image]}) + os.utime(syspath(item.path), (1000000000, 1000000000)) + + item.write(tags={"images": [image]}) + + assert item.current_mtime() != 1000000000 + + def test_write_skip_records_the_compared_mtime(self): + # A change landing while the file is read and compared stays newer + # than the recorded mtime, so that a later sync still sees it. + item = self.add_item_fixture(format="MP3") + item.write() + mtime = item.current_mtime() + real_init = MediaFile.__init__ + + def racy_init(mediafile, *args, **kwargs): + os.utime(syspath(item.path), (mtime + 100, mtime + 100)) + real_init(mediafile, *args, **kwargs) + + with patch.object(MediaFile, "__init__", racy_init): + item.write() + + assert item.mtime == mtime + def test_write_file_with_an_unreadable_image(self): - # Images are not compared, so an unreadable one does not stop the - # write. + # Only the fields being written are compared, so an image beets + # cannot read does not stop the write. path = os.path.join(self.temp_dir, b"unreadable_image.ogg") shutil.copy(os.path.join(_common.RSRC, b"full.ogg"), path) mediafile = mutagen.File(syspath(path)) diff --git a/test/test_plugins.py b/test/test_plugins.py index a129decba8..e49b618a47 100644 --- a/test/test_plugins.py +++ b/test/test_plugins.py @@ -75,9 +75,10 @@ def test_after_write_skipped_when_file_has_the_tags(self): class EventPlugin(plugins.BeetsPlugin): def __init__(self): super().__init__() - self.register_listener( - "after_write", lambda item, path: events.append(path) - ) + self.register_listener("after_write", self.on_after_write) + + def on_after_write(self, item, path): + events.append(path) self.register_plugin(EventPlugin) item = self.add_item_fixture() From 06356435c56380c98a028816c5ecb232a27b900c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Thu, 16 Jul 2026 19:54:59 +0300 Subject: [PATCH 8/8] importadded: wrap the note the way the docs check wants The line ran a few characters short of the width the formatter uses, which the docs check caught. --- docs/plugins/importadded.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/plugins/importadded.rst b/docs/plugins/importadded.rst index baea85485a..cba12d0c87 100644 --- a/docs/plugins/importadded.rst +++ b/docs/plugins/importadded.rst @@ -47,9 +47,9 @@ file. There are two options available: - **preserve_mtimes**: After importing files, re-set their mtimes to their original value. Default: ``no``. - **preserve_write_mtimes**: After writing files, re-set their mtimes to their - original value. A file that already holds the tags being written is not - saved, so its mtime is not re-set either; pass ``-f`` to ``beet write`` to - force the save. Default: ``no``. + original value. A file that already holds the tags being written is not saved, + so its mtime is not re-set either; pass ``-f`` to ``beet write`` to force the + save. Default: ``no``. Reimport --------