fix: accept a destination inside the device, and say so in --help

The script required the destination to be its own mount point, while the
documentation and its own usage text both told the user to pass
/media/IPOD/Music. The documented invocation was rejected.

A subdirectory is the better target, so the guard was what was wrong. --delete
is confined to it, and the device path budget is now derived from it -- the
part of the destination below its mount point -- rather than configured, so the
budget cannot disagree with where the files are actually going. The check that
matters is that the destination sits on a FAT filesystem, which is also what
catches an unmounted device: /media/IPOD/Music then resolves to the host's own
root filesystem, and emptying that is the outcome all of these guards exist to
prevent.

Three further faults found by testing the guards rather than reasoning about
them:

Stripping the trailing slash from "/" left an empty string, so the guard
refusing the host root never fired and the user got "destination is not a
directory" instead.

die() printed only its first argument, so the second half of the non-FAT
message -- the half saying to check whether the device is mounted -- was
silently dropped.

--help was not handled at all. Only -h reached the usage text, and it exited 2
to stderr, which is right for misuse and wrong for someone asking a question.
Help now goes to stdout and exits zero, and carries the guidance rather than
leaving it to the README, since the question it answers is asked at a terminal.

The test stage installs bash, rsync and findmnt, none of which are in the base
image, and the tests skip rather than fail where they are absent -- a machine
without rsync is not a machine that would run this script.

Dockerfile

@@ -26,6 +26,9 @@ ENTRYPOINT ["music-mirror"]
 FROM runtime AS test
 
 RUN pip install --no-cache-dir pytest
+# sync-to-ipod.sh and its tests need these; the runtime image deliberately does
+# not carry them, and neither does the base.
+RUN apk add --no-cache bash rsync findmnt
 COPY pytest.ini ./
 # Host-side tools; not in the runtime image, but the suite covers them.
 COPY tools ./tools

README.md

@@ -150,9 +150,15 @@ tools/sync-to-ipod.sh /mnt/tank/media/music-mp3 /media/IPOD/Music
 tools/sync-to-ipod.sh -n /mnt/tank/media/music-mp3 /media/IPOD/Music   # dry run
 ```
 
-It refuses to start unless the destination is a mounted FAT filesystem that is
-its own mount point, because `--delete` aimed at the wrong directory empties it
-and does not announce itself. It also excludes `/.rockbox`, the scrobbler logs
+The destination is where the artist folders should end up — normally a
+subdirectory such as `/media/IPOD/Music`, not the card root. A subdirectory is
+the better target: `--delete` is confined to it, and the device path budget is
+derived from it rather than configured, so the two cannot disagree.
+
+It refuses to start unless the destination is on a mounted FAT filesystem. That
+check is also what catches an unmounted device — `/media/IPOD/Music` then
+resolves to the host's own root filesystem, and this refuses to empty that.
+`--help` says all of it. It also excludes `/.rockbox`, the scrobbler logs
 and the various filesystem metadata directories from deletion — the mirror does
 not contain them, and without the exclusion a sync to the card root would
 remove the Rockbox install.
@@ -236,6 +242,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`

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

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "music-mirror"
-version = "0.2.1"
+version = "0.3.0"
 description = "Maintain a lossy MP3 mirror of a lossless music library"
 readme = "README.md"
 requires-python = ">=3.11"

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

tests/test_sync_to_ipod.py

