Skip to content

Commit 692bffa

Browse files
ralyodioclaude
andcommitted
fix: a manual sync transfers what you selected, and asks first
Selecting two folders on a server and pressing Sync to Local copied the whole directory. Two movies became forty thousand files. The panes have always let you select entries. `page.tsx` built its request from `source.path` and never read `state.selected`, so the selection reached the counter in the pane footer and nothing else. Every transfer was the whole directory, every time. A second bug in the same click, found on the way. The rail sets the direction and starts the transfer in one handler: onDirection('rtl') onRun() `setDirection` does not change the value the current render closed over, so `run` built its request from the PREVIOUS direction. Pressing the arrow that was not armed transferred the opposite way: "Sync to Local" copied local over the server. With Mirror armed it would have deleted the wrong side. The direction is now passed to the run rather than read back out of state. Selections now become an rsync `--files-from` list. The trap there, confirmed against rsync 3.4.1 rather than assumed: rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/ rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/ >f+++++++++ movieA/a.mkv `--files-from` switches recursion off and `--archive` does not switch it back on, so a selected folder would have copied as an empty folder and reported success. `buildRsyncArgs` now forces `--recursive` whenever a file list is in play. The list is NUL-separated (`--from0`), because a newline is legal in a filename and a line-separated list would split one name into two paths that do not exist. Mirror plus a selection deletes only inside what was selected. Verified, not assumed: the other reading of `--delete` here would empty the destination. Everything previews and is approved now, mirror or not. Both the desktop and the CLI skipped the dry run for a plain sync, on the reasoning that it "buys no safety, because nothing is deleted either way". Deleting is not the only way to regret a transfer, and this bug is the proof: by the time anything was on screen it was already copying. The dialog says which it is, Sync or Mirror, and carries the scope ("2 selected items" / "Whole folder") next to the counts, because a count alone never says how many of what you asked for. Confirming starts the request that was measured, not one rebuilt from whatever the panes say by then. All three interfaces: - Desktop: the pane selection is sent, validated as entry names rather than paths. That list becomes a `--files-from` rooted at the source directory, so a renderer able to put `../` in it would be choosing which files leave the machine. - CLI: `--only NAME`, repeatable, and a plain sync now previews and asks. Only where somebody is there to answer: a pipe, `--non-interactive` or `--yes` proceeds as before, so scheduled profile runs keep working. `--no-preview` restores the old skip-the-scan behaviour. - TUI: it had a cursor and no way to select at all, and `s` started an immediate transfer of everything on screen. Space marks entries, `s` scans and asks before it moves anything. Two smaller things this turned up: - `--only` did not parse. It was missing from VALUE_FLAGS, so it consumed no value, the names fell through to the positionals, and the flag silently did nothing. Caught by running the built binary, not by reading it. - The preview dialog's "Trust this pair from now on" checkbox is gone. It set renderer state that nothing read, and could not have worked if wired: saveProfile hard-codes `trustDeletes: false` on purpose, because unattended mirroring is the one way a delete list runs with nobody looking at it. Verified against the real rsync and the real dev server. `--only movieA --only movieB` over ssh: 4 files, not 7. The desktop driven in headless Chromium under the app's real CSP: the preview request carries the ssh source, both names, and does not start until approved. The TUI driven in a pty: marks render, the confirm appears, `n` transfers nothing, `y` copies the two marked folders with their contents and leaves the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52
1 parent cd820b6 commit 692bffa

13 files changed

Lines changed: 839 additions & 102 deletions

File tree

‎apps/cli/src/commands/transfer.ts‎

Lines changed: 73 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
import { randomUUID } from 'node:crypto'
2+
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
3+
import { tmpdir } from 'node:os'
4+
import { join } from 'node:path'
25
import { createInterface } from 'node:readline/promises'
36
import type { DiskPushStore } from '@diskpush/database'
47
import { capabilityCacheKey } from '../resolve.js'
@@ -15,7 +18,7 @@ import { summarizeChanges, topologyOf } from '@diskpush/schemas'
1518
import { EXIT } from '../exit-codes.js'
1619
import { estimateRemaining, formatBytes, formatDuration, formatRate, pluralize, table } from '../format.js'
1720
import { failure, type Output } from '../output.js'
18-
import { flagValue, hasFlag, type ParsedArgv } from '../parse-argv.js'
21+
import { flagValue, flagValues, hasFlag, type ParsedArgv } from '../parse-argv.js'
1922
import { detectLocalCapabilities, optionsFromFlags, resolveEndpoint } from '../resolve.js'
2023
import type { RsyncCapabilities } from '@diskpush/rsync-core'
2124

