From 7d4627bb8f592459e13e6999eb435970d78f73ed Mon Sep 17 00:00:00 2001 From: wordofglass Date: Tue, 22 Aug 2017 22:57:10 +0200 Subject: [PATCH 1/6] Ensure mtime is non-zero for Items in test fixtures. 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. --- test/_common.py | 1 + test/helper.py | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/test/_common.py b/test/_common.py index f3213ec31f..fc7b650b32 100644 --- a/test/_common.py +++ b/test/_common.py @@ -90,6 +90,7 @@ def item(lib=None): mb_artistid='someID-3', mb_albumartistid='someID-4', album_id=None, + mtime=12345, ) if lib: lib.add(i) diff --git a/test/helper.py b/test/helper.py index 795fd2ac07..5af742e12b 100644 --- a/test/helper.py +++ b/test/helper.py @@ -315,6 +315,8 @@ def create_item(self, **values): item = Item(**values_) if 'path' not in values: item['path'] = 'audio.' + item['format'].lower() + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 return item def add_item(self, **values): @@ -365,6 +367,8 @@ def add_item_fixtures(self, ext='mp3', count=1): item = Item.from_path(path) item.album = u'\u00e4lbum {0}'.format(i) # Check unicode paths item.title = u't\u00eftle {0}'.format(i) + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 item.add(self.lib) item.move(copy=True) item.store() @@ -380,6 +384,8 @@ def add_album_fixture(self, track_count=1, ext='mp3'): item = Item.from_path(path) item.album = u'\u00e4lbum' # Check unicode paths item.title = u't\u00eftle {0}'.format(i) + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 item.add(self.lib) item.move(copy=True) item.store() From 18c82cab5f35194cc0037e940c1f873e04c3271f Mon Sep 17 00:00:00 2001 From: wordofglass Date: Tue, 22 Aug 2017 23:17:39 +0200 Subject: [PATCH 2/6] test_edit: Add a comment explaining why I didn't change the writing behaviour I was midway through writing a test when realizing that it's best the way it is... --- test/test_edit.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test_edit.py b/test/test_edit.py index 2900d092ad..9486e84859 100644 --- a/test/test_edit.py +++ b/test/test_edit.py @@ -217,6 +217,9 @@ def test_single_edit_add_field(self, mock_write): ['a']) self.assertEqual(self.lib.items(u'id:1')[0].foo, 'bar') + # Even though a flexible attribute was written (which is not directly + # written to the tags), write should still be called since templates + # might use it. self.assertCounts(mock_write, write_call_count=1, title_starts_with=u't\u00eftle') From c811102eb2d1b2bd06bde8f3a89a42d0ecbefbdd Mon Sep 17 00:00:00 2001 From: wordofglass Date: Tue, 22 Aug 2017 23:22:58 +0200 Subject: [PATCH 3/6] test_edit: Expect mtime to change, unbreaks the multiple-item tests. --- test/test_edit.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_edit.py b/test/test_edit.py index 9486e84859..8953ea2f11 100644 --- a/test/test_edit.py +++ b/test/test_edit.py @@ -162,7 +162,7 @@ def test_title_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT, title_starts_with=u'modified t\u00eftle') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['title']) + ['title', 'mtime']) def test_single_title_edit_apply(self, mock_write): """Edit title for one item in the library, then apply changes.""" @@ -202,7 +202,7 @@ def test_album_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['album']) + ['album', 'mtime']) # Ensure album is *not* modified. self.album.load() self.assertEqual(self.album.album, u'\u00e4lbum') @@ -235,7 +235,7 @@ def test_a_album_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertEqual(self.album.album, u'modified \u00e4lbum') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['album']) + ['album', 'mtime']) def test_a_albumartist_edit_apply(self, mock_write): """Album query (-a), edit albumartist field, apply changes.""" @@ -249,7 +249,7 @@ def test_a_albumartist_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertEqual(self.album.albumartist, u'the modified album artist') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['albumartist']) + ['albumartist', 'mtime']) def test_malformed_yaml(self, mock_write): """Edit the yaml file incorrectly (resulting in a malformed yaml From b903b44a0c733227cd284ce631231a0caa62d47e Mon Sep 17 00:00:00 2001 From: wordofglass Date: Tue, 22 Aug 2017 23:31:40 +0200 Subject: [PATCH 4/6] test_edit: Fix test that intended to change one item but affected two. This didn't manifest as a testing failure since the plugin (mostly silently) drops id changes. --- test/test_edit.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/test_edit.py b/test/test_edit.py index 8953ea2f11..0a6a286f5c 100644 --- a/test/test_edit.py +++ b/test/test_edit.py @@ -210,13 +210,14 @@ def test_album_edit_apply(self, mock_write): def test_single_edit_add_field(self, mock_write): """Edit the yaml file appending an extra field to the first item, then apply changes.""" - # Append "foo: bar" to item with id == 1. - self.run_mocked_command({'replacements': {u"id: 1": - u"id: 1\nfoo: bar"}}, + # Append "foo: bar" to item with id == 2. ("id: 1" would match both + # "id: 1" and "id: 10") + self.run_mocked_command({'replacements': {u"id: 2": + u"id: 2\nfoo: bar"}}, # Apply changes. ['a']) - self.assertEqual(self.lib.items(u'id:1')[0].foo, 'bar') + self.assertEqual(self.lib.items(u'id:2')[0].foo, 'bar') # Even though a flexible attribute was written (which is not directly # written to the tags), write should still be called since templates # might use it. From c4fb2d195550a13d3ef69fd47c5851a2fb6a4dca Mon Sep 17 00:00:00 2001 From: wordofglass Date: Tue, 22 Aug 2017 18:21:47 +0200 Subject: [PATCH 5/6] Item: Do not reset the mtime in __setitem__ unless the value actually changes Unbreaks the remaining edit plugin tests. --- beets/dbcore/db.py | 15 ++++++++++++--- beets/library.py | 6 +++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index a714d94922..82371fb171 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -242,8 +242,9 @@ def __getitem__(self, key): else: raise KeyError(key) - def __setitem__(self, key, value): - """Assign the value for a field. + def _setitem(self, key, value): + """Assign the value for a field, return whether new and old value + differ. """ # Choose where to place the value. if key in self._fields: @@ -257,9 +258,17 @@ def __setitem__(self, key, value): # Assign value and possibly mark as dirty. old_value = source.get(key) source[key] = value - if self._always_dirty or old_value != value: + changed = old_value != value + if self._always_dirty or changed: self._dirty.add(key) + return changed + + def __setitem__(self, key, value): + """Assign the value for a field. + """ + self._setitem(key, value) + def __delitem__(self, key): """Remove a flexible attribute from the model. """ diff --git a/beets/library.py b/beets/library.py index f294e6d4f5..988628a5c5 100644 --- a/beets/library.py +++ b/beets/library.py @@ -547,10 +547,10 @@ def __setitem__(self, key, value): elif isinstance(value, BLOB_TYPE): value = bytes(value) - if key in MediaFile.fields(): - self.mtime = 0 # Reset mtime on dirty. + changed = super(Item, self)._setitem(key, value) - super(Item, self).__setitem__(key, value) + if changed and key in MediaFile.fields(): + self.mtime = 0 # Reset mtime on dirty. def update(self, values): """Set all key/value pairs in the mapping. If mtime is From 024689e781113259d9d6397d42dcd2aa2ed5f04a Mon Sep 17 00:00:00 2001 From: wordofglass Date: Fri, 25 Aug 2017 15:34:16 +0200 Subject: [PATCH 6/6] Changelog entry for the mtime fixes --- docs/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index c71eec055f..6190022ed0 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -27,6 +27,10 @@ Fixes: * :doc:`/plugins/edit`: Fix a bug when editing items during a ``-L`` re-import. Previously, diffs against against unrelated items could be shown or beets could crash with a traceback. :bug:`2659` +* 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. For developers: