diff --git a/README.md b/README.md index b6aae83..4f55c96 100644 --- a/README.md +++ b/README.md @@ -236,6 +236,26 @@ 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. + +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 shortened component keeps its extension and gains four hex digits of +the original name — two long names sharing a prefix would otherwise cut to 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..a4f5738 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,56 @@ def fat32_safe(component): return cleaned or "_" -def mirror_path_for(source, source_root, mirror_root, safe=False): +def shorten_component(component, budget): + """Return a component of at most `budget` characters, marked as shortened. + + The mark is four hex digits of the original name. Two different long names + would otherwise cut down to 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() + tail = f"~{digest}{extension}" + return stem[: max(1, budget - len(tail))].rstrip(". ") + tail + + +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 +414,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 +470,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 +486,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 +544,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 +564,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 +574,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 +597,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 +695,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 +766,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(args.device_prefix.strip("/")) - 2) + 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 +803,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..097bfd8 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -613,3 +613,95 @@ 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 diff --git a/tools/check_fat32.py b/tools/check_fat32.py index d0baf30..7ba0fa5 100755 --- a/tools/check_fat32.py +++ b/tools/check_fat32.py @@ -21,12 +21,14 @@ 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 problems_with(relative, budget=PATH_LIMIT): """Return every reason this relative path is unfit for FAT32.""" found = [] for part in relative.parts: @@ -36,8 +38,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 +54,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(args.device_prefix.strip("/")) - 2) root = Path(args.root) if not root.is_dir(): @@ -68,7 +84,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 +98,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