Fix for #630

osxphotos/photoexporter.py

@@ -1007,6 +1007,9 @@ class PhotoExporter:
                 elif options.exiftool:
                     sig_exif = export_db.get_stat_exif_for_file(dest_str)
                     cmp_orig = fileutil.cmp_file_sig(dest_str, sig_exif)
+                    if cmp_orig:
+                        # if signatures match also need to compare exifdata to see if metadata changed
+                        cmp_orig = not self._should_run_exiftool(dest_str, options)
                     sig_exif = (
                         sig_exif[0],
                         sig_exif[1],
@@ -1303,27 +1306,7 @@ class PhotoExporter:
         # determine if we need to write the exif metadata
         # if we are not updating, we always write
         # else, need to check the database to determine if we need to write
-        run_exiftool = not options.update
+        run_exiftool = self._should_run_exiftool(dest, options)
-        if options.update:
-            files_are_different = False
-            old_data = export_db.get_exifdata_for_file(dest)
-            if old_data is not None:
-                old_data = json.loads(old_data)[0]
-                current_data = json.loads(self._exiftool_json_sidecar(options=options))[
-                    0
-                ]
-                if old_data != current_data:
-                    files_are_different = True
-
-            if old_data is None or files_are_different:
-                # didn't have old data, assume we need to write it
-                # or files were different
-                run_exiftool = True
-            else:
-                verbose(
-                    f"Skipped up to date exiftool metadata for {pathlib.Path(dest).name}"
-                )
-
         if run_exiftool:
             verbose(f"Writing metadata with exiftool for {pathlib.Path(dest).name}")
             if not options.dry_run:
@@ -1342,8 +1325,32 @@ class PhotoExporter:
             )
             exiftool_results.exif_updated.append(dest)
             exiftool_results.to_touch.append(dest)
+        else:
+            verbose(
+                f"Skipped up to date exiftool metadata for {pathlib.Path(dest).name}"
+            )
         return exiftool_results
 
+    def _should_run_exiftool(self, dest, options: ExportOptions) -> bool:
+        """Return True if exiftool should be run to update metadata"""
+        run_exiftool = not options.update
+        if options.update:
+            files_are_different = False
+            old_data = options.export_db.get_exifdata_for_file(dest)
+            if old_data is not None:
+                old_data = json.loads(old_data)[0]
+                current_data = json.loads(self._exiftool_json_sidecar(options=options))[
+                    0
+                ]
+                if old_data != current_data:
+                    files_are_different = True
+
+            if old_data is None or files_are_different:
+                # didn't have old data, assume we need to write it
+                # or files were different
+                run_exiftool = True
+        return run_exiftool
+
     def _write_exif_data(self, filepath: str, options: ExportOptions):
         """write exif data to image file at filepath
 

tests/test_cli.py

@@ -1971,6 +1971,89 @@ def test_export_exiftool():
                     assert exif[key] == CLI_EXIFTOOL[uuid][key]
 
 
+@pytest.mark.skipif(exiftool is None, reason="exiftool not installed")
+def test_export_exiftool_template_change():
+    """Test --exiftool when template changes with --update, #630"""
+    import glob
+    import os
+    import os.path
+
+    from osxphotos.cli import export
+    from osxphotos.exiftool import ExifTool
+
+    runner = CliRunner()
+    cwd = os.getcwd()
+    # pylint: disable=not-context-manager
+    with runner.isolated_filesystem():
+        for uuid in CLI_EXIFTOOL:
+            # export with --exiftool
+            result = runner.invoke(
+                export,
+                [
+                    os.path.join(cwd, PHOTOS_DB_15_7),
+                    ".",
+                    "-V",
+                    "--exiftool",
+                    "--uuid",
+                    f"{uuid}",
+                ],
+            )
+            assert result.exit_code == 0
+
+            # export with --update, should be no change
+            result = runner.invoke(
+                export,
+                [
+                    os.path.join(cwd, PHOTOS_DB_15_7),
+                    ".",
+                    "-V",
+                    "--exiftool",
+                    "--update",
+                    "--uuid",
+                    f"{uuid}",
+                ],
+            )
+            assert result.exit_code == 0
+            assert "exported: 0" in result.output
+
+            # export with --update and template change, should export
+            result = runner.invoke(
+                export,
+                [
+                    os.path.join(cwd, PHOTOS_DB_15_7),
+                    ".",
+                    "-V",
+                    "--exiftool",
+                    "--keyword-template",
+                    "FOO",
+                    "--update",
+                    "--uuid",
+                    f"{uuid}",
+                ],
+            )
+            assert result.exit_code == 0
+            assert "updated EXIF data: 1" in result.output
+
+
+            # export with --update, nothing should export
+            result = runner.invoke(
+                export,
+                [
+                    os.path.join(cwd, PHOTOS_DB_15_7),
+                    ".",
+                    "-V",
+                    "--exiftool",
+                    "--keyword-template",
+                    "FOO",
+                    "--update",
+                    "--uuid",
+                    f"{uuid}",
+                ],
+            )
+            assert result.exit_code == 0
+            assert "exported: 0" in result.output
+            assert "updated EXIF data: 0" in result.output
+
 @pytest.mark.skipif(exiftool is None, reason="exiftool not installed")
 def test_export_exiftool_path():
     """test --exiftool with --exiftool-path"""
