Skip to content

Commit 160d0c7

Browse files
max-sixtyclaude
andcommitted
fix(relocate): atomic no-overwrite rename for --clobber backup (#2865)
`wt switch --clobber` and `wt step relocate --clobber` each had their own logic for backing up a path blocking the target, and the two had diverged. relocate used an `exists()` check followed by `std::fs::rename` — a time-of-check/time-of-use race, since `std::fs::rename` silently replaces an existing file or empty directory on Unix and could destroy a just-created backup. switch (#2849) instead used `renamore::rename_exclusive`, an atomic no-overwrite rename, and counted up through suffixes (`…-2`, `…-3`, …) on a name collision rather than failing. This PR replaces both with one shared helper in `commands::backup`. `back_up_clobbered_path_now` computes the timestamped name and delegates to the suffix-iterating `back_up_clobbered_path`, which moves the blocker with `renamore::rename_exclusive` — atomic and no-overwrite, so an existing backup is never clobbered; a name collision just lands on the next free `-N` name. relocate gains the suffix fallback (previously it failed closed on `AlreadyExists`), the timestamp computation is no longer duplicated across the two call sites, and `renamore::rename_exclusive` now has a single caller. `renamore` is already a direct dependency and keeps the platform FFI and its `unsafe` inside that crate, so worktrunk stays `unsafe_code = "forbid"`. relocate's backup name changes from `.bak-<timestamp>` to the extension-aware `.bak.<timestamp>` form used by switch (e.g. `repo.feature` → `repo.feature.bak.<timestamp>`). The `test_relocate_clobber_error_backup_exists` regression test asserted the old fail-closed behavior and is rewritten as `test_relocate_clobber_falls_back_when_backup_taken`, mirroring the switch test. CLI help and the generated doc mirrors are updated for the new naming and fallback. The shared helper carries the suffix-iteration, collision, and missing-source unit tests (moved from `resolve.rs`); the relocate `--clobber` path keeps its integration coverage. > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f6343d7 commit 160d0c7

11 files changed

Lines changed: 305 additions & 248 deletions

File tree

docs/content/step.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,9 @@ this by using a temporary location.
826826
### Clobbering
827827

828828
With `--clobber`, non-worktree paths at target locations are moved to
829-
`<path>.bak-<timestamp>` before relocating.
829+
`<path>.bak.<timestamp>` before relocating. If that name is already taken,
830+
the move counts up (`…-2`, `…-3`, …) until it finds a free name, so an
831+
existing backup is never overwritten.
830832

831833
### Main worktree behavior
832834

@@ -866,7 +868,8 @@ Usage: <b><span class=c>wt step relocate</span></b> <span class=c>[OPTIONS]</spa
866868
<b><span class=c>--clobber</span></b>
867869
Backup non-worktree paths at target locations
868870

869-
Moves blocking paths to <b>&lt;path&gt;.bak-&lt;timestamp&gt;</b>.
871+
Moves blocking paths to <b>&lt;path&gt;.bak.&lt;timestamp&gt;</b>. If that name is taken, counts up (<b>…-2</b>, <b>…-3</b>
872+
, …) to a free name.
870873

871874
<b><span class=c>-h</span></b>, <b><span class=c>--help</span></b>
872875
Print help (see a summary with &#39;-h&#39;)

skills/worktrunk/reference/step.md

Lines changed: 5 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/cli/step.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,7 +631,9 @@ this by using a temporary location.
631631
## Clobbering
632632
633633
With `--clobber`, non-worktree paths at target locations are moved to
634-
`<path>.bak-<timestamp>` before relocating.
634+
`<path>.bak.<timestamp>` before relocating. If that name is already taken,
635+
the move counts up (`…-2`, `…-3`, …) until it finds a free name, so an
636+
existing backup is never overwritten.
635637
636638
## Main worktree behavior
637639
@@ -663,7 +665,8 @@ Note: This command is experimental and may change in future versions.
663665

664666
/// Backup non-worktree paths at target locations
665667
///
666-
/// Moves blocking paths to `<path>.bak-<timestamp>`.
668+
/// Moves blocking paths to `<path>.bak.<timestamp>`. If that name is
669+
/// taken, counts up (`…-2`, `…-3`, …) to a free name.
667670
#[arg(long)]
668671
clobber: bool,
669672

src/commands/backup.rs

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
//! Atomic no-overwrite backup of a path blocking a `--clobber` move.
2+
//!
3+
//! Both `wt switch --clobber` and `wt step relocate --clobber` need to move a
4+
//! stale or blocking path aside before they can take its place. They share this
5+
//! helper so the two paths behave identically: the backup name and the atomic
6+
//! no-overwrite rename are defined once.
7+
//!
8+
//! # Why an atomic rename
9+
//!
10+
//! The backup name is only second-resolution (`.bak.<YYYYmmdd-HHMMSS>`), so an
11+
//! `exists()` check followed by a rename would race: another process could
12+
//! create that path in the gap. [`renamore::rename_exclusive`] closes the gap
13+
//! — an atomic no-overwrite rename (`renameat2(RENAME_NOREPLACE)` on Linux,
14+
//! `renamex_np(RENAME_EXCL)` on macOS, `MoveFileExW` on Windows) that fails
15+
//! closed rather than overwriting an existing backup. `std::fs::rename`
16+
//! silently replaces an existing file or empty directory and cannot be used
17+
//! here. A name collision is not fatal: the move counts up (`…-2`, `…-3`, …)
18+
//! until it lands on a free name.
19+
20+
use std::path::{Path, PathBuf};
21+
22+
use worktrunk::path::format_path_for_display;
23+
24+
/// Generate a backup path for the given path with a timestamp suffix.
25+
///
26+
/// For paths with extensions: `file.txt` → `file.txt.bak.TIMESTAMP`
27+
/// For paths without extensions: `foo` → `foo.bak.TIMESTAMP`
28+
///
29+
/// Returns an error for unusual paths without a file name (e.g., `/` or `..`).
30+
fn generate_backup_path(path: &Path, suffix: &str) -> anyhow::Result<PathBuf> {
31+
let file_name = path.file_name().ok_or_else(|| {
32+
anyhow::anyhow!(
33+
"Cannot generate backup path for {}",
34+
format_path_for_display(path)
35+
)
36+
})?;
37+
38+
if path.extension().is_none() {
39+
// Path has no extension (e.g., /repo/feature)
40+
Ok(path.with_file_name(format!("{}.bak.{suffix}", file_name.to_string_lossy())))
41+
} else {
42+
// Path has an extension (e.g., /repo.feature or /file.txt)
43+
Ok(path.with_extension(format!(
44+
"{}.bak.{suffix}",
45+
path.extension()
46+
.map(|e| e.to_string_lossy().to_string())
47+
.unwrap_or_default()
48+
)))
49+
}
50+
}
51+
52+
/// Move `blocking_path` aside to a `.bak.<base_suffix>` sibling.
53+
///
54+
/// If that name is already taken — a same-second clobber, or a path that raced
55+
/// in after planning — it counts up (`…-2`, `…-3`, …) until it finds a free
56+
/// name. Every attempt is an atomic no-overwrite rename
57+
/// ([`renamore::rename_exclusive`]), so an existing backup is never overwritten;
58+
/// the move just lands on the next free name. Returns the path the blocking
59+
/// directory was moved to.
60+
///
61+
/// `base_suffix` is a parameter rather than computed internally so tests can
62+
/// pass a fixed value; [`back_up_clobbered_path_now`] is the production entry
63+
/// point that derives the timestamp.
64+
fn back_up_clobbered_path(blocking_path: &Path, base_suffix: &str) -> anyhow::Result<PathBuf> {
65+
// Count up until a free name is found. This cannot spin forever: a
66+
// directory holds finitely many entries, so some `-N` is always unused.
67+
let mut n: u64 = 1;
68+
loop {
69+
// First attempt uses the bare suffix; later ones disambiguate with -N.
70+
let suffix = if n == 1 {
71+
base_suffix.to_string()
72+
} else {
73+
format!("{base_suffix}-{n}")
74+
};
75+
let candidate = generate_backup_path(blocking_path, &suffix)?;
76+
match renamore::rename_exclusive(blocking_path, &candidate) {
77+
Ok(()) => return Ok(candidate),
78+
Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => n += 1,
79+
Err(err) => {
80+
return Err(anyhow::Error::new(err).context(format!(
81+
"Failed to move {} to {}",
82+
format_path_for_display(blocking_path),
83+
format_path_for_display(&candidate),
84+
)));
85+
}
86+
}
87+
}
88+
}
89+
90+
/// Move `blocking_path` aside to a timestamped `.bak.<YYYYmmdd-HHMMSS>` sibling.
91+
///
92+
/// Wraps [`back_up_clobbered_path`] with the timestamp suffix computed at move
93+
/// time, so the suffix reflects when the path is actually set aside. Returns the
94+
/// path the blocking directory was moved to.
95+
pub(crate) fn back_up_clobbered_path_now(blocking_path: &Path) -> anyhow::Result<PathBuf> {
96+
let timestamp_secs = worktrunk::utils::epoch_now() as i64;
97+
let datetime =
98+
chrono::DateTime::from_timestamp(timestamp_secs, 0).unwrap_or_else(chrono::Utc::now);
99+
let base_suffix = datetime.format("%Y%m%d-%H%M%S").to_string();
100+
back_up_clobbered_path(blocking_path, &base_suffix)
101+
}
102+
103+
#[cfg(test)]
104+
mod tests {
105+
use super::*;
106+
107+
#[test]
108+
fn test_generate_backup_path_with_extension() {
109+
// Paths with extensions: file.txt -> file.txt.bak.TIMESTAMP
110+
let path = PathBuf::from("/tmp/repo.feature");
111+
let backup = generate_backup_path(&path, "20250101-000000").unwrap();
112+
assert_eq!(
113+
backup,
114+
PathBuf::from("/tmp/repo.feature.bak.20250101-000000")
115+
);
116+
117+
let path = PathBuf::from("/tmp/file.txt");
118+
let backup = generate_backup_path(&path, "20250101-000000").unwrap();
119+
assert_eq!(backup, PathBuf::from("/tmp/file.txt.bak.20250101-000000"));
120+
}
121+
122+
#[test]
123+
fn test_generate_backup_path_without_extension() {
124+
// Paths without extensions: foo -> foo.bak.TIMESTAMP
125+
let path = PathBuf::from("/tmp/repo/feature");
126+
let backup = generate_backup_path(&path, "20250101-000000").unwrap();
127+
assert_eq!(
128+
backup,
129+
PathBuf::from("/tmp/repo/feature.bak.20250101-000000")
130+
);
131+
132+
let path = PathBuf::from("/tmp/mydir");
133+
let backup = generate_backup_path(&path, "20250101-000000").unwrap();
134+
assert_eq!(backup, PathBuf::from("/tmp/mydir.bak.20250101-000000"));
135+
}
136+
137+
#[test]
138+
fn test_generate_backup_path_unusual_paths() {
139+
// Root path has no file name
140+
let path = PathBuf::from("/");
141+
assert!(generate_backup_path(&path, "20250101-000000").is_err());
142+
143+
// Parent reference has no file name
144+
let path = PathBuf::from("..");
145+
assert!(generate_backup_path(&path, "20250101-000000").is_err());
146+
}
147+
148+
#[test]
149+
fn test_back_up_clobbered_path_moves_to_fresh_suffix() {
150+
let temp = tempfile::tempdir().unwrap();
151+
let stale = temp.path().join("feature");
152+
std::fs::create_dir(&stale).unwrap();
153+
std::fs::write(stale.join("file"), "content").unwrap();
154+
155+
let used = back_up_clobbered_path(&stale, "20250101-000000").unwrap();
156+
157+
assert_eq!(used, temp.path().join("feature.bak.20250101-000000"));
158+
assert!(!stale.exists(), "stale path should be moved away");
159+
assert_eq!(
160+
std::fs::read_to_string(used.join("file")).unwrap(),
161+
"content"
162+
);
163+
}
164+
165+
#[test]
166+
fn test_back_up_clobbered_path_falls_back_when_suffix_taken() {
167+
let temp = tempfile::tempdir().unwrap();
168+
let stale = temp.path().join("feature");
169+
std::fs::create_dir(&stale).unwrap();
170+
171+
// The preferred backup name and its first -N variant are both taken.
172+
let taken = temp.path().join("feature.bak.20250101-000000");
173+
std::fs::create_dir(&taken).unwrap();
174+
std::fs::write(taken.join("keep"), "pre-existing").unwrap();
175+
std::fs::create_dir(temp.path().join("feature.bak.20250101-000000-2")).unwrap();
176+
177+
let used = back_up_clobbered_path(&stale, "20250101-000000").unwrap();
178+
179+
// Lands on -3; neither pre-existing backup is overwritten.
180+
assert_eq!(used, temp.path().join("feature.bak.20250101-000000-3"));
181+
assert!(!stale.exists());
182+
assert_eq!(
183+
std::fs::read_to_string(taken.join("keep")).unwrap(),
184+
"pre-existing"
185+
);
186+
}
187+
188+
#[test]
189+
fn test_back_up_clobbered_path_errors_when_source_missing() {
190+
// A missing source fails the rename with a non-AlreadyExists error,
191+
// which surfaces (with the "Failed to move" context) rather than being
192+
// retried.
193+
let temp = tempfile::tempdir().unwrap();
194+
let missing = temp.path().join("does-not-exist");
195+
let err = back_up_clobbered_path(&missing, "20250101-000000").unwrap_err();
196+
assert!(
197+
err.to_string().contains("Failed to move"),
198+
"expected wrapped error, got: {err}"
199+
);
200+
}
201+
202+
#[test]
203+
fn test_back_up_clobbered_path_keeps_incrementing_past_many_collisions() {
204+
// There is no attempt cap: the move keeps counting up until a free
205+
// name is found, however many backups already exist.
206+
let temp = tempfile::tempdir().unwrap();
207+
let stale = temp.path().join("feature");
208+
std::fs::create_dir(&stale).unwrap();
209+
210+
// Occupy the preferred name and the first 49 -N fallbacks (suffix "S").
211+
std::fs::create_dir(temp.path().join("feature.bak.S")).unwrap();
212+
for n in 2..=50 {
213+
std::fs::create_dir(temp.path().join(format!("feature.bak.S-{n}"))).unwrap();
214+
}
215+
216+
let used = back_up_clobbered_path(&stale, "S").unwrap();
217+
218+
assert_eq!(used, temp.path().join("feature.bak.S-51"));
219+
assert!(!stale.exists(), "stale path should be moved away");
220+
}
221+
222+
#[test]
223+
fn test_back_up_clobbered_path_now_uses_timestamped_suffix() {
224+
let temp = tempfile::tempdir().unwrap();
225+
let stale = temp.path().join("feature");
226+
std::fs::create_dir(&stale).unwrap();
227+
228+
let used = back_up_clobbered_path_now(&stale).unwrap();
229+
230+
let name = used.file_name().unwrap().to_string_lossy();
231+
assert!(
232+
name.starts_with("feature.bak."),
233+
"expected timestamped backup name, got: {name}"
234+
);
235+
assert!(!stale.exists(), "stale path should be moved away");
236+
}
237+
}

src/commands/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
mod alias;
2+
pub(crate) mod backup;
23
pub(crate) mod command_approval;
34
pub(crate) mod command_executor;
45
pub(crate) mod commit;

src/commands/relocate.rs

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ use worktrunk::styling::{
3333
warning_message,
3434
};
3535

36+
use super::backup;
3637
use super::commit::{CommitGenerator, StageMode};
3738
use super::worktree::compute_worktree_path;
3839

@@ -375,39 +376,13 @@ impl<'a> RelocationExecutor<'a> {
375376
}
376377

377378
if clobber {
378-
// Backup the blocker
379-
let timestamp_secs = worktrunk::utils::epoch_now() as i64;
380-
let datetime = chrono::DateTime::from_timestamp(timestamp_secs, 0)
381-
.unwrap_or_else(chrono::Utc::now);
382-
let suffix = datetime.format("%Y%m%d-%H%M%S");
383-
let backup_path = expected_path.with_file_name(format!(
384-
"{}.bak-{suffix}",
385-
expected_path
386-
.file_name()
387-
.unwrap_or_default()
388-
.to_string_lossy()
389-
));
390-
// Fail closed if the backup destination already exists: `fs::rename`
391-
// can silently replace an existing file (and some empty
392-
// directories) on Unix, which would destroy a prior backup.
393-
if backup_path.exists() {
394-
anyhow::bail!(
395-
"Backup path already exists: {}",
396-
format_path_for_display(&backup_path)
397-
);
398-
}
379+
// Atomically move the blocker aside to a timestamped backup.
380+
// A backup name already taken is never overwritten — the move
381+
// falls back to the next free `-N` name.
399382
let src = format_path_for_display(expected_path);
383+
let backup_path = backup::back_up_clobbered_path_now(expected_path)?;
400384
let dest = format_path_for_display(&backup_path);
401-
eprintln!(
402-
"{}",
403-
progress_message(cformat!("Backing up {src} → {dest}"))
404-
);
405-
std::fs::rename(expected_path, &backup_path).with_context(|| {
406-
format!(
407-
"Failed to backup {}",
408-
format_path_for_display(expected_path)
409-
)
410-
})?;
385+
eprintln!("{}", progress_message(cformat!("Backed up {src} → {dest}")));
411386
} else {
412387
let blocked_path = format_path_for_display(expected_path);
413388
let msg = cformat!("Skipping <bold>{branch}</> (target blocked: {blocked_path})");

src/commands/step/relocate.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use worktrunk::styling::println;
1818
/// |------|---------|
1919
/// | `--dry-run` | Show what would be moved without moving |
2020
/// | `--commit` | Auto-commit dirty worktrees with LLM-generated messages before relocating |
21-
/// | `--clobber` | Move non-worktree paths out of the way (`<path>.bak-<timestamp>`) |
21+
/// | `--clobber` | Move non-worktree paths out of the way (`<path>.bak.<timestamp>`) |
2222
/// | `[branches...]` | Specific branches to relocate (default: all mismatched) |
2323
pub fn step_relocate(
2424
branches: Vec<String>,

0 commit comments

Comments
 (0)