From d5dce9c769d3ab7b14ecc8a83c741c64744b85fc Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:21:09 +0100 Subject: [PATCH] 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 | 3 + README.md | 12 ++- tests/test_sync_to_ipod.py | 176 +++++++++++++++++++++++++++++++++++++ tools/sync-to-ipod.sh | 67 +++++++++++--- 4 files changed, 244 insertions(+), 14 deletions(-) create mode 100644 tests/test_sync_to_ipod.py diff --git a/Dockerfile b/Dockerfile index 9291180..dfe0299 100644 --- a/Dockerfile +++ b/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 diff --git a/README.md b/README.md index 12df336..8f694cb 100644 --- a/README.md +++ b/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. diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py new file mode 100644 index 0000000..2aef186 --- /dev/null +++ b/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 diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index 2253c31..ca81d4c 100755 --- a/tools/sync-to-ipod.sh +++ b/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] -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