From 3d30ba5c9c81d1e46b1661cb25008ad43a5aaf74 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 14 Aug 2026 06:23:56 -0700 Subject: [PATCH 1/5] fix(promote): keep a source the cross-device move did not fully copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/commands/step/copy_ignored.rs | 10 ++-- src/commands/step/promote.rs | 73 +++++++++++++++++++++++-- src/copy.rs | 89 +++++++++++++++++++++++++++++-- 3 files changed, 160 insertions(+), 12 deletions(-) diff --git a/src/commands/step/copy_ignored.rs b/src/commands/step/copy_ignored.rs index 440d5b8212..18234d42bf 100644 --- a/src/commands/step/copy_ignored.rs +++ b/src/commands/step/copy_ignored.rs @@ -242,10 +242,12 @@ pub fn step_copy_ignored( let dest_entry = dest_path.join(relative); if *is_dir { - copy_dir_recursive(src_entry, &dest_entry, Some(&dest_path), force, &progress) - .with_context(|| { - format!("copying directory {}", format_path_for_display(relative)) - })?; + // A pure copy deletes no source, so the skip count has nothing to guard. + let _skipped = + copy_dir_recursive(src_entry, &dest_entry, Some(&dest_path), force, &progress) + .with_context(|| { + format!("copying directory {}", format_path_for_display(relative)) + })?; } else { if let Some(parent) = dest_entry.parent() { fs::create_dir_all(parent).with_context(|| { diff --git a/src/commands/step/promote.rs b/src/commands/step/promote.rs index ffec9b1a7a..5ca0c82a98 100644 --- a/src/commands/step/promote.rs +++ b/src/commands/step/promote.rs @@ -4,11 +4,12 @@ use std::fs; use std::io::ErrorKind; use std::path::{Path, PathBuf}; -use anyhow::Context; +use anyhow::{Context, bail}; use color_print::cformat; use path_slash::PathExt as _; use worktrunk::copy::{copy_dir_recursive, copy_leaf}; use worktrunk::git::Repository; +use worktrunk::path::format_path_for_display; use worktrunk::progress::Progress; use worktrunk::styling::{eprintln, hint_message, info_message, success_message, warning_message}; @@ -34,12 +35,27 @@ fn move_entry(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { } /// Copy then delete — fallback when `rename` fails with EXDEV (cross-device). +/// +/// Refuses to delete when the copy skipped an entry: the source still holds +/// content the destination never received, so the delete would destroy it. fn copy_and_remove(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { if is_dir { - copy_dir_recursive(src, dest, None, true, &Progress::disabled())?; + let skipped = copy_dir_recursive(src, dest, None, true, &Progress::disabled())?; + if skipped > 0 { + let entry_word = if skipped == 1 { "entry" } else { "entries" }; + bail!( + "{skipped} {entry_word} under {} could not be copied, so the source is left in place; run with -vv to list them", + format_path_for_display(src) + ); + } fs::remove_dir_all(src).context(format!("removing source directory {}", src.display()))?; } else { - copy_leaf(src, dest, None, true)?; + if copy_leaf(src, dest, None, true)?.is_none() { + bail!( + "{} could not be copied, so the source is left in place; run with -vv for the reason", + format_path_for_display(src) + ); + } fs::remove_file(src).context(format!("removing source file {}", src.display()))?; } @@ -509,4 +525,55 @@ mod tests { ); assert_eq!(fs::read_to_string(dest.join("root.txt")).unwrap(), "root"); } + + #[cfg(unix)] + #[test] + fn test_copy_and_remove_keeps_a_directory_holding_an_uncopied_entry() { + // The move copies then deletes, so an entry the copy skipped would be + // destroyed rather than moved. A socket is the deterministic stand-in: + // it has no copy, exactly as a source that vanished mid-walk has none. + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("srcdir"); + let dest = tmp.path().join("destdir"); + fs::create_dir_all(&src).unwrap(); + fs::write(src.join("regular.txt"), "content").unwrap(); + let _listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + + let err = copy_and_remove(&src, &dest, true).unwrap_err(); + + assert!( + err.to_string().contains("1 entry under") + && err.to_string().contains("could not be copied"), + "unexpected error: {err}" + ); + assert!(src.join("sock").exists()); + assert_eq!( + fs::read_to_string(src.join("regular.txt")).unwrap(), + "content" + ); + // The partial copy is left behind: `check_leftover_staging` is what + // stops a later promote from walking past it. + assert_eq!( + fs::read_to_string(dest.join("regular.txt")).unwrap(), + "content" + ); + } + + #[test] + fn test_copy_and_remove_keeps_a_file_that_was_not_copied() { + // The leaf branch reads `copy_leaf`'s `Ok(None)` the same way. A source + // that is already gone is the deterministic case; `fs::remove_file` + // must not be reached, since it would delete a file recreated since. + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("gone.txt"); + let dest = tmp.path().join("dest.txt"); + + let err = copy_and_remove(&src, &dest, false).unwrap_err(); + + assert!( + err.to_string().contains("could not be copied"), + "unexpected error: {err}" + ); + assert!(!dest.exists()); + } } diff --git a/src/copy.rs b/src/copy.rs index 553cda40e2..87c0f0fb83 100644 --- a/src/copy.rs +++ b/src/copy.rs @@ -22,6 +22,7 @@ use std::fs; use std::io::ErrorKind; use std::path::{Path, PathBuf}; use std::sync::LazyLock; +use std::sync::atomic::{AtomicUsize, Ordering}; use anyhow::Context; use rayon::prelude::*; @@ -187,7 +188,7 @@ struct CopyLeaf { /// /// Walks the tree iteratively (no recursion), then copies all files and /// symlinks in parallel on a dedicated 4-thread pool. Non-regular files -/// (sockets, FIFOs) are silently skipped. Existing entries at the destination +/// (sockets, FIFOs) are skipped. Existing entries at the destination /// are skipped for idempotent usage. /// /// When `force` is true, existing files and symlinks at the destination are @@ -201,6 +202,13 @@ struct CopyLeaf { /// of the source's. Every such skip logs at debug (`wt -vv`). The tree's own /// root is the exception: the caller named it, so its absence is an error. /// +/// Returns how many of the entries the walk collected were not copied. A pure +/// copy ignores it; a caller that deletes the source afterwards must refuse on a +/// non-zero count, since the source still holds content the destination never +/// received. Zero does not prove the destination holds everything the source +/// does — a file created after `read_dir` snapshots its directory is never seen, +/// and copy-then-delete cannot close that without a rename. +/// /// Each copied leaf is recorded on `progress` — /// skipped entries are not counted — so a `progress` dedicated to this call /// reports `(files_copied, bytes_copied)` in `totals()` afterwards; a shared @@ -209,14 +217,16 @@ struct CopyLeaf { /// When `root` is `Some`, refuses destination directory ancestry that resolves /// outside `root`. Leaves inherit the guarantee because `entry.file_name()` is /// a single basename and cannot escape the validated parent directory. +#[must_use = "a caller that deletes the source must refuse on a non-zero skip count"] pub fn copy_dir_recursive( src: &Path, dest: &Path, root: Option<&Path>, force: bool, progress: &Progress, -) -> anyhow::Result<()> { +) -> anyhow::Result { // Phase 1: Walk directories iteratively, creating dest dirs and collecting leaves. + let mut skipped = 0usize; let mut leaves = Vec::new(); // The bool marks the tree's own root: the caller named it, so its absence // is a real error, while a subdirectory that disappears mid-walk is the @@ -237,6 +247,7 @@ pub fn copy_dir_recursive( Ok(entries) => entries, Err(e) if e.kind() == ErrorKind::NotFound && !is_root => { tracing::debug!(path = %src_dir.display(), "skipping vanished directory: {}", src_dir.display()); + skipped += 1; continue; } Err(e) => { @@ -256,6 +267,7 @@ pub fn copy_dir_recursive( // The entry was listed but is gone already — same race. Err(e) if e.kind() == ErrorKind::NotFound => { tracing::debug!(path = %entry.path().display(), "skipping vanished entry: {}", entry.path().display()); + skipped += 1; continue; } Err(e) => { @@ -274,22 +286,30 @@ pub fn copy_dir_recursive( dest: dest_path, }); } else { + // Sockets and FIFOs have no copy, so the destination never + // receives them — a skip like any other. tracing::debug!(path = %src_path.display(), "skipping non-regular file: {}", src_path.display()); + skipped += 1; } } } // Phase 2: Copy all leaves in parallel. + let skipped_leaves = AtomicUsize::new(0); COPY_POOL.install(|| { leaves .par_iter() .try_for_each(|leaf| -> anyhow::Result<()> { - if let Some(bytes) = copy_leaf(&leaf.src, &leaf.dest, None, force)? { - progress.record(bytes); + match copy_leaf(&leaf.src, &leaf.dest, None, force)? { + Some(bytes) => progress.record(bytes), + None => { + skipped_leaves.fetch_add(1, Ordering::Relaxed); + } } Ok(()) }) })?; + skipped += skipped_leaves.into_inner(); // Phase 3: Preserve source directory permissions AFTER copying contents. // Must be done after copying — if the source lacks write permission (e.g., 0o555), @@ -303,6 +323,7 @@ pub fn copy_dir_recursive( // permissions rather than failing a batch that otherwise succeeded. Err(e) if e.kind() == ErrorKind::NotFound => { tracing::debug!(path = %src_dir.display(), "skipping permissions for vanished directory: {}", src_dir.display()); + skipped += 1; continue; } Err(e) => { @@ -314,7 +335,7 @@ pub fn copy_dir_recursive( .with_context(|| format!("setting permissions on {}", dest_dir.display()))?; } - Ok(()) + Ok(skipped) } /// Remove a file, ignoring "not found" errors. Reports whether one was removed, @@ -444,6 +465,64 @@ mod tests { assert_eq!(fs::read(&dest).unwrap(), b"source content"); } + #[test] + fn test_copy_dir_recursive_counts_skipped_existing_destination() { + // The skipped count is what tells a caller that deletes the source + // whether the destination received everything. An idempotent re-run + // skips a leaf that is already there, and says so. + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("src"); + let dest = dir.path().join("dest"); + fs::create_dir_all(&src).unwrap(); + fs::create_dir_all(&dest).unwrap(); + fs::write(src.join("a"), b"a").unwrap(); + fs::write(src.join("b"), b"b").unwrap(); + fs::write(dest.join("a"), b"pre-existing").unwrap(); + + let skipped = copy_dir_recursive(&src, &dest, None, false, &Progress::disabled()).unwrap(); + + assert_eq!(skipped, 1); + assert_eq!(fs::read(dest.join("a")).unwrap(), b"pre-existing"); + assert_eq!(fs::read(dest.join("b")).unwrap(), b"b"); + } + + #[cfg(unix)] + #[test] + fn test_copy_dir_recursive_counts_non_regular_file() { + // A socket has no copy, so the destination never receives it. Counting + // it keeps `copy_and_remove` from deleting a source that still holds + // something the destination does not. + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("src"); + let dest = dir.path().join("dest"); + fs::create_dir_all(&src).unwrap(); + fs::write(src.join("regular"), b"content").unwrap(); + let _listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + + let skipped = copy_dir_recursive(&src, &dest, None, true, &Progress::disabled()).unwrap(); + + assert_eq!(skipped, 1); + assert_eq!(fs::read(dest.join("regular")).unwrap(), b"content"); + assert!(!dest.join("sock").exists()); + } + + #[test] + fn test_copy_dir_recursive_reports_no_skips_for_a_complete_copy() { + // The control: a clean copy reports zero, which is what lets a move + // treat any non-zero count as a reason to keep the source. + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("src"); + let dest = dir.path().join("dest"); + fs::create_dir_all(src.join("nested")).unwrap(); + fs::write(src.join("a"), b"a").unwrap(); + fs::write(src.join("nested").join("b"), b"b").unwrap(); + + let skipped = copy_dir_recursive(&src, &dest, None, true, &Progress::disabled()).unwrap(); + + assert_eq!(skipped, 0); + assert_eq!(fs::read(dest.join("nested").join("b")).unwrap(), b"b"); + } + #[test] fn test_copy_dir_recursive_missing_source_root_errors() { // A subdirectory that vanishes mid-walk is skipped, but the root the From ad977d30399e2f880c30b8ecefd0ffc80d181d78 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 14 Aug 2026 06:23:57 -0700 Subject: [PATCH 2/5] Release v0.74.0 --- CHANGELOG.md | 28 ++++++++++++++++++++++++++-- Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18334de5f3..5a06368e44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,37 @@ # Changelog -## Unreleased +## 0.74.0 ### Improved -- **`wt config approvals add --yes` records approvals without a terminal**: the command refused every non-interactive run, so a container that had just cloned a project could only pre-approve it by hand-writing `approvals.toml` from `wt config approvals list --format=json`. `--yes` now lists what it trusts and writes it. A save it cannot make fails the command rather than warning and exiting 0, since the record is all `add` produces — a change that applies to the interactive run too. +- **A JSON `true` or `null` nested in a `vars` value renders as `True` or `None`**: minijinja 2.22 adopted Jinja2's spelling, so `{{ vars.. }}` into an object set by `wt config state vars set` prints those words. A var whose whole value is `true` stays a string. (Breaking: the old spellings were `true` and `none`.) ([#3795](https://github.com/max-sixty/worktrunk/pull/3795)) + +- **`wt config approvals add --yes` records approvals without a terminal**: the command refused every non-interactive run, so a container or CI job could not pre-approve a project it had just cloned. `--yes` now lists what it trusts and writes it, and a save it cannot make fails the command rather than warning and exiting 0. ([#3819](https://github.com/max-sixty/worktrunk/pull/3819)) - **`remote_repo` names the repository as the remote spells it**: `repo` is the directory on disk, so a renamed clone reports the new name. `{{ remote_repo }}` takes it from the primary remote's URL, available everywhere `owner` is and unset when no remote parses. ([#3745](https://github.com/max-sixty/worktrunk/pull/3745), thanks @canac) +### Fixed + +- **`wt remove --force` deleted a live worktree moved onto another's registered path**: the ownership gate asked which *repository* the occupant belonged to, which a sibling worktree of this repo satisfies, so the directory went to trash with its uncommitted work. The directory and the registration must now name each other, as `git worktree remove` requires. Also reaches `wt merge`, `wt step prune`, and picker removal, and refuses a bare repository's path that `--force` previously deleted. ([#3808](https://github.com/max-sixty/worktrunk/pull/3808)) + +- **`wt step copy-ignored` skips a source file that vanishes mid-copy**: the walk collects leaves, then copies them in parallel, so a file removed in between — a concurrent build rewriting `target/`, say — aborted the batch, under `--force` after deleting the destination. The source is stat'd before anything is removed now. ([#3744](https://github.com/max-sixty/worktrunk/pull/3744), fixes [#3743](https://github.com/max-sixty/worktrunk/issues/3743), thanks @dataders for reporting and fixing) + +- **`wt step promote` refuses to delete a source it did not fully copy**: across filesystems the move copies then deletes, and an entry with no copy — a socket or FIFO, or a file a concurrent build rewrote mid-walk — was deleted anyway. The copy now reports what it skipped, and a non-zero count fails the move. + +- **An `ssh://` remote's host comes from the authority, not a later path segment**: the search for the userinfo `@` covered the whole remainder, so `ssh://git@attacker.example/owner/repo@github.com/org/repo.git` resolved to `github.com` and borrowed that repository's approvals while git dialed the attacker. Every URL form now splits the authority first, so `@` in a namespace parses instead of being rejected. ([#3813](https://github.com/max-sixty/worktrunk/pull/3813)) + +- **`WORKTRUNK_*` env vars and `--config-set` outrank a `[projects."…"]` entry**: layer and specificity resolved separately, so a project entry answered for the global key whichever layer set it — `WORKTRUNK_WORKTREE_PATH` could not override a project's `worktree-path`. Both invocation layers now apply at either scope, and the help text gained a precedence section. ([#3790](https://github.com/max-sixty/worktrunk/pull/3790), fixes [#3788](https://github.com/max-sixty/worktrunk/issues/3788), thanks @jeremy0dell for reporting) + +- **Shell completions register under the `--cmd` name**: `wt config shell init zsh --cmd wot` renamed the wrapper, but clap emitted the registration under `wt`. Bash and zsh completed nothing and regenerated the script on every TAB, PowerShell registered the wrong command, and zsh's `compdef` handed worktrunk's completer to the other `wt`. ([#3817](https://github.com/max-sixty/worktrunk/pull/3817), fixes [#3816](https://github.com/max-sixty/worktrunk/issues/3816), thanks @sbarre for reporting) + +### Documentation + +- **The picker's `Alt-x` never forces**: the keybinding table read `Remove selected worktree/branch`, which sent a reader looking for a force-remove that isn't there. ([#3811](https://github.com/max-sixty/worktrunk/pull/3811), raised in [#3809](https://github.com/max-sixty/worktrunk/issues/3809), thanks @josh-burton) + +### Internal + +- **Library API rework** (Breaking library API): `GitError::WorktreePathNotOurs` gained an `occupant_registered_at` field, and `WorkingTree::ensure_belongs_to_repo` is now `ensure_holds_this_worktree`. ([#3808](https://github.com/max-sixty/worktrunk/pull/3808)) + ## 0.73.0 ### Fixed diff --git a/Cargo.lock b/Cargo.lock index 0d398db282..61c1e11cf0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4810,7 +4810,7 @@ dependencies = [ [[package]] name = "worktrunk" -version = "0.73.0" +version = "0.74.0" dependencies = [ "ansi-str", "ansi-to-html", diff --git a/Cargo.toml b/Cargo.toml index 5a0324e0ea..8ff400abc4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,7 +49,7 @@ filterset = "test(/readme_sync/)" [package] name = "worktrunk" -version = "0.73.0" +version = "0.74.0" edition = "2024" rust-version = "1.96" build = "build.rs" From 49b1c90ae8ce3bb0358fc4923a2d02ebb37b5a60 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 14 Aug 2026 06:26:31 -0700 Subject: [PATCH 3/5] docs(changelog): cite the promote fix's PR --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a06368e44..e07a408467 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ - **`wt step copy-ignored` skips a source file that vanishes mid-copy**: the walk collects leaves, then copies them in parallel, so a file removed in between — a concurrent build rewriting `target/`, say — aborted the batch, under `--force` after deleting the destination. The source is stat'd before anything is removed now. ([#3744](https://github.com/max-sixty/worktrunk/pull/3744), fixes [#3743](https://github.com/max-sixty/worktrunk/issues/3743), thanks @dataders for reporting and fixing) -- **`wt step promote` refuses to delete a source it did not fully copy**: across filesystems the move copies then deletes, and an entry with no copy — a socket or FIFO, or a file a concurrent build rewrote mid-walk — was deleted anyway. The copy now reports what it skipped, and a non-zero count fails the move. +- **`wt step promote` refuses to delete a source it did not fully copy**: across filesystems the move copies then deletes, and an entry with no copy — a socket or FIFO, or a file a concurrent build rewrote mid-walk — was deleted anyway. The copy now reports what it skipped, and a non-zero count fails the move. ([#3820](https://github.com/max-sixty/worktrunk/pull/3820)) - **An `ssh://` remote's host comes from the authority, not a later path segment**: the search for the userinfo `@` covered the whole remainder, so `ssh://git@attacker.example/owner/repo@github.com/org/repo.git` resolved to `github.com` and borrowed that repository's approvals while git dialed the attacker. Every URL form now splits the authority first, so `@` in a namespace parses instead of being rejected. ([#3813](https://github.com/max-sixty/worktrunk/pull/3813)) From d5e86a6fffeee71fc442e982400b7a87096dd46d Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 14 Aug 2026 08:16:23 -0700 Subject: [PATCH 4/5] fix(promote): let a non-regular file through the copy's refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 2 +- src/commands/step/promote.rs | 89 ++++++++++++++++-------------------- src/copy.rs | 27 +++++------ 3 files changed, 55 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e07a408467..7c98f04cdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Improved -- **A JSON `true` or `null` nested in a `vars` value renders as `True` or `None`**: minijinja 2.22 adopted Jinja2's spelling, so `{{ vars.. }}` into an object set by `wt config state vars set` prints those words. A var whose whole value is `true` stays a string. (Breaking: the old spellings were `true` and `none`.) ([#3795](https://github.com/max-sixty/worktrunk/pull/3795)) +- **A JSON bool or null nested in a `vars` value renders as `True`, `False`, or `None`**: minijinja 2.22 adopted Jinja2's spelling for all three, so `{{ vars.. }}` into an object set by `wt config state vars set` prints those words. A var whose whole value is `true` stays a string. (Breaking: the old spellings were `true`, `false`, and `none`.) ([#3795](https://github.com/max-sixty/worktrunk/pull/3795)) - **`wt config approvals add --yes` records approvals without a terminal**: the command refused every non-interactive run, so a container or CI job could not pre-approve a project it had just cloned. `--yes` now lists what it trusts and writes it, and a save it cannot make fails the command rather than warning and exiting 0. ([#3819](https://github.com/max-sixty/worktrunk/pull/3819)) diff --git a/src/commands/step/promote.rs b/src/commands/step/promote.rs index 5ca0c82a98..355d284334 100644 --- a/src/commands/step/promote.rs +++ b/src/commands/step/promote.rs @@ -39,24 +39,21 @@ fn move_entry(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { /// Refuses to delete when the copy skipped an entry: the source still holds /// content the destination never received, so the delete would destroy it. fn copy_and_remove(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { + let skipped = if is_dir { + copy_dir_recursive(src, dest, None, true, &Progress::disabled())? + } else { + usize::from(copy_leaf(src, dest, None, true)?.is_none()) + }; + if skipped > 0 { + bail!( + "{} was not fully copied, so the source is left in place; run with -vv to list what was skipped", + format_path_for_display(src) + ); + } + if is_dir { - let skipped = copy_dir_recursive(src, dest, None, true, &Progress::disabled())?; - if skipped > 0 { - let entry_word = if skipped == 1 { "entry" } else { "entries" }; - bail!( - "{skipped} {entry_word} under {} could not be copied, so the source is left in place; run with -vv to list them", - format_path_for_display(src) - ); - } fs::remove_dir_all(src).context(format!("removing source directory {}", src.display()))?; } else { - if copy_leaf(src, dest, None, true)?.is_none() { - bail!( - "{} could not be copied, so the source is left in place; run with -vv for the reason", - format_path_for_display(src) - ); - } - fs::remove_file(src).context(format!("removing source file {}", src.display()))?; } Ok(()) @@ -526,54 +523,48 @@ mod tests { assert_eq!(fs::read_to_string(dest.join("root.txt")).unwrap(), "root"); } - #[cfg(unix)] #[test] - fn test_copy_and_remove_keeps_a_directory_holding_an_uncopied_entry() { + fn test_copy_and_remove_keeps_a_source_that_was_not_fully_copied() { // The move copies then deletes, so an entry the copy skipped would be - // destroyed rather than moved. A socket is the deterministic stand-in: - // it has no copy, exactly as a source that vanished mid-walk has none. + // destroyed rather than moved. A source that is already gone is the + // deterministic case: `copy_leaf` reports the skip, and the delete — + // which would take a file recreated since — must not be reached. let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("srcdir"); - let dest = tmp.path().join("destdir"); - fs::create_dir_all(&src).unwrap(); - fs::write(src.join("regular.txt"), "content").unwrap(); - let _listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + let src = tmp.path().join("gone.txt"); + let dest = tmp.path().join("dest.txt"); - let err = copy_and_remove(&src, &dest, true).unwrap_err(); + let err = copy_and_remove(&src, &dest, false).unwrap_err(); assert!( - err.to_string().contains("1 entry under") - && err.to_string().contains("could not be copied"), + err.to_string().contains("was not fully copied"), "unexpected error: {err}" ); - assert!(src.join("sock").exists()); - assert_eq!( - fs::read_to_string(src.join("regular.txt")).unwrap(), - "content" - ); - // The partial copy is left behind: `check_leftover_staging` is what - // stops a later promote from walking past it. - assert_eq!( - fs::read_to_string(dest.join("regular.txt")).unwrap(), - "content" - ); + assert!(!dest.exists()); } + #[cfg(unix)] #[test] - fn test_copy_and_remove_keeps_a_file_that_was_not_copied() { - // The leaf branch reads `copy_leaf`'s `Ok(None)` the same way. A source - // that is already gone is the deterministic case; `fs::remove_file` - // must not be reached, since it would delete a file recreated since. + fn test_copy_and_remove_drops_a_non_regular_file() { + // A socket has no content the destination can be short of, so it does + // not hold up the move. Refusing over one would strand real content: + // `distribute_staged` runs after the branch exchange, and a promote + // that dies there leaves the staged files behind a `check_leftover_ + // staging` refusal whose remedy deletes them. let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("gone.txt"); - let dest = tmp.path().join("dest.txt"); + let src = tmp.path().join("srcdir"); + let dest = tmp.path().join("destdir"); + fs::create_dir_all(&src).unwrap(); + fs::write(src.join("regular.txt"), "content").unwrap(); + let listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + drop(listener); - let err = copy_and_remove(&src, &dest, false).unwrap_err(); + copy_and_remove(&src, &dest, true).unwrap(); - assert!( - err.to_string().contains("could not be copied"), - "unexpected error: {err}" + assert!(!src.exists()); + assert_eq!( + fs::read_to_string(dest.join("regular.txt")).unwrap(), + "content" ); - assert!(!dest.exists()); + assert!(!dest.join("sock").exists()); } } diff --git a/src/copy.rs b/src/copy.rs index 87c0f0fb83..74f2db593b 100644 --- a/src/copy.rs +++ b/src/copy.rs @@ -202,10 +202,13 @@ struct CopyLeaf { /// of the source's. Every such skip logs at debug (`wt -vv`). The tree's own /// root is the exception: the caller named it, so its absence is an error. /// -/// Returns how many of the entries the walk collected were not copied. A pure -/// copy ignores it; a caller that deletes the source afterwards must refuse on a -/// non-zero count, since the source still holds content the destination never -/// received. Zero does not prove the destination holds everything the source +/// Returns how many of the entries the walk collected to copy were not copied. +/// A pure copy ignores it; a caller that deletes the source afterwards must +/// refuse on a non-zero count, since the source still holds content the +/// destination never received. A non-regular file is dropped at classification +/// rather than counted: it carries no content the destination can be short of, +/// so refusing over one would cost a caller the whole operation to protect +/// nothing. Zero does not prove the destination holds everything the source /// does — a file created after `read_dir` snapshots its directory is never seen, /// and copy-then-delete cannot close that without a rename. /// @@ -286,10 +289,7 @@ pub fn copy_dir_recursive( dest: dest_path, }); } else { - // Sockets and FIFOs have no copy, so the destination never - // receives them — a skip like any other. tracing::debug!(path = %src_path.display(), "skipping non-regular file: {}", src_path.display()); - skipped += 1; } } } @@ -488,20 +488,21 @@ mod tests { #[cfg(unix)] #[test] - fn test_copy_dir_recursive_counts_non_regular_file() { - // A socket has no copy, so the destination never receives it. Counting - // it keeps `copy_and_remove` from deleting a source that still holds - // something the destination does not. + fn test_copy_dir_recursive_does_not_count_a_non_regular_file() { + // A socket is dropped at classification, not counted: the count exists + // so a caller that deletes the source can refuse, and a socket carries + // no content that refusal would protect. let dir = tempfile::tempdir().unwrap(); let src = dir.path().join("src"); let dest = dir.path().join("dest"); fs::create_dir_all(&src).unwrap(); fs::write(src.join("regular"), b"content").unwrap(); - let _listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + let listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); + drop(listener); let skipped = copy_dir_recursive(&src, &dest, None, true, &Progress::disabled()).unwrap(); - assert_eq!(skipped, 1); + assert_eq!(skipped, 0); assert_eq!(fs::read(dest.join("regular")).unwrap(), b"content"); assert!(!dest.join("sock").exists()); } From 2c6e7aff1e922396567d7b849147532d6062acf5 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 14 Aug 2026 09:30:22 -0700 Subject: [PATCH 5/5] docs(promote): say what the cross-device move destroys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/commands/step/promote.rs | 9 +++++++-- src/copy.rs | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/commands/step/promote.rs b/src/commands/step/promote.rs index 355d284334..d1e664a133 100644 --- a/src/commands/step/promote.rs +++ b/src/commands/step/promote.rs @@ -36,8 +36,10 @@ fn move_entry(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { /// Copy then delete — fallback when `rename` fails with EXDEV (cross-device). /// -/// Refuses to delete when the copy skipped an entry: the source still holds -/// content the destination never received, so the delete would destroy it. +/// Refuses to delete when the copy reports a skip, since the source still holds +/// content the destination never received. A socket or FIFO is not a skip and +/// goes with the rest of the source: it carries no content to lose, and +/// refusing over one would abort a promote after the branch exchange. fn copy_and_remove(src: &Path, dest: &Path, is_dir: bool) -> anyhow::Result<()> { let skipped = if is_dir { copy_dir_recursive(src, dest, None, true, &Progress::disabled())? @@ -557,6 +559,9 @@ mod tests { fs::write(src.join("regular.txt"), "content").unwrap(); let listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); drop(listener); + // Closing the fd doesn't unlink, so the socket is still there to + // classify. Asserted, so the test can't pass on a fixture that made none. + assert!(src.join("sock").exists()); copy_and_remove(&src, &dest, true).unwrap(); diff --git a/src/copy.rs b/src/copy.rs index 74f2db593b..a4e9673543 100644 --- a/src/copy.rs +++ b/src/copy.rs @@ -499,6 +499,9 @@ mod tests { fs::write(src.join("regular"), b"content").unwrap(); let listener = std::os::unix::net::UnixListener::bind(src.join("sock")).unwrap(); drop(listener); + // Closing the fd doesn't unlink, so the socket is still there to + // classify. Asserted, so a zero count can't come from an empty fixture. + assert!(src.join("sock").exists()); let skipped = copy_dir_recursive(&src, &dest, None, true, &Progress::disabled()).unwrap();