Merge pull request 'fix: report renames honestly in a dry run' (#7) from fix/dry-run-renames into main

Reviewed-on: #7

README.md

@@ -229,6 +229,13 @@ producing files that already exist byte for byte. The run moves them instead,
 and says so. Prune then finds nothing to remove because nothing was left
 behind.
 
+Renames are counted apart from encodes in the pass summary, and `--dry-run`
+reports `would rename` rather than `would encode` — the difference between the
+two is a minute against an afternoon, so a preview that conflated them would be
+worse than no preview. A dry run also does not list the pre-rename files as
+orphans: nothing was moved, so they are still there, but they are what a real
+run would move rather than what it would delete.
+
 ### Album art
 
 Rockbox looks for cover art **on the filesystem** — `cover.jpg`, `folder.jpg`

music_mirror.py

@@ -357,7 +357,7 @@ def copy(source, mirror, dry_run):
     return Result("copied", mirror)
 
 
-def adopt_existing(source, mirror, previous):
+def adopt_existing(source, mirror, previous, dry_run=False):
     """Move an already-encoded file to its new name. Returns whether it moved.
 
     Turning on FAT32-safe naming changes the path of every track whose name
@@ -367,6 +367,9 @@ def adopt_existing(source, mirror, previous):
     """
     if previous == mirror or not previous.is_file() or not is_current(source, previous):
         return False
+    if dry_run:
+        logger.info("would rename %s -> %s", previous.name, mirror.name)
+        return True
     mirror.parent.mkdir(parents=True, exist_ok=True)
     os.replace(previous, mirror)
     logger.info("renamed %s -> %s", previous.name, mirror.name)
@@ -375,8 +378,12 @@ def adopt_existing(source, mirror, previous):
 
 def process(source, mirror, quality_args, dry_run, previous=None):
     """Bring one source file's mirror entry up to date."""
-    if previous is not None and not dry_run and not mirror.exists():
-        adopt_existing(source, mirror, previous)
+    # Counted separately from an encode, and reported in a dry run, because the
+    # difference between moving a file and re-encoding it is the difference
+    # between a minute and an afternoon.
+    if previous is not None and not mirror.exists():
+        if adopt_existing(source, mirror, previous, dry_run):
+            return Result("renamed", mirror)
     if is_current(source, mirror):
         # A mirror written before this bit was set has a correct mtime, so
         # nothing else in the pass would ever revisit it. Top it up here
@@ -483,7 +490,7 @@ def run_once(
     """
     started = time.monotonic()
     logger.info("pass starting with %d concurrent encoders", jobs)
-    counts = {"encoded": 0, "copied": 0, "skipped": 0, "failed": 0}
+    counts = {"encoded": 0, "copied": 0, "renamed": 0, "skipped": 0, "failed": 0}
     failures = []
 
     work = plan(scan_root, source_root, mirror_root, safe)
@@ -506,16 +513,27 @@ def run_once(
             if result.action == "failed":
                 failures.append(result)
 
-    removed = prune(mirror_root, set(work), dry_run) if do_prune else 0
+    expected = set(work)
+    if safe and dry_run:
+        # Nothing was actually renamed, so the pre-sanitisation files are still
+        # on disk. They are not orphans -- they are the files a real run would
+        # move -- and reporting them for deletion would misrepresent the pass
+        # twice over.
+        expected |= {
+            mirror_path_for(source, source_root, mirror_root) for source in work.values()
+        }
+    removed = prune(mirror_root, expected, dry_run) if do_prune else 0
 
     for failure in failures:
         logger.error("failed: %s: %s", failure.path, failure.error)
 
     logger.info(
-        "pass complete in %.1fs: %d encoded, %d copied, %d up to date, %d removed, %d failed",
+        "pass complete in %.1fs: %d encoded, %d copied, %d renamed, %d up to date,"
+        " %d removed, %d failed",
         time.monotonic() - started,
         counts["encoded"],
         counts["copied"],
+        counts["renamed"],
         counts["skipped"],
         removed,
         counts["failed"],

tests/test_music_mirror.py

@@ -558,3 +558,58 @@ def test_a_cover_left_without_tracks_is_pruned(tmp_path, make_flac, make_cover):
     run(source, mirror)
 
     assert not (mirror / "Gone").exists()
+
+
+def test_a_dry_run_reports_a_rename_not_an_encode(tmp_path, make_flac, caplog):
+    """The difference between moving a file and re-encoding it is the
+    difference between a minute and an afternoon, so a dry run must not
+    describe the first as the second."""
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "Album" / "Where Are You?.flac")
+    run(source, mirror)
+
+    with caplog.at_level("INFO"):
+        run(source, mirror, "--fat32-safe", "--dry-run")
+
+    assert "would rename" in caplog.text
+    assert "would encode" not in caplog.text
+
+
+def test_a_dry_run_does_not_call_the_old_paths_orphans(tmp_path, make_flac, caplog):
+    """Nothing was renamed, so they are still there -- but they are the files a
+    real run would move, not files it would delete."""
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "Album" / "Where Are You?.flac")
+    run(source, mirror)
+
+    with caplog.at_level("INFO"):
+        run(source, mirror, "--fat32-safe", "--dry-run")
+
+    assert "would remove orphan" not in caplog.text
+
+
+def test_a_dry_run_moves_nothing(tmp_path, make_flac):
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "Album" / "Where Are You?.flac")
+    run(source, mirror)
+
+    run(source, mirror, "--fat32-safe", "--dry-run")
+
+    assert (mirror / "Album" / "Where Are You?.mp3").is_file()
+    assert not (mirror / "Album" / "Where Are You_.mp3").exists()
+
+
+def test_renames_are_counted_separately_from_encodes(tmp_path, make_flac, caplog):
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "Album" / "Where Are You?.flac")
+    run(source, mirror)
+
+    with caplog.at_level("INFO"):
+        run(source, mirror, "--fat32-safe")
+
+    assert "1 renamed" in caplog.text
+    assert "0 encoded" in caplog.text