@@ -0,0 +1,176 @@
+"""The guards on sync-to-ipod.sh, which are the substance of the script.
+
+rsync --delete is being aimed at a whole filesystem, so every refusal here is
+protecting against emptying the wrong directory -- a mistake that does not
+announce itself.
+"""
+
+import shutil
+import subprocess
+from pathlib import Path
+
+import pytest
+
+SCRIPT = Path(__file__).resolve().parent.parent / "tools" / "sync-to-ipod.sh"
+
+# Skipped rather than failed where the tools are absent: this is a host-side
+# script, and a machine without rsync is not a machine that would run it.
+REQUIRED = ("bash", "rsync", "findmnt")
+pytestmark = pytest.mark.skipif(
+    not all(shutil.which(tool) for tool in REQUIRED),
+    reason=f"needs {', '.join(REQUIRED)} on PATH",
+)
+
+
+def run(*arguments):
+    return subprocess.run(
+        ["bash", str(SCRIPT), *arguments], capture_output=True, text=True
+    )
+
+
+@pytest.fixture
+def mirror(tmp_path):
+    source = tmp_path / "mirror"
+    (source / "Album").mkdir(parents=True)
+    (source / "Album" / "track.mp3").write_bytes(b"x")
+    return source
+
+
+def test_the_host_root_is_refused(mirror):
+    """Stripping the trailing slash from "/" leaves an empty string, and an
+    earlier version then reported it as "not a directory" instead."""
+    result = run(str(mirror), "/")
+
+    assert result.returncode == 1
+    assert "refusing to sync onto /" in result.stderr
+
+
+def test_an_empty_mirror_is_refused(tmp_path):
+    """Mirroring nothing onto the device would delete everything on it."""
+    empty = tmp_path / "empty"
+    empty.mkdir()
+    destination = tmp_path / "dest"
+    destination.mkdir()
+
+    result = run(str(empty), str(destination))
+
+    assert result.returncode == 1
+    assert "refusing to mirror nothing" in result.stderr
+
+
+def test_syncing_a_directory_onto_itself_is_refused(mirror):
+    result = run(str(mirror), str(mirror))
+
+    assert result.returncode == 1
+    assert "same directory" in result.stderr
+
+
+def test_a_non_fat_destination_is_refused(mirror, tmp_path):
+    """Which is also how an unmounted device is caught: /media/IPOD/Music then
+    resolves to the host's own root filesystem."""
+    destination = tmp_path / "dest"
+    destination.mkdir()
+
+    result = run(str(mirror), str(destination))
+
+    assert result.returncode == 1
+    assert "not FAT" in result.stderr
+    assert "Is the device mounted?" in result.stderr
+
+
+def test_a_missing_destination_is_refused(mirror, tmp_path):
+    result = run(str(mirror), str(tmp_path / "nowhere"))
+
+    assert result.returncode == 1
+    assert "not a directory" in result.stderr
+
+
+def test_a_subdirectory_of_the_device_is_a_valid_target(mirror, tmp_path):
+    """The better target, in fact: --delete is confined to it."""
+    destination = tmp_path / "dest" / "Music"
+    destination.mkdir(parents=True)
+
+    result = run("-f", "-n", str(mirror), str(destination))
+
+    assert result.returncode == 0, result.stderr
+    assert "dry run, nothing was written" in result.stderr
+
+
+def test_the_device_prefix_is_derived_from_the_destination(mirror, tmp_path):
+    """Derived rather than configured, so it cannot disagree with where the
+    files are actually going -- and the device's path limit applies to it."""
+    destination = tmp_path / "dest" / "Music"
+    destination.mkdir(parents=True)
+
+    result = run("-f", "-n", str(mirror), str(destination))
+
+    assert "the device will see this as /" in result.stderr
+
+
+def test_a_dry_run_writes_nothing(mirror, tmp_path):
+    destination = tmp_path / "dest"
+    destination.mkdir()
+
+    run("-f", "-n", str(mirror), str(destination))
+
+    assert list(destination.iterdir()) == []
+
+
+def test_rockbox_is_never_deleted(mirror, tmp_path):
+    """A sync to the card root would otherwise remove the Rockbox install,
+    since the mirror does not contain it."""
+    destination = tmp_path / "dest"
+    destination.mkdir()
+    (destination / ".rockbox").mkdir()
+    (destination / ".rockbox" / "rockbox.ipod").write_bytes(b"firmware")
+    (destination / ".scrobbler.log").write_bytes(b"#AUDIOSCROBBLER/1.1\n")
+    (destination / "Stale.mp3").write_bytes(b"old")
+
+    result = run("-f", "-S", "-U", str(mirror), str(destination))
+
+    assert result.returncode == 0, result.stderr
+    assert (destination / ".rockbox" / "rockbox.ipod").is_file()
+    assert (destination / ".scrobbler.log").is_file()
+    # But a track whose source has gone is still removed. That is the point.
+    assert not (destination / "Stale.mp3").exists()
+    assert (destination / "Album" / "track.mp3").is_file()
+
+
+def test_help_goes_to_stdout_and_exits_clean():
+    """Asking for help is not an error; getting the arguments wrong is."""
+    result = run("--help")
+
+    assert result.returncode == 0
+    assert result.stdout.startswith("usage:")
+    assert result.stderr == ""
+
+
+def test_short_help_behaves_the_same():
+    result = run("-h")
+
+    assert result.returncode == 0
+    assert result.stdout.startswith("usage:")
+
+
+def test_misuse_goes_to_stderr_and_does_not():
+    result = run("only-one-argument")
+
+    assert result.returncode == 2
+    assert result.stderr.startswith("usage:")
+    assert result.stdout == ""
+
+
+def test_the_help_explains_what_the_destination_should_be():
+    """The question this script actually gets asked."""
+    help_text = run("--help").stdout
+
+    assert "/media/IPOD/Music" in help_text
+    assert "artist folders" in help_text
+    assert ".rockbox" in help_text
+
+
+def test_the_help_says_how_to_reach_and_leave_disk_mode():
+    help_text = run("--help").stdout
+
+    assert "Menu+Select" in help_text
+    assert "holding Play" in help_text

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

tools/sync-to-ipod.sh

