workspace delete: remove workspace directory - #9981
Conversation
50748a2 to
cb1f8f3
Compare
|
cc @joshka @steveklabnik since you also argued for that behavior. |
459a4a9 to
9cb08bd
Compare
|
To me,
I don't think this is accurate always, since there could be ignored files that the user doesn't want to lose. |
ShiroKSH
left a comment
There was a problem hiding this comment.
This deletes un-snapshotted changes from a non-current workspace.
+1 (bikeshed) The opposite of add is generally remove if looking for a good word for this and aligns well with rm meaning remove.
Deletion is fine if and only if things are snapshotted first and have astrong path to properly being handled - i.e. There's a few of things that should probably shape thinking on this: #9786 - I owe some updates As a general rule here, the test coverage should be over both non default and default workspaces and include testing the undo. |
9cb08bd to
86877b7
Compare
Thanks, I've fixed and added tests for this 👍
I'm not sure I see the need for both a
Then don't forget the workspace until you're ready? Or move the ignored files out then forget? Not sure I can really fathom an instance where you need to forget the workspace but leave all the files behind:
|
this should be allowed IMO
Something to think about here is how the various pieces here compose, like what if I want to forget a workspace that is on a network share that I don't have access to, then move that elsewhere to somewhere I do have access and readd it. The bits don't exist in a vacuum here. I think that's some of the context about why forget and removing the files was seen as separable (I could be wrong, I haven't dug deep on that one) |
ShiroKSH
left a comment
There was a problem hiding this comment.
The previous P1 is fixed: the new regression test preserves an unsnapshotted change in a non-current workspace. A separate data-loss path remains below.
86877b7 to
36584d4
Compare
ShiroKSH
left a comment
There was a problem hiding this comment.
The split into non-destructive workspace forget and guarded workspace delete addresses the preceding review. Two P1s remain in the new undo/redo and current-workspace paths.
36584d4 to
7828670
Compare
|
I've moved the undo/redo changes into its own commit since that's a bit more involved |
Add `jj workspace delete` for destructive workspace cleanup while keeping `jj workspace forget` non-destructive. The delete command snapshots target workspaces before removing them from the repo view and deleting their workspace directories. Reject destructive operation modes, unreachable workspaces, the main workspace, and current workspace deletion on Windows where removing the process's cwd is not viable. Update docs, CLI help, and workspace tests for the new command.
7828670 to
a1cfdd1
Compare
ShiroKSH
left a comment
There was a problem hiding this comment.
The explicit workspace names and Windows guard resolve the prior current-directory P1. The undo path required for a destructive workspace delete is still broken.
|
|
||
| workspace_command = command.workspace_helper_no_snapshot(ui).await?; | ||
|
|
||
| forget_workspaces(ui, &mut workspace_command, &delete_ws, "delete").await?; |
There was a problem hiding this comment.
[P1] workspace delete must not reuse the irreversible forget metadata path until undo can restore it. This helper deletes the workspace-store entry before the operation, but the unchanged jj undo only restores the repo view. Repro on this head: create secondary, run jj workspace delete secondary, then jj undo. jj workspace list shows secondary, but its directory is still missing, jj workspace root --name secondary reports no recorded path, and jj workspace add --name secondary ../secondary fails because the name is already tracked. That leaves the user unable to recover the deleted workspace through the promised operation history. Retain the path metadata and restore the workspace on undo (with a safe redo), or defer this command until that behavior ships; add the regression test.
There was a problem hiding this comment.
@ShiroKSH, did you see the second commit in the stack? I was trying to keep these separate but I suppose I can just combine the PRs if you want
Add
jj workspace deletefor destructive workspace cleanup.jj workspace forgetremains non-destructive and only stops tracking the workspace in the repo.jj workspace deletesnapshots each target workspace before removal, forgets it from the repo view, and removes its directory from disk. It requires explicit workspace names and refuses--ignore-working-copy,--at-operation,--no-integrate-operation, unreachable workspaces, and the main workspace. Deleting the current workspace is also rejected on Windows, where removing the process's cwd is not viable; Unix continues to allow deleting the current non-main workspace.Ignored files are still deleted as part of directory removal, matching the explicit destructive semantics of
workspace delete.Relates to #9941 (comment)
CC: @yuja
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)