From b6eeee3b54b91a77da01ff0d1aeead2ede30bccb Mon Sep 17 00:00:00 2001 From: Oluwatobi Ogundimu Date: Sat, 1 Aug 2026 13:37:17 +0100 Subject: [PATCH] SC-11: dry-run mode (--dry-run / -n) Only Receiver needed dry-run awareness - Sender's planning work runs unconditionally regardless of dry-run. All 8 write call sites in Receiver (directory/symlink creation, ApplyAttributes for symlinks/ dirs/files, ApplyHardLinks, WriteFile) individually gated behind dry-run - a disclosed, deliberate divergence from real rsync's docs, since grsync has no cheap size+mtime shortcut to skip it the way real rsync's default mode does (pre-existing architectural choice from SC-3/SC-16, not something to invent here). New -i/--itemize-changes flag implementing real rsync's YXcstpoguax format, verified against the actual man page, scoped honestly to what grsync tracks (u/a/x always ., no checksum-gated c). Added an h code for hard-link secondaries beyond the ticket's explicit list, since SC-18 already computes that grouping and >f+++++++++ would otherwise misrepresent a linked file as an ordinary new one. Dry-run safety verified across all three transports. Itemize/verbose output works for local, SSH (new stderr passthrough in transport.Session), and daemon-GET - daemon-PUT has no channel back to the client once the handshake ends, disclosed explicitly (README + a one-time CLI notice) rather than silently broken or over-engineered. TestReceiver_DryRunMakesNoFilesystemChanges proves zero filesystem changes against a tree exercising all 8 write paths. Dry-run vs. real-run itemize output proven byte-identical at pipeline and CLI level. Clean on native Windows and cross-compiled Linux. --- README.md | 184 +++++++++++++++++++-- internal/cli/root.go | 70 ++------ internal/cli/rsync_url.go | 14 +- internal/cli/sync.go | 95 +++++++++-- internal/cli/sync_test.go | 123 ++++++++++++++ internal/daemon/client.go | 9 +- internal/daemon/client_test.go | 9 +- internal/daemon/server_test.go | 90 ++++++++++- internal/daemon/session.go | 52 +++++- internal/daemon/session_test.go | 9 +- internal/pipeline/itemize.go | 251 +++++++++++++++++++++++++++++ internal/pipeline/itemize_test.go | 161 ++++++++++++++++++ internal/pipeline/pipeline_test.go | 142 +++++++++++++++- internal/pipeline/receiver.go | 177 ++++++++++++++++---- internal/pipeline/ssh_test.go | 53 ++++++ internal/sync/walk.go | 82 +++++++--- internal/sync/walk_test.go | 40 +++++ internal/transport/session.go | 15 +- 18 files changed, 1399 insertions(+), 177 deletions(-) create mode 100644 internal/pipeline/itemize.go create mode 100644 internal/pipeline/itemize_test.go diff --git a/README.md b/README.md index cc3719b..077fdc0 100644 --- a/README.md +++ b/README.md @@ -11,12 +11,14 @@ really walks, filters, diffs, transfers, and reconstructs files, applying requested attributes along the way. See [End-to-End Sync Pipeline](#end-to-end-sync-pipeline) below for exactly how the pieces connect and, just as importantly, what's still explicitly -out of scope (compression, progress reporting, `--dry-run`, partial/ -append transfers, batch mode, full `--delete`, and device/special files) -- this is real, working sync, not yet full feature parity. Hard links -*are* now preserved, opt-in via `-H`/`--hard-links` exactly like real -rsync's own flag (see -[File Attribute Preservation](#file-attribute-preservation) below). +out of scope (compression, progress reporting, partial/append transfers, +batch mode, full `--delete`, and device/special files) - this is real, +working sync, not yet full feature parity. Hard links *are* now +preserved, opt-in via `-H`/`--hard-links` exactly like real rsync's own +flag (see [File Attribute Preservation](#file-attribute-preservation) +below), and `--dry-run`/`-n` is a genuine trial run - full planning, zero +filesystem changes - with real `--itemize-changes`/`-i` output matching +rsync's own format (see [Dry-Run Mode](#dry-run-mode) below). `grsync --daemon` also now speaks a real subset of the rsync daemon protocol - `rsyncd.conf` parsing, the `@RSYNCD` greeting/handshake, module @@ -54,11 +56,12 @@ argument is always the destination. | Flag | Shorthand | Description | |---|---|---| | `--archive` | `-a` | archive mode | -| `--verbose` | `-v` | increase verbosity | +| `--verbose` | `-v` | print each updated item's path (superseded by `--itemize-changes` when both are given, see [Dry-Run Mode](#dry-run-mode)) | | `--compress` | `-z` | compress data during transfer | | `--recursive` | `-r` | recurse into directories | | `--dirs` | `-d` | list directories without recursing into them (implied by `-r`) | -| `--dry-run` | `-n` | show what would be transferred | +| `--dry-run` | `-n` | perform a trial run: full planning (file list, filters, deltas), zero filesystem changes (see [Dry-Run Mode](#dry-run-mode)) | +| `--itemize-changes` | `-i` | print a change-summary line per updated item, real rsync's own 11-character `%i` format (see [Dry-Run Mode](#dry-run-mode)) | | `--delete` | | delete extraneous files from destination | | `--progress` | | show progress during transfer | | `--exclude PATTERN` | | exclude matching files (repeatable) | @@ -339,15 +342,162 @@ mentioned. (Full `--delete` semantics remain a separate, later ticket.) **Explicitly out of scope for this pipeline** (some pre-existing gaps, restated here so they're not mistaken for oversights specific to this -ticket): compression, progress reporting, real `--dry-run` (still the -flag-echoing placeholder, to avoid silently performing a real sync when a -dry run was requested), partial/append transfers, batch mode, pulling -from a remote source (only local-source syncs are supported - push, not -pull), and device/special files. `sync.ApplySpecialFile` exists and is -tested, but nothing in `pipeline.Receiver` calls it yet - unlike hard -links (see [File Attribute Preservation](#file-attribute-preservation) -above, now wired in), this needs elevated privilege to test meaningfully -and was left out rather than expanding this integration further. +ticket): compression, progress reporting, partial/append transfers, +batch mode, pulling from a remote source (only local-source syncs are +supported - push, not pull), and device/special files. +`sync.ApplySpecialFile` exists and is tested, but nothing in +`pipeline.Receiver` calls it yet - unlike hard links (see +[File Attribute Preservation](#file-attribute-preservation) above, now +wired in), this needs elevated privilege to test meaningfully and was +left out rather than expanding this integration further. + +## Dry-Run Mode + +`--dry-run`/`-n` is a genuine trial run, not the flag-echoing placeholder +it used to be: `pipeline.Receiver` performs every planning step exactly +as a real sync would - the full signature/delta exchange, hard-link +grouping, comparing each entry against the destination's current +state - and simply skips the eight calls that would actually touch the +filesystem. Every other write path (`Sender`, and everything upstream of +`Receiver`) is completely unaffected by dry-run; only the receiving +side's own final commit step is skipped, exactly where the "no changes" +guarantee actually needs to be enforced. + +### The eight audited write calls + +`Receiver`'s own doc comment names them explicitly, and each one is +individually gated behind `if !ropts.DryRun`, not a single outer branch +that happens to skip the whole function (which would also have skipped +the planning work the ticket requires to stay real): + +- Two `os.MkdirAll` calls and an `os.WriteFile` in `receiveRegularFile`. +- An `os.MkdirAll` and `sync.ApplyAttributes` in `receiveSymlink` - + `ApplyAttributes` is the one that actually calls `os.Symlink` for a + symlink entry (see [File Attribute Preservation](#file-attribute-preservation)), + not just `chmod`/`chtimes`, which is exactly why skipping it alone + (not a separate `os.Symlink` call) is what makes a symlink entry a + genuine no-op in dry-run. +- `sync.ApplyAttributes` for a directory, in the deferred + attribute-application pass. +- `sync.ApplyHardLinks`, in the hard-link pass. + +`TestReceiver_DryRunMakesNoFilesystemChanges` in +`internal/pipeline/pipeline_test.go` is the test built specifically to +catch a regression here: it syncs a tree exercising every one of those +eight paths (a top-level file, a nested directory with a file inside it, +a symlink, and - best-effort - two hard-linked files) against a +completely empty destination and asserts the destination is *still* +completely empty afterward, not just that no error was returned. + +### What still runs, and why + +The full signature/delta exchange happens over the wire in dry-run mode +exactly as it would for a real sync - **this is a deliberate divergence +from real rsync's own documented dry-run behavior**, worth stating +explicitly rather than glossing over. Real rsync's docs say a dry run +"does not send the actual data for file transfers," but that's only true +because real rsync's *default* mode has a cheap size+mtime "quick check" +that skips the signature/delta algorithm entirely for files it can +already tell are unchanged - grsync has no such shortcut (a pre-existing, +deliberate architectural choice from the original delta-transfer and +pipeline-integration work: full delta always runs, full stop). Matching +real rsync's dry-run network behavior *by letter* would mean building a +new quick-check mechanism that doesn't otherwise exist in this codebase, +just to serve this one mode - worse than disclosing the honest +divergence. The content comparison itself is genuinely free either way: +`sync.ApplyDelta` is pure in-memory work, so computing it costs nothing +whether or not the result is about to be written, and it's the only way +grsync can correctly answer "did this file's content actually change" +for itemize purposes without a `--checksum`-style flag. + +### `--itemize-changes`/`-i`: real rsync's actual format, not an approximation + +Verified against `rsync.1`'s own `--itemize-changes` section rather than +invented: the format is `YXcstpoguax`, 11 characters - `Y` (update type: +`>` transferred, `c` local change/creation, `h` hard link, `.` not +updated), `X` (file type: `f`/`d`/`L`), then 9 attribute letters +(`c` checksum/value-differs, `s` size, `t` time, `p` perms, `o` owner, +`g` group, `u`/`n`/`b` atime/crtime, `a` ACL, `x` xattr) - `.` for +unchanged, `+` for newly-created. A completely unchanged item is not +printed at all with a single `-i`, matching real rsync's own default +(a second `-i`/`-vv` would show them too; grsync doesn't implement that +second tier). Output lines match real rsync's own default +(`--out-format='%i %n%L'`): the code, the path, and `" -> target"` for a +symlink. + +Scoped to what grsync actually tracks, each disclosed explicitly rather +than silently approximated: + +- **`u`/`a`/`x` (atime/crtime, ACL, xattr) are always `.`** - grsync has + no `--atimes`/`--acl`/`--xattr` flags, matching real rsync's own + behavior when those options aren't given. +- **Attribute-`c` (checksum) never fires for a regular file** - real + rsync gates it behind `--checksum`, which grsync doesn't have; only + `s`/`t`/`p`/`o`/`g` are ever used to describe what changed about a + file's content or attributes, matching this ticket's own explicit + scope. +- **`h` (hard-link secondary member) is implemented even though the + ticket's own explicit list didn't name it** - SC-18 already computed + exactly this grouping information, and reporting a hard-linked file as + an ordinary new file (`>f+++++++++`) would be a real, easily-avoidable + inaccuracy. + +`--verbose`/`-v` alone (without `-i`) prints just the path (plus +`" -> target"` for a symlink) per changed item - real rsync's own +`"%n%L"` default for `-v` without `-i`; `-i` takes precedence when both +are given. + +Verified against real rsync's own documented guarantee ("The output of +`--itemize-changes` is supposed to be exactly the same on a dry run and +a subsequent real run") by `TestReceiver_DryRunItemizeMatchesRealRunItemize` +and its CLI-level counterpart `TestE2E_DryRunAndRealRunItemizeMatch`: +both compare a dry run's itemize output against a real run's, on a +*separate*, equally fresh destination - not a second real run against +the same one, which would legitimately report nothing left to do and +prove nothing about the dry run's accuracy. + +### Across transports + +- **Local**: `pipeline.Receiver` runs in-process; `ReceiverOptions` + (dry-run, itemize, verbose, and where to write it) is passed straight + through. +- **SSH**: the remote `grsync --server` process is invoked with + `--dry-run`/`--itemize-changes`/`--verbose` as ordinary flags on its + own command line (see `syncToRemote` in `internal/cli/sync.go`) - no + new wire protocol needed, since `--server` mode already parses its own + real CLI flags. Itemize/verbose *output* needed one small addition: + `transport.Session` now passes the remote subprocess's stderr through + to this process's own stderr live, not just buffering it for a + post-mortem error message - stdout is the framed wire protocol itself + (see [End-to-End Sync Pipeline](#end-to-end-sync-pipeline)), so + `runServer` writes its reporting there, never to stdout, and that + passthrough is what lets it actually reach the local terminal. + `TestSSHLocalhost_DryRunMakesNoChanges` proves the no-write guarantee + over a real SSH connection to `127.0.0.1` (skipped gracefully if no + local `sshd` is reachable, the same as this project's other real-SSH + tests). +- **`rsync://` daemon**: dry-run's no-write guarantee is fully supported + in both directions, verified over a real TCP connection by + `TestDaemon_RealTCP_DryRunPutMakesNoChanges` and + `TestDaemon_RealTCP_DryRunGetMakesNoChanges`. A download + (`DirectionGet`) needs nothing special - the client's own `Receiver` + runs locally, exactly like a local sync. An upload (`DirectionPut`) + needed a small, genuine protocol extension: the client sends + `"put --dry-run"` instead of `"put"` on the direction line + (`daemon.dryRunToken`) so the *server's* `Receiver` - the side that + actually decides whether to write, for this direction - knows to skip + its writes. + + **Itemize/verbose output is not available for an `rsync://` upload**, + a real, disclosed gap rather than something silently half-working: once + the module handshake ends, the daemon connection is pure binary wire + protocol with no channel for arbitrary text, unlike SSH's genuinely + separate stderr stream - adding one would be real, separate protocol + work, not a small extension of this ticket. `--dry-run`'s actual + no-write guarantee needs no such channel and is unaffected; `grsync` + prints a one-time note (to stderr) when `-i`/`-v` is combined with an + `rsync://` destination, rather than silently producing no output and + leaving the user to wonder why. ## rsync Daemon Mode diff --git a/internal/cli/root.go b/internal/cli/root.go index 6aa1bd8..28cc7a7 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -6,9 +6,6 @@ package cli import ( - "fmt" - "strings" - "github.com/spf13/cobra" "github.com/syntaxroot-cc/grsync/internal/daemon" @@ -58,6 +55,7 @@ type options struct { group bool links bool hardLinks bool + itemize bool filterRules []FilterRule rsh string server bool @@ -106,8 +104,8 @@ func NewRootCmd() *cobra.Command { Use: "grsync ... ", Short: "grsync synchronizes files between one or more sources and a destination", Long: "grsync is an rsync-inspired file synchronization tool.\n" + - "Local-to-local and local-to-remote (SSH) syncs are supported; " + - "--dry-run, compression, progress reporting, and full --delete are not yet.", + "Local-to-local and local-to-remote (SSH) syncs are supported, including --dry-run " + + "and --itemize-changes; compression, progress reporting, and full --delete are not yet.", // --server takes exactly one positional arg (the destination path) // rather than the normal ... shape: it is how // a remote-invoked grsync (e.g. `ssh host grsync --server /dest`) @@ -133,26 +131,22 @@ func NewRootCmd() *cobra.Command { return runServer(cmd, args[0], opts) } sources, destination := args[:len(args)-1], args[len(args)-1] - if opts.dryRun { - // A real dry-run (list what would change, transfer - // nothing) is explicitly out of scope for now - falling - // through to a real sync here would silently do the - // opposite of what --dry-run promises, which is worse - // than not supporting it yet. Until real dry-run support - // lands, this stays on the flag-echoing placeholder. - return run(cmd, sources, destination, opts) - } return runSync(cmd, sources, destination, opts) }, } flags := cmd.Flags() flags.BoolVarP(&opts.archive, "archive", "a", false, "archive mode (equivalent to common rsync defaults)") - flags.BoolVarP(&opts.verbose, "verbose", "v", false, "increase output verbosity") + flags.BoolVarP(&opts.verbose, "verbose", "v", false, + "mention each updated item's path (superseded by --itemize-changes when both are given, "+ + "matching real rsync's own -v/-i relationship)") flags.BoolVarP(&opts.compress, "compress", "z", false, "compress file data during transfer") flags.BoolVarP(&opts.recursive, "recursive", "r", false, "recurse into directories") flags.BoolVarP(&opts.dirs, "dirs", "d", false, "include directories themselves without recursing into their contents (implied by --recursive)") - flags.BoolVarP(&opts.dryRun, "dry-run", "n", false, "show what would be transferred without transferring") + flags.BoolVarP(&opts.dryRun, "dry-run", "n", false, "perform a trial run: full planning (file list, filters, deltas) with no filesystem changes") + flags.BoolVarP(&opts.itemize, "itemize-changes", "i", false, + "output a change-summary line per updated item, real rsync's own 11-character %i format "+ + "(YXcstpoguax - see the README's Dry-Run Mode section); most useful with --dry-run") flags.BoolVar(&opts.delete, "delete", false, "delete extraneous files from destination") flags.BoolVar(&opts.progress, "progress", false, "show progress during transfer") flags.Var(&filterRuleFlag{ruleType: FilterRuleExclude, rules: &opts.filterRules}, @@ -197,50 +191,6 @@ func NewRootCmd() *cobra.Command { return cmd } -// run is the placeholder command body. It only echoes back what was parsed -// so the flag wiring can be verified end to end; the actual sync/transport -// work lands in later tickets. -func run(cmd *cobra.Command, sources []string, destination string, opts *options) error { - var rules strings.Builder - if len(opts.filterRules) == 0 { - rules.WriteString("[]") - } else { - for i, r := range opts.filterRules { - if i > 0 { - rules.WriteString(", ") - } - fmt.Fprintf(&rules, "%s:%s", r.Type, r.Pattern) - } - } - - summary := fmt.Sprintf( - "sources: %v\n"+ - "destination: %s\n"+ - "archive: %t\n"+ - "verbose: %t\n"+ - "compress: %t\n"+ - "recursive: %t\n"+ - "dirs: %t\n"+ - "dry-run: %t\n"+ - "delete: %t\n"+ - "progress: %t\n"+ - "perms: %t\n"+ - "times: %t\n"+ - "owner: %t\n"+ - "group: %t\n"+ - "links: %t\n"+ - "rsh: %q\n"+ - "filters: %s\n", - sources, destination, - opts.archive, opts.verbose, opts.compress, opts.recursive, opts.dirs, opts.dryRun, - opts.delete, opts.progress, opts.perms, opts.times, opts.owner, opts.group, opts.links, - opts.rsh, rules.String(), - ) - - _, err := fmt.Fprint(cmd.OutOrStdout(), summary) - return err -} - // Execute runs the root command using os.Args, as called from main(). func Execute() error { return NewRootCmd().Execute() diff --git a/internal/cli/rsync_url.go b/internal/cli/rsync_url.go index 64fe872..5ae8e5f 100644 --- a/internal/cli/rsync_url.go +++ b/internal/cli/rsync_url.go @@ -8,6 +8,7 @@ import ( "time" "github.com/syntaxroot-cc/grsync/internal/daemon" + "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" ) @@ -35,7 +36,15 @@ const dialDaemonTimeout = 10 * time.Second // is the only AttrOptions field DialClient's Sender-side (DirectionPut) // call actually consults, but it's threaded through as a full // sync.AttrOptions to match DialClient's own signature. -func syncToRsyncDaemon(src string, u daemon.URL, password daemon.PasswordFunc, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool) error { +// +// dryRun is the only ReceiverOptions field that reaches the daemon at +// all for this direction: the module's Receiver runs on the server, not +// here, so DialModule sends it as an extra token on the wire (see +// daemon.dryRunToken) rather than anything this function does directly. +// Itemize/Verbose are deliberately not passed through - see runSync's +// own one-time note about why daemon-PUT reporting output isn't +// available, printed before this function is ever called. +func syncToRsyncDaemon(src string, u daemon.URL, password daemon.PasswordFunc, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool, dryRun bool) error { port := u.Port if port == 0 { port = daemon.DefaultPort @@ -49,5 +58,6 @@ func syncToRsyncDaemon(src string, u daemon.URL, password daemon.PasswordFunc, w defer func() { _ = nc.Close() }() user := resolveUser(u.User) - return daemon.DialClient(nc, u.Module, user, password, daemon.DirectionPut, src, rules, walkOpts, sync.AttrOptions{HardLinks: hardLinks}) + ropts := pipeline.ReceiverOptions{DryRun: dryRun} + return daemon.DialClient(nc, u.Module, user, password, daemon.DirectionPut, src, rules, walkOpts, sync.AttrOptions{HardLinks: hardLinks}, ropts) } diff --git a/internal/cli/sync.go b/internal/cli/sync.go index 5dfc4a5..7845235 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -49,6 +49,17 @@ func effectiveAttrOptions(opts *options) sync.AttrOptions { } } +// effectiveReceiverOptions computes pipeline.ReceiverOptions from opts: +// --dry-run, --itemize-changes, and --verbose, all reported to output. +func effectiveReceiverOptions(opts *options, output io.Writer) pipeline.ReceiverOptions { + return pipeline.ReceiverOptions{ + DryRun: opts.dryRun, + Itemize: opts.itemize, + Verbose: opts.verbose, + Output: output, + } +} + // toSyncRawRules converts the CLI's FilterRule list to sync.RawRule. // FilterRuleType's string values were chosen to exactly match // sync.RuleKind's ("include", "exclude", "filter", "exclude-from", @@ -64,11 +75,13 @@ func toSyncRawRules(filterRules []FilterRule) []sync.RawRule { return raw } -// runSync is the real sync entry point (as opposed to run, the -// flag-echoing placeholder still used for --dry-run). For each source, it -// syncs that source into destination - in-process for a local -// destination, over an SSH-spawned connection for a remote user@host:path -// one, or over a plain TCP connection to an rsync:// daemon module. +// runSync is the real sync entry point. For each source, it syncs that +// source into destination - in-process for a local destination, over an +// SSH-spawned connection for a remote user@host:path one, or over a +// plain TCP connection to an rsync:// daemon module. opts.dryRun makes +// this a full trial run - every planning step still happens, nothing is +// actually written - see pipeline.Receiver's own doc comment for exactly +// which calls that skips. // // Pulling FROM a remote source (SSH or an rsync:// daemon) is not yet // supported, only a local source to a local, SSH, or daemon destination - @@ -123,24 +136,43 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt password = resolvePassword(opts.passwordFile, cmd.InOrStdin()) } + ropts := effectiveReceiverOptions(opts, cmd.OutOrStdout()) + if isRsyncDaemon && ropts.Reporting() { + // The daemon protocol has no channel for this: once the module + // handshake ends, the connection is pure binary wire protocol + // (see internal/daemon's own doc comment on where the real-vs-gob + // boundary sits) with nowhere to carry itemize/verbose text back + // to the client, unlike SSH's genuinely separate stderr stream. + // --dry-run's actual safety guarantee (no writes happen) still + // fully applies; only the reporting text is unavailable here. + // Noting this once, up front, rather than silently producing no + // output and leaving the user to wonder why. + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --itemize-changes/--verbose output is not available for an rsync:// "+ + "daemon destination (the daemon protocol has no channel for it); --dry-run's no-write guarantee still applies") + } + for _, src := range sources { switch { case isRsyncDaemon: - if err := syncToRsyncDaemon(src, rsyncURL, password, walkOpts, rules, attrOpts.HardLinks); err != nil { + if err := syncToRsyncDaemon(src, rsyncURL, password, walkOpts, rules, attrOpts.HardLinks, opts.dryRun); err != nil { return fmt.Errorf("syncing %q to %q: %w", src, destination, err) } case isRemote: - if err := syncToRemote(opts.rsh, src, remote, walkOpts, rules, attrOpts.HardLinks); err != nil { + if err := syncToRemote(opts.rsh, src, remote, walkOpts, rules, attrOpts.HardLinks, ropts); err != nil { return fmt.Errorf("syncing %q to %q: %w", src, destination, err) } default: - if err := syncLocal(src, destination, walkOpts, rules, attrOpts); err != nil { + if err := syncLocal(src, destination, walkOpts, rules, attrOpts, ropts); err != nil { return fmt.Errorf("syncing %q to %q: %w", src, destination, err) } } } - _, err = fmt.Fprintf(cmd.OutOrStdout(), "synced %d source(s) to %s\n", len(sources), destination) + verb := "synced" + if opts.dryRun { + verb = "would sync" + } + _, err = fmt.Fprintf(cmd.OutOrStdout(), "%s %d source(s) to %s\n", verb, len(sources), destination) return err } @@ -149,7 +181,7 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt // way, the exact same pipeline.Sender/pipeline.Receiver functions that // carry out a remote sync are what a local sync exercises too, instead of // a second, independently-trusted implementation of the same logic. -func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions) error { +func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions) error { senderReadsFromReceiver, receiverWritesToSender := io.Pipe() receiverReadsFromSender, senderWritesToReceiver := io.Pipe() @@ -159,7 +191,7 @@ func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, a senderErrCh := make(chan error, 1) go func() { senderErrCh <- pipeline.Sender(sender, src, walkOpts, rules, attrOpts.HardLinks) }() - receiverErr := pipeline.Receiver(receiver, dest, attrOpts) + receiverErr := pipeline.Receiver(receiver, dest, attrOpts, ropts) senderErr := <-senderErrCh if receiverErr != nil { @@ -171,8 +203,29 @@ func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, a // syncToRemote spawns `grsync --server DEST` on the remote host via SSH // (or whatever --rsh overrides it to), performs the handshake, then runs // the sender side of the pipeline against that connection. -func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool) error { - session, err := transport.Dial(rsh, remote.User, remote.Host, []string{"grsync", "--server", remote.Path}) +// +// ropts.DryRun/Itemize/Verbose are passed as extra flags on that remote +// command line (e.g. "grsync --server --dry-run -i DEST"), not over any +// new wire message: the remote --server process parses them the normal +// way, via its own real CLI flag handling (see runServer), and the +// receiving side's dry-run/itemize decision is made entirely on the +// remote side, exactly where pipeline.Receiver actually runs for this +// transport - there is nothing for the local, sending side to decide +// here at all. +func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool, ropts pipeline.ReceiverOptions) error { + remoteArgs := []string{"grsync", "--server"} + if ropts.DryRun { + remoteArgs = append(remoteArgs, "--dry-run") + } + if ropts.Itemize { + remoteArgs = append(remoteArgs, "--itemize-changes") + } + if ropts.Verbose { + remoteArgs = append(remoteArgs, "--verbose") + } + remoteArgs = append(remoteArgs, remote.Path) + + session, err := transport.Dial(rsh, remote.User, remote.Host, remoteArgs) if err != nil { return fmt.Errorf("connecting to %s: %w", remote.Host, err) } @@ -194,6 +247,19 @@ func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.Wa // runServer implements --server mode: perform the handshake, then run the // receiver side of the pipeline against dest, reading/writing the // command's own stdin/stdout. +// +// opts here is this process's own locally-parsed flags - for a real +// remote invocation, that means whatever syncToRemote put on the ssh +// command line (see its own doc comment), so --dry-run/-i/-v "just work" +// through the same argv-parsing path every other flag already does, no +// separate propagation mechanism required. Itemize/verbose output goes +// to cmd.ErrOrStderr(), never stdout: stdout here is the framed wire +// protocol itself (see transport.WriteFrame/ReadFrame), so writing +// human-readable text there would corrupt it. In real (non-test) use, +// ErrOrStderr() is this process's actual stderr, which Session (the +// local side's view of this same subprocess) passes through live to the +// local user's terminal - see session.go's own doc comment on why that +// pass-through exists. func runServer(cmd *cobra.Command, dest string, opts *options) error { stdin, stdout := cmd.InOrStdin(), cmd.OutOrStdout() @@ -202,5 +268,6 @@ func runServer(cmd *cobra.Command, dest string, opts *options) error { } rw := pipeReadWriter{Reader: stdin, Writer: stdout} - return pipeline.Receiver(rw, dest, effectiveAttrOptions(opts)) + ropts := effectiveReceiverOptions(opts, cmd.ErrOrStderr()) + return pipeline.Receiver(rw, dest, effectiveAttrOptions(opts), ropts) } diff --git a/internal/cli/sync_test.go b/internal/cli/sync_test.go index 29921f6..882cf6b 100644 --- a/internal/cli/sync_test.go +++ b/internal/cli/sync_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/syntaxroot-cc/grsync/internal/sync" @@ -40,6 +41,128 @@ func TestE2E_LocalToLocal(t *testing.T) { assertTreesMatch(t, src, dst, symlinksSupported) } +// TestE2E_DryRunMakesNoFilesystemChanges drives the real CLI command +// with --dry-run/-n against a rich source tree and confirms the +// destination is completely empty afterward - the same guarantee +// internal/pipeline's own TestReceiver_DryRunMakesNoFilesystemChanges +// proves at the pipeline level, checked again here through the actual +// command a user types, flags and all. +func TestE2E_DryRunMakesNoFilesystemChanges(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + + mustWriteFile(t, filepath.Join(src, "top.txt"), "top level content") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + if err := os.Symlink("nested.txt", filepath.Join(src, "sub", "link.txt")); err != nil { + t.Logf("symlink creation unsupported in this environment, tree will not include one: %v", err) + } + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "-H", "--dry-run", src, dst}) + cmd.SetOut(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + entries, err := os.ReadDir(dst) + if err != nil { + t.Fatalf("ReadDir(dst): %v", err) + } + if len(entries) != 0 { + t.Errorf("destination is not empty after --dry-run: %v", entries) + } +} + +// TestE2E_DryRunItemizeOutput drives the real CLI command with +// --dry-run and --itemize-changes together and confirms the printed +// output actually contains real rsync's own %i format codes - a new +// file's ">f+++++++++" and a new directory's "cd+++++++++" - not just +// that the command exits without error. +func TestE2E_DryRunItemizeOutput(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + + mustWriteFile(t, filepath.Join(src, "new.txt"), "brand new content") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + + cmd := NewRootCmd() + var out strings.Builder + cmd.SetArgs([]string{"-a", "--dry-run", "-i", src, dst}) + cmd.SetOut(&out) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + output := out.String() + if !strings.Contains(output, ">f+++++++++ new.txt") { + t.Errorf("output = %q, want it to contain %q", output, ">f+++++++++ new.txt") + } + if !strings.Contains(output, "cd+++++++++ sub") { + t.Errorf("output = %q, want it to contain %q", output, "cd+++++++++ sub") + } + if !strings.Contains(output, ">f+++++++++ sub/nested.txt") { + t.Errorf("output = %q, want it to contain %q", output, ">f+++++++++ sub/nested.txt") + } + + // --dry-run's own guarantee, checked here too: itemize output + // claiming a transfer happened must not have been accompanied by an + // actual one. + entries, err := os.ReadDir(dst) + if err != nil { + t.Fatalf("ReadDir(dst): %v", err) + } + if len(entries) != 0 { + t.Errorf("destination is not empty despite --dry-run: %v", entries) + } +} + +// TestE2E_DryRunAndRealRunItemizeMatch is +// TestReceiver_DryRunItemizeMatchesRealRunItemize's CLI-level +// counterpart: real rsync's own documented guarantee ("The output of +// --itemize-changes is supposed to be exactly the same on a dry run and +// a subsequent real run") checked through the actual command, against +// two separate fresh destinations. +func TestE2E_DryRunAndRealRunItemizeMatch(t *testing.T) { + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "file.txt"), "some content") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + + dryRunDst := t.TempDir() + var dryRunOut strings.Builder + dryRunCmd := NewRootCmd() + dryRunCmd.SetArgs([]string{"-a", "--dry-run", "-i", src, dryRunDst}) + dryRunCmd.SetOut(&dryRunOut) + if err := dryRunCmd.Execute(); err != nil { + t.Fatalf("dry-run Execute returned error: %v", err) + } + + realDst := t.TempDir() + var realOut strings.Builder + realCmd := NewRootCmd() + realCmd.SetArgs([]string{"-a", "-i", src, realDst}) + realCmd.SetOut(&realOut) + if err := realCmd.Execute(); err != nil { + t.Fatalf("real-run Execute returned error: %v", err) + } + + // The trailing summary line ("would sync ... to DRYDST" vs "synced + // ... to REALDST") legitimately differs - only the itemize lines + // above it need to match, so both outputs are trimmed to just those. + dryRunLines := strings.Split(strings.TrimSpace(dryRunOut.String()), "\n") + realLines := strings.Split(strings.TrimSpace(realOut.String()), "\n") + if len(dryRunLines) < 2 || len(realLines) < 2 { + t.Fatalf("expected at least one itemize line plus a summary line; dry-run = %q, real = %q", dryRunOut.String(), realOut.String()) + } + dryRunItemize := strings.Join(dryRunLines[:len(dryRunLines)-1], "\n") + realItemize := strings.Join(realLines[:len(realLines)-1], "\n") + if dryRunItemize != realItemize { + t.Errorf("dry-run itemize output does not match a real run's:\ndry-run:\n%s\nreal:\n%s", dryRunItemize, realItemize) + } +} + // TestE2E_HardLinksPreservedWithFlag drives the real CLI command with // -H/--hard-links and confirms two hard-linked source files arrive at // the destination still hard-linked to each other (os.SameFile), not diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 830f41d..cb0fcbe 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -4,6 +4,7 @@ import ( "fmt" "net" + "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" ) @@ -19,8 +20,10 @@ import ( // // module must be non-empty: DialClient runs a transfer, not a listing - // callers that want to list a daemon's modules should use DialGreeting -// directly with an empty module instead. -func DialClient(nc net.Conn, module, user string, password PasswordFunc, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions) error { +// directly with an empty module instead. See DialModule's own doc +// comment for exactly what ropts does and doesn't reach on each +// direction. +func DialClient(nc net.Conn, module, user string, password PasswordFunc, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions) error { if module == "" { return fmt.Errorf("DialClient requires a module name") } @@ -33,5 +36,5 @@ func DialClient(nc net.Conn, module, user string, password PasswordFunc, directi if err := DialAuth(c, user, password); err != nil { return err } - return DialModule(c, direction, localPath, rules, walkOpts, attrOpts) + return DialModule(c, direction, localPath, rules, walkOpts, attrOpts, ropts) } diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index fedebe9..0f5ebf2 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" ) @@ -31,7 +32,7 @@ func TestDialClient_DownloadOverRealTCP(t *testing.T) { nc := dialRawConn(t, addr) dest := t.TempDir() - err := DialClient(nc, "public", "", StaticPassword(""), DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}) + err := DialClient(nc, "public", "", StaticPassword(""), DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}) if err != nil { t.Fatalf("DialClient returned error: %v", err) } @@ -65,7 +66,7 @@ func TestDialClient_UploadOverRealTCP(t *testing.T) { t.Fatalf("compiling empty rule set: %v", err) } - err = DialClient(nc, "incoming", "alice", StaticPassword("hunter2"), DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}) + err = DialClient(nc, "incoming", "alice", StaticPassword("hunter2"), DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}) if err != nil { t.Fatalf("DialClient returned error: %v", err) } @@ -105,7 +106,7 @@ func TestDialClient_PasswordFuncNotCalledForAnonymousModule(t *testing.T) { }) dest := t.TempDir() - err := DialClient(nc, "public", "", poisonedPassword, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}) + err := DialClient(nc, "public", "", poisonedPassword, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}) if err != nil { t.Fatalf("DialClient returned error: %v", err) } @@ -119,7 +120,7 @@ func TestDialClient_RejectsEmptyModule(t *testing.T) { addr, _ := startTestDaemon(t, cfg) nc := dialRawConn(t, addr) - err := DialClient(nc, "", "", StaticPassword(""), DirectionGet, t.TempDir(), nil, sync.WalkOptions{}, sync.AttrOptions{}) + err := DialClient(nc, "", "", StaticPassword(""), DirectionGet, t.TempDir(), nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}) if err == nil { t.Fatalf("DialClient with an empty module returned nil error, want an error") } diff --git a/internal/daemon/server_test.go b/internal/daemon/server_test.go index 8fea8ae..6d20194 100644 --- a/internal/daemon/server_test.go +++ b/internal/daemon/server_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" ) @@ -59,7 +60,7 @@ func TestDaemon_RealTCP_AnonymousDownload(t *testing.T) { t.Fatalf("DialAuth: %v", err) } dest := t.TempDir() - if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}); err != nil { + if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}); err != nil { t.Fatalf("DialModule: %v", err) } @@ -101,7 +102,7 @@ func TestDaemon_RealTCP_AuthenticatedUpload(t *testing.T) { if err != nil { t.Fatalf("compiling empty rule set: %v", err) } - if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}); err != nil { + if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}); err != nil { t.Fatalf("DialModule: %v", err) } @@ -163,3 +164,88 @@ func TestDaemon_RealTCP_ModuleListing(t *testing.T) { t.Errorf("listing = %q, want it to NOT contain the hidden module", joined) } } + +// TestDaemon_RealTCP_DryRunPutMakesNoChanges is the daemon protocol's +// real proof for the dry-run wire extension: a DirectionPut with +// ReceiverOptions.DryRun set sends "put --dry-run" on the direction line +// (see dryRunToken), and the module's directory - where ServeModule's +// Receiver actually runs - must stay completely empty afterward, over an +// actual TCP connection, not just a same-process pipe. +func TestDaemon_RealTCP_DryRunPutMakesNoChanges(t *testing.T) { + modRoot := t.TempDir() + cfg := &Config{Modules: map[string]Module{ + "incoming": {Name: "incoming", Path: modRoot, ReadOnly: false}, + }} + addr, errLog := startTestDaemon(t, cfg) + + client := dialTestDaemon(t, addr) + if _, err := DialGreeting(client, "incoming"); err != nil { + t.Fatalf("DialGreeting: %v", err) + } + if err := DialAuth(client, "", StaticPassword("")); err != nil { + t.Fatalf("DialAuth: %v", err) + } + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "upload.txt"), "should never be written to the module") + rules, err := sync.CompileRules(nil) + if err != nil { + t.Fatalf("compiling empty rule set: %v", err) + } + ropts := pipeline.ReceiverOptions{DryRun: true} + if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}, ropts); err != nil { + t.Fatalf("DialModule: %v", err) + } + + entries, err := os.ReadDir(modRoot) + if err != nil { + t.Fatalf("ReadDir(modRoot): %v", err) + } + if len(entries) != 0 { + t.Errorf("module directory is not empty after a dry-run put: %v", entries) + } + if errLog.Len() != 0 { + t.Errorf("daemon error log = %q, want empty", errLog.String()) + } +} + +// TestDaemon_RealTCP_DryRunGetMakesNoChanges is DryRun's counterpart for +// DirectionGet: unlike PUT, this needs no protocol extension at all - the +// client's own Receiver runs locally here, so ReceiverOptions.DryRun is +// simply consulted directly, the same as a local sync - but it's worth +// proving over a real daemon connection too, not just assumed from the +// PUT case working. +func TestDaemon_RealTCP_DryRunGetMakesNoChanges(t *testing.T) { + modRoot := t.TempDir() + mustWriteFile(t, filepath.Join(modRoot, "readme.txt"), "should never be downloaded") + + cfg := &Config{Modules: map[string]Module{ + "public": {Name: "public", Path: modRoot, ReadOnly: true, List: true}, + }} + addr, errLog := startTestDaemon(t, cfg) + + client := dialTestDaemon(t, addr) + if _, err := DialGreeting(client, "public"); err != nil { + t.Fatalf("DialGreeting: %v", err) + } + if err := DialAuth(client, "", StaticPassword("")); err != nil { + t.Fatalf("DialAuth: %v", err) + } + + dest := t.TempDir() + ropts := pipeline.ReceiverOptions{DryRun: true} + if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}, ropts); err != nil { + t.Fatalf("DialModule: %v", err) + } + + entries, err := os.ReadDir(dest) + if err != nil { + t.Fatalf("ReadDir(dest): %v", err) + } + if len(entries) != 0 { + t.Errorf("destination is not empty after a dry-run get: %v", entries) + } + if errLog.Len() != 0 { + t.Errorf("daemon error log = %q, want empty", errLog.String()) + } +} diff --git a/internal/daemon/session.go b/internal/daemon/session.go index 5640366..540c376 100644 --- a/internal/daemon/session.go +++ b/internal/daemon/session.go @@ -3,6 +3,7 @@ package daemon import ( "errors" "fmt" + "strings" "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" @@ -80,6 +81,17 @@ func moduleAttrOptions() sync.AttrOptions { return sync.AttrOptions{Perms: true, Times: true, Owner: true, Group: true, Links: true, HardLinks: true} } +// dryRunToken is appended as a second, space-separated field on the +// direction line - "put --dry-run" instead of just "put" - the one +// piece of protocol extension a client-requested dry-run needs for a +// DirectionPut: the connection's Receiver runs on this (server) side, so +// there is no other way for the client to communicate "plan this, but +// don't actually write it" without adding a wire signal for it. A +// DirectionGet needs nothing equivalent - that side's Receiver runs +// locally on the client, entirely its own decision to make (see +// DialModule). +const dryRunToken = "--dry-run" + // ServeModule runs one authenticated client's session against the // already-selected module m: reads the client's requested Direction, // enforces read-only, acknowledges with "@RSYNCD: OK" or refuses with an @@ -95,7 +107,13 @@ func ServeModule(c *conn, m Module) error { if err != nil { return fmt.Errorf("reading direction: %w", err) } - direction := Direction(line) + fields := strings.Fields(line) + if len(fields) == 0 { + _ = writeLine(c.w, fmt.Sprintf("@ERROR: invalid direction %q", line)) + return fmt.Errorf("invalid direction %q", line) + } + direction := Direction(fields[0]) + dryRun := direction == DirectionPut && len(fields) > 1 && fields[1] == dryRunToken if direction == DirectionPut && m.ReadOnly { _ = writeLine(c.w, "@ERROR: module is read only") @@ -121,7 +139,15 @@ func ServeModule(c *conn, m Module) error { } return waitForTransferDone(c) case DirectionPut: - if err := pipeline.Receiver(c, m.Path, moduleAttrOptions()); err != nil { + // No Itemize/Verbose/Output here: the daemon protocol has no + // channel back to the client for reporting text once the + // handshake ends (unlike SSH's separate stderr stream) - see the + // README's Dry-Run Mode section for the full explanation of this + // disclosed gap. DryRun's actual no-write guarantee, in contrast, + // needs no channel at all beyond the dryRunToken above: it's + // purely a local decision this call makes about its own writes. + ropts := pipeline.ReceiverOptions{DryRun: dryRun} + if err := pipeline.Receiver(c, m.Path, moduleAttrOptions(), ropts); err != nil { return err } return writeLine(c.w, transferDone) @@ -140,8 +166,24 @@ func ServeModule(c *conn, m Module) error { // DirectionPut's Sender detects hard links at all - the same field // serves both directions since it's one "does the client want hard links // preserved" decision either way. -func DialModule(c *conn, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions) error { - if err := writeLine(c.w, string(direction)); err != nil { +// +// ropts matters differently depending on direction: for DirectionGet, +// the client's own Receiver runs locally, so ropts (DryRun, Itemize, +// Verbose, Output) all apply directly, exactly like a local sync. For +// DirectionPut, the client runs Sender, which has no dry-run concept at +// all (see pipeline.Sender's own doc comment) - only ropts.DryRun is +// used here, sent as an extra token on the direction line (dryRunToken) +// so the *server's* Receiver, which is the side that actually decides +// whether to write, knows to skip its writes. Itemize/Verbose are +// silently unusable for a DirectionPut, since the daemon protocol has no +// channel to carry that reporting text back from the server - see +// ServeModule's own comment on the same limitation. +func DialModule(c *conn, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions) error { + directionLine := string(direction) + if direction == DirectionPut && ropts.DryRun { + directionLine += " " + dryRunToken + } + if err := writeLine(c.w, directionLine); err != nil { return fmt.Errorf("sending direction: %w", err) } @@ -155,7 +197,7 @@ func DialModule(c *conn, direction Direction, localPath string, rules []sync.Rul switch direction { case DirectionGet: - if err := pipeline.Receiver(c, localPath, attrOpts); err != nil { + if err := pipeline.Receiver(c, localPath, attrOpts, ropts); err != nil { return err } return writeLine(c.w, transferDone) diff --git a/internal/daemon/session_test.go b/internal/daemon/session_test.go index 71ad613..f2c8159 100644 --- a/internal/daemon/session_test.go +++ b/internal/daemon/session_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "testing" + "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" ) @@ -40,7 +41,7 @@ func TestServeModule_GetDownloadsFiles(t *testing.T) { go func() { serverErrCh <- ServeModule(server, m) }() dest := t.TempDir() - if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{Perms: true}); err != nil { + if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{Perms: true}, pipeline.ReceiverOptions{}); err != nil { t.Fatalf("DialModule returned error: %v", err) } if err := <-serverErrCh; err != nil { @@ -75,7 +76,7 @@ func TestServeModule_GetHonorsModuleExclude(t *testing.T) { go func() { serverErrCh <- ServeModule(server, m) }() dest := t.TempDir() - if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}); err != nil { + if err := DialModule(client, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}); err != nil { t.Fatalf("DialModule returned error: %v", err) } if err := <-serverErrCh; err != nil { @@ -101,7 +102,7 @@ func TestServeModule_PutToReadOnlyModuleFails(t *testing.T) { src := t.TempDir() mustWriteFile(t, filepath.Join(src, "upload.txt"), "trying to push this up") - dialErr := DialModule(client, DirectionPut, src, nil, sync.WalkOptions{}, sync.AttrOptions{}) + dialErr := DialModule(client, DirectionPut, src, nil, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}) if dialErr == nil { t.Fatalf("DialModule against a read-only module returned nil error, want an error") } @@ -128,7 +129,7 @@ func TestServeModule_PutToWritableModuleSucceeds(t *testing.T) { if err != nil { t.Fatalf("compiling empty rule set: %v", err) } - if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}); err != nil { + if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}, pipeline.ReceiverOptions{}); err != nil { t.Fatalf("DialModule returned error: %v", err) } if err := <-serverErrCh; err != nil { diff --git a/internal/pipeline/itemize.go b/internal/pipeline/itemize.go new file mode 100644 index 0000000..b8cd140 --- /dev/null +++ b/internal/pipeline/itemize.go @@ -0,0 +1,251 @@ +package pipeline + +import ( + "fmt" + "io" + "io/fs" + "strings" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +// ReceiverOptions bundles Receiver's dry-run and reporting behavior - +// kept separate from sync.AttrOptions, which controls *what* gets +// preserved, not whether writes happen at all or what gets reported +// about them. +type ReceiverOptions struct { + // DryRun, when true, makes Receiver perform every planning step + // (signature/delta exchange, hard-link grouping, itemize + // computation) exactly as a real run would, but skip every + // filesystem write - see Receiver's doc comment for the audited + // list of exactly which calls that covers. + DryRun bool + // Itemize, when true, writes one real-rsync-format %i line (see + // itemizeFile/itemizeDir/itemizeSymlink) per changed entry to + // Output. Takes precedence over Verbose when both are set, matching + // real rsync's own -i implying strictly more detail than -v alone. + Itemize bool + // Verbose, when true and Itemize is false, writes just the path + // (plus " -> target" for a changed symlink) per changed entry to + // Output - real rsync's own default "%n%L" format for -v without -i. + Verbose bool + // Output is where Itemize/Verbose lines are written, one per line. + // A nil Output is treated as io.Discard, so a caller that wants no + // reporting at all doesn't need to construct a discard writer + // itself. + Output io.Writer +} + +func (o ReceiverOptions) output() io.Writer { + if o.Output == nil { + return io.Discard + } + return o.Output +} + +// Reporting reports whether o requests any change reporting at all +// (Itemize or Verbose) - exported since callers outside this package +// (internal/cli, deciding whether to print its own one-time daemon-PUT +// reporting-gap note) need it too, not just Receiver itself. +func (o ReceiverOptions) Reporting() bool { + return o.Itemize || o.Verbose +} + +// itemizeAttrs holds the 9 attribute-letter positions of real rsync's +// %i format (the "cstpoguax" tail of "YXcstpoguax", see rsync.1's +// --itemize-changes section) in order: checksum/value, size, time, +// perms, owner, group, atime/crtime, ACL, xattr. The last three (atime/ +// crtime, ACL, xattr) are never set to anything but '.' anywhere in this +// package - grsync has no --atimes/--acl/--xattr flags, matching real +// rsync's own behavior when those options aren't given. +type itemizeAttrs [9]byte + +func newItemizeAttrs() itemizeAttrs { + return itemizeAttrs{'.', '.', '.', '.', '.', '.', '.', '.', '.'} +} + +func (a itemizeAttrs) String() string { return string(a[:]) } + +// changed reports whether any position was set to something other than +// ".", i.e. whether this attrs value actually represents a difference +// worth reporting at all. +func (a itemizeAttrs) changed() bool { + for _, b := range a { + if b != '.' { + return true + } + } + return false +} + +const itemizeNewSuffix = "+++++++++" // real rsync's own "newly created" marker, all 9 positions + +// itemizeFile computes the %i code for a regular-file entry, comparing +// it against old (the destination's current state, from +// sync.LstatEntry) when existed is true. contentChanged reports whether +// the file's actual bytes differ - computed by the caller via +// sync.ApplyDelta, since that's the only way grsync (which has no +// --checksum-gated shortcut and no quick-check) can know for certain, +// and it costs nothing extra to compute since ApplyDelta is pure +// in-memory work Receiver already has to do for dry-run's planning-only +// requirement anyway. +// +// report is false exactly when nothing about the entry differs at all - +// matching real rsync's own default (single -i) behavior of not +// mentioning completely unchanged items. +func itemizeFile(entry sync.FileEntry, old sync.FileEntry, existed bool, contentChanged bool, opts sync.AttrOptions) (line string, report bool) { + if !existed { + return ">f" + itemizeNewSuffix, true + } + + a := newItemizeAttrs() + if old.Size != entry.Size { + a[1] = 's' + } + if opts.Times && !old.ModTime.Equal(entry.ModTime) { + a[2] = 't' + } + if opts.Perms && old.Mode.Perm() != entry.Mode.Perm() { + a[3] = 'p' + } + if opts.Owner && old.OwnershipAvailable && entry.OwnershipAvailable && old.UID != entry.UID { + a[4] = 'o' + } + if opts.Group && old.OwnershipAvailable && entry.OwnershipAvailable && old.GID != entry.GID { + a[5] = 'g' + } + + if !a.changed() && !contentChanged { + return "", false + } + + // Real rsync's own distinction: '>' means the file's data was + // actually transferred; '.' means it wasn't (attributes-only + // update) - see the man page's own "." definition: "the item is not + // being updated (though it might have attributes that are being + // modified)". + y := byte('.') + if contentChanged { + y = '>' + } + return string(y) + "f" + a.String(), true +} + +// itemizeDir computes the %i code for a directory entry. Directories +// have no byte content, so there is no "s" (size) or content-changed +// concept for them at all - only the attribute letters real rsync's own +// format actually applies to a directory. +func itemizeDir(entry sync.FileEntry, old sync.FileEntry, existed bool, opts sync.AttrOptions) (line string, report bool) { + if !existed { + return "cd" + itemizeNewSuffix, true + } + + a := newItemizeAttrs() + if opts.Times && !old.ModTime.Equal(entry.ModTime) { + a[2] = 't' + } + if opts.Perms && old.Mode.Perm() != entry.Mode.Perm() { + a[3] = 'p' + } + if opts.Owner && old.OwnershipAvailable && entry.OwnershipAvailable && old.UID != entry.UID { + a[4] = 'o' + } + if opts.Group && old.OwnershipAvailable && entry.OwnershipAvailable && old.GID != entry.GID { + a[5] = 'g' + } + + if !a.changed() { + return "", false + } + return ".d" + a.String(), true +} + +// itemizeSymlink computes the %i code for a symlink entry. A symlink +// is always fully recreated (never diffed byte-by-byte - see +// sync.applySymlink), so its own attribute-"c" position means "the +// link's target value differs", the same "changed value" meaning the +// man page documents for symlinks/devices/specials, distinct from what +// "c" means for a regular file (checksum, which grsync never sets for +// files at all - see itemizeFile). +func itemizeSymlink(entry sync.FileEntry, old sync.FileEntry, existed bool, opts sync.AttrOptions) (line string, report bool) { + if !existed { + return "cL" + itemizeNewSuffix, true + } + + a := newItemizeAttrs() + if old.LinkTarget != entry.LinkTarget { + a[0] = 'c' + } + if opts.Owner && old.OwnershipAvailable && entry.OwnershipAvailable && old.UID != entry.UID { + a[4] = 'o' + } + if opts.Group && old.OwnershipAvailable && entry.OwnershipAvailable && old.GID != entry.GID { + a[5] = 'g' + } + + if !a.changed() { + return "", false + } + // Y is always 'c' when a symlink is being reported at all: unlike a + // regular file, there is no "attributes changed but the link itself + // wasn't re-created" case - applySymlink always removes and + // recreates it (see its own doc comment), so any reported symlink + // change is by definition a local recreation. + return "cL" + a.String(), true +} + +// itemizeHardLink is the %i code for a hard-link group's secondary +// member: real rsync's own 'h' update type ("the item is a hard link to +// another item"). Unlike the other item kinds, this never depends on +// comparing against any prior destination state - sync.ApplyHardLinks +// always removes and relinks unconditionally (see its own doc comment), +// so a secondary member is always reported, exactly like a symlink is +// always reported as a full recreation rather than a partial update. +func itemizeHardLink() string { + return "hf" + itemizeNewSuffix +} + +// formatItemizeLine joins an %i code with the real "%i %n%L" layout +// real rsync's own -i uses: the code, a space, the path, and - for a +// symlink - " -> target". +func formatItemizeLine(code, path, linkTarget string) string { + var b strings.Builder + b.WriteString(code) + b.WriteByte(' ') + b.WriteString(path) + if linkTarget != "" { + b.WriteString(" -> ") + b.WriteString(linkTarget) + } + return b.String() +} + +// formatVerboseLine is real rsync's own "%n%L" format, used when -v is +// given without -i: just the path, plus " -> target" for a symlink. +func formatVerboseLine(path, linkTarget string) string { + if linkTarget == "" { + return path + } + return path + " -> " + linkTarget +} + +// reportChange writes one itemize/verbose line for entry to ropts.Output, +// if ropts requests any reporting at all and report says this entry is +// actually worth mentioning - matching real rsync's own default (single +// -i) behavior of never mentioning a completely unchanged item. +func reportChange(ropts ReceiverOptions, code string, report bool, entry sync.FileEntry) { + if !ropts.Reporting() || !report { + return + } + + linkTarget := "" + if entry.Mode&fs.ModeSymlink != 0 { + linkTarget = entry.LinkTarget + } + + if ropts.Itemize { + _, _ = fmt.Fprintln(ropts.output(), formatItemizeLine(code, entry.Path, linkTarget)) + return + } + _, _ = fmt.Fprintln(ropts.output(), formatVerboseLine(entry.Path, linkTarget)) +} diff --git a/internal/pipeline/itemize_test.go b/internal/pipeline/itemize_test.go new file mode 100644 index 0000000..dfe09b8 --- /dev/null +++ b/internal/pipeline/itemize_test.go @@ -0,0 +1,161 @@ +package pipeline + +import ( + "io/fs" + "testing" + "time" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +var fullAttrOpts = sync.AttrOptions{Perms: true, Times: true, Owner: true, Group: true, Links: true} + +func TestItemizeFile_New(t *testing.T) { + entry := sync.FileEntry{Path: "new.txt"} + code, report := itemizeFile(entry, sync.FileEntry{}, false, true, fullAttrOpts) + if !report { + t.Fatalf("report = false for a new file, want true") + } + if code != ">f+++++++++" { + t.Errorf("code = %q, want %q (real rsync's own \"newly created\" format)", code, ">f+++++++++") + } +} + +func TestItemizeFile_SizeAndTimeChanged(t *testing.T) { + entry := sync.FileEntry{Path: "f.txt", Size: 100, ModTime: time.Unix(2000, 0)} + old := sync.FileEntry{Size: 50, ModTime: time.Unix(1000, 0)} + code, report := itemizeFile(entry, old, true, true, fullAttrOpts) + if !report { + t.Fatalf("report = false for a changed file, want true") + } + // Y='>' (content transferred), X='f', then c=".", s="s", t="t", p/o/g/u/a/x=".". + if code != ">f.st......" { + t.Errorf("code = %q, want %q", code, ">f.st......") + } +} + +func TestItemizeFile_PermsOwnerGroupChanged(t *testing.T) { + entry := sync.FileEntry{ + Path: "f.txt", Size: 10, ModTime: time.Unix(1000, 0), Mode: fs.FileMode(0o644), + UID: 2000, GID: 2000, OwnershipAvailable: true, + } + old := sync.FileEntry{ + Size: 10, ModTime: time.Unix(1000, 0), Mode: fs.FileMode(0o600), + UID: 1000, GID: 1000, OwnershipAvailable: true, + } + // Content unchanged (contentChanged=false) so this exercises the + // "attributes-only update" case: Y must be '.', not '>', matching + // real rsync's own documented meaning of '.' - "the item is not + // being updated (though it might have attributes that are being + // modified)". + code, report := itemizeFile(entry, old, true, false, fullAttrOpts) + if !report { + t.Fatalf("report = false for a file with changed perms/owner/group, want true") + } + if code != ".f...pog..." { + t.Errorf("code = %q, want %q", code, ".f...pog...") + } +} + +func TestItemizeFile_UnchangedIsNotReported(t *testing.T) { + entry := sync.FileEntry{Path: "f.txt", Size: 10, ModTime: time.Unix(1000, 0), Mode: fs.FileMode(0o644)} + old := entry + _, report := itemizeFile(entry, old, true, false, fullAttrOpts) + if report { + t.Errorf("report = true for a completely unchanged file, want false (real rsync's own default -i behavior)") + } +} + +func TestItemizeFile_AttrsIgnoredWhenNotRequested(t *testing.T) { + entry := sync.FileEntry{Path: "f.txt", Size: 10, ModTime: time.Unix(2000, 0), Mode: fs.FileMode(0o600)} + old := sync.FileEntry{Size: 10, ModTime: time.Unix(1000, 0), Mode: fs.FileMode(0o644)} + // opts requests none of Times/Perms/Owner/Group - real rsync's own + // rule is that each attribute letter "requires" its flag; without + // it, even a real underlying difference must not be reported. + _, report := itemizeFile(entry, old, true, false, sync.AttrOptions{}) + if report { + t.Errorf("report = true when no attribute flags were requested, want false") + } +} + +func TestItemizeDir_New(t *testing.T) { + code, report := itemizeDir(sync.FileEntry{Path: "d"}, sync.FileEntry{}, false, fullAttrOpts) + if !report || code != "cd+++++++++" { + t.Errorf("itemizeDir(new) = (%q, %v), want (%q, true)", code, report, "cd+++++++++") + } +} + +func TestItemizeDir_ExistingTimeChanged(t *testing.T) { + entry := sync.FileEntry{Path: "d", ModTime: time.Unix(2000, 0)} + old := sync.FileEntry{ModTime: time.Unix(1000, 0)} + code, report := itemizeDir(entry, old, true, fullAttrOpts) + if !report { + t.Fatalf("report = false for a directory with a changed mtime, want true") + } + // Y='.': a directory is never "created" when it already existed, + // only touched - matching real rsync's own '.' semantics. + if code != ".d..t......" { + t.Errorf("code = %q, want %q", code, ".d..t......") + } +} + +func TestItemizeDir_ExistingUnchangedIsNotReported(t *testing.T) { + entry := sync.FileEntry{Path: "d", ModTime: time.Unix(1000, 0), Mode: fs.FileMode(0o755)} + _, report := itemizeDir(entry, entry, true, fullAttrOpts) + if report { + t.Errorf("report = true for a completely unchanged directory, want false") + } +} + +func TestItemizeSymlink_New(t *testing.T) { + code, report := itemizeSymlink(sync.FileEntry{Path: "l", LinkTarget: "target"}, sync.FileEntry{}, false, fullAttrOpts) + if !report || code != "cL+++++++++" { + t.Errorf("itemizeSymlink(new) = (%q, %v), want (%q, true)", code, report, "cL+++++++++") + } +} + +func TestItemizeSymlink_TargetChanged(t *testing.T) { + entry := sync.FileEntry{Path: "l", LinkTarget: "new-target"} + old := sync.FileEntry{LinkTarget: "old-target"} + code, report := itemizeSymlink(entry, old, true, fullAttrOpts) + if !report { + t.Fatalf("report = false for a symlink with a changed target, want true") + } + // attribute-c means "changed value" for a symlink (not checksum), + // per the man page's own distinction from a regular file's c. + if code != "cLc........" { + t.Errorf("code = %q, want %q", code, "cLc........") + } +} + +func TestItemizeSymlink_UnchangedTargetIsNotReported(t *testing.T) { + entry := sync.FileEntry{Path: "l", LinkTarget: "same-target"} + _, report := itemizeSymlink(entry, entry, true, fullAttrOpts) + if report { + t.Errorf("report = true for a symlink whose target didn't change, want false") + } +} + +func TestItemizeHardLink(t *testing.T) { + if got := itemizeHardLink(); got != "hf+++++++++" { + t.Errorf("itemizeHardLink() = %q, want %q", got, "hf+++++++++") + } +} + +func TestFormatItemizeLine(t *testing.T) { + if got := formatItemizeLine(">f+++++++++", "new.txt", ""); got != ">f+++++++++ new.txt" { + t.Errorf("formatItemizeLine (file) = %q, want %q", got, ">f+++++++++ new.txt") + } + if got := formatItemizeLine("cL+++++++++", "link.txt", "target.txt"); got != "cL+++++++++ link.txt -> target.txt" { + t.Errorf("formatItemizeLine (symlink) = %q, want %q", got, "cL+++++++++ link.txt -> target.txt") + } +} + +func TestFormatVerboseLine(t *testing.T) { + if got := formatVerboseLine("new.txt", ""); got != "new.txt" { + t.Errorf("formatVerboseLine (file) = %q, want %q", got, "new.txt") + } + if got := formatVerboseLine("link.txt", "target.txt"); got != "link.txt -> target.txt" { + t.Errorf("formatVerboseLine (symlink) = %q, want %q", got, "link.txt -> target.txt") + } +} diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 39d7203..6fa7e81 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1,6 +1,7 @@ package pipeline import ( + "bytes" "fmt" "io" "os" @@ -29,7 +30,7 @@ func runSenderReceiver(t *testing.T, src, dest string, walkOpts sync.WalkOptions go func() { senderErrCh <- Sender(sender, src, walkOpts, rules, attrOpts.HardLinks) }() receiverErrCh := make(chan error, 1) - go func() { receiverErrCh <- Receiver(receiver, dest, attrOpts) }() + go func() { receiverErrCh <- Receiver(receiver, dest, attrOpts, ReceiverOptions{}) }() select { case err := <-receiverErrCh: @@ -310,6 +311,141 @@ func TestSenderReceiver_HardLinksNotPreservedWithoutOptIn(t *testing.T) { } } +// runSenderReceiverWithOptions is runSenderReceiver's counterpart for +// tests that need control over ReceiverOptions (dry-run, itemize +// reporting) - kept as a separate helper rather than adding a parameter +// to runSenderReceiver itself, since none of that function's many +// existing callers care, and ReceiverOptions{} (real writes, no +// reporting) is exactly what they already get. +func runSenderReceiverWithOptions(t *testing.T, src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts ReceiverOptions) { + t.Helper() + + senderReadsFromReceiver, receiverWritesToSender := io.Pipe() + receiverReadsFromSender, senderWritesToReceiver := io.Pipe() + + sender := pipeReadWriter{Reader: senderReadsFromReceiver, Writer: senderWritesToReceiver} + receiver := pipeReadWriter{Reader: receiverReadsFromSender, Writer: receiverWritesToSender} + + senderErrCh := make(chan error, 1) + go func() { senderErrCh <- Sender(sender, src, walkOpts, rules, attrOpts.HardLinks) }() + + receiverErrCh := make(chan error, 1) + go func() { receiverErrCh <- Receiver(receiver, dest, attrOpts, ropts) }() + + select { + case err := <-receiverErrCh: + if err != nil { + t.Fatalf("Receiver returned error: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("Receiver did not complete within 10s") + } + select { + case err := <-senderErrCh: + if err != nil { + t.Fatalf("Sender returned error: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("Sender did not complete within 10s") + } +} + +// buildRichTree creates a source tree exercising every write path +// Receiver has: a top-level file, a nested directory with a file inside +// it, a symlink, and (best-effort - silently omitted if unsupported in +// this environment) two hard-linked files - so a dry-run test against it +// genuinely exercises all eight audited write call sites from Receiver's +// own doc comment, not just the easy ones. +func buildRichTree(t *testing.T, root string) { + t.Helper() + mustWriteFile(t, filepath.Join(root, "top.txt"), "top level content") + mustMkdirAll(t, filepath.Join(root, "sub")) + mustWriteFile(t, filepath.Join(root, "sub", "nested.txt"), "nested content") + if err := os.Symlink("nested.txt", filepath.Join(root, "sub", "link.txt")); err != nil { + t.Logf("symlink creation unsupported in this environment, tree will not include one: %v", err) + } + mustWriteFile(t, filepath.Join(root, "original.txt"), "shared content") + if err := os.Link(filepath.Join(root, "original.txt"), filepath.Join(root, "linked.txt")); err != nil { + t.Logf("hard link creation unsupported in this environment, tree will not include one: %v", err) + } +} + +// TestReceiver_DryRunMakesNoFilesystemChanges is SC-11's single most +// important test: a dry-run sync against a completely empty destination +// must leave it completely empty afterward - not just "no error was +// returned." buildRichTree's source is specifically built to exercise +// every one of Receiver's eight audited write call sites (two regular- +// file MkdirAlls plus a WriteFile, a symlink's MkdirAll plus +// ApplyAttributes - which itself calls os.Symlink, not just chmod/ +// chtimes - a directory's deferred ApplyAttributes, and +// ApplyHardLinks); if any one of them were reachable despite DryRun +// being set, this test catches it directly, by finding something in +// destRoot that shouldn't be there, rather than trusting the audit alone. +func TestReceiver_DryRunMakesNoFilesystemChanges(t *testing.T) { + srcRoot := t.TempDir() + destRoot := t.TempDir() + buildRichTree(t, srcRoot) + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, + sync.AttrOptions{Perms: true, Times: true, Owner: true, Group: true, Links: true, HardLinks: true}, + ReceiverOptions{DryRun: true}) + + entries, err := os.ReadDir(destRoot) + if err != nil { + t.Fatalf("ReadDir(destRoot): %v", err) + } + if len(entries) != 0 { + t.Errorf("destRoot is not empty after a dry run: %v", entries) + } +} + +// TestReceiver_DryRunItemizeMatchesRealRunItemize is real rsync's own +// documented dry-run guarantee, made concrete: "The output of +// --itemize-changes is supposed to be exactly the same on a dry run and +// a subsequent real run" (rsync.1's --dry-run section) - compared here +// against a real run on a *separate*, equally fresh destination, not a +// second real run against the same one (which would legitimately report +// nothing left to do, proving nothing about the dry run's accuracy). +func TestReceiver_DryRunItemizeMatchesRealRunItemize(t *testing.T) { + srcRoot := t.TempDir() + buildRichTree(t, srcRoot) + + attrOpts := sync.AttrOptions{Perms: true, Times: true, Owner: true, Group: true, Links: true, HardLinks: true} + + dryRunDest := t.TempDir() + var dryRunOutput bytes.Buffer + runSenderReceiverWithOptions(t, srcRoot, dryRunDest, + sync.WalkOptions{Recursive: true}, nil, attrOpts, + ReceiverOptions{DryRun: true, Itemize: true, Output: &dryRunOutput}) + + realRunDest := t.TempDir() + var realRunOutput bytes.Buffer + runSenderReceiverWithOptions(t, srcRoot, realRunDest, + sync.WalkOptions{Recursive: true}, nil, attrOpts, + ReceiverOptions{DryRun: false, Itemize: true, Output: &realRunOutput}) + + if dryRunOutput.Len() == 0 { + t.Fatalf("dry-run produced no itemize output at all - the test tree isn't exercising anything") + } + if dryRunOutput.String() != realRunOutput.String() { + t.Errorf("dry-run itemize output does not match a real run's:\ndry-run:\n%s\nreal run:\n%s", + dryRunOutput.String(), realRunOutput.String()) + } + + // The dry run's destination must still be untouched - what makes the + // comparison above meaningful in the first place: if the dry run had + // actually written files, it wouldn't be comparable to a real run + // against a genuinely fresh destination at all. + entries, err := os.ReadDir(dryRunDest) + if err != nil { + t.Fatalf("ReadDir(dryRunDest): %v", err) + } + if len(entries) != 0 { + t.Errorf("dry-run destination is not empty: %v", entries) + } +} + // TestReceiver_AppliesHardLinksFromReceivedGroups exercises Receiver's // hard-link handling directly against a hand-built file list, the same // way TestReceiver_ConnectionDropsMidTransfer drives Receiver against a @@ -388,7 +524,7 @@ func TestReceiver_AppliesHardLinksFromReceivedGroups(t *testing.T) { }() receiverErrCh := make(chan error, 1) - go func() { receiverErrCh <- Receiver(receiver, destRoot, sync.AttrOptions{}) }() + go func() { receiverErrCh <- Receiver(receiver, destRoot, sync.AttrOptions{}, ReceiverOptions{}) }() select { case err := <-receiverErrCh: @@ -493,7 +629,7 @@ func TestReceiver_ConnectionDropsMidTransfer(t *testing.T) { }() errCh := make(chan error, 1) - go func() { errCh <- Receiver(receiver, destRoot, sync.AttrOptions{}) }() + go func() { errCh <- Receiver(receiver, destRoot, sync.AttrOptions{}, ReceiverOptions{}) }() select { case err := <-errCh: diff --git a/internal/pipeline/receiver.go b/internal/pipeline/receiver.go index 903e6d9..5eed2ae 100644 --- a/internal/pipeline/receiver.go +++ b/internal/pipeline/receiver.go @@ -1,6 +1,7 @@ package pipeline import ( + "bytes" "fmt" "io" "io/fs" @@ -19,14 +20,26 @@ import ( // group), or - for every other member of a hard-link group - is skipped // here entirely and instead recreated as a real hard link once its // group's first member has been fully written, in the dedicated pass -// below. Attributes are applied per opts along the way. +// below. Attributes are applied per attrOpts along the way. +// +// ropts.DryRun makes every one of those write points a no-op while every +// planning step (signature/delta exchange, hard-link grouping, itemize +// comparison against the destination's current state) still runs +// exactly as it would for a real transfer - see the individual +// receive* helpers below for the specific guarded calls, all eight of +// them audited: two os.MkdirAll calls and an os.WriteFile in +// receiveRegularFile, an os.MkdirAll and sync.ApplyAttributes (which +// itself calls os.Symlink for a symlink entry, not just chmod/chtimes) +// in receiveSymlink, sync.ApplyAttributes for a directory in the +// deferred pass below, and sync.ApplyHardLinks in the hard-link pass +// below. // // A destination file not mentioned in the sender's list is never touched // at all: Receiver only ever acts on paths that appear in the received // list, by construction - there's no separate destination-side walk to // reconcile against it, so nothing here can delete or corrupt an // unrelated file. (Full --delete semantics are explicitly out of scope.) -func Receiver(rw io.ReadWriter, dest string, opts sync.AttrOptions) error { +func Receiver(rw io.ReadWriter, dest string, attrOpts sync.AttrOptions, ropts ReceiverOptions) error { entries, groups, err := recvFileList(rw) if err != nil { return fmt.Errorf("receiving file list: %w", err) @@ -56,6 +69,9 @@ func Receiver(rw io.ReadWriter, dest string, opts sync.AttrOptions) error { // hard-link pass runs before this one for the same reason: os.Link // creates a new directory entry too, and doing that after a // directory's final mtime was already set would bump it right back. + // (A directory's *itemize* comparison still happens at first + // encounter, below, before anything is created inside it - only the + // attribute *application* is deferred.) var dirEntries []sync.FileEntry for _, entry := range entries { @@ -63,52 +79,114 @@ func Receiver(rw io.ReadWriter, dest string, opts sync.AttrOptions) error { switch { case entry.IsDir: - if err := os.MkdirAll(destPath, 0o755); err != nil { - return fmt.Errorf("creating directory %q: %w", entry.Path, err) + if err := receiveDir(destPath, entry, attrOpts, ropts); err != nil { + return err } dirEntries = append(dirEntries, entry) continue case entry.Mode&fs.ModeSymlink != 0: - if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("creating parent directory for %q: %w", entry.Path, err) - } - if _, err := sync.ApplyAttributes(entry, destPath, opts); err != nil { - return fmt.Errorf("creating symlink %q: %w", entry.Path, err) + if err := receiveSymlink(destPath, entry, attrOpts, ropts); err != nil { + return err } continue case secondary[entry.Path]: + reportChange(ropts, itemizeHardLink(), true, entry) continue } - if err := receiveRegularFile(rw, destPath, entry, opts); err != nil { + if err := receiveRegularFile(rw, destPath, entry, attrOpts, ropts); err != nil { return err } } - for _, group := range groups { - if err := sync.ApplyHardLinks(dest, group); err != nil { - return fmt.Errorf("linking hard-link group starting at %q: %w", group[0], err) + if !ropts.DryRun { + for _, group := range groups { + if err := sync.ApplyHardLinks(dest, group); err != nil { + return fmt.Errorf("linking hard-link group starting at %q: %w", group[0], err) + } } } - for i := len(dirEntries) - 1; i >= 0; i-- { - entry := dirEntries[i] - destPath := filepath.Join(dest, filepath.FromSlash(entry.Path)) - if _, err := sync.ApplyAttributes(entry, destPath, opts); err != nil { - return fmt.Errorf("applying attributes to directory %q: %w", entry.Path, err) + if !ropts.DryRun { + for i := len(dirEntries) - 1; i >= 0; i-- { + entry := dirEntries[i] + destPath := filepath.Join(dest, filepath.FromSlash(entry.Path)) + if _, err := sync.ApplyAttributes(entry, destPath, attrOpts); err != nil { + return fmt.Errorf("applying attributes to directory %q: %w", entry.Path, err) + } + } + } + + return nil +} + +// receiveDir handles one directory entry: creates it (unless dry-run), +// and reports its itemize line based on comparing entry against whatever +// already existed at destPath *before* that creation - the comparison +// itself is read-only (sync.LstatEntry), so it runs identically whether +// or not the MkdirAll below actually happens. +func receiveDir(destPath string, entry sync.FileEntry, attrOpts sync.AttrOptions, ropts ReceiverOptions) error { + old, existed, err := lstatExisting(destPath) + if err != nil { + return fmt.Errorf("checking existing %q: %w", entry.Path, err) + } + + if !ropts.DryRun { + if err := os.MkdirAll(destPath, 0o755); err != nil { + return fmt.Errorf("creating directory %q: %w", entry.Path, err) + } + } + + code, report := itemizeDir(entry, old, existed, attrOpts) + reportChange(ropts, code, report, entry) + return nil +} + +// receiveSymlink handles one symlink entry. Guarded on attrOpts.Links up +// front, matching sync.ApplyAttributes' own behavior exactly: without +// --links, a symlink entry is already a silent no-op there (see its doc +// comment), so there is nothing to write and nothing to report either - +// this short-circuit keeps that consistent rather than reporting a +// change that sync.ApplyAttributes would never actually have made. +func receiveSymlink(destPath string, entry sync.FileEntry, attrOpts sync.AttrOptions, ropts ReceiverOptions) error { + if !attrOpts.Links { + return nil + } + + old, existed, err := lstatExisting(destPath) + if err != nil { + return fmt.Errorf("checking existing %q: %w", entry.Path, err) + } + + if !ropts.DryRun { + if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { + return fmt.Errorf("creating parent directory for %q: %w", entry.Path, err) + } + if _, err := sync.ApplyAttributes(entry, destPath, attrOpts); err != nil { + return fmt.Errorf("creating symlink %q: %w", entry.Path, err) } } + code, report := itemizeSymlink(entry, old, existed, attrOpts) + reportChange(ropts, code, report, entry) return nil } // receiveRegularFile handles one regular-file entry: computes a // signature against whatever's currently at destPath (or an empty // signature if nothing is - see below), sends it, receives the sender's -// delta, reconstructs the file, and applies attributes. -func receiveRegularFile(rw io.ReadWriter, destPath string, entry sync.FileEntry, opts sync.AttrOptions) error { +// delta, and reconstructs what the file's bytes would become - all of +// this runs identically in dry-run mode, since it's exactly the planning +// work needed to report accurate itemize output; only the final +// os.WriteFile/os.MkdirAll/sync.ApplyAttributes calls are skipped. +func receiveRegularFile(rw io.ReadWriter, destPath string, entry sync.FileEntry, attrOpts sync.AttrOptions, ropts ReceiverOptions) error { + old, existed, err := lstatExisting(destPath) + if err != nil { + return fmt.Errorf("checking existing %q: %w", entry.Path, err) + } + oldData, err := os.ReadFile(destPath) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("reading existing %q: %w", entry.Path, err) @@ -137,23 +215,52 @@ func receiveRegularFile(rw io.ReadWriter, destPath string, entry sync.FileEntry, if err != nil { return fmt.Errorf("applying delta for %q: %w", entry.Path, err) } + // ApplyDelta is pure in-memory work - computing it, and this + // comparison, costs nothing extra and runs the same whether or not + // the result is about to be written, which is exactly what lets + // dry-run report a genuinely correct "did the content change" bit + // without a --checksum-style flag or a quick-check shortcut this + // codebase doesn't otherwise have (see the README's Dry-Run Mode + // section for why that's a deliberate, disclosed choice, not an + // oversight). + contentChanged := !bytes.Equal(oldData, newData) - // Belt-and-suspenders: the entry's parent directory should already - // exist by this point whenever it was itself part of the transfer - // (Walk's sort guarantees it was created earlier in this same loop), - // but MkdirAll is a cheap no-op when the directory is already there, - // and this removes any fragile dependency on that ordering holding - // for paths whose parent wasn't part of the list at all (e.g. dest - // itself, for a non-recursive sync with no directory entries). - if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("creating parent directory for %q: %w", entry.Path, err) - } - if err := os.WriteFile(destPath, newData, 0o644); err != nil { - return fmt.Errorf("writing %q: %w", entry.Path, err) + if !ropts.DryRun { + // Belt-and-suspenders: the entry's parent directory should already + // exist by this point whenever it was itself part of the transfer + // (Walk's sort guarantees it was created earlier in this same loop), + // but MkdirAll is a cheap no-op when the directory is already there, + // and this removes any fragile dependency on that ordering holding + // for paths whose parent wasn't part of the list at all (e.g. dest + // itself, for a non-recursive sync with no directory entries). + if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { + return fmt.Errorf("creating parent directory for %q: %w", entry.Path, err) + } + if err := os.WriteFile(destPath, newData, 0o644); err != nil { + return fmt.Errorf("writing %q: %w", entry.Path, err) + } + if _, err := sync.ApplyAttributes(entry, destPath, attrOpts); err != nil { + return fmt.Errorf("applying attributes to %q: %w", entry.Path, err) + } } - if _, err := sync.ApplyAttributes(entry, destPath, opts); err != nil { - return fmt.Errorf("applying attributes to %q: %w", entry.Path, err) - } + code, report := itemizeFile(entry, old, existed, contentChanged, attrOpts) + reportChange(ropts, code, report, entry) return nil } + +// lstatExisting is sync.LstatEntry with the "not found" case turned into +// a plain (zero value, false, nil) result instead of an error the caller +// has to unwrap - every call site here wants exactly that: "did +// something already exist, and if so what was it," never treating +// nonexistence itself as a failure. +func lstatExisting(path string) (entry sync.FileEntry, existed bool, err error) { + entry, err = sync.LstatEntry(path) + if err != nil { + if os.IsNotExist(err) { + return sync.FileEntry{}, false, nil + } + return sync.FileEntry{}, false, err + } + return entry, true, nil +} diff --git a/internal/pipeline/ssh_test.go b/internal/pipeline/ssh_test.go index 398a3bf..05cbf74 100644 --- a/internal/pipeline/ssh_test.go +++ b/internal/pipeline/ssh_test.go @@ -98,3 +98,56 @@ func TestSSHLocalhost_SyncRoundTrip(t *testing.T) { assertSameContent(t, filepath.Join(src, "top.txt"), filepath.Join(dest, "top.txt")) assertSameContent(t, filepath.Join(src, "sub", "nested.txt"), filepath.Join(dest, "sub", "nested.txt")) } + +// TestSSHLocalhost_DryRunMakesNoChanges is the real, over-the-wire proof +// that --dry-run's no-write guarantee holds for the SSH transport +// specifically: the remote grsync --server process here is invoked with +// --dry-run on its own command line (exactly what internal/cli's +// syncToRemote does for a real invocation - see its own doc comment), +// so this exercises the actual mechanism a real `grsync --dry-run src +// user@host:dest` run would use, not a stand-in for it. +func TestSSHLocalhost_DryRunMakesNoChanges(t *testing.T) { + requireLocalSSHServer(t) + grsyncPath := buildGrsyncBinary(t) + + src := t.TempDir() + dest := t.TempDir() + mustWriteFile(t, filepath.Join(src, "top.txt"), "top level content") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + + session, err := transport.Dial("", "", "127.0.0.1", []string{grsyncPath, "--server", "--dry-run", dest}) + if err != nil { + t.Fatalf("Dial returned error: %v", err) + } + + if err := transport.Handshake(session); err != nil { + t.Fatalf("Handshake returned error: %v", err) + } + + sendErrCh := make(chan error, 1) + go func() { + sendErrCh <- Sender(session, src, sync.WalkOptions{Recursive: true}, nil, false) + }() + + select { + case err := <-sendErrCh: + if err != nil { + t.Fatalf("Sender returned error: %v", err) + } + case <-time.After(20 * time.Second): + t.Fatal("Sender did not complete within 20s") + } + + if err := session.Close(); err != nil { + t.Errorf("Session.Close returned error: %v", err) + } + + entries, err := os.ReadDir(dest) + if err != nil { + t.Fatalf("ReadDir(dest): %v", err) + } + if len(entries) != 0 { + t.Errorf("dest is not empty after a --dry-run --server sync over real SSH: %v", entries) + } +} diff --git a/internal/sync/walk.go b/internal/sync/walk.go index a30de4c..d80f008 100644 --- a/internal/sync/walk.go +++ b/internal/sync/walk.go @@ -49,6 +49,60 @@ type WalkOptions struct { Dirs bool } +// buildFileEntry constructs a FileEntry for path from its already-Lstat'd +// info, leaving Path unset - Walk fills it in as a root-relative, +// "/"-separated string; LstatEntry (below) leaves it empty, since a +// caller comparing a single existing destination path against a received +// FileEntry has no use for a second, redundant Path value. +func buildFileEntry(path string, info fs.FileInfo) (FileEntry, error) { + entry := FileEntry{ + Size: info.Size(), + ModTime: info.ModTime(), + Mode: info.Mode(), + IsDir: info.IsDir(), + } + + // lookupUIDGID is platform-specific (see uidgid_unix.go / + // uidgid_windows.go): on Windows it always reports unavailable, + // leaving UID/GID at their zero value. OwnershipAvailable carries + // that ok flag through so callers can't mistake the zero value for + // a real uid/gid of 0 (root) - see uidgid_windows.go for why. + entry.UID, entry.GID, entry.OwnershipAvailable = lookupUIDGID(info) + + // info.Mode()&fs.ModeSymlink is only ever set by Lstat (Stat + // resolves through it), which is exactly why every caller of this + // function Lstats rather than Stats: this branch would never be + // reachable otherwise. + if info.Mode()&fs.ModeSymlink != 0 { + target, err := os.Readlink(path) + if err != nil { + return FileEntry{}, err + } + entry.LinkTarget = target + } + + return entry, nil +} + +// LstatEntry builds a FileEntry for a single existing path the same way +// Walk builds one for each path it visits (same fields, same Lstat-not-Stat +// symlink handling), without needing a whole tree walk - for a caller that +// already knows the one path it cares about, e.g. comparing a sync's +// incoming FileEntry against whatever currently exists at the destination. +// Path is left empty; the caller already knows what path it asked about. +// +// The returned error satisfies os.IsNotExist(err) when path doesn't exist, +// exactly like a direct os.Lstat call would - callers should check for +// that the same way they already do for os.ReadFile/os.Stat elsewhere in +// this codebase, not a distinct "found bool" out-parameter. +func LstatEntry(path string) (FileEntry, error) { + info, err := os.Lstat(path) + if err != nil { + return FileEntry{}, err + } + return buildFileEntry(path, info) +} + // Walk collects a FileEntry for every entry found under root, not including // root itself, subject to opts. Paths are relative to root and // "/"-separated. @@ -87,31 +141,11 @@ func Walk(root string, opts WalkOptions) ([]FileEntry, error) { return err } - entry := FileEntry{ - Path: filepath.ToSlash(rel), - Size: info.Size(), - ModTime: info.ModTime(), - Mode: info.Mode(), - IsDir: info.IsDir(), - } - - // lookupUIDGID is platform-specific (see uidgid_unix.go / - // uidgid_windows.go): on Windows it always reports unavailable, - // leaving UID/GID at their zero value. OwnershipAvailable carries - // that ok flag through so callers can't mistake the zero value for - // a real uid/gid of 0 (root) - see uidgid_windows.go for why. - entry.UID, entry.GID, entry.OwnershipAvailable = lookupUIDGID(info) - - // info.Mode()&fs.ModeSymlink is only ever set by Lstat (Stat - // resolves through it), which is exactly why Lstat was required - // above: this branch would never be reachable otherwise. - if info.Mode()&fs.ModeSymlink != 0 { - target, err := os.Readlink(path) - if err != nil { - return err - } - entry.LinkTarget = target + entry, err := buildFileEntry(path, info) + if err != nil { + return err } + entry.Path = filepath.ToSlash(rel) entries = append(entries, entry) diff --git a/internal/sync/walk_test.go b/internal/sync/walk_test.go index d57d303..a550ecd 100644 --- a/internal/sync/walk_test.go +++ b/internal/sync/walk_test.go @@ -96,6 +96,46 @@ func TestWalk_Symlink(t *testing.T) { } } +// TestLstatEntry_MatchesWalk confirms LstatEntry and Walk build identical +// FileEntry data for the same path (everything except Path itself, which +// Walk fills in as root-relative and LstatEntry deliberately leaves +// empty) - proving the extracted buildFileEntry helper didn't silently +// diverge between the two call sites. +func TestLstatEntry_MatchesWalk(t *testing.T) { + root := t.TempDir() + mustWriteFile(t, filepath.Join(root, "file.txt"), "content") + + walked, err := Walk(root, WalkOptions{Recursive: true}) + if err != nil { + t.Fatalf("Walk returned error: %v", err) + } + if len(walked) != 1 { + t.Fatalf("got %d entries, want 1", len(walked)) + } + want := walked[0] + + got, err := LstatEntry(filepath.Join(root, "file.txt")) + if err != nil { + t.Fatalf("LstatEntry returned error: %v", err) + } + + if got.Path != "" { + t.Errorf("LstatEntry Path = %q, want empty", got.Path) + } + if got.Size != want.Size || got.Mode != want.Mode || !got.ModTime.Equal(want.ModTime) || + got.IsDir != want.IsDir || got.UID != want.UID || got.GID != want.GID || + got.OwnershipAvailable != want.OwnershipAvailable { + t.Errorf("LstatEntry = %+v, want the same fields as Walk's entry (ignoring Path) = %+v", got, want) + } +} + +func TestLstatEntry_NonexistentPathReturnsNotExist(t *testing.T) { + _, err := LstatEntry(filepath.Join(t.TempDir(), "does-not-exist")) + if !os.IsNotExist(err) { + t.Errorf("LstatEntry on a nonexistent path returned err = %v, want an error satisfying os.IsNotExist", err) + } +} + func TestWalk_RecursiveAndDirsFlags(t *testing.T) { root := t.TempDir() diff --git a/internal/transport/session.go b/internal/transport/session.go index 8fc4d97..e33fe79 100644 --- a/internal/transport/session.go +++ b/internal/transport/session.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "io" + "os" "os/exec" "strings" ) @@ -21,9 +22,15 @@ type Session struct { // Dial spawns the remote-shell command built by BuildRSHCommand (ssh, or // whatever --rsh/-e overrides it to) and returns a Session wrapping its -// stdin/stdout. The subprocess's stderr is captured (not connected to -// this process's own stderr) so it can be surfaced as part of a -// meaningful error from Close if the process exits non-zero. +// stdin/stdout. The subprocess's stderr is both captured (so it can be +// surfaced as part of a meaningful error from Close if the process exits +// non-zero) and passed straight through to this process's own stderr +// live, as it arrives - not just replayed after the fact. That passthrough +// is what lets a remote --server process's own itemize/verbose output +// (see internal/cli's runServer, which writes exactly there, never to +// stdout - stdout here is the framed wire protocol itself) actually +// reach the local user's terminal during a real-time transfer, the same +// way real rsync's own remote messages do. // // Host-key verification is never touched here: this deliberately never // adds flags like "-o StrictHostKeyChecking=no" or a null @@ -56,7 +63,7 @@ func Dial(rsh, user, host string, remoteArgs []string) (*Session, error) { } var stderr bytes.Buffer - cmd.Stderr = &stderr + cmd.Stderr = io.MultiWriter(&stderr, os.Stderr) if err := cmd.Start(); err != nil { // Unlike the StdoutPipe case above, Start failing here *does*