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 while 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.

The usage text carries the guidance rather than only the README, since the
question it answers is asked at the terminal.

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,168 @@
+"""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 subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+SCRIPT = Path(__file__).resolve().parent.parent / "tools" / "sync-to-ipod.sh"
+
+
+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