feat(copy): use rsync by default with scp fallback#297
feat(copy): use rsync by default with scp fallback#297immanuel-peter wants to merge 3 commits intobrevdev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the brev copy command to prefer rsync for transfers (for better incremental performance) while preserving compatibility by falling back to scp when rsync fails.
Changes:
- Switch transfer execution to attempt
rsyncfirst with automaticscpfallback. - Factor transfer argument construction into helper functions for
rsync/scp. - Add unit tests covering argument construction and fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/cmd/copy/copy.go |
Implements rsync-first transfer strategy with scp fallback and refactors argument building. |
pkg/cmd/copy/copy_test.go |
Adds tests for rsync/scp arg construction and fallback execution behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cmd/copy/copy.go
Outdated
|
|
||
| startTime := time.Now() | ||
| func combinedOutputRunner(name string, args ...string) ([]byte, error) { | ||
| cmd := exec.Command(name, args...) //nolint:gosec |
There was a problem hiding this comment.
//nolint:gosec here suppresses a security linter finding without any justification. Elsewhere in the codebase, nolint:gosec is consistently annotated with a short reason (e.g., agentskill.go uses //nolint:gosec // skill files are not sensitive). Add a brief rationale here (and, if applicable, note what input validation makes this safe) to avoid masking real issues in future changes.
| cmd := exec.Command(name, args...) //nolint:gosec | |
| cmd := exec.Command(name, args...) //nolint:gosec // name is restricted to rsync/scp and args are validated paths, so command injection risk is controlled |
pkg/cmd/copy/copy.go
Outdated
| startTime := time.Now() | ||
| fellBack, err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner) | ||
| if fellBack { | ||
| t.Vprint(t.Yellow("rsync failed, falling back to scp...\n")) | ||
| } |
There was a problem hiding this comment.
The fallback status message is printed after transferWithFallback returns, which means the user will see "falling back to scp..." only after scp has already completed (or failed). Consider moving the rsync/scp orchestration into runCopyWithFallback (or providing a callback from transferWithFallback) so the message is emitted immediately before invoking scp.
pkg/cmd/copy/copy.go
Outdated
|
|
||
| scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) | ||
| if scpErr != nil { | ||
| return true, fmt.Errorf("rsync failed: %v\nscp fallback failed: %w", err, scpErr) |
There was a problem hiding this comment.
transferWithFallback wraps the rsync error with an additional "rsync failed:" prefix even though runRsyncCommand already returns an error starting with "rsync failed:". This results in duplicated prefixes (e.g., rsync failed: rsync failed: ...) and makes the combined error harder to read. Consider either removing the prefix from runRsyncCommand/runSCPCommand or changing this formatting to avoid repeating it (and keep the output sections intact).
| return true, fmt.Errorf("rsync failed: %v\nscp fallback failed: %w", err, scpErr) | |
| return true, fmt.Errorf("%v\nscp fallback failed: %w", err, scpErr) |
Summary
Why rsync first
rsync is generally better for repeated and large transfers because it supports incremental/delta transfer, can reduce bytes sent, and is typically faster and more bandwidth-efficient than scp for changed files/directories.
Compatibility
We keep scp as a fallback path so environments that lack working rsync still succeed with the previous transfer mechanism.
Verification
go test ./pkg/cmd/copygo test ./pkg/cmd/...