@@ -61,6 +64,39 @@ export async function runTransfer(
6164

6265
if (alias.deleteMode !== 'off') options.deleteMode = alias.deleteMode
6366

67+
/*
68+
* `--only NAME` narrows the transfer to entries inside the source directory,
69+
* the CLI's version of ticking rows in the desktop's pane. It becomes an
70+
* rsync `--files-from` list, which is why the names are checked here: a
71+
* separator or a `..` would silently widen the transfer to somewhere the
72+
* user did not name.
73+
*/
74+
const only = flagValues(parsed, '--only')
75+
let selectionCleanup: (() => void) | null = null
76+
if (only.length > 0) {
77+
const bad = only.find(
78+
(name) => name.includes('/') || name.includes('\\') || name.includes('\0') || name === '.' || name === '..',
79+
)
80+
if (bad !== undefined) {
81+
return failure(
82+
output,
83+
`--only takes a name inside the source directory, not a path: ${JSON.stringify(bad)}.`,
84+
EXIT.usage,
85+
)
86+
}
87+
const directory = mkdtempSync(join(tmpdir(), 'diskpush-only-'))
88+
const listPath = join(directory, 'files-from')
89+
// NUL-separated: a newline is legal in a filename, and a line-separated
90+
// list would split one such name into two paths that do not exist.
91+
writeFileSync(listPath, `${only.join('\0')}\0`)
92+
options = { ...options, filesFrom: listPath, from0: true }
93+
selectionCleanup = () => rmSync(directory, { recursive: true, force: true })
94+
// Registered rather than called at each of this function's many returns:
95+
// rsync has read the list long before the process ends, and one handler
96+
// cannot be forgotten the way eight call sites can.
97+
process.once('exit', selectionCleanup)
98+
}
99+
64100
const source = await resolveEndpoint(store, sourceInput)
65101
const destination = await resolveEndpoint(store, destinationInput)
66102
const topology = topologyOf(source.endpoint, destination.endpoint)
@@ -100,9 +136,20 @@ export async function runTransfer(
100136
}
101137

102138
// --- preview ------------------------------------------------------------
103-
// Mirror always previews before it can run. A plain sync previews only when
104-
// asked, because its dry run costs a full scan for no safety benefit.
105-
const wantsPreview = options.deleteMode !== 'off' || options.dryRun
139+
/*
140+
* Everything previews, not only a mirror.
141+
*
142+
* This used to skip the dry run for a plain sync, on the reasoning that it
143+
* "costs a full scan for no safety benefit". Deleting is not the only way to
144+
* regret a transfer: two named folders can turn into forty thousand files,
145+
* and by the time anything is on screen it is already copying. A scan is
146+
* cheap next to that.
147+
*
148+
* `--yes` still skips the question, which is what a script passes; the scan
149+
* itself is skipped only by `--no-preview`, for someone who genuinely wants
150+
* the old behaviour.
151+
*/
152+
const wantsPreview = !hasFlag(parsed, '--no-preview')
106153
let preview: Awaited<ReturnType<typeof runPreview>> | null = null
107154

108155
if (wantsPreview) {
@@ -147,6 +194,28 @@ export async function runTransfer(
147194
}
148195
}
149196

197+
/*
198+
* A plain sync is approved too, but only where there is somebody to ask.
199+
*
200+
* A script that pipes us, passes --non-interactive, or passes --yes has
201+
* already decided, and turning those into a refusal would break every
202+
* scheduled profile run. The prompt is for the interactive case, which is
203+
* the one where a surprise is possible.
204+
*/
205+
if (options.deleteMode === 'off' && preview) {
206+
const moving = preview.changes.filter((c) => c.action === 'add' || c.action === 'update').length
207+
const asking = !hasFlag(parsed, '--yes') && !hasFlag(parsed, '--non-interactive') && process.stdin.isTTY
208+
209+
if (moving === 0) {
210+
return finish(output, EXIT.ok, 'Nothing to transfer. The destination already matches.', {
211+
changes: summarizeChanges(preview.changes),
212+
})
213+
}
214+
if (asking && !(await confirm(`${alias.label} ${pluralize(moving, 'file')}?`))) {
215+
return failure(output, `${alias.label} cancelled. Nothing was transferred.`, EXIT.refused)
216+
}
217+
}
218+
150219
// --- run -----------------------------------------------------------------
151220
let plan: ExecutionPlan
152221
try {

‎apps/cli/src/parse-argv.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,3 +116,28 @@ describe('commands', () => {
116116
expect(parsed.positionals).toEqual(['sync', './b/'])
117117
})
118118
})
119+
120+
describe('--only', () => {
121+
/*
122+
* The bug: `--only` was not in VALUE_FLAGS, so it consumed nothing. Each
123+
* name fell through to the positionals, `flagValues` came back empty, and
124+
* `diskpush sync SRC DST --only movieA --only movieB` synced the whole
125+
* directory anyway. Which is the exact bug --only exists to fix.
126+
*/
127+
it('takes a value, and repeats', () => {
128+
const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'movieA', '--only', 'movieB'])
129+
expect(flagValues(parsed, '--only')).toEqual(['movieA', 'movieB'])
130+
// The names are flag values, not endpoints.
131+
expect(parsed.positionals).toEqual(['dev:/srv/', './out/'])
132+
})
133+
134+
it('keeps a name with spaces in one piece', () => {
135+
const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'The Movie (2019)'])
136+
expect(flagValues(parsed, '--only')).toEqual(['The Movie (2019)'])
137+
})
138+
139+
it('accepts the --only=NAME form too', () => {
140+
const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only=movieA'])
141+
expect(flagValues(parsed, '--only')).toEqual(['movieA'])
142+
})
143+
})

‎apps/cli/src/parse-argv.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ export const VALUE_FLAGS = new Set([
3333
'--exclude-from',
3434
'--include-from',
3535
'--files-from',
36+
// Repeatable: one entry name inside the source directory, per occurrence.
37+
'--only',
3638
'--bwlimit',
3739
'--max-size',
3840
'--min-size',

0 commit comments

Comments
 (0)