From d5dce9c769d3ab7b14ecc8a83c741c64744b85fc Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:21:09 +0100 Subject: [PATCH 1/7] 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 From 37b841f0090d4cc95739e96631420eae77fe56eb Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:31:13 +0100 Subject: [PATCH 2/7] feat: show which album is copying, and how far through The transfer looked hung. rsync prints nothing while it builds its file list, which on fifty thousand files over USB is several minutes of silence, and --info=progress2 does not help: with incremental recursion its percentage is computed against a list rsync has not finished discovering, so it moves backwards as often as forwards. The script now counts what needs copying first and says so, then renders its own single line that rewrites in place, showing the album currently going across and a percentage against a total that is actually known. Counting costs a second pass over the tree. That is the price of a percentage meaning something, and it is cheaper than staring at a blank terminal wondering whether the thing has died. Directories are excluded from the count. rsync reports those too, and including them puts the figure past a hundred per cent. Piped to a log the line becomes a plain one every thirty seconds, because a log full of carriage returns and escape codes is not a log anybody reads. --- README.md | 13 +++++ tests/test_rsync_progress.py | 104 +++++++++++++++++++++++++++++++++++ tools/rsync_progress.py | 89 ++++++++++++++++++++++++++++++ tools/sync-to-ipod.sh | 24 +++++++- 4 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 tests/test_rsync_progress.py create mode 100755 tools/rsync_progress.py diff --git a/README.md b/README.md index 8f694cb..e0853c5 100644 --- a/README.md +++ b/README.md @@ -163,6 +163,19 @@ and the various filesystem metadata directories from deletion — the mirror doe not contain them, and without the exclusion a sync to the card root would remove the Rockbox install. +Progress is a single line that rewrites itself: + +``` +[ 12,345 / 49,600 24%] King Gizzard & the Lizard Wizard / PetroDragonic Apoc… +``` + +rsync says nothing at all while it builds its file list, which on fifty +thousand files over USB is minutes of apparent hang, and its own `progress2` +percentage is computed against a list it has not finished discovering. So the +script counts first — a second pass over the tree, which is what a percentage +that means something costs — and renders the rest itself. Piped to a log it +prints a plain line every thirty seconds instead, with no carriage returns. + The unmount is the point of doing this in a script. FAT32 has no journal and the device is reached through disk mode, so an interrupted write is corruption that needs `fsck.vfat` from another machine. diff --git a/tests/test_rsync_progress.py b/tests/test_rsync_progress.py new file mode 100644 index 0000000..9a25fca --- /dev/null +++ b/tests/test_rsync_progress.py @@ -0,0 +1,104 @@ +import io +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "tools")) + +import rsync_progress # noqa: E402 + + +class NotATerminal(io.StringIO): + def isatty(self): + return False + + +class Terminal(io.StringIO): + def isatty(self): + return True + + +def run(lines, total=0, out=None): + out = out or NotATerminal() + rsync_progress.main(["--total", str(total)], stream=io.StringIO(lines), out=out) + return out.getvalue() + + +def test_the_artist_and_album_are_pulled_from_the_path(): + assert rsync_progress.album_of("Pendulum/Immersion/01 - Watercolour.mp3") == ( + "Pendulum / Immersion" + ) + + +def test_a_shallower_path_degrades_rather_than_failing(): + assert rsync_progress.album_of("Pendulum/loose.mp3") == "Pendulum" + assert rsync_progress.album_of("loose.mp3") == "" + + +def test_directories_are_not_counted(): + """rsync reports them too, and counting them puts the percentage past 100.""" + output = run("Artist/\nArtist/Album/\nArtist/Album/track.mp3\n", total=1) + + assert "1 / 1" in output + assert "100%" in output + + +def test_the_percentage_tracks_the_total(): + output = run("".join(f"A/B/{i}.mp3\n" for i in range(5)), total=10) + + assert "5 / 10" in output + assert " 50%" in output + + +def test_without_a_total_it_counts_instead_of_guessing(): + output = run("A/B/one.mp3\nA/B/two.mp3\n") + + assert "2 files" in output + assert "%" not in output + + +def test_a_final_line_is_always_printed(): + """Otherwise the last state of a rewriting line is whatever it happened to + be when the interval last elapsed.""" + output = run("A/B/one.mp3\n", total=1) + + assert output.endswith("\n") + assert "1 / 1" in output + + +def test_nothing_transferred_still_reports(): + output = run("", total=0) + + assert "0 files" in output + + +def test_a_log_gets_no_carriage_returns(): + """A non-terminal filling with \\r and escape codes is unreadable.""" + output = run("".join(f"A/B/{i}.mp3\n" for i in range(50)), total=50) + + assert "\r" not in output + assert "\033" not in output + + +def test_a_terminal_rewrites_one_line(): + output = run("".join(f"A/B/{i}.mp3\n" for i in range(50)), total=50, out=Terminal()) + + assert "\r\033[2K" in output + + +@pytest.mark.parametrize( + ("text", "width", "expected"), + [ + ("short", 20, "short"), + ("King Gizzard / PetroDragonic Apocalypse", 20, "…agonic Apocalypse"), + ], +) +def test_long_labels_are_trimmed_from_the_left(text, width, expected): + """The album is the informative end, so the artist is what gets cut.""" + trimmed = rsync_progress.fit(text, width) + + assert len(trimmed) <= width + if len(text) > width: + assert trimmed.startswith("…") + assert text.endswith(trimmed.lstrip("…")) diff --git a/tools/rsync_progress.py b/tools/rsync_progress.py new file mode 100755 index 0000000..853ce31 --- /dev/null +++ b/tools/rsync_progress.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python3 +"""Render rsync's per-file output as a single updating status line. + +Fed the paths rsync reports with --out-format='%n', one per line. Prints one +line that rewrites itself, showing how far through the transfer is and which +album is currently going across, rather than either scrolling fifty thousand +filenames past or -- as rsync does while it builds its file list -- saying +nothing at all for several minutes. + +Falls back to periodic plain lines when stderr is not a terminal, so a log does +not fill up with carriage returns. +""" + +import argparse +import os +import shutil +import sys +import time + + +def album_of(path): + """Return "Artist / Album" for a mirror-relative path.""" + parts = [part for part in path.strip("/").split("/") if part] + if len(parts) >= 3: + return f"{parts[0]} / {parts[1]}" + if len(parts) == 2: + return parts[0] + return "" + + +def fit(text, width): + """Trim to the terminal, from the left: the album matters more than the artist.""" + if width <= 1 or len(text) <= width: + return text + return "…" + text[-(width - 1) :] + + +def render(done, total, label, width): + if total > 0: + share = min(100, done * 100 // total) + head = f"[{done:>6,} / {total:<6,} {share:>3}%] " + else: + head = f"[{done:>6,} files] " + return head + fit(label, max(0, width - len(head))) + + +def main(argv=None, stream=None, out=None): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--total", type=int, default=0, help="files expected") + parser.add_argument("--interval", type=float, default=0.1, help="seconds between redraws") + args = parser.parse_args(argv) + + stream = stream or sys.stdin + out = out or sys.stderr + interactive = out.isatty() + width = shutil.get_terminal_size((100, 24)).columns - 1 + + done = 0 + last_drawn = 0.0 + label = "" + for line in stream: + path = line.rstrip("\n") + # rsync reports directories too, with a trailing slash. They are not + # files and counting them would put the percentage past a hundred. + if not path or path.endswith("/"): + continue + done += 1 + label = album_of(path) or os.path.basename(path) + + now = time.monotonic() + if interactive: + if now - last_drawn >= args.interval: + out.write("\r\033[2K" + render(done, args.total, label, width)) + out.flush() + last_drawn = now + elif now - last_drawn >= 30: + out.write(render(done, args.total, label, width) + "\n") + out.flush() + last_drawn = now + + if interactive: + out.write("\r\033[2K") + out.write(render(done, args.total, label, width).rstrip() + "\n") + out.flush() + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index ca81d4c..9983f52 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -45,6 +45,11 @@ 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. +Progress is one line that rewrites itself, showing the album currently going +across and how far through the transfer is. Working the total out first means a +second pass over the tree, which is the price of a percentage that means +something; rsync's own is computed against a file list it is still building. + 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 @@ -146,26 +151,39 @@ fi # asking for them produces a screenful of errors and a non-zero exit. # --modify-window=2 because FAT stores mtimes to two-second resolution, without # which every file looks changed and the whole library is copied every time. +options=(--recursive --times --delete --modify-window=2) # --delete removes tracks whose source has gone, which is the point. It would # also remove everything on the device that the mirror does not contain -- and # if the destination is the card root that means /.rockbox, the Rockbox install # itself. Excluded paths are not deleted unless --delete-excluded is given, # which it never is here. -options=(--recursive --times --delete --modify-window=2 --human-readable --info=progress2) for owned in "/.rockbox" "/.scrobbler.log" "/.scrobbler.log.*" "/.playlist_control" \ "/System Volume Information" "/.Spotlight-V100" "/.Trashes" "/.fseventsd"; do options+=(--exclude "$owned") done -$dry_run && options+=(--dry-run --verbose) printf 'sync-to-ipod: %s -> %s\n' "$mirror" "$destination" >&2 -rsync "${options[@]}" "$mirror/" "$destination/" if $dry_run; then + rsync "${options[@]}" --dry-run --verbose "$mirror/" "$destination/" printf 'sync-to-ipod: dry run, nothing was written\n' >&2 exit 0 fi +# rsync says nothing at all while it builds its file list, which on fifty +# thousand files over USB is minutes of apparent hang. Counting first costs a +# second pass over the tree but means the transfer can show a real percentage +# rather than a number that grows as rsync discovers more work. +printf 'sync-to-ipod: working out what needs copying...\n' >&2 +total=$(rsync "${options[@]}" --dry-run --out-format='%n' "$mirror/" "$destination/" | + grep -cve '/$' || true) +printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 + +rsync "${options[@]}" --out-format='%n' "$mirror/" "$destination/" | + python3 "$here/rsync_progress.py" --total "$total" +status=${PIPESTATUS[0]} +[ "$status" -eq 0 ] || die "rsync exited $status" + sync if $unmount; then device=$(findmnt -no SOURCE --target "$destination") From 131c80f5debb2eb722a73f26fd54e82bf0cf7dce Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:40:32 +0100 Subject: [PATCH 3/7] feat: estimate the time remaining from bytes and observed rate rsync reports each file's size with %l as it completes, which is all an estimate needs: bytes done over time elapsed is the same arithmetic rsync would do internally, and requires nothing it does not already print. The scan pass now sums those sizes as well as counting files, so both a percentage and an estimate have a real denominator. The rate is measured over a trailing thirty seconds rather than the whole run, so it follows a device that slows down instead of averaging the slowdown away -- which for a card reader that thermally throttles, or a USB link that renegotiates after an hour, is the difference between a useful estimate and a reassuring one. Below two seconds no rate is reported at all. The first handful of files arrive microseconds apart, and dividing by that window produces a rate in the gigabytes per second and an estimate of zero, which is worse than showing nothing. Directory entries are excluded from the byte total as well as the file count. rsync reports them with a 4096 inode size, which across six thousand album directories is several megabytes of transfer that never happens. --- README.md | 17 +++-- tests/test_rsync_progress.py | 88 +++++++++++++++++++++++-- tools/rsync_progress.py | 121 ++++++++++++++++++++++++++++++++--- tools/sync-to-ipod.sh | 20 ++++-- 4 files changed, 219 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index e0853c5..29bde38 100644 --- a/README.md +++ b/README.md @@ -166,15 +166,24 @@ remove the Rockbox install. Progress is a single line that rewrites itself: ``` -[ 12,345 / 49,600 24%] King Gizzard & the Lizard Wizard / PetroDragonic Apoc… +[ 24%] 12,345/49,600 3.2 GiB/13.1 GiB 4.4 MiB/s ETA 38m12s King Gizzard / Petro… ``` +The estimate comes from rsync's `%l`, which gives each file's size as it +completes. Bytes done over time elapsed is the same arithmetic rsync would do, +and needs nothing it does not already print. The rate is measured over a +trailing thirty seconds rather than the whole run, so it follows a device that +slows down instead of averaging the slowdown away — and it is suppressed +entirely for the first two seconds, where the window is microseconds wide and +would report gigabytes per second. + rsync says nothing at all while it builds its file list, which on fifty thousand files over USB is minutes of apparent hang, and its own `progress2` percentage is computed against a list it has not finished discovering. So the -script counts first — a second pass over the tree, which is what a percentage -that means something costs — and renders the rest itself. Piped to a log it -prints a plain line every thirty seconds instead, with no carriage returns. +script counts first — files and bytes both, a second pass over the tree, which +is what a percentage and an estimate that mean something cost — and renders the +rest itself. Piped to a log it prints a plain line every thirty seconds +instead, with no carriage returns, and a summary at the end either way. The unmount is the point of doing this in a script. FAT32 has no journal and the device is reached through disk mode, so an interrupted write is corruption diff --git a/tests/test_rsync_progress.py b/tests/test_rsync_progress.py index 9a25fca..bea69d4 100644 --- a/tests/test_rsync_progress.py +++ b/tests/test_rsync_progress.py @@ -19,9 +19,13 @@ class Terminal(io.StringIO): return True -def run(lines, total=0, out=None): +def run(lines, total=0, out=None, bytes_expected=0): out = out or NotATerminal() - rsync_progress.main(["--total", str(total)], stream=io.StringIO(lines), out=out) + rsync_progress.main( + ["--total", str(total), "--bytes", str(bytes_expected)], + stream=io.StringIO(lines), + out=out, + ) return out.getvalue() @@ -40,15 +44,15 @@ def test_directories_are_not_counted(): """rsync reports them too, and counting them puts the percentage past 100.""" output = run("Artist/\nArtist/Album/\nArtist/Album/track.mp3\n", total=1) - assert "1 / 1" in output + assert "1/1" in output assert "100%" in output def test_the_percentage_tracks_the_total(): output = run("".join(f"A/B/{i}.mp3\n" for i in range(5)), total=10) - assert "5 / 10" in output - assert " 50%" in output + assert "5/10" in output + assert "50%" in output def test_without_a_total_it_counts_instead_of_guessing(): @@ -64,7 +68,7 @@ def test_a_final_line_is_always_printed(): output = run("A/B/one.mp3\n", total=1) assert output.endswith("\n") - assert "1 / 1" in output + assert "1/1" in output def test_nothing_transferred_still_reports(): @@ -102,3 +106,75 @@ def test_long_labels_are_trimmed_from_the_left(text, width, expected): if len(text) > width: assert trimmed.startswith("…") assert text.endswith(trimmed.lstrip("…")) + + +def test_the_size_and_path_are_parsed(): + assert rsync_progress.parse("5000 Artist/Album/Track.mp3\n") == ( + 5000, + "Artist/Album/Track.mp3", + ) + + +def test_a_filename_containing_spaces_survives(): + """Splitting on every space would lose most of the library.""" + assert rsync_progress.parse("1234 Artist/An Album/A Track With Spaces.mp3") == ( + 1234, + "Artist/An Album/A Track With Spaces.mp3", + ) + + +def test_a_bare_path_is_tolerated(): + """In case this is fed --out-format='%n' by something older.""" + assert rsync_progress.parse("Artist/Album/Track.mp3") == (0, "Artist/Album/Track.mp3") + + +def test_directory_sizes_do_not_inflate_the_total(): + """rsync reports directories with a 4096 inode size, which is several + megabytes of nothing across six thousand albums.""" + output = run("4096 Artist/\n4096 Artist/Album/\n5000 Artist/Album/t.mp3\n", total=1) + + assert "4.9 KiB" in output + assert "12" not in output.split("Artist")[0] + + +def test_a_rate_is_not_reported_until_it_means_something(): + """The first files arrive microseconds apart and would give a rate in the + gigabytes per second and an ETA of zero.""" + rate = rsync_progress.Rate() + rate.add(100.0, 0) + rate.add(100.5, 5_000_000) + + assert rate.per_second() == 0.0 + + +def test_a_rate_over_a_long_enough_window_is_reported(): + rate = rsync_progress.Rate() + rate.add(100.0, 0) + rate.add(110.0, 10_000_000) + + assert rate.per_second() == pytest.approx(1_000_000) + + +def test_the_window_forgets_the_distant_past(): + """So the estimate follows a device that slows down rather than averaging + the slowdown away.""" + rate = rsync_progress.Rate(window=30.0) + for second in range(0, 100, 10): + rate.add(float(second), second * 1_000_000) + rate.add(200.0, 100_000_000) + + assert rate.samples[0][0] >= 90.0 + + +@pytest.mark.parametrize( + ("seconds", "expected"), + [(0, "0s"), (45, "45s"), (60, "1m00s"), (1092, "18m12s"), (7500, "2h05m")], +) +def test_durations_read_without_arithmetic(seconds, expected): + assert rsync_progress.human_duration(seconds) == expected + + +def test_a_summary_is_printed_at_the_end(): + output = run("5000000 A/B/one.mp3\n", total=1, bytes_expected=5000000) + + assert "copied 4.8 MiB in" in output diff --git a/tools/rsync_progress.py b/tools/rsync_progress.py index 853ce31..e5be8d8 100755 --- a/tools/rsync_progress.py +++ b/tools/rsync_progress.py @@ -1,7 +1,10 @@ #!/usr/bin/env python3 """Render rsync's per-file output as a single updating status line. -Fed the paths rsync reports with --out-format='%n', one per line. Prints one +Fed the size and path rsync reports with --out-format='%l %n', one per line. +The size is what makes an estimate possible: rsync's own rate is not exposed +per file, but bytes completed over time elapsed is the same arithmetic and +needs nothing rsync does not already print. Prints one line that rewrites itself, showing how far through the transfer is and which album is currently going across, rather than either scrolling fifty thousand filenames past or -- as rsync does while it builds its file list -- saying @@ -12,11 +15,76 @@ not fill up with carriage returns. """ import argparse +import collections import os import shutil import sys import time +# The estimate is taken over a trailing window rather than the whole run, so it +# follows a device that slows down instead of averaging the slowdown away. +RATE_WINDOW_SECONDS = 30.0 + +# Below this the window is too narrow to divide by: the first few files arrive +# in microseconds and produce a rate in the gigabytes per second, and an ETA of +# nothing at all. Better to show neither until the figure means something. +RATE_MINIMUM_SPAN_SECONDS = 2.0 + + +def parse(line): + """Return (bytes, path) for one line of rsync output. + + Tolerates a bare path, in case someone runs this against --out-format='%n'. + """ + line = line.rstrip("\n") + size, separator, path = line.partition(" ") + if separator and size.isdigit(): + return int(size), path + return 0, line + + +def human_bytes(count): + size = float(count) + for unit in ("B", "KiB", "MiB", "GiB", "TiB"): + if size < 1024 or unit == "TiB": + return f"{size:.1f} {unit}" + size /= 1024 + + +def human_duration(seconds): + """Return a duration nobody has to do arithmetic on.""" + seconds = int(seconds) + if seconds < 60: + return f"{seconds}s" + if seconds < 3600: + return f"{seconds // 60}m{seconds % 60:02d}s" + return f"{seconds // 3600}h{(seconds % 3600) // 60:02d}m" + + +class Rate: + """Bytes per second over a trailing window.""" + + def __init__(self, window=RATE_WINDOW_SECONDS): + self.window = window + self.samples = collections.deque() + + def add(self, when, total_bytes): + self.samples.append((when, total_bytes)) + while len(self.samples) > 2 and when - self.samples[0][0] > self.window: + self.samples.popleft() + + def per_second(self): + if len(self.samples) < 2: + return 0.0 + (first_time, first_bytes), (last_time, last_bytes) = ( + self.samples[0], + self.samples[-1], + ) + elapsed = last_time - first_time + if elapsed < RATE_MINIMUM_SPAN_SECONDS: + return 0.0 + return (last_bytes - first_bytes) / elapsed + def album_of(path): """Return "Artist / Album" for a mirror-relative path.""" @@ -35,18 +103,29 @@ def fit(text, width): return "…" + text[-(width - 1) :] -def render(done, total, label, width): +def render(done, total, copied, expected, rate, label, width): + """Build the status line, giving whatever room is left to the album.""" if total > 0: share = min(100, done * 100 // total) - head = f"[{done:>6,} / {total:<6,} {share:>3}%] " + head = f"[{share:>3}%] {done:,}/{total:,}" else: - head = f"[{done:>6,} files] " + head = f"[{done:,} files]" + + if expected > 0: + head += f" {human_bytes(copied)}/{human_bytes(expected)}" + if rate > 0: + head += f" {human_bytes(rate)}/s" + remaining = expected - copied + if remaining > 0: + head += f" ETA {human_duration(remaining / rate)}" + head += " " return head + fit(label, max(0, width - len(head))) def main(argv=None, stream=None, out=None): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--total", type=int, default=0, help="files expected") + parser.add_argument("--bytes", type=int, default=0, help="bytes expected") parser.add_argument("--interval", type=float, default=0.1, help="seconds between redraws") args = parser.parse_args(argv) @@ -56,31 +135,53 @@ def main(argv=None, stream=None, out=None): width = shutil.get_terminal_size((100, 24)).columns - 1 done = 0 + copied = 0 + rate = Rate() + started = time.monotonic() + rate.add(started, 0) last_drawn = 0.0 label = "" + for line in stream: - path = line.rstrip("\n") - # rsync reports directories too, with a trailing slash. They are not - # files and counting them would put the percentage past a hundred. + size, path = parse(line) + # rsync reports directories too, with a trailing slash and an inode + # size. Counting them puts the percentage past a hundred and the byte + # total well over what will actually be transferred. if not path or path.endswith("/"): continue done += 1 + copied += size label = album_of(path) or os.path.basename(path) now = time.monotonic() + rate.add(now, copied) if interactive: if now - last_drawn >= args.interval: - out.write("\r\033[2K" + render(done, args.total, label, width)) + out.write( + "\r\033[2K" + + render(done, args.total, copied, args.bytes, rate.per_second(), + label, width) + ) out.flush() last_drawn = now elif now - last_drawn >= 30: - out.write(render(done, args.total, label, width) + "\n") + out.write( + render(done, args.total, copied, args.bytes, rate.per_second(), label, width) + + "\n" + ) out.flush() last_drawn = now + elapsed = max(1e-9, time.monotonic() - started) if interactive: out.write("\r\033[2K") - out.write(render(done, args.total, label, width).rstrip() + "\n") + summary = render(done, args.total, copied, args.bytes, 0, label, width).rstrip() + out.write(f"{summary}\n") + if copied: + out.write( + f"copied {human_bytes(copied)} in {human_duration(elapsed)}" + f" at {human_bytes(copied / elapsed)}/s\n" + ) out.flush() return 0 diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index 9983f52..ae7ae5f 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -46,9 +46,10 @@ catches an unmounted device: /media/IPOD/Music then resolves to the host's own root filesystem, and this refuses to empty that. Progress is one line that rewrites itself, showing the album currently going -across and how far through the transfer is. Working the total out first means a -second pass over the tree, which is the price of a percentage that means -something; rsync's own is computed against a file list it is still building. +across, how far through the transfer is, the rate, and an estimate of what is +left. Working the totals out first means a second pass over the tree, which is +the price of figures that mean something; rsync's own percentage is computed +against a file list it is still building. Reach the device with the Apple firmware's disk mode: Menu+Select to reboot, then immediately Select+Play. Power off afterwards by holding Play. @@ -175,12 +176,17 @@ fi # second pass over the tree but means the transfer can show a real percentage # rather than a number that grows as rsync discovers more work. printf 'sync-to-ipod: working out what needs copying...\n' >&2 -total=$(rsync "${options[@]}" --dry-run --out-format='%n' "$mirror/" "$destination/" | - grep -cve '/$' || true) +# %l is the file's size, which is what makes an estimate possible. Directories +# are dropped: rsync reports those too, with an inode size that would inflate +# the total by several megabytes of nothing. +counted=$(rsync "${options[@]}" --dry-run --out-format='%l %n' "$mirror/" "$destination/" | + awk '!/\/$/ { files++; bytes += $1 } END { print files + 0, bytes + 0 }') +total=${counted% *} +total_bytes=${counted#* } printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 -rsync "${options[@]}" --out-format='%n' "$mirror/" "$destination/" | - python3 "$here/rsync_progress.py" --total "$total" +rsync "${options[@]}" --out-format='%l %n' "$mirror/" "$destination/" | + python3 "$here/rsync_progress.py" --total "$total" --bytes "$total_bytes" status=${PIPESTATUS[0]} [ "$status" -eq 0 ] || die "rsync exited $status" From d6ef70922cb4fa24381ebed62803c5cdd7e0e1b9 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:44:53 +0100 Subject: [PATCH 4/7] perf: cut round trips over a network mount, and allow skipping the count The transfer is metadata-bound rather than throughput-bound. Fifty thousand files is fifty thousand round trips, and the counting pass added for the percentage doubles that. --whole-file is already implied when both ends are local paths, which an SMB or FAT mount is, but stating it records that the delta algorithm is deliberately unwanted here: it would read every destination file back over USB to checksum it, in order to avoid resending an MP3 that has changed in its entirety anyway. --omit-dir-times drops one setattr per directory. Across six thousand album folders on a FAT card that is six thousand operations spent on timestamps nothing reads. -Q skips the counting pass. The percentage and the estimate are worth a second walk of a local tree and frequently are not worth one of a network mount, so that is now a choice rather than a fixed cost. The README covers the part that is not an rsync flag at all: SMB defaults to a one second attribute cache, so nearly every stat goes to the wire, twice. An actimeo of sixty on the mount does more than any of the above, and closes most of the gap that would otherwise argue for moving to NFS. --- README.md | 24 ++++++++++++++++++++++ tests/test_sync_to_ipod.py | 30 ++++++++++++++++++++++++++++ tools/sync-to-ipod.sh | 41 ++++++++++++++++++++++++++++---------- 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 29bde38..e2275e6 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,30 @@ is what a percentage and an estimate that mean something cost — and renders th rest itself. Piped to a log it prints a plain line every thirty seconds instead, with no carriage returns, and a summary at the end either way. +### Making it faster over a network mount + +The transfer is metadata-bound, not throughput-bound: 49,600 files means 49,600 +round trips, and the counting pass doubles that. In rough order of what it is +worth doing: + +| Lever | Why | +| ----- | --- | +| Mount the source with `actimeo=60,cache=loose` | SMB defaults to a **one second** attribute cache, so nearly every `stat` goes to the wire — twice, once per pass. This is the single biggest change and it is a mount option, not an rsync flag. | +| Put the card in a reader for the first load | USB 2.0 through an iPod in disk mode is the floor for the destination. No amount of source tuning gets past it. | +| `-Q` | Skips the counting pass entirely. Costs the percentage and the estimate, saves a whole walk of the tree. | +| `--whole-file`, `--omit-dir-times` | Already set. The first stops rsync checksumming destination files it is about to overwrite whole; the second drops a setattr per directory, 6,150 of them. | + +**NFS instead of SMB** is worth trying but is not the big win it looks like. +Its attribute caching defaults are far more generous than SMB's — `acregmax` of +sixty seconds against `actimeo=1` — which is precisely the gap that +`actimeo=60` closes on the mount you already have. Bulk read throughput between +the two is much of a muchness on a gigabit link. Try the mount option first; it +is one line and needs no change on the NAS. + +And if the destination is the iPod rather than a card reader, none of this +matters much: the source can feed data faster than USB 2.0 through an iPod will +take it either way. + The unmount is the point of doing this in a script. FAT32 has no journal and the device is reached through disk mode, so an interrupted write is corruption that needs `fsck.vfat` from another machine. diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py index 2aef186..386599a 100644 --- a/tests/test_sync_to_ipod.py +++ b/tests/test_sync_to_ipod.py @@ -174,3 +174,33 @@ def test_the_help_says_how_to_reach_and_leave_disk_mode(): assert "Menu+Select" in help_text assert "holding Play" in help_text + + +def test_quick_mode_skips_the_counting_pass(mirror, tmp_path): + """Over SMB the walk is the expensive part, and doing it twice for a + percentage is not always the trade you want.""" + destination = tmp_path / "dest" + destination.mkdir() + + result = run("-f", "-S", "-U", "-Q", str(mirror), str(destination)) + + assert result.returncode == 0, result.stderr + assert "skipping the count" in result.stderr + assert "files to copy" not in result.stderr + assert (destination / "Album" / "track.mp3").is_file() + + +def test_the_delta_algorithm_is_disabled(mirror, tmp_path): + """It would read every destination file back over USB to checksum it, to + avoid resending an MP3 that has changed in its entirety anyway.""" + script = SCRIPT.read_text() + + assert "--whole-file" in script + + +def test_directory_timestamps_are_not_set(mirror, tmp_path): + """One setattr round trip per directory, across six thousand albums, to set + timestamps nothing reads.""" + script = SCRIPT.read_text() + + assert "--omit-dir-times" in script diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index ae7ae5f..71f7c8f 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -22,6 +22,8 @@ usage() { usage: sync-to-ipod.sh [options] -n dry run; show what would change and touch nothing + -Q skip the counting pass; no percentage or estimate, but one less walk + of the source tree, which over SMB is the expensive part -f copy even if the FAT32 check finds unacceptable paths -S skip submitting the Rockbox scrobbler log to Last.fm -U leave the destination mounted afterwards @@ -58,15 +60,17 @@ USAGE } dry_run=false +quick=false force=false unmount=true scrobble=true for argument in "$@"; do [ "$argument" = "--help" ] && usage help done -while getopts ":nfSUh" option; do +while getopts ":nQfSUh" option; do case "$option" in n) dry_run=true ;; + Q) quick=true ;; f) force=true ;; S) scrobble=false ;; U) unmount=false ;; @@ -152,7 +156,16 @@ fi # asking for them produces a screenful of errors and a non-zero exit. # --modify-window=2 because FAT stores mtimes to two-second resolution, without # which every file looks changed and the whole library is copied every time. -options=(--recursive --times --delete --modify-window=2) +# --whole-file is already the default when both ends are local paths, and an +# SMB or FAT mount counts as one, but stating it documents that the delta +# algorithm is deliberately not wanted: it would read every destination file +# back over USB to compute a checksum, to save sending an MP3 that has changed +# entirely anyway. +# +# --omit-dir-times drops a setattr round trip per directory. Across six +# thousand album folders on a FAT card that is six thousand operations to set +# timestamps nothing reads. +options=(--recursive --times --delete --modify-window=2 --whole-file --omit-dir-times) # --delete removes tracks whose source has gone, which is the point. It would # also remove everything on the device that the mirror does not contain -- and # if the destination is the card root that means /.rockbox, the Rockbox install @@ -175,15 +188,21 @@ fi # thousand files over USB is minutes of apparent hang. Counting first costs a # second pass over the tree but means the transfer can show a real percentage # rather than a number that grows as rsync discovers more work. -printf 'sync-to-ipod: working out what needs copying...\n' >&2 -# %l is the file's size, which is what makes an estimate possible. Directories -# are dropped: rsync reports those too, with an inode size that would inflate -# the total by several megabytes of nothing. -counted=$(rsync "${options[@]}" --dry-run --out-format='%l %n' "$mirror/" "$destination/" | - awk '!/\/$/ { files++; bytes += $1 } END { print files + 0, bytes + 0 }') -total=${counted% *} -total_bytes=${counted#* } -printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 +total=0 +total_bytes=0 +if $quick; then + printf 'sync-to-ipod: skipping the count; no percentage or estimate\n' >&2 +else + printf 'sync-to-ipod: working out what needs copying...\n' >&2 + # %l is the file's size, which is what makes an estimate possible. + # Directories are dropped: rsync reports those too, with an inode size that + # would inflate the total by several megabytes of nothing. + counted=$(rsync "${options[@]}" --dry-run --out-format='%l %n' "$mirror/" "$destination/" | + awk '!/\/$/ { files++; bytes += $1 } END { print files + 0, bytes + 0 }') + total=${counted% *} + total_bytes=${counted#* } + printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 +fi rsync "${options[@]}" --out-format='%l %n' "$mirror/" "$destination/" | python3 "$here/rsync_progress.py" --total "$total" --bytes "$total_bytes" From 8228c81b5cce3fd83c3863bc97f74e5f96a24f52 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:51:42 +0100 Subject: [PATCH 5/7] fix: flush and unmount even when the sync is interrupted rsync itself is safe under interruption. It writes to a hidden temporary file and renames it into place only once complete, and by default deletes any partial file when interrupted -- verified both in the manual and by killing a transfer and inspecting what was left, which was nothing. --partial is deliberately absent and there is now a test asserting it stays that way. An unclean kill can leave a hidden .track.mp3.XXXXXX behind; it is unplayable, it is not in the source, and the next run's --delete removes it. The script was not safe. Ctrl-C killed it before the sync and the unmount, leaving a journal-less FAT filesystem holding dirty buffers -- which is the exact corruption the script exists to prevent, arrived at by the most likely route a person would take. INT and TERM are now trapped. Both the normal path and the interrupt path call the same finish function, so the flush and the unmount cannot drift apart, and an interrupted run exits 130 rather than pretending to have succeeded. The test is structural rather than timed. Reproducing a mid-transfer signal needs a payload large enough to be slow, and a test that depends on winning a race is a test that fails in CI for reasons that have nothing to do with the code. The behaviour was verified by hand: SIGTERM mid-transfer gave exit 130, the flush ran, and the destination held no short files and no leftover temporaries. --- README.md | 21 +++++++++++++++++ tests/test_sync_to_ipod.py | 24 ++++++++++++++++++++ tools/sync-to-ipod.sh | 46 +++++++++++++++++++++++++++----------- 3 files changed, 78 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index e2275e6..cab2446 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,27 @@ is what a percentage and an estimate that mean something cost — and renders th rest itself. Piped to a log it prints a plain line every thirty seconds instead, with no carriage returns, and a summary at the end either way. +### If the sync is interrupted + +No partially copied track is ever left under a name Rockbox would play. rsync +writes to a hidden temporary file and only renames it into place once the file +is complete, and *"by default, rsync will delete any partially transferred file +if the transfer is interrupted"*. `--partial` is deliberately not used, and +there is a test asserting it never will be. + +After an unclean kill or a power cut a hidden `.track.mp3.XXXXXX` can survive. +It is not playable, it is not in the source, and the next run's `--delete` +removes it. + +The real risk is not rsync's, it is the filesystem's: FAT32 has no journal, so +buffers that never reach the card are corruption. The script therefore traps +`INT` and `TERM` and still flushes and unmounts on the way out, exiting 130. +Both the normal path and the interrupt path call the same function, so they +cannot drift apart. + +The one case nothing can help with is pulling the cable or the card mid-write. +Wait for the unmount. + ### Making it faster over a network mount The transfer is metadata-bound, not throughput-bound: 49,600 files means 49,600 diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py index 386599a..1813068 100644 --- a/tests/test_sync_to_ipod.py +++ b/tests/test_sync_to_ipod.py @@ -204,3 +204,27 @@ def test_directory_timestamps_are_not_set(mirror, tmp_path): script = SCRIPT.read_text() assert "--omit-dir-times" in script + + +def test_an_interrupt_is_trapped_so_the_filesystem_is_flushed(): + """Ctrl-C during a transfer would otherwise skip the sync and the unmount, + leaving a journal-less FAT filesystem with dirty buffers -- which is the + corruption this script exists to prevent. + + Structural rather than timed: reproducing a mid-transfer signal needs a + payload large enough to be slow, and a test that depends on losing a race + is a test that fails in CI for no reason. + """ + script = SCRIPT.read_text() + + assert "trap interrupted INT TERM" in script + assert "exit 130" in script + + +def test_the_flush_and_unmount_happen_on_every_exit_path(): + script = SCRIPT.read_text() + + # Both the normal path and the interrupt path go through the same function, + # so one cannot drift from the other. + assert script.count("finish\n") >= 2 + assert "--partial" not in script, "rsync must delete partial files, not keep them" diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index 71f7c8f..ed119d4 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -204,21 +204,41 @@ else printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 fi +# Flushing and unmounting is the whole reason this is a script, so it has to +# happen on the way out whichever way that is. Ctrl-C during a transfer would +# otherwise leave a FAT filesystem with dirty buffers and no journal, which is +# the corruption this exists to avoid. +finish() { + sync + if $unmount; then + device=$(findmnt -no SOURCE --target "$destination" 2>/dev/null || true) + if [ -n "$device" ]; then + printf 'sync-to-ipod: unmounting %s\n' "$device" >&2 + if command -v udisksctl >/dev/null 2>&1; then + udisksctl unmount -b "$device" || umount -- "$destination" || true + else + umount -- "$destination" || true + fi + printf 'sync-to-ipod: safe to disconnect\n' >&2 + fi + else + printf 'sync-to-ipod: still mounted; unmount before disconnecting\n' >&2 + fi +} + +interrupted() { + trap - INT TERM + printf '\nsync-to-ipod: interrupted -- rsync leaves no partial files, but the\n' >&2 + printf 'sync-to-ipod: filesystem still needs flushing before you pull anything\n' >&2 + finish + exit 130 +} + +trap interrupted INT TERM + rsync "${options[@]}" --out-format='%l %n' "$mirror/" "$destination/" | python3 "$here/rsync_progress.py" --total "$total" --bytes "$total_bytes" status=${PIPESTATUS[0]} [ "$status" -eq 0 ] || die "rsync exited $status" -sync -if $unmount; then - device=$(findmnt -no SOURCE --target "$destination") - printf 'sync-to-ipod: unmounting %s\n' "$device" >&2 - if command -v udisksctl >/dev/null 2>&1; then - udisksctl unmount -b "$device" - else - umount -- "$destination" - fi - printf 'sync-to-ipod: safe to disconnect\n' >&2 -else - printf 'sync-to-ipod: still mounted; unmount before disconnecting\n' >&2 -fi +finish From 9091c4d049e7b246090087a0611c50f09ec6a220 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:54:19 +0100 Subject: [PATCH 6/7] docs: stop overstating the risk of interrupting a sync The README claimed that interrupting left buffers unwritten and therefore corruption. That is wrong. The kernel flushes dirty pages within dirty_expire_centisecs, thirty seconds by default, and umount syncs before it returns, so losing data requires interrupting and pulling the card inside that window and skipping the unmount. The trap is still worth having, for a smaller and more honest reason: it removes a manual step and makes the exit deterministic, so the same "safe to disconnect" appears whichever way the run ends. What actually loses data is pulling the card without unmounting at all, which has nothing to do with whether the transfer was interrupted. --- README.md | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index cab2446..b2f8c52 100644 --- a/README.md +++ b/README.md @@ -197,14 +197,20 @@ After an unclean kill or a power cut a hidden `.track.mp3.XXXXXX` can survive. It is not playable, it is not in the source, and the next run's `--delete` removes it. -The real risk is not rsync's, it is the filesystem's: FAT32 has no journal, so -buffers that never reach the card are corruption. The script therefore traps -`INT` and `TERM` and still flushes and unmounts on the way out, exiting 130. -Both the normal path and the interrupt path call the same function, so they -cannot drift apart. +Interrupting does not, by itself, endanger the filesystem. The kernel flushes +dirty pages within `dirty_expire_centisecs` — thirty seconds by default — and +`umount` always syncs before it returns. Losing data needs you to interrupt, +*and* pull the card inside that window, *and* skip the unmount. -The one case nothing can help with is pulling the cable or the card mid-write. -Wait for the unmount. +The script still traps `INT` and `TERM` and flushes and unmounts on the way +out, exiting 130. Not because a Ctrl-C is dangerous, but because it removes the +manual step and makes the exit deterministic — you get the same "safe to +disconnect" either way, rather than having to remember which path you took. +Both paths call the same function, so they cannot drift apart. + +What genuinely does lose data is pulling the cable or the card without +unmounting at all, interrupted or not. FAT32 has no journal. Wait for the +unmount line. ### Making it faster over a network mount From c99b423b72cb345231b5d3709a2b817d2966ba30 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Wed, 26 Aug 2026 13:20:20 +0100 Subject: [PATCH 7/7] feat: rebuild the Rockbox database during the sync, off the mirror The on-device database commit does not work at this library size. It sorts the whole index in whatever memory core_alloc_maximum() can scrape together, and on fifty thousand tracks it runs for hours or aborts with a data abort -- observed across several builds including stable. Rockbox ships a host-side builder for exactly this, and the sync is the moment the library changes, so it belongs here. MUSIC_MIRROR_DATABASE_TOOL points at it; the step is skipped with a note when unset, as the scrobbler step is. The scan runs against a scratch root -- a real .rockbox beside a symlink standing in for wherever the music lands on the device -- so the paths recorded are the ones Rockbox will look up, while the bytes are read from the mirror rather than over USB. Only the dozen .tcd files cross to the card. Verified: scanning through the symlink records /Music/... paths while reading from somewhere else entirely. The scratch root is kept between runs because the builder is incremental. A second pass over unchanged files performs no metadata reads and finishes in a fraction of a second, so only the first build pays the full cost. That cost, measured rather than guessed: about 49 reads and 43 seeks per file, the parser probing the head for ID3v2 and the tail for ID3v1. Two thousand files in half a second on local disk. Over SMB the opens and the head/tail split are real round trips, making a first full scan minutes rather than seconds -- still preferable to an on-device commit that does not finish. There is nothing to parallelise: the tool is single-threaded and two instances cannot produce one database. --- README.md | 38 ++++++++++++++++++++++++ tests/test_sync_to_ipod.py | 43 +++++++++++++++++++++++++++ tools/sync-to-ipod.sh | 61 +++++++++++++++++++++++++++++++++++++- 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b2f8c52..9e20d89 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,44 @@ is what a percentage and an estimate that mean something cost — and renders th rest itself. Piped to a log it prints a plain line every thirty seconds instead, with no carriage returns, and a summary at the end either way. +### The Rockbox database + +Point `MUSIC_MIRROR_DATABASE_TOOL` at Rockbox's host-side builder and the sync +rebuilds the database itself, so it never has to happen on the device. + +```sh +git clone --depth 1 https://github.com/Rockbox/rockbox.git +cd rockbox && mkdir build-db && cd build-db +../tools/configure --target=ipodvideo --type=d && make -j$(nproc) +``` + +It needs a native compiler and SDL2 development headers, not the ARM +cross-toolchain, and `tools/configure` detects `__aarch64__` correctly. On a +distribution without `/usr/bin/perl` or `gcc-ar` — NixOS, say — patch the +shebangs in `tools/*.pl` and pass `AR=ar`. + +Building it here rather than on the device is not merely faster. The on-device +commit sorts the whole index in whatever memory `core_alloc_maximum()` can +scrape together; on a fifty-thousand-track library it runs for hours or aborts +outright. + +The scan runs against a scratch root — a real `.rockbox` beside a symlink +standing in for wherever the music lands on the device — so the paths recorded +are the ones Rockbox will look up, while the **bytes are read from the mirror +rather than over USB**. Only the dozen `.tcd` files cross to the card. + +Cost, measured: the parser makes about 49 reads and 43 seeks per file, probing +the head for ID3v2 and the tail for ID3v1. On a local disk that is 2,000 files +in half a second. Over SMB, readahead absorbs most of the reads but the opens +and the head/tail split are real round trips, so a first full scan is minutes +rather than seconds. It is a one-time cost: the builder is incremental, and the +scratch root is kept between runs, so a later pass over unchanged files does no +metadata reads at all. + +If minutes is still too many, run the builder where the mirror is local — on +the NAS — and copy the `.tcd` files across. There is nothing to parallelise: +the tool is single-threaded, and two instances cannot produce one database. + ### If the sync is interrupted No partially copied track is ever left under a name Rockbox would play. rsync diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py index 1813068..c0b4e82 100644 --- a/tests/test_sync_to_ipod.py +++ b/tests/test_sync_to_ipod.py @@ -228,3 +228,46 @@ def test_the_flush_and_unmount_happen_on_every_exit_path(): # so one cannot drift from the other. assert script.count("finish\n") >= 2 assert "--partial" not in script, "rsync must delete partial files, not keep them" + + +def test_the_database_step_is_skipped_without_a_tool(mirror, tmp_path, monkeypatch): + """Opt-in, like the scrobbler: absent configuration is not an error.""" + destination = tmp_path / "dest" + destination.mkdir() + monkeypatch.delenv("MUSIC_MIRROR_DATABASE_TOOL", raising=False) + + result = run("-f", "-S", "-U", str(mirror), str(destination)) + + assert result.returncode == 0, result.stderr + assert "no database tool configured" in result.stderr + + +def test_a_missing_database_tool_is_refused(mirror, tmp_path, monkeypatch): + destination = tmp_path / "dest" + destination.mkdir() + monkeypatch.setenv("MUSIC_MIRROR_DATABASE_TOOL", str(tmp_path / "nonexistent")) + + result = run("-f", "-S", "-U", str(mirror), str(destination)) + + assert result.returncode == 1 + assert "not executable" in result.stderr + + +def test_the_database_step_can_be_skipped(mirror, tmp_path, monkeypatch): + destination = tmp_path / "dest" + destination.mkdir() + monkeypatch.setenv("MUSIC_MIRROR_DATABASE_TOOL", str(tmp_path / "nonexistent")) + + result = run("-f", "-S", "-U", "-B", str(mirror), str(destination)) + + assert result.returncode == 0, result.stderr + assert "not executable" not in result.stderr + + +def test_the_scan_reads_from_the_mirror_not_the_device(): + """The whole point: tags come off the mirror, only the .tcd files go over + USB. Reading 49,600 files through an iPod's USB bridge is the slow path.""" + script = SCRIPT.read_text() + + assert 'ln -s "$mirror"' in script + assert 'cd "$scratch"' in script diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index ed119d4..9b7a0fc 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -26,8 +26,16 @@ usage: sync-to-ipod.sh [options] of the source tree, which over SMB is the expensive part -f copy even if the FAT32 check finds unacceptable paths -S skip submitting the Rockbox scrobbler log to Last.fm + -B skip rebuilding the Rockbox database -U leave the destination mounted afterwards +Rebuilding the database needs MUSIC_MIRROR_DATABASE_TOOL pointing at Rockbox's +host-side builder (tools/database, built with ./tools/configure --type=d). It +is skipped with a note when unset. The scan reads tags from the mirror rather +than from the device, so it costs seconds rather than the hours an on-device +commit takes -- and on a large library the on-device commit may not finish at +all. + 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. @@ -64,15 +72,17 @@ quick=false force=false unmount=true scrobble=true +database=true for argument in "$@"; do [ "$argument" = "--help" ] && usage help done -while getopts ":nQfSUh" option; do +while getopts ":nQfSBUh" option; do case "$option" in n) dry_run=true ;; Q) quick=true ;; f) force=true ;; S) scrobble=false ;; + B) database=false ;; U) unmount=false ;; h) usage help ;; *) usage ;; @@ -241,4 +251,53 @@ rsync "${options[@]}" --out-format='%l %n' "$mirror/" "$destination/" | status=${PIPESTATUS[0]} [ "$status" -eq 0 ] || die "rsync exited $status" +# Rockbox reads its database from .tcd files in .rockbox. Building them here +# rather than on the device is not just faster: the on-device commit sorts the +# whole index in whatever memory it can scrape together, and on a large library +# it runs for hours or dies outright. +# +# The scan reads tags through a scratch root -- a real .rockbox beside a symlink +# standing in for where the music lands on the device -- so the paths recorded +# match what Rockbox will look up, while the bytes are read from the mirror +# instead of over USB. The scratch is kept between runs because the builder is +# incremental: a second pass over unchanged files does no work at all. +rebuild_database() { + local tool=${MUSIC_MIRROR_DATABASE_TOOL:-} + if [ -z "$tool" ]; then + printf 'sync-to-ipod: no database tool configured, skipping the database\n' >&2 + return 0 + fi + [ -x "$tool" ] || die "$tool is not executable" + + local device_rockbox="$mounted_on/.rockbox" + if [ ! -d "$device_rockbox" ]; then + printf 'sync-to-ipod: no .rockbox on the device, skipping the database\n' >&2 + return 0 + fi + + local scratch="${XDG_CACHE_HOME:-$HOME/.cache}/music-mirror/database" + mkdir -p "$scratch/.rockbox" + + # Rebuild the symlink layout each time; the mirror path or the device + # prefix may have changed since the last run. + find "$scratch" -maxdepth 1 -type l -delete + if [ "$device_prefix" = "/" ]; then + ln -s "$mirror"/* "$scratch/" 2>/dev/null || true + else + local under=${device_prefix#/} + rm -rf "${scratch:?}/${under%%/*}" + mkdir -p "$scratch/$(dirname "$under")" + ln -s "$mirror" "$scratch/$under" + fi + + printf 'sync-to-ipod: building the database from the mirror...\n' >&2 + ( cd "$scratch" && "$tool" ) >/dev/null || die "the database build failed" + + cp -- "$scratch"/.rockbox/*.tcd "$device_rockbox/" || + die "could not copy the database onto the device" + printf 'sync-to-ipod: database copied to %s\n' "$device_rockbox" >&2 +} + +$database && rebuild_database + finish