diff --git a/apps/cli/src/commands/transfer.ts b/apps/cli/src/commands/transfer.ts index 0e7aabb..8dddb89 100644 --- a/apps/cli/src/commands/transfer.ts +++ b/apps/cli/src/commands/transfer.ts @@ -1,4 +1,7 @@ import { randomUUID } from 'node:crypto' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { createInterface } from 'node:readline/promises' import type { DiskPushStore } from '@diskpush/database' import { capabilityCacheKey } from '../resolve.js' @@ -15,7 +18,7 @@ import { summarizeChanges, topologyOf } from '@diskpush/schemas' import { EXIT } from '../exit-codes.js' import { estimateRemaining, formatBytes, formatDuration, formatRate, pluralize, table } from '../format.js' import { failure, type Output } from '../output.js' -import { flagValue, hasFlag, type ParsedArgv } from '../parse-argv.js' +import { flagValue, flagValues, hasFlag, type ParsedArgv } from '../parse-argv.js' import { detectLocalCapabilities, optionsFromFlags, resolveEndpoint } from '../resolve.js' import type { RsyncCapabilities } from '@diskpush/rsync-core' @@ -61,6 +64,39 @@ export async function runTransfer( if (alias.deleteMode !== 'off') options.deleteMode = alias.deleteMode + /* + * `--only NAME` narrows the transfer to entries inside the source directory, + * the CLI's version of ticking rows in the desktop's pane. It becomes an + * rsync `--files-from` list, which is why the names are checked here: a + * separator or a `..` would silently widen the transfer to somewhere the + * user did not name. + */ + const only = flagValues(parsed, '--only') + let selectionCleanup: (() => void) | null = null + if (only.length > 0) { + const bad = only.find( + (name) => name.includes('/') || name.includes('\\') || name.includes('\0') || name === '.' || name === '..', + ) + if (bad !== undefined) { + return failure( + output, + `--only takes a name inside the source directory, not a path: ${JSON.stringify(bad)}.`, + EXIT.usage, + ) + } + const directory = mkdtempSync(join(tmpdir(), 'diskpush-only-')) + const listPath = join(directory, 'files-from') + // NUL-separated: a newline is legal in a filename, and a line-separated + // list would split one such name into two paths that do not exist. + writeFileSync(listPath, `${only.join('\0')}\0`) + options = { ...options, filesFrom: listPath, from0: true } + selectionCleanup = () => rmSync(directory, { recursive: true, force: true }) + // Registered rather than called at each of this function's many returns: + // rsync has read the list long before the process ends, and one handler + // cannot be forgotten the way eight call sites can. + process.once('exit', selectionCleanup) + } + const source = await resolveEndpoint(store, sourceInput) const destination = await resolveEndpoint(store, destinationInput) const topology = topologyOf(source.endpoint, destination.endpoint) @@ -100,9 +136,20 @@ export async function runTransfer( } // --- preview ------------------------------------------------------------ - // Mirror always previews before it can run. A plain sync previews only when - // asked, because its dry run costs a full scan for no safety benefit. - const wantsPreview = options.deleteMode !== 'off' || options.dryRun + /* + * Everything previews, not only a mirror. + * + * This used to skip the dry run for a plain sync, on the reasoning that it + * "costs a full scan for no safety benefit". Deleting is not the only way to + * regret a transfer: two named folders can turn into forty thousand files, + * and by the time anything is on screen it is already copying. A scan is + * cheap next to that. + * + * `--yes` still skips the question, which is what a script passes; the scan + * itself is skipped only by `--no-preview`, for someone who genuinely wants + * the old behaviour. + */ + const wantsPreview = !hasFlag(parsed, '--no-preview') let preview: Awaited> | null = null if (wantsPreview) { @@ -147,6 +194,28 @@ export async function runTransfer( } } + /* + * A plain sync is approved too, but only where there is somebody to ask. + * + * A script that pipes us, passes --non-interactive, or passes --yes has + * already decided, and turning those into a refusal would break every + * scheduled profile run. The prompt is for the interactive case, which is + * the one where a surprise is possible. + */ + if (options.deleteMode === 'off' && preview) { + const moving = preview.changes.filter((c) => c.action === 'add' || c.action === 'update').length + const asking = !hasFlag(parsed, '--yes') && !hasFlag(parsed, '--non-interactive') && process.stdin.isTTY + + if (moving === 0) { + return finish(output, EXIT.ok, 'Nothing to transfer. The destination already matches.', { + changes: summarizeChanges(preview.changes), + }) + } + if (asking && !(await confirm(`${alias.label} ${pluralize(moving, 'file')}?`))) { + return failure(output, `${alias.label} cancelled. Nothing was transferred.`, EXIT.refused) + } + } + // --- run ----------------------------------------------------------------- let plan: ExecutionPlan try { diff --git a/apps/cli/src/parse-argv.test.ts b/apps/cli/src/parse-argv.test.ts index b9e992d..ef858c3 100644 --- a/apps/cli/src/parse-argv.test.ts +++ b/apps/cli/src/parse-argv.test.ts @@ -116,3 +116,28 @@ describe('commands', () => { expect(parsed.positionals).toEqual(['sync', './b/']) }) }) + +describe('--only', () => { + /* + * The bug: `--only` was not in VALUE_FLAGS, so it consumed nothing. Each + * name fell through to the positionals, `flagValues` came back empty, and + * `diskpush sync SRC DST --only movieA --only movieB` synced the whole + * directory anyway. Which is the exact bug --only exists to fix. + */ + it('takes a value, and repeats', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'movieA', '--only', 'movieB']) + expect(flagValues(parsed, '--only')).toEqual(['movieA', 'movieB']) + // The names are flag values, not endpoints. + expect(parsed.positionals).toEqual(['dev:/srv/', './out/']) + }) + + it('keeps a name with spaces in one piece', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'The Movie (2019)']) + expect(flagValues(parsed, '--only')).toEqual(['The Movie (2019)']) + }) + + it('accepts the --only=NAME form too', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only=movieA']) + expect(flagValues(parsed, '--only')).toEqual(['movieA']) + }) +}) diff --git a/apps/cli/src/parse-argv.ts b/apps/cli/src/parse-argv.ts index 0e1bd29..f31497a 100644 --- a/apps/cli/src/parse-argv.ts +++ b/apps/cli/src/parse-argv.ts @@ -33,6 +33,8 @@ export const VALUE_FLAGS = new Set([ '--exclude-from', '--include-from', '--files-from', + // Repeatable: one entry name inside the source directory, per occurrence. + '--only', '--bwlimit', '--max-size', '--min-size', diff --git a/apps/cli/src/tui/app.ts b/apps/cli/src/tui/app.ts index 56eed9c..5878b48 100644 --- a/apps/cli/src/tui/app.ts +++ b/apps/cli/src/tui/app.ts @@ -1,5 +1,5 @@ -import { readdirSync, statSync } from 'node:fs' -import { homedir } from 'node:os' +import { mkdtempSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' import { join, posix } from 'node:path' import { knownHostsPath } from '@diskpush/database' import { SftpBrowser, SshSession } from '@diskpush/ssh-core' @@ -31,10 +31,19 @@ export type Pane = { index: number offset: number error: string | null + /** + * Entries marked with space. Empty means the whole directory, which is what + * every transfer here used to be: the pane had a cursor and no way to say + * "these two", so `s` always sent everything you were looking at. + * + * Names, not paths, and cleared whenever the pane moves, because a mark + * refers to a row in the directory currently on screen. + */ + marked: Set } const HELP = - 'tab switch arrows/jk move enter open left up c change endpoint s sync to other p preview r refresh q quit' + 'tab switch arrows/jk move space mark enter open left up c endpoint s sync p preview r refresh q quit' /** Somewhere a pane can point at: this machine, or a server. */ export type EndpointChoice = { @@ -82,7 +91,7 @@ export function buildEndpointChoices( } export function blankPane(label: string, path: string, connection: Connection | null = null): Pane { - return { label, connection, path, entries: [], index: 0, offset: 0, error: null } + return { label, connection, path, entries: [], index: 0, offset: 0, error: null, marked: new Set() } } export class Tui { @@ -99,6 +108,8 @@ export class Tui { * resolver is held until a key answers it. */ private hostKey: { host: string; fingerprint: string; keyType: string; decide: (trust: boolean) => void } | null = null + /** The pending "transfer N files?" question, while it is on screen. */ + private confirm: { headline: string; detail: string; decide: (approved: boolean) => void } | null = null constructor( left: Pane, @@ -157,6 +168,9 @@ export class Tui { pane.entries = pane.connection ? await this.listRemote(pane) : listLocal(pane.path) pane.index = 0 pane.offset = 0 + // A mark names a row in the directory that was on screen. Carrying it + // into a new listing would transfer whatever happened to share the name. + pane.marked.clear() } catch (error) { pane.entries = [] pane.error = error instanceof Error ? error.message : String(error) @@ -210,10 +224,14 @@ export class Tui { const entry = pane.entries[pane.offset + row] if (!entry) return '' const selected = pane.offset + row === pane.index && side === this.active + const marked = pane.marked.has(entry.name) const name = entry.isDirectory ? `${entry.name}/` : entry.name + // The mark sits in its own column so a name never shifts when it is + // toggled, and it survives the reverse-video cursor. const size = entry.isDirectory ? '' : formatSize(entry.size) - const body = `${pad(truncate(name, paneWidth - 8), paneWidth - 8)} ${size.padStart(6)}` - return selected ? `${ansi.reverse}${body}${ansi.reset}` : body + const body = `${marked ? '*' : ' '}${pad(truncate(name, paneWidth - 9), paneWidth - 9)} ${size.padStart(6)}` + if (selected) return `${ansi.reverse}${body}${ansi.reset}` + return marked ? `${ansi.yellow}${body}${ansi.reset}` : body }) out.push(` ${pad(cells[0] ?? '', paneWidth)} ${pad(cells[1] ?? '', paneWidth)}`) } @@ -224,6 +242,7 @@ export class Tui { let frame = out.join('\n') if (this.picker) frame += this.renderPicker(columns, rows) if (this.hostKey) frame += this.renderHostKey(columns, rows) + if (this.confirm) frame += this.renderConfirm(columns, rows) process.stdout.write(frame) } @@ -257,6 +276,16 @@ export class Tui { return true } + if (this.confirm) { + // Anything that is not an explicit yes cancels. A transfer is not the + // sort of thing to start because a key was mashed, and `q` here means + // "not this" rather than "quit", the same as it does in the picker. + const decide = this.confirm.decide + this.confirm = null + decide(isChar(key, 'y') || isChar(key, 'Y')) + return true + } + if (this.picker) { // Escape closes the picker rather than the app: inside a dialog it means // "not this", which is not the same as "quit". @@ -295,6 +324,8 @@ export class Tui { await this.goUp() } else if (key === 'right' || key === 'enter' || isChar(key, 'l')) { await this.enter() + } else if (isChar(key, ' ')) { + this.toggleMark() } else if (isChar(key, 'c')) { this.openPicker() } else if (isChar(key, 'r')) { @@ -307,6 +338,21 @@ export class Tui { return true } + /** + * Marks or unmarks the row under the cursor, then steps down. + * + * Stepping down is what every file manager does and what makes marking a + * run of files one key repeated rather than an alternation of two. + */ + private toggleMark(): void { + const pane = this.current + const entry = pane.entries[pane.index] + if (!entry) return + if (pane.marked.has(entry.name)) pane.marked.delete(entry.name) + else pane.marked.add(entry.name) + this.move(1) + } + private move(delta: number): void { const pane = this.current const last = Math.max(0, pane.entries.length - 1) @@ -395,6 +441,30 @@ export class Tui { } /** The host-key question, drawn over everything. */ + private renderConfirm(columns: number, rows: number): string { + const ask = this.confirm! + const width = Math.max(40, Math.min(72, columns - 6)) + const left = Math.max(1, Math.floor((columns - width) / 2)) + const top = Math.max(1, Math.floor(rows / 2) - 2) + const inner = width - 2 + const out: string[] = [] + const line = (row: number, body: string) => out.push(`${ansi.moveTo(row, left)}${body}`) + + line(top, `${ansi.blue}+${'-'.repeat(inner)}+${ansi.reset}`) + line( + top + 1, + `${ansi.blue}|${ansi.reset}${ansi.bold}${pad(` ${truncate(ask.headline, inner - 2)}`, inner)}${ansi.reset}${ansi.blue}|${ansi.reset}`, + ) + line( + top + 2, + `${ansi.blue}|${ansi.reset}${ansi.dim}${pad(` ${truncate(ask.detail, inner - 2)}`, inner)}${ansi.reset}${ansi.blue}|${ansi.reset}`, + ) + line(top + 3, `${ansi.blue}|${ansi.reset}${pad('', inner)}${ansi.blue}|${ansi.reset}`) + line(top + 4, `${ansi.blue}|${ansi.reset}${pad(' y transfer n cancel', inner)}${ansi.blue}|${ansi.reset}`) + line(top + 5, `${ansi.blue}+${'-'.repeat(inner)}+${ansi.reset}`) + return out.join('') + } + private renderHostKey(columns: number, rows: number): string { const key = this.hostKey! const width = Math.max(40, Math.min(72, columns - 6)) @@ -432,28 +502,85 @@ export class Tui { await this.load(this.active) } + /** + * The marked entries as a file rsync can read, or null for the whole folder. + * + * NUL-separated, because a newline is legal in a filename and a + * line-separated list would split one such name into two paths that do not + * exist. + */ + private markList(pane: Pane): { path: string; cleanup: () => void } | null { + if (pane.marked.size === 0) return null + const directory = mkdtempSync(join(tmpdir(), 'diskpush-marks-')) + const path = join(directory, 'files-from') + writeFileSync(path, `${[...pane.marked].join('\0')}\0`) + return { path, cleanup: () => rmSync(directory, { recursive: true, force: true }) } + } + + /** + * Previews, then transfers what was previewed. + * + * `s` used to start an immediate transfer of the entire directory. There was + * no way to say "these two" and no moment at which anything could be + * refused: by the time a number was on screen the files were already moving. + * Now the dry run always runs first, and a real transfer waits on a yes. + */ private async transfer(previewOnly: boolean): Promise { const source = this.current const destination = this.other + const scope = source.marked.size > 0 ? `${source.marked.size} marked` : 'whole folder' this.busy = true - this.status = `${ansi.yellow}${previewOnly ? 'Previewing' : 'Syncing'} ${source.path} -> ${destination.path}${ansi.reset}` + this.status = `${ansi.yellow}Scanning ${source.path} -> ${destination.path} (${scope})${ansi.reset}` this.render() + const list = this.markList(source) try { const remote = source.connection ?? destination.connection - const plan = planTransfer({ + const optionsFor = (dryRun: boolean) => + defaultRsyncOptions({ + dryRun, + stats: true, + ...(list ? { filesFrom: list.path, from0: true } : {}), + }) + const shell = remote ? { remoteShell: { keyPath: remote.keyPath, port: remote.port } } : {} + const endpoints = { source: parseEndpoint(endpointString(source)), destination: parseEndpoint(endpointString(destination)), - options: defaultRsyncOptions({ dryRun: previewOnly, stats: true }), - ...(remote ? { remoteShell: { keyPath: remote.keyPath, port: remote.port } } : {}), - }) - const result = await runToCompletion(plan) + } + + const dry = await runToCompletion(planTransfer({ ...endpoints, options: optionsFor(true), ...shell })) + const preview = summarizeChanges(dry.changes) + const moving = preview.add + preview.update + + if (!dry.ok) { + this.status = `${ansi.red}${truncate(dry.message, 200)}${ansi.reset}` + return + } + if (previewOnly) { + this.status = `${ansi.green}Preview (${scope}): ${preview.add} to add, ${preview.update} to update, ${preview.unchanged} unchanged${ansi.reset}` + return + } + if (moving === 0) { + this.status = `${ansi.green}Nothing to transfer (${scope}). The destination already matches.${ansi.reset}` + return + } + + const approved = await this.ask( + `Transfer ${moving} file${moving === 1 ? '' : 's'} (${scope})?`, + `into ${destination.path}`, + ) + if (!approved) { + this.status = `${ansi.yellow}Cancelled. Nothing was transferred.${ansi.reset}` + return + } + + this.status = `${ansi.yellow}Syncing ${source.path} -> ${destination.path}${ansi.reset}` + this.render() + const result = await runToCompletion(planTransfer({ ...endpoints, options: optionsFor(false), ...shell })) const summary = summarizeChanges(result.changes) if (!result.ok) { this.status = `${ansi.red}${truncate(result.message, 200)}${ansi.reset}` - } else if (previewOnly) { - this.status = `${ansi.green}Preview: ${summary.add} to add, ${summary.update} to update, ${summary.unchanged} unchanged${ansi.reset}` } else { this.status = `${ansi.green}Synced ${summary.add + summary.update} files${ansi.reset}` await this.load(this.active === 'left' ? 'right' : 'left') @@ -461,10 +588,26 @@ export class Tui { } catch (error) { this.status = `${ansi.red}${truncate(error instanceof Error ? error.message : String(error), 200)}${ansi.reset}` } finally { + list?.cleanup() this.busy = false } } + /** + * A yes/no question over the panes. Resolves false on anything but y. + * + * Two lines, because `truncate` keeps the END of a string (the right choice + * for a path, the wrong one for a sentence). One combined line lost its own + * verb: "Transfer 4 files to /very/long/path?" rendered as "…r 4 files to + * /very/long/path?", which is a question about nothing. + */ + private ask(headline: string, detail: string): Promise { + return new Promise((resolve) => { + this.confirm = { headline, detail, decide: resolve } + this.render() + }) + } + close(): void { for (const session of this.sessions.values()) session.close() } diff --git a/apps/desktop/electron/main/services/selection.test.ts b/apps/desktop/electron/main/services/selection.test.ts new file mode 100644 index 0000000..e9ec887 --- /dev/null +++ b/apps/desktop/electron/main/services/selection.test.ts @@ -0,0 +1,142 @@ +import { existsSync, readFileSync } from 'node:fs' +import { describe, expect, it, vi } from 'vitest' +import type { RsyncEvent } from '@diskpush/schemas' +import { TransferRequestSchema } from '../../shared/contract.js' + +/** A run that ends immediately; these tests are about the plan, not the stream. */ +function emptyRun() { + return { + cancel: () => {}, + events: { + [Symbol.asyncIterator]() { + return { next: () => Promise.resolve({ value: undefined as never, done: true as const }) } + }, + }, + } +} + +const planned: Array<{ options: { filesFrom?: string | null; from0?: boolean }; list: string | null }> = [] + +vi.mock('./store.js', () => ({ + store: async () => ({ getSetting: async (_key: string, fallback: T) => fallback }), +})) + +vi.mock('@diskpush/rsync-core', () => ({ + planTransfer: (input: { options: { filesFrom?: string | null; from0?: boolean } }) => { + // Read here rather than from the test body: this runs while the transfer + // still owns the file, so there is no window to race against its cleanup. + const path = input.options.filesFrom + planned.push({ options: input.options, list: path ? readFileSync(path, 'utf8') : null }) + return { binary: 'rsync', args: [], display: 'rsync', controlDisplay: null, warnings: [] } + }, + runPlan: () => emptyRun(), + parseRsyncCapabilities: () => ({}), + intersectCapabilities: (a: unknown) => a, + unknownCapabilities: () => ({}), +})) + +const { previewTransfer } = await import('./transfers.js') + +const BASE = { + source: { type: 'local' as const, path: '/src/' }, + destination: { type: 'local' as const, path: '/dst/' }, + options: { + archive: true, + checksum: false, + compression: 'auto' as const, + deleteMode: 'off' as const, + hardLinks: false, + acls: false, + xattrs: false, + numericIds: false, + update: false, + ignoreExisting: false, + existingOnly: false, + inplace: false, + excludes: [], + includes: [], + bwlimit: null, + maxSize: null, + minSize: null, + }, + deletesConfirmed: false, +} + +const sender = () => ({ isDestroyed: () => false, send: () => {} }) as never + +describe('a selection reaches rsync', () => { + /* + * The bug: the panes let you select entries and the request only ever + * carried the directory, so ticking two folders and pressing Sync copied the + * whole tree. Two movies became forty thousand files. + */ + it('turns picked entries into a NUL-separated --files-from list', async () => { + planned.length = 0 + await previewTransfer( + { ...BASE, selection: ['The Movie (2019)', 'Another Movie'], previewId: 's1' }, + sender(), + ) + + const options = planned.at(-1)!.options + expect(options.from0).toBe(true) + expect(options.filesFrom).toBeTruthy() + }) + + it('writes the names verbatim, separated by NUL', async () => { + planned.length = 0 + await previewTransfer({ ...BASE, selection: ['a b.mkv', 'weird\tname'], previewId: 's2' }, sender()) + + expect(planned.at(-1)!.list).toBe('a b.mkv\0weird\tname\0') + }) + + it('leaves an unselected transfer alone, so the whole folder still syncs', async () => { + planned.length = 0 + await previewTransfer({ ...BASE, selection: [], previewId: 's3' }, sender()) + + const options = planned.at(-1)!.options + expect(options.filesFrom).toBeFalsy() + expect(options.from0).toBeFalsy() + }) + + it('removes the list file when the run is over', async () => { + planned.length = 0 + await previewTransfer({ ...BASE, selection: ['one'], previewId: 's4' }, sender()) + + const path = planned.at(-1)!.options.filesFrom + expect(path).toBeTruthy() + expect(existsSync(path!)).toBe(false) + }) +}) + +describe('the selection is names, never paths', () => { + /* + * This list becomes an rsync --files-from rooted at the source directory, so + * a renderer that could put a separator or `..` in here would be choosing + * which files leave the machine. The schema is the only thing standing + * between those two facts. + */ + const reject = (selection: string[]) => + TransferRequestSchema.safeParse({ ...BASE, selection }).success + + it('rejects anything that could climb out of the directory', () => { + expect(reject(['../../etc/shadow'])).toBe(false) + expect(reject(['..'])).toBe(false) + expect(reject(['sub/dir'])).toBe(false) + expect(reject(['back\\slash'])).toBe(false) + expect(reject(['nul\0byte'])).toBe(false) + expect(reject(['/etc/passwd'])).toBe(false) + }) + + it('accepts ordinary names, spaces and all', () => { + expect(reject(['The Movie (2019).mkv', 'Another.Movie.2020'])).toBe(true) + }) + + it('defaults to the whole folder when the field is absent', () => { + const parsed = TransferRequestSchema.parse({ + source: BASE.source, + destination: BASE.destination, + options: BASE.options, + }) + expect(parsed.selection).toEqual([]) + }) +}) diff --git a/apps/desktop/electron/main/services/transfers.ts b/apps/desktop/electron/main/services/transfers.ts index 30f134b..ed8dd5a 100644 --- a/apps/desktop/electron/main/services/transfers.ts +++ b/apps/desktop/electron/main/services/transfers.ts @@ -1,4 +1,7 @@ import { randomUUID } from 'node:crypto' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import type { WebContents } from 'electron' import { intersectCapabilities, @@ -101,31 +104,85 @@ function optionsFrom(input: TransferOptions): RsyncOptions { }) } -async function buildPlan(request: TransferRequest, overrides: Partial = {}): Promise { +/** + * A selection, as a file rsync can read. + * + * NUL-separated (`--from0`) rather than one name per line, because a newline is + * a legal character in a filename and a line-separated list would silently + * split such a name into two paths that do not exist. + * + * The names were validated as entry names on the way in, so none of them can + * contain a separator or `..`; rsync resolves each one against the source + * directory and nothing here can point outside it. + * + * Returns the file path and the cleanup to run once rsync has exited. rsync + * reads the list at startup, but deleting it early is a race nobody needs. + */ +async function writeSelectionList( + selection: readonly string[], +): Promise<{ path: string; cleanup: () => Promise }> { + const directory = await mkdtemp(join(tmpdir(), 'diskpush-selection-')) + const path = join(directory, 'files-from') + await writeFile(path, `${selection.join('\0')}\0`, 'utf8') + return { + // Awaited rather than fire-and-forget, so "the run is over" and "the list + // is gone" are the same moment. A detached rm leaves a window where a + // caller cannot tell whether cleanup happened or simply had not yet. + path, + cleanup: () => rm(directory, { recursive: true, force: true }).catch(() => {}), + } +} + +type BuiltPlan = { plan: ExecutionPlan; cleanup: () => Promise } + +async function buildPlan(request: TransferRequest, overrides: Partial = {}): Promise { const source = await resolveEndpoint(request.source) const destination = await resolveEndpoint(request.destination) const capabilities = await capabilitiesFor([source.connectionId, destination.connectionId]) - const options = { ...optionsFrom(request.options), ...overrides } + + /* + * A selection narrows the transfer to what the user picked. Without this the + * request only ever carried the directory, so ticking two folders and + * pressing Sync copied the entire tree: the panes let you select, and the + * selection reached nothing. + */ + const selection = request.selection ?? [] + const list = selection.length > 0 ? await writeSelectionList(selection) : null + const options = { + ...optionsFrom(request.options), + ...(list ? { filesFrom: list.path, from0: true } : {}), + ...overrides, + } + const cleanup = async () => { + await list?.cleanup() + } const isServerToServer = source.endpoint.type === 'ssh' && destination.endpoint.type === 'ssh' const sourceConnection = source.connectionId ? await resolveConnection(source.connectionId) : null - return planTransfer({ - source: source.endpoint, - destination: destination.endpoint, - options, - capabilities, - deletesConfirmed: request.deletesConfirmed, - ...(isServerToServer - ? { - sourceShell: await shellOptionsFor(source.connectionId), - destinationShell: await shellOptionsFor(destination.connectionId), - sourceRsyncPath: sourceConnection?.rsyncPath ?? null, - } - : { - remoteShell: await shellOptionsFor(source.connectionId ?? destination.connectionId), - }), - }) + try { + const plan = planTransfer({ + source: source.endpoint, + destination: destination.endpoint, + options, + capabilities, + deletesConfirmed: request.deletesConfirmed, + ...(isServerToServer + ? { + sourceShell: await shellOptionsFor(source.connectionId), + destinationShell: await shellOptionsFor(destination.connectionId), + sourceRsyncPath: sourceConnection?.rsyncPath ?? null, + } + : { + remoteShell: await shellOptionsFor(source.connectionId ?? destination.connectionId), + }), + }) + return { plan, cleanup } + } catch (error) { + // A rejected plan still wrote a list file. + await cleanup() + throw error + } } /** @@ -199,7 +256,7 @@ export async function previewTransfer( request: TransferRequest & { previewId: string }, sender: WebContents, ): Promise { - const plan = await buildPlan(request, { dryRun: true }) + const { plan, cleanup } = await buildPlan(request, { dryRun: true }) const summary = summarizeChanges([]) const deletes: string[] = [] @@ -274,6 +331,7 @@ export async function previewTransfer( } } finally { previews.delete(request.previewId) + await cleanup() } emit(true) @@ -307,7 +365,7 @@ export function cancelPreview(previewId: string): boolean { export type StartedJob = { jobId: string; command: string; control: string | null; warnings: string[] } export async function startTransfer(request: TransferRequest, sender: WebContents): Promise { - const plan = await buildPlan(request) + const { plan, cleanup } = await buildPlan(request) const jobId = randomUUID() const db = await store() @@ -369,6 +427,7 @@ export async function startTransfer(request: TransferRequest, sender: WebContent } } running.delete(jobId) + await cleanup() })() return { jobId, command: plan.display, control: plan.controlDisplay ?? null, warnings: plan.warnings } diff --git a/apps/desktop/electron/shared/contract.ts b/apps/desktop/electron/shared/contract.ts index 78f10eb..0121e3e 100644 --- a/apps/desktop/electron/shared/contract.ts +++ b/apps/desktop/electron/shared/contract.ts @@ -75,6 +75,23 @@ export const PathSchema = z.string().min(1).max(4096) export const ConnectionIdSchema = z.string().min(1).max(128) +/** + * A single entry name inside a directory — never a path. + * + * Every mutating operation takes a directory plus one of these and joins them + * in the main process, so the renderer cannot walk out of the folder it is + * showing. `..`, a separator or a NUL would each be a way to do exactly that. + */ +export const EntryNameSchema = z + .string() + .min(1) + .max(255) + .refine((name) => !name.includes('/') && !name.includes('\\') && !name.includes('\0'), { + message: 'A name cannot contain a path separator.', + }) + .refine((name) => name !== '.' && name !== '..', { message: 'That name is reserved.' }) + .refine((name) => name.trim() === name, { message: 'A name cannot begin or end with a space.' }) + export const EndpointRefSchema = z.discriminatedUnion('type', [ z.object({ type: z.literal('local'), path: PathSchema }), z.object({ type: z.literal('ssh'), connectionId: ConnectionIdSchema, path: PathSchema }), @@ -134,6 +151,17 @@ export const TransferRequestSchema = z.object({ options: TransferOptionsSchema, /** Only meaningful for a delete-enabled job, and only after a preview. */ deletesConfirmed: z.boolean().default(false), + /** + * The entries the user picked in the source pane. Empty means the whole + * directory, which is what every transfer used to be. + * + * Names, never paths, and validated as such: the main process turns them + * into an rsync `--files-from` list rooted at the source directory, so a + * renderer that could put `../` or an absolute path in here would be + * choosing which files leave the machine. That is why this reuses + * EntryNameSchema rather than PathSchema. + */ + selection: z.array(EntryNameSchema).max(10000).default([]), }) export type TransferRequest = z.infer @@ -181,23 +209,6 @@ export const RenameRequestSchema = z.object({ to: PathSchema, }) -/** - * A single entry name inside a directory — never a path. - * - * Every mutating operation takes a directory plus one of these and joins them - * in the main process, so the renderer cannot walk out of the folder it is - * showing. `..`, a separator or a NUL would each be a way to do exactly that. - */ -export const EntryNameSchema = z - .string() - .min(1) - .max(255) - .refine((name) => !name.includes('/') && !name.includes('\\') && !name.includes('\0'), { - message: 'A name cannot contain a path separator.', - }) - .refine((name) => name !== '.' && name !== '..', { message: 'That name is reserved.' }) - .refine((name) => name.trim() === name, { message: 'A name cannot begin or end with a space.' }) - /** Create a directory or an empty file: `name` inside `directory`. */ export const CreateEntryRequestSchema = z.object({ connectionId: ConnectionIdSchema.optional(), diff --git a/apps/desktop/src/app/page.tsx b/apps/desktop/src/app/page.tsx index 61c7d41..c8cf488 100644 --- a/apps/desktop/src/app/page.tsx +++ b/apps/desktop/src/app/page.tsx @@ -21,7 +21,7 @@ import { ProfileBar } from '@/components/profile-bar' import { ServerManager } from '@/components/server-manager' import { endpointLabel, loadPane, Pane, type PaneEndpoint, type PaneState } from '@/components/pane' import { TransferRail } from '@/components/transfer-rail' -import { MirrorPreviewDialog, TransferBand, type ActiveJob } from '@/components/transfer-panel' +import { TransferBand, TransferPreviewDialog, type ActiveJob } from '@/components/transfer-panel' import { Button } from '@/components/ui/button' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { @@ -75,6 +75,15 @@ function TabButton({ ) } +/** What the renderer sends to preview or start a transfer. */ +type TransferDraft = { + source: ReturnType + destination: ReturnType + options: { deleteMode: 'off' | 'delay' } + deletesConfirmed: boolean + selection: string[] +} + const blankPane = (endpoint: PaneEndpoint, path: string): PaneState => ({ endpoint, path, @@ -93,7 +102,6 @@ export default function Workspace() { const [active, setActive] = useState<'left' | 'right'>('left') const [direction, setDirection] = useState<'ltr' | 'rtl'>('ltr') const [mirror, setMirror] = useState(false) - const [trust, setTrust] = useState(false) const [preview, setPreview] = useState(null) const [previewProgress, setPreviewProgress] = useState(null) const [previewOpen, setPreviewOpen] = useState(false) @@ -107,6 +115,14 @@ export default function Workspace() { * saw. Every result and every progress event is matched against this id. */ const previewIdRef = useRef(null) + /** + * The request the open preview was built from. + * + * Confirming runs THIS, not a request rebuilt from whatever the panes say by + * then. A dialog that says "delete 12 files" has to start the transfer it + * measured, even if something changed a pane underneath it. + */ + const approvedRequestRef = useRef(null) const [job, setJob] = useState(null) const [error, setError] = useState(null) const [showConnection, setShowConnection] = useState(false) @@ -216,16 +232,33 @@ export default function Workspace() { const allConnections = useMemo(() => [...saved, ...sshConfig], [saved, sshConfig]) const route = `${endpointLabel(source.endpoint, allConnections)} → ${endpointLabel(destination.endpoint, allConnections)}` - const request = useMemo( - () => ({ - // The panes show directory contents, so a sync between them means "make - // these contents match", not "nest this directory inside that one". - source: refFor(source, withTrailingSlash(source.path)), - destination: refFor(destination, withTrailingSlash(destination.path)), - options: { deleteMode: mirror ? ('delay' as const) : ('off' as const) }, - deletesConfirmed: false, - }), - [source, destination, mirror], + /** + * The transfer for a given direction. + * + * It takes the direction rather than reading it, because the rail sets the + * direction and starts the transfer in the same click. `setDirection` does + * not change the value this render already closed over, so a request built + * from state ran the PREVIOUS direction: pressing "Sync to Local" while the + * other arrow was armed copied local over the server instead, and with + * Mirror on it would have deleted the wrong side. + */ + const requestFor = useCallback( + (towards: 'ltr' | 'rtl'): TransferDraft => { + const from = towards === 'ltr' ? left : right + const to = towards === 'ltr' ? right : left + return { + // The panes show directory contents, so a sync between them means "make + // these contents match", not "nest this directory inside that one". + source: refFor(from, withTrailingSlash(from.path)), + destination: refFor(to, withTrailingSlash(to.path)), + options: { deleteMode: mirror ? ('delay' as const) : ('off' as const) }, + deletesConfirmed: false, + // What the user ticked in the pane the files come FROM. Empty means the + // whole directory. + selection: [...from.selected], + } + }, + [left, right, mirror], ) /** @@ -243,7 +276,9 @@ export default function Workspace() { if (previewId) void api()?.transfers.cancelPreview(previewId) }, []) - const runPreview = useCallback(async () => { + const runPreview = useCallback(async (towards: 'ltr' | 'rtl') => { + const request = requestFor(towards) + approvedRequestRef.current = request const previewId = crypto.randomUUID() // Supersedes any scan already running, so pressing Preview twice does not // leave two rsync processes walking the same trees. @@ -268,10 +303,12 @@ export default function Workspace() { setPreviewOpen(false) setError(caught instanceof Error ? caught.message : String(caught)) } - }, [request]) + }, [requestFor]) const start = useCallback( async (deletesConfirmed: boolean) => { + const request = approvedRequestRef.current + if (!request) return setError(null) previewIdRef.current = null setPreviewOpen(false) @@ -294,7 +331,7 @@ export default function Workspace() { setError(caught instanceof Error ? caught.message : String(caught)) } }, - [request], + [], ) /** @@ -337,6 +374,10 @@ export default function Workspace() { const saveProfile = useCallback( async (name: string) => { setError(null) + // A profile stores the pair and its options. Not the selection: a saved + // pair is meant to be re-runnable later, and a list of entry names that + // were ticked once is not a thing that stays true. + const request = requestFor(direction) try { await unwrap( api()?.profiles.save({ @@ -354,7 +395,7 @@ export default function Workspace() { setError(caught instanceof Error ? caught.message : String(caught)) } }, - [request, direction], + [requestFor, direction], ) const removeProfile = useCallback(async (id: string) => { @@ -367,12 +408,26 @@ export default function Workspace() { } }, []) - const run = useCallback(async () => { - // Mirror always previews. A plain sync does not: its dry run costs a full - // scan and buys no safety, because nothing is deleted either way. - if (mirror) await runPreview() - else await start(false) - }, [mirror, runPreview, start]) + /** + * Every manual transfer is previewed and approved. Mirror is not special. + * + * This used to start a plain sync immediately, on the reasoning that a dry + * run "buys no safety, because nothing is deleted either way". Deleting is + * not the only way to regret a transfer: the run that prompted this change + * was two selected folders that turned into forty thousand files, and by the + * time anything is on screen it is already copying. Now the same dialog that + * guards a mirror shows what a sync would do, and nothing starts until it is + * approved. + * + * The direction is passed in rather than read: the rail sets it and runs in + * one click, and the state has not updated yet. + */ + const run = useCallback( + async (towards: 'ltr' | 'rtl') => { + await runPreview(towards) + }, + [runPreview], + ) if (outsideShell) { return ( @@ -567,8 +622,8 @@ export default function Workspace() { rightLabel={railLabel(right.endpoint, allConnections)} onDirection={setDirection} onToggleMirror={() => setMirror((value) => !value)} - onPreview={runPreview} - onRun={run} + onPreview={() => void runPreview(direction)} + onRun={(towards) => void run(towards)} /> setShowConnection(false)} onSaved={() => void refreshConnections()} /> - void start(true)} diff --git a/apps/desktop/src/components/transfer-panel.tsx b/apps/desktop/src/components/transfer-panel.tsx index f10fc9e..1cbb329 100644 --- a/apps/desktop/src/components/transfer-panel.tsx +++ b/apps/desktop/src/components/transfer-panel.tsx @@ -5,7 +5,6 @@ import { ArrowRight, ChevronRight, CircleCheck, Trash2, TriangleAlert } from 'lu import type { PreviewProgress, PreviewResult } from '@/lib/api' import { formatBytes, formatDuration, formatRate } from '@/lib/format' import { Button } from '@/components/ui/button' -import { Checkbox } from '@/components/ui/checkbox' import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog' import { Progress } from '@/components/ui/progress' import { ScrollArea } from '@/components/ui/scroll-area' @@ -116,13 +115,13 @@ function ScanProgress({ progress, onCancel }: { progress: PreviewProgress; onCan * hundreds of thousands and a DOM node each is what froze the window. The * count on the confirm button is always the true one. */ -export function MirrorPreviewDialog({ +export function TransferPreviewDialog({ preview, progress, open, route, - trust, - onTrustChange, + mirror, + selectionCount, onCancel, onStopScan, onConfirm, @@ -131,8 +130,10 @@ export function MirrorPreviewDialog({ progress: PreviewProgress | null open: boolean route: string - trust: boolean - onTrustChange: (value: boolean) => void + /** Deletes are armed. Changes what this dialog is for, not only its wording. */ + mirror: boolean + /** Entries ticked in the source pane. 0 means the whole folder. */ + selectionCount: number onCancel: () => void onStopScan: () => void onConfirm: () => void @@ -140,6 +141,9 @@ export function MirrorPreviewDialog({ const deletes = preview?.deletes ?? [] const deleteTotal = preview?.deleteTotal ?? 0 const hidden = Math.max(0, deleteTotal - deletes.length) + // Additions and updates: what a plain sync actually moves. Deletions are + // counted separately, on the button that arms them. + const changeTotal = (preview?.summary.add ?? 0) + (preview?.summary.update ?? 0) const summary = preview?.summary const cancelRef = useRef(null) @@ -162,16 +166,43 @@ export function MirrorPreviewDialog({ initialFocus={cancelRef} className="max-w-2xl grid-rows-[auto_minmax(0,1fr)_auto] gap-0 border-line-strong bg-popover p-0 [--dialog-pad:0px]" > + {/* + The header says which of the two things this is. Every manual + transfer comes through here now, not only a mirror, and a plain sync + wearing a red warning triangle and the word "Mirror" would be its own + kind of wrong. + */} - - + + {mirror ? : }
- Mirror + {mirror ? 'Mirror' : 'Sync'} {route}
+ {/* + The scope, stated up front. Two ticked folders that turn into forty + thousand files is the failure this dialog exists to catch, and the + counts below only ever say how many, never how many of what was + asked for. + */} + 0 ? 'bg-primary/12 text-primary' : 'bg-secondary text-muted-foreground', + )} + > + {selectionCount > 0 + ? `${selectionCount.toLocaleString()} selected item${selectionCount === 1 ? '' : 's'}` + : 'Whole folder'} + {/* One scrolling body, so a preview with a long delete list keeps its @@ -280,13 +311,16 @@ export function MirrorPreviewDialog({ to render throughout the scan, so a full-strength Mirror button sat under the spinner offering to run a mirror whose delete list did not exist yet -- inert, which reads as a control that ignores you. + + There is no "trust this pair from now on" in here any more either. It + set a piece of renderer state that nothing read, and it could not have + worked if it were wired: saveProfile hard-codes `trustDeletes: false` + on purpose, because unattended mirroring is the one way a delete list + runs with nobody looking at it. A checkbox offering to skip a + confirmation that is deliberately never skipped is worse than none. */} {preview ? ( - - +
{/* Focus opens on Cancel, not on the confirm and not on the trust @@ -299,10 +333,16 @@ export function MirrorPreviewDialog({
diff --git a/apps/desktop/src/components/transfer-rail.tsx b/apps/desktop/src/components/transfer-rail.tsx index 4ba5f28..c33a3c1 100644 --- a/apps/desktop/src/components/transfer-rail.tsx +++ b/apps/desktop/src/components/transfer-rail.tsx @@ -133,7 +133,14 @@ export function TransferRail({ onDirection: (direction: 'ltr' | 'rtl') => void onToggleMirror: () => void onPreview: () => void - onRun: () => void + /* + * Takes the direction it is starting. The rail sets the direction and starts + * the run in the same click, and `setDirection` does not change what this + * render already closed over, so a caller reading the direction from state + * ran the previous one: the arrow you pressed and the way the files moved + * disagreed on the first press. + */ + onRun: (towards: 'ltr' | 'rtl') => void }) { return ( // Every control in the rail is now one width and one corner radius. It @@ -148,7 +155,7 @@ export function TransferRail({ busy={busy} onClick={() => { onDirection('ltr') - onRun() + onRun('ltr') }} /> { onDirection('rtl') - onRun() + onRun('rtl') }} /> diff --git a/packages/rsync-core/src/args.ts b/packages/rsync-core/src/args.ts index fe35a5b..e547313 100644 --- a/packages/rsync-core/src/args.ts +++ b/packages/rsync-core/src/args.ts @@ -98,6 +98,21 @@ export function buildRsyncArgs(input: BuildArgsInput): BuildArgsResult { if (options.archive) args.push('--archive') else args.push('--recursive') + /* + * `--files-from` turns rsync's recursion OFF, and `--archive` does not turn + * it back on. Without an explicit `--recursive` a selected directory copies + * as an empty directory and none of its contents, which looks like a + * successful transfer and is the worst possible failure for a sync tool. + * + * rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * >f+++++++++ movieA/a.mkv + * + * Harmless when `--archive` already asked for recursion without a file list: + * rsync takes the last-wins flag and both say the same thing. + */ + if (options.filesFrom && options.archive) args.push('--recursive') + if (options.hardLinks) args.push('--hard-links') if (options.acls) { if (capabilities.acls || !capabilities.version) args.push('--acls') @@ -140,7 +155,13 @@ export function buildRsyncArgs(input: BuildArgsInput): BuildArgsResult { if (options.includeFrom) args.push(`--include-from=${options.includeFrom}`) for (const exclude of options.excludes) args.push(`--exclude=${exclude}`) if (options.excludeFrom) args.push(`--exclude-from=${options.excludeFrom}`) - if (options.filesFrom) args.push(`--files-from=${options.filesFrom}`) + if (options.filesFrom) { + // Order between these two does not matter (rsync parses the whole argv + // before it opens the list); they are kept together so the separator and + // the file it applies to are read as one thing. + if (options.from0) args.push('--from0') + args.push(`--files-from=${options.filesFrom}`) + } if (options.pruneEmptyDirs) args.push('--prune-empty-dirs') if (options.relative) args.push('--relative') if (options.maxSize) args.push(`--max-size=${options.maxSize}`) diff --git a/packages/rsync-core/src/files-from.test.ts b/packages/rsync-core/src/files-from.test.ts new file mode 100644 index 0000000..c99f8b9 --- /dev/null +++ b/packages/rsync-core/src/files-from.test.ts @@ -0,0 +1,136 @@ +import { mkdtempSync, mkdirSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { defaultRsyncOptions } from '@diskpush/schemas' +import { buildRsyncArgs } from './args.js' +import { runToCompletion } from './runner.js' +import { planTransfer } from './plan.js' + +const local = (path: string) => ({ type: 'local' as const, path }) + +describe('buildRsyncArgs with a file list', () => { + /* + * The bug this exists to prevent, confirmed against rsync 3.4.1: + * + * 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 rsync's recursion off and `--archive` does not + * switch it back on, so selecting a folder would copy an empty folder and + * call it a success. + */ + it('forces --recursive, because --archive does not imply it under --files-from', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({ filesFrom: '/tmp/list', from0: true }), + }) + + expect(args).toContain('--archive') + expect(args).toContain('--recursive') + expect(args).toContain('--from0') + expect(args).toContain('--files-from=/tmp/list') + }) + + it('does not add --recursive for an ordinary archive transfer', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({}), + }) + + expect(args).toContain('--archive') + expect(args).not.toContain('--recursive') + }) + + it('leaves --from0 off unless it was asked for', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({ filesFrom: '/tmp/list' }), + }) + + expect(args).not.toContain('--from0') + expect(args).toContain('--files-from=/tmp/list') + }) +}) + +/** + * Against the real binary, because the whole point is a behaviour of rsync + * that the flag names do not tell you about. + */ +describe('live rsync: a selected folder', () => { + function fixture() { + const root = mkdtempSync(join(tmpdir(), 'diskpush-files-from-')) + for (const dir of ['src/movieA', 'src/movieB', 'src/other', 'dst']) { + mkdirSync(join(root, dir), { recursive: true }) + } + writeFileSync(join(root, 'src/movieA/a.mkv'), 'a') + writeFileSync(join(root, 'src/movieB/b.mkv'), 'b') + writeFileSync(join(root, 'src/other/o.txt'), 'o') + writeFileSync(join(root, 'src/top.txt'), 'top') + writeFileSync(join(root, 'list'), 'movieA\0movieB\0') + return root + } + + it('copies its contents, and nothing that was not selected', async () => { + const root = fixture() + const plan = planTransfer({ + source: local(`${join(root, 'src')}/`), + destination: local(`${join(root, 'dst')}/`), + options: defaultRsyncOptions({ + filesFrom: join(root, 'list'), + from0: true, + dryRun: true, + }), + }) + + const result = await runToCompletion(plan) + const paths = result.changes.map((change) => change.path) + + expect(result.ok).toBe(true) + // The contents, not just the directory entry. This is the assertion that + // fails if the --recursive above is ever removed. + expect(paths).toContain('movieA/a.mkv') + expect(paths).toContain('movieB/b.mkv') + // Everything the user did not select stays out of it. + expect(paths).not.toContain('other/o.txt') + expect(paths).not.toContain('top.txt') + }) + + /* + * Mirror plus a selection deletes only inside what was selected. Verified + * rather than assumed, because the alternative reading of `--delete` here + * (everything at the destination that is not in the list) would wipe the + * folder, and that is not a thing to find out in production. + */ + it('scopes a mirror delete to the selection', async () => { + const root = fixture() + mkdirSync(join(root, 'dst/movieA'), { recursive: true }) + mkdirSync(join(root, 'dst/keepme'), { recursive: true }) + writeFileSync(join(root, 'dst/movieA/stale.mkv'), 'stale') + writeFileSync(join(root, 'dst/keepme/k.txt'), 'keep') + writeFileSync(join(root, 'dst/untouched.txt'), 'untouched') + + const plan = planTransfer({ + source: local(`${join(root, 'src')}/`), + destination: local(`${join(root, 'dst')}/`), + options: defaultRsyncOptions({ + filesFrom: join(root, 'list'), + from0: true, + deleteMode: 'delay', + dryRun: true, + }), + deletesConfirmed: true, + }) + + const result = await runToCompletion(plan) + const deletes = result.changes.filter((c) => c.action === 'delete').map((c) => c.path) + + expect(deletes).toContain('movieA/stale.mkv') + expect(deletes).not.toContain('keepme/k.txt') + expect(deletes).not.toContain('untouched.txt') + }) +}) diff --git a/packages/schemas/src/rsync-options.ts b/packages/schemas/src/rsync-options.ts index 7f6aff6..b7de4a1 100644 --- a/packages/schemas/src/rsync-options.ts +++ b/packages/schemas/src/rsync-options.ts @@ -52,6 +52,15 @@ export const RsyncOptionsSchema = z.object({ excludeFrom: z.string().min(1).nullable().default(null), includeFrom: z.string().min(1).nullable().default(null), filesFrom: z.string().min(1).nullable().default(null), + /** + * Read `filesFrom` as NUL-separated rather than one path per line. + * + * A newline is a legal character in a filename on every platform DiskPush + * runs on, so a line-separated list silently turns one such name into two + * paths that do not exist. Anything generating the list from real directory + * entries should set this. + */ + from0: z.boolean().default(false), maxSize: z.string().min(1).nullable().default(null), minSize: z.string().min(1).nullable().default(null), pruneEmptyDirs: z.boolean().default(false),