diff --git a/README.md b/README.md index b6aae83..12df336 100644 --- a/README.md +++ b/README.md @@ -236,6 +236,51 @@ 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. +### Path length + +Rockbox's `MAX_PATH` is 260, from `firmware/include/fs_defines.h`, and it bounds +the path *as the device sees it*. The directory the mirror is copied into comes +out of the same budget, so `--device-prefix` (default `/Music`) is subtracted +from `--max-path` to get what a mirror-relative path may spend: + +| Destination on the device | Mirror-relative budget | +| ------------------------- | ---------------------- | +| `/Music/` | 253 | +| the card root | 259 | + +Worth being exact about, because a checker that measures mirror-relative paths +against the flat 260 quietly passes everything from 253 to 260 — and those are +the paths most likely to be near the edge in the first place. + +Over-budget paths are shortened from the **deepest component outward**: the +track name carries the least navigational value and the artist directory the +most, so the filename goes first and the artist is touched only if nothing else +will do. + +A component is cut **from the middle**, not the end, because of how these names +are built. Lidarr writes `Artist - Album - 07 - Flamethrower.mp3` inside a +directory already named for that artist and album, so a long album title +appears three times in one path and the informative part — the track number and +title — is at the very end. Cutting from the end throws exactly that away: + +``` +before King Gizzard & the Lizard Wizard - PetroDragonic Apocalypse; or, Dawn of Eternal + Night - An Annihilation of Planet Earth and the Beginning of Merciless + Damnation - 07 - Flamethrower.mp3 +after King Gizzard & the Lizard~c526~ginning of Merciless Damnation - 07 - Flamethrower.mp3 +``` + +Two thirds of the remaining room goes to the tail, since the head is usually a +restatement of the directory it sits in. A shortened component gains four hex +digits of the original name: two names sharing both a head and a tail would +otherwise produce the same string, and a silent collision between two tracks is +worse than an ugly filename. + +The result is stable: the same source always produces the same shortened name, +so a pass does not rename what the previous pass wrote. A path too deeply +nested to fit without reducing every component to nonsense is left alone and +reported instead. + ### 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 0dc88d4..9ff0968 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -17,6 +17,7 @@ import argparse import concurrent.futures import fcntl import functools +import hashlib import logging import os import re @@ -88,6 +89,17 @@ MTIME_TOLERANCE_SECONDS = 2 # never arrives -- "Kick Out the Epic Motherf**ker" is a real example. FAT32_RESERVED = re.compile(r'[<>:"/\\|?*\x00-\x1f]') +# Rockbox's MAX_PATH, from firmware/include/fs_defines.h. It bounds the whole +# path as the device sees it, so the budget for a mirror-relative path is this +# less whatever directory the mirror is copied into. +MAX_PATH = 260 +DEVICE_PREFIX = "/Music" + +# A component cut below this is no longer recognisable, and a path that cannot +# be brought under the limit without going there is better reported than +# mangled. +MIN_COMPONENT = 12 + # The mirror exists to be read back by something else -- an SMB share, another # account on the box -- so everything written into it has to be group-readable. # Neither writer manages that unaided: tempfile.mkstemp forces 0600 whatever the @@ -155,11 +167,83 @@ def fat32_safe(component): return cleaned or "_" -def mirror_path_for(source, source_root, mirror_root, safe=False): +def device_prefix_length(prefix): + """Return the on-device prefix as it will actually appear, with slashes. + + "/Music" costs seven characters -- the leading slash, the name, and the + separator before the mirror's own path -- while an empty prefix costs one. + Approximating that loses a character at the root, which is precisely where + the longest paths are. + """ + cleaned = prefix.strip("/") + return f"/{cleaned}/" if cleaned else "/" + + +def shorten_component(component, budget): + """Return a component of at most `budget` characters, cut from the middle. + + From the middle, not the end, because of how these names are built. Lidarr + writes "Artist - Album - 07 - Flamethrower.mp3" inside a directory already + named for that artist and album, so the informative part -- the track + number and title -- is at the very end. Cutting from the end discards it + and leaves every track on the record with the same name. + + The four hex digits are of the original component. Two names sharing both a + head and a tail would otherwise produce the same string, and a silent + collision between two tracks is worse than an ugly filename. + """ + stem, dot, extension = component.rpartition(".") + if not dot or len(extension) > 4: + stem, extension = component, "" + else: + extension = dot + extension + + digest = hashlib.blake2s(component.encode("utf-8"), digest_size=2).hexdigest() + marker = f"~{digest}~" + room = max(2, budget - len(extension) - len(marker)) + if room >= len(stem): + return stem + extension + + # Two thirds to the tail: the head is usually a restatement of the + # directory it sits in, and the tail is what tells two tracks apart. + keep_end = min(len(stem), room * 2 // 3) + keep_start = max(1, room - keep_end) + return stem[:keep_start].rstrip(". ") + marker + stem[len(stem) - keep_end :] + extension + + +def fit_path(relative, budget): + """Return a relative path within `budget` characters, or the best available. + + Shortened from the deepest component outward. The filename carries the least + navigational value and the artist directory the most, so the track name is + sacrificed before the album and the album before the artist. + """ + parts = list(relative.parts) + for index in reversed(range(len(parts))): + overage = len(str(Path(*parts))) - budget + if overage <= 0: + break + allowed = max(MIN_COMPONENT, len(parts[index]) - overage) + if allowed < len(parts[index]): + parts[index] = shorten_component(parts[index], allowed) + fitted = Path(*parts) + if len(str(fitted)) > budget: + logger.warning( + "%s is still %d characters over the limit after shortening; it is too" + " deeply nested to fit", + relative, + len(str(fitted)) - budget, + ) + return fitted + + +def mirror_path_for(source, source_root, mirror_root, safe=False, budget=0): """Return the mirror path corresponding to a source file.""" relative = source.relative_to(source_root).with_suffix(MIRROR_SUFFIX) if safe: relative = Path(*(fat32_safe(part) for part in relative.parts)) + if budget > 0 and len(str(relative)) > budget: + relative = fit_path(relative, budget) return mirror_root / relative @@ -357,23 +441,29 @@ def copy(source, mirror, dry_run): return Result("copied", mirror) -def adopt_existing(source, mirror, previous, dry_run=False): +def adopt_existing(source, mirror, candidates, 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 held a reserved character. Without this the run would encode them all again and then prune the originals -- hours of work to produce files that already exist, byte for byte, under the old name. + + Several candidates are tried because there is more than one previous + naming: the original, and the sanitised-but-not-yet-shortened form left by + an earlier version. """ - 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) + for previous in candidates: + if previous == mirror or not previous.is_file() or not is_current(source, previous): + continue + 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) return True - mirror.parent.mkdir(parents=True, exist_ok=True) - os.replace(previous, mirror) - logger.info("renamed %s -> %s", previous.name, mirror.name) - return True + return False def process(source, mirror, quality_args, dry_run, previous=None): @@ -407,7 +497,7 @@ def find_sources(root): yield path -def plan(scan_root, source_root, mirror_root, safe=False): +def plan(scan_root, source_root, mirror_root, safe=False, budget=0): """Map each mirror path to the one source that should produce it. Two sources can want the same mirror path -- `01 Song.flac` alongside a @@ -423,7 +513,7 @@ def plan(scan_root, source_root, mirror_root, safe=False): # that now beats discovering it as a silent overwrite during the copy. seen = {} for source in find_sources(scan_root): - mirror = mirror_path_for(source, source_root, mirror_root, safe) + mirror = mirror_path_for(source, source_root, mirror_root, safe, budget) key = str(mirror).casefold() if safe else str(mirror) rival_path = seen.get(key) rival = chosen.get(rival_path) if rival_path else None @@ -481,7 +571,15 @@ def prune(mirror_root, expected, dry_run): def run_once( - scan_root, source_root, mirror_root, quality_args, jobs, dry_run, do_prune, safe=False + scan_root, + source_root, + mirror_root, + quality_args, + jobs, + dry_run, + do_prune, + safe=False, + budget=0, ): """Run a single pass. Returns the number of failures. @@ -493,7 +591,7 @@ def run_once( counts = {"encoded": 0, "copied": 0, "renamed": 0, "skipped": 0, "failed": 0} failures = [] - work = plan(scan_root, source_root, mirror_root, safe) + work = plan(scan_root, source_root, mirror_root, safe, budget) with concurrent.futures.ThreadPoolExecutor(max_workers=jobs) as pool: futures = [ @@ -503,7 +601,14 @@ def run_once( mirror, quality_args, dry_run, - mirror_path_for(source, source_root, mirror_root) if safe else None, + ( + [ + mirror_path_for(source, source_root, mirror_root), + mirror_path_for(source, source_root, mirror_root, True), + ] + if safe + else None + ), ) for mirror, source in work.items() ] @@ -519,9 +624,9 @@ def run_once( # 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() - } + for source in work.values(): + expected.add(mirror_path_for(source, source_root, mirror_root)) + expected.add(mirror_path_for(source, source_root, mirror_root, True)) removed = prune(mirror_root, expected, dry_run) if do_prune else 0 for failure in failures: @@ -617,6 +722,19 @@ def build_parser(): help="name mirror files so a FAT32 device will accept them" " (env MUSIC_MIRROR_FAT32_SAFE)", ) + parser.add_argument( + "--max-path", + type=int, + default=int(os.getenv("MUSIC_MIRROR_MAX_PATH", str(MAX_PATH))), + help=f"longest path the device will take, counted from its root; Rockbox's" + f" MAX_PATH is {MAX_PATH} (env MUSIC_MIRROR_MAX_PATH)", + ) + parser.add_argument( + "--device-prefix", + default=os.getenv("MUSIC_MIRROR_DEVICE_PREFIX", DEVICE_PREFIX), + help="directory the mirror is copied into on the device, whose length comes" + " out of the path budget (env MUSIC_MIRROR_DEVICE_PREFIX)", + ) parser.add_argument( "--no-prune", action="store_true", @@ -675,6 +793,17 @@ def main(argv=None): # A partial pass cannot tell an orphan from a file outside its scope. do_prune = False + # The device's limit covers the whole path it will see, so what the mirror + # may spend is that less the directory it gets copied into. + budget = max(0, args.max_path - len(device_prefix_length(args.device_prefix))) + if args.fat32_safe: + logger.info( + "paths are limited to %d characters, from --max-path %d less the %r prefix", + budget, + args.max_path, + args.device_prefix, + ) + lock = acquire_lock(mirror_root) if lock is None: logger.error("another pass is already running over %s", mirror_root) @@ -701,6 +830,7 @@ def main(argv=None): args.dry_run, do_prune, args.fat32_safe, + budget, ) if interval is None or stopping: return 1 if failures else 0 diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index a710f3c..16bbbe9 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -613,3 +613,176 @@ def test_renames_are_counted_separately_from_encodes(tmp_path, make_flac, caplog assert "1 renamed" in caplog.text assert "0 encoded" in caplog.text + + +def test_a_long_path_is_shortened_from_the_deepest_component(tmp_path, make_flac): + """The track name carries the least navigational value and the artist the + most, so the filename is sacrificed before the album.""" + artist = "A" * 60 + album = "B" * 60 + title = "C" * 150 + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / artist / album / f"{title}.flac") + + run(source, mirror, "--fat32-safe", "--max-path", "160", "--device-prefix", "/Music") + + written = list(mirror.rglob("*.mp3")) + assert len(written) == 1 + relative = written[0].relative_to(mirror) + assert relative.parts[0] == artist, "the artist directory should be untouched" + assert relative.parts[1] == album, "the album directory should be untouched" + assert len(str(relative)) <= 160 - len("Music") - 2 + + +def test_shortening_is_stable_across_passes(tmp_path, make_flac): + """An unstable name would rename every file on every pass, for ever.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / ("D" * 80) / ("E" * 80) / f"{'F' * 120}.flac") + + run(source, mirror, "--fat32-safe", "--max-path", "180") + first = sorted(str(p.relative_to(mirror)) for p in mirror.rglob("*.mp3")) + stamp = next(mirror.rglob("*.mp3")).stat().st_mtime_ns + + run(source, mirror, "--fat32-safe", "--max-path", "180") + + assert sorted(str(p.relative_to(mirror)) for p in mirror.rglob("*.mp3")) == first + assert next(mirror.rglob("*.mp3")).stat().st_mtime_ns == stamp + + +def test_two_long_names_do_not_collide_after_shortening(tmp_path, make_flac): + """They share a prefix and cut to the same string; the hash is what keeps + them apart.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + shared = "G" * 140 + make_flac(source / "Album" / f"{shared}one.flac") + make_flac(source / "Album" / f"{shared}two.flac") + + run(source, mirror, "--fat32-safe", "--max-path", "120") + + assert len(list(mirror.rglob("*.mp3"))) == 2 + + +def test_a_sanitised_mirror_is_renamed_rather_than_re_encoded_when_shortening( + tmp_path, make_flac +): + """The previous naming is sanitised-but-not-shortened, not the original.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "Album" / f"Where Are You? {'H' * 140}.flac") + + run(source, mirror, "--fat32-safe", "--max-path", "400") + before = next(mirror.rglob("*.mp3")) + contents = before.read_bytes() + + run(source, mirror, "--fat32-safe", "--max-path", "120") + + after = next(mirror.rglob("*.mp3")) + assert after != before + assert after.read_bytes() == contents, "it was re-encoded rather than moved" + + +def test_shortening_only_applies_when_over_budget(tmp_path, make_flac): + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "Artist" / "Album" / "Short Name.flac") + + run(source, mirror, "--fat32-safe") + + assert (mirror / "Artist" / "Album" / "Short Name.mp3").is_file() + + +def test_a_path_that_cannot_be_made_to_fit_is_reported(tmp_path, make_flac, caplog): + """Too deeply nested to shorten without making every component unreadable.""" + deep = Path(*["I" * 20 for _ in range(10)]) + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / deep / "track.flac") + + with caplog.at_level("WARNING"): + run(source, mirror, "--fat32-safe", "--max-path", "80") + + assert "too" in caplog.text and "nested" in caplog.text + + +# Lidarr writes "Artist - Album - NN - Title.mp3" inside a directory already +# named for that artist and album, so a long album title appears three times in +# one path. These are real paths from a real library. +GIZZARD_ALBUM = ( + "PetroDragonic Apocalypse; or, Dawn of Eternal Night - An Annihilation of" + " Planet Earth and the Beginning of Merciless Damnation" +) +GIZZARD_TRACKS = ( + "01 - Motor Spirit", + "02 - Supercell", + "03 - Converge", + "04 - Witchcraft", + "05 - Gila Monster", + "06 - Dragon", + "07 - Flamethrower", +) + + +def gizzard_path(track): + return Path( + "King Gizzard & the Lizard Wizard", + f"{GIZZARD_ALBUM} (2023)", + f"King Gizzard & the Lizard Wizard - {GIZZARD_ALBUM} - {track}.mp3", + ) + + +def test_shortening_keeps_the_part_that_tells_tracks_apart(): + """Cutting from the end discards the track number and title, which is all + that distinguishes one track on the record from another.""" + for track in GIZZARD_TRACKS: + fitted = music_mirror.fit_path(gizzard_path(track), 253) + assert len(str(fitted)) <= 253 + assert fitted.name.endswith(f"{track}.mp3"), fitted.name + + +def test_every_track_on_a_long_album_keeps_a_distinct_name(): + fitted = {music_mirror.fit_path(gizzard_path(t), 253).name for t in GIZZARD_TRACKS} + + assert len(fitted) == len(GIZZARD_TRACKS) + + +def test_a_long_title_keeps_both_ends(): + """The Beatles' 'The Long One' is one track whose title is the long part.""" + path = Path( + "The Beatles", + "Abbey Road (1969)", + "Digital Media 03", + "The Beatles - Abbey Road - 09 - The Long One - You Never Give Me Your Money" + " + Sun King + Mean Mr Mustard + Her Majesty + Polythene Pam + She Came In" + " Through the Bathroom Window+ Golden Slumbers + Carry That Weight + The End.mp3", + ) + + fitted = music_mirror.fit_path(path, 253) + + assert len(str(fitted)) <= 253 + assert fitted.name.startswith("The Beatles - Abbey Road - 09 - The Long One") + assert fitted.name.endswith("The End.mp3") + + +@pytest.mark.parametrize( + ("prefix", "expected"), + [("/Music", 253), ("Music", 253), ("/Music/", 253), ("", 259), ("/", 259)], +) +def test_the_device_prefix_is_costed_exactly(prefix, expected): + """A mirror-relative path of 253 characters becomes 260 on the device once + /Music/ is in front of it, which is the whole of the limit. Approximating + the prefix loses a character at the root, where the longest paths are.""" + assert 260 - len(music_mirror.device_prefix_length(prefix)) == expected + + +def test_the_budget_is_reported_so_it_can_be_checked(tmp_path, make_flac, caplog): + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "a.flac") + + with caplog.at_level("INFO"): + run(source, mirror, "--fat32-safe") + + assert "limited to 253 characters" in caplog.text diff --git a/tools/check_fat32.py b/tools/check_fat32.py index d0baf30..1c851f2 100755 --- a/tools/check_fat32.py +++ b/tools/check_fat32.py @@ -21,12 +21,26 @@ from pathlib import Path RESERVED = re.compile(r'[<>:"\\|?*\x00-\x1f]') COMPONENT_LIMIT = 255 -# Rockbox builds paths into a fixed buffer; long trees fail on the device even -# when every individual component is legal. +# Rockbox's MAX_PATH, from firmware/include/fs_defines.h. It bounds the path as +# the device sees it, so the directory the mirror is copied into comes out of +# the same budget. PATH_LIMIT = 260 +DEVICE_PREFIX = "/Music" -def problems_with(relative): +def device_prefix_length(prefix): + """Return the on-device prefix as it will actually appear, with slashes. + + "/Music" costs seven characters -- the leading slash, the name, and the + separator before the mirror's own path -- while an empty prefix costs one. + Approximating that loses a character at the root, which is precisely where + the longest paths are. + """ + cleaned = prefix.strip("/") + return f"/{cleaned}/" if cleaned else "/" + + +def problems_with(relative, budget=PATH_LIMIT): """Return every reason this relative path is unfit for FAT32.""" found = [] for part in relative.parts: @@ -36,8 +50,8 @@ def problems_with(relative): found.append(f"trailing dot or space in {part!r}") if len(part) > COMPONENT_LIMIT: found.append(f"component of {len(part)} characters") - if len(str(relative)) > PATH_LIMIT: - found.append(f"path of {len(str(relative))} characters") + if len(str(relative)) > budget: + found.append(f"path of {len(str(relative))} characters, over a budget of {budget}") return found @@ -52,7 +66,21 @@ def main(argv=None): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("root", help="directory to check, e.g. the mirror") parser.add_argument("--limit", type=int, default=0, help="show at most this many") + parser.add_argument( + "--max-path", + type=int, + default=PATH_LIMIT, + help=f"longest path the device will take, from its root (default {PATH_LIMIT}," + " Rockbox's MAX_PATH)", + ) + parser.add_argument( + "--device-prefix", + default=DEVICE_PREFIX, + help="directory the mirror is copied into on the device; its length comes out" + f" of the budget (default {DEVICE_PREFIX})", + ) args = parser.parse_args(argv) + budget = max(0, args.max_path - len(device_prefix_length(args.device_prefix))) root = Path(args.root) if not root.is_dir(): @@ -68,7 +96,7 @@ def main(argv=None): # different strings, and the collision check would miss it. key = unicodedata.normalize("NFC", str(relative)).casefold() by_case[key].append(relative) - for problem in problems_with(relative): + for problem in problems_with(relative, budget): faults.append((relative, problem)) for relative, group in sorted(by_case.items()): @@ -82,8 +110,8 @@ def main(argv=None): print(f"\n{len(faults)} problems across {total} files", file=sys.stderr) if faults: print( - "Run music-mirror with --fat32-safe to have the mirror named" - " acceptably in the first place.", + "Run music-mirror with --fat32-safe to have the mirror named acceptably" + " in the first place; it shortens over-long paths as well.", file=sys.stderr, ) return 1 if faults else 0