diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b0c6298..0fded09 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -75,12 +75,15 @@ jobs: # windows-latest's own default toolchain without access to a real # GitHub Actions runner to test against - so -race runs where it's # known-good (Linux), and windows-latest still gets a full, - # real, native (not cross-compiled) test run without it. The - # actual value -race provides - catching a genuine data race in - # this project's own logic (SC-10's progress-reporter goroutine, - # SC-6's per-connection daemon handling) - isn't platform-specific - # in what it would find, so one platform running it is enough to - # get that benefit. + # real, native (not cross-compiled) test run without it. This + # isn't just checkbox coverage: the first real CI run of this leg + # caught a genuine deadlock in syncLocal (internal/cli/sync.go) - + # a receiver failure partway through a sync left the sender + # goroutine parked forever reading a reply that would never come, + # timing out the whole test binary after 10 minutes. The bug ran + # fine every time on Windows without -race (the race window never + # opened), which is exactly why relying on local, non-race runs + # alone would never have caught it. - name: go test -race (Linux) if: runner.os == 'Linux' run: go test -race -timeout 10m ./... diff --git a/README.md b/README.md index 3a31196..636fd03 100644 --- a/README.md +++ b/README.md @@ -1589,14 +1589,21 @@ matters as much as its size for real throughput. Windows runners. `-race` runs on the Linux leg only: it requires cgo and a C toolchain, reliably available on `ubuntu-latest` but not something this project can verify for `windows-latest` without a real runner to -test against, and the races it would actually catch (SC-10's -progress-reporter goroutine, SC-6's per-connection daemon handling) -aren't platform-specific in nature, so one reliable platform is enough. -The Linux leg also installs a real `rsync` binary so the comparison tests -above get a genuine execution somewhere in CI, not just a proof they can -skip; the Windows leg has no equivalent easy install and instead -exercises the graceful-skip path, matching a real Windows dev machine -without rsync. +test against. This isn't theoretical - the first real CI run of this leg +caught a genuine deadlock in `syncLocal` (`internal/cli/sync.go`): a +receiver failure partway through a sync left the sender goroutine parked +forever reading a reply that would never come, timing out the whole test +binary after 10 minutes. The same test passed instantly, every time, on +Windows without `-race` - the race window only opened under `-race`'s own +added scheduling overhead - which is exactly why local, non-race runs +alone would never have caught it. (Fixed by having both `syncLocal` +goroutines close the pipe halves they own, with `CloseWithError`, once +they're done - see the function's own comments for the full +explanation.) The Linux leg also installs a real `rsync` binary so the +comparison tests above get a genuine execution somewhere in CI, not just +a proof they can skip; the Windows leg has no equivalent easy install and +instead exercises the graceful-skip path, matching a real Windows dev +machine without rsync. ## Architecture diff --git a/internal/cli/sync.go b/internal/cli/sync.go index a5c7d00..0fb502c 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -13,17 +13,13 @@ import ( "github.com/syntaxroot-cc/grsync/internal/transport" ) -// pipeReadWriter joins two separate io.Reader/io.Writer halves into a -// single io.ReadWriter, needed anywhere a connection is represented as -// two directional pipe ends (the local-to-local case below) or as a -// command's separate stdin/stdout (the --server case). +// pipeReadWriter joins separate Reader/Writer halves into a single io.ReadWriter. type pipeReadWriter struct { io.Reader io.Writer } -// effectiveWalkOptions computes sync.WalkOptions from opts: --archive -// implies --recursive, matching real rsync's -a (-rlptgoD). +// effectiveWalkOptions computes sync.WalkOptions from opts; --archive implies --recursive. func effectiveWalkOptions(opts *options) sync.WalkOptions { return sync.WalkOptions{ Recursive: opts.archive || opts.recursive, @@ -31,14 +27,9 @@ func effectiveWalkOptions(opts *options) sync.WalkOptions { } } -// effectiveAttrOptions computes sync.AttrOptions from opts: --archive -// implies perms/times/owner/group/links, matching real rsync's -a -// (-rlptgoD, minus the r which effectiveWalkOptions handles, and minus -// devices/specials - see the README's note on why device files are -// deferred rather than wired up here). HardLinks is deliberately NOT -// included in that implication: real rsync's own -a does not imply -H -// either (-a is exactly -rlptgoD, no H), so --archive alone must not -// turn hard-link detection on. +// effectiveAttrOptions computes sync.AttrOptions from opts; --archive implies +// perms/times/owner/group/links but not hard links, matching real rsync's -a +// (-rlptgoD, which does not include -H). func effectiveAttrOptions(opts *options) sync.AttrOptions { return sync.AttrOptions{ Perms: opts.archive || opts.perms, @@ -50,9 +41,7 @@ func effectiveAttrOptions(opts *options) sync.AttrOptions { } } -// effectiveReceiverOptions computes pipeline.ReceiverOptions from opts: -// --dry-run, --itemize-changes, --verbose, --progress, and --stats, all -// reported to output. +// effectiveReceiverOptions computes pipeline.ReceiverOptions from opts. func effectiveReceiverOptions(opts *options, output io.Writer) pipeline.ReceiverOptions { return pipeline.ReceiverOptions{ DryRun: opts.dryRun, @@ -69,21 +58,13 @@ func effectiveReceiverOptions(opts *options, output io.Writer) pipeline.Receiver } // effectiveCompressOptions computes pipeline.CompressOptions from opts, -// mirroring real rsync's own --compress-level implication rule ("The -// --compress option is implied as long as the level chosen is not a -// 'don't compress' level" - rsync.1): compression is enabled whenever -// either -z or --compress-level was given at all, at whatever level -// --compress-level requested (clamped) or the real-rsync-verified -// default of 6 if only -z was given, UNLESS that level clamps to 0 -// ("off"), which disables compression outright even if -z was also -// given - matching real rsync's own documented "--zl=0 turns compression -// off" behavior exactly, including its override of -z. +// matching real rsync's rule that --compress is implied whenever +// --compress-level is given a non-zero level. // -// cmd.Flags().Changed, not opts.compressLevel's zero value, is what +// cmd.Flags().Changed (not opts.compressLevel's zero value) is what // distinguishes "--compress-level was never given" from "--compress-level=0 -// was given explicitly" - those two cases mean different things (default -// level 6 vs. explicitly off) and pflag's own IntVar can't tell them -// apart by value alone, since 0 is also its unset zero value. +// was given explicitly," since pflag's IntVar can't tell those apart by +// value alone. func effectiveCompressOptions(cmd *cobra.Command, opts *options) pipeline.CompressOptions { levelGiven := cmd.Flags().Changed("compress-level") if !opts.compress && !levelGiven { @@ -106,12 +87,8 @@ func effectiveCompressOptions(cmd *cobra.Command, opts *options) pipeline.Compre } // 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", -// "include-from"), so this is a direct conversion rather than a mapping -// table - if the two ever drift apart, this line stops compiling as a -// straight cast, which is a more useful failure mode than a silent -// mismatch would be. +// FilterRuleType's string values are chosen to match sync.RuleKind's +// exactly, so this is a direct cast rather than a mapping table. func toSyncRawRules(filterRules []FilterRule) []sync.RawRule { raw := make([]sync.RawRule, len(filterRules)) for i, r := range filterRules { @@ -120,18 +97,10 @@ func toSyncRawRules(filterRules []FilterRule) []sync.RawRule { return raw } -// 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 - -// this scope is explicitly "push," not pull mode, matching the existing -// SSH-transport restriction rather than introducing a new asymmetry. +// runSync is the real sync entry point: for each source, syncs it into +// destination, in-process for a local destination, over SSH for a +// user@host:path one, or over TCP for an rsync:// daemon module. Pulling +// from a remote source is not yet supported, only pushing to one. func runSync(cmd *cobra.Command, sources []string, destination string, opts *options) error { for _, src := range sources { if isRsyncURL(src) { @@ -150,15 +119,6 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt return fmt.Errorf("compiling filter rules: %w", err) } - // Resolved once, up front, regardless of destination type: --ipv4 and - // --ipv6 conflicting is a flag-level error, not something that should - // only surface once a particular destination happens to need it (see - // tcpNetwork's own doc comment for why the combination is rejected - // outright rather than picking one). network only actually changes - // behavior for an rsync:// daemon destination (syncToRsyncDaemon, - // below) and --ipv4/--ipv6's forwarding to ssh (syncToRemote, below) - - // it's meaningless for a local sync, exactly like real rsync's own - // scope for these flags. network, err := tcpNetwork(opts.ipv4, opts.ipv6) if err != nil { return err @@ -168,54 +128,30 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt return err } - // --append/--append-verify are two different behaviors for the same - // underlying idea (see the README's Partial and Append Transfers - // section for the real, verified-against-source distinction) - like - // --ipv4/--ipv6 above, rejecting the combination outright is clearer - // than silently preferring one. if opts.appendMode && opts.appendVerify { return fmt.Errorf("--append and --append-verify are mutually exclusive") } - // --write-batch and --read-batch can never both reach here at once - - // root.go's own Args validator rejects that combination before RunE - // ever dispatches to runSync or runReadBatch - so only --write-batch - // needs handling in this function at all. + // root.go's Args validator already rejects --write-batch and + // --read-batch together, so only --write-batch needs handling here. // - // writingBatch, not opts.writeBatch != "" directly, is what the rest - // of this function actually consults: real rsync's own source - // (options.c) silently disables --write-batch entirely when --dry-run - // is set ("else if (dry_run) write_batch = 0") rather than treating - // the combination as an error - a dry run never computes a real delta - // to capture in the first place, so there is nothing genuine to write. - // grsync replicates that exact behavior (not an invented one) but, - // consistent with this project's own established preference for - // disclosure over silence (see e.g. --compress-level=0's own note), - // prints an explicit one-time note rather than leaving it fully - // silent the way real rsync does. + // Real rsync silently disables --write-batch under --dry-run + // (options.c: "else if (dry_run) write_batch = 0"), since a dry run + // never computes a real delta to capture. grsync matches that + // behavior but prints an explicit note instead of staying silent. writingBatch := opts.writeBatch != "" if writingBatch && opts.dryRun { _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --write-batch has no effect combined with --dry-run "+ "(matching real rsync's own behavior - a dry run never computes a real delta to capture) - no batch file will be written") writingBatch = false } - // A batch file's own file-list frame corresponds to exactly one - // Sender/Receiver session; concatenating more than one source's worth - // of sessions into a single file would leave pipeline.Receiver's - // single recvFileList call (see runReadBatch) unable to correctly - // replay anything past the first - so, unlike an ordinary sync, - // --write-batch requires exactly one source, checked explicitly - // rather than silently only capturing the first. + // A batch file holds exactly one Sender/Receiver session's file-list + // frame; runReadBatch's single recvFileList call can't replay more + // than one, so --write-batch requires exactly one source. if writingBatch && len(sources) != 1 { return fmt.Errorf("--write-batch requires exactly one source, got %d", len(sources)) } - // isRsyncURL is checked, and rsyncURL parsed, before - // transport.ParseRemotePath ever looks at destination: an rsync:// - // URL is never valid [user@]host:path syntax (ParseRemotePath itself - // now refuses anything containing "://"), but checking here first - // means that's true by construction, not just by the two parsers - // happening to agree. var rsyncURL daemon.URL isRsyncDaemon := isRsyncURL(destination) if isRsyncDaemon { @@ -234,36 +170,18 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt remote, isRemote := transport.ParseRemotePath(destination) if writingBatch && isRsyncDaemon { - // Unlike the local and SSH cases, there is no clean tap point - // here: pipeline.Sender's writes to an rsync:// daemon connection - // share the same net.Conn the greeting/auth handshake already - // used (see daemon.DialClient), so capturing only the - // batch-worthy frames (not the handshake bytes ahead of them) - // would need real daemon-package changes, not just a wrapped - // io.Writer at the call site the way syncLocal/syncToRemote use - // below. Rejecting outright is more honest than silently - // producing an empty or corrupt batch file. + // A daemon connection's Sender writes share the same net.Conn the + // handshake used (daemon.DialClient), so tapping only the + // batch-worthy frames would need daemon-package changes, not just + // a wrapped io.Writer here. return fmt.Errorf("--write-batch is not supported for an rsync:// daemon destination") } - // Opened once, before the (now guaranteed-single) source's own sync - // runs, and closed explicitly once it succeeds - see the deferred - // cleanup below for the early-return safety net. batchFile is nil - // whenever writingBatch is false, and every call site downstream - // checks that explicitly rather than passing a possibly-nil - // *os.File where an io.Writer is expected, avoiding the same - // typed-nil-interface gotcha SC-14's own resolveLocalAddr caller had - // to guard against. - // - // Self-review finding: if the sync itself fails partway (the loop - // below returns an error), batchFile is still non-nil when this - // deferred cleanup runs - on top of closing it, it also removes the - // file entirely, rather than leaving a truncated, half-written batch - // file on disk that looks like a real deliverable but would only - // ever fail (or silently under-apply) on a later --read-batch. The - // success path below sets batchFile back to nil once it has already - // closed the file cleanly, which is what turns this into a no-op for - // a genuinely completed batch. + // batchFile is closed and cleared to nil on the success path below; + // if the sync fails partway, this deferred cleanup also removes the + // file, rather than leaving a truncated one that looks like a real + // deliverable but would only ever fail (or silently under-apply) on + // a later --read-batch. var batchFile *os.File if writingBatch { f, err := os.Create(opts.writeBatch) @@ -279,10 +197,9 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt }() } - // Resolved once, outside the per-source loop below, so a multi-source - // sync against the same daemon destination only ever prompts for (or - // reads) a password once - not once per source, even though each - // source gets its own connection, same as the SSH path already does. + // Resolved once, outside the per-source loop, so a multi-source sync + // against the same daemon destination only prompts for (or reads) a + // password once. var password daemon.PasswordFunc if isRsyncDaemon { password = resolvePassword(opts.passwordFile, cmd.InOrStdin()) @@ -290,43 +207,22 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt ropts := effectiveReceiverOptions(opts, cmd.OutOrStdout()) if isRsyncDaemon && ropts.Reporting() { - // The daemon protocol has no channel for any of 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 reporting text - // back to the client, unlike SSH's genuinely separate stderr - // stream. This was SC-11's disclosed limitation for - // itemize/verbose; --progress/--stats have the exact same - // limitation for the exact same reason, not a new gap - see the - // README's Progress and Stats section. --dry-run's actual - // no-write guarantee still fully applies regardless; 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. + // Once the module handshake ends, the daemon connection is pure + // binary wire protocol with no channel for reporting text back to + // the client, unlike SSH's separate stderr stream. --dry-run's + // no-write guarantee still applies; only the reporting text is + // unavailable. _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --itemize-changes/--verbose/--progress/--stats 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") } if isRsyncDaemon && (ropts.KeepPartial() || ropts.AppendMode()) { - // A different flavor of the same underlying limitation: for an - // rsync:// daemon upload, the module's Receiver runs on the - // server (see daemon.ServeModule's own DirectionPut comment), not - // here, and syncToRsyncDaemon only ever forwards DryRun to it (via - // daemon.dryRunToken) - not the rest of ReceiverOptions. Extending - // that wire protocol to also carry Partial/PartialDir/Append/ - // AppendVerify is real, separate protocol work outside SC-12's own - // scope (it depends on SC-17's daemon-CLI wiring, not the other - // way around), so this direction silently ignores all four rather - // than honoring them - noting that plainly, once, rather than - // letting the user assume they took effect. + // The module's Receiver runs on the server (daemon.ServeModule), + // and syncToRsyncDaemon only forwards DryRun to it, not these - + // extending the wire protocol to carry them is separate work. _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --partial/--partial-dir/--append/--append-verify are not available "+ "for an rsync:// daemon destination (the module's receiver runs on the server, which has no way to learn these were requested)") } - // batchFile is nil unless writingBatch - every branch below passes it - // straight through as the io.Writer syncLocal/syncToRemote tee the - // sender's own output into (see their own doc comments); syncToRsyncDaemon - // is never reached when writingBatch, since that combination already - // returned an error above. var batchWriter io.Writer if batchFile != nil { batchWriter = batchFile @@ -351,7 +247,7 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt if batchFile != nil { closeErr := batchFile.Close() - batchFile = nil // the deferred cleanup above becomes a no-op now that this succeeded + batchFile = nil // makes the deferred cleanup above a no-op if closeErr != nil { return fmt.Errorf("closing batch file %q: %w", opts.writeBatch, closeErr) } @@ -366,21 +262,12 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt return err } -// syncLocal runs the sender and receiver in-process, connected by a pair -// of io.Pipes, rather than a separate code path for the local case: this -// 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. +// syncLocal runs the sender and receiver in-process, connected by a pair of +// io.Pipes, so a local sync exercises the same pipeline.Sender/Receiver +// functions a remote sync uses. // -// batchWriter, when non-nil (--write-batch), receives a byte-for-byte -// copy of everything Sender writes to the receiver - Sender only ever -// writes FrameFileList once and FrameDelta per regular file on this -// particular pipe (every other message on this connection flows the -// other way, from Receiver to Sender), so that copy is already exactly -// the batch file's own content, with no filtering needed. See -// runReadBatch for the other half of this: replaying that same byte -// stream back into a fresh pipeline.Receiver call with no live Sender at -// all. +// batchWriter, when non-nil (--write-batch), receives a byte-for-byte copy +// of everything Sender writes to the receiver. func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions, copts pipeline.CompressOptions, batchWriter io.Writer) error { senderReadsFromReceiver, receiverWritesToSender := io.Pipe() receiverReadsFromSender, senderWritesToReceiver := io.Pipe() @@ -394,9 +281,22 @@ func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, a receiver := pipeReadWriter{Reader: receiverReadsFromSender, Writer: receiverWritesToSender} senderErrCh := make(chan error, 1) - go func() { senderErrCh <- pipeline.Sender(sender, src, walkOpts, rules, attrOpts.HardLinks, copts) }() + go func() { + err := pipeline.Sender(sender, src, walkOpts, rules, attrOpts.HardLinks, copts) + // Without closing both pipe halves here, a failure on either side + // partway through the protocol can leave the other blocked on a + // pipe read/write forever, since neither pipeline.Sender nor + // Receiver ever closes them itself (found by go test -race + // hanging on a receiver-failure test for its full timeout). + _ = senderWritesToReceiver.CloseWithError(err) + _ = senderReadsFromReceiver.Close() + senderErrCh <- err + }() receiverErr := pipeline.Receiver(receiver, dest, attrOpts, ropts) + _ = receiverWritesToSender.CloseWithError(receiverErr) + _ = receiverReadsFromSender.Close() + senderErr := <-senderErrCh if receiverErr != nil { @@ -405,50 +305,15 @@ func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, a return senderErr } -// 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. -// -// ropts.DryRun/Itemize/Verbose/Progress/Stats 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 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. This is the same mechanism SC-11 established for -// DryRun/Itemize/Verbose; Progress/Stats, and now Partial/PartialDir/ -// Append/AppendVerify, just reuse it rather than inventing a second -// one - the remote --server process's own Receiver call decides -// everything about temp files/resumption/append locally, from its own -// argv, exactly like it already decides DryRun. +// 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. // -// copts needs none of that: --compress/-z is a Sender-side decision (see -// pipeline.CompressOptions' own doc comment), and Sender runs right here, -// locally, for this transport - there is nothing for the remote -// --server process to be told via argv at all. The remote Receiver just -// decompresses whatever each deltaMessage's own Compressed marker says, -// exactly like every other transport. -// -// ipv4/ipv6 are SC-14's own contribution, and land somewhere different -// again: grsync never dials this connection itself at all (ssh, or -// whatever --rsh overrides it to, does), so there's no net.Dial call -// here to pass a network string to. Instead they're forwarded straight -// through to transport.Dial/BuildRSHCommand, which inserts a real -4/-6 -// onto the spawned command's own argv - but only when that command is -// genuinely ssh (see BuildRSHCommand's own doc comment for exactly when, -// verified against real rsync's own identical behavior). --address has -// no equivalent here at all: real rsync's own documented --address scope -// never includes the rsh/ssh transport (see resolveLocalAddr's own doc -// comment), so it isn't threaded through to this function. -// -// batchWriter is syncLocal's own SC-13 contribution, applying here too: -// once the handshake completes, session's own Write calls carry exactly -// the same FrameFileList/FrameDelta bytes a local sync's sender-to- -// receiver pipe does (session.Read is where the receiver's own signature -// traffic and any live stderr passthrough arrive - never mixed into -// Write), so tapping it after the handshake captures precisely the -// batch-worthy bytes and nothing from the handshake itself. +// ropts' reporting/partial/append fields are passed as extra flags on the +// remote command line rather than over a new wire message; the remote +// --server process parses them normally and its own Receiver call decides +// everything locally. --address has no equivalent here: it's out of scope +// for the ssh/rsh transport, matching real rsync. func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool, ropts pipeline.ReceiverOptions, copts pipeline.CompressOptions, ipv4, ipv6 bool, batchWriter io.Writer) error { remoteArgs := []string{"grsync", "--server"} if ropts.DryRun { @@ -467,12 +332,8 @@ func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.Wa remoteArgs = append(remoteArgs, "--stats") } if ropts.PartialDir != "" { - // --partial-dir implies --partial (see ReceiverOptions.KeepPartial's - // own doc comment), so there's no need to also forward a bare - // --partial alongside it - one less argv token, and it avoids ever - // sending a value that could look like two separate flags if it - // happened to be forwarded as "--partial-dir" "value" instead of - // this single "--partial-dir=value" token. + // --partial-dir implies --partial, so there's no need to also + // forward a bare --partial alongside it. remoteArgs = append(remoteArgs, "--partial-dir="+ropts.PartialDir) } else if ropts.Partial { remoteArgs = append(remoteArgs, "--partial") @@ -509,22 +370,13 @@ func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.Wa return closeErr } -// 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. +// runServer implements --server mode: performs the handshake, then runs 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. +// Itemize/verbose output goes to cmd.ErrOrStderr(), never stdout: stdout is +// the framed wire protocol itself, so writing human-readable text there +// would corrupt it. func runServer(cmd *cobra.Command, dest string, opts *options) error { stdin, stdout := cmd.InOrStdin(), cmd.OutOrStdout() @@ -538,31 +390,15 @@ func runServer(cmd *cobra.Command, dest string, opts *options) error { } // runReadBatch implements --read-batch=FILE: applies the file list and -// per-file deltas previously captured by --write-batch (see syncLocal/ -// syncToRemote's own batchWriter doc comments) directly to dest, with no -// source argument, source walk, or live sender connection of any kind - -// FILE already carries everything pipeline.Receiver needs. -// -// This reuses Receiver completely unchanged, exactly as the ticket -// asked for: Receiver has no idea, and no need to know, whether the -// io.ReadWriter it was given is a live connection or a replayed file. -// Its own signature writes (sendSignature, receiver.go) go to -// io.Discard - there is no live sender left to read them, and none is -// needed, since the recorded deltas were already computed against a -// real signature at write-batch time, not against whatever Receiver's -// own fresh signature-generation happens to produce here. Its delta -// reads (recvDelta) come from FILE instead of a socket, in exactly the -// order Sender originally wrote them, which recvFileList/recvDelta's -// own framing (transport.ReadFrame) requires nothing special to handle. +// per-file deltas previously captured by --write-batch directly to dest, +// with no source argument or live sender connection. // -// A malformed or foreign (e.g. real-rsync-produced) FILE fails here with -// a clear, ordinary decode/frame-type error from the same -// recvFileList/recvDelta machinery a live sync already relies on to -// reject a corrupted or out-of-order connection - there is no dedicated -// batch-format validation to bypass, because there is no separate batch -// format at all: it's the same gob-framed messages either way (see the -// README's Batch Mode section for why that's a deliberate scope -// decision, not an oversight). +// This reuses Receiver unchanged: its signature writes go to io.Discard +// (there's no live sender to read them), and its delta reads come from +// FILE instead of a socket. A malformed or foreign FILE fails with the +// same frame-decode errors a live sync already relies on to reject a +// corrupted connection - there's no separate batch-format validation +// because there's no separate batch format at all. func runReadBatch(cmd *cobra.Command, dest string, opts *options) error { var r io.Reader if opts.readBatch == "-" { diff --git a/internal/daemon/auth.go b/internal/daemon/auth.go index b2f5fff..ee08ceb 100644 --- a/internal/daemon/auth.go +++ b/internal/daemon/auth.go @@ -14,63 +14,36 @@ import ( "golang.org/x/crypto/md4" ) -// ErrAuthFailed is returned by ServeAuth and DialAuth when authentication -// is attempted and fails - a wrong password, an unauthorized user, or a -// secrets file that can't be read. The wire-level detail behind it is -// deliberately generic (see ServeAuth), matching real rsync's own refusal -// to distinguish "no such user" from "wrong password" in its response. +// ErrAuthFailed is returned by ServeAuth and DialAuth when authentication fails. var ErrAuthFailed = errors.New("authentication failed") -// AuthRequired reports whether a client must authenticate to use m: real -// rsyncd.conf's own rule is that a module requires auth exactly when it -// has a non-empty "auth users" list. +// AuthRequired reports whether a client must authenticate to use m. func (m Module) AuthRequired() bool { return len(m.AuthUsers) > 0 } // PasswordFunc resolves the password to use for daemon authentication. -// DialAuth calls it at most once, and only if the server actually -// challenges for a password (an "@RSYNCD: AUTHREQD" line) - matching real -// rsync's own client behavior, where auth_client() is only ever invoked -// in response to that same line. Connecting to a module that turns out -// not to require authentication never calls this at all, so a caller -// backing it with an interactive terminal prompt or a --password-file -// read never triggers either one against an anonymous module - resolving -// eagerly, before knowing whether the server will ask, would be a real -// regression from real rsync's behavior here, not just a style choice. +// DialAuth calls it at most once, and only if the server sends an +// "@RSYNCD: AUTHREQD" challenge; a module that turns out not to require +// auth never triggers a call at all. type PasswordFunc func() (string, error) -// StaticPassword wraps an already-known password (e.g. a test fixture, or -// a caller that has already decided eager resolution is fine) as a -// PasswordFunc. +// StaticPassword wraps an already-known password as a PasswordFunc. func StaticPassword(password string) PasswordFunc { return func() (string, error) { return password, nil } } -// md4Hash returns the base64-encoded (standard alphabet, no padding - the -// same encoding real rsync's own base64_encode(..., pad=0) produces) MD4 -// digest of secret followed by challenge. This matches real rsync's -// generate_hash() in authenticate.c exactly: the secret is hashed first, -// then the challenge, with no seed byte - verified against the actual -// rsync source rather than assumed, since getting the byte order wrong -// here would silently produce a client and server that only interoperate -// with each other, never with real rsync or real docs describing the -// algorithm. +// md4Hash returns the base64-encoded MD4 digest of secret followed by +// challenge, matching real rsync's generate_hash(): secret then challenge, +// no seed byte. func md4Hash(secret, challenge string) string { h := md4.New() - // hash.Hash.Write (which io.WriteString goes through) never returns - // an error - its doc comment guarantees this - so there is nothing - // meaningful to check here. _, _ = io.WriteString(h, secret) _, _ = io.WriteString(h, challenge) return base64.RawStdEncoding.EncodeToString(h.Sum(nil)) } // generateChallenge returns a fresh, random, base64-encoded challenge. -// Real rsync derives its challenge from the client address, current time, -// and pid; grsync uses a CSPRNG instead, which is at least as -// unpredictable and far simpler - nothing in the protocol requires the -// challenge to be derived any particular way, only that it not repeat. func generateChallenge() (string, error) { buf := make([]byte, 16) if _, err := rand.Read(buf); err != nil { @@ -79,10 +52,7 @@ func generateChallenge() (string, error) { return base64.RawStdEncoding.EncodeToString(buf), nil } -// readSecretsFile parses a "name:secret" per-line secrets file, matching -// the real "secrets file" format. Blank lines and "#"-prefixed lines are -// skipped; a line with no ":" is skipped rather than treated as an error, -// since it can never match a submitted username anyway. +// readSecretsFile parses a "name:secret" per-line secrets file. func readSecretsFile(path string) (map[string]string, error) { f, err := os.Open(path) if err != nil { @@ -118,16 +88,10 @@ func userAllowed(user string, allowed []string) bool { return false } -// ServeAuth runs the server side of module authentication, following -// ServeGreeting having already selected m. If m doesn't require auth, it -// writes "@RSYNCD: OK" and returns immediately with an empty user. Password -// comparison happens in constant time (crypto/subtle) so a wrong-length or -// wrong-content response can't be distinguished by timing; the refusal -// reason is likewise never revealed on the wire, matching real rsync's own -// single generic "@ERROR: auth failed on module " message regardless -// of whether the username was unknown, unauthorized, or the password was -// wrong - only the server's own logs (not implemented here) would ever see -// that detail in real rsync. +// ServeAuth runs the server side of module authentication for m, which +// ServeGreeting has already selected. Password comparison is constant-time +// and the failure reason is never revealed on the wire, matching real +// rsync's single generic auth-failure message. func ServeAuth(c *conn, m Module) (user string, err error) { if !m.AuthRequired() { if err := writeLine(c.w, "@RSYNCD: OK"); err != nil { @@ -181,15 +145,10 @@ func ServeAuth(c *conn, m Module) (user string, err error) { return submittedUser, nil } -// DialAuth runs the client side of authentication: reads lines until -// "@RSYNCD: OK", answering an "@RSYNCD: AUTHREQD " line (if one -// arrives) with " " - the password -// itself is never sent, only this one-way hash of it, so ErrAuthFailed -// and any wire capture of this exchange should never contain it. An empty -// user is sent as "nobody", matching real rsync's own client behavior for -// anonymous-looking auth attempts. password is only ever called if the -// server actually asks for one - see PasswordFunc's doc comment for why -// that laziness matters, not just how it works. +// DialAuth runs the client side of authentication: it reads lines until +// "@RSYNCD: OK", answering any "@RSYNCD: AUTHREQD " with +// " ". The password itself is never +// sent on the wire. An empty user is sent as "nobody". func DialAuth(c *conn, user string, password PasswordFunc) error { for { line, err := readLine(c.r) diff --git a/internal/daemon/auth_test.go b/internal/daemon/auth_test.go index 1f03a3d..347bcc3 100644 --- a/internal/daemon/auth_test.go +++ b/internal/daemon/auth_test.go @@ -20,8 +20,7 @@ func writeSecretsFile(t *testing.T, content string) string { } // authPipe wires up a client/server conn pair over io.Pipe, each side's -// writes also captured into its own log buffer - so a test can inspect -// every byte either side put on the wire, not just the final result. +// writes also captured into its own log buffer. func authPipe() (client, server *conn, clientWireLog, serverWireLog *bytes.Buffer) { clientR, serverW := io.Pipe() serverR, clientW := io.Pipe() @@ -117,10 +116,8 @@ func TestAuth_UnauthorizedUserFails(t *testing.T) { } } -// TestAuth_NoPlaintextPasswordOnWire is the self-review requirement made -// concrete: it inspects the actual bytes each side wrote, not just the -// outcome, and fails if the raw password ever appears in either -// direction. +// TestAuth_NoPlaintextPasswordOnWire fails if the raw password ever +// appears in either side's wire bytes. func TestAuth_NoPlaintextPasswordOnWire(t *testing.T) { const password = "correct-horse-battery-staple" secretsPath := writeSecretsFile(t, "alice:"+password+"\n") diff --git a/internal/daemon/client.go b/internal/daemon/client.go index f75f986..8702752 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -9,20 +9,10 @@ import ( ) // DialClient runs a full client-side daemon session over an already- -// connected nc: the greeting/module-selection handshake, authentication -// (only actually triggered if the server challenges for it - see -// PasswordFunc), and the resulting transfer against localPath. It is the -// client-side counterpart to ServeConn, and exists for the same reason: -// DialGreeting/DialAuth/DialModule are built around this package's own -// unexported conn type, so external callers (internal/cli, in -// particular) had no way to actually reach them until now - this is -// where net.Conn crosses into that internal representation. -// -// module must be non-empty: DialClient runs a transfer, not a listing - +// connected nc: the greeting/module-selection handshake, authentication, +// and the resulting transfer against localPath. module must be non-empty; // callers that want to list a daemon's modules should use DialGreeting -// directly with an empty module instead. See DialModule's own doc -// comment for exactly what ropts and copts do and don't reach on each -// direction. +// 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, ropts pipeline.ReceiverOptions, copts pipeline.CompressOptions) error { if module == "" { return fmt.Errorf("DialClient requires a module name") diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index 5aedc4d..868617a 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -83,12 +83,9 @@ func TestDialClient_UploadOverRealTCP(t *testing.T) { } } -// TestDialClient_PasswordFuncNotCalledForAnonymousModule is the laziness -// guarantee made concrete: PasswordFunc must never be invoked when the -// server never challenges for a password, exactly matching real rsync's -// own auth_client(), which is only ever called in response to an -// AUTHREQD line. A PasswordFunc backed by an interactive terminal prompt -// or a --password-file read must not fire against an anonymous module. +// TestDialClient_PasswordFuncNotCalledForAnonymousModule confirms +// PasswordFunc is never invoked when the server doesn't challenge for a +// password (an anonymous module). func TestDialClient_PasswordFuncNotCalledForAnonymousModule(t *testing.T) { modRoot := t.TempDir() mustWriteFile(t, filepath.Join(modRoot, "open.txt"), "no auth needed") diff --git a/internal/daemon/config.go b/internal/daemon/config.go index ffbfcc1..0f3652b 100644 --- a/internal/daemon/config.go +++ b/internal/daemon/config.go @@ -1,10 +1,8 @@ // Package daemon implements grsync's rsync-daemon-protocol server: parsing // rsyncd.conf, the rsync:// URL scheme, the @RSYNCD greeting/handshake, // MD4 challenge-response authentication, and per-module access control. -// Once a client has authenticated and selected a module, this package -// hands the connection straight to internal/pipeline's existing -// Sender/Receiver - the daemon protocol is a second way to *establish* a -// connection, not a second way to *transfer files*. +// Once a client has authenticated and selected a module, the connection is +// handed to internal/pipeline's Sender/Receiver for the actual transfer. package daemon import ( @@ -19,31 +17,24 @@ import ( // whatever global defaults it didn't override. type Module struct { Name string - // Path is the directory on the daemon's filesystem this module - // exposes. Required for every module - rsyncd.conf itself requires it. + // Path is the directory on the daemon's filesystem this module exposes. Path string - // ReadOnly defaults to true, matching real rsync: "The default is for - // all modules to be read only." + // ReadOnly defaults to true, matching real rsync. ReadOnly bool - // List defaults to true; "list = false" hides the module from a - // #list request without preventing a client who already knows its - // name from connecting to it - a real, documented rsyncd.conf option, - // not an invented one. + // List defaults to true; false hides the module from a #list request + // without preventing a client who already knows its name from connecting. List bool - // Comment is shown alongside the module name in a #list response - // ("\t", real rsync's own listing format). + // Comment is shown alongside the module name in a #list response. Comment string // Exclude is the raw, space-separated pattern list from the "exclude" - // parameter, not yet compiled into sync.Rule - see access.go. + // parameter, not yet compiled into sync.Rule. Exclude []string // AuthUsers is the raw, comma/space-separated list from "auth users". // A non-empty list means this module requires authentication. AuthUsers []string - // SecretsFile is the path to a "name:password" per-line file, per - // "secrets file". + // SecretsFile is the path to a "name:password" per-line file. SecretsFile string - // MaxConnections is the simultaneous-connection cap for this module; - // 0 (the default) means unlimited, matching real rsync. + // MaxConnections is the simultaneous-connection cap; 0 means unlimited. MaxConnections int } @@ -52,33 +43,14 @@ type Config struct { Modules map[string]Module } -// moduleDefaults returns the built-in defaults every module starts from -// before its own [section] parameters (or the file's global parameters, -// set before any module header) are applied on top. func moduleDefaults() Module { return Module{ReadOnly: true, List: true} } -// ParseConfig parses rsyncd.conf content from r. -// -// Syntax, matching the real format (verified against the actual -// rsyncd.conf(5) man page, not assumed): global parameters may appear -// before any module header and become that module's starting defaults; -// a module begins with "[name]" and continues until the next module or -// EOF; "#"-prefixed lines are comments; blank lines are ignored; a line -// ending in "\" continues on the next line; only the first "=" in a -// "name = value" line is significant, and whitespace around it is -// trimmed. -// -// A parameter name this package doesn't implement (real rsyncd.conf has -// dozens - "uid", "hosts allow", "log file", "timeout", and more) is -// accepted and silently ignored, not an error: rejecting a real, -// syntactically valid config file just because it uses an option this -// package hasn't implemented yet would be worse than ignoring that one -// line. A line that isn't valid "name = value" or "[section]" syntax at -// all, or a recognized parameter with a malformed value (e.g. -// "max connections = abc"), is a hard parse error - that distinction is -// deliberate, not an oversight. +// ParseConfig parses rsyncd.conf content from r. Global parameters before +// any module header become that module's starting defaults. An +// unrecognized parameter name is silently ignored; a malformed line or a +// recognized parameter with an invalid value is a hard error. func ParseConfig(r io.Reader) (*Config, error) { lines, err := readLogicalLines(r) if err != nil { diff --git a/internal/daemon/fuzz_test.go b/internal/daemon/fuzz_test.go index d419568..650f40d 100644 --- a/internal/daemon/fuzz_test.go +++ b/internal/daemon/fuzz_test.go @@ -6,16 +6,7 @@ import ( "testing" ) -// FuzzReadGreeting is SC-15's fuzz target for the daemon protocol's own -// greeting-line parsing (the "@RSYNCD: VERSION.SUB ..." line every -// connection starts with, before any authentication happens - genuinely -// untrusted network input, arriving before the peer has proven anything -// about itself). The property checked is that readGreeting never panics -// for any line content, however malformed - its own prefix check, -// strings.Fields split, and two strconv.Atoi calls all look -// defensively coded already, but fuzzing confirms that holds for inputs -// nobody thought to hand-write, not just the cases -// TestReadGreeting-style unit tests already cover. +// FuzzReadGreeting checks that readGreeting never panics on malformed input. func FuzzReadGreeting(f *testing.F) { f.Add("@RSYNCD: 31.0\n") f.Add("@RSYNCD: 30\n") @@ -28,24 +19,12 @@ func FuzzReadGreeting(f *testing.F) { f.Fuzz(func(_ *testing.T, line string) { r := bufio.NewReader(strings.NewReader(line)) - // readGreeting itself calls readLine, so a line with no trailing - // "\n" is a valid, expected input here too (readLine returns an - // error for it) - not appended manually, so this fuzzes the real - // end-to-end parsing path exactly as a live connection would - // present it. _, _, _ = readGreeting(r) }) } -// FuzzReadLine is SC-15's fuzz target for the single shared line-reading -// primitive every text-based phase of the daemon protocol (greeting, -// module selection, authentication) reads through - the one chokepoint -// genuinely untrusted network bytes always pass. The property checked is -// exactly what readLine's own doc comment promises: it never panics, and -// it never returns a line longer than maxLineLength, regardless of how -// much unterminated data a hostile or corrupted peer sends - the -// protection that keeps an attacker from forcing unbounded memory growth -// just by never sending a newline. +// FuzzReadLine checks that readLine never panics and never returns a line +// longer than maxLineLength. func FuzzReadLine(f *testing.F) { f.Add("hello\n") f.Add("\n") diff --git a/internal/daemon/protocol.go b/internal/daemon/protocol.go index 6b9d275..221f526 100644 --- a/internal/daemon/protocol.go +++ b/internal/daemon/protocol.go @@ -10,27 +10,19 @@ import ( "strings" ) -// ProtocolVersion/SubProtocolVersion are what grsync's daemon claims in -// its @RSYNCD greeting line. This only governs the greeting/handshake -// text exchange this package implements - it is not a claim that -// everything a real rsync client would expect at this protocol version -// (digest negotiation, the real binary wire format for the transfer -// itself) is implemented. See the package doc comment and README for the -// exact boundary: handshake/auth are real-protocol-shaped, the transfer -// that follows is internal/pipeline's own gob protocol. +// ProtocolVersion/SubProtocolVersion are what grsync's daemon claims in its +// @RSYNCD greeting line. This only covers the greeting/handshake text +// exchange this package implements, not real rsync's binary wire protocol +// for the transfer itself (see the package doc comment). const ( ProtocolVersion = 31 SubProtocolVersion = 0 ) // conn bundles a connection's read and write sides into one io.ReadWriter -// that every protocol step - greeting, module selection, auth, and -// finally the handoff to pipeline.Sender/Receiver - shares. This matters -// for correctness, not just convenience: line-based reads (bufio.Reader) -// can buffer bytes past the line they were asked for, and if later code -// switched to reading directly from the underlying net.Conn instead of -// continuing through this same *bufio.Reader, any bytes already -// buffered-but-unread would be silently lost. +// shared by every protocol step. Reads must stay routed through the same +// *bufio.Reader throughout: switching to the raw net.Conn partway would +// silently drop any bytes already buffered but unread. type conn struct { r *bufio.Reader w io.Writer @@ -40,8 +32,6 @@ func newConn(rw io.ReadWriter) *conn { return &conn{r: bufio.NewReader(rw), w: rw} } -// Read/Write let *conn itself satisfy io.ReadWriter, so it can be handed -// directly to pipeline.Sender/Receiver once the handshake is done. func (c *conn) Read(p []byte) (int, error) { return c.r.Read(p) } func (c *conn) Write(p []byte) (int, error) { return c.w.Write(p) } @@ -51,21 +41,14 @@ func writeLine(w io.Writer, s string) error { } // maxLineLength bounds every line read during the connection's text-based -// phases (greeting, module selection, auth). Without a cap, an -// unauthenticated client could force unbounded memory growth just by -// sending bytes with no "\n" - bufio.Reader.ReadString itself keeps -// growing its buffer until the delimiter appears. Real rsync's own line -// reader (read_line_old) is bounded the same way, not just this package's -// own invention. +// phases. Without a cap, an unauthenticated client could force unbounded +// memory growth by sending bytes with no "\n". const maxLineLength = 8192 // readLine reads one line, stripping the trailing "\n" and any "\r" -// immediately before it (tolerating a CRLF-sending peer without requiring -// one, since real rsync's own daemon protocol is LF-only). Reads -// byte-by-byte through r rather than via ReadString so an over-length -// line can be rejected before consuming unbounded memory, while still -// only ever going through the one shared *bufio.Reader every phase of -// this package uses. +// immediately before it. Reads byte-by-byte rather than via +// bufio.Reader.ReadString so an over-length line is rejected before +// consuming unbounded memory. func readLine(r *bufio.Reader) (string, error) { var buf []byte for { @@ -89,9 +72,8 @@ func writeGreeting(w io.Writer) error { } // readGreeting parses a peer's "@RSYNCD: . ..." -// line. Any digest-list tokens after the version (real rsync protocol -// 30+ uses these to negotiate MD4 vs MD5) are accepted but ignored - this -// package always uses classic MD4 (see auth.go), not digest negotiation. +// line. Any digest-list tokens after the version are accepted but ignored - +// this package always uses classic MD4 (see auth.go), not digest negotiation. func readGreeting(r *bufio.Reader) (version, subVersion int, err error) { line, err := readLine(r) if err != nil { @@ -120,10 +102,9 @@ func readGreeting(r *bufio.Reader) (version, subVersion int, err error) { return version, subVersion, nil } -// ErrModuleListRequested is returned by ServeGreeting when the client -// asked to list modules (and the listing has already been written) -// rather than selecting one - the caller should close the connection at -// that point, not proceed to authentication or transfer. +// ErrModuleListRequested is returned by ServeGreeting when the client asked +// to list modules rather than selecting one; the caller should close the +// connection at that point. var ErrModuleListRequested = errors.New("client requested module listing, connection should now close") // ServeGreeting runs the server side of the initial handshake: writes our @@ -158,10 +139,8 @@ func ServeGreeting(c *conn, cfg *Config) (selected Module, err error) { return m, nil } -// writeModuleList writes every listable module (List == true - "list = -// false" modules are deliberately excluded here, not just documented as -// excluded) as "\t", then a terminating "@RSYNCD: EXIT" -// line, matching real rsync's own listing terminator. +// writeModuleList writes every listable module (List == true) as +// "\t", then a terminating "@RSYNCD: EXIT" line. func writeModuleList(w io.Writer, cfg *Config) error { names := make([]string, 0, len(cfg.Modules)) for name, m := range cfg.Modules { @@ -169,7 +148,7 @@ func writeModuleList(w io.Writer, cfg *Config) error { names = append(names, name) } } - sort.Strings(names) // deterministic order; map iteration alone isn't + sort.Strings(names) for _, name := range names { if err := writeLine(w, fmt.Sprintf("%s\t%s", name, cfg.Modules[name].Comment)); err != nil { @@ -179,17 +158,13 @@ func writeModuleList(w io.Writer, cfg *Config) error { return writeLine(w, "@RSYNCD: EXIT") } -// DialGreeting runs the client side of the initial handshake against an -// already-connected transport: reads the daemon's greeting first, then -// sends ours in reply, matching real rsync's actual ordering (the daemon -// speaks first on accept; the client's greeting is a response to it, not -// sent independently). Getting this backwards would deadlock a real +// DialGreeting runs the client side of the initial handshake: reads the +// daemon's greeting first, then sends ours in reply - the daemon speaks +// first on accept, so getting this order backwards deadlocks a real // synchronous transport where both ends' first move is a write with -// nobody yet reading - which is exactly how this ordering bug was caught. -// Then sends module (or "#list" if module is empty, matching a bare -// rsync://host URL). Returns the raw module-list lines when listing was -// requested (module == ""); returns nil lines otherwise, ready for the -// caller to proceed to authentication. +// nobody yet reading. Then sends module (or "#list" if module is empty). +// Returns the raw module-list lines when listing was requested (module == +// ""); returns nil lines otherwise, ready for the caller to authenticate. func DialGreeting(c *conn, module string) (listing []string, err error) { if _, _, err := readGreeting(c.r); err != nil { return nil, err diff --git a/internal/daemon/protocol_test.go b/internal/daemon/protocol_test.go index 9f44ddd..a15e91f 100644 --- a/internal/daemon/protocol_test.go +++ b/internal/daemon/protocol_test.go @@ -18,9 +18,7 @@ func TestReadLine_RejectsOverLongLineInsteadOfUnboundedGrowth(t *testing.T) { } } -// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter - -// the same small helper internal/transport and internal/pipeline each -// define for their own tests. +// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter. type pipeReadWriter struct { io.Reader io.Writer @@ -74,11 +72,9 @@ func TestGreeting_UnknownModuleErrors(t *testing.T) { if _, err := DialGreeting(client, "does-not-exist"); err != nil { t.Fatalf("DialGreeting returned error: %v", err) } - // DialGreeting itself doesn't interpret the server's post-selection - // response (that's the auth phase's job, in a later step) - so the - // test reads it directly here, both to confirm the server actually - // sent an @ERROR line and to drain the pipe so ServeGreeting's write - // doesn't block forever waiting for a reader that will never come. + // DialGreeting doesn't interpret the server's post-selection response, + // so read it directly here to confirm the @ERROR line and drain the + // pipe so ServeGreeting's write doesn't block. response, err := readLine(client.r) if err != nil { t.Fatalf("reading server response: %v", err) @@ -127,10 +123,8 @@ func TestGreeting_ModuleListingHidesListFalseModules(t *testing.T) { } func TestGreeting_HiddenModuleStillReachableByName(t *testing.T) { - // "list = false" hides a module from listing, but real rsyncd.conf - // does not make it unreachable to a client who already knows its - // name - confirming that distinction is implemented correctly, not - // just "list = false" accidentally behaving like a full block. + // "list = false" hides a module from listing but doesn't block access + // to a client who already knows its name. clientR, serverW := io.Pipe() serverR, clientW := io.Pipe() client := newConn(pipeReadWriter{Reader: clientR, Writer: clientW}) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 1932c4a..2441d02 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -8,12 +8,9 @@ import ( ) // ServeConn runs one full connection's daemon protocol end to end: the -// greeting/module-selection handshake, authentication (only performed at -// all if the selected module requires it), and the resulting transfer - -// using only this package's own per-phase functions plus the connection -// itself. A bare module-listing request (ErrModuleListRequested) is a -// normal outcome, not a failure; the connection is simply done at that -// point. +// greeting/module-selection handshake, authentication (if the selected +// module requires it), and the resulting transfer. A bare module-listing +// request (ErrModuleListRequested) is a normal outcome, not a failure. func ServeConn(nc net.Conn, cfg *Config) error { c := newConn(nc) @@ -29,12 +26,10 @@ func ServeConn(nc net.Conn, cfg *Config) error { return ServeModule(c, m) } -// Serve accepts connections on ln until Accept itself fails (typically -// because ln was closed), running ServeConn for each one in its own -// goroutine so one slow or misbehaving client can't block any other. -// Per-connection errors are written to errLog rather than returned - -// Serve's own return only ever reflects the listener itself failing, not -// any individual client's session. +// Serve accepts connections on ln until Accept fails (typically because ln +// was closed), running ServeConn for each one in its own goroutine so one +// slow or misbehaving client can't block any other. Per-connection errors +// go to errLog; Serve's own return only reflects the listener failing. func Serve(ln net.Listener, cfg *Config, errLog io.Writer) error { for { nc, err := ln.Accept() @@ -44,14 +39,9 @@ func Serve(ln net.Listener, cfg *Config, errLog io.Writer) error { go func() { defer func() { _ = nc.Close() }() - // A network-facing daemon must not let one connection's bad - // input (malformed data anywhere downstream in the - // handshake/auth/transfer chain, not just malformed - // rsyncd.conf) take the whole process - and therefore every - // other in-flight connection - down with it. Go doesn't - // isolate goroutine panics on its own, so this recover is - // what makes "one client, one goroutine" actually mean one - // client's failure stays scoped to that client. + // Go doesn't isolate goroutine panics on its own; without this + // recover, one client's bad input could take down every other + // in-flight connection along with it. defer func() { if r := recover(); r != nil { _, _ = fmt.Fprintf(errLog, "%s: panic: %v\n", nc.RemoteAddr(), r) diff --git a/internal/daemon/server_test.go b/internal/daemon/server_test.go index 4a2fe3b..8ae4ad4 100644 --- a/internal/daemon/server_test.go +++ b/internal/daemon/server_test.go @@ -13,11 +13,7 @@ import ( ) // startTestDaemon listens on 127.0.0.1:0 (an OS-assigned free port) and -// serves cfg in the background until the test ends. Using a real TCP -// listener rather than io.Pipe is deliberate here: unlike the SSH -// transport tests, this needs no external binary or environment -// dependency (no sshd), so there's no reason to settle for anything less -// than a real end-to-end loopback round trip. +// serves cfg in the background until the test ends. func startTestDaemon(t *testing.T, cfg *Config) (addr string, errLog *bytes.Buffer) { t.Helper() @@ -165,12 +161,8 @@ func TestDaemon_RealTCP_ModuleListing(t *testing.T) { } } -// 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. +// TestDaemon_RealTCP_DryRunPutMakesNoChanges confirms a DirectionPut with +// DryRun set leaves the module directory completely empty. func TestDaemon_RealTCP_DryRunPutMakesNoChanges(t *testing.T) { modRoot := t.TempDir() cfg := &Config{Modules: map[string]Module{ @@ -210,11 +202,7 @@ func TestDaemon_RealTCP_DryRunPutMakesNoChanges(t *testing.T) { } // 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. +// DirectionGet, where the client's own Receiver runs locally. func TestDaemon_RealTCP_DryRunGetMakesNoChanges(t *testing.T) { modRoot := t.TempDir() mustWriteFile(t, filepath.Join(modRoot, "readme.txt"), "should never be downloaded") @@ -251,10 +239,7 @@ func TestDaemon_RealTCP_DryRunGetMakesNoChanges(t *testing.T) { } // TestDaemon_RealTCP_StatsWorkForGet confirms Stats works fully for a -// module download, over a real TCP connection: DirectionGet's Receiver -// runs on the client, exactly like a local sync, so ReceiverOptions -// reaches it directly with no protocol involvement at all - unlike -// DirectionPut, there is no daemon-specific limitation here to disclose. +// module download, since DirectionGet's Receiver runs on the client. func TestDaemon_RealTCP_StatsWorkForGet(t *testing.T) { modRoot := t.TempDir() mustWriteFile(t, filepath.Join(modRoot, "readme.txt"), "some real content to report stats about") @@ -287,16 +272,10 @@ func TestDaemon_RealTCP_StatsWorkForGet(t *testing.T) { } } -// TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks confirms -// the disclosed daemon-PUT limitation fails safely rather than breaking -// anything: even if a caller sets Progress/Stats on the *client's* -// ReceiverOptions for a DirectionPut, the transfer still completes -// correctly, because those fields never cross the wire at all - only -// DryRun does, via dryRunToken (see ServeModule/DialModule's own doc -// comments) - so the server-side Receiver that would actually need them -// never even sees them. This is what "consistent, not a new gap" means -// concretely: the same fields SC-11 already established as inert for -// daemon-PUT stay inert for Progress/Stats too, without erroring. +// TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks confirms that +// setting Progress/Stats on a DirectionPut's client-side ReceiverOptions +// is silently inert (only DryRun crosses the wire, via dryRunToken) +// rather than causing an error. func TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks(t *testing.T) { modRoot := t.TempDir() cfg := &Config{Modules: map[string]Module{ @@ -319,8 +298,6 @@ func TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks(t *testing.T) { t.Fatalf("compiling empty rule set: %v", err) } - // Progress/Stats set here deliberately, to prove they're harmlessly - // ignored for this direction rather than causing an error. var clientSideOutput bytes.Buffer ropts := pipeline.ReceiverOptions{Progress: true, Stats: true, Output: &clientSideOutput} if err := DialModule(client, DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}, ropts, pipeline.CompressOptions{}); err != nil { @@ -334,11 +311,6 @@ func TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks(t *testing.T) { if string(got) != "pushed despite requesting progress/stats" { t.Errorf("uploaded content = %q, want %q", got, "pushed despite requesting progress/stats") } - // Nothing was ever printed on the client side either: DialModule's - // DirectionPut branch runs pipeline.Sender, which never consults - // ReceiverOptions at all (see pipeline.Sender's own doc comment) - - // there is no client-side reporting output for an upload regardless - // of transport. if clientSideOutput.Len() != 0 { t.Errorf("client-side output = %q, want empty (Sender never reports progress/stats)", clientSideOutput.String()) } @@ -347,14 +319,9 @@ func TestDaemon_RealTCP_PutIgnoresProgressAndStatsButStillWorks(t *testing.T) { } } -// TestDaemon_RealTCP_PutWithCompressUploadsCorrectly is SC-9's real, -// over-the-wire proof for the daemon transport: DirectionPut runs -// pipeline.Sender on the client side (see DialModule's own doc comment), -// exactly where --compress/-z's decision belongs, so this drives that -// same client-side Sender with CompressOptions.Enabled against a real -// TCP daemon connection and confirms the upload still arrives byte- -// correct - the server's Receiver only ever reacts to each deltaMessage's -// own Compressed marker, needing no daemon-protocol change at all. +// TestDaemon_RealTCP_PutWithCompressUploadsCorrectly confirms a +// DirectionPut with CompressOptions.Enabled arrives byte-correct over a +// real TCP daemon connection. func TestDaemon_RealTCP_PutWithCompressUploadsCorrectly(t *testing.T) { modRoot := t.TempDir() cfg := &Config{Modules: map[string]Module{ diff --git a/internal/daemon/session.go b/internal/daemon/session.go index 02620f8..c0335da 100644 --- a/internal/daemon/session.go +++ b/internal/daemon/session.go @@ -10,19 +10,14 @@ import ( ) // Direction is which way file data moves in a module session, sent by the -// client as a single line immediately after authentication succeeds (or -// immediately after ServeGreeting, for a module that needs none). +// client as a single line immediately after authentication succeeds. type Direction string const ( - // DirectionGet means the client downloads from the module - the - // daemon runs pipeline.Sender against the module's Path, the client - // runs pipeline.Receiver against its local destination. + // DirectionGet means the client downloads from the module. DirectionGet Direction = "get" - // DirectionPut means the client uploads to the module - the daemon - // runs pipeline.Receiver against the module's Path, the client runs - // pipeline.Sender against its local source. Refused outright for a - // read-only module, before any transfer code runs. + // DirectionPut means the client uploads to the module. Refused outright + // for a read-only module, before any transfer code runs. DirectionPut Direction = "put" ) @@ -32,12 +27,10 @@ var ErrReadOnly = errors.New("module is read only") // transferDone is sent by whichever side ran pipeline.Receiver once it // returns, and waited for by whichever side ran pipeline.Sender before -// that side's own call returns. This matters for real callers, not just -// tests: pipeline.Sender returning only means the last delta has been -// written to the connection, not that the receiver has finished applying -// it and closing its destination files - closing the TCP connection right -// after Sender returns (as --daemon's per-connection goroutine does) can -// otherwise race the receiver's still-in-flight disk writes. +// returning itself. Without it, closing the TCP connection right after +// Sender returns could race the receiver's still-in-flight disk writes, +// since Sender returning only means the last delta has been written, not +// that the receiver has finished applying it. const transferDone = "@GRSYNC: DONE" func waitForTransferDone(c *conn) error { @@ -51,18 +44,12 @@ func waitForTransferDone(c *conn) error { return nil } -// moduleRules compiles m.Exclude into sync.Rule via the same -// CompileRules/Included machinery internal/cli uses for --exclude, -// rather than a second, independently-trusted pattern matcher for module -// access control. -// -// This is applied on the DirectionGet (download) path only: that's where -// the daemon itself walks m.Path and can filter what it sends, matching -// exclude's most common real-world use (restricting what a mirror -// serves). pipeline.Receiver has no per-entry filtering hook, so a -// DirectionPut upload to a non-read-only module is not filtered against -// this list - a deliberate, documented scope boundary, not an oversight; -// see the README's daemon section. +// moduleRules compiles m.Exclude into sync.Rule via the same CompileRules +// machinery internal/cli uses for --exclude. Only applied on the +// DirectionGet (download) path, where the daemon walks m.Path itself; +// pipeline.Receiver has no per-entry filtering hook, so a DirectionPut +// upload is not filtered against this list (see the README's daemon +// section). func moduleRules(m Module) ([]sync.Rule, error) { raw := make([]sync.RawRule, len(m.Exclude)) for i, pattern := range m.Exclude { @@ -71,37 +58,25 @@ func moduleRules(m Module) ([]sync.Rule, error) { return sync.CompileRules(raw) } -// moduleAttrOptions is what a daemon module preserves - on an upload -// (applied by the receiving Receiver) and, via its HardLinks field, on a -// download too (consulted by the sending Sender). Fixed, rather than -// client-controlled, since the module (not the client) owns what its own -// rsyncd.conf-configured directory considers worth preserving - a client -// can't currently ask a module to preserve more or less than this. +// moduleAttrOptions is what a daemon module preserves: fixed rather than +// client-controlled, since the module owns what its rsyncd.conf-configured +// directory considers worth preserving. 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). +// dryRunToken is appended as a second field on the direction line ("put +// --dry-run") - the wire signal a DirectionPut needs since its Receiver +// runs on the server side, with no other way for the client to say "plan +// this, don't write it." 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 -// "@ERROR" line, and only then hands the connection to pipeline.Sender or -// pipeline.Receiver. That ack is required, not cosmetic: without it, a -// refused DirectionPut would leave the client's pipeline.Sender blocked -// writing a file list nobody is left to read - the same deadlock shape -// the greeting phase's unknown-module case has, avoided here the same -// way, by never letting either side commit to the transfer until the -// other has confirmed it's ready. +// already-selected module m: reads the requested Direction, enforces +// read-only, acknowledges with "@RSYNCD: OK" or refuses with "@ERROR", and +// only then hands the connection to pipeline.Sender or Receiver. The ack +// is required: without it, a refused DirectionPut would leave the +// client's Sender blocked writing a file list nobody is left to read. func ServeModule(c *conn, m Module) error { line, err := readLine(c.r) if err != nil { @@ -134,27 +109,18 @@ func ServeModule(c *conn, m Module) error { if err != nil { return fmt.Errorf("compiling module %q exclude rules: %w", m.Name, err) } - // pipeline.CompressOptions{} (disabled): a module download has no - // CLI wiring at all yet (see runSync's own "pulling... is not yet - // supported" restriction), so there is no client-facing --compress - // flag that could reach this server-side Sender call in the first - // place - consistent with that existing scope boundary, not a new - // gap. See the README's Compression section. + // pipeline.CompressOptions{} (disabled): module downloads have no + // CLI-facing --compress wiring yet (see the README's Compression + // section). if err := pipeline.Sender(c, m.Path, sync.WalkOptions{Recursive: true}, rules, moduleAttrOptions().HardLinks, pipeline.CompressOptions{}); err != nil { return err } return waitForTransferDone(c) case DirectionPut: - // No Itemize/Verbose/Progress/Stats 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 Progress and Stats section for the full explanation of - // this disclosed gap, which SC-11 already established for - // itemize/verbose and SC-10 confirms applies identically to - // progress/stats, not a new limitation. 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. + // No Itemize/Verbose/Progress/Stats: the daemon protocol has no + // channel back to the client for reporting text once the handshake + // ends (see the README's Progress and Stats section). DryRun needs + // no channel, since it's a local decision driven by dryRunToken. ropts := pipeline.ReceiverOptions{DryRun: dryRun} if err := pipeline.Receiver(c, m.Path, moduleAttrOptions(), ropts); err != nil { return err @@ -165,35 +131,12 @@ func ServeModule(c *conn, m Module) error { } } -// DialModule runs the client side of a module session: sends the -// requested direction, waits for the server's ack (see ServeModule) and -// fails without touching the pipeline at all if it's an @ERROR instead, -// then runs the matching pipeline side against localPath. rules and -// walkOpts govern what the client's own Sender walk includes on a -// DirectionPut; attrOpts governs what the client's own Receiver preserves -// on a DirectionGet, and its HardLinks field also governs whether a -// 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. -// -// 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. -// -// copts is the DirectionPut mirror image of that same asymmetry: it only -// matters for that direction's client-side Sender call (--compress/-z is -// entirely a sending-side decision, see pipeline.CompressOptions' own -// doc comment) and is simply unused for DirectionGet, where the -// server-side Sender that would consult it has no CLI wiring at all yet -// (see ServeModule's own comment on that same boundary). +// DialModule runs the client side of a module session: sends the requested +// direction, waits for the server's "@RSYNCD: OK" ack, then runs the +// matching pipeline side against localPath. For DirectionPut, ropts.DryRun +// is sent via dryRunToken so the server's Receiver (which decides whether +// to write) knows to skip writes; Itemize/Verbose/copts don't apply in +// that direction, for the same reasons as ServeModule. func DialModule(c *conn, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions, copts pipeline.CompressOptions) error { directionLine := string(direction) if direction == DirectionPut && ropts.DryRun { diff --git a/internal/daemon/url.go b/internal/daemon/url.go index dc152a6..7b8ea12 100644 --- a/internal/daemon/url.go +++ b/internal/daemon/url.go @@ -11,22 +11,16 @@ import ( const DefaultPort = 873 // URL is a parsed rsync://[user@]host[:port]/module[/path] endpoint. -// This is a distinct addressing scheme from internal/transport's -// [user@]host:path SSH syntax - the two coexist in the same CLI, and -// grsync's own scheme prefix ("rsync://") is what tells them apart, the -// same way it tells real rsync's two connection methods apart. type URL struct { User string // empty if no "user@" was present Host string // Port is 0 if the URL didn't specify one; callers should treat that - // as DefaultPort, not as "port zero" (an invalid TCP port anyway). + // as DefaultPort. Port int // Module is empty for a bare "rsync://host" or "rsync://host/" URL, - // which is not an error: that's real rsync's own syntax for "list - // this daemon's available modules" rather than selecting one. + // which means "list this daemon's available modules" rather than an error. Module string - // Path is whatever followed the module name, empty if nothing did. - Path string + Path string } // ParseURL parses s as an rsync:// URL. @@ -49,9 +43,6 @@ func ParseURL(s string) (URL, error) { if portStr := u.Port(); portStr != "" { port, err := strconv.Atoi(portStr) if err != nil { - // u.Port() already validates this is numeric per net/url's - // own rules, so this should be unreachable - checked anyway - // rather than trusting that invariant silently. return URL{}, fmt.Errorf("%q has an invalid port %q", s, portStr) } result.Port = port diff --git a/internal/pipeline/append_test.go b/internal/pipeline/append_test.go index 54fec4c..de1be2d 100644 --- a/internal/pipeline/append_test.go +++ b/internal/pipeline/append_test.go @@ -55,23 +55,12 @@ func TestAppendTailOps_ExactMatchOmitsDataOp(t *testing.T) { opsEqual(t, ops, []sync.DeltaOp{sync.CopyOp{BlockIndex: 0}}) } -// TestAppendTailOps_DiminishedFileReturnsError is the "diminished file" -// race real rsync itself warns about (source shrank below what the -// receiver already trusted) - grsync treats it as a hard error rather -// than real rsync's own skip-with-warning, a disclosed, deliberate -// simplification (see appendTailOps' own doc comment). func TestAppendTailOps_DiminishedFileReturnsError(t *testing.T) { if _, err := appendTailOps(100, []byte("short")); err == nil { t.Error("appendTailOps with a source shorter than the trusted length returned nil error, want an error") } } -// TestReceiver_AppendTransfersOnlyNewTail is --append's core wire-level -// proof: a destination file that's a genuine prefix of the source must -// end up correct, while sending meaningfully fewer bytes than a full -// re-transfer would need - proving the existing prefix was never -// resent as literal data, not just that the final content happens to be -// right. func TestReceiver_AppendTransfersOnlyNewTail(t *testing.T) { prefix := strings.Repeat("already on disk, must not be resent ", 100) tail := strings.Repeat("brand new tail data ", 100) @@ -86,14 +75,9 @@ func TestReceiver_AppendTransfersOnlyNewTail(t *testing.T) { assertSameContent(t, filepath.Join(appendSrc, "growing.log"), filepath.Join(appendDest, "growing.log")) - // Baseline: the exact same transfer with no append mode at all, which - // still needs to run the full weak/strong rolling-checksum block scan - // (real, genuine work, not literal retransmission) - --append is - // specifically about skipping the SIGNATURE exchange for the prefix - // entirely (see sender.go's own appendTailOps), not just about the - // resulting delta happening to be efficient, so this baseline mainly - // confirms append mode isn't somehow *more* expensive; the literal-data - // byte count comparison below is append's real, direct proof. + // Baseline: the same transfer without append mode, mainly to confirm + // append isn't somehow more expensive; the literal-data byte count + // comparison below is append's real, direct proof. normalSrc, normalDest := t.TempDir(), t.TempDir() mustWriteFile(t, filepath.Join(normalSrc, "growing.log"), full) mustWriteFile(t, filepath.Join(normalDest, "growing.log"), prefix) @@ -104,9 +88,8 @@ func TestReceiver_AppendTransfersOnlyNewTail(t *testing.T) { t.Errorf("--append wrote %d bytes, normal delta-transfer wrote %d bytes, want --append no larger", appendBytes, normalBytes) } - // The direct proof: --stats' own "Literal data" field for the append - // run must equal exactly len(tail) - if the prefix had been resent as - // literal data too, this would be much larger. + // "Literal data" must equal exactly len(tail): if the prefix had been + // resent as literal data too, this would be much larger. var out bytes.Buffer statsSrc, statsDest := t.TempDir(), t.TempDir() mustWriteFile(t, filepath.Join(statsSrc, "growing.log"), full) @@ -119,13 +102,6 @@ func TestReceiver_AppendTransfersOnlyNewTail(t *testing.T) { } } -// TestReceiver_AppendDoesNotVerifyCorruptedPrefix is the self-review's -// own explicit ask made concrete: --append's documented real-rsync risk -// ("can be dangerous if you aren't 100% sure... existing content...is -// also known to be the same") is reproduced faithfully here, not -// silently made safer or worse - a WRONG existing prefix is blindly -// trusted and the final file ends up with that wrong prefix intact, not -// silently corrected and not aborted with an error either. func TestReceiver_AppendDoesNotVerifyCorruptedPrefix(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() realContent := "AAAAAAAAAA" + "tail data that gets appended" @@ -147,11 +123,6 @@ func TestReceiver_AppendDoesNotVerifyCorruptedPrefix(t *testing.T) { } } -// TestReceiver_AppendVerifyDetectsCorruptedPrefix is -// TestReceiver_AppendDoesNotVerifyCorruptedPrefix's --append-verify -// counterpart: the exact same corrupted-prefix scenario must instead -// produce a fully correct result, proving verification actually caught -// and fixed the mismatch rather than blindly trusting it. func TestReceiver_AppendVerifyDetectsCorruptedPrefix(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() realContent := strings.Repeat("A", 1400) + "tail data that gets appended, well past one block" @@ -166,12 +137,6 @@ func TestReceiver_AppendVerifyDetectsCorruptedPrefix(t *testing.T) { assertSameContent(t, filepath.Join(srcRoot, "file.txt"), filepath.Join(destRoot, "file.txt")) } -// TestReceiver_AppendSkipsDestinationNotShorterThanSource is real -// rsync's own documented eligibility rule made concrete: a destination -// that's already at least as long as the source must be left completely -// untouched, even though its content genuinely differs from the -// source's - --append/--append-verify unconditionally skip such files -// rather than comparing and possibly updating them. func TestReceiver_AppendSkipsDestinationNotShorterThanSource(t *testing.T) { for _, ropts := range []ReceiverOptions{{Append: true}, {AppendVerify: true}} { srcRoot, destRoot := t.TempDir(), t.TempDir() @@ -191,12 +156,6 @@ func TestReceiver_AppendSkipsDestinationNotShorterThanSource(t *testing.T) { } } -// TestReceiver_AppendTransfersBrandNewFileNormally confirms real -// rsync's own documented "new files are transferred" rule: append -// semantics only ever apply to an EXISTING, shorter destination file - -// a file that doesn't exist yet at the destination is transferred -// completely normally under --append/--append-verify, not skipped and -// not specially handled. func TestReceiver_AppendTransfersBrandNewFileNormally(t *testing.T) { for _, ropts := range []ReceiverOptions{{Append: true}, {AppendVerify: true}} { srcRoot, destRoot := t.TempDir(), t.TempDir() @@ -209,9 +168,6 @@ func TestReceiver_AppendTransfersBrandNewFileNormally(t *testing.T) { } } -// TestReceiver_AppendWorksWithDryRun confirms Step 4's dry-run -// requirement: the append-aware signature/delta exchange still runs (for -// accurate itemize planning), but no file is created or modified. func TestReceiver_AppendWorksWithDryRun(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() mustWriteFile(t, filepath.Join(srcRoot, "growing.log"), "prefixTAIL") @@ -244,13 +200,6 @@ func TestReceiver_AppendWorksWithDryRun(t *testing.T) { } } -// TestReceiver_AppendExcludesHardLinkSecondaryMembers locks in Step 4's -// hard-link requirement: a hard-link group's secondary member never goes -// through the signature/delta exchange at all (it's linked directly from -// the group's first member instead - see Receiver's own hard-link pass), -// so --append is structurally moot for it, not specially excluded by any -// append-specific code. This test's real job is proving that structural -// fact still holds with --append enabled, not just documenting it. func TestReceiver_AppendExcludesHardLinkSecondaryMembers(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() diff --git a/internal/pipeline/compress.go b/internal/pipeline/compress.go index 00537ec..07911ef 100644 --- a/internal/pipeline/compress.go +++ b/internal/pipeline/compress.go @@ -9,19 +9,12 @@ import ( "strings" ) -// DefaultCompressLevel is real rsync's own zlib default, verified against -// upstream's token.c (init_compression_level's def_level for the zlib/ -// zlibx choice) rather than assumed. +// DefaultCompressLevel is real rsync's own zlib default compression level. const DefaultCompressLevel = 6 -// ClampCompressLevel mirrors real rsync's own --compress-level handling -// for zlib compression (token.c's init_compression_level), verified -// against upstream source and rsync.1's own documented wording rather -// than guessed: 0 is a distinct "off" sentinel, not clamped up into -// range; -1 (zlib.DefaultCompression) explicitly means "use the default -// level" (6); anything else out of [1, 9] is silently limited into range -// ("If you specify a too-large or too-small value, the number is -// silently limited to a valid value" - rsync.1's own wording). +// ClampCompressLevel mirrors real rsync's own --compress-level handling: +// 0 is a distinct "off" sentinel, -1 means "use the default level", and +// anything else out of [1, 9] is silently clamped into range. func ClampCompressLevel(level int) int { switch { case level == zlib.NoCompression: // 0: explicit "off" @@ -38,34 +31,21 @@ func ClampCompressLevel(level int) int { } // CompressOptions governs whether/how Sender compresses each regular -// file's literal delta data (--compress/-z) before sending it - see the -// README's Compression section. It is consulted only by Sender: Receiver -// needs no compression options of its own at all, since each -// deltaMessage's own Compressed marker (messages.go) already says -// whether its literal data needs decompressing first - a purely -// data-driven decision on that side, not a policy one. +// file's literal delta data (--compress/-z) before sending it. type CompressOptions struct { Enabled bool - // Level is a zlib compression level from 1 (fastest) to 9 (smallest), - // meaningful only when Enabled - see ClampCompressLevel, which every - // caller that constructs a CompressOptions with Enabled: true is - // expected to have already run Level through. + // Level is a zlib compression level from 1 (fastest) to 9 (smallest); + // callers should run it through ClampCompressLevel first. Level int // SkipSuffixes is a lowercase, dot-free list of file suffixes (e.g. - // "gz", "jpg") to send uncompressed even when Enabled - real rsync's - // own --skip-compress default list (DefaultSkipCompressSuffixes) or a - // caller override (see ParseSkipCompressList). Nil/empty means "skip - // nothing," matching real rsync's own documented meaning of an empty - // --skip-compress=LIST. + // "gz", "jpg") to send uncompressed even when Enabled. Nil/empty means + // "skip nothing". SkipSuffixes []string } // DefaultSkipCompressSuffixes is real rsync's own built-in --skip-compress -// suffix list, copied verbatim from rsync.1.md's own documented default -// (the same list default-dont-compress.h is generated from) rather than -// invented - files with one of these suffixes are already compressed -// formats where running zlib over them again wastes CPU for no size -// benefit. +// suffix list: file types that are already compressed, so recompressing +// them wastes CPU for no size benefit. var DefaultSkipCompressSuffixes = []string{ "3g2", "3gp", "7z", "aac", "ace", "apk", "avi", "bz2", "deb", "dmg", "ear", "f4v", "flac", "flv", "gpg", "gz", "iso", "jar", "jpeg", "jpg", @@ -81,16 +61,7 @@ var DefaultSkipCompressSuffixes = []string{ // ParseSkipCompressList parses a real rsync --skip-compress=LIST value: // suffixes without their leading dot, separated by "/". An empty string -// is a meaningful value in its own right ("skip nothing"), not "unset" - -// see effectiveCompressOptions (internal/cli) for how that distinction -// from "the flag was never given at all" is made. -// -// Real rsync's own LIST grammar also supports bracketed character -// classes inside a suffix (e.g. "mp[34]" for "mp3"/"mp4"); grsync's -// --skip-compress does not, a deliberate, disclosed scope reduction (see -// the README's Compression section) rather than a silent gap - plain -// slash-separated suffixes cover the default list and the overwhelming -// majority of real-world uses. +// is a meaningful value in its own right ("skip nothing"), not "unset". func ParseSkipCompressList(list string) []string { if list == "" { return nil @@ -125,29 +96,12 @@ func skipCompressSuffix(path string, skipSuffixes []string) bool { // compressLiteral zlib-compresses data at level, returning ok == false if // compression didn't actually help (the result is not smaller than data // itself) so the caller can fall back to sending it raw. -// -// This check matters more here than it would for real rsync's own zlib -// usage: real rsync keeps one persistent deflate stream open per file, -// so its fixed ~8-byte zlib header/trailer cost is paid once per file no -// matter how many separate literal runs cross the wire. grsync's -// deltaMessage is sent as a single, independent frame per file with no -// persistent compression context to reuse across files - toWireDeltaOps -// already amortizes that overhead across everything within one file by -// compressing the whole concatenated literal stream as a single unit -// rather than op-by-op (see deltaMessage's own doc comment), but a file -// whose total literal data is tiny (a few changed bytes in an otherwise- -// unchanged large file - exactly the case delta transfer exists for) or -// already-incompressible can still legitimately come out larger -// compressed than raw. Falling back per file, only when it doesn't pay -// off, is a small, real improvement over always compressing regardless. func compressLiteral(data []byte, level int) (compressed []byte, ok bool) { var buf bytes.Buffer w, err := zlib.NewWriterLevel(&buf, level) if err != nil { - // ClampCompressLevel guarantees level is 1-9, which zlib always - // accepts - this should be unreachable, but treating any error as - // "just send raw" is safe (a pure optimization, never required - // for correctness) rather than propagating a hard failure for it. + // level is always 1-9 via ClampCompressLevel, so this should be + // unreachable; falling back to raw is safe either way. return nil, false } if _, err := w.Write(data); err != nil { diff --git a/internal/pipeline/compress_test.go b/internal/pipeline/compress_test.go index b86ee6e..9a2c3bb 100644 --- a/internal/pipeline/compress_test.go +++ b/internal/pipeline/compress_test.go @@ -94,11 +94,6 @@ func TestCompressDecompressLiteral_RoundTrip(t *testing.T) { } } -// TestCompressLiteral_FallsBackWhenNotSmaller is the self-review's tiny- -// payload concern made concrete: a handful of bytes can never come out -// smaller after zlib's own ~8-byte header/trailer overhead, so -// compressLiteral must report ok == false rather than silently returning -// something larger than the input. func TestCompressLiteral_FallsBackWhenNotSmaller(t *testing.T) { tiny := []byte{1, 2, 3} if _, ok := compressLiteral(tiny, DefaultCompressLevel); ok { diff --git a/internal/pipeline/itemize.go b/internal/pipeline/itemize.go index 1049519..6372269 100644 --- a/internal/pipeline/itemize.go +++ b/internal/pipeline/itemize.go @@ -10,82 +10,49 @@ import ( ) // 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. +// kept separate from sync.AttrOptions, which controls what gets +// preserved, not whether writes happen or what gets reported. 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, when true, runs every planning step but skips filesystem writes. 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, when true, writes one real-rsync-format %i line per changed + // entry to Output. Takes precedence over Verbose when both are set. 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. + // (plus " -> target" for a changed symlink) per changed entry to Output. Verbose bool // Progress, when true, writes a live-updating line per regular file - // as its data is written to disk (see progress.go's own doc comment - // for exactly what this measures and why, given grsync's non- - // streaming wire protocol), to Output. + // as its data is written to disk. Progress bool - // Stats, when true, writes a real-rsync-format summary block (see - // stats.go's formatStats) to Output once the whole sync completes. + // Stats, when true, writes a summary block to Output once the sync completes. Stats bool // Output is where Itemize/Verbose/Progress/Stats output is written. - // A nil Output is treated as io.Discard, so a caller that wants none - // of this doesn't need to construct a discard writer itself. + // A nil Output is treated as io.Discard. Output io.Writer - // Partial, when true, keeps a regular file's temp file (rather than - // deleting it) if the transfer aborts before that file's rename into - // place - see partial.go's own doc comment for exactly what "partial" - // means in grsync's frame-per-file architecture (file granularity, - // not true mid-file resumption). Implied by a non-empty PartialDir, - // matching real rsync's own documented "--partial-dir... also - // implying that [--partial] be enabled." + // Partial, when true, keeps a regular file's temp file instead of + // deleting it if the transfer aborts before rename into place. + // Implied by a non-empty PartialDir. Partial bool // PartialDir, when non-empty, is where an abandoned temp file goes - // instead of being renamed onto the destination path directly - see - // partial.go's partialFilePath for the relative-vs-absolute placement - // rule. A file found here is also used as a resume basis (the - // signature comparison source) on a later run, then deleted once its - // transfer completes successfully. + // instead of the destination path directly. A file found here is also + // used as a resume basis on a later run, then deleted on success. PartialDir string // Append, when true, blindly trusts a shorter destination file's - // existing bytes and transfers only the new tail - see - // messages.go's appendTail and receiver.go's own doc comment for the - // real, verified-against-source distinction from AppendVerify. - // Mutually exclusive with AppendVerify (internal/cli validates this). + // existing bytes and transfers only the new tail. Mutually exclusive + // with AppendVerify (internal/cli validates this). Append bool - // AppendVerify is like Append, but runs the completely normal - // signature/delta comparison over the existing prefix instead of - // trusting it blindly - see receiver.go's own doc comment for why - // this needs no new algorithm at all, just an eligibility gate on - // top of the pre-existing flow. + // AppendVerify is like Append, but verifies the existing prefix via a + // normal signature/delta comparison instead of trusting it blindly. AppendVerify bool } -// AppendMode reports whether either append flag is set - Append and -// AppendVerify share the same file-eligibility rules (see receiver.go), -// differing only in whether the existing prefix is trusted or verified. -// Exported since internal/cli needs it too, to warn when combined with -// an rsync:// daemon upload destination (see runSync's own comment on -// why that direction can't honor either flag, the same disclosed -// daemon-PUT limitation SC-10/SC-11 already established for reporting). +// AppendMode reports whether either append flag is set. func (o ReceiverOptions) AppendMode() bool { return o.Append || o.AppendVerify } -// KeepPartial reports whether an aborted temp file should be kept at -// all - PartialDir implies Partial, matching real rsync's own -// documented "--partial-dir... also implying that [--partial] be -// enabled." Exported for the same reason AppendMode is. +// KeepPartial reports whether an aborted temp file should be kept at all. func (o ReceiverOptions) KeepPartial() bool { return o.Partial || o.PartialDir != "" } @@ -97,24 +64,16 @@ func (o ReceiverOptions) output() io.Writer { return o.Output } -// Reporting reports whether o requests any output at all - Itemize, -// Verbose, Progress, or Stats - exported since callers outside this -// package (internal/cli, deciding whether to print its own one-time -// daemon-PUT no-reporting-channel note) need it too, not just Receiver -// itself. All four share the same disclosed daemon-PUT limitation (see -// the README's Progress and Stats section), so one check covers all of -// them consistently rather than needing a separate one per flag. +// Reporting reports whether o requests any output at all: Itemize, +// Verbose, Progress, or Stats. func (o ReceiverOptions) Reporting() bool { return o.Itemize || o.Verbose || o.Progress || o.Stats } // 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. +// %i format ("cstpoguax") in order: checksum/value, size, time, perms, +// owner, group, atime/crtime, ACL, xattr. The last three are never set +// to anything but '.' here - grsync has no --atimes/--acl/--xattr flags. type itemizeAttrs [9]byte func newItemizeAttrs() itemizeAttrs { @@ -123,9 +82,7 @@ func newItemizeAttrs() 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. +// changed reports whether any position differs from the "unchanged" default. func (a itemizeAttrs) changed() bool { for _, b := range a { if b != '.' { @@ -135,21 +92,12 @@ func (a itemizeAttrs) changed() bool { return false } -const itemizeNewSuffix = "+++++++++" // real rsync's own "newly created" marker, all 9 positions +const itemizeNewSuffix = "+++++++++" // real rsync's "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. +// itemizeFile computes the %i code for a regular-file entry, comparing it +// against old (the destination's current state) when existed is true. +// contentChanged reports whether the file's actual bytes differ. +// report is false when nothing about the entry differs. 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 @@ -176,11 +124,7 @@ func itemizeFile(entry sync.FileEntry, old sync.FileEntry, existed bool, content 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)". + // '>' means data was actually transferred; '.' means attributes-only. y := byte('.') if contentChanged { y = '>' @@ -188,10 +132,7 @@ func itemizeFile(entry sync.FileEntry, old sync.FileEntry, existed bool, content 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. +// itemizeDir computes the %i code for a directory entry. func itemizeDir(entry sync.FileEntry, old sync.FileEntry, existed bool, opts sync.AttrOptions) (line string, report bool) { if !existed { return "cd" + itemizeNewSuffix, true @@ -217,13 +158,9 @@ func itemizeDir(entry sync.FileEntry, old sync.FileEntry, existed bool, opts syn 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). +// itemizeSymlink computes the %i code for a symlink entry. Its "c" +// position means the link target changed, unlike "c" for a regular file +// (checksum, which grsync never sets). func itemizeSymlink(entry sync.FileEntry, old sync.FileEntry, existed bool, opts sync.AttrOptions) (line string, report bool) { if !existed { return "cL" + itemizeNewSuffix, true @@ -243,28 +180,16 @@ func itemizeSymlink(entry sync.FileEntry, old sync.FileEntry, existed bool, opts 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. +// member (real rsync's 'h' update type). 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". +// formatItemizeLine joins an %i code with real rsync's "%i %n%L" layout. func formatItemizeLine(code, path, linkTarget string) string { var b strings.Builder b.WriteString(code) @@ -277,8 +202,8 @@ func formatItemizeLine(code, path, linkTarget string) string { 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. +// formatVerboseLine is real rsync's "%n%L" format, used when -v is given +// without -i. func formatVerboseLine(path, linkTarget string) string { if linkTarget == "" { return path @@ -287,13 +212,7 @@ func formatVerboseLine(path, linkTarget string) string { } // reportChange writes one itemize/verbose line for entry to ropts.Output, -// if ropts requests itemize or verbose reporting specifically (not -// ropts.Reporting(), which also covers Progress/Stats - those have -// nothing to do with per-entry itemize/verbose lines, and gating on the -// broader check would incorrectly print verbose-style lines for a caller -// that asked only for --progress or --stats) 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. +// if requested and report says this entry is worth mentioning. func reportChange(ropts ReceiverOptions, code string, report bool, entry sync.FileEntry) { if (!ropts.Itemize && !ropts.Verbose) || !report { return diff --git a/internal/pipeline/itemize_test.go b/internal/pipeline/itemize_test.go index dfe09b8..94bf1b4 100644 --- a/internal/pipeline/itemize_test.go +++ b/internal/pipeline/itemize_test.go @@ -43,11 +43,8 @@ func TestItemizeFile_PermsOwnerGroupChanged(t *testing.T) { 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)". + // contentChanged=false exercises the attributes-only update case: Y + // must be '.', not '>'. code, report := itemizeFile(entry, old, true, false, fullAttrOpts) if !report { t.Fatalf("report = false for a file with changed perms/owner/group, want true") @@ -69,9 +66,6 @@ func TestItemizeFile_UnchangedIsNotReported(t *testing.T) { 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") diff --git a/internal/pipeline/messages.go b/internal/pipeline/messages.go index a0eda2b..32bf2f5 100644 --- a/internal/pipeline/messages.go +++ b/internal/pipeline/messages.go @@ -1,8 +1,6 @@ // Package pipeline wires internal/sync (file enumeration, filtering, // delta algorithm, attribute preservation) and internal/transport (framed -// subprocess/SSH connections) together into an actual sync. Neither of -// those packages imports the other - this package sits above both, -// importing them, so that layering stays intact. +// subprocess/SSH connections) together into an actual sync. package pipeline import ( @@ -16,14 +14,8 @@ import ( ) // Encoding note: every message below is encoded with encoding/gob, not -// upstream rsync's actual wire protocol. That's a deliberate scope -// boundary, not an oversight: rsync's real wire format is an intricate, -// versioned binary protocol, and reimplementing it is a large, separate -// effort that doesn't belong inside this already-large integration -// ticket. gob is a reasonable, low-effort, *correct* choice specifically -// because grsync only ever talks to grsync here, never to real rsync - -// but true rsync protocol interoperability, if ever wanted, is future -// work, not something to let "protocol" quietly come to mean "gob." +// upstream rsync's actual wire protocol - a deliberate scope boundary, +// since grsync only ever talks to grsync here. // deltaOpKind tags which sync.DeltaOp variant a wireDeltaOp represents. type deltaOpKind byte @@ -34,10 +26,8 @@ const ( ) // wireDeltaOp is a wire-safe stand-in for sync.DeltaOp: DeltaOp is a -// sealed interface (CopyOp/DataOp), which gob cannot encode directly -// without registering concrete types with the encoder. Converting -// explicitly to/from this struct is more transparent than relying on -// gob's interface-registration machinery for just two variants. +// sealed interface (CopyOp/DataOp), which gob can't encode directly +// without registering concrete types. type wireDeltaOp struct { Kind deltaOpKind BlockIndex int // valid when Kind == deltaOpKindCopy @@ -45,13 +35,10 @@ type wireDeltaOp struct { Length int // valid when Kind == deltaOpKindData and the enclosing deltaMessage IS compressed: how many bytes of its decompressed Literal stream belong to this op } -// toWireDeltaOps converts ops to their wire form, compressing this -// file's entire literal-data stream as a single zlib unit when copts -// calls for it (see deltaMessage's own doc comment for why whole-file, -// not per-op) - path is only used to check copts.SkipSuffixes, never -// otherwise. CopyOp block indices are never touched: they're plain -// integers, not data, and compressing them would only add overhead for -// nothing. +// toWireDeltaOps converts ops to their wire form, compressing this file's +// entire literal-data stream as a single zlib unit when copts calls for +// it. CopyOp block indices are never compressed, since they're plain +// integers, not data. func toWireDeltaOps(ops []sync.DeltaOp, path string, copts CompressOptions) (wire []wireDeltaOp, compressed bool, literal []byte, err error) { wire = make([]wireDeltaOp, len(ops)) @@ -89,9 +76,8 @@ func toWireDeltaOps(ops []sync.DeltaOp, path string, copts CompressOptions) (wir } // fromWireDeltaOps reverses toWireDeltaOps: when compressed is true, it -// decompresses literal once and re-slices it back into each op's own -// bytes using the Length each wireDeltaOp carried; otherwise each op's -// Bytes is used directly, exactly as before compression existed. +// decompresses literal once and re-slices it back into each op's bytes +// using the Length each wireDeltaOp carried. func fromWireDeltaOps(wire []wireDeltaOp, compressed bool, literal []byte) ([]sync.DeltaOp, error) { var decompressed []byte if compressed { @@ -127,18 +113,13 @@ func fromWireDeltaOps(wire []wireDeltaOp, compressed bool, literal []byte) ([]sy } // signatureMessage is FrameSignature's payload: one regular file's -// signature, tagged with its Path. Path is included even though both -// sides already process the file list in the same agreed-upon order - -// it's a cheap, valuable consistency check (see recvSignature/recvDelta) -// against a class of bug (an off-by-one, a dropped frame) that -// position-only encoding could never detect and would silently -// misapply one file's delta to another. +// signature, tagged with Path as a consistency check against an +// off-by-one or dropped frame silently misapplying one file's delta to +// another. // -// Append (SC-12's own contribution) tells Sender how to respond to this -// signature - see appendAction's own doc comment for the three -// possibilities. It defaults to appendNone (gob's zero value), so every -// signatureMessage sent before --append/--append-verify existed decodes -// exactly as it always did. +// Append tells Sender how to respond to this signature, and defaults to +// appendNone (gob's zero value), so a signatureMessage sent before +// --append existed still decodes correctly. type signatureMessage struct { Path string Sig sync.Signature @@ -146,43 +127,33 @@ type signatureMessage struct { } // appendAction is carried on a signatureMessage to tell Sender how to -// respond to it - see receiver.go's own doc comment on where each value -// gets chosen, and sender.go's own doc comment on how each is handled. +// respond to it. type appendAction byte const ( - // appendNone is the normal, pre-SC-12 flow: Sender runs - // sync.GenerateDelta against Sig exactly as it always has. + // appendNone is the normal flow: Sender runs sync.GenerateDelta + // against Sig as usual. appendNone appendAction = iota - // appendTail means "the receiver's existing Sig.BlockSize bytes are - // blindly trusted, unverified - send only the literal tail past - // that offset" (--append). Sig.BlockSize carries that trusted - // offset (not a real block size at all here); Sig.Blocks is unused. - // See receiver.go for exactly when this is chosen and sender.go for - // how it's handled. + // appendTail means the receiver's existing Sig.BlockSize bytes are + // trusted unverified; only the literal tail past that offset is sent. + // Sig.BlockSize carries that trusted offset, not a real block size; + // Sig.Blocks is unused. appendTail - // appendSkip means "the destination is already at least as long as - // the source - don't read or compare anything at all, just - // acknowledge with an empty delta" (the --append/--append-verify - // "not shorter, skip entirely" eligibility rule). + // appendSkip means the destination is already at least as long as the + // source: acknowledge with an empty delta without reading or + // comparing anything. appendSkip ) // deltaMessage is FrameDelta's payload: one regular file's delta ops, -// tagged with its Path for the same reason as signatureMessage. +// tagged with Path for the same reason as signatureMessage. // // Literal holds every DataOp's bytes for this file, zlib-compressed -// together as a single stream when Compressed is true - each op's own -// wireDeltaOp.Bytes is left empty in that case, and its Length instead -// says how many of Literal's decompressed bytes are its (see -// toWireDeltaOps/fromWireDeltaOps). Compressing the whole file's literal -// data as one unit, rather than op-by-op, amortizes zlib's fixed ~8-byte -// header/trailer overhead across the entire file instead of paying it -// again for every small literal run a scattered-changes file can -// produce - see compressLiteral's own doc comment. When Compressed is -// false, Literal is unused (nil) and every op carries its own Bytes -// directly, exactly the wire shape this type had before --compress -// existed. +// together as a single stream when Compressed is true, amortizing zlib's +// fixed header/trailer overhead across the whole file instead of paying +// it per op; each wireDeltaOp's Length then says how many of Literal's +// decompressed bytes are its own. When Compressed is false, Literal is +// unused and every op carries its own Bytes directly. type deltaMessage struct { Path string Ops []wireDeltaOp @@ -205,10 +176,8 @@ func decodeGob(data []byte, v any) error { return nil } -// readTypedFrame reads one frame from rw and confirms it has the -// expected type, translating a FrameError from the peer into a normal Go -// error along the way so a remote-side failure surfaces as an error here -// rather than a confusing "wrong frame type" mismatch. +// readTypedFrame reads one frame from rw and confirms it has the expected +// type, translating a peer's FrameError into a normal Go error. func readTypedFrame(rw io.Reader, want transport.FrameType, what string) (transport.Frame, error) { f, err := transport.ReadFrame(rw) if err != nil { @@ -224,12 +193,8 @@ func readTypedFrame(rw io.Reader, want transport.FrameType, what string) (transp } // fileListMessage is FrameFileList's payload: the filtered file list plus -// which entries are hard-linked to each other on the source, so that -// grouping travels with the list itself rather than needing a separate -// round trip - HardLinkGroups is empty exactly when there's nothing to -// say about hard links, either because the source tree has none or -// because the sending platform can't detect them (sync.HardLinksSupported() -// is false). +// which entries are hard-linked to each other, so the grouping travels +// with the list instead of needing a separate round trip. type fileListMessage struct { Entries []sync.FileEntry HardLinkGroups []sync.HardLinkGroup diff --git a/internal/pipeline/messages_test.go b/internal/pipeline/messages_test.go index f75ba89..39526c8 100644 --- a/internal/pipeline/messages_test.go +++ b/internal/pipeline/messages_test.go @@ -141,13 +141,6 @@ func TestDeltaRoundTrip(t *testing.T) { } } -// TestDeltaRoundTrip_Compressed is SC-9's core wire-format proof: sending -// a delta with compression enabled must produce a smaller frame than the -// same ops sent uncompressed, and recvDelta on the compressed side must -// still reconstruct byte-identical ops - proving toWireDeltaOps/ -// fromWireDeltaOps' compress-once-per-file, re-slice-by-Length design -// (see deltaMessage's own doc comment) round-trips correctly, not just -// that compression was attempted. func TestDeltaRoundTrip_Compressed(t *testing.T) { literal := []byte(strings.Repeat("compressible literal data ", 200)) ops := []sync.DeltaOp{ @@ -198,13 +191,8 @@ func TestDeltaRoundTrip_Compressed(t *testing.T) { } } -// TestDeltaRoundTrip_SkipCompressSuffixIsGenuinelyUncompressed is the -// ticket's explicit "inspect actual bytes sent, not just trust the flag -// was read" requirement: it decodes the raw gob frame directly (the same -// way recvDelta itself does internally) and asserts deltaMessage.Compressed -// is false and every op still carries its own literal Bytes, for a path -// whose suffix is in copts.SkipSuffixes - even though copts.Enabled is -// true and the literal data is highly compressible. +// Decodes the raw gob frame directly to inspect deltaMessage itself, +// rather than trusting that the Enabled flag was merely read. func TestDeltaRoundTrip_SkipCompressSuffixIsGenuinelyUncompressed(t *testing.T) { literal := []byte(strings.Repeat("z", 5000)) // trivially compressible ops := []sync.DeltaOp{sync.DataOp{Bytes: literal}} @@ -235,11 +223,6 @@ func TestDeltaRoundTrip_SkipCompressSuffixIsGenuinelyUncompressed(t *testing.T) } } -// TestDeltaRoundTrip_DisabledIsGenuinelyUncompressed is -// TestDeltaRoundTrip_SkipCompressSuffixIsGenuinelyUncompressed's -// counterpart for CompressOptions{} (the zero value, --compress not -// given at all): the wire frame must be byte-identical in shape to the -// pre-SC-9 format, not merely "the same size by coincidence." func TestDeltaRoundTrip_DisabledIsGenuinelyUncompressed(t *testing.T) { ops := []sync.DeltaOp{sync.DataOp{Bytes: []byte(strings.Repeat("y", 5000))}} diff --git a/internal/pipeline/partial.go b/internal/pipeline/partial.go index a33fef1..43305c8 100644 --- a/internal/pipeline/partial.go +++ b/internal/pipeline/partial.go @@ -1,31 +1,16 @@ // partial.go implements --partial/--partial-dir. // -// grsync's wire protocol has no streaming I/O (see sync.ApplyDelta's own -// doc comment): one regular file's delta arrives as a single, atomic -// gob-encoded frame, fully decoded into memory before a single byte of -// it is written to disk. That means there is no such thing as "half of -// this file's delta arrived" - a dropped connection mid-frame just means -// this file's transfer never started at all, and everything already -// written for *earlier* files in the same sync stays exactly as -// complete as it already was. --partial in grsync is therefore -// file-granularity, not real rsync's true byte-level mid-file -// resumption: "partial" describes which whole files survive an aborted -// multi-file sync, not a partially-written single file left in a -// half-complete state. +// grsync's wire protocol has no streaming I/O: a file's delta arrives as +// one atomic frame, fully decoded before any byte is written to disk. So +// --partial here is file-granularity, not real rsync's byte-level +// mid-file resumption - it decides which whole files survive an aborted +// multi-file sync, not how a half-written single file is left. // -// That distinction only holds, though, if writing a file's new content -// is itself all-or-nothing from the destination's point of view - and -// before this ticket, it wasn't: receiveRegularFile wrote straight to -// destPath (os.WriteFile / a chunked os.OpenFile+Write loop for -// progress reporting), so a process killed mid-write left a genuinely -// truncated, corrupted file sitting at the real destination path, with -// no flag able to prevent or recover from it. Implementing --partial -// correctly requires the prerequisite real rsync itself always has: a -// separate temp file, written first, then atomically renamed into -// place only once it's complete. That temp-file+rename path is now -// unconditional (see receiver.go), not something --partial turns on - -// --partial/--partial-dir only control what happens to that temp file -// if the transfer aborts before the rename. +// Writing a file's new content must itself be all-or-nothing from the +// destination's point of view, so every regular file is written to a temp +// file first and atomically renamed into place; --partial/--partial-dir +// only control what happens to that temp file if the transfer aborts +// before the rename. package pipeline @@ -35,15 +20,11 @@ import ( "path/filepath" ) -// createTempFile creates a new, uniquely-named temp file in the same -// directory as destPath - same-directory, not a system temp dir, so the -// eventual rename into place is same-filesystem (and therefore atomic on -// every platform this project supports). The returned file's mode is -// explicitly set to 0644 (best-effort - see the inline comment) to match -// what a file written without --perms has always ended up as -// (os.WriteFile's own default), since os.CreateTemp's own default of -// 0600 would otherwise quietly become the new no-perms default the -// moment nothing later overrides it via sync.ApplyAttributes. +// createTempFile creates a uniquely-named temp file next to destPath +// (same directory, so the eventual rename is same-filesystem and atomic). +// Its mode is set to 0644 to match os.WriteFile's own default, since +// os.CreateTemp's 0600 default would otherwise become the new no-perms +// default whenever --perms doesn't override it later. func createTempFile(destPath string) (*os.File, error) { dir := filepath.Dir(destPath) base := filepath.Base(destPath) @@ -51,23 +32,17 @@ func createTempFile(destPath string) (*os.File, error) { if err != nil { return nil, err } - // Best-effort: a handful of exotic filesystems don't support - // changing an open file's mode the same way a normal POSIX one - // does. Getting this wrong only matters when --perms is NOT given - // (sync.ApplyAttributes overwrites it correctly whenever --perms - // IS given, after the rename below), so a failure here is not worth - // aborting the whole transfer over. + // Best-effort: some exotic filesystems can't chmod an open file, and + // this only matters when --perms is not given (sync.ApplyAttributes + // fixes it otherwise), so a failure here isn't worth aborting over. _ = f.Chmod(0o644) return f, nil } -// writeToTempFileWithProgress is writeFileWithProgress's SC-12 -// counterpart: identical chunking/progress-reporting behavior (see its -// own doc comment for why nil-progress and small files both skip -// chunking), but targets a fresh temp file next to destPath instead of -// writing destPath directly. tmpPath is always returned, even when err -// != nil, so the caller can still apply Partial/PartialDir policy to -// however much was actually written before the failure. +// writeToTempFileWithProgress writes data to a fresh temp file next to +// destPath, chunked with progress updates when progress is non-nil. +// tmpPath is always returned, even on error, so the caller can still apply +// Partial/PartialDir policy to whatever was actually written. func writeToTempFileWithProgress(destPath string, data []byte, progress *progressReporter, path string, xferNum, totalFiles, filesLeft int) (tmpPath string, err error) { f, err := createTempFile(destPath) if err != nil { @@ -110,17 +85,11 @@ func writeToTempFileWithProgress(destPath string, data []byte, progress *progres } // partialFilePath computes where relPath's partial file lives under -// ropts.PartialDir, mirroring real rsync's own documented placement -// rule: a relative PartialDir is created inside *each file's own -// destination directory* ("this makes it easy to use a relative path... -// to have rsync create the partial-directory in the destination file's -// directory"), so files with the same basename in different -// subdirectories can never collide with each other. An absolute -// PartialDir is a single shared directory, so relPath's full relative -// path is mirrored underneath it instead - real rsync's docs don't spell -// out this exact collision-avoidance scheme for the absolute case, but -// mirroring the relative path is the only scheme that can't collide -// between two files sharing a basename in different subdirectories. +// partialDir, mirroring real rsync's placement rule: a relative +// partialDir is created inside each file's own destination directory; an +// absolute one is a single shared directory, mirroring relPath's full +// relative path underneath it so same-basename files in different +// subdirectories can't collide. func partialFilePath(destPath, partialDir, relPath string) string { if filepath.IsAbs(partialDir) { return filepath.Join(partialDir, filepath.FromSlash(relPath)) @@ -128,16 +97,11 @@ func partialFilePath(destPath, partialDir, relPath string) string { return filepath.Join(filepath.Dir(destPath), partialDir, filepath.Base(destPath)) } -// loadPartialBasis returns the content of relPath's partial file, if -// ropts.PartialDir is set and a usable file is actually there - used in -// place of the real destination file's own content as the delta -// comparison basis, giving a resumed transfer real content-level -// speedup (see delta.go's own algorithm: a signature built from a -// partial file's already-correct prefix naturally yields CopyOps for -// whatever still matches and DataOps only for the genuinely new tail). -// ok is false whenever there's nothing usable to resume from - PartialDir -// unset, no file there, or an unreadable one - in which case the caller -// falls back to the real destination file exactly as it always has. +// loadPartialBasis returns relPath's partial file content, if PartialDir +// is set and a usable file is there, to use as the delta comparison basis +// instead of the real destination file. ok is false whenever there's +// nothing usable to resume from, and the caller falls back to the real +// destination file. func loadPartialBasis(destPath string, ropts ReceiverOptions, relPath string) (data []byte, ok bool) { if ropts.PartialDir == "" { return nil, false @@ -149,15 +113,12 @@ func loadPartialBasis(destPath string, ropts ReceiverOptions, relPath string) (d return data, true } -// finishRegularFileWrite is receiveRegularFile's single exit point for -// everything written by writeToTempFileWithProgress: on success -// (writeErr == nil), it commits by renaming tmpPath onto destPath and, -// if a partial-dir file was used as this transfer's basis, removes it -// (real rsync's own documented "delete it after it has served its -// purpose"). On any failure - the write itself, or the commit rename - -// it hands off to abandonOrKeep to apply Partial/PartialDir policy, then -// returns the ORIGINAL error (a failure to honor Partial is never more -// important than surfacing why the transfer actually failed). +// finishRegularFileWrite is receiveRegularFile's single exit point after +// writeToTempFileWithProgress: on success it renames tmpPath onto +// destPath and removes a used partial-dir basis file; on any failure it +// hands off to abandonOrKeep and returns the original error, since +// honoring Partial is never more important than surfacing why the +// transfer failed. func finishRegularFileWrite(tmpPath, destPath string, writeErr error, ropts ReceiverOptions, relPath string, usedPartialBasis bool) error { if writeErr == nil { if err := os.Rename(tmpPath, destPath); err != nil { @@ -172,20 +133,10 @@ func finishRegularFileWrite(tmpPath, destPath string, writeErr error, ropts Rece } // abandonOrKeep applies Partial/PartialDir policy to a temp file whose -// transfer did not complete successfully, then returns origErr -// unchanged so the caller's own error still propagates. Every exit path -// here ends with tmpPath gone from its original same-directory-as-dest -// location - either deleted outright, or moved somewhere else entirely -// (partial-dir or destPath itself) - so a temp file this function has -// handled is never left sitting next to the destination under its own -// ".name.RANDOM.grsync-tmp" name, regardless of which policy applies. -// -// This is also the self-review's own "could a partial file leak outside -// --partial-dir" answer made concrete: whenever ropts.PartialDir is set, -// the only two destinations tmpPath can end up at are partialFilePath's -// own return value or deletion - destPath itself is never touched by -// this function in that case, so a --partial-dir user's real destination -// tree is never modified by an aborted transfer at all. +// transfer didn't complete, then returns origErr unchanged. Every exit +// path removes tmpPath from its original location - deleted, or moved to +// partial-dir/destPath - so a --partial-dir user's real destination tree +// is never touched by an aborted transfer. func abandonOrKeep(tmpPath, destPath string, ropts ReceiverOptions, relPath string, origErr error) error { if !ropts.KeepPartial() { _ = os.Remove(tmpPath) diff --git a/internal/pipeline/partial_integration_test.go b/internal/pipeline/partial_integration_test.go index 2c81892..dc03a1e 100644 --- a/internal/pipeline/partial_integration_test.go +++ b/internal/pipeline/partial_integration_test.go @@ -12,23 +12,6 @@ import ( "github.com/syntaxroot-cc/grsync/internal/sync" ) -// TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial -// is SC-12's own core "interrupted transfer" proof, matching the -// existing TestReceiver_ConnectionDropsMidTransfer/ -// TestReceiver_AppliesHardLinksFromReceivedGroups pattern of driving -// Receiver against a hand-built peer goroutine for exact control over -// when the connection dies. It drives home Step 3's own architectural -// finding: grsync's "partial" is file-granularity, not true mid-file -// resumption, so completed files survive a drop *regardless* of -// --partial, and the in-flight file at the moment of the drop is simply -// never attempted at all (absent, not corrupted, not partially written) - -// because its signature/delta exchange never got far enough to produce -// any bytes to write in the first place. This test runs the identical -// scenario with and without ReceiverOptions.Partial to prove that flag -// genuinely makes no difference to this particular failure mode - the -// flag's real, distinguishing effect (see partial_test.go's own -// abandonOrKeep tests) only matters for a failure *during* the local -// write phase, not a dropped connection between files. func TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial(t *testing.T) { for _, ropts := range []ReceiverOptions{{}, {Partial: true}, {PartialDir: ".rsync-partial"}} { t.Run(fmt.Sprintf("%+v", ropts), func(t *testing.T) { @@ -68,11 +51,8 @@ func TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial } } - // Receive the signature request for c.txt (proving Receiver - // got that far), then vanish without ever responding - - // simulating a connection that drops mid-transfer, the same - // way TestReceiver_ConnectionDropsMidTransfer does for a - // single file. + // Receive the signature request for c.txt, then vanish + // without ever responding. if _, err := recvSignature(peerReadsFromReceiver); err != nil { peerErrCh <- fmt.Errorf("receiving signature for c.txt: %w", err) return @@ -98,10 +78,8 @@ func TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial } } - // c.txt's transfer never got far enough to produce any bytes to - // write at all - it must be completely absent, not a - // zero-length or truncated file, and not present in a partial - // location either, regardless of ropts. + // c.txt must be completely absent, not zero-length or truncated, + // regardless of ropts. if _, statErr := os.Stat(filepath.Join(destRoot, "c.txt")); !os.IsNotExist(statErr) { t.Errorf("c.txt exists after the connection dropped before its own transfer began, want it completely absent") } @@ -126,25 +104,14 @@ func TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial } } -// TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry is "the pipeline -// can be told to resume," made concrete and measured, not just asserted: -// a leftover partial-dir file representing a genuine (correct) prefix of -// the real source content is picked up as the delta comparison basis on -// the next run - sync.GenerateDelta naturally turns that into CopyOps for -// the matching prefix and DataOps only for the new tail (see -// loadPartialBasis's own doc comment) - producing a real, measurable -// reduction in wire bytes compared to a from-scratch run with no -// resume basis available, and leaving the destination byte-for-byte -// correct either way. The partial-dir file is also removed once it's -// served its purpose, matching real rsync's own documented behavior. func TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry(t *testing.T) { fullContent := strings.Repeat("resumable content chunk, well over one block size ", 60) partialDir := ".rsync-partial" resumedSrc, resumedDest := t.TempDir(), t.TempDir() mustWriteFile(t, filepath.Join(resumedSrc, "big.txt"), fullContent) - // A genuine prefix of the real content, as if an earlier run had - // gotten this far before being interrupted. + // A genuine prefix, as if an earlier run had gotten this far before + // being interrupted. prefix := fullContent[:len(fullContent)*3/4] partialPath := filepath.Join(resumedDest, partialDir, "big.txt") mustMkdirAll(t, filepath.Dir(partialPath)) @@ -159,8 +126,7 @@ func TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry(t *testing.T) { t.Errorf("partial-dir file %q still exists after a successful resumed transfer, want it removed", partialPath) } - // Baseline: the identical transfer with no partial-dir file to resume - // from at all (a completely fresh destination). + // Baseline: a completely fresh destination, no resume basis. freshSrc, freshDest := t.TempDir(), t.TempDir() mustWriteFile(t, filepath.Join(freshSrc, "big.txt"), fullContent) freshBytes := runSenderReceiverWithCompressOptions(t, freshSrc, freshDest, @@ -171,13 +137,6 @@ func TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry(t *testing.T) { } } -// TestReceiver_PartialDirBasisIgnoresRealDestinationContent confirms -// loadPartialBasis genuinely takes priority over the real destination -// file when both exist: the real destination here is deliberately wrong -// (would produce an incorrect comparison basis on its own), while the -// partial-dir file holds the correct prefix - the final result must -// still be correct, proving the partial-dir file is what actually got -// used, not silently ignored in favor of the real (wrong) destination. func TestReceiver_PartialDirBasisIgnoresRealDestinationContent(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() fullContent := strings.Repeat("A", 1400) + "the new tail" @@ -197,16 +156,6 @@ func TestReceiver_PartialDirBasisIgnoresRealDestinationContent(t *testing.T) { assertSameContent(t, filepath.Join(srcRoot, "file.txt"), filepath.Join(destRoot, "file.txt")) } -// TestReceiver_PartialDirCreatesNoFilesAndDeletesNothingDuringDryRun is -// this ticket's own explicit dependency requirement (SC-11 interaction): -// with --dry-run set, --partial-dir must neither write a temp file, nor -// commit anything to destPath, nor delete the leftover partial-dir file -// it would otherwise have consumed and cleaned up on a real run - -// loadPartialBasis is read-only by construction (see its own doc -// comment), and the temp-file/rename/cleanup code all lives inside -// receiveRegularFile's `if !ctx.ropts.DryRun` guard, so a dry run should -// never reach any of it; this test is the proof, not just the -// structural argument. func TestReceiver_PartialDirCreatesNoFilesAndDeletesNothingDuringDryRun(t *testing.T) { srcRoot, destRoot := t.TempDir(), t.TempDir() fullContent := strings.Repeat("resumable content ", 100) diff --git a/internal/pipeline/partial_test.go b/internal/pipeline/partial_test.go index 18bcde0..652b3dd 100644 --- a/internal/pipeline/partial_test.go +++ b/internal/pipeline/partial_test.go @@ -44,9 +44,6 @@ func TestCreateTempFile_DefaultModeMatchesOsWriteFile(t *testing.T) { if err != nil { t.Fatalf("Stat: %v", err) } - // os.CreateTemp's own default is 0600; createTempFile must override - // that to 0644 so a sync run without --perms doesn't quietly start - // producing more restrictive files than os.WriteFile always has. if info.Mode().Perm() != 0o644 { t.Errorf("temp file mode = %v, want 0644 (matching os.WriteFile's own default for a no---perms sync)", info.Mode().Perm()) } @@ -74,11 +71,6 @@ func TestPartialFilePath_AbsoluteDirMirrorsRelativePath(t *testing.T) { } } -// TestPartialFilePath_AbsoluteDirAvoidsBasenameCollisions is the actual -// reason the absolute case mirrors the full relative path instead of -// just the basename: two different source files sharing a basename in -// different subdirectories must not collide under one shared -// partial-dir. func TestPartialFilePath_AbsoluteDirAvoidsBasenameCollisions(t *testing.T) { absDir := string(filepath.Separator) + "partial-store" if runtime.GOOS == "windows" { @@ -163,11 +155,6 @@ func TestFinishRegularFileWrite_SuccessRenamesIntoPlace(t *testing.T) { } } -// TestFinishRegularFileWrite_SuccessCleansUpUsedPartialBasis is real -// rsync's own documented "delete it after it has served its purpose" -// behavior: once a partial-dir file's content has been successfully -// folded into a completed transfer, it should be removed so it doesn't -// linger as stale, misleading resume data for a future run. func TestFinishRegularFileWrite_SuccessCleansUpUsedPartialBasis(t *testing.T) { dir := t.TempDir() destPath := filepath.Join(dir, "file.txt") @@ -248,12 +235,6 @@ func TestAbandonOrKeep_PartialDirMovesContentThereInsteadOfDestPath(t *testing.T } } -// TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent is the -// self-review's own "could a partial file leak outside --partial-dir" -// question, answered concretely: an aborted transfer with --partial-dir -// set must never modify whatever was already sitting at the real -// destination path, even though a plain --partial (no dir) would -// deliberately overwrite it with the partial content. func TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent(t *testing.T) { dir := t.TempDir() destPath := filepath.Join(dir, "file.txt") @@ -274,12 +255,6 @@ func TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent(t *testing.T } } -// TestAbandonOrKeep_NeverLeavesATempFileNextToDestPath is the same -// self-review question from the opposite angle: regardless of which -// policy applies, the raw ".file.txt.*.grsync-tmp" temp file must never -// be the thing left behind in the destination's own directory - it's -// always either deleted or moved to one of the two real, documented -// destinations (destPath itself, or inside PartialDir). func TestAbandonOrKeep_NeverLeavesATempFileNextToDestPath(t *testing.T) { for _, ropts := range []ReceiverOptions{ {}, diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 3441ac0..e2f2734 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -15,9 +15,7 @@ import ( ) // runSenderReceiver drives Sender and Receiver concurrently over a pair -// of io.Pipes wired crosswise, the same in-memory-transport pattern used -// in internal/transport's own handshake test - no subprocess or SSH -// needed to validate the pipeline logic itself. +// of in-memory pipes wired crosswise. func runSenderReceiver(t *testing.T, src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions) { t.Helper() @@ -51,11 +49,7 @@ func runSenderReceiver(t *testing.T, src, dest string, walkOpts sync.WalkOptions } } -// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter, -// the same helper internal/transport's own handshake test defines for -// itself - duplicated here rather than exported from transport, since -// it's a small, test-only convenience, not part of either package's -// real API. +// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter. type pipeReadWriter struct { io.Reader io.Writer @@ -120,15 +114,6 @@ func TestSenderReceiver_DestinationOnlyFileIsLeftAlone(t *testing.T) { } } -// TestSenderReceiver_VerboseAloneShowsNamesOnly is SC-10's Step 4 -// confirmation, not a reimplementation: SC-11 already wired -v to print -// real rsync's own "%n%L" (bare path, no itemize code) when Verbose is -// set without Itemize, but no test ever exercised that combination -// directly - worth checking explicitly, especially since this exact area -// (reportChange's gating) turned out to have a real bug introduced while -// broadening ReceiverOptions.Reporting() for --progress/--stats (see -// itemize.go's reportChange, which now correctly gates on Itemize/Verbose -// specifically, not the broader Reporting()). func TestSenderReceiver_VerboseAloneShowsNamesOnly(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -149,7 +134,6 @@ func TestSenderReceiver_VerboseAloneShowsNamesOnly(t *testing.T) { if !strings.Contains(output, "sub/nested.txt") { t.Errorf("output = %q, want it to mention sub/nested.txt", output) } - // The defining difference from -i: no itemize code prefix at all. if strings.Contains(output, "+++++++++") { t.Errorf("output = %q, want bare paths only (no itemize codes) since Itemize was not requested", output) } @@ -183,14 +167,6 @@ func TestSenderReceiver_Symlink(t *testing.T) { } } -// TestSenderReceiver_DirectoryAttributesSurviveChildCreation is the direct -// proof for the ordering fix in Receiver: a directory's mtime is set to a -// deliberately distinctive value at the source. If Receiver applied -// directory attributes immediately upon creating the directory (before -// writing its children), the filesystem would silently bump that mtime -// again the moment the child file inside it gets created afterward, -// and this test would catch that regression by finding the destination -// directory's mtime does NOT match the source's. func TestSenderReceiver_DirectoryAttributesSurviveChildCreation(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -199,11 +175,7 @@ func TestSenderReceiver_DirectoryAttributesSurviveChildCreation(t *testing.T) { mustMkdirAll(t, srcSub) mustWriteFile(t, filepath.Join(srcSub, "child.txt"), "child content") - // A deliberately distinctive, easy-to-misidentify-as-"now" mtime, set - // on the source directory *after* its child already exists - matching - // what a real source tree looks like (the directory's own mtime - // reflects whenever it was last deliberately touched, not literally - // "the moment before this test ran"). + // Set after the child already exists, matching a real source tree. wantDirTime := time.Date(2019, time.May, 4, 10, 0, 0, 0, time.UTC) if err := os.Chtimes(srcSub, wantDirTime, wantDirTime); err != nil { t.Fatalf("Chtimes on source directory: %v", err) @@ -218,32 +190,14 @@ func TestSenderReceiver_DirectoryAttributesSurviveChildCreation(t *testing.T) { t.Fatalf("Stat destination directory: %v", err) } if !info.ModTime().Equal(wantDirTime) { - t.Errorf("destination directory ModTime = %v, want %v (likely bumped by child creation - "+ - "directory attributes must be applied AFTER children are written, not before)", + t.Errorf("destination directory ModTime = %v, want %v (likely bumped by child creation)", info.ModTime(), wantDirTime) } } -// TestSenderReceiver_HardLinks is SC-18's end-to-end proof, with -// AttrOptions.HardLinks explicitly requested (the -H/--hard-links -// opt-in - see TestSenderReceiver_HardLinksNotPreservedWithoutOptIn for -// the default-off case): two hard-linked source files must arrive at the -// destination still hard-linked to each other (the same underlying file, -// not two independent copies that merely happen to have identical -// content), and an unrelated third file must NOT be linked to either of -// them. Where this platform can't detect hard links at all (Windows - -// sync.HardLinksSupported()), the same sync must still succeed and -// produce byte-correct content - as independent copies, not an error - -// exactly the graceful-degradation behavior sync.DetectHardLinks itself -// already documents. -// -// os.SameFile is used for the linked/unlinked check rather than -// platform-specific stat code in the test itself: it reports genuine -// same-file identity on every platform Go supports, including Windows -// (via the Win32 file index), even though grsync's own hard-link -// *detection* can't see that identity there - so this test can assert -// the real, portable ground truth regardless of what grsync itself was -// able to observe. +// TestSenderReceiver_HardLinks also covers graceful degradation on a +// platform where sync.HardLinksSupported() is false: the sync must still +// succeed, as independent copies rather than an error. func TestSenderReceiver_HardLinks(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -294,9 +248,8 @@ func TestSenderReceiver_HardLinks(t *testing.T) { t.Fatalf("%q and %q are independent files at the destination, want them hard-linked (same underlying file) like they are at the source", destOriginal, destLinked) } - // Prove it directly, not just via os.SameFile: a write through one - // path must be visible through the other, the same proof - // internal/sync's own TestApplyHardLinks uses. + // Prove it directly: a write through one path must be visible through + // the other. if err := os.WriteFile(destLinked, []byte("changed via the linked path"), 0o644); err != nil { t.Fatalf("WriteFile(%q): %v", destLinked, err) } @@ -309,15 +262,6 @@ func TestSenderReceiver_HardLinks(t *testing.T) { } } -// TestSenderReceiver_HardLinksNotPreservedWithoutOptIn is -// TestSenderReceiver_HardLinks's counterpart for the default case: -// without AttrOptions.HardLinks set, hard-linked source files must sync -// as independent copies - correct content, but not linked - even on a -// platform that could have detected and preserved the relationship. This -// is the direct proof that hard-link detection is genuinely opt-in -// (matching real rsync's own -H/--hard-links, not implied by any other -// flag), not just documented as opt-in while actually running -// unconditionally. func TestSenderReceiver_HardLinksNotPreservedWithoutOptIn(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -347,12 +291,8 @@ 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. +// runSenderReceiverWithOptions is runSenderReceiver with control over +// ReceiverOptions. func runSenderReceiverWithOptions(t *testing.T, src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts ReceiverOptions) { t.Helper() @@ -386,19 +326,10 @@ func runSenderReceiverWithOptions(t *testing.T, src, dest string, walkOpts sync. } } -// runSenderReceiverWithCompressOptions is runSenderReceiverWithOptions's -// counterpart for tests that need control over the sender's compression -// behavior - kept separate for the same reason runSenderReceiverWithOptions -// itself was (see its own doc comment): none of the many existing -// runSenderReceiver/runSenderReceiverWithOptions callers care about -// compression, so Sender's CompressOptions parameter stays defaulted to -// CompressOptions{} (disabled) in those two instead of forcing every -// existing call site to pass one. -// -// Returns the number of bytes Sender actually wrote to the connection -// (via the same countingReadWriter Stats itself uses - see stats.go), so -// callers can compare compressed vs. uncompressed wire size against real -// bytes sent, not just trust that the flag was read. +// runSenderReceiverWithCompressOptions is runSenderReceiverWithOptions +// with control over Sender's CompressOptions. Returns the number of bytes +// Sender actually wrote to the connection, so callers can compare +// compressed vs. uncompressed wire size. func runSenderReceiverWithCompressOptions(t *testing.T, src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts ReceiverOptions, copts CompressOptions) (bytesWritten int64) { t.Helper() @@ -434,12 +365,6 @@ func runSenderReceiverWithCompressOptions(t *testing.T, src, dest string, walkOp return sender.written } -// TestSenderReceiver_CompressReducesWireBytesForCompressibleContent is -// SC-9's core end-to-end proof: syncing the exact same highly-compressible -// file with --compress enabled must write genuinely fewer bytes to the -// wire than syncing it uncompressed, and the destination content must -// still come out byte-identical to the source either way - compression -// must never be observable in the result, only in the traffic. func TestSenderReceiver_CompressReducesWireBytesForCompressibleContent(t *testing.T) { content := strings.Repeat("the quick brown fox jumps over the lazy dog ", 2000) // ~90KB, highly compressible @@ -462,13 +387,6 @@ func TestSenderReceiver_CompressReducesWireBytesForCompressibleContent(t *testin assertSameContent(t, filepath.Join(compressedSrc, "big.txt"), filepath.Join(compressedDest, "big.txt")) } -// TestSenderReceiver_SkipCompressLeavesMatchingSuffixUncompressed proves -// --skip-compress end to end, not just at the wire-message level -// (TestDeltaRoundTrip_SkipCompressSuffixIsGenuinelyUncompressed already -// covers that): a highly-compressible file whose suffix is skip-listed -// must write essentially the same number of bytes as a fully-disabled -// compression run, while an identical file whose suffix is NOT -// skip-listed, synced in the same run, still compresses. func TestSenderReceiver_SkipCompressLeavesMatchingSuffixUncompressed(t *testing.T) { content := strings.Repeat("the quick brown fox jumps over the lazy dog ", 2000) @@ -484,12 +402,8 @@ func TestSenderReceiver_SkipCompressLeavesMatchingSuffixUncompressed(t *testing. sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{}, CompressOptions{Enabled: true, Level: DefaultCompressLevel, SkipSuffixes: []string{"bin"}}) - // Not asserting exact equality - gob framing overhead can vary by a - // handful of bytes for unrelated reasons - but a skipped file must be - // nowhere near as small as a genuinely compressed one would be; the - // compressible-content test above already shows compression more than - // halving a similarly-sized file, so any close-to-baseline result here - // is conclusive that skip-compress actually took effect. + // Not exact equality (gob framing overhead varies slightly), but a + // skipped file must stay close to the uncompressed baseline. if skipBytes < baselineBytes*9/10 { t.Errorf("skip-listed file wrote %d bytes, baseline (uncompressed) wrote %d bytes, want them close - skip-compress should have left this file uncompressed", skipBytes, baselineBytes) } @@ -497,22 +411,8 @@ func TestSenderReceiver_SkipCompressLeavesMatchingSuffixUncompressed(t *testing. assertSameContent(t, filepath.Join(src, "skip.bin"), filepath.Join(dest, "skip.bin")) } -// TestReceiver_StatsBytesReceivedReflectCompressedSize confirms Step 5's -// stats convention: real rsync's own "Total bytes sent"/"Total bytes -// received" measure what actually crossed the wire, which - once -// --compress is in play - genuinely is the compressed size, not the -// original file size. stats.go's countingReadWriter already wraps the -// raw connection needing no changes for this (see its own doc comment); -// this test is the proof that holds end to end, not just an inspection -// of the code. -// -// It's specifically "Total bytes received" (not "sent") that carries the -// file's compressible data here: Stats is computed on the Receiver side -// (SC-10's own design - see stats.go), and Receiver receives the file -// list and delta payloads from Sender while only ever sending small -// signature/ack messages back - so the large, compression-sensitive -// traffic flows in the "received" direction from this side's own point -// of view, not "sent". +// Uses "Total bytes received" since Stats is computed receiver-side, and +// the compressible payload flows from Sender to Receiver. func TestReceiver_StatsBytesReceivedReflectCompressedSize(t *testing.T) { content := strings.Repeat("the quick brown fox jumps over the lazy dog ", 2000) @@ -539,20 +439,15 @@ func TestReceiver_StatsBytesReceivedReflectCompressedSize(t *testing.T) { "stats must reflect actual wire bytes, not original file size", compressedReceived, uncompressedReceived) } - // Total file size is a property of the files themselves, unaffected by - // compression - the two runs synced byte-identical content, so this - // field specifically must match despite bytes sent differing. + // Total file size is unaffected by compression, unlike bytes sent. if got, want := statsField(t, compressedOut.String(), "Total file size"), statsField(t, uncompressedOut.String(), "Total file size"); got != want { t.Errorf("Total file size = %d with compression, %d without, want equal - compression must not affect this field", got, want) } } // runSenderReceiverWithCompressAndReceiverOptions combines -// runSenderReceiverWithCompressOptions and runSenderReceiverWithOptions' -// separate concerns (compression behavior and receiver-side reporting) -// for the one test above that needs both at once, rather than growing -// either of those two into a shared do-everything helper every other -// caller would need to pass zero values through. +// runSenderReceiverWithCompressOptions and runSenderReceiverWithOptions +// for tests needing both compression and receiver-side reporting control. func runSenderReceiverWithCompressAndReceiverOptions(t *testing.T, src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, attrOpts sync.AttrOptions, ropts ReceiverOptions, copts CompressOptions) { t.Helper() @@ -586,12 +481,6 @@ func runSenderReceiverWithCompressAndReceiverOptions(t *testing.T, src, dest str } } -// TestSenderReceiver_CompressWorksWithDryRun confirms Step 5's dry-run -// interaction: the signature/delta exchange (including compression of -// the delta's literal data) still needs to run byte-correct for accurate -// itemize output even though nothing is written - compression must not -// break that, and the dry-run destination must still end up completely -// empty. func TestSenderReceiver_CompressWorksWithDryRun(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -617,13 +506,6 @@ func TestSenderReceiver_CompressWorksWithDryRun(t *testing.T) { } } -// TestSenderReceiver_CompressWorksWithHardLinks confirms Step 5's -// hard-link interaction: a hard-link group's secondary members never go -// through the signature/delta exchange at all (Sender/Receiver both skip -// them outright), so --compress is moot for them by construction: this -// test's only job is to confirm that skip still holds correctly with -// --compress enabled, i.e. compression didn't somehow reintroduce a -// signature/delta round trip for a secondary member. func TestSenderReceiver_CompressWorksWithHardLinks(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -657,15 +539,9 @@ func TestSenderReceiver_CompressWorksWithHardLinks(t *testing.T) { } } -// TestSenderReceiver_CompressWorksWithEmptyAndUnchangedFiles is a -// self-review edge case: toWireDeltaOps only attempts compression when a -// file's concatenated literal data is non-empty (see its own doc -// comment), so a brand-new empty file (zero DataOps, per SC-10's own -// investigation into ApplyDelta's accumulator) and a byte-identical -// unchanged file (all CopyOps, zero DataOps) both take the "nothing to -// compress" path through that same function - this confirms both still -// sync correctly with --compress enabled, not just that compressible -// content does. +// Covers the edge case where toWireDeltaOps' "nothing to compress" path +// is taken: an empty file (zero DataOps) and an unchanged file (all +// CopyOps). func TestSenderReceiver_CompressWorksWithEmptyAndUnchangedFiles(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -684,11 +560,8 @@ func TestSenderReceiver_CompressWorksWithEmptyAndUnchangedFiles(t *testing.T) { } // 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. +// Receiver has: a file, a nested directory with a file, a symlink, and +// (best-effort) two hard-linked files. func buildRichTree(t *testing.T, root string) { t.Helper() mustWriteFile(t, filepath.Join(root, "top.txt"), "top level content") @@ -703,17 +576,6 @@ func buildRichTree(t *testing.T, root string) { } } -// 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() @@ -733,13 +595,9 @@ func TestReceiver_DryRunMakesNoFilesystemChanges(t *testing.T) { } } -// 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). +// Checks real rsync's documented guarantee that --itemize-changes output +// is identical between a dry run and a real run, comparing against a +// real run on a separate, equally fresh destination. func TestReceiver_DryRunItemizeMatchesRealRunItemize(t *testing.T) { srcRoot := t.TempDir() buildRichTree(t, srcRoot) @@ -766,10 +624,6 @@ func TestReceiver_DryRunItemizeMatchesRealRunItemize(t *testing.T) { 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) @@ -779,22 +633,12 @@ func TestReceiver_DryRunItemizeMatchesRealRunItemize(t *testing.T) { } } -// TestReceiver_AppliesHardLinksFromReceivedGroups exercises Receiver's -// hard-link handling directly against a hand-built file list, the same -// way TestReceiver_ConnectionDropsMidTransfer drives Receiver against a -// raw peer goroutine rather than a real Sender. This matters on a -// platform where sync.HardLinksSupported() is false (Windows): Receiver's -// own linking logic - skip a group's secondary members in the main loop, -// link them once their primary is written - has nothing to do with -// whether *this* platform's Sender could have detected the grouping, so -// it can and should be verified directly, standing in for whatever -// Sender a real Linux/macOS peer would be running. -// -// The peer goroutine responds to a signature request for "original.txt" -// only; if Receiver incorrectly asked for a signature for "linked.txt" -// too (i.e. failed to skip it as a secondary member), nothing would ever -// answer that second request and the test would time out rather than -// silently pass. +// TestReceiver_AppliesHardLinksFromReceivedGroups drives Receiver +// directly against a hand-built file list, so hard-link handling is +// verified even on a platform where Sender itself can't detect them +// (Windows). If Receiver failed to skip a secondary member, the peer +// goroutine below would never see its signature request and the test +// would time out rather than silently pass. func TestReceiver_AppliesHardLinksFromReceivedGroups(t *testing.T) { destRoot := t.TempDir() @@ -802,11 +646,8 @@ func TestReceiver_AppliesHardLinksFromReceivedGroups(t *testing.T) { receiverReadsFromPeer, peerWritesToReceiver := io.Pipe() receiver := pipeReadWriter{Reader: receiverReadsFromPeer, Writer: receiverWritesToPeer} - // Named so the alphabetical sort order sync.DetectHardLinks itself - // would produce is obvious at a glance: "aaa-primary.txt" sorts - // first, so it's group[0] - the member Receiver writes normally - - // and "zzz-secondary.txt" is the one that must be skipped and linked - // instead. + // aaa-primary.txt sorts first, so it's group[0] (written normally); + // zzz-secondary.txt must be skipped and linked instead. const content = "shared content, sent once for the whole group" entries := []sync.FileEntry{ {Path: "aaa-primary.txt", Mode: 0o644, Size: int64(len(content))}, @@ -904,11 +745,6 @@ func TestReceiver_AppliesHardLinksFromReceivedGroups(t *testing.T) { } } -// TestSender_ConnectionDropsMidTransfer confirms a dropped connection -// produces a prompt, clear error - not a hang and not a silently -// swallowed failure. The "receiver" here reads the file list (so Sender -// gets past that point) and then closes its side without ever sending a -// signature, simulating a connection that dies mid-transfer. func TestSender_ConnectionDropsMidTransfer(t *testing.T) { srcRoot := t.TempDir() mustWriteFile(t, filepath.Join(srcRoot, "file.txt"), "content that will never get a delta exchanged for it") @@ -918,10 +754,7 @@ func TestSender_ConnectionDropsMidTransfer(t *testing.T) { sender := pipeReadWriter{Reader: senderReadsFromPeer, Writer: senderWritesToPeer} go func() { - // Read (and discard) exactly the file list frame, then vanish - - // close both pipe halves without ever sending a signature back, - // simulating a connection that dies immediately after the initial - // exchange. + // Read the file list, then vanish without ever sending a signature. _, _ = transport.ReadFrame(peerReadsFromSender) _ = peerWritesToSender.Close() _ = peerReadsFromSender.Close() @@ -942,10 +775,6 @@ func TestSender_ConnectionDropsMidTransfer(t *testing.T) { } } -// TestReceiver_ConnectionDropsMidTransfer is TestSender_ConnectionDropsMidTransfer's -// counterpart for the other direction: the "sender" here sends a valid -// file list, then vanishes without ever responding to the signature -// Receiver sends back. func TestReceiver_ConnectionDropsMidTransfer(t *testing.T) { destRoot := t.TempDir() @@ -955,9 +784,7 @@ func TestReceiver_ConnectionDropsMidTransfer(t *testing.T) { go func() { _ = sendFileList(peerWritesToReceiver, []sync.FileEntry{{Path: "file.txt", Mode: 0o644}}, nil) - // Read (and discard) the signature Receiver sends back for - // file.txt, then vanish - closing both pipe halves without ever - // sending a delta. + // Read the signature, then vanish without ever sending a delta. _, _ = transport.ReadFrame(peerReadsFromReceiver) _ = peerWritesToReceiver.Close() _ = peerReadsFromReceiver.Close() diff --git a/internal/pipeline/progress.go b/internal/pipeline/progress.go index 199002b..4eb165e 100644 --- a/internal/pipeline/progress.go +++ b/internal/pipeline/progress.go @@ -6,25 +6,12 @@ import ( "time" ) -// progressWriteChunkSize is how large each disk-write chunk is when -// progress reporting is enabled: small enough to give real, visible -// granularity for a large file's write-to-disk phase, large enough not -// to turn an ordinary-sized file's write into thousands of syscalls. -// -// This is real, honest progress for exactly what it measures - bytes -// committed to disk for the current file - and no more: grsync's wire -// protocol delivers a whole file's delta as a single frame (see -// sync.ApplyDelta's own doc comment: "no streaming I/O anywhere yet"), -// so there is no point during network transfer where partial bytes are -// actually observable the way real rsync's own streaming protocol -// allows. Reporting progress against the disk-write phase instead of an -// imagined network stream is a deliberate, disclosed scope boundary - -// see the README's Progress and Stats section - not an approximation of -// something grsync doesn't actually do. +// progressWriteChunkSize is the disk-write chunk size used for progress +// reporting. Progress tracks disk-write bytes, not network bytes, since a +// whole file's delta arrives as a single frame with no partial point. const progressWriteChunkSize = 256 * 1024 -// progressUpdate is one snapshot of a single file's write-to-disk -// progress, sent non-blockingly to the formatting goroutine. +// progressUpdate is one snapshot of a single file's write-to-disk progress. type progressUpdate struct { path string bytesDone int64 @@ -35,29 +22,16 @@ type progressUpdate struct { filesLeft int // entries not yet processed after this one, matching real rsync's "to-chk" } -// progressReporter formats and prints progressUpdates on its own -// goroutine, so the transfer loop never blocks on however slow (or -// entirely absent) the output writer is. -// -// report's send is always non-blocking (a full channel silently drops -// the update, including a file's very last one) rather than switching to -// a blocking send for updates judged "too important to drop": a -// consistent rule is simpler to reason about and test than one with a -// special case, and a dropped 100%-complete line is a cosmetic gap, not -// a correctness one - the transfer itself, and the itemize/stats -// reporting that runs independently of this goroutine, are entirely -// unaffected either way. +// progressReporter formats and prints progressUpdates on its own goroutine, +// so the transfer loop never blocks on the output writer. type progressReporter struct { updates chan progressUpdate done chan struct{} start time.Time } -// newProgressReporter starts the formatting goroutine immediately; -// callers must call stop() exactly once (typically via defer, right -// after construction) so it's guaranteed to exit even on an early-error -// return - the self-review requirement this exists to satisfy is "no -// goroutine leaks," not just "usually cleans up." +// newProgressReporter starts the formatting goroutine; callers must call +// stop() exactly once (typically via defer) to avoid leaking it. func newProgressReporter(output io.Writer) *progressReporter { pr := &progressReporter{ updates: make(chan progressUpdate, 8), @@ -75,10 +49,8 @@ func (pr *progressReporter) run(output io.Writer) { } } -// report sends u to the formatting goroutine without blocking. See the -// type's own doc comment for why every update, including a file's last, -// uses the same non-blocking send rather than a blocking one for -// "important" updates. +// report sends u to the formatting goroutine, dropping it instead of +// blocking the transfer if the channel is full. func (pr *progressReporter) report(u progressUpdate) { select { case pr.updates <- u: @@ -86,33 +58,20 @@ func (pr *progressReporter) report(u progressUpdate) { } } -// stop closes the update channel and waits for the goroutine to drain -// and exit. Safe to call at most once; Receiver only ever constructs one -// progressReporter per call and defers stop() immediately, so this is -// never at risk of a double-close. +// stop closes the update channel and waits for the goroutine to drain and +// exit. Must be called at most once. func (pr *progressReporter) stop() { close(pr.updates) <-pr.done } -// formatProgressLine renders u in real rsync's own --progress format -// (verified against rsync.1's --progress section): while a file is -// still transferring, " % /s \n" with -// raw (non-comma-grouped) byte counts, matching the man page's own -// shown example ("782448 63% 110.64kB/s 0:00:04"); on that file's -// final update, real rsync instead prints a completion summary line - -// " 100% /s (xfr#N, to-chk=M/T)" - with -// comma-grouped bytes, matching its own shown example -// ("1,238,099 100% 146.38kB/s 0:00:08 (xfr#5, to-chk=169/396)"). +// formatProgressLine renders u in real rsync's --progress format: a live +// " % /s " line while transferring, then a +// comma-grouped " 100% /s (xfr#N, to-chk=M/T)" +// summary on the file's final update. // -// elapsed is this file's own transfer time (time since this -// progressReporter started - approximated as "since the whole Receiver -// call began" rather than "since this specific file began," since -// grsync doesn't currently track a per-file start time separately; for -// the common case of one file dominating a sync this is the same number -// either way, and for many small files it under-reports each -// individual file's own rate rather than over-reporting it - a -// disclosed simplification, not a hidden one). +// elapsed approximates per-file time as time since the whole Receiver call +// began, since grsync doesn't track each file's own start time. func formatProgressLine(u progressUpdate, elapsed time.Duration) string { percent := 0 if u.fileSize > 0 { @@ -139,10 +98,8 @@ func formatProgressLine(u progressUpdate, elapsed time.Duration) string { commaInt(u.bytesDone), formatRate(rate), formatDuration(elapsed), u.xferNum, u.filesLeft, u.totalFiles) } -// formatRate matches real rsync's own kB/MB/GB-per-second scaling -// (human_dnum): kilobytes (1000-based, matching rsync's own choice of -// decimal, not binary, units here) for anything under a megabyte/sec, -// megabytes beyond that. +// formatRate matches real rsync's kB/MB/GB-per-second scaling: decimal +// (1000-based) units, kB below 1MB/s and MB/GB beyond that. func formatRate(bytesPerSec float64) string { switch { case bytesPerSec >= 1e9: @@ -154,10 +111,8 @@ func formatRate(bytesPerSec float64) string { } } -// formatDuration matches real rsync's own h:mm:ss elapsed-time format -// exactly: both of the man page's own shown examples are "0:00:04" and -// "0:00:08" - hours always present (unpadded), minutes and seconds -// always zero-padded to 2 digits, even when hours is 0. +// formatDuration matches real rsync's h:mm:ss format: hours unpadded, +// minutes and seconds always zero-padded to 2 digits. func formatDuration(d time.Duration) string { if d < 0 { d = 0 diff --git a/internal/pipeline/progress_test.go b/internal/pipeline/progress_test.go index c7cd5be..3e26269 100644 --- a/internal/pipeline/progress_test.go +++ b/internal/pipeline/progress_test.go @@ -10,9 +10,7 @@ import ( "github.com/syntaxroot-cc/grsync/internal/sync" ) -// blockingWriter never returns from Write until the test closes unblock - -// standing in for a consumer (terminal, slow pipe) that has fallen -// arbitrarily far behind. +// blockingWriter never returns from Write until the test closes unblock. type blockingWriter struct { unblock chan struct{} } @@ -22,12 +20,6 @@ func (w *blockingWriter) Write(p []byte) (int, error) { return len(p), nil } -// TestProgressReporter_ReportDoesNotBlockOnSlowConsumer is the ticket's -// core concurrency proof: report() must return immediately regardless of -// how far behind the formatting goroutine has fallen, not just "usually" -// - here the goroutine is stuck inside Write for the test's entire -// duration, guaranteeing the buffered channel fills completely, and -// report() must still never block. func TestProgressReporter_ReportDoesNotBlockOnSlowConsumer(t *testing.T) { w := &blockingWriter{unblock: make(chan struct{})} pr := newProgressReporter(w) @@ -46,17 +38,10 @@ func TestProgressReporter_ReportDoesNotBlockOnSlowConsumer(t *testing.T) { t.Fatal("report() blocked despite a full channel and a stuck consumer - the non-blocking send is broken") } - close(w.unblock) // release the goroutine's pending Write so stop() below can complete + close(w.unblock) // let the stuck Write return, so stop() below can complete pr.stop() } -// TestProgressReporter_StopDoesNotLeakTheGoroutine is the self-review's -// explicit "no goroutine leak" requirement made concrete: stop() blocks -// until the formatting goroutine has actually exited (via <-pr.done), so -// this test completing at all - within the timeout - is itself the -// proof. If the goroutine leaked (stuck reading from a channel nobody -// closed, or blocked writing with nobody to unblock it), stop() would -// hang and this test would time out rather than silently "pass". func TestProgressReporter_StopDoesNotLeakTheGoroutine(t *testing.T) { var buf bytes.Buffer pr := newProgressReporter(&buf) @@ -75,11 +60,6 @@ func TestProgressReporter_StopDoesNotLeakTheGoroutine(t *testing.T) { } } -// TestProgressReporter_StopWithNoUpdatesSent confirms the empty case -// (construct, never report anything, stop) doesn't deadlock either - -// closing an empty channel and draining a range loop over it is exactly -// as safe as draining a non-empty one, but worth confirming directly -// rather than assuming. func TestProgressReporter_StopWithNoUpdatesSent(t *testing.T) { var buf bytes.Buffer pr := newProgressReporter(&buf) @@ -97,17 +77,11 @@ func TestProgressReporter_StopWithNoUpdatesSent(t *testing.T) { } } -// TestReceiver_ProgressFiresMultipleTimesForLargeFile confirms progress -// reporting genuinely chunks a large file's disk write - not just a -// single "done" line - by syncing a file well over -// progressWriteChunkSize and counting the resulting output lines. func TestReceiver_ProgressFiresMultipleTimesForLargeFile(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() - // 3+ chunks' worth (progressWriteChunkSize is 256KiB), so a correct - // implementation reports more than just a single completion line. - content := strings.Repeat("x", progressWriteChunkSize*3+1000) + content := strings.Repeat("x", progressWriteChunkSize*3+1000) // 3+ chunks' worth mustWriteFile(t, filepath.Join(srcRoot, "big.bin"), content) var out bytes.Buffer @@ -133,9 +107,6 @@ func TestReceiver_ProgressFiresMultipleTimesForLargeFile(t *testing.T) { assertSameContent(t, filepath.Join(srcRoot, "big.bin"), filepath.Join(destRoot, "big.bin")) } -// TestFormatDuration matches real rsync's own h:mm:ss format exactly - -// both of its man page's own shown examples ("0:00:04", "0:00:08") keep -// the hours component even when it's zero. func TestFormatDuration(t *testing.T) { tests := []struct { in time.Duration @@ -153,18 +124,12 @@ func TestFormatDuration(t *testing.T) { } } -// TestFormatProgressLine_MatchesRealRsyncExamples checks against the man -// page's own two shown lines almost verbatim: the in-progress line -// ("782448 63% 110.64kB/s 0:00:04") and the completion line -// ("1,238,099 100% 146.38kB/s 0:00:08 (xfr#5, to-chk=169/396)"). +// Checks against real rsync's man page examples: "782448 63% +// 110.64kB/s 0:00:04" and "1,238,099 100% 146.38kB/s 0:00:08 +// (xfr#5, to-chk=169/396)". Expected values below use elapsed=4s/8s +// instead of the man page's own, so rate/ETA are computed fresh rather +// than copied. func TestFormatProgressLine_MatchesRealRsyncExamples(t *testing.T) { - // bytesDone=782448 of fileSize=1238099 over elapsed=4s: percent = - // int(782448*100/1238099) = 63; rate = 782448/4 = 195612 B/s = - // "195.61kB"; eta = (1238099-782448)/195612 ≈ 2.33s, truncated to 2s - // = "0:00:02" - all computed from formatProgressLine's own real - // arithmetic, not copied from the man page's own (differently - // generated) example numbers, since reusing its byte count doesn't - // reproduce its exact rate/ETA too. inProgress := formatProgressLine( progressUpdate{bytesDone: 782448, fileSize: 1238099}, 4*time.Second, @@ -201,12 +166,6 @@ func TestFormatRate(t *testing.T) { } } -// TestReceiver_ProgressDoesNotFireDuringDryRun confirms the documented -// design boundary: Progress specifically measures bytes committed to -// disk (see progress.go's own doc comment), and dry-run skips that write -// entirely, so there is nothing to report - unlike Stats, which stays -// fully accurate in dry-run since none of its fields depend on the write -// actually happening. func TestReceiver_ProgressDoesNotFireDuringDryRun(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() diff --git a/internal/pipeline/receiver.go b/internal/pipeline/receiver.go index c344195..a05d611 100644 --- a/internal/pipeline/receiver.go +++ b/internal/pipeline/receiver.go @@ -12,19 +12,15 @@ import ( "github.com/syntaxroot-cc/grsync/internal/sync" ) -// receiveContext bundles the per-call state Receiver's helpers need -// beyond the read-only attrOpts/ropts every one of them already took: -// the optional stats accumulator, the optional progress reporter, and -// the running transfer-count bookkeeping progress's completion line -// needs (real rsync's own "xfr#N, to-chk=M/T"). Introduced here rather -// than adding yet more individual parameters to receiveDir/ -// receiveSymlink/receiveRegularFile's already-substantial signatures. +// receiveContext bundles the per-call state shared across Receiver's +// helpers: the optional stats accumulator, progress reporter, and +// transfer-count bookkeeping for progress's "xfr#N, to-chk=M/T" line. type receiveContext struct { attrOpts sync.AttrOptions ropts ReceiverOptions stats *Stats // nil unless ropts.Stats - progress *progressReporter // nil unless ropts.Progress (and never during DryRun - see writeToTempFileWithProgress) + progress *progressReporter // nil unless ropts.Progress (never during DryRun) totalFiles int // every entry in the received list, all types processed int // entries handled so far, for filesLeft = totalFiles - processed @@ -32,49 +28,26 @@ type receiveContext struct { } // Receiver runs the receiving side of a sync over rw: receives the -// sender's file list (with its hard-link grouping), then for each entry -// either creates it directly (directories, symlinks - both have -// everything they need already inside the FileEntry, no round trip -// required), exchanges a signature/delta with the sender (a regular file -// that is either unlinked or the first-seen member of a hard-link -// 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 attrOpts along the way. +// sender's file list, then for each entry either creates it directly +// (directories, symlinks), exchanges a signature/delta with the sender +// (a regular file), or - for a hard-link group's non-first member - is +// relinked to that first member in a dedicated pass afterward. 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, stats -// accumulation) still runs exactly as it would for a real transfer - see -// the individual receive* helpers below for the specific guarded calls, -// all of them audited: two os.MkdirAll calls and a -// writeToTempFileWithProgress+os.Rename pair (see partial.go) 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. ropts.Progress is the one exception that does *not* fire during -// a dry run: it specifically measures bytes committed to disk (see -// progress.go's own doc comment for why, given grsync's non-streaming -// wire protocol), and dry-run skips that write entirely, so there is -// nothing for it to report. ropts.Partial/PartialDir are likewise -// meaningless during a dry run: with no temp file ever created, there is -// nothing for either to keep or discard - see partial.go's own package -// doc comment for the full write-path design. +// ropts.DryRun turns every write into a no-op while all planning +// (signature/delta exchange, itemize comparison, stats) still runs, so +// reporting stays accurate. ropts.Progress is the one exception: it +// measures bytes actually committed to disk, so it never fires in a dry +// run. // -// 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.) +// Receiver only ever touches paths present in the sender's file list; +// there is no destination-side walk, so nothing here can delete or +// modify an unrelated file (--delete is out of scope). func Receiver(rw io.ReadWriter, dest string, attrOpts sync.AttrOptions, ropts ReceiverOptions) error { start := time.Now() - // Stats needs to know how many bytes crossed this connection; wrapping - // rw itself (rather than threading a length return value through every - // send/recv helper in messages.go) means Sender and its own tests need - // no changes at all for this - see countingReadWriter's own doc - // comment. + // Wrapping rw lets Stats count bytes crossing the connection without + // threading a length return through every send/recv helper. var counter *countingReadWriter if ropts.Stats { counter = &countingReadWriter{rw: rw} @@ -95,10 +68,9 @@ func Receiver(rw io.ReadWriter, dest string, attrOpts sync.AttrOptions, ropts Re defer ctx.progress.stop() } - // secondary marks every hard-link group member except the first: - // group[0] is written normally, like any other regular file, and - // every other member is linked to it in the dedicated pass below - // instead of being written out a second time. + // secondary marks every hard-link group member except the first, which + // is linked to it in the dedicated pass below instead of being written + // out a second time. secondary := make(map[string]bool) for _, group := range groups { for _, path := range group[1:] { @@ -106,22 +78,13 @@ func Receiver(rw io.ReadWriter, dest string, attrOpts sync.AttrOptions, ropts Re } } - // Directory attributes are deferred to a final pass below, applied - // deepest-first, rather than immediately when each directory is - // created: applying them immediately would have the filesystem - // silently re-bump a directory's mtime the moment something is later - // created inside it, undoing the very preservation just performed - - // or, for a read-only permission mode, block creating those children - // at all. Walk's own sort guarantees a parent directory's entry - // always precedes its children's in entries, so collecting them here - // in list order and processing that collection in reverse gives - // children-before-parents for free, without a second sort. The - // 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.) + // Directory attributes are deferred to a final pass, applied + // deepest-first: applying them as each directory is created would let + // a later child re-bump the parent's mtime, or a read-only mode block + // creating that child at all. entries is parent-before-child (Walk's + // sort), so reversing this collected list gives children-before-parents + // for free. The hard-link pass above runs first for the same reason: + // os.Link also touches the parent directory's mtime. var dirEntries []sync.FileEntry for _, entry := range entries { @@ -185,11 +148,8 @@ func Receiver(rw io.ReadWriter, dest string, attrOpts sync.AttrOptions, ropts Re 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. +// receiveDir creates one directory entry (unless dry-run) and reports its +// itemize line based on what existed at destPath before creation. func receiveDir(ctx *receiveContext, destPath string, entry sync.FileEntry) error { old, existed, err := lstatExisting(destPath) if err != nil { @@ -214,13 +174,9 @@ func receiveDir(ctx *receiveContext, destPath string, entry sync.FileEntry) erro 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 or count -// either - this short-circuit keeps that consistent rather than -// reporting or counting a change that sync.ApplyAttributes would never -// actually have made. +// receiveSymlink handles one symlink entry. Guarded on attrOpts.Links, +// matching sync.ApplyAttributes' own no-op behavior without --links, so +// nothing is written, reported, or counted either. func receiveSymlink(ctx *receiveContext, destPath string, entry sync.FileEntry) error { if !ctx.attrOpts.Links { return nil @@ -253,48 +209,26 @@ func receiveSymlink(ctx *receiveContext, destPath string, entry sync.FileEntry) } // 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, 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/stats output; only the final -// write (via a temp file - see partial.go - and sync.ApplyAttributes, and -// any progress reporting about either) is skipped. +// signature against destPath's current bytes, sends it, receives the +// sender's delta, and reconstructs the new content. This all runs even in +// dry-run mode, since it's exactly the work needed for accurate +// itemize/stats output; only the final write is skipped. // -// --append/--append-verify (SC-12) share the same two eligibility rules, -// verified against real rsync's own source (generator.c) rather than -// assumed: a file that doesn't exist at the destination yet is -// transferred completely normally - append semantics only ever apply to -// an existing, shorter file, matching real rsync's own documented "new -// files are transferred" rule. A destination that's already at least as -// long as the source is skipped entirely and unconditionally - not -// compared, not touched at all - matching real rsync's own documented -// behavior exactly (and generator.c's own append_mode-gated skip). -// -// For a genuinely shorter destination file, the two flags diverge -// exactly where real rsync's own source diverges (generate_and_send_sums: -// plain --append returns immediately after writing only a header, -// sending zero real block checksums; --append-verify falls through to -// the completely normal per-block signature loop): --append blindly -// trusts the existing prefix - no sync.GenerateSignature call for it at -// all, Sender is told (via appendTail) to send only the literal tail - -// while --append-verify runs the exact same sync.GenerateSignature/ -// GenerateDelta/ApplyDelta pipeline as an entirely normal sync, needing -// no new algorithm at all: the eligibility gate above is the only thing -// --append-verify actually adds on top of a vanilla transfer. +// --append/--append-verify apply only to an existing, shorter destination +// file; a new or already-as-long destination is handled (or skipped) +// exactly as without either flag. For an eligible file, --append blindly +// trusts the existing prefix and has Sender send only the literal tail; +// --append-verify instead runs the normal signature/delta/apply pipeline, +// verifying the prefix like any other sync. func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, entry sync.FileEntry) error { old, existed, err := lstatExisting(destPath) if err != nil { return fmt.Errorf("checking existing %q: %w", entry.Path, err) } - // usedPartialBasis is deliberately never true under append mode: - // --append/--append-verify are entirely about the REAL destination - // file's own existing bytes (trusted or verified), and mixing that - // with a --partial-dir staging file's content would blur two - // separately-reasoned-about features together for no real benefit - - // see partial.go's loadPartialBasis for the resume mechanism this - // skips here. + // usedPartialBasis is never true under append mode: append trusts or + // verifies the real destination file's own bytes, not a --partial-dir + // staging file's content. usedPartialBasis := false var oldData []byte if !ctx.ropts.AppendMode() { @@ -308,13 +242,9 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, return fmt.Errorf("reading existing %q: %w", entry.Path, err) } } - // oldData is nil when the file doesn't exist yet at the destination - // and no partial-dir file was found either (the new-file case). - // sync.GenerateSignature on nil/empty data naturally produces a - // Signature with zero Blocks, which makes sync.GenerateDelta emit a - // single all-DataOp delta (nothing to match against) - exactly the - // "new file" behavior needed, falling directly out of the existing - // SC-3 API with no special-casing required here. + // oldData is nil for a brand-new file; GenerateSignature on nil + // naturally yields a zero-block signature, and GenerateDelta a single + // all-DataOp delta, with no special-casing needed here. action := appendNone var sig sync.Signature @@ -322,18 +252,15 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, case !ctx.ropts.AppendMode() || !existed: sig = sync.GenerateSignature(oldData) case int64(len(oldData)) >= entry.Size: - // Append eligibility rule: skip entirely, don't even compute a - // real signature for potentially-huge existing data we're never - // going to consult - see this function's own doc comment. + // Destination already at least as long as source: skip without + // computing a signature for data we'll never consult. action = appendSkip case ctx.ropts.AppendVerify: sig = sync.GenerateSignature(oldData) default: // ctx.ropts.Append, and genuinely shorter than entry.Size action = appendTail - // BlockSize carries the trusted offset, not a real block size - - // see appendTail's own doc comment. Blocks is left empty: - // Sender never reads it for this action, since nothing here is - // verified at all. + // BlockSize carries the trusted append offset, not a real block + // size; Blocks is left empty since Sender never reads it here. sig = sync.Signature{BlockSize: len(oldData)} } @@ -351,14 +278,8 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, var newData []byte if action == appendSkip { - // Unconditional skip: real rsync's own documented rule for a - // destination that's already not shorter than the source is to - // leave it alone entirely, not to compare and possibly find it - // already matches byte for byte - so newData is oldData, - // verbatim, not the result of applying Sender's (empty) delta to - // it (which sync.ApplyDelta would otherwise reduce to nothing at - // all, since it never implicitly copies oldData forward on its - // own - see its own doc comment). + // Unconditional skip leaves the destination untouched: newData is + // oldData verbatim, not the result of applying an (empty) delta. newData = oldData } else { newData, err = sync.ApplyDelta(oldData, ops, sig) @@ -366,26 +287,11 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, 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). + // ApplyDelta is pure in-memory work, so computing it (and this + // comparison) is free even when dry-run means it's never written. contentChanged := action != appendSkip && !bytes.Equal(oldData, newData) - // transferred is deliberately not just contentChanged: a brand-new - // *empty* file has oldData == nil and newData == nil (ApplyDelta's - // accumulator is never appended to when there are no ops at all), so - // bytes.Equal(nil, nil) is true and contentChanged alone would miss - // it - even though creating a new file, empty or not, is exactly the - // kind of thing "Number of regular files transferred" is supposed to - // count. itemizeFile doesn't have this problem since it checks - // !existed as its own first, higher-priority branch (see its own - // code) - this mirrors that same priority for stats/xfer purposes. - // appendSkip is never "transferred" by definition - nothing was even - // compared, let alone changed. + // transferred isn't just contentChanged: a brand-new empty file has + // oldData == newData == nil, so contentChanged alone would miss it. transferred := action != appendSkip && (!existed || contentChanged) if ctx.stats != nil { @@ -404,13 +310,9 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, } if !ctx.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). + // MkdirAll is a no-op if the parent already exists; this covers + // paths whose parent wasn't itself part of the transferred list + // (e.g. dest itself, for a non-recursive sync). if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { return fmt.Errorf("creating parent directory for %q: %w", entry.Path, err) } @@ -422,12 +324,9 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, return fmt.Errorf("writing %q: %w", entry.Path, err) } } - // Applied regardless of transferred: real rsync's own documented - // --append behavior explicitly "does not interfere with the - // updating of a file's non-content attributes... when the file - // does not need to be transferred" - an appendSkip'd file (or - // any other untransferred-but-existing file) still gets its - // permissions/times/etc. brought in line if requested. + // Attributes are applied even when not transferred: real rsync + // still updates a file's non-content attributes when its content + // didn't need to change. if _, err := sync.ApplyAttributes(entry, destPath, ctx.attrOpts); err != nil { return fmt.Errorf("applying attributes to %q: %w", entry.Path, err) } @@ -438,12 +337,8 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, return nil } -// deltaByteCounts sums a delta's DataOp bytes (literal, unmatched data) -// and CopyOp bytes (matched, copied from the old file) - the same block- -// boundary math sync.ApplyDelta itself uses (including the final block -// potentially being shorter than blockSize), computed here rather than -// having ApplyDelta report it directly, since Stats is a pipeline-level -// concern ApplyDelta itself has no reason to know about. +// deltaByteCounts sums a delta's literal (DataOp) and matched (CopyOp) +// bytes, using the same block-boundary math as sync.ApplyDelta. func deltaByteCounts(ops []sync.DeltaOp, blockSize int, oldDataLen int) (literal, matched int64) { for _, op := range ops { switch o := op.(type) { @@ -463,11 +358,8 @@ func deltaByteCounts(ops []sync.DeltaOp, blockSize int, oldDataLen int) (literal return literal, matched } -// 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. +// lstatExisting is sync.LstatEntry with "not found" turned into a plain +// (zero value, false, nil) result instead of an error to unwrap. func lstatExisting(path string) (entry sync.FileEntry, existed bool, err error) { entry, err = sync.LstatEntry(path) if err != nil { diff --git a/internal/pipeline/sender.go b/internal/pipeline/sender.go index cf860fe..2234685 100644 --- a/internal/pipeline/sender.go +++ b/internal/pipeline/sender.go @@ -11,33 +11,19 @@ import ( ) // Sender runs the sending side of a sync over rw: walks and filters src, -// detects which entries are hard-linked to each other (only if -// hardLinks is true - see below), sends the resulting file list (with -// that grouping attached), then for each regular-file entry that isn't a -// secondary member of a hard-link group receives the receiver's -// signature, computes a delta against the current source bytes, and -// sends it back. +// detects hard links (if hardLinks is true), sends the resulting file +// list, then for each regular-file entry that isn't a hard-link group's +// secondary member, receives the receiver's signature, computes a delta +// against the current source bytes, and sends it back. // -// Directories and symlinks are deliberately not part of this exchange at -// all: a directory has no byte content to diff, and a symlink's entire -// "content" is its LinkTarget, which already travels inside the FileEntry -// in the file list itself. A hard-linked group's second-and-later members -// are skipped the same way, for a different reason: their data is -// byte-identical to the group's first member by definition (they're the -// same inode), so exchanging a signature/delta for them would just -// re-transfer bytes the receiver is about to get for free via -// sync.ApplyHardLinks instead. +// Directories and symlinks never exchange a signature/delta: a directory +// has no content to diff, and a symlink's content already travels as its +// LinkTarget in the file list. A hard-link group's secondary members are +// skipped too, since they're byte-identical to the first member by +// definition; the receiver relinks them instead via sync.ApplyHardLinks. // -// hardLinks mirrors real rsync's own -H/--hard-links flag: off by -// default, and deliberately not implied by --archive (real rsync's -a is -// -rlptgoD, no H) - detecting hard links means an extra Lstat per entry, -// a cost real rsync doesn't spend unless asked to, so grsync doesn't -// either. -// -// copts governs --compress/-z: entirely a sending-side decision (see -// CompressOptions' own doc comment) - Receiver needs no counterpart -// parameter at all, since decompression is driven purely by what each -// deltaMessage's own Compressed marker says, on every transport. +// hardLinks mirrors real rsync's -H flag: off by default and not implied +// by --archive, since detecting hard links costs an extra Lstat per entry. func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []sync.Rule, hardLinks bool, copts CompressOptions) error { entries, err := sync.Walk(src, walkOpts) if err != nil { @@ -45,11 +31,9 @@ func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []syn } entries = sync.FilterEntries(entries, rules) - // DetectHardLinks is skipped entirely, not just left to return no - // groups, in two cases: hardLinks wasn't requested, or the platform - // can't detect them at all (DetectHardLinks would always return - // nothing there anyway, but skipping the call also skips the - // Lstat-per-entry cost that would otherwise buy nothing). + // Skipped entirely (not just left to return nothing) when hardLinks is + // off or the platform can't detect them, avoiding a wasted + // Lstat-per-entry cost. var groups []sync.HardLinkGroup if hardLinks && sync.HardLinksSupported() { groups, err = sync.DetectHardLinks(src, entries) @@ -78,22 +62,15 @@ func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []syn if err != nil { return fmt.Errorf("receiving signature for %q: %w", entry.Path, err) } - // Both sides process the same file list in the same order, so - // this should never actually mismatch - but checking it costs - // nothing and turns a silent "wrong file's delta computed - // against the wrong signature" corruption bug into a clear, - // immediate error instead. + // Both sides process the same file list in the same order; this + // check turns a silent delta/signature mismatch into a clear error. if sigMsg.Path != entry.Path { return fmt.Errorf("signature arrived out of order: got %q, want %q", sigMsg.Path, entry.Path) } if sigMsg.Append == appendSkip { - // The receiver already decided this file doesn't need - // touching at all (--append/--append-verify's "destination - // not shorter than source" eligibility rule) - acknowledge - // with an empty delta without even opening the file, - // matching real rsync's own documented behavior of never - // comparing such files at all. + // The receiver already decided this file needs no comparison + // at all; acknowledge with an empty delta without opening it. if err := sendDelta(rw, entry.Path, nil, copts); err != nil { return fmt.Errorf("sending delta for %q: %w", entry.Path, err) } @@ -123,26 +100,14 @@ func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []syn return nil } -// appendTailOps builds the delta for --append: a single CopyOp -// representing "trust the receiver's first trustedLen bytes exactly as -// they are, without verifying them at all" - a legitimate, direct use of -// CopyOp's own documented contract (it only ever claims to copy a block -// unchanged, never that the block was checksum-verified; that -// verification is a property of how sync.GenerateDelta happens to find -// a match, not of CopyOp itself), followed by a DataOp for whatever of -// data comes after that offset - literal bytes the receiver has never -// seen. Verified against real rsync's own source (generator.c/sender.c) -// for the underlying "trust the prefix, send only the tail" behavior. +// appendTailOps builds the --append delta: a CopyOp{BlockIndex: 0} +// trusting the receiver's first trustedLen bytes unverified, followed by +// a DataOp for whatever of data comes after that offset. // -// A source file that has shrunk below trustedLen since the receiver -// last checked its own file's length (a narrow, real race - real -// rsync calls this a "diminished" file and skips it with a warning, -// continuing the rest of the transfer) is treated as a hard error here -// instead: grsync's Receiver has no general "skip this one file, keep -// going" mechanism anywhere else in the codebase, and inventing one -// solely for this narrow race is a bigger change than this ticket -// calls for - see the README's Partial and Append Transfers section for -// this disclosed scope difference. +// A source file that has shrunk below trustedLen (real rsync calls this a +// "diminished" file and skips it with a warning, continuing the transfer) +// is a hard error here instead: grsync has no general per-file +// skip-and-continue mechanism. func appendTailOps(trustedLen int, data []byte) ([]sync.DeltaOp, error) { if trustedLen > len(data) { return nil, fmt.Errorf("source file shrank to %d bytes, below the %d bytes already trusted on the receiving side (a \"diminished\" file - see real rsync's own --append docs)", len(data), trustedLen) diff --git a/internal/pipeline/ssh_test.go b/internal/pipeline/ssh_test.go index 38c5bbb..bac0add 100644 --- a/internal/pipeline/ssh_test.go +++ b/internal/pipeline/ssh_test.go @@ -13,13 +13,6 @@ import ( "github.com/syntaxroot-cc/grsync/internal/transport" ) -// requireLocalSSHServer and buildGrsyncBinary mirror -// internal/transport/integration_test.go's own helpers of the same name -// and purpose (a real SSH server capability probe, and building the real -// binary fresh) - duplicated rather than shared across packages, since Go -// test files aren't importable, and these are small enough that a shared -// test-support package would be more machinery than the two call sites -// justify. func requireLocalSSHServer(t *testing.T) { t.Helper() cmd := exec.Command("ssh", "-o", "BatchMode=yes", "-o", "ConnectTimeout=5", "127.0.0.1", "true") @@ -42,23 +35,9 @@ func buildGrsyncBinary(t *testing.T) string { return out } -// TestSSHLocalhost_SyncRoundTrip is this ticket's real, over-the-wire -// proof: it spawns the actual built grsync binary in --server mode via -// real ssh to 127.0.0.1 (not a mock, not an in-process pipe), runs the -// real Sender against that connection, and confirms the destination tree -// genuinely matches the source. -// -// The remote command here is the built binary's *full path*, not the bare -// "grsync" internal/cli's syncToRemote actually uses for a real -// invocation. That's a deliberate, documented difference for -// testability: a plain `go test` run has no "grsync" installed on the -// target's PATH to find (nothing was ever "installed" anywhere), so this -// test bypasses that PATH-resolution question entirely and points ssh -// directly at the freshly-built binary instead. internal/cli's real -// behavior - assuming "grsync" is on the remote PATH, exactly like real -// rsync assumes "rsync" is - is unit-tested elsewhere (BuildRSHCommand, -// syncToRemote's construction), just not exercised through an actual -// remote PATH lookup here. +// The remote command uses the built binary's full path rather than the +// bare "grsync" a real invocation would use, since nothing is installed +// on the test target's PATH. func TestSSHLocalhost_SyncRoundTrip(t *testing.T) { requireLocalSSHServer(t) grsyncPath := buildGrsyncBinary(t) @@ -100,13 +79,6 @@ func TestSSHLocalhost_SyncRoundTrip(t *testing.T) { 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) @@ -153,18 +125,9 @@ func TestSSHLocalhost_DryRunMakesNoChanges(t *testing.T) { } } -// TestSSHLocalhost_ProgressAndStatsDoNotBreakTheTransfer is the real, -// over-the-wire proof that --progress/--stats on the remote --server -// command line (exactly what internal/cli's syncToRemote adds for a real -// invocation - see its own doc comment) don't corrupt or interfere with -// the actual transfer. It does not attempt to capture and verify the -// remote process's own stderr text (session.go's live passthrough of -// it) - that would need OS-level stderr redirection in this test -// process, disproportionate complexity for what SC-11's own SSH tests -// already established isn't otherwise verified end to end (they check -// write-safety and transfer correctness, not stderr content); the -// destination matching the source byte-for-byte is what actually proves -// progress reporting's chunked write path didn't corrupt anything. +// Doesn't attempt to capture the remote process's stderr text; the +// destination matching the source byte-for-byte is what proves progress +// reporting's chunked write path didn't corrupt anything. func TestSSHLocalhost_ProgressAndStatsDoNotBreakTheTransfer(t *testing.T) { requireLocalSSHServer(t) grsyncPath := buildGrsyncBinary(t) @@ -204,15 +167,9 @@ func TestSSHLocalhost_ProgressAndStatsDoNotBreakTheTransfer(t *testing.T) { assertSameContent(t, filepath.Join(src, "big.bin"), filepath.Join(dest, "big.bin")) } -// TestSSHLocalhost_CompressDoesNotBreakTheTransfer is the real, -// over-the-wire proof that --compress/-z's client-side decision (see -// pipeline.CompressOptions' own doc comment - Sender runs locally here, -// so no remote --server argv change is needed at all, unlike --dry-run/ -// --itemize-changes/--verbose/--progress/--stats) doesn't corrupt or -// interfere with an actual transfer over real SSH: the remote --server -// process needs no compression-related flag on its own command line, -// since its Receiver just reacts to each deltaMessage's own Compressed -// marker. +// Unlike --dry-run/--verbose/--progress/--stats, --compress needs no +// remote --server argv flag: Receiver just reacts to each deltaMessage's +// own Compressed marker. func TestSSHLocalhost_CompressDoesNotBreakTheTransfer(t *testing.T) { requireLocalSSHServer(t) grsyncPath := buildGrsyncBinary(t) @@ -253,15 +210,6 @@ func TestSSHLocalhost_CompressDoesNotBreakTheTransfer(t *testing.T) { assertSameContent(t, filepath.Join(src, "big.txt"), filepath.Join(dest, "big.txt")) } -// TestSSHLocalhost_IPv4ForwardedToSSHDoesNotBreakTheTransfer is SC-14's -// real, over-the-wire proof for the SSH transport: transport.Dial's -// ipv4 parameter reaches transport.BuildRSHCommand, which inserts a real -// "-4" into the spawned ssh process's own argv (see BuildRSHCommand's -// own doc comment - grsync never dials this connection itself, ssh -// does) - this drives that real path end to end against a real local -// sshd and confirms the forwarded -4 doesn't break anything, connecting -// to 127.0.0.1 (a genuine IPv4 address, so ssh's own -4 has nothing to -// object to here). func TestSSHLocalhost_IPv4ForwardedToSSHDoesNotBreakTheTransfer(t *testing.T) { requireLocalSSHServer(t) grsyncPath := buildGrsyncBinary(t) @@ -300,16 +248,8 @@ func TestSSHLocalhost_IPv4ForwardedToSSHDoesNotBreakTheTransfer(t *testing.T) { assertSameContent(t, filepath.Join(src, "top.txt"), filepath.Join(dest, "top.txt")) } -// TestSSHLocalhost_AppendAndPartialDoNotBreakTheTransfer is SC-12's -// real, over-the-wire proof for the SSH transport: --append/--partial -// are forwarded to the remote --server process as ordinary argv flags -// (see cli.syncToRemote's own doc comment - the same mechanism SC-11 -// established for --dry-run/--itemize-changes), parsed there through -// the server's own normal flag handling, with no wire-protocol change -// needed at all. This drives that real path end to end against a real -// local sshd: the destination file is a genuine prefix of the source, so -// a real --append tail-only transfer actually happens, not just a -// harmless no-op. +// The destination file is a genuine prefix of the source, so a real +// --append tail-only transfer happens, not just a harmless no-op. func TestSSHLocalhost_AppendAndPartialDoNotBreakTheTransfer(t *testing.T) { requireLocalSSHServer(t) grsyncPath := buildGrsyncBinary(t) diff --git a/internal/pipeline/stats.go b/internal/pipeline/stats.go index d749a0c..85e674b 100644 --- a/internal/pipeline/stats.go +++ b/internal/pipeline/stats.go @@ -8,16 +8,9 @@ import ( "time" ) -// countingReadWriter wraps rw, counting every byte read and written - -// used to populate Stats.BytesSent/BytesReceived from the actual bytes -// crossing this connection (frame headers included, not just gob -// payloads), the same quantity real rsync's own "Total bytes sent"/ -// "Total bytes received" measure, just over grsync's own gob wire -// protocol instead of rsync's real one (see messages.go's encoding -// note). Wrapping the connection itself, rather than threading a length -// return value through every send/recv helper in messages.go, means -// Sender and its own tests need no changes at all for this - only -// Receiver ever needs to know these counts. +// countingReadWriter wraps rw, counting every byte read and written, to +// populate Stats.BytesSent/BytesReceived from what actually crosses this +// connection (frame headers included, not just gob payloads). type countingReadWriter struct { rw io.ReadWriter read int64 @@ -38,13 +31,8 @@ func (c *countingReadWriter) Write(p []byte) (int, error) { // Stats accumulates statistics over one Receiver call, mirroring real // rsync's own --stats fields where grsync can compute them accurately. -// Fields real rsync reports that grsync deliberately omits - Number of -// deleted files (no --delete), File list size/generation-time/transfer- -// time (not measured anywhere in this architecture), and ACL/xattr/ -// device/special counts (not tracked, the same scope boundary SC-11's -// itemize format already established) - are left out entirely rather -// than reported as a misleading zero; see the README's Progress and -// Stats section for the full list. +// Fields grsync can't measure (deleted files, ACL/xattr/device counts, +// etc.) are omitted entirely rather than reported as a misleading zero. type Stats struct { RegularFiles int Directories int @@ -55,22 +43,17 @@ type Stats struct { CreatedSymlinks int // RegularFilesTransferred is how many regular files actually had - // different content and were rewritten - the same contentChanged - // bit itemizeFile already computes for its Y code, reused here - // rather than recomputed. + // different content and were rewritten. RegularFilesTransferred int // TotalFileSize is the sum of Size across every regular-file entry - // considered (matching real rsync's own "does not count directories - // or special files, but does include symlinks" - except grsync - // scopes this to regular files only, since symlink "size" isn't a - // meaningful transferred-bytes quantity in this pipeline). + // considered; unlike real rsync, this excludes symlinks, since + // symlink "size" isn't a meaningful transferred-bytes quantity here. TotalFileSize int64 TotalTransferredFileSize int64 - // LiteralData and MatchedData are computed by inspecting the - // DataOp/CopyOp list each regular file's delta already produces - - // no new information needs to cross the wire for this. + // LiteralData and MatchedData come from each regular file's delta + // DataOp/CopyOp list. LiteralData int64 MatchedData int64 @@ -90,13 +73,8 @@ func (s Stats) NumCreatedFiles() int { return s.CreatedRegularFiles + s.CreatedDirectories + s.CreatedSymlinks } -// SpeedupRatio is real rsync's own formula, verified against its actual -// source (main.c's output_summary, not just the man page's looser -// prose): total file size divided by the sum of bytes sent and -// received. Real rsync's own comment calls this "really just a -// feel-good bigger-is-better number," not a rigorous metric - -// reproduced here exactly as-is, caveat included, rather than a -// differently-reasoned formula that happens to also produce a ratio. +// SpeedupRatio is real rsync's own formula: total file size divided by +// the sum of bytes sent and received. func (s Stats) SpeedupRatio() float64 { total := s.BytesSent + s.BytesReceived if total == 0 { @@ -106,10 +84,7 @@ func (s Stats) SpeedupRatio() float64 { } // BytesPerSecond is total bytes transferred divided by elapsed wall-clock -// time, matching real rsync's own bytes_per_sec_human_dnum(). Zero -// elapsed time (a sync fast enough that no measurable time passed, or a -// synthetic test driving Receiver directly without going through the -// real clock) reports 0 rather than dividing by zero. +// time. Zero elapsed time reports 0 rather than dividing by zero. func (s Stats) BytesPerSecond() float64 { if s.Elapsed <= 0 { return 0 @@ -118,10 +93,8 @@ func (s Stats) BytesPerSecond() float64 { } // commaInt formats n with comma thousands separators, matching real -// rsync's own comma_num()/human_num() output for --stats byte counts. -// (--progress's own live line, in contrast, shows raw ungrouped digits - -// see formatProgressLine - matching the different function real rsync -// itself uses there.) +// rsync's --stats byte counts (--progress's live line shows raw ungrouped +// digits instead - see formatProgressLine). func commaInt(n int64) string { s := strconv.FormatInt(n, 10) neg := strings.HasPrefix(s, "-") @@ -142,8 +115,7 @@ func commaInt(n int64) string { } // commaFloat2 formats f with comma thousands separators and exactly 2 -// decimal places, matching real rsync's own comma_dnum(f, 2) - used for -// the speedup ratio and the bytes/sec rate in --stats output. +// decimal places, matching real rsync's comma_dnum(f, 2). func commaFloat2(f float64) string { neg := f < 0 if neg { @@ -162,11 +134,8 @@ func commaFloat2(f float64) string { return fmt.Sprintf("%s%s.%02d", sign, commaInt(whole), frac) } -// typeBreakdown is real rsync's own "(reg: R, dir: D, link: L)" suffix, -// omitting any type whose count is zero entirely - matching the man -// page's own documented rule ("If any value is 0, it is completely -// omitted from the list"), which also naturally hides grsync's lack of -// device/special support rather than needing a separate carve-out for it. +// typeBreakdown is real rsync's "(reg: R, dir: D, link: L)" suffix, +// omitting any type whose count is zero. func typeBreakdown(reg, dir, link int) string { var parts []string if reg > 0 { @@ -184,13 +153,10 @@ func typeBreakdown(reg, dir, link int) string { return " (" + strings.Join(parts, ", ") + ")" } -// formatStats renders s in real rsync's own --stats structure (verified -// against main.c's output_summary): a detailed field-by-field block, -// then the shorter sent/received/rate and total-size/speedup summary -// every --stats run ends with regardless of verbosity. dryRun appends -// real rsync's own "(DRY RUN)" suffix to the speedup line when set, -// reusing a real, already-verified rsync convention rather than -// inventing a different way to flag the same thing. +// formatStats renders s in real rsync's --stats structure: a detailed +// field-by-field block, then the sent/received/rate and +// total-size/speedup summary. dryRun appends real rsync's own +// "(DRY RUN)" suffix to the speedup line. func formatStats(s Stats, dryRun bool) string { var b strings.Builder diff --git a/internal/pipeline/stats_test.go b/internal/pipeline/stats_test.go index 3d38c90..1143d5c 100644 --- a/internal/pipeline/stats_test.go +++ b/internal/pipeline/stats_test.go @@ -12,8 +12,6 @@ import ( ) func TestSpeedupRatio_MatchesRealRsyncFormula(t *testing.T) { - // Verified against real rsync's own source (main.c's output_summary): - // total_size / (total_written + total_read). s := Stats{TotalFileSize: 1000, BytesSent: 100, BytesReceived: 150} got := s.SpeedupRatio() want := 1000.0 / (100.0 + 150.0) @@ -82,26 +80,14 @@ func statsField(t *testing.T, output, label string) int64 { return n } -// TestReceiver_StatsAccuracy is SC-10's core accuracy proof: a small -// tree where every number is exactly predictable by construction (not -// approximated) - two brand-new regular files (entirely literal data, -// no old data to match against), one byte-identical existing file -// (entirely matched data, zero literal), and one new directory - checked -// against the actual printed --stats output, not the internal Stats -// struct directly, so this proves the real end-user-visible behavior, -// not just the accumulation logic in isolation. func TestReceiver_StatsAccuracy(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() const newContent = "0123456789" // 10 bytes, brand new - // unchangedContent is deliberately 2 full blocks (sync.DefaultBlockSize - // is 700): GenerateDelta can only ever produce a CopyOp for a window - // at least one full block long (see delta.go's own "fewer than - // blockSize bytes remain" comment), so a short "identical" file - one - // shorter than a single block - would always transfer as 100% literal - // data regardless of whether it actually matches, proving nothing - // about Matched data specifically. + // 2 full blocks (sync.DefaultBlockSize is 700): GenerateDelta can only + // produce a CopyOp for a window at least one full block long, so a + // shorter "identical" file would always transfer as literal data. unchangedContent := strings.Repeat("ABCDEFGHIJ", 140) // 1400 bytes, byte-identical at both ends const nestedContent = "nested" // 6 bytes, brand new, inside a brand-new directory @@ -142,9 +128,6 @@ func TestReceiver_StatsAccuracy(t *testing.T) { t.Errorf("Total transferred file size = %d, want %d", got, wantTransferredSize) } - // new.txt and nested.txt are entirely literal (no old data existed to - // match against); unchanged.txt is entirely matched (byte-identical), - // contributing 0 literal and its full size to matched. wantLiteral := int64(len(newContent) + len(nestedContent)) if got := statsField(t, output, "Literal data"); got != wantLiteral { t.Errorf("Literal data = %d, want %d", got, wantLiteral) @@ -160,21 +143,15 @@ func TestReceiver_StatsAccuracy(t *testing.T) { t.Errorf("Total bytes sent/received = %d/%d, want both > 0", sent, received) } - // The speedup line is self-consistent with the formula and the - // actual sent/received counts already verified above - not an - // independently hard-coded expectation, which would just duplicate - // (and risk silently drifting from) the real byte counts gob - // encoding happens to produce. + // Self-consistent with the formula and the sent/received counts + // already verified above, rather than an independently hard-coded + // expectation. wantSpeedup := commaFloat2(float64(wantTotalSize) / float64(sent+received)) if !bytes.Contains(out.Bytes(), []byte("speedup is "+wantSpeedup)) { t.Errorf("output does not contain expected speedup %q:\n%s", wantSpeedup, output) } } -// TestReceiver_StatsOmitsDeletedFilesLine confirms real rsync's own -// documented rule is followed: "Number of deleted files" is only -// printed when deletions are in effect - grsync never implements -// --delete at all (see the README), so this line must never appear. func TestReceiver_StatsOmitsDeletedFilesLine(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -190,13 +167,6 @@ func TestReceiver_StatsOmitsDeletedFilesLine(t *testing.T) { } } -// TestReceiver_StatsCountsNewEmptyFileAsTransferred is a deliberate edge -// case: a brand-new *empty* file has oldData == nil and newData == nil -// (ApplyDelta's accumulator is never appended to when there are no -// delta ops at all), so a naive bytes.Equal(oldData, newData) check -// alone would say "unchanged" even though a real file was just created - -// this is exactly the gap receiveRegularFile's transferred variable -// (distinct from contentChanged) exists to close. func TestReceiver_StatsCountsNewEmptyFileAsTransferred(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() @@ -215,12 +185,6 @@ func TestReceiver_StatsCountsNewEmptyFileAsTransferred(t *testing.T) { } } -// TestReceiver_StatsWorksInDryRun confirms Stats is fully dry-run -// compatible, unlike Progress: every field it reports (file counts, -// sizes, literal/matched data, even bytes sent/received) is derived -// from planning data and the wire exchange, neither of which dry-run -// skips - only the final disk write is skipped, and Stats doesn't -// depend on that. func TestReceiver_StatsWorksInDryRun(t *testing.T) { srcRoot := t.TempDir() destRoot := t.TempDir() diff --git a/internal/sync/attributes.go b/internal/sync/attributes.go index 7d52769..b3e8e02 100644 --- a/internal/sync/attributes.go +++ b/internal/sync/attributes.go @@ -9,58 +9,28 @@ import ( // applyPerms applies entry's permission bits to path via os.Chmod. // -// mode is deliberately fs.FileMode.Perm() here, not the raw FileEntry.Mode: -// FileMode packs type bits (fs.ModeDir, fs.ModeSymlink, etc.) into its high -// bits alongside the low 9 permission bits, and os.Chmod on most platforms -// ignores or rejects those type bits rather than erroring - passing the raw -// value through would either silently do nothing useful with the type -// information or produce a confusing partial result. Perm() strips -// everything except the permission bits, which is all Chmod can act on -// anyway. +// mode.Perm() strips FileMode's type bits (fs.ModeDir, fs.ModeSymlink, ...) +// before the call, since os.Chmod only understands the low 9 permission bits. func applyPerms(path string, mode fs.FileMode) error { return os.Chmod(path, mode.Perm()) } -// applyTimes applies modTime to path as both the access and modification -// time via os.Chtimes. +// applyTimes applies modTime to path as both access and modification time via os.Chtimes. // -// os.Chtimes has no way to change only mtime and leave atime alone - -// POSIX utimes() always sets both together - so some value has to be -// chosen for atime. rsync itself doesn't meaningfully preserve atime as -// part of -a/--archive either. Using modTime for atime too, rather than -// time.Now(), keeps this idempotent: reapplying the same FileEntry's -// attributes a second time (e.g. re-running a sync that changed nothing) -// produces byte-identical timestamps both times, instead of atime -// drifting to a new "now" on every run. +// os.Chtimes can't set mtime without also setting atime (POSIX utimes sets +// both together). Using modTime for atime too keeps repeated syncs +// idempotent, instead of atime drifting to a new "now" on every run. func applyTimes(path string, modTime time.Time) error { return os.Chtimes(path, modTime, modTime) } -// applyOwnership applies entry's UID and/or GID to path via os.Lchown - -// not os.Chown - so that when entry is a symlink, ownership is set on the -// link itself rather than whatever it points at (matching Lstat's own -// convention elsewhere in this package). +// applyOwnership applies entry's UID/GID to path via os.Lchown, so that on a +// symlink ownership is set on the link itself rather than its target. // -// applyOwner and applyGroup are independent switches (mirroring the -// separate --owner/--group flags): os.Lchown treats a uid or gid of -1 as -// "leave this one unchanged", so requesting only one of the two still -// takes a single syscall rather than needing two different code paths. -// -// If neither applyOwner nor applyGroup is requested, this is a no-op: -// (false, false, nil). If they are requested but entry.OwnershipAvailable -// is false, Lchown is deliberately never called - applying UID/GID 0 -// would silently claim "owned by root", which is not what an unavailable -// value means (see FileEntry.OwnershipAvailable's doc comment). That case -// is reported back as skipped=true rather than left indistinguishable -// from "successfully applied", so a caller can surface it instead of it -// disappearing into a bare nil error. -// -// Operational note: changing ownership to an arbitrary uid/gid is a -// privileged operation on most POSIX systems (requires root / CAP_CHOWN). -// Expect Lchown to return a permission error when grsync runs unprivileged -// against files not already owned by the current user - that's a -// constraint on how grsync must be run, not something this function -// tries to work around. +// applyOwner and applyGroup are independent: Lchown treats a uid/gid of -1 as +// "leave unchanged". If entry.OwnershipAvailable is false, Lchown is never +// called - applying UID/GID 0 would wrongly claim "owned by root" - and +// skipped=true is returned instead of a silent no-op. func applyOwnership(path string, entry FileEntry, applyOwner, applyGroup bool) (applied, skipped bool, err error) { if !applyOwner && !applyGroup { return false, false, nil @@ -82,21 +52,12 @@ func applyOwnership(path string, entry FileEntry, applyOwner, applyGroup bool) ( return true, false, nil } -// applySymlink creates destPath as a symlink pointing at entry.LinkTarget, -// rather than following the link and copying whatever it points to - the -// entire point of preserving a symlink as a symlink. +// applySymlink creates destPath as a symlink pointing at entry.LinkTarget. // -// Unlike Path, LinkTarget is passed through exactly as Walk captured it -// (via os.Readlink) - not slash-normalized. That's deliberate, not an -// oversight: Path is always a relative path Walk itself constructs, so -// normalizing it to "/" is safe and purely internal. LinkTarget is -// arbitrary data authored by whatever created the original symlink - it -// may be an absolute path pointing outside the transfer tree entirely, on -// POSIX it may even legally contain a literal backslash (a normal -// filename character there, not a separator), and real rsync itself -// treats symlink targets as opaque strings rather than rewriting them. -// Slash-normalizing it could corrupt a target that was never meant to be -// interpreted as portable path syntax. +// Unlike Path, LinkTarget is passed through exactly as Walk captured it, not +// slash-normalized: it's opaque data from whatever created the original +// symlink, may be absolute or point outside the transfer tree, and on POSIX +// may legally contain a literal backslash (not a separator there). func applySymlink(destPath string, entry FileEntry) error { if entry.Mode&fs.ModeSymlink == 0 { return fmt.Errorf("entry %q is not a symlink (Mode=%v)", entry.Path, entry.Mode) @@ -105,11 +66,8 @@ func applySymlink(destPath string, entry FileEntry) error { return fmt.Errorf("entry %q is a symlink but has no LinkTarget", entry.Path) } - // os.Symlink fails with "file exists" if destPath is already - // occupied, which is the normal case when re-applying attributes (or - // re-running a sync) against a destination that already has a prior - // version of this entry. Remove whatever's there first; a missing - // path is not an error here. + // os.Symlink fails with "file exists" if destPath is already occupied + // (e.g. re-running a sync); remove any existing entry first. if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("removing existing entry at %q: %w", destPath, err) } @@ -117,22 +75,14 @@ func applySymlink(destPath string, entry FileEntry) error { return os.Symlink(entry.LinkTarget, destPath) } -// AttrOptions selects which attribute categories ApplyAttributes should -// preserve, mirroring the CLI's --perms/--times/--owner/--group/--links/ -// --hard-links/--devices flags so each can be toggled independently, the -// same way --archive bundles several of these together while individual -// flags still control them separately. +// AttrOptions selects which attribute categories ApplyAttributes preserves, +// mirroring the CLI's --perms/--times/--owner/--group/--links/--hard-links/ +// --devices flags. // -// HardLinks and Devices are included here for a complete, consistent -// shape matching all seven flags, but ApplyAttributes itself does not act -// on them: both are inherently multi-entry operations (a hard link only -// means something in relation to at least one other FileEntry; a device -// file needs privileged recreation handled separately - see -// DetectHardLinks/ApplyHardLinks in hardlinks.go and ApplySpecialFile in -// specialfiles.go). A caller orchestrating a full sync consults -// opts.HardLinks/opts.Devices itself to decide whether to invoke those -// separately; ApplyAttributes silently ignoring them here would be -// misleading, so this is called out explicitly rather than left implicit. +// HardLinks and Devices are included for a complete shape matching all seven +// flags, but ApplyAttributes does not act on them - both are multi-entry +// operations handled separately (see DetectHardLinks/ApplyHardLinks in +// hardlinks.go and ApplySpecialFile in specialfiles.go). type AttrOptions struct { Perms bool Times bool @@ -143,10 +93,8 @@ type AttrOptions struct { Devices bool } -// AttrResult reports what ApplyAttributes actually did, distinguishing -// "applied" from "explicitly skipped" so a caller can surface a skip -// (e.g. "ownership unavailable on this platform") instead of it -// disappearing silently into a bare nil error. +// AttrResult reports what ApplyAttributes did, distinguishing "applied" from +// "explicitly skipped" (e.g. ownership unavailable) so a caller can surface it. type AttrResult struct { PermsApplied bool TimesApplied bool @@ -155,21 +103,15 @@ type AttrResult struct { LinkApplied bool } -// ApplyAttributes applies the attribute categories opts selects to -// destPath, using entry's metadata (as captured by Walk). destPath must -// already exist as the right kind of filesystem entry for non-symlink -// attributes (e.g. already written by ApplyDelta for a regular file); -// for a symlink entry with opts.Links set, ApplyAttributes creates the -// symlink itself, since a symlink has no separate byte-content delta step. +// ApplyAttributes applies the attribute categories opts selects to destPath, +// using entry's metadata as captured by Walk. destPath must already exist as +// the right kind of filesystem entry for non-symlink attributes; for a +// symlink entry with opts.Links set, ApplyAttributes creates the symlink itself. // -// For a symlink entry, Perms and Times are silently not applied even if -// requested: os.Chmod and os.Chtimes both follow symlinks (Go's standard -// library has no portable Lchmod/Lchtimes), so calling them on a symlink -// path would either modify whatever the link points to - not the link -// itself - or error outright if that target doesn't exist yet. Ownership -// is the exception: os.Lchown (used by applyOwnership) correctly targets -// the symlink itself, matching Lstat's own convention, so Owner/Group are -// still applied normally. +// For a symlink, Perms and Times are silently skipped even if requested: +// os.Chmod/os.Chtimes follow symlinks (Go has no portable Lchmod/Lchtimes), +// so calling them would affect the target, not the link. Ownership is the +// exception since os.Lchown targets the link itself. func ApplyAttributes(entry FileEntry, destPath string, opts AttrOptions) (AttrResult, error) { var result AttrResult diff --git a/internal/sync/attributes_test.go b/internal/sync/attributes_test.go index 25e4027..5b1b417 100644 --- a/internal/sync/attributes_test.go +++ b/internal/sync/attributes_test.go @@ -8,16 +8,11 @@ import ( "time" ) -// wantPerm returns the permission bits Stat should actually report after -// applyPerms(path, mode) on the current platform. On POSIX this is just -// mode.Perm() - full fidelity. On Windows, os.Chmod can only toggle the -// read-only attribute: any owner-write bit (0200) present makes the file -// fully writable (reported as 0666), and its absence makes it read-only -// (reported as 0444) - there's no way to represent finer-grained POSIX -// permissions through the Win32 attribute model Go's os.Chmod uses there. -// This is a real, verified platform difference (confirmed by running the -// naive POSIX-only assertion here and watching it fail with 666 instead -// of the requested 600), not a gap in applyPerms itself. +// wantPerm returns the permission bits Stat should report after +// applyPerms(path, mode) on the current platform. On Windows, os.Chmod can +// only toggle the read-only attribute: any owner-write bit makes the file +// fully writable (0666), and its absence makes it read-only (0444) - there's +// no way to represent finer-grained POSIX permissions there. func wantPerm(mode os.FileMode) os.FileMode { if runtime.GOOS != "windows" { return mode.Perm() @@ -55,11 +50,6 @@ func TestApplyPerms_IgnoresTypeBits(t *testing.T) { t.Fatalf("WriteFile: %v", err) } - // A FileEntry.Mode for a regular file with 0755 perms, as Lstat would - // actually report it (no high type bits set for a plain file - but - // this proves applyPerms takes a plain fs.FileMode and behaves - // correctly on a value that's already realistic, not just a bare - // octal literal). if err := applyPerms(path, os.FileMode(0o755)); err != nil { t.Fatalf("applyPerms returned error: %v", err) } @@ -74,17 +64,13 @@ func TestApplyPerms_IgnoresTypeBits(t *testing.T) { } func TestApplyPerms_ReadOnlyOnWindowsWhenNoWriteBit(t *testing.T) { - // Directly exercises the read-only side of Windows' reduced-fidelity - // Chmod, rather than only ever testing modes that happen to include - // a write bit. dir := t.TempDir() path := filepath.Join(dir, "file.txt") if err := os.WriteFile(path, []byte("hello"), 0o644); err != nil { t.Fatalf("WriteFile: %v", err) } t.Cleanup(func() { - // Best-effort: restore write permission so t.TempDir()'s own - // cleanup can actually remove this file afterward. + // Restore write permission so t.TempDir()'s cleanup can remove this file. if err := os.Chmod(path, 0o644); err != nil { t.Logf("cleanup: failed to restore writable permissions on %s: %v", path, err) } @@ -110,11 +96,8 @@ func TestApplyTimes(t *testing.T) { t.Fatalf("WriteFile: %v", err) } - // Truncated to whole seconds: some filesystems (notably FAT variants, - // and historically some others) don't store sub-second mtime - // precision, so a nanosecond-precision want value could produce a - // flaky round-trip mismatch that has nothing to do with applyTimes - // itself being wrong. + // Whole seconds: some filesystems (e.g. FAT) don't store sub-second + // mtime precision, which would make a nanosecond-precision want flaky. want := time.Date(2020, time.March, 15, 10, 30, 0, 0, time.UTC) if err := applyTimes(path, want); err != nil { @@ -161,11 +144,6 @@ func TestApplyTimes_Idempotent(t *testing.T) { } func TestApplyOwnership_SkippedWhenUnavailable(t *testing.T) { - // This is a pure logic check, not dependent on syscall behavior or - // privilege at all: applyOwnership must return before ever calling - // Lchown when OwnershipAvailable is false, which is provable purely - // from the returned (applied, skipped, err) tuple - no real file - // ownership needs to change for this assertion to be meaningful. entry := FileEntry{UID: 0, GID: 0, OwnershipAvailable: false} applied, skipped, err := applyOwnership("/does/not/need/to/exist", entry, true, true) @@ -203,10 +181,8 @@ func TestApplyOwnership_AppliedWhenAvailable(t *testing.T) { t.Fatalf("WriteFile: %v", err) } - // Chown to our own current uid/gid: even an unprivileged process can - // always do this (you can always "change" ownership to yourself), - // unlike chowning to an arbitrary other user, which needs root - so - // this exercises the real syscall without requiring elevation. + // Chown to our own uid/gid: unlike an arbitrary other user, this needs + // no elevated privilege, so it still exercises the real syscall. entry := FileEntry{ UID: uint32(os.Getuid()), GID: uint32(os.Getgid()), @@ -225,10 +201,8 @@ func TestApplyOwnership_AppliedWhenAvailable(t *testing.T) { } } -// trySymlink creates a throwaway symlink to check whether this -// environment supports it at all (matching the same check TestWalk_Symlink -// in walk_test.go uses), skipping the calling test rather than failing it -// when unsupported - e.g. Windows without Developer Mode or elevation. +// trySymlink skips the calling test if this environment can't create +// symlinks (e.g. Windows without Developer Mode or elevation). func trySymlink(t *testing.T) { t.Helper() dir := t.TempDir() @@ -245,7 +219,7 @@ func TestApplySymlink(t *testing.T) { entry := FileEntry{ Path: "link", Mode: os.ModeSymlink | 0o777, - LinkTarget: "some-target-file", // no path separator - see applySymlink's doc comment on why LinkTarget isn't slash-normalized + LinkTarget: "some-target-file", } if err := applySymlink(destPath, entry); err != nil { @@ -386,9 +360,6 @@ func TestApplyAttributes_SymlinkSkipsPermsAndTimes(t *testing.T) { LinkTarget: "some-target-file", } - // Perms and Times are requested here, same as Links - proving they're - // deliberately skipped for a symlink entry rather than accidentally - // omitted from the test's opts. result, err := ApplyAttributes(entry, destPath, AttrOptions{Perms: true, Times: true, Links: true}) if err != nil { t.Fatalf("ApplyAttributes returned error: %v", err) diff --git a/internal/sync/blocks.go b/internal/sync/blocks.go index 07df94e..af9d692 100644 --- a/internal/sync/blocks.go +++ b/internal/sync/blocks.go @@ -2,17 +2,13 @@ package sync // DefaultBlockSize is the fixed block size used to split files for the // delta-transfer algorithm. Real rsync computes this dynamically per file -// (roughly proportional to the square root of the file size, within -// tunable bounds); using one fixed size here is a deliberate -// simplification for this ticket - dynamic block sizing is a future -// refinement, not something the algorithm itself depends on. +// (roughly proportional to the square root of file size); using one fixed +// size here is a deliberate simplification, not an algorithmic requirement. const DefaultBlockSize = 700 // splitBlocks splits data into fixed-size blocks of blockSize bytes each. -// The final block is shorter than blockSize whenever len(data) isn't an -// exact multiple of it; it's still included, never dropped or padded out -// to a full block. Returned slices share data's backing array rather than -// being copied. +// The final block is shorter than blockSize when len(data) isn't an exact +// multiple of it. Returned slices share data's backing array. func splitBlocks(data []byte, blockSize int) [][]byte { if blockSize <= 0 { return nil diff --git a/internal/sync/blocks_test.go b/internal/sync/blocks_test.go index 2383a23..dabece2 100644 --- a/internal/sync/blocks_test.go +++ b/internal/sync/blocks_test.go @@ -27,9 +27,6 @@ func TestSplitBlocks(t *testing.T) { t.Errorf("block %d: len = %d, want %d", i, len(blocks[i]), wantLen) } } - // Reassembling every block must exactly reproduce the input - - // this is the property that actually matters (no bytes lost, - // duplicated, or reordered), not just the length list. var got []byte for _, b := range blocks { got = append(got, b...) diff --git a/internal/sync/checksum.go b/internal/sync/checksum.go index 473d56e..cf0ea2c 100644 --- a/internal/sync/checksum.go +++ b/internal/sync/checksum.go @@ -2,24 +2,17 @@ package sync import "crypto/md5" -// strongChecksum is the collision-resistant checksum used to confirm a -// weak-checksum match. MD5 is not cryptographically safe against a -// deliberate adversary, but that's not what it's used for here - it's -// only there to catch the rare case where two different blocks happen to -// share a weak checksum (see delta generation), which stdlib md5 is more -// than sufficient for. +// strongChecksum confirms a weak-checksum match. MD5 isn't used for +// cryptographic security here, only to catch the rare case where two +// different blocks share a weak checksum. func strongChecksum(block []byte) [md5.Size]byte { return md5.Sum(block) } -// rollingChecksumModulus is 65536 (2^16) - a power of two, not a prime. -// Real Adler-32 uses the largest prime below 65536 (65521) instead; this -// is rsync's own simpler variant, chosen deliberately because a -// power-of-two modulus makes unsigned-integer wraparound during roll() -// mathematically safe (see the comment there) without extra bounds -// handling. It rolls in O(1), which is the only property that actually -// matters here - this is not meant to be byte-compatible with stdlib -// hash/adler32. +// rollingChecksumModulus is 65536 (2^16), not a prime like real Adler-32's +// 65521. rsync's own simpler variant uses a power of two deliberately: it +// makes the unsigned-integer wraparound in roll() mathematically safe +// without extra bounds handling (see roll()'s comment). const rollingChecksumModulus = 1 << 16 // weakChecksum is a rolling checksum over a fixed-size window: two 16-bit @@ -50,17 +43,14 @@ func (w weakChecksum) sum() uint32 { return w.a + w.b*rollingChecksumModulus } -// roll advances the window by exactly one byte: out is the byte leaving -// at the window's start, in is the byte entering at its end. This is O(1) -// regardless of window size - the entire point of a rolling checksum, -// versus calling newWeakChecksum on the shifted window from scratch. +// roll advances the window by one byte in O(1): out is the byte leaving at +// the window's start, in is the byte entering at its end. // -// The subtractions below can underflow as uint32 arithmetic (e.g. if -// w.a < out). That's fine, not a bug: Go's unsigned integers wrap modulo -// 2^32, and since rollingChecksumModulus (2^16) evenly divides 2^32, the -// wrapped value still reduces to the mathematically correct result mod -// 2^16 after the final "% rollingChecksumModulus". A prime modulus (real -// Adler-32's 65521) would not have this property. +// The subtractions below can underflow as uint32 arithmetic. That's fine: +// Go's unsigned integers wrap modulo 2^32, and since rollingChecksumModulus +// (2^16) evenly divides 2^32, the wrapped value still reduces to the +// mathematically correct result mod 2^16. A prime modulus would not have +// this property. func (w weakChecksum) roll(out, in byte) weakChecksum { a := (w.a - uint32(out) + uint32(in)) % rollingChecksumModulus b := (w.b - w.length*uint32(out) + a) % rollingChecksumModulus diff --git a/internal/sync/checksum_test.go b/internal/sync/checksum_test.go index b9816b2..559d4d6 100644 --- a/internal/sync/checksum_test.go +++ b/internal/sync/checksum_test.go @@ -18,13 +18,8 @@ func TestStrongChecksum(t *testing.T) { } } -// TestWeakChecksum_RollMatchesFromScratch is the load-bearing test for the -// entire rolling-checksum design: it proves roll() produces exactly the -// same result as recomputing from scratch, at every single offset across -// a test string, not just a couple of spot checks. If this property ever -// broke, the delta algorithm would silently miss real block matches (or -// worse, produce false ones that only got caught by the strong checksum, -// masking the bug) - so this needs direct proof, not just "it compiles". +// TestWeakChecksum_RollMatchesFromScratch checks that roll() matches +// recomputing from scratch at every offset across a test string. func TestWeakChecksum_RollMatchesFromScratch(t *testing.T) { data := []byte("the quick brown fox jumps over the lazy dog, then jumps back again") const windowSize = 8 diff --git a/internal/sync/delta.go b/internal/sync/delta.go index d8da8b0..64830d7 100644 --- a/internal/sync/delta.go +++ b/internal/sync/delta.go @@ -2,51 +2,45 @@ package sync import "fmt" -// DeltaOp is one operation in a delta stream, produced by GenerateDelta -// and consumed by ApplyDelta. It's a sealed interface - isDeltaOp is -// unexported, so CopyOp and DataOp are its only implementations; callers -// type-switch on the concrete type. +// DeltaOp is one operation in a delta stream, produced by GenerateDelta and +// consumed by ApplyDelta. isDeltaOp is unexported so CopyOp and DataOp are +// its only implementations; callers type-switch on the concrete type. type DeltaOp interface { isDeltaOp() } -// CopyOp copies block BlockIndex - an index into the Blocks of the -// Signature that GenerateDelta was given - from the receiver's old file, -// unchanged. +// CopyOp copies block BlockIndex - an index into the Signature's Blocks +// that GenerateDelta was given - from the receiver's old file, unchanged. type CopyOp struct { BlockIndex int } func (CopyOp) isDeltaOp() {} -// DataOp writes Bytes literally: data present in the new file that didn't -// match any block in the old file's signature. +// DataOp writes Bytes literally: data that didn't match any signature block. type DataOp struct { Bytes []byte } func (DataOp) isDeltaOp() {} -// GenerateDelta compares newData against sig - a signature of some old -// data the receiver already has - and produces an ordered delta that, -// applied to that old data via ApplyDelta, reconstructs newData. +// GenerateDelta compares newData against sig, a signature of old data the +// receiver already has, and produces an ordered delta that reconstructs +// newData when applied to that old data via ApplyDelta. // // It slides a blockSize-byte window across newData one byte at a time, // maintaining the rolling weak checksum incrementally (weakChecksum.roll, -// O(1) per byte) rather than recomputing it from scratch at every -// position - recomputing would silently make this an O(n*blockSize) -// scan, defeating the entire reason a rolling checksum exists. +// O(1) per byte) instead of recomputing it from scratch at every position, +// which would degrade this to an O(n*blockSize) scan. func GenerateDelta(sig Signature, newData []byte) []DeltaOp { blockSize := sig.BlockSize if blockSize <= 0 { blockSize = DefaultBlockSize } - // weak checksum -> indices of every block sharing it. A weak checksum - // collision (two different blocks that happen to produce the same - // 32-bit weak sum) is expected to happen occasionally by chance; the - // slice of candidates lets the strong-checksum check below disambiguate - // rather than assuming the first weak match is correct. + // weak checksum -> indices of every block sharing it. Two different + // blocks colliding on their 32-bit weak sum is expected occasionally by + // chance; the strong-checksum check below disambiguates candidates. weakIndex := make(map[uint32][]int, len(sig.Blocks)) for i, b := range sig.Blocks { weakIndex[b.Weak] = append(weakIndex[b.Weak], i) @@ -60,15 +54,13 @@ func GenerateDelta(sig Signature, newData []byte) []DeltaOp { return } ops = append(ops, DataOp{Bytes: pending}) - // Reset to nil (not just len 0) so the next append starts a fresh - // backing array instead of potentially growing into - and - // corrupting - the slice just handed to the DataOp above. + // Reset to nil, not just len 0, so the next append starts a fresh + // backing array instead of growing into the slice just handed to DataOp. pending = nil } - // tryMatch checks whether the blockSize-byte window at newData[at:] - - // whose already-computed rolling checksum is weak - matches a - // signature block. Confirms via strong checksum before accepting. + // tryMatch checks whether the blockSize-byte window at newData[at:], with + // already-computed rolling checksum weak, matches a signature block. tryMatch := func(at int, weak weakChecksum) (blockIndex int, ok bool) { candidates, found := weakIndex[weak.sum()] if !found { @@ -87,17 +79,12 @@ func GenerateDelta(sig Signature, newData []byte) []DeltaOp { pos := 0 for pos < n { if pos+blockSize > n { - // Fewer than blockSize bytes remain: no full window left to - // possibly match, so the rest of the file is literal data. - // (No need to advance pos before this break - nothing reads - // it again once the loop exits.) + // Fewer than blockSize bytes remain: no full window left to match. pending = append(pending, newData[pos:]...) break } - // Freshly computed only here - once per match/skip-ahead, not per - // byte - then advanced with roll() for every subsequent byte the - // inner loop steps through without a match. + // Computed fresh only here, then advanced with roll() per byte below. weak := newWeakChecksum(newData[pos : pos+blockSize]) for { if idx, ok := tryMatch(pos, weak); ok { @@ -110,7 +97,7 @@ func GenerateDelta(sig Signature, newData []byte) []DeltaOp { pending = append(pending, newData[pos]) pos++ if pos+blockSize > n { - break // not enough bytes left for a full window anymore + break } weak = weak.roll(newData[pos-1], newData[pos+blockSize-1]) } @@ -121,34 +108,17 @@ func GenerateDelta(sig Signature, newData []byte) []DeltaOp { } // ApplyDelta reconstructs a file from oldData and an ordered []DeltaOp -// produced by GenerateDelta(sig, ...) against that same oldData: each -// CopyOp copies its referenced block out of oldData, and each DataOp -// writes its literal bytes, in order. +// produced by GenerateDelta(sig, ...) against that same oldData. // -// sig must be the exact Signature GenerateDelta was called with - a -// CopyOp only carries a block index, not byte offsets, so BlockSize is -// the only way to recover which bytes of oldData that index refers to. -// Passing a different Signature (or a hand-built one with a mismatched -// BlockSize) would silently reconstruct the wrong bytes; that's exactly -// the class of bug bundling BlockSize into Signature (see signature.go) -// is meant to make harder, by giving ApplyDelta and GenerateDelta the -// same single source of truth for it instead of two independent -// parameters that could drift apart. +// sig must be the exact Signature GenerateDelta was called with: a CopyOp +// only carries a block index, not byte offsets, so BlockSize is the only way +// to recover which bytes of oldData that index refers to. A mismatched +// BlockSize would silently reconstruct the wrong bytes. // -// The result is returned as a []byte rather than written to an io.Writer: -// every function in this package so far (Walk aside) operates on -// in-memory byte slices - there's no streaming I/O anywhere yet for this -// to plug into - so an io.Writer parameter would just be unused -// flexibility at this stage. That can change if/when a streaming -// transport is introduced later. -// blockSize is validated lazily, only once a CopyOp actually needs it to -// translate a BlockIndex into a byte range - not unconditionally up -// front - so a delta with no CopyOps at all (every real caller today -// still has BlockSize > 0 via GenerateSignature's own DefaultBlockSize, -// but SC-12's append-mode construction can legitimately produce a -// Signature with BlockSize == 0 when the receiver's existing file is -// empty, since there is then no prefix block to describe at all) never -// gets rejected for a value that would never actually be used. +// blockSize is validated lazily, only once a CopyOp needs it to translate a +// BlockIndex into a byte range - append-mode construction can legitimately +// produce a Signature with BlockSize == 0 when the old file is empty, and a +// delta with no CopyOps should not be rejected for a value it never uses. func ApplyDelta(oldData []byte, ops []DeltaOp, sig Signature) ([]byte, error) { blockSize := sig.BlockSize diff --git a/internal/sync/delta_bench_test.go b/internal/sync/delta_bench_test.go index 731ce07..d0f84a7 100644 --- a/internal/sync/delta_bench_test.go +++ b/internal/sync/delta_bench_test.go @@ -7,15 +7,9 @@ import ( ) // generateBenchData returns (oldData, newData), both n bytes long: oldData -// is pseudo-random (a fixed seed, so every run of a given size/percent -// combination benchmarks the identical input, not fresh randomness each -// time), and newData is derived from it by changing roughly -// changePercent% of its bytes, scattered at random positions rather than -// as one contiguous run - closer to how a real file's edits tend to be -// spread through it than a single block of difference would be. -// changePercent >= 100 instead generates a completely independent -// newData, guaranteeing "genuinely nothing matches" rather than relying -// on random flips to (probabilistically, imperfectly) cover every byte. +// is pseudo-random from a fixed seed, and newData changes roughly +// changePercent% of its bytes at scattered random positions. +// changePercent >= 100 instead generates a fully independent newData. func generateBenchData(n, changePercent int, seed int64) (oldData, newData []byte) { rng := rand.New(rand.NewSource(seed)) oldData = make([]byte, n) @@ -36,16 +30,8 @@ func generateBenchData(n, changePercent int, seed int64) (oldData, newData []byt return oldData, newData } -// BenchmarkGenerateDelta measures block-matching throughput - almost -// certainly the most performance-sensitive code in this project, since -// it runs once per regular file on every sync, over the file's entire -// content. Varied across two independent dimensions, not just one -// arbitrary case: file size (does throughput hold up as files grow, or -// does something scale worse than linearly), and change percentage (a -// file that's mostly identical to its old version, real delta transfer's -// whole reason to exist, versus one that's mostly or entirely different, -// where nearly every window has to fall through to a literal DataOp -// instead of matching a block). +// BenchmarkGenerateDelta measures block-matching throughput across file size +// and change percentage, since both can affect how it scales. func BenchmarkGenerateDelta(b *testing.B) { sizes := []int{10 * 1024, 100 * 1024, 1024 * 1024} changePercents := []int{0, 10, 50, 100} @@ -68,12 +54,8 @@ func BenchmarkGenerateDelta(b *testing.B) { } } -// BenchmarkGenerateSignature measures the other half of a real sync's -// per-file cost on the receiving side: splitting the existing -// destination file into blocks and checksumming each one. Unlike -// GenerateDelta, signature generation doesn't depend on how similar the -// two files are - only on the existing file's own size - so this only -// varies that one dimension. +// BenchmarkGenerateSignature measures signature generation, which depends +// only on file size, not on similarity between old and new data. func BenchmarkGenerateSignature(b *testing.B) { sizes := []int{10 * 1024, 100 * 1024, 1024 * 1024} diff --git a/internal/sync/delta_test.go b/internal/sync/delta_test.go index fc6fe23..315421f 100644 --- a/internal/sync/delta_test.go +++ b/internal/sync/delta_test.go @@ -32,15 +32,6 @@ func TestApplyDelta_OutOfRangeBlockIndexErrors(t *testing.T) { } } -// TestApplyDelta_InvalidBlockSizeErrorsOnlyWhenACopyOpNeedsIt is SC-12's -// own relaxation of ApplyDelta's block-size validation, made concrete: a -// zero/negative BlockSize is only ever actually consulted when -// translating a CopyOp's BlockIndex into a byte range, so it must only -// be rejected when ops genuinely contains a CopyOp - not unconditionally -// up front, which would reject SC-12's own append-mode construction for -// a brand-new (zero-length existing) destination file, where a -// Signature with BlockSize == 0 and no CopyOp at all is entirely valid -// (there is no prefix block to describe). func TestApplyDelta_InvalidBlockSizeErrorsOnlyWhenACopyOpNeedsIt(t *testing.T) { if _, err := ApplyDelta([]byte("data"), []DeltaOp{CopyOp{BlockIndex: 0}}, Signature{BlockSize: 0}); err == nil { t.Errorf("ApplyDelta with a zero block size and a CopyOp returned nil error, want an error") @@ -63,21 +54,10 @@ func TestApplyDelta_InvalidBlockSizeErrorsOnlyWhenACopyOpNeedsIt(t *testing.T) { } } -// TestGenerateDelta_WeakChecksumCollisionDisambiguatedByStrongChecksum -// exercises the actual weak-checksum-collision path, rather than assuming -// the strong-checksum disambiguation code is correct because it looks -// right. block1, block2, and block3 below are three deliberately -// constructed, genuinely different 3-byte sequences that all produce the -// identical weak checksum (a=60, b=100 - hand-verified against -// newWeakChecksum's exact weighting convention, no modular wraparound -// needed): with weight (n-i) for byte i in a window of n=3, all three -// satisfy sum=60 and 3x+2y+z=100 simultaneously by construction. -// -// If GenerateDelta ever regressed to trusting a weak-checksum match -// without confirming it via strong checksum, this test would silently -// start producing corrupted output (a CopyOp pointing at the wrong -// block) - exactly the class of bug a weak checksum alone can't catch, -// which is the entire reason a strong checksum exists. +// block1, block2, and block3 are three different 3-byte sequences +// constructed to all produce the identical weak checksum (a=60, b=100), +// so this test exercises the strong-checksum disambiguation path rather +// than just trusting it looks correct. func TestGenerateDelta_WeakChecksumCollisionDisambiguatedByStrongChecksum(t *testing.T) { block1 := []byte{10, 20, 30} block2 := []byte{15, 10, 35} @@ -128,7 +108,7 @@ func TestGenerateDelta_WeakChecksumCollisionDisambiguatedByStrongChecksum(t *tes } d, ok := ops[0].(DataOp) if !ok || string(d.Bytes) != string(block3) { - t.Errorf("op = %+v, want DataOp{Bytes: %v} - a weak-only match must not produce a CopyOp", ops[0], block3) + t.Errorf("op = %+v, want DataOp{Bytes: %v}", ops[0], block3) } }) } @@ -146,7 +126,6 @@ func TestGenerateDelta_IdenticalFileIsAllCopies(t *testing.T) { if copies != 4 { t.Errorf("got %d CopyOps, want 4 (one per block): %+v", copies, ops) } - // Order matters too: block 0 first, then 1, 2, 3. for i, op := range ops { cp, ok := op.(CopyOp) if !ok || cp.BlockIndex != i { @@ -170,8 +149,6 @@ func TestGenerateDelta_CompletelyDifferentFileIsAllData(t *testing.T) { t.Fatalf("got 0 DataOps, want at least 1") } - // Reassembling every DataOp's bytes must reproduce newData exactly, - // since nothing was copied. var got []byte for _, op := range ops { got = append(got, op.(DataOp).Bytes...) @@ -198,9 +175,6 @@ func TestGenerateDelta_SingleByteChangeInMiddle(t *testing.T) { t.Fatalf("got 0 DataOps despite a changed byte, want at least 1") } - // The whole point of the algorithm: a one-byte change should cost a - // small, bounded amount of literal data, not force the whole file (or - // even the whole surrounding block) to be retransmitted as data. var totalDataBytes int for _, op := range ops { if d, ok := op.(DataOp); ok { @@ -211,9 +185,6 @@ func TestGenerateDelta_SingleByteChangeInMiddle(t *testing.T) { t.Errorf("total literal bytes = %d, want a small bounded amount for a single changed byte", totalDataBytes) } - // Reconstructing from ops must still reproduce newData exactly - the - // strongest check that "mostly CopyOps, one small DataOp" is actually - // correct, not just small. reconstructed, err := ApplyDelta(old, ops, sig) if err != nil { t.Fatalf("ApplyDelta returned error: %v", err) diff --git a/internal/sync/filter.go b/internal/sync/filter.go index 408514b..7afda73 100644 --- a/internal/sync/filter.go +++ b/internal/sync/filter.go @@ -8,11 +8,9 @@ import ( "strings" ) -// RuleKind identifies which flag produced a raw filter rule, before any -// pattern parsing or file expansion happens. It mirrors the shape of -// internal/cli's FilterRule (Type + Pattern) but is defined independently: -// internal/sync must never import internal/cli, since cli is expected to -// depend on sync (not the other way around) once they're wired together. +// RuleKind identifies which flag produced a raw filter rule, before pattern +// parsing or file expansion happens. Defined independently from internal/cli's +// FilterRule since internal/sync must never import internal/cli. type RuleKind string const ( @@ -20,15 +18,10 @@ const ( RuleInclude RuleKind = "include" // RuleExclude is a direct --exclude pattern. RuleExclude RuleKind = "exclude" - // RuleFilter is a raw --filter rule line, e.g. "+ *.txt", "- .git/", - // or "merge FILE" - see parseFilterLine for the subset of rsync's - // filter-rule syntax this supports. + // RuleFilter is a raw --filter rule line, e.g. "+ *.txt", "- .git/", or "merge FILE". RuleFilter RuleKind = "filter" - // RuleExcludeFrom has a Pattern that is a file path, not a filter - // pattern itself. CompileRules reads that file and inserts one - // exclude rule per line at this exact position in the list, - // preserving overall command-line order rather than appending - // everything to the end. + // RuleExcludeFrom is a --exclude-from file path; CompileRules expands + // it in place at that position, preserving command-line order. RuleExcludeFrom RuleKind = "exclude-from" // RuleIncludeFrom is RuleExcludeFrom's --include-from counterpart. RuleIncludeFrom RuleKind = "include-from" @@ -51,21 +44,17 @@ const ( Exclude ) -// Rule is a single compiled, ready-to-match filter rule. Pattern has -// already had its leading "/" (Anchored) marker stripped by CompileRules; -// it never contains that marker itself. +// Rule is a single compiled, ready-to-match filter rule. Pattern has already +// had its leading "/" and trailing "/" markers stripped by CompileRules. // -// Anchored matches real rsync's actual rule: a pattern anchors to the -// transfer root if it has a leading "/", contains any other "/", or -// contains "**" - only a pattern with none of those (a bare filename, e.g. -// "*.log") matches at any depth, against the final path component only. +// A pattern anchors to the transfer root if it has a leading "/", contains +// any other "/", or contains "**"; only a bare filename like "*.log" matches +// at any depth, against the final path component only (matching rsync). type Rule struct { Action Action Pattern string Anchored bool - // DirOnly means this rule only ever matches directories - set from a - // trailing "/" on the original pattern, stripped by CompileRules just - // like the anchor marker. + // DirOnly means the rule only matches directories (from a trailing "/" on the original pattern). DirOnly bool } @@ -82,8 +71,7 @@ func (r Rule) matches(entryPath string, isDir bool) bool { return matchSegments(patternSegs, pathSegs) } - // Unanchored: the pattern may match starting at any depth, as if - // "**/" were implicitly prepended to it. + // Unanchored: try matching starting at any depth, as if "**/" were prepended. for start := 0; start <= len(pathSegs); start++ { if matchSegments(patternSegs, pathSegs[start:]) { return true @@ -92,13 +80,9 @@ func (r Rule) matches(entryPath string, isDir bool) bool { return false } -// matchSegments matches a "/"-split pattern against a "/"-split path, -// segment by segment. Every segment except "**" is matched with -// path.Match, which already gives us "*" (any run of characters within the -// segment) and "?" (exactly one character) for free, without needing a -// hand-rolled matcher of our own. "**" is handled explicitly: it may -// consume zero or more path segments, so both possibilities (consume none -// and keep matching, or consume one and recurse) are tried. +// matchSegments matches a "/"-split pattern against a "/"-split path. +// Every segment except "**" is matched with path.Match. "**" may consume +// zero or more path segments, so both possibilities are tried. func matchSegments(patternSegs, pathSegs []string) bool { if len(patternSegs) == 0 { return len(pathSegs) == 0 @@ -124,18 +108,14 @@ func matchSegments(patternSegs, pathSegs []string) bool { return matchSegments(patternSegs[1:], pathSegs[1:]) } -// compilePattern parses a single raw pattern's anchor ("/" prefix) and -// dir-only ("/" suffix) markers into a ready-to-match Rule with the given -// action. Shared by direct --include/--exclude rules, each line read from -// an --exclude-from/--include-from file, and --filter rule lines. +// compilePattern parses a pattern's anchor ("/" prefix) and dir-only ("/" +// suffix) markers into a ready-to-match Rule. Shared by direct +// --include/--exclude rules, --exclude-from/--include-from file lines, and +// --filter rule lines. // -// A pattern containing any empty "/"-separated segment - a bare "/" or "" -// overall, or an internal "//" typo like "a//b" - is rejected rather than -// silently compiled: no real FileEntry.Path segment is ever empty (Walk() -// never produces one), so a Rule requiring an empty segment could never -// match anything. Compiling it anyway would leave the user with a filter -// rule that silently does nothing forever, which is worse than a clear -// error at compile time. +// A pattern with an empty "/"-segment (bare "/", "", or an "a//b" typo) is +// rejected rather than compiled: it could never match a real FileEntry.Path, +// so it would silently become a no-op rule instead of a clear error. func compilePattern(action Action, pattern string) (Rule, error) { leadingSlash := strings.HasPrefix(pattern, "/") if leadingSlash { @@ -151,19 +131,15 @@ func compilePattern(action Action, pattern string) (Rule, error) { } } - // A leading "/" always anchors. So does any *other* "/" still present - // in the pattern, or a "**" anywhere in it - matching real rsync's - // rule, not just the "leading slash only" simplification this started - // as. Only a genuinely slash-free, "**"-free pattern (a bare filename) - // matches at any depth. + // Anchors on leading "/", any other "/", or "**" anywhere - matching rsync's rule. anchored := leadingSlash || strings.Contains(pattern, "/") || strings.Contains(pattern, "**") return Rule{Action: action, Pattern: pattern, Anchored: anchored, DirOnly: dirOnly}, nil } // readPatternFile reads one pattern per line from path, for -// --exclude-from/--include-from. Blank lines and lines starting with "#" -// or ";" are skipped, matching rsync's own filter-file comment convention. +// --exclude-from/--include-from. Blank lines and lines starting with "#" or +// ";" are skipped, matching rsync's filter-file comment convention. func readPatternFile(path string) (patterns []string, err error) { f, err := os.Open(path) if err != nil { @@ -190,21 +166,17 @@ func readPatternFile(path string) (patterns []string, err error) { } // parsedFilterLine is the result of parsing one line of --filter RULE -// syntax - whether it came directly from a --filter flag or from a line -// inside a merge file. +// syntax, whether from a --filter flag or a line inside a merge file. type parsedFilterLine struct { isMerge bool mergeFile string rule Rule // valid only when !isMerge } -// parseFilterLine implements a deliberately small subset of rsync's -// --filter rule syntax: "+ PATTERN" / "- PATTERN" (and their word-form -// equivalents "include PATTERN" / "exclude PATTERN"), plus "merge FILE". -// Real rsync's filter language is much larger (modifiers like "-C", -// "dir-merge" with per-directory semantics, "!", exclude-if-present rules, -// and more) - none of that is implemented here; anything outside this -// subset is a hard parse error rather than a silent no-op. +// parseFilterLine implements a subset of rsync's --filter syntax: +// "+ PATTERN" / "- PATTERN" (and "include"/"exclude" word forms), plus +// "merge FILE". Anything outside this subset is a hard parse error, not a +// silent no-op. func parseFilterLine(line string) (parsedFilterLine, error) { switch { case strings.HasPrefix(line, "+ "): @@ -228,14 +200,9 @@ func parseFilterLine(line string) (parsedFilterLine, error) { } } -// expandMergeFile reads path and parses each non-comment, non-blank line -// with parseFilterLine - the same syntax --filter itself accepts. Nested -// merge (a merge file whose own lines include another "merge OTHER" -// directive) is deliberately unsupported: rather than silently recursing -// (risking an infinite loop on a self-referential merge file) or silently -// dropping the nested directive, it's a hard error. This covers the -// "basic merge case"; full nested/dir-merge support is a larger feature -// left for later if it turns out to be needed. +// expandMergeFile reads path and parses each line with parseFilterLine. +// Nested merge directives are a hard error rather than silently recursing +// (risking a loop on a self-referential file) or silently dropping them. func expandMergeFile(path string) ([]Rule, error) { lines, err := readPatternFile(path) if err != nil { @@ -256,13 +223,9 @@ func expandMergeFile(path string) ([]Rule, error) { return rules, nil } -// CompileRules converts raw rules - --include/--exclude patterns, -// --filter rule lines (including "merge FILE"), and --exclude-from/ -// --include-from file references - into a single ordered, ready-to-match -// Rule list. From-file and merged rules are expanded in place at the -// position their flag occurred, so e.g. a --exclude-from sandwiched -// between two direct --exclude flags on the command line stays sandwiched -// between their compiled Rules, not appended after them. +// CompileRules converts raw rules into a single ordered, ready-to-match Rule +// list. From-file and merged rules are expanded in place at the position +// their flag occurred, preserving command-line order. func CompileRules(raw []RawRule) ([]Rule, error) { var rules []Rule for _, r := range raw { @@ -316,16 +279,8 @@ func CompileRules(raw []RawRule) ([]Rule, error) { return rules, nil } -// Included evaluates rules against a single FileEntry using rsync's -// first-match-wins semantics: rules are tried in order, and the action of -// the first one that matches decides the outcome. If no rule matches, the -// entry is included by default - matching rsync's own default behavior of -// transferring anything not explicitly excluded. -// -// entry.Path is never empty and never represents the transfer root itself: -// Walk() (internal/sync/walk.go) deliberately excludes the root from its -// own output, so there's no "path exactly equal to the root" case for this -// function to special-case. +// Included evaluates rules against entry using rsync's first-match-wins +// semantics; if no rule matches, the entry is included by default. func Included(rules []Rule, entry FileEntry) bool { for _, r := range rules { if r.matches(entry.Path, entry.IsDir) { @@ -335,11 +290,7 @@ func Included(rules []Rule, entry FileEntry) bool { return true } -// FilterEntries returns the subset of entries that rules includes, in -// their original order. It is a post-pass over an already-collected -// Walk() result rather than a predicate threaded into Walk() itself - see -// the design note on this tradeoff where FilterEntries is introduced in -// the accompanying documentation/commit. +// FilterEntries returns the subset of entries that rules includes, preserving order. func FilterEntries(entries []FileEntry, rules []Rule) []FileEntry { kept := make([]FileEntry, 0, len(entries)) for _, e := range entries { diff --git a/internal/sync/filter_test.go b/internal/sync/filter_test.go index 787f4dd..de1b936 100644 --- a/internal/sync/filter_test.go +++ b/internal/sync/filter_test.go @@ -70,10 +70,6 @@ func TestCompileRules_Anchoring(t *testing.T) { } func TestCompileRules_InternalSlashAnchorsWithoutLeadingSlash(t *testing.T) { - // Real rsync's actual rule: a pattern anchors to the root if it has a - // leading "/", OR contains any other "/", OR contains "**" - not just - // on an explicit leading "/". "src/main.go" (no leading slash, but an - // internal one) must behave the same as "/src/main.go" here. rules := mustCompile(t, []RawRule{ {Kind: RuleExclude, Pattern: "src/main.go"}, }) @@ -91,8 +87,6 @@ func TestCompileRules_InternalSlashAnchorsWithoutLeadingSlash(t *testing.T) { } func TestCompileRules_DoubleStarAnchorsWithoutSlash(t *testing.T) { - // Same rsync rule, the "**" half: a pattern containing "**" anchors - // even with no "/" anywhere in it. rules := mustCompile(t, []RawRule{ {Kind: RuleExclude, Pattern: "**cache**"}, }) @@ -102,7 +96,6 @@ func TestCompileRules_DoubleStarAnchorsWithoutSlash(t *testing.T) { } func TestCompileRules_BareFilenameStaysUnanchored(t *testing.T) { - // The one case that must NOT anchor: no "/" at all, no "**" at all. rules := mustCompile(t, []RawRule{ {Kind: RuleExclude, Pattern: "*.log"}, }) @@ -163,10 +156,6 @@ func TestIncluded_NoRulesMatchDefaultsToIncluded(t *testing.T) { func TestIncluded_FirstMatchWinsOrderMatters(t *testing.T) { entry := FileEntry{Path: "keep.log"} - // Same two rules, opposite order: the first one to match should win in - // both cases, so swapping the order must flip the outcome. If it - // didn't, evaluation wouldn't actually be "first match wins" - it'd be - // "last match wins" or "most specific wins" or something else. includeFirst := mustCompile(t, []RawRule{ {Kind: RuleInclude, Pattern: "keep.log"}, {Kind: RuleExclude, Pattern: "*.log"}, @@ -221,10 +210,6 @@ func TestCompileRules_ExcludeFromPreservesPosition(t *testing.T) { {Kind: RuleInclude, Pattern: "last.txt"}, }) - // Position matters here: the two patterns read from the file must land - // between "first.txt" and "last.txt", not get appended after - // "last.txt" - that would silently reorder rules relative to what the - // user typed on the command line, breaking first-match-wins semantics. want := []struct { action Action pattern string @@ -344,14 +329,12 @@ func TestCompileRules_EmptyPatternErrors(t *testing.T) { for _, pattern := range tests { _, err := CompileRules([]RawRule{{Kind: RuleExclude, Pattern: pattern}}) if err == nil { - t.Errorf("CompileRules with pattern %q returned nil error, want an error (a silent no-op rule is worse than a clear failure)", pattern) + t.Errorf("CompileRules with pattern %q returned nil error, want an error", pattern) } } } func TestRule_WildcardOnlyPatternsMatchEverything(t *testing.T) { - // This is intended behavior, matching real rsync: a bare "*" or "**" - // is a deliberate "match everything" rule, not a bug to guard against. entries := []FileEntry{{Path: "a"}, {Path: "a/b"}, {Path: "a/b/c.txt"}} star := mustCompile(t, []RawRule{{Kind: RuleExclude, Pattern: "*"}}) diff --git a/internal/sync/hardlinks.go b/internal/sync/hardlinks.go index 5f4780e..1ee829d 100644 --- a/internal/sync/hardlinks.go +++ b/internal/sync/hardlinks.go @@ -9,36 +9,18 @@ import ( ) // HardLinkGroup is a set of paths (relative, "/"-separated, matching -// FileEntry.Path) that are all hard-linked to the same underlying file - -// i.e. they share one (device, inode) identity on POSIX - and so must be -// recreated as hard links of each other on the receiving end, not as -// independent copies of the same content. +// FileEntry.Path) that share one (device, inode) identity on POSIX and must +// be recreated as hard links of each other, not independent copies. type HardLinkGroup []string -// DetectHardLinks re-examines entries under root (paths relative to root, -// as produced by Walk) and groups together every regular file that shares -// an inode with at least one other entry. +// DetectHardLinks re-examines entries under root and groups together every +// regular file that shares an inode with at least one other entry. Only +// groups with more than one member are returned. // -// This is a separate pass over Walk's output, not something Walk itself -// tracks - the same tradeoff FilterEntries (filter.go) already made for -// the same reason: it keeps Walk unchanged and independently testable. -// The cost here is re-Lstat'ing each non-directory, non-symlink entry; -// directories and symlinks are skipped outright since neither can be -// hard-linked to a regular file's data. -// -// Only groups with more than one member are returned - a file sharing its -// inode with nothing else needs no special handling, it's just a normal -// file. On a platform where lookupHardLinkKey can't determine inode -// identity (Windows - see hardlinks_windows.go), every entry is treated -// as its own singleton, so DetectHardLinks always returns no groups -// there; that's a documented platform gap, not a bug in this function. -// -// An empty result is genuinely ambiguous on its own: it means either "no -// hard links exist in this tree" or "this platform can't detect them at -// all," and those call for different handling by whoever's orchestrating -// a sync (the latter is worth a one-time warning; the former isn't worth -// mentioning). Call HardLinksSupported() to tell them apart explicitly -// rather than guessing from an empty slice. +// On a platform where lookupHardLinkKey can't determine inode identity +// (Windows), every entry is its own singleton, so this always returns no +// groups there. An empty result is thus ambiguous between "no hard links" +// and "can't detect them" - call HardLinksSupported() to tell those apart. func DetectHardLinks(root string, entries []FileEntry) ([]HardLinkGroup, error) { byKey := make(map[hardLinkKey][]string) @@ -64,11 +46,11 @@ func DetectHardLinks(root string, entries []FileEntry) ([]HardLinkGroup, error) if len(paths) < 2 { continue } - sort.Strings(paths) // deterministic member order, same reasoning as Walk's own sort + sort.Strings(paths) // deterministic member order groups = append(groups, HardLinkGroup(paths)) } sort.Slice(groups, func(i, j int) bool { - return groups[i][0] < groups[j][0] // deterministic group order too + return groups[i][0] < groups[j][0] // deterministic group order }) return groups, nil @@ -90,8 +72,7 @@ func ApplyHardLinks(destRoot string, group HardLinkGroup) error { if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { return fmt.Errorf("creating parent directory for %q: %w", p, err) } - // os.Link fails if dest already exists, the same reason - // applySymlink removes any existing entry first. + // os.Link fails if dest already exists. if err := os.Remove(dest); err != nil && !os.IsNotExist(err) { return fmt.Errorf("removing existing entry at %q: %w", dest, err) } diff --git a/internal/sync/hardlinks_test.go b/internal/sync/hardlinks_test.go index 66ff587..5a7cb81 100644 --- a/internal/sync/hardlinks_test.go +++ b/internal/sync/hardlinks_test.go @@ -35,9 +35,6 @@ func TestDetectHardLinks(t *testing.T) { } if runtime.GOOS == "windows" { - // lookupHardLinkKey always reports unavailable on Windows (see - // hardlinks_windows.go) - even though a real hard link was just - // created above, detection can't observe it, by design. if len(groups) != 0 { t.Errorf("got %d groups on Windows, want 0 (hard-link identity is unavailable there)", len(groups)) } @@ -87,9 +84,6 @@ func TestApplyHardLinks(t *testing.T) { t.Skipf("hard link creation unsupported in this environment: %v", err) } - // Prove they're genuinely the same underlying file, not just two - // copies with equal content: writing through one path must be visible - // through the other. linkedPath := filepath.Join(destRoot, "linked.txt") if err := os.WriteFile(linkedPath, []byte("changed via the linked path"), 0o644); err != nil { t.Fatalf("WriteFile: %v", err) diff --git a/internal/sync/hardlinks_unix.go b/internal/sync/hardlinks_unix.go index a44abc3..e4eb4ac 100644 --- a/internal/sync/hardlinks_unix.go +++ b/internal/sync/hardlinks_unix.go @@ -7,28 +7,21 @@ import ( "syscall" ) -// hardLinkKey identifies a file's underlying inode, uniquely across -// possibly multiple filesystems: an inode number alone is only unique -// within a single device, so two files on different devices/filesystems -// could coincidentally share an inode number without actually being -// hard-linked to each other. (Dev, Ino) together is the identity POSIX -// actually guarantees. +// hardLinkKey identifies a file's underlying inode. An inode number alone is +// only unique within a device, so (Dev, Ino) together is the identity POSIX +// actually guarantees across filesystems. type hardLinkKey struct { Dev uint64 Ino uint64 } // lookupHardLinkKey extracts the (dev, ino) identity from a Lstat'd -// os.FileInfo, the same way lookupUIDGID (uidgid_unix.go) extracts -// uid/gid - via the syscall.Stat_t underlying the info. ok is false only -// if the platform's Sys() doesn't actually return that type. +// os.FileInfo via its syscall.Stat_t. ok is false only if the platform's +// Sys() doesn't return that type. // -// stat.Dev is used as-is: it's already uint64 on the Linux target this -// project actually builds and tests for (see .github/workflows/ci.yaml). -// Some other POSIX platforms (e.g. Darwin) declare Dev as a narrower -// signed type, which would need an explicit conversion - not added here -// speculatively for a platform this project doesn't build or test -// against; that can be added if/when Darwin support is actually taken on. +// stat.Dev is used as-is since it's already uint64 on Linux, the only +// platform this project builds/tests for; other POSIX platforms (e.g. +// Darwin) declare it as a narrower signed type and would need conversion. func lookupHardLinkKey(info os.FileInfo) (hardLinkKey, bool) { stat, ok := info.Sys().(*syscall.Stat_t) if !ok { @@ -37,9 +30,5 @@ func lookupHardLinkKey(info os.FileInfo) (hardLinkKey, bool) { return hardLinkKey{Dev: stat.Dev, Ino: stat.Ino}, true } -// HardLinksSupported reports whether this platform can detect hard links -// at all - see DetectHardLinks's doc comment for how to use this to -// surface the Windows gap explicitly (e.g. a one-time warning) rather -// than leaving callers to infer it from an empty result list, which is -// ambiguous with "this tree just has no hard links". +// HardLinksSupported reports whether this platform can detect hard links at all. func HardLinksSupported() bool { return true } diff --git a/internal/sync/hardlinks_windows.go b/internal/sync/hardlinks_windows.go index 69a17fb..636c5b3 100644 --- a/internal/sync/hardlinks_windows.go +++ b/internal/sync/hardlinks_windows.go @@ -12,22 +12,14 @@ type hardLinkKey struct { } // lookupHardLinkKey always reports unavailable on Windows: the (dev, ino) -// identity this package's hard-link detection relies on comes from POSIX -// os.FileInfo.Sys() (*syscall.Stat_t), which Windows doesn't provide - -// its Sys() there is *syscall.Win32FileAttributeData instead. Windows -// NTFS does have its own file-identity concept (a volume serial number -// plus a 64-bit file index, retrievable via -// GetFileInformationByHandle/BY_HANDLE_FILE_INFORMATION), and os.Link -// does work on Windows/NTFS - but wiring up that Win32-specific API is a -// meaningfully larger, separate piece of platform code, not a small -// extension of the POSIX path. Scoping it out here means hard-linked -// files on a Windows source are simply treated as independent, unlinked -// files (correct output, just missed space-saving/consistency benefit) -// rather than the sync producing wrong results. +// identity comes from POSIX os.FileInfo.Sys() (*syscall.Stat_t), which +// Windows doesn't provide (its Sys() is *syscall.Win32FileAttributeData). +// NTFS has its own file-identity concept via GetFileInformationByHandle, +// but wiring that up is out of scope here; hard-linked files on a Windows +// source are simply treated as independent files instead. func lookupHardLinkKey(_ os.FileInfo) (hardLinkKey, bool) { return hardLinkKey{}, false } -// HardLinksSupported reports whether this platform can detect hard links -// at all - always false on Windows. See DetectHardLinks's doc comment. +// HardLinksSupported reports whether this platform can detect hard links at all. func HardLinksSupported() bool { return false } diff --git a/internal/sync/roundtrip_test.go b/internal/sync/roundtrip_test.go index 0b48f81..f4b9787 100644 --- a/internal/sync/roundtrip_test.go +++ b/internal/sync/roundtrip_test.go @@ -5,13 +5,9 @@ import ( "testing" ) -// TestRoundTrip runs the full receiver/sender/receiver cycle for each -// case: generate a Signature from the old file, generate a delta from -// (old, new) against that signature, apply the delta to the old file, and -// confirm the result is byte-for-byte identical to the new file. This is -// the property the whole algorithm exists to guarantee - none of the -// individual-step tests elsewhere in this package substitute for actually -// proving the full cycle reproduces the target file exactly. +// TestRoundTrip generates a Signature from the old file, generates a delta +// from (old, new) against it, applies the delta to the old file, and +// confirms the result is byte-for-byte identical to the new file. func TestRoundTrip(t *testing.T) { const blockSize = 8 diff --git a/internal/sync/signature.go b/internal/sync/signature.go index 88eb7b3..6a9028a 100644 --- a/internal/sync/signature.go +++ b/internal/sync/signature.go @@ -10,13 +10,11 @@ type BlockSignature struct { Strong [md5.Size]byte } -// Signature is an ordered list of per-block checksums for a file, plus -// the block size used to produce them. BlockSize travels with the -// checksums (rather than being a separate parameter callers must keep in -// sync) because delta generation and reconstruction both need to know -// exactly how the blocks were cut - a mismatched block size would make -// every checksum meaningless. A block's position in Blocks is its index, -// which is how a CopyOp (see delta.go) refers back to it. +// Signature is an ordered list of per-block checksums for a file, plus the +// block size used to produce them. BlockSize travels with the checksums +// rather than being a separate parameter, since a mismatched block size +// would make every checksum meaningless. A block's position in Blocks is +// its index, which is how a CopyOp (see delta.go) refers back to it. type Signature struct { BlockSize int Blocks []BlockSignature diff --git a/internal/sync/specialfiles.go b/internal/sync/specialfiles.go index 518e1ed..50f31aa 100644 --- a/internal/sync/specialfiles.go +++ b/internal/sync/specialfiles.go @@ -12,11 +12,9 @@ import ( type SpecialFileType int const ( - // NotSpecial is a regular file, directory, or symlink - none of - // which ClassifySpecialFile is concerned with. + // NotSpecial is a regular file, directory, or symlink. NotSpecial SpecialFileType = iota - // NamedPipe is a FIFO. The only SpecialFileType ApplySpecialFile - // actually recreates - see its doc comment for why. + // NamedPipe is a FIFO, the only SpecialFileType ApplySpecialFile actually recreates. NamedPipe // Socket is a Unix domain socket file. Socket @@ -26,13 +24,7 @@ const ( BlockDevice ) -// ClassifySpecialFile reports which SpecialFileType, if any, entry's Mode -// represents. fs.FileMode's type bits are portable Go constants with the -// same meaning regardless of host OS, even though most of them (the -// device/socket bits especially) only ever actually get set by a real -// Lstat on POSIX - so this classification itself needs no build tag, even -// though actually recreating some of these types does (see -// specialfiles_unix.go / specialfiles_windows.go). +// ClassifySpecialFile reports which SpecialFileType, if any, entry's Mode represents. func ClassifySpecialFile(entry FileEntry) SpecialFileType { switch { case entry.Mode&fs.ModeNamedPipe != 0: @@ -40,10 +32,9 @@ func ClassifySpecialFile(entry FileEntry) SpecialFileType { case entry.Mode&fs.ModeSocket != 0: return Socket case entry.Mode&fs.ModeCharDevice != 0: - // Must be checked before the bare ModeDevice case below: Go's - // fs.FileMode convention sets ModeCharDevice as a modifier on - // top of ModeDevice for character devices, so a character - // device has *both* bits set, not ModeCharDevice alone. + // Must be checked before the bare ModeDevice case: Go sets + // ModeCharDevice as a modifier on top of ModeDevice, so a character + // device has both bits set, not ModeCharDevice alone. return CharDevice case entry.Mode&fs.ModeDevice != 0: return BlockDevice @@ -54,27 +45,15 @@ func ClassifySpecialFile(entry FileEntry) SpecialFileType { // ErrSpecialFileUnsupported is returned by ApplySpecialFile for socket, // character device, and block device entries: none of these are actually -// recreated by this package. -// -// Character/block device nodes need syscall.Mknod, which requires -// CAP_MKNOD/root on Linux (and has no meaningful equivalent to attempt on -// Windows at all) - untestable without that privilege, and attempting it -// anyway would just fail for most callers while quietly calling that -// "support". A Unix domain socket file without a live listening process -// behind it also isn't meaningfully equivalent to the original; simply -// creating an inert socket-typed filesystem node isn't the same thing as -// preserving a socket. Rather than do either job partially, this is -// scoped down to detecting these types (via ClassifySpecialFile) but not -// recreating them - only named pipes (FIFOs) are actually created, since -// syscall.Mkfifo needs no elevated privilege and is fully testable. +// recreated by this package. Device nodes need privileged syscall.Mknod, and +// an inert socket-typed node isn't a meaningful stand-in for a live socket - +// only named pipes (via syscall.Mkfifo) are unprivileged and worth creating. var ErrSpecialFileUnsupported = errors.New("special file type is detected but not recreated by this package") // ApplySpecialFile creates destPath as the special file entry represents, -// for the one type this package actually supports creating: named pipes. -// For Socket/CharDevice/BlockDevice it returns an error wrapping -// ErrSpecialFileUnsupported. For NotSpecial (a regular file, directory, -// or symlink), it also returns an error: this function is only meaningful -// for an entry that actually is one of the special types above. +// for the one type this package supports creating: named pipes. For +// Socket/CharDevice/BlockDevice it returns an error wrapping +// ErrSpecialFileUnsupported; for NotSpecial it also errors. func ApplySpecialFile(destPath string, entry FileEntry) error { switch ClassifySpecialFile(entry) { case NamedPipe: diff --git a/internal/sync/specialfiles_windows.go b/internal/sync/specialfiles_windows.go index 5aadfe3..59ac309 100644 --- a/internal/sync/specialfiles_windows.go +++ b/internal/sync/specialfiles_windows.go @@ -7,11 +7,9 @@ import ( "io/fs" ) -// applyNamedPipe always fails on Windows: syscall.Mkfifo doesn't exist -// there. Windows named pipes are a distinct IPC mechanism under \\.\pipe\ -// rather than a filesystem-node type created at an arbitrary path the way -// a POSIX FIFO is, so there's no equivalent "create this path as a named -// pipe" operation to perform here at all. +// applyNamedPipe always fails on Windows: named pipes there live under +// \\.\pipe\ as a distinct IPC mechanism, not a filesystem-node type +// creatable at an arbitrary path the way a POSIX FIFO is. func applyNamedPipe(destPath string, _ fs.FileMode) error { return fmt.Errorf("creating a named pipe at %q: not supported on Windows", destPath) } diff --git a/internal/sync/uidgid_unix.go b/internal/sync/uidgid_unix.go index 77cc408..18ec9f7 100644 --- a/internal/sync/uidgid_unix.go +++ b/internal/sync/uidgid_unix.go @@ -7,11 +7,9 @@ import ( "syscall" ) -// lookupUIDGID extracts the owning user/group ID from a Lstat'd -// os.FileInfo. On POSIX platforms this is available via the -// syscall.Stat_t underlying the info; ok is false only if the platform's -// Sys() doesn't actually return that type (unexpected, but Sys() is -// documented as platform-dependent, so it's not guaranteed). +// lookupUIDGID extracts the owning user/group ID from a Lstat'd os.FileInfo +// via its syscall.Stat_t. ok is false only if the platform's Sys() doesn't +// return that type. func lookupUIDGID(info os.FileInfo) (uid, gid uint32, ok bool) { stat, ok := info.Sys().(*syscall.Stat_t) if !ok { diff --git a/internal/sync/walk.go b/internal/sync/walk.go index d80f008..cfe66a5 100644 --- a/internal/sync/walk.go +++ b/internal/sync/walk.go @@ -1,6 +1,5 @@ -// Package sync builds the file list ("flist") that grsync compares between -// source and destination before any data transfer happens - the same -// planning phase upstream rsync performs before its delta algorithm runs. +// Package sync builds the file list that grsync compares between source and +// destination before any data transfer happens. package sync import ( @@ -12,11 +11,8 @@ import ( ) // FileEntry describes a single file, directory, or symlink discovered under -// a source root. Path is always relative to that root, using "/" as the -// separator regardless of host OS - rsync's wire protocol and file lists -// are always "/"-separated, and grsync targets protocol-level -// interoperability, so paths are normalized at collection time rather than -// left OS-native and converted later. +// a source root. Path is always relative to that root and "/"-separated +// regardless of host OS, matching rsync's wire protocol. type FileEntry struct { Path string Size int64 @@ -24,10 +20,9 @@ type FileEntry struct { Mode fs.FileMode UID uint32 GID uint32 - // OwnershipAvailable reports whether UID/GID were actually populated. - // A real uid/gid of 0 (root) is a valid value, so callers must check - // this rather than treating a zero UID/GID as "unavailable" - see - // uidgid_windows.go, where it is always false. + // OwnershipAvailable reports whether UID/GID were actually populated: a + // real uid/gid of 0 (root) is valid, so a zero value alone doesn't mean + // "unavailable" (see uidgid_windows.go, where this is always false). OwnershipAvailable bool LinkTarget string IsDir bool @@ -36,24 +31,19 @@ type FileEntry struct { // WalkOptions controls how far Walk descends, mirroring rsync's own // -r/--recursive and -d/--dirs flags: // -// - Recursive=false, Dirs=false (rsync's default with neither flag): -// directories are skipped entirely - not listed, not descended into. -// Only regular files/symlinks directly under root are collected. -// - Recursive=false, Dirs=true (-d): directories are listed (so they can -// be created on the receiving end) but their contents are not - Walk -// does not descend into them. -// - Recursive=true (-r): full recursion into every subdirectory, -// regardless of Dirs - this matches rsync, where -r makes -d redundant. +// - Recursive=false, Dirs=false: directories are skipped entirely (rsync's +// default with neither flag). +// - Recursive=false, Dirs=true (-d): directories are listed but not +// descended into. +// - Recursive=true (-r): full recursion regardless of Dirs, since -r makes +// -d redundant. type WalkOptions struct { Recursive bool 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. +// info, leaving Path unset for the caller to fill in. func buildFileEntry(path string, info fs.FileInfo) (FileEntry, error) { entry := FileEntry{ Size: info.Size(), @@ -62,17 +52,10 @@ func buildFileEntry(path string, info fs.FileInfo) (FileEntry, error) { 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. + // info.Mode()&fs.ModeSymlink is only ever set by Lstat, not Stat - every + // caller of this function must Lstat, or this branch is unreachable. if info.Mode()&fs.ModeSymlink != 0 { target, err := os.Readlink(path) if err != nil { @@ -84,17 +67,10 @@ func buildFileEntry(path string, info fs.FileInfo) (FileEntry, error) { 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. +// LstatEntry builds a FileEntry for a single existing path the same way Walk +// builds one for each path it visits, without needing a whole tree walk. +// Path is left empty. The returned error satisfies os.IsNotExist(err) when +// path doesn't exist, exactly like a direct os.Lstat call would. func LstatEntry(path string) (FileEntry, error) { info, err := os.Lstat(path) if err != nil { @@ -114,14 +90,10 @@ func Walk(root string, opts WalkOptions) ([]FileEntry, error) { return err } if path == root { - // The root itself isn't part of its own file list. return nil } if !opts.Recursive && d.IsDir() && !opts.Dirs { - // Neither -r nor -d: rsync skips directories entirely rather - // than creating them empty, so this one is neither listed nor - // descended into. return filepath.SkipDir } @@ -131,11 +103,7 @@ func Walk(root string, opts WalkOptions) ([]FileEntry, error) { } // os.Lstat, not os.Stat: a symlink must be reported as itself, not - // resolved to whatever it points at. WalkDir's own DirEntry is - // already lstat-derived (it never follows symlinks when deciding - // what to recurse into), but calling Lstat explicitly here makes - // that guarantee obvious at the call site rather than implicit in - // WalkDir's internals. + // resolved to whatever it points at. info, err := os.Lstat(path) if err != nil { return err @@ -150,8 +118,6 @@ func Walk(root string, opts WalkOptions) ([]FileEntry, error) { entries = append(entries, entry) if !opts.Recursive && d.IsDir() { - // Whether or not -d caused this directory to be listed above, - // without -r its contents are never descended into. return filepath.SkipDir } @@ -161,13 +127,10 @@ func Walk(root string, opts WalkOptions) ([]FileEntry, error) { return nil, err } - // filepath.WalkDir only guarantees lexicographic order within each - // directory, not across the whole tree (a directory's contents are - // interleaved with sibling entries in traversal order, not sorted - // globally). Sorting the flattened list by full path afterward is what - // actually gives deterministic, rsync-comparable ordering; plain Go - // string comparison is byte-wise, matching C's strcmp, which is what - // rsync's own pathname sort relies on. + // filepath.WalkDir only guarantees order within each directory, not + // across the whole tree, so sort the flattened list by full path. Go's + // byte-wise string comparison matches C's strcmp, which rsync's own + // pathname sort relies on. sort.Slice(entries, func(i, j int) bool { return entries[i].Path < entries[j].Path }) diff --git a/internal/sync/walk_test.go b/internal/sync/walk_test.go index a550ecd..1ee3c1e 100644 --- a/internal/sync/walk_test.go +++ b/internal/sync/walk_test.go @@ -58,9 +58,6 @@ func TestWalk_Symlink(t *testing.T) { linkPath := filepath.Join(root, "link.txt") if err := os.Symlink("real.txt", linkPath); err != nil { - // Creating symlinks on Windows requires Developer Mode or an - // elevated process; treat that as "unsupported here", not a - // failure, rather than forcing every environment to be elevated. t.Skipf("symlink creation unsupported in this environment: %v", err) } @@ -89,18 +86,13 @@ func TestWalk_Symlink(t *testing.T) { t.Errorf("link.txt: LinkTarget = %q, want %q", link.LinkTarget, "real.txt") } - // The symlink must not have been followed: its own Size should reflect - // the length of the link text, not the 5-byte target file's contents. + // Must not have been followed: Size should reflect the link text, not + // the 5-byte target file. if link.Size == 5 { t.Errorf("link.txt: Size = %d looks like the resolved target's size, want the symlink's own size", link.Size) } } -// 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") @@ -210,8 +202,6 @@ func TestWalk_OwnershipAvailable(t *testing.T) { } entry := entries[0] - // Windows has no POSIX uid/gid concept; every other platform this repo - // builds for does (see uidgid_unix.go / uidgid_windows.go). wantAvailable := runtime.GOOS != "windows" if entry.OwnershipAvailable != wantAvailable { t.Errorf("OwnershipAvailable = %v, want %v (GOOS=%s)", entry.OwnershipAvailable, wantAvailable, runtime.GOOS) @@ -258,9 +248,7 @@ func TestWalk_VaryingFileSizes(t *testing.T) { func TestWalk_DeterministicSortOrder(t *testing.T) { root := t.TempDir() - // Create files in an order that does not match their sorted order, so a - // pass would only happen if Walk actually sorts rather than incidentally - // returning creation order or directory-read order. + // Created out of sorted order so a pass requires Walk to actually sort. for _, name := range []string{"charlie.txt", "alpha.txt", "bravo.txt", "delta.txt"} { mustWriteFile(t, filepath.Join(root, name), name) } diff --git a/internal/transport/frame.go b/internal/transport/frame.go index f61ba1a..1eb9b59 100644 --- a/internal/transport/frame.go +++ b/internal/transport/frame.go @@ -6,44 +6,26 @@ import ( "io" ) -// FrameType tags what kind of message a Frame carries, so a single -// stdin/stdout byte stream can multiplex different message kinds (file -// list entries, signatures, delta ops, control messages) without a -// separate channel for each. +// FrameType tags what kind of message a Frame carries. type FrameType byte const ( - // FrameHello and FrameHelloAck are the minimal --server handshake: - // the client sends FrameHello, the server replies with FrameHelloAck. + // FrameHello is the client's handshake greeting. FrameHello FrameType = iota // FrameHelloAck is the server's reply to FrameHello. FrameHelloAck - // FrameError carries a human-readable error message from one side to - // the other, rather than the connection just dying silently. + // FrameError carries a human-readable error message from one side to the other. FrameError - // FrameFileList carries a gob-encoded []sync.FileEntry, sent once by - // the sender after a successful handshake: the filtered list of - // everything it intends to sync. internal/pipeline owns the encoding; - // this package only tags and frames the bytes. + // FrameFileList carries a gob-encoded []sync.FileEntry. FrameFileList - // FrameSignature carries a gob-encoded per-file signature, sent by - // the receiver for each regular-file entry in the list it received - - // proactively, in list order, not in response to a separate request - // message (the file list itself is the implicit request for all of - // them). + // FrameSignature carries a gob-encoded per-file signature. FrameSignature - // FrameDelta carries a gob-encoded per-file delta (the sender's reply - // to a FrameSignature), in the same list order. + // FrameDelta carries a gob-encoded per-file delta. FrameDelta ) -// maxFramePayload bounds how large a single frame's payload may be. A -// length prefix isn't validated against anything else in this protocol -// (there's no higher-level "expected size" to check it against), so -// without a cap, a corrupted stream or a hostile peer could send a -// length like 0xFFFFFFFF and force ReadFrame to attempt a multi-gigabyte -// allocation before ever reading a single payload byte. 64 MiB is -// comfortably larger than any frame type this ticket defines needs. +// maxFramePayload bounds a frame's payload size so a corrupted or hostile +// length prefix can't force an oversized allocation before any data is read. const maxFramePayload = 64 * 1024 * 1024 // Frame is a single multiplexed protocol message. diff --git a/internal/transport/frame_test.go b/internal/transport/frame_test.go index eca3df8..3ee9c71 100644 --- a/internal/transport/frame_test.go +++ b/internal/transport/frame_test.go @@ -62,9 +62,6 @@ func TestWriteReadFrame_MultipleFramesInSequence(t *testing.T) { } } - // The stream must be fully consumed - proves frame boundaries were - // tracked correctly rather than one frame's read accidentally - // consuming into the next frame's bytes (or leaving some behind). if buf.Len() != 0 { t.Errorf("%d bytes left unread after consuming all frames", buf.Len()) } diff --git a/internal/transport/fuzz_test.go b/internal/transport/fuzz_test.go index eba12ab..f73f090 100644 --- a/internal/transport/fuzz_test.go +++ b/internal/transport/fuzz_test.go @@ -5,19 +5,8 @@ import ( "testing" ) -// FuzzReadFrame is a bonus SC-15 target beyond the ticket's own three -// named ones: the literal "binary parsing of untrusted input" its own -// rationale describes, one layer below internal/daemon's text-based -// greeting/auth lines - every transport (local pipe, SSH session, and a -// daemon connection once its own text handshake ends) reads its actual -// payload frames through this exact function. The property checked is -// ReadFrame's own documented safety guarantee: it never panics, and it -// never attempts to allocate a payload larger than maxFramePayload, -// regardless of what bytes a corrupted stream or hostile peer sends - -// precisely the protection SC-13's own TestE2E_ReadBatchOfMalformedFileFailsClearly -// found catching a garbage --read-batch file cleanly rather than -// crashing; this fuzz target explores far beyond that one hand-picked -// case. +// FuzzReadFrame checks that ReadFrame never panics or over-allocates +// beyond maxFramePayload on a corrupted or hostile stream. func FuzzReadFrame(f *testing.F) { var valid bytes.Buffer _ = WriteFrame(&valid, Frame{Type: FrameFileList, Payload: []byte("hello")}) diff --git a/internal/transport/handshake.go b/internal/transport/handshake.go index 45162a4..ed56c74 100644 --- a/internal/transport/handshake.go +++ b/internal/transport/handshake.go @@ -5,21 +5,13 @@ import ( "io" ) -// ProtocolVersion identifies this ticket's minimal handshake protocol. -// Bump it if the frame types or handshake sequence ever change -// incompatibly, so an old client/server pair fails the version check -// below instead of misinterpreting each other's frames. +// ProtocolVersion identifies the handshake protocol version. Bump it if the +// frame types or handshake sequence ever change incompatibly. const ProtocolVersion = 1 -// ServeHandshake implements grsync's --server-mode entry point for this -// ticket's scope: read one FrameHello from r, verify its protocol -// version, and reply with one FrameHelloAck on w. -// -// This proves the subprocess/pipe/framing machinery works end to end -// through a real remote-shell connection - it is deliberately not a full -// sync server. Wiring an actual file-list/signature/delta exchange on top -// of this frame/session foundation is later, separately-scoped work (see -// README's note on this). +// ServeHandshake implements the server side of the handshake: it reads one +// FrameHello from r, verifies its protocol version, and replies with one +// FrameHelloAck on w. func ServeHandshake(r io.Reader, w io.Writer) error { f, err := ReadFrame(r) if err != nil { @@ -35,10 +27,8 @@ func ServeHandshake(r io.Reader, w io.Writer) error { return WriteFrame(w, Frame{Type: FrameHelloAck, Payload: []byte{ProtocolVersion}}) } -// Handshake performs the client side of ServeHandshake over rw: sends -// FrameHello, then reads back FrameHelloAck, confirming the round trip -// actually completed rather than just assuming the connection is good -// because Dial succeeded. +// Handshake performs the client side of ServeHandshake: it sends FrameHello +// then reads back FrameHelloAck. func Handshake(rw io.ReadWriter) error { if err := WriteFrame(rw, Frame{Type: FrameHello, Payload: []byte{ProtocolVersion}}); err != nil { return fmt.Errorf("sending hello: %w", err) diff --git a/internal/transport/handshake_test.go b/internal/transport/handshake_test.go index bfd85c8..0fb431d 100644 --- a/internal/transport/handshake_test.go +++ b/internal/transport/handshake_test.go @@ -5,18 +5,13 @@ import ( "testing" ) -// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter, so -// Handshake (which needs one bidirectional stream, like a real Session) -// can be driven against an in-memory pipe instead of a real subprocess. +// pipeReadWriter joins two io.Pipe halves into a single io.ReadWriter. type pipeReadWriter struct { io.Reader io.Writer } func TestHandshake_PureLogic(t *testing.T) { - // Two independent pipes, one per direction, wired crosswise so each - // side's writes become the other side's reads - simulating a real - // bidirectional Session without spawning a subprocess at all. clientReadsFromServer, serverWritesToClient := io.Pipe() serverReadsFromClient, clientWritesToServer := io.Pipe() @@ -66,9 +61,7 @@ func TestHandshake_RejectsWrongFrameType(t *testing.T) { _ = w.Close() }() - // Handshake's own outgoing FrameHello is simply discarded here - this - // test only cares what happens when the response it reads back isn't - // a FrameHelloAck. + // Outgoing FrameHello is discarded; only the response matters here. client := pipeReadWriter{Reader: r, Writer: io.Discard} if err := Handshake(client); err == nil { t.Fatalf("Handshake with a non-HelloAck frame returned nil error, want an error") diff --git a/internal/transport/integration_test.go b/internal/transport/integration_test.go index afe9f03..da1e0f1 100644 --- a/internal/transport/integration_test.go +++ b/internal/transport/integration_test.go @@ -9,21 +9,10 @@ import ( "time" ) -// requireLocalSSHServer skips the calling test unless a real SSH server -// is actually reachable at 127.0.0.1 for the current user, non- -// interactively. This is a capability probe, not an assumption: most -// dev machines and CI runners have an ssh *client* installed but no -// sshd listening, so skipping here (rather than failing) is the normal, -// expected outcome almost everywhere this runs - the same pattern -// TestWalk_Symlink (internal/sync) and the Lchown/Mkfifo tests -// (internal/sync/attributes_test.go, specialfiles_test.go) already use -// for privilege/platform-gated behavior. -// -// 127.0.0.1 is used rather than "localhost": in at least this project's -// own Windows dev environment, the bundled ssh client failed to resolve -// "localhost" at all (a local DNS/hosts quirk, not an SSH problem) before -// ever getting to the real "is anything listening" question - the IP -// literal sidesteps that unrelated failure mode entirely. +// requireLocalSSHServer skips the test unless an SSH server is reachable +// at 127.0.0.1 non-interactively (most dev/CI machines have a client but +// no sshd running). 127.0.0.1 is used instead of "localhost" to avoid a +// local DNS resolution quirk seen on this project's Windows dev machine. func requireLocalSSHServer(t *testing.T) { t.Helper() cmd := exec.Command("ssh", @@ -35,10 +24,7 @@ func requireLocalSSHServer(t *testing.T) { } } -// buildGrsyncBinary compiles cmd/grsync fresh into a temp file and -// returns its path, so this test exercises the real --server flag wiring -// in internal/cli/root.go exactly as a real invocation would, rather than -// some test-only stand-in for it. +// buildGrsyncBinary compiles cmd/grsync into a temp binary and returns its path. func buildGrsyncBinary(t *testing.T) string { t.Helper() diff --git a/internal/transport/remotepath.go b/internal/transport/remotepath.go index 5662424..538feb2 100644 --- a/internal/transport/remotepath.go +++ b/internal/transport/remotepath.go @@ -15,36 +15,16 @@ type RemotePath struct { Path string } -// ParseRemotePath reports whether s looks like a remote -// [user@]host:path (or IPv6 [user@][host]:path) endpoint rather than a -// local filesystem path, returning the parsed form when it does. +// ParseRemotePath reports whether s is a remote [user@]host:path (or IPv6 +// [user@][host]:path) endpoint rather than a local filesystem path, +// returning the parsed form when it does. // -// Disambiguation rule, in order: -// -// 0. A "://" anywhere in s (e.g. "rsync://host/module") is never this -// syntax at all - real [user@]host:path syntax never contains one, -// and without this check "rsync://host/module" would otherwise parse -// as host "rsync", path "//host/module", which is wrong in a way -// that's easy to miss (it "succeeds" instead of failing loudly). -// Checked alongside the Windows-drive-letter case below, before any -// of the numbered rules that follow ever run. -// 1. A single ASCII letter immediately followed by ":" (e.g. "C:", -// "C:\Users\...") is always a Windows drive letter, never a remote -// host - real single-letter hostnames in this position are -// vanishingly rare in practice, while grsync runs natively on -// Windows (unlike upstream rsync), so this ambiguity has to be -// resolved in favor of the overwhelmingly common case. -// 2. A "/" appearing before the separating ":" means this can't be -// [user@]host:path at all - no real hostname contains a "/", so -// finding one first proves whatever precedes the colon is a path -// segment, not a host (this also naturally handles a "user@" prefix -// that isn't really one, e.g. a local path that happens to contain -// "@"). -// 3. Otherwise, an "[...]" immediately after any "user@" prefix is an -// IPv6 literal - the separating ":" is the one right after the -// closing "]", not the first ":" in the string (an IPv6 address is -// full of colons itself). -// 4. Otherwise, the first ":" is the separator. +// A "://" anywhere disqualifies it (so rsync daemon URLs aren't +// misparsed as host "rsync"), a leading single-letter drive spec like +// "C:" is always treated as a Windows path, and a "/" before the +// separating ":" also disqualifies it since no real hostname contains +// one. IPv6 hosts must be bracketed, since the address itself is full of +// colons. func ParseRemotePath(s string) (RemotePath, bool) { if s == "" || isWindowsDriveLetterPath(s) || strings.Contains(s, "://") { return RemotePath{}, false diff --git a/internal/transport/rsh.go b/internal/transport/rsh.go index 3ee8ec3..76b1ea7 100644 --- a/internal/transport/rsh.go +++ b/internal/transport/rsh.go @@ -2,22 +2,10 @@ package transport import "strings" -// sshAddressFamilyFlag returns "-4" or "-6" to insert into the -// remote-shell argv when ipv4/ipv6 was requested AND program is -// genuinely ssh - real rsync's own exact behavior, verified against -// upstream source (main.c's do_cmd(): "if (default_af_hint == AF_INET -// && strcmp(t, "ssh") == 0) args[argc++] = "-4";", where t is the -// resolved remote-shell command's basename). Real rsync's own -4/-6 is -// never forwarded for any other remote shell - its own man page says so -// explicitly: "For other remote shells you'll need to specify '--rsh -// SHELL -4' directly" - so this returns "" (nothing to insert) for -// anything but ssh, rather than guessing at another program's own -// address-family flag syntax. -// -// program is compared by basename, not the full path, matching real -// rsync's own strrchr(cmd, '/')-based check - and additionally strips a -// trailing ".exe", which upstream's Unix-only C code never needs to but -// grsync does, running natively on Windows too. +// sshAddressFamilyFlag returns "-4" or "-6" to insert into the argv when +// ipv4/ipv6 was requested and the resolved program (matched by basename, +// ignoring a trailing ".exe") is ssh; it returns "" for any other remote +// shell, matching real rsync's own behavior. func sshAddressFamilyFlag(program string, ipv4, ipv6 bool) string { if !ipv4 && !ipv6 { return "" @@ -45,21 +33,9 @@ const DefaultRSH = "ssh" // ["ssh", "user@host", "grsync", "--server"]. // // rsh is the raw --rsh/-e override string (e.g. "ssh -p 2222 -i key.pem"), -// or empty to use DefaultRSH. This is deliberately the *only* -// customization mechanism here - there's no separate --port or -// --identity flag, matching real rsync: upstream's --port only applies to -// daemon-mode (rsync://) connections, not the remote-shell transport, and -// its -i flag already means --itemize-changes, not "identity file" - so -// port/identity/ProxyJump/etc. are customized via -e (e.g. -// -e "ssh -p 2222 -i key.pem") or the user's own ~/.ssh/config, exactly as -// with real rsync. Inventing grsync-only flags for these would be a -// *worse* match for SC-1's parity goal, not a better one. -// -// ipv4/ipv6 (--ipv4/--ipv6, SC-14) are the one deliberate exception to -// that "customize everything via -e" philosophy, because real rsync -// itself makes it one: see sshAddressFamilyFlag's own doc comment for -// exactly when -4/-6 does and doesn't get inserted here, verified -// against upstream's own source rather than assumed. +// or empty to use DefaultRSH; it's the only customization mechanism - +// there's no separate --port or --identity flag, matching real rsync. +// ipv4/ipv6 additionally insert -4/-6 per sshAddressFamilyFlag. func BuildRSHCommand(rsh, user, host string, remoteArgs []string, ipv4, ipv6 bool) []string { fields := splitRSHCommand(rsh) if len(fields) == 0 { @@ -82,18 +58,11 @@ func BuildRSHCommand(rsh, user, host string, remoteArgs []string, ipv4, ipv6 boo } // splitRSHCommand splits an --rsh/-e command string into argv-style -// fields, honoring single- and double-quoted substrings so an argument -// containing spaces (e.g. -e `ssh -o "ProxyCommand=nc %h %p"`) survives as -// one field instead of being split apart - strings.Fields alone would -// silently mis-split exactly that case, which is common enough in real -// -e usage to be worth handling. +// fields, honoring single- and double-quoted substrings so a quoted +// argument containing spaces survives as one field. // -// This is not a full POSIX shell parser: no backslash escaping, no -// nested/mixed quotes, no variable expansion - just enough for the common -// case of one quoted argument. An unterminated quote is not treated as an -// error; whatever follows the opening quote to the end of the string -// becomes that field's content, which is a reasonable low-stakes fallback -// for malformed CLI input rather than something worth failing on. +// This is not a full shell parser: no escaping, no nested quotes, and an +// unterminated quote just runs to the end of the string rather than erroring. func splitRSHCommand(s string) []string { var fields []string var current strings.Builder diff --git a/internal/transport/rsh_test.go b/internal/transport/rsh_test.go index 94a3530..6daa838 100644 --- a/internal/transport/rsh_test.go +++ b/internal/transport/rsh_test.go @@ -46,12 +46,6 @@ func TestBuildRSHCommand_QuotedArgumentSurvivesAsOneField(t *testing.T) { } } -// TestBuildRSHCommand_IPv4ForwardedToDefaultSSH is SC-14's core proof: -// with no --rsh override at all (the default "ssh" is used), --ipv4 -// must insert a real "-4" into the ssh argv, right before the target - -// matching real rsync's own exact insertion point (main.c's do_cmd(), -// verified against upstream source: args[argc++] = "-4"; happens -// immediately before args[argc++] = machine;). func TestBuildRSHCommand_IPv4ForwardedToDefaultSSH(t *testing.T) { got := BuildRSHCommand("", "", "example.com", []string{"grsync", "--server"}, true, false) want := []string{"ssh", "-4", "example.com", "grsync", "--server"} @@ -68,11 +62,6 @@ func TestBuildRSHCommand_IPv6ForwardedToDefaultSSH(t *testing.T) { } } -// TestBuildRSHCommand_IPv4ForwardedWhenRSHOverrideIsStillSSH confirms -// the forwarding survives an --rsh override, as long as the resolved -// program is still genuinely ssh (e.g. "ssh -p 2222 -i key.pem") - -// real rsync's own check is on the resolved program's basename, not on -// whether --rsh/-e was given at all. func TestBuildRSHCommand_IPv4ForwardedWhenRSHOverrideIsStillSSH(t *testing.T) { got := BuildRSHCommand("ssh -p 2222 -i key.pem", "alice", "example.com", []string{"grsync", "--server"}, true, false) want := []string{"ssh", "-p", "2222", "-i", "key.pem", "-4", "alice@example.com", "grsync", "--server"} @@ -81,11 +70,6 @@ func TestBuildRSHCommand_IPv4ForwardedWhenRSHOverrideIsStillSSH(t *testing.T) { } } -// TestBuildRSHCommand_NotForwardedForNonSSHRemoteShell is real rsync's -// own documented limit made concrete: "For other remote shells you'll -// need to specify '--rsh SHELL -4' directly" - so --ipv4/--ipv6 must be -// silently NOT forwarded when the remote shell isn't ssh, rather than -// guessing at some other program's own address-family flag syntax. func TestBuildRSHCommand_NotForwardedForNonSSHRemoteShell(t *testing.T) { got := BuildRSHCommand("rsh", "", "example.com", []string{"grsync", "--server"}, true, false) want := []string{"rsh", "example.com", "grsync", "--server"} @@ -94,9 +78,6 @@ func TestBuildRSHCommand_NotForwardedForNonSSHRemoteShell(t *testing.T) { } } -// TestBuildRSHCommand_IPv4ForwardedForFullSSHPath confirms the basename -// check, not an exact-string check: an --rsh override giving ssh's full -// path must still be recognized as ssh. func TestBuildRSHCommand_IPv4ForwardedForFullSSHPath(t *testing.T) { got := BuildRSHCommand("/usr/bin/ssh", "", "example.com", nil, true, false) want := []string{"/usr/bin/ssh", "-4", "example.com"} @@ -105,9 +86,6 @@ func TestBuildRSHCommand_IPv4ForwardedForFullSSHPath(t *testing.T) { } } -// TestBuildRSHCommand_IPv4ForwardedForWindowsSSHExe confirms the ".exe" -// stripping grsync's own cross-platform (native Windows) support needs -// that real rsync's Unix-only C code never had to handle. func TestBuildRSHCommand_IPv4ForwardedForWindowsSSHExe(t *testing.T) { got := BuildRSHCommand(`C:\Windows\System32\OpenSSH\ssh.exe`, "", "example.com", nil, true, false) want := []string{`C:\Windows\System32\OpenSSH\ssh.exe`, "-4", "example.com"} diff --git a/internal/transport/session.go b/internal/transport/session.go index ec41587..f1882b2 100644 --- a/internal/transport/session.go +++ b/internal/transport/session.go @@ -10,9 +10,8 @@ import ( ) // Session wraps a running remote-shell subprocess (e.g. ssh), exposing -// its stdin/stdout as a single Read/Write pair so the framed protocol -// (frame.go) can be layered directly on top without the caller needing -// to know this is a subprocess at all. +// its stdin/stdout as a single Read/Write pair so the framed protocol can +// be layered directly on top. type Session struct { cmd *exec.Cmd stdin io.WriteCloser @@ -20,26 +19,13 @@ type Session struct { stderr *bytes.Buffer } -// 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 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. +// Dial spawns the remote-shell command built by BuildRSHCommand and +// returns a Session wrapping its stdin/stdout. The subprocess's stderr is +// captured (to enrich Close's error on a non-zero exit) and also passed +// through live to this process's own stderr. // -// Host-key verification is never touched here: this deliberately never -// adds flags like "-o StrictHostKeyChecking=no" or a null -// UserKnownHostsFile. Whatever the invoked command (ssh by default) does -// by default - checking known_hosts, prompting or failing on an unknown -// or changed host key - is exactly what happens, unmodified. There is no -// stubbed-out or weakened host-key behavior to document here because none -// of that logic is reimplemented at all; it's entirely the system ssh -// client's own, unchanged behavior. +// Host-key verification is left entirely to the invoked command (ssh by +// default); no StrictHostKeyChecking or known_hosts flags are added here. func Dial(rsh, user, host string, remoteArgs []string, ipv4, ipv6 bool) (*Session, error) { argv := BuildRSHCommand(rsh, user, host, remoteArgs, ipv4, ipv6) cmd := exec.Command(argv[0], argv[1:]...) @@ -50,14 +36,8 @@ func Dial(rsh, user, host string, remoteArgs []string, ipv4, ipv6 bool) (*Sessio } stdout, err := cmd.StdoutPipe() if err != nil { - // StdinPipe already succeeded above, and Start is never reached - // on this path - so nothing will ever close that pipe's file - // descriptor automatically (that's normally Cmd.Wait's job, and - // Wait only does it once Start has actually run). Verified - // against golang/go#58369, which documents this exact gap: Start - // itself failing *does* clean up already-created pipes, but never - // reaching Start at all does not. Close stdin explicitly here - // rather than leak it. + // Start is never reached on this path, so stdin's pipe won't be + // closed automatically by cmd.Wait; close it explicitly. _ = stdin.Close() return nil, fmt.Errorf("creating stdout pipe: %w", err) } @@ -66,9 +46,7 @@ func Dial(rsh, user, host string, remoteArgs []string, ipv4, ipv6 bool) (*Sessio cmd.Stderr = io.MultiWriter(&stderr, os.Stderr) if err := cmd.Start(); err != nil { - // Unlike the StdoutPipe case above, Start failing here *does* - // close both already-created pipes as part of its own documented - // cleanup (golang/go#58369) - nothing further to release. + // Start failing here already closes both pipes; nothing to clean up. return nil, fmt.Errorf("starting %q: %w", argv[0], err) } @@ -82,18 +60,9 @@ func (s *Session) Read(p []byte) (int, error) { return s.stdout.Read(p) } func (s *Session) Write(p []byte) (int, error) { return s.stdin.Write(p) } // Close closes the subprocess's stdin (signaling EOF to the remote side) -// and waits for it to exit. -// -// stdout is deliberately not closed here: cmd.Wait's own documentation -// states it closes any pipe created via StdoutPipe automatically once the -// command exits, and that closing it earlier is incorrect if reads from -// it haven't all completed yet - so an explicit s.stdout.Close() here -// would either be redundant or actively wrong, depending on timing. -// -// If the process exited with an error, that error is enriched with -// whatever the subprocess wrote to stderr: a bare "exit status 255" is -// nearly useless without the diagnostic message ssh itself printed -// explaining why (unknown host, auth failure, connection refused, ...). +// and waits for it to exit; stdout is left for cmd.Wait to close itself. +// If the process exited with an error, stderr output is appended for a +// more useful message than a bare exit code. func (s *Session) Close() error { stdinErr := s.stdin.Close() waitErr := s.cmd.Wait()