Clean up partial jj/git workspace on failed maudebox new - #1
Conversation
`jj workspace add --name N -r REV TARGET` registers the workspace and creates the target directory before resolving REV. A bad revision (e.g. the user typing `--from main` in a repo whose default branch is `master`) leaves both behind, and the next attempt with the same name then fails on "Workspace named 'N' already exists" or "Target path already exists" — a hostile second error that obscures the real one and forces the user to manually run `jj workspace forget` plus `rm -rf` to recover. Wrap the workspace/worktree creation step so a failure triggers a silent rollback: `jj workspace forget` (or `git worktree remove --force`) in the source repo, plus `rm -rf` on the target path. Print a one-line notice only when something was actually rolled back, so a pre-creation failure (e.g. stale working copy) doesn't claim cleanup it didn't do.
martint
left a comment
martint
left a comment
There was a problem hiding this comment.
The rollback goal is valid, but the cleanup must distinguish state created by this invocation from state that existed beforehand. The inline issues can unregister an existing workspace, misreport partial cleanup, or delete a directory Maudebox does not own. Please also rebase onto current main (the PR currently conflicts in src/cmd/new.rs) and add regression coverage for pre-existing workspace names, early failures, late revision failures, and partial cleanup.
|
|
||
| fn cleanup_failed_jj(source: &Path, name: &str, target: &Path) { | ||
| let forgot = Command::new("jj") | ||
| .args(["workspace", "forget", name]) |
There was a problem hiding this comment.
This forget is unconditional after every workspace add error. If target is absent but name already belongs to another workspace, the add fails with Workspace named ... already exists, and this unregisters that pre-existing workspace. I reproduced that with jj 0.44. Please only forget the name when this invocation can prove it registered that name at this target.
| .map(|s| s.success()) | ||
| .unwrap_or(false); | ||
| let removed = std::fs::remove_dir_all(target).is_ok(); | ||
| if forgot || removed { |
There was a problem hiding this comment.
jj workspace forget missing-name exits 0 on jj 0.44 with Nothing changed, so forgot is true and an early failure still prints Cleaned up. The forgot || removed condition also claims full cleanup when one half failed and retry-blocking state remains. Track actual ownership/change and report each residual cleanup failure instead.
| .status() | ||
| .map(|s| s.success()) | ||
| .unwrap_or(false); | ||
| let removed_dir = std::fs::remove_dir_all(target).is_ok(); |
There was a problem hiding this comment.
This recursive deletion (and the identical jj path) does not prove the directory was created by this invocation. Another process can create target after the initial exists() check; if the add then fails for an unrelated reason, this removes unowned contents. Only delete a path whose creation by this invocation has been established.
2 participants
Summary
jj workspace add --name N -r REV TARGETregisters the workspace and creates the target directory before resolvingREV. A bad revision (e.g.--from mainin a repo whose default branch ismaster) therefore leaves both behind, and the nextmaudebox new <same-name>attempt then trips over a hostile second error ("Workspace named 'N' already exists" / "Target path already exists") that obscures the real one and forces manual recovery withjj workspace forget+rm -rf.This change wraps the workspace/worktree creation step in
cmd::new::runso a failure triggers a silent rollback in the source repo:jj workspace forget <name>+rm -rf <target>git worktree remove --force <target>+rm -rf <target>(mostly defensive —git worktree addis more atomic)A one-line "Cleaned up partial workspace/worktree at: …" notice is printed only when at least one of the two operations actually removed state, so a pre-creation failure (e.g. stale working copy) doesn't falsely claim cleanup.
Test plan
No CI in this repo and no automated tests added for the cleanup helpers. Verified locally with my repo ~/Developer/jjuicy:
maudebox new test-late --from nonexistent ~/Developer/jjuicy true.jj workspace addregistered the workspace and created the directory, then failed on-r nonexistent. After cleanup: target directory gone,jj workspace listno longer showstest-late. Retry with a valid--fromsucceeds on the first try.cargo build --release+cargo install --path .clean.maudebox new <name> --from <valid-rev> <source> <cmd>still works on the happy path.🤖 This PR was created by Claude and approved by a human 🧍♂️