From b2172145e7528324e2e7d9bf7f0910b2fd4e51f5 Mon Sep 17 00:00:00 2001 From: Daniel Arroyo Date: Fri, 17 Jul 2026 18:40:16 -0400 Subject: [PATCH] Always copy source directory contents, not the directory itself --- internal/syncengine/rsync_runner.go | 17 ++++- internal/syncengine/rsync_runner_test.go | 86 ++++++++++++++++++++++-- web/src/pages/SyncPairs.tsx | 6 ++ 3 files changed, 101 insertions(+), 8 deletions(-) diff --git a/internal/syncengine/rsync_runner.go b/internal/syncengine/rsync_runner.go index ae5bcb0..259e1fe 100644 --- a/internal/syncengine/rsync_runner.go +++ b/internal/syncengine/rsync_runner.go @@ -69,15 +69,28 @@ func (r *RsyncRunner) buildArgs(pair *SyncPairConfig) []string { args = append(args, "--delete") } + src := ensureDirSlash(pair.Source) if pair.Direction == "pull" { - args = append(args, pair.Dest, pair.Source) + args = append(args, pair.Dest, src) } else { - args = append(args, pair.Source, pair.Dest) + args = append(args, src, pair.Dest) } return args } +// ensureDirSlash guarantees the source path is treated by rsync as a +// directory whose contents are copied, regardless of whether the user +// supplied a trailing slash. This avoids the common foot-gun where +// "rsync host:/path/series /dest/" creates /dest/series/ nested +// inside an extra "series" subdirectory. +func ensureDirSlash(p string) string { + if strings.HasSuffix(p, "/") { + return p + } + return p + "/" +} + type MachineKeys struct { Host string Port int diff --git a/internal/syncengine/rsync_runner_test.go b/internal/syncengine/rsync_runner_test.go index 5e6c39d..29fbb71 100644 --- a/internal/syncengine/rsync_runner_test.go +++ b/internal/syncengine/rsync_runner_test.go @@ -40,8 +40,8 @@ func TestBuildArgs_Push(t *testing.T) { if args[0] != "-aP" { t.Errorf("first flag = %q, want %q", args[0], "-aP") } - if args[len(args)-2] != "/local/src" { - t.Errorf("source = %q, want %q", args[len(args)-2], "/local/src") + if args[len(args)-2] != "/local/src/" { + t.Errorf("source = %q, want %q (auto-appended trailing slash)", args[len(args)-2], "/local/src/") } if args[len(args)-1] != "admin@10.5.0.144:/remote/dst" { t.Errorf("dest = %q, want %q", args[len(args)-1], "admin@10.5.0.144:/remote/dst") @@ -61,8 +61,8 @@ func TestBuildArgs_Pull(t *testing.T) { if args[len(args)-2] != "/local/dst" { t.Errorf("pull: second-to-last (dest) = %q, want %q", args[len(args)-2], "/local/dst") } - if args[len(args)-1] != "admin@10.5.0.144:/remote/src" { - t.Errorf("pull: last (source) = %q, want %q", args[len(args)-1], "admin@10.5.0.144:/remote/src") + if args[len(args)-1] != "admin@10.5.0.144:/remote/src/" { + t.Errorf("pull: last (source) = %q, want %q (auto-appended trailing slash)", args[len(args)-1], "admin@10.5.0.144:/remote/src/") } } @@ -162,8 +162,8 @@ func TestRunRemote_PushSrcAndDestStayAsIs(t *testing.T) { } srcArg, dstArg := tc.build() - if srcArg != "/share/homes/admin/media" { - t.Errorf("push src = %q, want raw path '/share/homes/admin/media' (no user@host: prefix added)", srcArg) + if srcArg != "/share/homes/admin/media/" { + t.Errorf("push src = %q, want '/share/homes/admin/media/' (auto-appended trailing slash, no user@host: prefix added)", srcArg) } if dstArg != "/share/media/peliculas" { t.Errorf("push dst = %q, want raw path '/share/media/peliculas'", dstArg) @@ -193,6 +193,80 @@ func TestRunRemote_PullSrcAndDestStayAsIs(t *testing.T) { } } +func TestBuildArgs_AutoAppendsTrailingSlashToSource(t *testing.T) { + cases := []struct { + name string + source string + dest string + direction string + wantSrc string + }{ + { + name: "push, source without trailing slash", + source: "/mnt/storage/multimedia/series", + dest: "/share/media/series", + direction: "push", + wantSrc: "/mnt/storage/multimedia/series/", + }, + { + name: "push, source already has trailing slash (idempotent)", + source: "/mnt/storage/multimedia/series/", + dest: "/share/media/series", + direction: "push", + wantSrc: "/mnt/storage/multimedia/series/", + }, + { + name: "push, remote source without trailing slash", + source: "admin@baby-nas:/mnt/storage/multimedia/series", + dest: "/share/media/series", + direction: "push", + wantSrc: "admin@baby-nas:/mnt/storage/multimedia/series/", + }, + { + name: "pull, source without trailing slash", + source: "admin@baby-nas:/mnt/storage/multimedia/series", + dest: "/share/media/series", + direction: "pull", + wantSrc: "admin@baby-nas:/mnt/storage/multimedia/series/", + }, + { + name: "mirror, source without trailing slash", + source: "/mnt/storage/multimedia/series", + dest: "admin@10.5.0.144:/share/media/series", + direction: "mirror", + wantSrc: "/mnt/storage/multimedia/series/", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + runner := NewRsyncRunner("/tmp/ssh", "") + pair := &SyncPairConfig{ + Source: tc.source, + Dest: tc.dest, + Direction: tc.direction, + } + args := runner.buildArgs(pair) + + var gotSrc, gotDest string + if tc.direction == "pull" { + gotDest = args[len(args)-2] + gotSrc = args[len(args)-1] + } else { + gotSrc = args[len(args)-2] + gotDest = args[len(args)-1] + } + + if gotSrc != tc.wantSrc { + t.Errorf("source = %q, want %q (dest must never be touched)", gotSrc, tc.wantSrc) + } + if gotDest != tc.dest { + t.Errorf("dest = %q, want %q (dest must never be normalized)", gotDest, tc.dest) + } + }) + } +} + func countOccurrences(s, substr string) int { return strings.Count(s, substr) } diff --git a/web/src/pages/SyncPairs.tsx b/web/src/pages/SyncPairs.tsx index 4e57a03..98485b6 100644 --- a/web/src/pages/SyncPairs.tsx +++ b/web/src/pages/SyncPairs.tsx @@ -375,6 +375,9 @@ export default function SyncPairs() { setForm({ ...form, source_path: e.target.value }) } /> +

+ Directory on the source machine. Its contents will be copied into the destination. Trailing / is optional. +