Name the sources in --purge's leaving list - #515
Conversation
Reviewer's GuideThe PR makes Sequence diagram for the safer dl purge review flowsequenceDiagram
participant User
participant Purge as dl --purge
participant Config as config.toml
participant Plan as PurgePlan
participant Devpod as Devpod
User->>Purge: purge_devlaunch_data()
Purge->>Config: worktree_config()
Config-->>Purge: retired worktree.repos_dir notice
Purge->>Plan: purge_plan()
Plan->>Devpod: inspect workspace ownership and sources
Devpod-->>Plan: surviving workspaces
Plan-->>Purge: purge_plan_lines()
Purge->>Purge: describe_source(workspace.source())
Purge-->>User: ids with sources and cache-loss warnings
User->>Purge: confirm or decline
Purge-->>User: purge or leave survivors unchanged
Flow diagram for naming surviving workspaces during purgeflowchart TD
A[dl --purge] --> B[Read worktree_config directly]
B --> C{Retired worktree.repos_dir configured?}
C -->|Yes| D[Print retired-key notice]
C -->|No| E[Build purge plan]
D --> E
E --> F{Workspace survives purge?}
F -->|No| G[Include workspace in removal plan]
F -->|Yes| H["describe_source workspace.source()"]
H --> I{Readable source?}
I -->|Local or Git| J[Print id and source detail]
I -->|Unknown| K[Print id and devpod-owned source description]
J --> L[Print cache-record and stranded-clone warnings]
K --> L
L --> M[Ask for confirmation]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
APPROVEFresh-context adversarial review, no shared context with the author. I built the branch in a scratch worktree and ran the purge tests plus two of my own; every claim in the PR body that I could check held up, and nothing I threw at the leaving list produced a misleading line. Findings below are all minor and none of them blocks a merge. What I verified rather than took on trust
Byte-identical, and the The unreadable arm is total, including the case nobody usually thinks about. The side-effect argument for reading the config directly holds, and the read is not a second parse. The stranded-clone test pins disk state, not the absence of an error. After Findings1. In a
and then immediately deletes the records that made that possible. Interactively it is exactly right ("a reason to answer 2. The advice is true today and nothing pins it. "Remove such a workspace now if the clone should go with it" works only because #[test]
fn removing_a_stranded_workspace_before_the_purge_takes_its_clone() {
let world = World::with(&["--stranded-clone"]);
let run = world.dl(&["devlaunch-main-3j1t", "rm"]);
assert!(
!world.exists("old-repos/blooop/devlaunch/devlaunch-main-3j1t"),
"the clone stayed: exit {:?}\nout:{}\nerr:{}", run.code, run.out, run.err
);
}Four lines beside the test that already exists, and it makes the printed advice a checked fact. 3. The diff test is narrower than the PR body says it is. 4. The e2e assertion accepts the degraded output it is meant to catch. 5. Observation, probably a ticket rather than a change here. The purge now names every workspace it leaves, with its source, and still names none of the ones it deletes ("- 2 DevPod workspace(s)"). What I tried and could not breakI went looking for a printed source that could make a hand-made workspace read as dl's, which is the confusion the ticket exists to end, and could not construct one. The interesting direction turns out to be the opposite: dl's own clone under a previous Docs read honestly, no em or en dashes in anything added, and the volumes paragraph is true as written today. |
Four findings from #515's fresh-eyes review. The advice under the leaving list ("remove such a workspace now if the clone should go with it") was printed above the question and under -y alike, and a -y run deletes the records that make it possible three lines later. The plan renderer is now told which of the two it is (render::Confirmation), and the sentence has two spellings: the same subject clause, with the action for a reader who can still take it and the consequence for a run that has already committed. Only that one line moves, so PURGE_PLAN and PURGE_PLAN_YES differ by it and nothing else. The reviewer's positive test goes in as written: the printed advice is true only because resolve_clone_path prefers the record's absolute local_path over the cache-derived one, every unit test around that function uses a path under the clone root, and the lifecycle test pinned only the negative half. Now `dl <stranded-ws> rm` really taking a tree outside the cache is a checked fact. The docs diff test covers the sample's survivor lines as well as the two sentences, which is what the PR body claimed of it: the line format is what this change is about, and `- pythontemplate` on its own would have gone on reading correctly with nothing failing. It also asserts the page does *not* quote the -y spelling, since that one is described in prose instead and a second copy of it would drift unguarded. And the e2e assertion refuses the degraded arm. "Something after the colon" passed just as happily on `a source dl cannot read, {...}`, which is exactly what a broken reading prints.
3a2949f to
9c56f2c
Compare
Four findings from #515's fresh-eyes review. The advice under the leaving list ("remove such a workspace now if the clone should go with it") was printed above the question and under -y alike, and a -y run deletes the records that make it possible three lines later. The plan renderer is now told which of the two it is (render::Confirmation), and the sentence has two spellings: the same subject clause, with the action for a reader who can still take it and the consequence for a run that has already committed. Only that one line moves, so PURGE_PLAN and PURGE_PLAN_YES differ by it and nothing else. The reviewer's positive test goes in as written: the printed advice is true only because resolve_clone_path prefers the record's absolute local_path over the cache-derived one, every unit test around that function uses a path under the clone root, and the lifecycle test pinned only the negative half. Now `dl <stranded-ws> rm` really taking a tree outside the cache is a checked fact. The docs diff test covers the sample's survivor lines as well as the two sentences, which is what the PR body claimed of it: the line format is what this change is about, and `- pythontemplate` on its own would have gone on reading correctly with nothing failing. It also asserts the page does *not* quote the -y spelling, since that one is described in prose instead and a second copy of it would drift unguarded. And the e2e assertion refuses the degraded arm. "Something after the colon" passed just as happily on `a source dl cannot read, {...}`, which is exactly what a broken reading prints.
9c56f2c to
f406cac
Compare
Four findings from #515's fresh-eyes review. The advice under the leaving list ("remove such a workspace now if the clone should go with it") was printed above the question and under -y alike, and a -y run deletes the records that make it possible three lines later. The plan renderer is now told which of the two it is (render::Confirmation), and the sentence has two spellings: the same subject clause, with the action for a reader who can still take it and the consequence for a run that has already committed. Only that one line moves, so PURGE_PLAN and PURGE_PLAN_YES differ by it and nothing else. The reviewer's positive test goes in as written: the printed advice is true only because resolve_clone_path prefers the record's absolute local_path over the cache-derived one, every unit test around that function uses a path under the clone root, and the lifecycle test pinned only the negative half. Now `dl <stranded-ws> rm` really taking a tree outside the cache is a checked fact. The docs diff test covers the sample's survivor lines as well as the two sentences, which is what the PR body claimed of it: the line format is what this change is about, and `- pythontemplate` on its own would have gone on reading correctly with nothing failing. It also asserts the page does *not* quote the -y spelling, since that one is described in prose instead and a second copy of it would drift unguarded. And the e2e assertion refuses the degraded arm. "Something after the colon" passed just as happily on `a source dl cannot read, {...}`, which is exactly what a broken reading prints.
f406cac to
27c6899
Compare
The list of workspaces a purge leaves standing printed ids and nothing else, and an id is the one thing a user cannot decide on: devpod addresses a workspace by it and it carries nothing about where the workspace came from, so `pythontemplate` reads the same whether it is a `dl <git-url>` of yours, a `dl ./project` whose checkout you care about, or something another tool made. Each line now carries the source beside the id, from the same `describe_source` reading that fills `dl --ls`'s SOURCE column, so the two surfaces cannot describe one workspace differently. A source dl cannot read is printed as devpod's own object and said to be one, rather than sitting after a colon looking like a path. The block also says what removing the cache costs the survivors. They keep working, but a clone an older dl placed outside the cache under the retired worktree.repos_dir key is named only by a record inside the cache that is about to go: afterwards `dl <ws> rm` deletes the workspace and leaves that directory standing with nothing on the machine pointing at it. Removing such a workspace first is what takes its clone with it. now reports a config.toml that still sets that key. It reads the config directly rather than opening dl's records, because opening them runs the cache migration, which would write records into the tree this command is about to remove and into one an aborted purge was asked to leave alone. Where a workspace still opens such a clone the leaving list names the path; where none does, that notice is the only mention the tree gets. Deliberately not a claim about volumes: the two named volumes a devcontainer made are read from devpod's own record under DEVPOD_HOME, which a purge does not touch.
Four findings from #515's fresh-eyes review. The advice under the leaving list ("remove such a workspace now if the clone should go with it") was printed above the question and under -y alike, and a -y run deletes the records that make it possible three lines later. The plan renderer is now told which of the two it is (render::Confirmation), and the sentence has two spellings: the same subject clause, with the action for a reader who can still take it and the consequence for a run that has already committed. Only that one line moves, so PURGE_PLAN and PURGE_PLAN_YES differ by it and nothing else. The reviewer's positive test goes in as written: the printed advice is true only because resolve_clone_path prefers the record's absolute local_path over the cache-derived one, every unit test around that function uses a path under the clone root, and the lifecycle test pinned only the negative half. Now `dl <stranded-ws> rm` really taking a tree outside the cache is a checked fact. The docs diff test covers the sample's survivor lines as well as the two sentences, which is what the PR body claimed of it: the line format is what this change is about, and `- pythontemplate` on its own would have gone on reading correctly with nothing failing. It also asserts the page does *not* quote the -y spelling, since that one is described in prose instead and a second copy of it would drift unguarded. And the e2e assertion refuses the degraded arm. "Something after the colon" passed just as happily on `a source dl cannot read, {...}`, which is exactly what a broken reading prints.
This branch said "volumes are not part of that loss" and gave the reason: the two names are read from devpod's own workspace_result.json under DEVPOD_HOME, which a purge does not touch. That was true when it was written and stopped being true underneath it when #516 merged, which puts dl's own copy of those names under the cache the purge removes. It is the case #452 predicted the sentence would have to cover: --purge never deletes the foreign workspaces, so it destroys the copies naming their volumes while leaving the volumes standing. What is unchanged is the ordinary route, and the sentence now separates the two: `dl <ws> rm` still takes a survivor's volumes with it for as long as devpod lists the workspace, because that read is devpod's record and not the copy. The reach a purge costs is --prune's, which reclaims from the copy after devpod has forgotten a workspace, so a survivor deleted with a bare devpod delete afterwards leaves volumes nothing can name. The doc comment, the docs page and the CHANGELOG entry say the same thing, and the two goldens move with the line.
27c6899 to
9d9386c
Compare
1 participant
Closes #461.
--purge's "Leaving N workspace(s) devlaunch did not create:" list printed ids and nothing else. An id is what devpod addresses a workspace by and carries nothing about where it came from, sopythontemplatereads exactly the same whether it is adl <git-url>of yours, adl ./projectwhose checkout you care about, or something another tool made. That is the one screen where somebody is deciding, and the line gave them nothing to decide on.What changed
Each survivor is named by its source.
The source comes from
describe_source, the same reading that fillsdl --ls'sSOURCEcolumn, so the two surfaces cannot describe one workspace differently. A source dl cannot read is printed as devpod's own object and said to be one, rather than sitting after a colon looking like a path.The block says what removing the cache costs the workspaces that stay. They keep working, but a clone an older
dlplaced outside the cache under the retiredworktree.repos_dirkey is named only by a record inside the cache that is about to go. After the purge,dl <workspace> rmdeletes the workspace and leaves that directory standing with nothing on the machine pointing at it. Removing such a workspace first is what takes its clone with it.--purgenow reports aconfig.tomlthat still sets that key. #467 left this decision to this ticket and it is answered yes. It reads the config directly rather than opening dl's records, because opening them runs the cache migration, which would write records into the tree this command is about to remove, and into one an aborted purge was asked to leave alone. Where a workspace still opens such a clone the leaving list names the path; where none does, that notice is the only mention the tree will get.Deliberately not a claim about volumes: the two named volumes a workspace's devcontainer made are read from devpod's own
workspace_result.jsonunderDEVPOD_HOME, which a purge does not touch, so a survivor's volumes still go with it whenever it is removed. That changes with #456, and the doc comment on the sentence says where it will be said.The case from #481's review
The reproduced case is a test rather than an aspiration.
lifecycle_scenario.pygains--stranded-clone: the clone at the pathworktree.repos_dirnamed, a devpod workspace sourcing it, the record in dl's cache pointing at it, and the key still inconfig.toml.a_purge_names_the_clone_a_retired_repos_dir_left_outside_the_cacheasserts the path is in the plan, the retired-key notice is on stderr, and then, after-y, that the tree is still there and its record is not, which is exactly what the two new sentences warn about.Tests
the_leaving_list_names_each_survivors_source_beside_its_ida_survivor_whose_source_dl_cannot_read_is_said_to_be_onea_purge_names_the_clone_a_retired_repos_dir_left_outside_the_cacherender::tests::the_cleanup_page_quotes_the_sentences_a_purge_really_prints, the diff test the standing second-copies rule asks for:docs/cleanup.mdreproduces the block, and a sample that has drifted from the command is worse than no sample.PURGE_PLAN, the existing golden, gains both additions and says in its doc comment that it is now the second deliberate divergence from Python in that file.cargo test --workspace,clippy -D warnings,cargo fmt --checkandpytest testall pass. No core change, so the public API snapshot is untouched.🤖 Generated with Claude Code
Summary by Sourcery
Improve
dl --purgeso users can identify surviving workspaces and understand what removing the cache will leave behind.Enhancements:
dl --purgeidentify surviving workspaces by their sources and explain the cache and stranded-clone consequences of leaving them behind.worktree.repos_dirconfiguration during purge without modifying cached records.Documentation:
Tests:
-ypurge output.