From d5f67c6de534deb9034c3c6adf88d2c545297bbb Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 11:45:27 +0100 Subject: [PATCH 1/3] feat: shorten paths that exceed the device's limit Rockbox's MAX_PATH is 260, defined in firmware/include/fs_defines.h and used to size the directory entry buffer in dir.h. It bounds the path as the device sees it, so the directory the mirror is copied into spends part of the same budget; --device-prefix accounts for that and defaults to /Music. 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 is cut first and the artist only if nothing else will serve. A shortened component keeps its extension and gains four hex digits of the original name: two long titles sharing a prefix cut to the same string otherwise, and a silent collision between two tracks is a worse outcome than an ugly filename. The result is stable. The same source always yields the same shortened name, so one pass does not rename what the last one wrote -- an unstable scheme would churn the whole mirror every six hours. A path too deeply nested to fit without reducing every component to nonsense is left alone and reported rather than mangled. Migration now tries more than one previous naming, because there is more than one. A mirror already running with --fat32-safe holds sanitised but unshortened paths, and matching only the original unsanitised name would have re-encoded every one of them instead of moving it. The checker gains the same two options, since it was measuring the mirror-relative path against a limit that applies to the device-absolute one, and so under-reported by the length of the destination directory. --- README.md | 20 ++++++ music_mirror.py | 139 ++++++++++++++++++++++++++++++++----- tests/test_music_mirror.py | 92 ++++++++++++++++++++++++ tools/check_fat32.py | 32 ++++++--- 4 files changed, 257 insertions(+), 26 deletions(-) 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 -- 2.54.0 From 46435feebd5033217499227b403628e981f9da32 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 11:50:42 +0100 Subject: [PATCH 2/3] fix: cut long names from the middle, not the end The shortening fitted the path and destroyed its meaning. Lidarr writes "Artist - Album - 07 - Flamethrower.mp3" inside a directory already named for that artist and album, so a long album title occurs three times in one path and everything that distinguishes one track from another sits at the very end. Cutting from the end removed precisely that: King Gizzard & the Lizard Wizard - PetroDragonic Apocalypse; or, Dawn of Eter~c526.mp3 All seven tracks on that record reduced to the same string bar the hash. The path fitted; the result was seven files nobody could tell apart on the device, which is a worse outcome than the failure it replaced. Cut from the middle instead, giving two thirds of the remaining room to the tail because the head is generally a restatement of the directory the file already sits in: King Gizzard & the Lizard~c526~ginning of Merciless Damnation - 07 - Flamethrower.mp3 The eight real paths that prompted this are now regression tests: every track on that album keeps its number and title, all seven names stay distinct, and The Beatles' "The Long One" -- whose length is the title itself rather than a repeated album name -- keeps both ends. --- README.md | 24 +++++++++++++--- music_mirror.py | 27 +++++++++++++---- tests/test_music_mirror.py | 59 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 4f55c96..dd9ba4f 100644 --- a/README.md +++ b/README.md @@ -246,10 +246,26 @@ 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. +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 diff --git a/music_mirror.py b/music_mirror.py index a4f5738..86cf13d 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -168,20 +168,35 @@ def fat32_safe(component): def shorten_component(component, budget): - """Return a component of at most `budget` characters, marked as shortened. + """Return a component of at most `budget` characters, cut from the middle. - 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. + 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() - tail = f"~{digest}{extension}" - return stem[: max(1, budget - len(tail))].rstrip(". ") + tail + 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): diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 097bfd8..0f0a4d7 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -705,3 +705,62 @@ def test_a_path_that_cannot_be_made_to_fit_is_reported(tmp_path, make_flac, capl 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") -- 2.54.0 From ece79515c0b522f1b7f710d5cad4cbe2760f7282 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 11:52:03 +0100 Subject: [PATCH 3/3] fix: cost the device prefix exactly rather than approximately The budget subtracted the prefix length plus two, on the assumption of a leading and a trailing slash. That is right for /Music and wrong for an empty prefix, where there is only one slash -- losing a character at the card root, which is exactly where the longest paths sit. Computed from the prefix as it will actually appear instead: /Music/ costs seven characters and gives a mirror-relative budget of 253, the root costs one and gives 259. Worth being exact about because the reverse error is worse. A checker comparing mirror-relative paths against the flat 260 passes everything between 253 and 260, and those are precisely the paths closest to the edge. --- README.md | 11 ++++++++++- music_mirror.py | 14 +++++++++++++- tests/test_music_mirror.py | 22 ++++++++++++++++++++++ tools/check_fat32.py | 14 +++++++++++++- 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index dd9ba4f..12df336 100644 --- a/README.md +++ b/README.md @@ -241,7 +241,16 @@ run would move rather than what it would delete. 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. +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 diff --git a/music_mirror.py b/music_mirror.py index 86cf13d..9ff0968 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -167,6 +167,18 @@ def fat32_safe(component): return cleaned or "_" +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. @@ -783,7 +795,7 @@ def main(argv=None): # 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) + 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", diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 0f0a4d7..16bbbe9 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -764,3 +764,25 @@ def test_a_long_title_keeps_both_ends(): 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 7ba0fa5..1c851f2 100755 --- a/tools/check_fat32.py +++ b/tools/check_fat32.py @@ -28,6 +28,18 @@ PATH_LIMIT = 260 DEVICE_PREFIX = "/Music" +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 = [] @@ -68,7 +80,7 @@ def main(argv=None): f" of the budget (default {DEVICE_PREFIX})", ) args = parser.parse_args(argv) - budget = max(0, args.max_path - len(args.device_prefix.strip("/")) - 2) + budget = max(0, args.max_path - len(device_prefix_length(args.device_prefix))) root = Path(args.root) if not root.is_dir(): -- 2.54.0