Release v0.74.0 - #3820
Conversation
`copy_and_remove` is the EXDEV fallback behind `wt step promote`'s staging moves: it copies the entry, then unconditionally deletes the source. #3744 made the copy tolerate a source that vanishes mid-walk — right for `wt step copy-ignored`, which never deletes a source, and wrong here, because the delete then takes content the destination never received. A gitignored file a concurrent build removed and rewrote is exactly the case that tolerance was built for, so the copy could report success having skipped it and the delete would destroy the rewritten file. Before #3744 the copy errored and the source survived. `copy_dir_recursive` now returns how many of the entries the walk collected were not copied, so a caller that deletes the source can tell a complete copy from a partial one. `copy_and_remove` refuses on a non-zero count and leaves the source in place; the leaf branch reads `copy_leaf`'s existing `Ok(None)` the same way. `wt step copy-ignored` is a pure copy, so it names the count and drops it — `#[must_use]` is what makes a third caller decide rather than inherit the old bug. Non-regular files (sockets, FIFOs) are skips on the same terms: they have no copy, so the destination never receives them, and the move deleted one from the source regardless. That case predates #3744 and is not a race, which is why a Rails `tmp/` socket makes the refusal deterministic enough to test. The count folds several causes together, so the error states only what it knows — how many entries were not copied — and points at `-vv`, where the walk already logs each skip and its reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
Withholding approval, not as a verdict on the change: .claude/skills/running-tend/references/review-pr.md § "Data-Loss Surface: Hold for Human Review" holds any diff that edits a file containing std::fs::remove_dir_all / std::fs::remove_file, and copy_and_remove in src/commands/step/promote.rs is that file. You authored this, so the human the rule asks for is already on it.
The fix traces correctly: with force = true the only Ok(None) paths left in copy_leaf are the vanished-source skips and the AlreadyExists race, both of which now land in the count, so the delete is unreachable when the destination is short. All three counted phase-1/3 skips log at debug, so run with -vv to list them delivers what it promises.
codecov/patch is red — 92 patch lines, 89 hits, 3 misses = 96.74% against an auto target of 98.1%. The three misses are the skipped += 1; counters at src/copy.rs lines 250, 270 and 326, each inside a pre-existing ErrorKind::NotFound arm whose own body already reads as a miss on the base commit (the codecov compare reports coverage.base == 1 for the tracing::debug! line above each one). They're the read_dir-vanished, file_type-vanished, and set_permissions-vanished races — no deterministic trigger, so the red is three lines pulled into an already-uncovered path rather than new untested logic. Per the root CLAUDE.md → Coverage that's the "ask before merging" case; handing over the arithmetic rather than contorting a test for it.
One design question inline on the socket/FIFO half, and one changelog accuracy fix.
Counting sockets and FIFOs toward the skip count changed an outcome without preventing a loss. A socket carries no content the destination can be short of, which is the whole justification for refusing, and the refusal lands worst where it is least recoverable: `distribute_staged` runs *after* `exchange_branches`, and staging sits under the git common dir, so a socket that reached staging by same-filesystem rename blocks the move out to a worktree on another filesystem. The promote then dies with the branches already swapped and the staged files behind a `check_leftover_staging` refusal whose remedy is to delete them. So the walk drops a non-regular file at classification, as it did before, and the count keeps to entries that were collected to copy. The socket case is pinned both ways: `copy_dir_recursive` reports zero, and `copy_and_remove` completes and deletes it, which is the pre-existing behavior now written down. The two branches share one guard, since they were asking the same question and answering it in two messages. That also puts the refusal on the deterministic path — a source that is already gone — rather than only on one reachable through a race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The split landed the way the thread asked: copy_dir_recursive drops a non-regular file at classification, so the count now means only "content the destination is short of" and a socket under an ignored directory no longer aborts a cross-device promote after the branch exchange. Both directions are pinned, and hoisting the skipped > 0 guard above the dir/leaf branch means one test covers the refusal for both.
codecov/patch is still red at 96.51% (86 patch lines, 83 hits). The three misses are the same skipped += 1; counters in the vanished-source arms the earlier review handed over — src/copy.rs lines 253, 273 and 326 — so the incremental added no new uncovered logic; it removed one covered patch line and left the miss set unchanged. Holding approval for the same reason as before, not on anything in this commit.
Two small things inline.
Two fixes from review, neither behavioral. `copy_and_remove`'s doc comment claimed the refusal covers everything the copy dropped. Since the split it doesn't: a socket or FIFO under `src` is dropped at classification and then taken by `remove_dir_all`. The rationale lives on `copy_dir_recursive`, but this is the deletion site, so it says what goes. Both socket tests asserted only absences — a zero count and no socket at the destination — which a fixture that created no socket would also satisfy. Each now asserts the socket exists before the call, since dropping a `UnixListener` closes the fd without unlinking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Cuts 0.74.0. Minor bump:
cargo semver-checksreports two breaking library changes from #3808 (GitError::WorktreePathNotOursgained a field,WorkingTree::ensure_belongs_to_repowas renamed), and patch is disallowed pre-1.0 with semver breakage.Alongside the release, one fix the release's data-loss review turned up.
The fix
wt step promotestages a worktree's gitignored files through<git-common-dir>/wt/staging/promoteand moves them back after the branch exchange. When a worktree and the git dir sit on different filesystems,fs::renamefails with EXDEV andcopy_and_removecopies then unconditionally deletes the source.#3744 made the copy tolerate a source that vanishes mid-walk — correct for
wt step copy-ignored, which never deletes a source, and wrong here: an incomplete copy reportedOkimmediately before the delete, so a gitignored file a concurrent build removed and rewrote was destroyed rather than moved. Before #3744 the copy errored and the source survived, so this was a regression introduced in this release window and caught before it shipped.copy_dir_recursivenow returns how many of the entries the walk collected to copy were not copied.copy_and_removerefuses on a non-zero count and leaves the source in place;copy-ignorednames the count and drops it, with#[must_use]so a future third caller decides rather than inheriting the old bug.A non-regular file is dropped at classification rather than counted, per @worktrunk-bot's review: a socket carries no content the destination can be short of, and refusing over one lands worst where it is least recoverable —
distribute_stagedruns afterexchange_branches, so a socket that reached staging by same-filesystem rename would kill the promote with the branches already swapped and the staged files behind acheck_leftover_stagingrefusal whose remedy deletes them. Both directions are pinned by tests.Release gates
wt hook pre-merge --yes: 4649 tests, lints, doctests, rustdoc under-Dwarnings.nightly.yamlgreen twice on this branch (full 3-OS matrix, feature-powerset, release-target, nix-flake, minimal-versions), and green on the cut-from tip 92dfb68.v0.73.0..HEADwith four independent finders (behavioral, blast-radius, shipped-automation, keyword). Thirteen candidates: one real, fixed here; twelve adjudicated acceptable and signed off.Known-red check
codecov/patchfails on the threeskipped += 1;counters insrc/copy.rs. Each sits inside a pre-existingErrorKind::NotFoundarm that was already uncovered at the base commit — the concurrent-rewrite races (read_dirvanished,entry.file_type()vanished,set_permissionsvanished), none with a deterministic trigger.codecov/projectpasses. Merging over it is approved.