From 284069d9eb583cc296233468570d60b8c120e078 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 11:26:24 +0100 Subject: [PATCH] fix: report renames honestly in a dry run --dry-run described a FAT32 rename as an encode. process() skipped the rename whenever dry_run was set, then found no file at the target and fell through to the encode path, so a preview of enabling --fat32-safe announced a full re-encode of every track whose name held a reserved character. The real run moves those files in a moment. A preview that inverts the cost of the thing being previewed is worse than no preview at all. Prune compounded it. With nothing renamed, the pre-sanitisation files are still on disk, and they were reported as orphans due for deletion -- so the same dry run claimed the library would be re-encoded and the originals thrown away, neither of which is true. Renames are now their own outcome: reported as "would rename" in a dry run, counted separately from encodes in the pass summary, and excluded from the orphan list when a dry run leaves them in place. --- README.md | 7 +++++ music_mirror.py | 30 ++++++++++++++++----- tests/test_music_mirror.py | 55 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 39f5639..b6aae83 100644 --- a/README.md +++ b/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` diff --git a/music_mirror.py b/music_mirror.py index 306f745..0dc88d4 100644 --- a/music_mirror.py +++ b/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"], diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index d01e0c8..a710f3c 100644 --- a/tests/test_music_mirror.py +++ b/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