@@ -12,7 +12,13 @@
 set -euo pipefail
 
 usage() {
-    cat >&2 <<'USAGE'
+    # Help goes to stdout and exits clean; misuse goes to stderr and does not.
+    local stream=2 code=2
+    if [ "${1:-}" = "help" ]; then
+        stream=1
+        code=0
+    fi
+    cat >&"$stream" <<'USAGE'
 usage: sync-to-ipod.sh [options] <mirror> <destination>
 
   -n  dry run; show what would change and touch nothing
@@ -24,34 +30,60 @@ Submitting scrobbles needs LASTFM_API_KEY and LASTFM_API_SECRET; it is skipped
 with a note when they are unset. Scrobbling is a write method and needs the
 secret, unlike the read-only calls elsewhere in these projects.
 
-The destination must be a mounted FAT filesystem. Reach it with the Apple
-firmware's disk mode: Menu+Select to reboot, then immediately Select+Play.
+The mirror is the directory holding the artist folders. The destination is
+where those folders should end up on the device -- not the card root, unless
+that is genuinely where you want them:
+
+  sync-to-ipod.sh /mnt/tank/media/music-mp3 /media/IPOD/Music
+
+A subdirectory is the better target: --delete is confined to it, and the path
+budget is derived from it, since the device's 260-character limit counts the
+whole path as the device sees it. /.rockbox and the scrobbler logs are never
+deleted wherever you point this.
+
+The destination must be on a mounted FAT filesystem. That check is also what
+catches an unmounted device: /media/IPOD/Music then resolves to the host's own
+root filesystem, and this refuses to empty that.
+
+Reach the device with the Apple firmware's disk mode: Menu+Select to reboot,
+then immediately Select+Play. Power off afterwards by holding Play.
 USAGE
-    exit 2
+    exit "$code"
 }
 
 dry_run=false
 force=false
 unmount=true
 scrobble=true
+for argument in "$@"; do
+    [ "$argument" = "--help" ] && usage help
+done
 while getopts ":nfSUh" option; do
     case "$option" in
         n) dry_run=true ;;
         f) force=true ;;
         S) scrobble=false ;;
         U) unmount=false ;;
+        h) usage help ;;
         *) usage ;;
     esac
 done
 shift $((OPTIND - 1))
 [ $# -eq 2 ] || usage
 
+# Trailing slashes are stripped for tidiness, but stripping one from "/" leaves
+# an empty string, and the guard below would then never see the root it is
+# there to refuse.
 mirror=${1%/}
+mirror=${mirror:-/}
 destination=${2%/}
+destination=${destination:-/}
 here=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)
 
 die() {
-    printf 'sync-to-ipod: %s\n' "$1" >&2
+    # Every argument, not just the first: the second half of a message is
+    # usually the half that says what to do about it.
+    printf 'sync-to-ipod: %s\n' "$*" >&2
     exit 1
 }
 
@@ -59,28 +91,41 @@ die() {
 [ -n "$(ls -A "$mirror")" ] || die "mirror $mirror is empty; refusing to mirror nothing"
 [ -d "$destination" ] || die "destination $destination is not a directory"
 
-# --delete makes every one of these load-bearing. A destination that is not its
-# own mount point means the path is wrong, and emptying the wrong directory is
-# not a mistake that announces itself.
+# --delete makes every one of these load-bearing. Emptying the wrong directory
+# is not a mistake that announces itself.
 case "$destination" in
     "" | "/" | "$HOME") die "refusing to sync onto $destination" ;;
 esac
 [ "$(readlink -f "$mirror")" != "$(readlink -f "$destination")" ] ||
     die "mirror and destination are the same directory"
-mountpoint -q -- "$destination" || die "$destination is not a mount point"
 
+# The filesystem the destination sits on, which is the check that matters: a
+# subdirectory of the card is a perfectly good target, and is the better one,
+# because --delete is then confined to it. Being FAT is also what proves the
+# card is mounted at all -- an unmounted /media/IPOD/Music resolves to the
+# host's own root filesystem, and this refuses to empty that.
 filesystem=$(findmnt -no FSTYPE --target "$destination")
+mounted_on=$(findmnt -no TARGET --target "$destination")
 case "$filesystem" in
     vfat | exfat) ;;
     *)
-        $force || die "$destination is $filesystem, not FAT; pass -f if that is deliberate"
+        $force ||
+            die "$destination is on a $filesystem filesystem, not FAT." \
+                "Is the device mounted? Pass -f if this is deliberate."
         printf 'sync-to-ipod: destination is %s, not FAT\n' "$filesystem" >&2
         ;;
 esac
 
+# What the device will call this directory, which is what its path limit
+# applies to. Derived rather than configured, so it cannot disagree with where
+# the files are actually going.
+device_prefix=${destination#"$mounted_on"}
+device_prefix="/${device_prefix#/}"
+printf 'sync-to-ipod: the device will see this as %s\n' "$device_prefix" >&2
+
 if $force; then
     printf 'sync-to-ipod: skipping the FAT32 check\n' >&2
-elif ! python3 "$here/check_fat32.py" "$mirror"; then
+elif ! python3 "$here/check_fat32.py" --device-prefix "$device_prefix" "$mirror"; then
     die "the mirror holds paths FAT32 will not take; run music-mirror with --fat32-safe"
 fi