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

@@ -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`

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):
+    for previous in candidates:
-        return False
+        if previous == mirror or not previous.is_file() or not is_current(source, previous):
-    if dry_run:
+            continue
-        logger.info("would rename %s -> %s", previous.name, mirror.name)
+        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)
+    return False
-    os.replace(previous, mirror)
-    logger.info("renamed %s -> %s", previous.name, mirror.name)
-    return True
 
 
 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 |= {
+        for source in work.values():
-            mirror_path_for(source, source_root, mirror_root) 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

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

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
+# Rockbox's MAX_PATH, from firmware/include/fs_defines.h. It bounds the path as
-# when every individual component is legal.
+# 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:
+    if len(str(relative)) > budget:
-        found.append(f"path of {len(str(relative))} characters")
+        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"
+            "Run music-mirror with --fat32-safe to have the mirror named acceptably"
-            " acceptably in the first place.",
+            " in the first place; it shortens over-long paths as well.",
             file=sys.stderr,
         )
     return 1 if faults else 0