@@ -4721,7 +4804,7 @@ def test_export_update_basic():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: 0, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, updated EXIF data: 0, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: 0, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, updated EXIF data: 0, missing: 3, error: 0"
             in result.output
         )
 
@@ -4838,7 +4921,7 @@ def test_export_update_exiftool():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 2, error: 1"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 3, error: 1"
             in result.output
         )
 
@@ -4885,7 +4968,7 @@ def test_export_update_hardlink():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: 0, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: 0, missing: 3, error: 0"
             in result.output
         )
         assert not os.path.samefile(CLI_EXPORT_UUID_FILENAME, photo.path)
@@ -4924,7 +5007,7 @@ def test_export_update_hardlink_exiftool():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 2, error: 1"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, skipped: 0, updated EXIF data: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 3, error: 1"
             in result.output
         )
         assert not os.path.samefile(CLI_EXPORT_UUID_FILENAME, photo.path)
@@ -4962,7 +5045,7 @@ def test_export_update_edits():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 1, updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-2}, updated EXIF data: 0, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 1, updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-2}, updated EXIF data: 0, missing: 3, error: 0"
             in result.output
         )
 
@@ -5060,7 +5143,7 @@ def test_export_update_no_db():
         # edited files will be re-exported because there won't be an edited signature
         # in the database
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_EDITED_15_7}, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7}, updated EXIF data: 0, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 0, updated: {PHOTOS_EDITED_15_7}, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7}, updated EXIF data: 0, missing: 3, error: 0"
             in result.output
         )
         assert os.path.isfile(OSXPHOTOS_EXPORT_DB)
@@ -5100,7 +5183,7 @@ def test_export_then_hardlink():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 3, error: 0"
             in result.output
         )
         assert os.path.samefile(CLI_EXPORT_UUID_FILENAME, photo.path)
@@ -5125,7 +5208,7 @@ def test_export_dry_run():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7}, missing: 3, error: 0"
             in result.output
         )
         for filepath in CLI_EXPORT_FILENAMES_DRY_RUN:
@@ -5171,7 +5254,7 @@ def test_export_update_edits_dry_run():
         )
         assert result.exit_code == 0
         assert (
-            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 1, updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-2}, updated EXIF data: 0, missing: 2, error: 0"
+            f"Processed: {PHOTOS_NOT_IN_TRASH_LEN_15_7} photos, exported: 1, updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-2}, updated EXIF data: 0, missing: 3, error: 0"
             in result.output
         )