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.

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/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