diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a084b0b3..4ebceb86 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,32 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - environment: [py310, py311, py312, py313] + # `default` is in here as the newest interpreter, and it is the entry + # that was missing. The pinned legs stopped at py313 while `default` + # resolves the unpinned `python = ">=3.10"` -- 3.14 today -- so the + # newest Python this project supports ran only `test-e2e`, and `pixi run + # ci`, the command README's Development section tells a developer to + # run, was the one selection no CI job ran. + # + # The interpreter that matters is not hypothetical: + # `workspace_state.read_clone` exists because `Path.is_dir()` raises on + # 3.13 and earlier and returns `False` on 3.14, and `False` is the + # fail-open answer. What this leg is for is the reversion -- somebody + # writing `clone.is_dir()` back in -- on the one interpreter where that + # call fails open rather than raising. It is not what catches a boundary + # written down wrongly: those tests assert the same answer on every + # version, so they are green on both sides of it. + # + # `default` rather than a `py314` feature on purpose: it needs no new + # environment in the lockfile, and it keeps tracking whatever the newest + # solvable Python is instead of pinning a version somebody has to + # remember to bump. The cost of that choice is worth knowing: `default` + # is the only leg here whose Python is not pinned, so `pixi update` + # moves it without anyone choosing to, and the day 3.15 solves this + # matrix stops covering 3.14 rather than adding to it. `frozen: true` + # below keeps each commit reproducible against its lockfile, so this is + # a thing to notice at lockfile review, not a hole in the gate. + environment: [py310, py311, py312, py313, default] steps: - name: Checkout uses: actions/checkout@v6 @@ -66,8 +91,8 @@ jobs: # a pixi dependency, already in the lockfile setup-pixi restores. # # Outside the matrix above, because what it exercises is devpod and Docker - # rather than a Python version; four copies would buy nothing and cost four - # image pulls. + # rather than a Python version; one copy per matrix leg would buy nothing and + # cost one image pull each. # # Per-PR rather than nightly. It costs a few minutes against a matrix that # already costs some and runs beside it, so it does not lengthen the wait by @@ -103,14 +128,14 @@ jobs: # One name for a branch ruleset to require, standing for every job above it. # # A ruleset names required checks as literal strings, so requiring the jobs - # individually means `ci (py310)`, `ci (py311)`, `ci (py312)`, `ci (py313)` - # and `e2e` written out in a repository setting -- a list that says nothing - # about why, that nobody reviews, and that goes stale the moment a matrix leg - # or a job is added or renamed here. The stale case is the dangerous one, - # because a required check that no longer exists is not a red tick: it is a - # merge that sails through ungated. Requiring `gate` instead puts the decision - # in this file, next to the jobs it is about, where a pull request that adds a - # job also adds it to `needs` under review. + # individually means `ci (py310)`, `ci (py311)`, `ci (py312)`, `ci (py313)`, + # `ci (default)` and `e2e` written out in a repository setting -- a list that + # says nothing about why, that nobody reviews, and that goes stale the moment + # a matrix leg or a job is added or renamed here. The stale case is the + # dangerous one, because a required check that no longer exists is not a red + # tick: it is a merge that sails through ungated. Requiring `gate` instead + # puts the decision in this file, next to the jobs it is about, where a pull + # request that adds a job also adds it to `needs` under review. # # It covers this workflow and only this workflow: `needs` cannot reach across # files, so the lint workflow's `prek` job is not in here and has to be diff --git a/CHANGELOG.md b/CHANGELOG.md index 561b3076..eb5bfeaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -242,6 +242,65 @@ table and should be judged on its own merits. ### Fixed +- **`dl rm` could delete a clone that held unsaved work, and say + nothing** ([#171](https://github.com/blooop/devlaunch/issues/171)). The guard + ran `git` in the clone directory with nothing pinning it there — no + `--git-dir`, no `--work-tree`, no ceiling — so git's repository discovery + walked up the parent chain. A clone whose `.git` was unusable (half-removed by + an interrupted delete, truncated, never finished) did not make git refuse: it + made git find an **ancestor** repository and answer about that one. With + `dl`'s cache under `$XDG_CACHE_HOME` and a dotfiles repository in `$HOME`, + that ancestor is ordinary — and when it was clean and fully pushed, the guard + reported "nothing would be lost" about somebody else's repository and the + clone went, untracked scratch files and all. Only a *tidy* host could hit it: + a dirty ancestor made the guard fire for the wrong reason and hid the bug. + + Git is now asked about one directory and cannot leave it, and "could not tell" + is an answer of its own that refuses the delete exactly as "would lose" does + — previously both were `None` and `None` meant delete freely. A directory that + is *there* but is not a repository git can read is now a refusal rather than a + clean bill of health; a directory that is *not* there still holds nothing, so + clearing up after a half-finished delete needs no `--force`. `--force` still + overrides, in both cases. + + **Breaking, in `dl --ls --json`:** `unsaved` was a string or `null` and is now + an object with exactly one key — `{"nothingToLose": true}`, + `{"wouldLose": ""}` or `{"couldNotTell": ""}` — the shape `disk` + already uses. It is `null` exactly where `devlaunch` is `false`: a workspace + `dl` did not create. The break is the safe way round: a reader that tested the + old field for truthiness now sees a truthy object for every arm, so it leaves + workspaces alone rather than deleting them. + + `unsaved`, `checkedOut` and `path` are answered for every workspace `dl` owns, + not only for the ones it still has a metadata record for. They used to gate on + the record while `devlaunch` and `disk` gated on the clone directory, so a + clone under the cache whose record had gone reported `devlaunch: true` with a + measured `disk` and `unsaved: null` beside them — `null` documented as "not + `dl`'s clone", on a clone `dl` had just called its own. That is the same + sentinel this entry is about, one layer out, and the same divergence + [PR #165](https://github.com/blooop/devlaunch/pull/165) closed for `disk`. + + A clone dl cannot even look at — a parent directory it has no search + permission on — is a "could not tell" too, and `Path.is_dir()` had no way to + say so. It gave a different wrong answer on each supported Python: up to and + including 3.13 it re-raised `PermissionError`, so `dl rm` failed closed + by crashing and `dl --ls --json` became a traceback for the whole listing + because of one workspace; on 3.14 it returns `False`, which read as "not + there, so nothing to lose" — a clone that may be full of work, reported as + free to delete. The errno is now read directly: ENOENT and ENOTDIR mean there + is no clone there, and everything else means dl was not allowed to find out. + A path with a NUL byte in it — a record a hand-edited `metadata.json` can + produce — is a "could not tell" as well rather than a `ValueError` out of the + listing. + + The boundary above was executed on 3.10.20, 3.11.15, 3.12.13, 3.13.14 and + 3.14.6, and the `ci` matrix now runs every one of those minor versions; it + previously stopped at 3.13, so `pixi run ci` never ran on the newest Python + this project supports. This entry said "3.13+" for two rounds of review, and + no test would have said otherwise — what they assert is the same on every + version, so they are green either side of wherever the prose puts the line. + Somebody running it is what corrected it. + - A cold `devpod up` no longer prints a red `fatal ... Process exited with status 1` from the tools probe. The probe asks a yes/no question and reports nothing; "no" is its everyday answer on a fresh workspace, and devpod diff --git a/README.md b/README.md index 4687b7a7..e986228f 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ A streamlined CLI for [devpod](https://devpod.sh) with intuitive autocomplete an [![PyPI](https://img.shields.io/pypi/v/devlaunch)](https://pypi.org/project/devlaunch/) [![Conda](https://img.shields.io/badge/conda-v0.0.9-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) [![License](https://img.shields.io/github/license/blooop/devlaunch)](https://opensource.org/license/mit/) -[![Python](https://img.shields.io/badge/python-3.10%20%7C%203.11%20%7C%203.12%20%7C%203.13-blue)](https://www.python.org/downloads/) +[![Python](https://img.shields.io/badge/python-3.10%20%7C%203.11%20%7C%203.12%20%7C%203.13%20%7C%203.14-blue)](https://www.python.org/downloads/) [![Pixi Badge](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/prefix-dev/pixi/main/assets/badge/v0.json)](https://pixi.sh) ## Installation @@ -450,24 +450,39 @@ cleanup tool must not ignore — `unsaved`: "repo": "blooop/devlaunch", "branch": "wayfinder/devlaunch-80", "state": "Stopped", - "unsaved": "2 uncommitted change(s) (pixi.lock, notes.md) and 1 unpushed commit(s)" + "unsaved": { + "wouldLose": "2 uncommitted change(s) (pixi.lock, notes.md) and 1 unpushed commit(s)" + } } ``` -`unsaved` is a description of what deleting would destroy, or `null` when the -clone holds nothing that does not also exist on a remote — uncommitted changes -(untracked files included) and commits no remote has. The changed paths are -named, not just counted, and that matters more than it looks: a devcontainer -that runs a package install in its `postCreateCommand` can leave a tracked -lockfile modified in *every* workspace it builds — this repo's own does — and as -a bare count that is indistinguishable from an hour of unsaved work. A cleanup -tool believing the count would then never clean anything. Named, it is -judgeable. A workspace `dl` did not -create reports `devlaunch: false` with no repo, branch or `unsaved`: there is no -clone of `dl`'s to protect, and it has no business inspecting your checkout. +`unsaved` is an object with exactly one key, and the key says which of three +answers it is: -**`dl rm` refuses when the clone holds unsaved work**, so a caller -that forgets to read the field is still caught: +| `unsaved` | Meaning | +| --- | --- | +| `{"nothingToLose": true}` | Everything in the clone exists on a remote too. Deleting it costs nothing. | +| `{"wouldLose": ""}` | Uncommitted changes (untracked files included), commits no remote has, or both. | +| `{"couldNotTell": ""}` | `git` could not read the clone as a repository — a half-removed `.git`, an interrupted delete. The files are still there and nothing has established that they exist anywhere else. | + +The changed paths are named, not just counted, and that matters more than it +looks: a devcontainer that runs a package install in its `postCreateCommand` can +leave a tracked lockfile modified in *every* workspace it builds — this repo's +own does — and as a bare count that is indistinguishable from an hour of unsaved +work. A cleanup tool believing the count would then never clean anything. Named, +it is judgeable. A workspace `dl` did not create reports `devlaunch: false` and +no `unsaved` — `unsaved` is `null` exactly where `devlaunch` is `false`, and +nowhere else: there is no clone of `dl`'s to protect, and it has no business +inspecting your checkout. (`repo` and `branch` are a weaker test and not the +same set: they come from `dl`'s metadata record, and a clone `dl` owns can have +lost its record while the clone and the work in it are still on disk. That clone +is inspected and reported like any other.) + +**`dl rm` refuses to delete a clone it would lose work from — when +the recorded clone holds unsaved work, and when it cannot tell what that clone +holds**, so a caller that forgets to read the field is still caught. (Recorded, +because that is the directory the guard reads; the case with no record is +neither, and is described below.) ``` $ dl blooop/repo@feature rm @@ -475,9 +490,28 @@ error: devlaunch-repo-feature-xyz holds 1 unpushed commit(s). Push or commit it, or run: dl blooop/repo@feature rm --force ``` +``` +$ dl blooop/repo@feature rm +error: devlaunch-repo-feature-xyz: git could not read /home/…/repo/feature: + fatal: not a git repository. devlaunch will not delete a clone it cannot + check. Look at it, or run: dl blooop/repo@feature rm --force +``` + That refusal is the only judgement `dl` makes here, and it is not about finished -work — it is `dl` declining to destroy the only copy of something. Say `--force` -if you mean it. +work — it is `dl` declining to destroy the only copy of something, including +when it cannot prove there is another copy. Say `--force` if you mean it. + +The guard reads `dl`'s metadata record, so the recorded directory is the one it +asks about. (The delete does not always remove that same directory: when the +recorded path is not on disk it falls back to a derived one. That divergence is +older than this guard and is tracked as devlaunch#174.) One case is therefore +neither a refusal nor a delete: a clone under `dl`'s cache that has **no** record — a metadata write +that failed, a record pruned, a cache restored without one. The listing still +reports what that clone holds, so `unsaved` is the field to read; but `rm` +removes the devpod workspace, exits `0` without asking for `--force`, and leaves +the clone on disk, because there is no recorded directory for it to remove +either. Nothing is destroyed, and nothing then points at the clone: it is yours +to keep or to `rm -rf` by hand. [`wf`](https://github.com/blooop/wayfinder) is the caller this was built for: it names its branches after its tickets, so it knows which workspaces belong to diff --git a/devlaunch/dl.py b/devlaunch/dl.py index 5d95a51f..6e5d6dde 100644 --- a/devlaunch/dl.py +++ b/devlaunch/dl.py @@ -436,21 +436,30 @@ def update_cache_background(force: bool = False) -> None: pass -def _unsaved_work_in(workspace_id: str) -> Optional[str]: - """What deleting *workspace_id* would destroy, or None if nothing would be. - - Answers None for a workspace devlaunch has no record of, which is the honest - answer rather than a permissive one: those are workspaces opened from a path - or a URL that dl never cloned and does not manage, so it has no clone of its - own to protect and no business inspecting someone's checkout to find one. +def _unsaved_work_in(workspace_id: str) -> workspace_state.Unsaved: + """What deleting *workspace_id* would destroy, as far as dl can establish. + + Answers `NothingToLose` for a workspace devlaunch has no record of, which is + the honest answer rather than a permissive one: those are workspaces opened + from a path or a URL that dl never cloned and does not manage, so it has no + clone of its own to protect and no business inspecting someone's checkout to + find one. + + A record dl cannot *read* is a different matter and gets a different answer + (devlaunch#171). Both used to be None, and None meant delete freely -- the + same conflation the clone-level guard had, one level up: dl does not know + where the clone is, so it has not established that anything is safe, and the + delete stops until somebody says `--force`. """ try: record = _get_clone_manager().storage.get_worktree_by_workspace_id(workspace_id) except (OSError, RuntimeError) as e: logging.debug(f"could not read the workspace record for {workspace_id}: {e}") - return None + return workspace_state.CouldNotTell( + f"could not read the workspace record for {workspace_id}: {e}" + ) if record is None: - return None + return workspace_state.NothingToLose() return workspace_state.holds_unsaved_work(Path(record.local_path)) @@ -469,10 +478,22 @@ def workspaces_as_json(with_size: bool = False) -> int: - `repo` and `branch` come from the record dl wrote when it made the clone; a workspace dl did not make has neither, and says so with `devlaunch: false` rather than a guess. - - `unsaved` is the field a caller must not ignore: a description of what - deleting would destroy, or null. `dl rm` refuses on it too, so a - caller that forgets is still caught -- but a caller that reads it can - leave the workspace alone instead of arguing with a refusal. + - `unsaved` is the field a caller must not ignore: an object with exactly + one key, and the key says which of three answers it is -- + `{"nothingToLose": true}`, `{"wouldLose": ""}`, or + `{"couldNotTell": ""}`. It is null exactly where `devlaunch` is + false: there is no clone of dl's own here to inspect. `dl rm` + refuses on the last two as well, so a caller that forgets is still + caught -- but a caller that reads it can leave the workspace alone + instead of arguing with a refusal. + + It used to be a string or null, and null carried "nothing would be lost" + as well as "could not find out" -- which is how a broken clone got + deleted with work in it (devlaunch#171). The break is deliberate and it + breaks the safe way: a reader that tested the old field for truthiness + now sees a truthy object for every arm, so it leaves workspaces alone. + - `path` is the directory `unsaved` and `checkedOut` describe, and is null + on the same workspaces `unsaved` is. - `state` is devpod's, one `devpod status` per workspace, which is why this is a command someone runs rather than something on the fast path. - `disk` appears only when `--size` was asked for, and describes dl's own @@ -486,9 +507,35 @@ def workspaces_as_json(with_size: bool = False) -> int: clone_mgr = _get_clone_manager() report: List[Dict[str, Any]] = [] for ws in workspaces: - mine = is_devlaunch_clone(ws, cache_dir) + # One question asked once, and `devlaunch`, `path` and `unsaved` all read + # its answer. `_measurable_clone` returns dl's own clone directory and + # returns None exactly when the workspace is not dl's, so `mine` is + # derived from the same value the other two are rather than computed + # beside them -- which is what makes "`unsaved: null` iff `devlaunch: + # false`" hold by construction instead of by a second gate that has to + # agree with the first. It was two gates, and dropping either one of them + # left the whole suite green. + clone_path = _measurable_clone(ws, cache_dir) + mine = clone_path is not None record = clone_mgr.storage.get_worktree_by_workspace_id(ws.id) if mine else None - clone_path = Path(record.local_path) if record else None + if record is not None: + # A record moves the row onto the recorded directory -- that is the + # one `dl rm`'s guard reads, and a listing that described a + # different directory from the guard would be worse than useless to + # the caller deciding whether to call it. It never makes the row + # about a workspace dl does not own, because there is no record to + # read unless `mine` already said so. + # + # Reading the record *first* and falling back to the clone was the + # sentinel bug of devlaunch#171 surviving one layer out. A clone + # under the cache that dl has no record for -- a metadata write that + # failed, a record pruned, a cache restored without one -- reported + # `devlaunch: true` and a measured `disk`, and `unsaved: null` beside + # them. `null` is documented as "not dl's clone", which that clone is + # not: dl says it is its own in the same object. This is the + # identical divergence #165 fixed for `disk` (see the comment on + # `_measurable_clone` below); `unsaved` was left behind by it. + clone_path = Path(record.local_path) state = workspace_state.read_clone(clone_path) if clone_path else None row: Dict[str, Any] = { "id": ws.id, @@ -503,7 +550,13 @@ def workspaces_as_json(with_size: bool = False) -> int: "path": str(clone_path) if clone_path else None, "state": get_workspace_state(ws.id), "lastUsed": ws.last_used, - "unsaved": state.unsaved if state else None, + # A nested object with one key, and the key says which kind of + # answer it is -- the shape `disk` uses, for the same reason + # (devlaunch#171). `null` keeps only its other meaning, and keeps it + # exactly: `mine` above is defined as `clone_path is not None`, so + # `unsaved: null` and `devlaunch: false` are the same set by + # construction rather than by agreement. + "unsaved": workspace_state.unsaved_as_json(state.unsaved) if state else None, } if with_size: # Absent unless asked, so a reader can never mistake "nobody asked" @@ -1217,15 +1270,22 @@ def is_devlaunch_clone(workspace: Workspace, cache_dir: pathlib.Path) -> bool: def _measurable_clone(workspace: Workspace, cache_dir: pathlib.Path) -> Optional[pathlib.Path]: - """The directory `--ls --size` may walk for *workspace*, or None. + """Devlaunch's own clone directory for *workspace*, or None when it has none. + + Where the boundary "is this dl's to touch?" is kept, for every surface that + needs it. `--ls --size` and the table's `SIZE` column walk it, and a + `--ls --json` row derives three more fields from this one answer: + `devlaunch` (there is a clone of dl's), `path` (which directory) and + `unsaved` (what is in it). Asking once is what stops those from disagreeing + about the same workspace -- `disk` and then `unsaved` each used to ask a + question of their own and each came apart from its neighbours in turn + (devlaunch#165, then #171). Only devlaunch's own clones. `dl ./path` opens somebody's project directory and `dl ` opens a source devlaunch never cloned; walking either would - put an unbounded stat of a stranger's tree behind a listing command, and the - bytes would not be devlaunch's disk in any case. Measuring what devlaunch - put on disk is the scope, and this is the only place that boundary is kept: - the table and the JSON listing both ask here, so the two cannot come to - different conclusions about the same workspace. + put an unbounded stat of a stranger's tree behind a listing command, the + bytes would not be devlaunch's disk in any case, and dl has no business + reporting on the state of a checkout it did not make. Ownership is the same predicate `--purge` deletes by, not the presence of a metadata record. A record can be missing from a cache that is very much on @@ -2142,7 +2202,8 @@ def print_help(): dl up Start the workspace without attaching dl stop Stop the workspace dl rm, prune Delete the workspace. Refuses if its clone - holds uncommitted or unpushed work; add + holds uncommitted or unpushed work, or if + git cannot read the clone to find out; add --force to delete it anyway. dl code Open in VS Code dl restart Stop and start (no rebuild) @@ -2561,13 +2622,29 @@ def _run_cli(argv: Optional[List[str]] = None) -> int: # does (a ticket tool, a script, a person), and this is what keeps a # confident caller from destroying an hour of somebody's afternoon. if "--force" not in args[2:]: + # Three answers, and only one of them is permission (devlaunch#171). + # Matched arm by arm rather than tested for truth, so that an answer + # this code does not know about stops the delete instead of sliding + # through an `else`. unsaved = _unsaved_work_in(workspace_id) - if unsaved: + if isinstance(unsaved, workspace_state.WouldLose): logging.error( - f"{workspace_id} holds {unsaved}. Push or commit it, or run: " + f"{workspace_id} holds {unsaved.description}. Push or commit it, or run: " f"dl {raw_spec} rm --force" ) return 1 + if isinstance(unsaved, workspace_state.CouldNotTell): + # Refused for not knowing, and it says which. The work is still + # on disk, and nothing has established that it exists anywhere + # else -- which is the same standing as unpushed work, and gets + # the same refusal and the same way past it. + logging.error( + f"{workspace_id}: {unsaved.reason}. devlaunch will not delete a clone it " + f"cannot check. Look at it, or run: dl {raw_spec} rm --force" + ) + return 1 + if not isinstance(unsaved, workspace_state.NothingToLose): + workspace_state.unhandled_unsaved(unsaved) return workspace_delete(workspace_id) if subcommand == "up": diff --git a/devlaunch/workspace_state.py b/devlaunch/workspace_state.py index afdfc7ba..0e992dcd 100644 --- a/devlaunch/workspace_state.py +++ b/devlaunch/workspace_state.py @@ -22,40 +22,248 @@ abandoned one are indistinguishable, a branch merged upstream may still have work to do, and a repo whose flow does not delete branches gets nothing. The caller that knows the answer should say the answer. + +**Three answers, not two (devlaunch#171).** This module used to answer +``Optional[str]``: a description of what would be lost, or ``None``. ``None`` +carried two meanings — "nothing would be lost" and "I could not find out" — and +the destructive caller read both as permission. That is not a hypothetical +conflation; it shipped, and it destroyed the wrong thing: + +``_git`` ran ``git … cwd=clone`` with no ``--git-dir``, no ``--work-tree`` and +no ceiling, so git's repository **discovery walked up the parent chain**. A +clone whose ``.git`` was unusable — truncated, half-removed by an interrupted +delete, or never finished — did not make git refuse. It made git find an +*ancestor* repository and answer confidently about that one. With ``dl``'s cache +under ``$XDG_CACHE_HOME`` and a dotfiles repository in ``$HOME``, that ancestor +is common; when it was clean and fully pushed, ``holds_unsaved_work`` returned +``None`` for a clone holding untracked work and ``dl rm`` deleted it +without so much as asking for ``--force``. The failure needed a *tidy* host to +appear, because a dirty ancestor made the guard fire — for the wrong reason, +about the wrong repository — and hid it. + +Both halves are fixed here and neither is sufficient alone: + +1. Every git command names its repository explicitly (:func:`_git`), so + discovery is switched off and an unusable ``.git`` produces a refusal. +2. A refusal has somewhere to go: :data:`Unsaved` is a total sum — + :class:`NothingToLose`, :class:`WouldLose`, :class:`CouldNotTell` — and + every caller must name the arm it is handling. "Could not tell" refuses a + delete exactly as "would lose" does. """ import logging +import os +import stat import subprocess from dataclasses import dataclass from pathlib import Path -from typing import List, Optional +from typing import Any, Dict, List, NoReturn, Optional, Union logger = logging.getLogger(__name__) +@dataclass(frozen=True) +class NothingToLose: + """Everything in the clone exists somewhere else. Deleting it costs nothing. + + Carries no payload on purpose: there is nothing to say. It is a distinct + type rather than a ``None`` so that a caller cannot reach it by accident — + the whole of devlaunch#171 was a caller reaching it by accident. + """ + + +@dataclass(frozen=True) +class WouldLose: + """Deleting the clone would destroy *description*. + + A description rather than a flag, and that is load-bearing: it is printed to + a person deciding whether to force the delete, and "3 uncommitted change(s) + (pixi.lock, notes.md, …) and 2 unpushed commit(s)" is the thing that answers + them. See :func:`_name_a_few` for why the count alone is not enough. + """ + + description: str + + def __post_init__(self) -> None: + # An empty description would print as "workspace holds ." and read as a + # bug in dl rather than as a reason to stop. If there is nothing to + # name, the answer is NothingToLose, not a WouldLose with no words. + if not self.description: + raise ValueError("WouldLose needs something to say; use NothingToLose() instead") + + +@dataclass(frozen=True) +class CouldNotTell: + """git could not be asked about this clone, and *reason* is what it said. + + Not a failure to report — a report. It is the answer for a directory that is + there but is not a repository git can read, and it must stop a delete for + the same reason :class:`WouldLose` does: the files are still on disk, and + nothing has established that they exist anywhere else. + + The reason is carried rather than reconstructed at the point of printing, + because the cause is not guessable from the path: an interrupted delete, a + ``.git`` written by a container as another user, a truncated gitfile and a + directory that was never a clone all arrive here and read differently to + the person who has to decide what to do about it. It names the directory it + is about, which is the specific thing the shipped bug got wrong. + """ + + reason: str + + +# Three arms and no fourth. "The directory is not there" is not a fourth: it is +# NothingToLose, because there is no work in it to lose -- the same answer +# disk_usage gives such a directory (Measured(0)), and what lets a caller clear +# away a workspace whose clone was already removed by hand. +Unsaved = Union[NothingToLose, WouldLose, CouldNotTell] + + +def unhandled_unsaved(unsaved: NoReturn) -> NoReturn: + """Reject an unsaved arm nobody handled -- at type-check time, not at runtime. + + Exported because the arms are read outside this module (``dl.py``'s delete + guard and its JSON listing), and an ``else`` hand-rolled at a call site is + exactly how a fourth arm would be silently read as "safe to delete". + + Hand-rolled rather than :func:`typing.assert_never`, which is 3.11+ while + this package supports 3.10; a parameter typed ``NoReturn`` gets the same + treatment from the checker. The same shape as + :func:`devlaunch.disk_usage._unhandled_usage`, deliberately. + """ + raise AssertionError(f"Unhandled unsaved-work answer: {unsaved!r}") + + +def unsaved_as_json(unsaved: Unsaved) -> Dict[str, Any]: + """How an answer reads to a tool: one key, and the key says which kind it is. + + Deliberately not a nullable string. A caller that reads ``nothingToLose`` + has been told nothing would be lost; a caller that reads ``couldNotTell`` + has been told dl does not know, and cannot have got there by finding a + field absent or null. The shape :func:`devlaunch.disk_usage.usage_as_json` + already uses, for the same reason. + + This is a deliberate break in ``dl --ls --json``: the field used to be a + string or ``null``. It breaks in the safe direction — a reader that tested + the old field for truthiness now sees an object, which is truthy for every + arm, so it leaves workspaces alone rather than deleting them. ``null`` + survives one level up, in ``dl.py``, where it keeps its other meaning: + there is no clone of dl's own there to inspect. + """ + if isinstance(unsaved, NothingToLose): + return {"nothingToLose": True} + if isinstance(unsaved, WouldLose): + return {"wouldLose": unsaved.description} + if isinstance(unsaved, CouldNotTell): + return {"couldNotTell": unsaved.reason} + unhandled_unsaved(unsaved) + + @dataclass(frozen=True) class CloneState: """What one workspace clone holds, as far as git can tell. - ``unsaved`` is the load-bearing field and is deliberately a description - rather than a flag: it is printed to a person who is deciding whether to - force a delete, and "3 uncommitted change(s) and 2 unpushed commit(s)" is - the thing that answers them. ``None`` means the clone holds nothing that - does not also exist on a remote. + ``branch`` is what the clone has checked out, or ``None`` when git could not + say — an unusable ``.git``, or an unborn HEAD. + + The two fields are independent, and an earlier draft of this docstring said + otherwise ("``None`` in every case where ``unsaved`` is a + :class:`CouldNotTell`"). A clone git *can* read as a repository but whose + remote-tracking refs are broken gives ``CloneState(branch='feature', + unsaved=CouldNotTell(...))``: ``git status`` answered, so the branch is + known, and only the later ``git log … --not --remotes`` refused. That is the + shape ``test_a_readable_repo_whose_remote_refs_are_broken_is_could_not_tell`` + builds, and the behaviour is right — it was the invariant that was wrong. + + ``branch`` is reported beside the recorded branch rather than + instead of it (``dl --ls --json`` prints both), so a clone an agent moved + off its branch is visible as such. """ branch: Optional[str] - unsaved: Optional[str] + unsaved: Unsaved + + +@dataclass(frozen=True) +class GitSaid: + """git ran and exited 0. ``output`` may be empty; empty is an answer.""" + + output: str + + +@dataclass(frozen=True) +class GitRefused: + """git could not answer, and ``reason`` is what it said about that.""" + + reason: str + + +# The distinction the old `Optional[str]` could not make safely. `""` and +# `None` are both falsey, so `if status:` read a *refused* `git status` as a +# clean tree -- the same sentinel bug as the one above, one layer down, and it +# would have survived the discovery fix on its own. +GitAnswer = Union[GitSaid, GitRefused] + +def _git(repo: Path, *args: str) -> GitAnswer: + """Ask git about *repo* — and only about *repo*. -def _git(repo: Path, *args: str) -> Optional[str]: - """Run git in *repo*, returning stdout, or ``None`` if it refused. + ``--git-dir`` and ``--work-tree`` are the fix for devlaunch#171 and are not + optional decoration. Passing ``cwd=`` alone leaves git's repository + discovery switched on, and discovery walks up the parent chain: on a clone + whose ``.git`` is unusable git does not refuse, it finds an **ancestor** + repository and answers about that. Naming the git directory switches + discovery off entirely, so the only repository git can reach is this one and + an unusable ``.git`` becomes a refusal — which is what the caller needs. - A refusal is "cannot tell", never an answer: a clone that is broken, gone or - not a repository must not stop the other workspaces being reported, and must - never be reported as *safe to delete* on the strength of a failed command — - every caller here treats ``None`` as "no information", and the one place - that matters (:func:`holds_unsaved_work`) fails safe explicitly. + Verified against real git (2.55.0) rather than assumed, on each shape a + broken clone actually takes: a ``.git`` directory holding garbage, an empty + ``.git`` directory, a ``.git`` with HEAD and nothing else, a real clone with + its object store deleted, and a truncated gitfile. All five refuse here. + **Four of the five** answered about the ancestor under plain ``cwd=``: the + truncated gitfile did not, because git treats an unreadable gitfile as a + hard error (``fatal: invalid gitfile format``) rather than continuing + discovery upward, so that one shape was never part of the bug — it is listed + because it is a shape a broken clone takes, not because ``cwd=`` mishandled + it. A healthy clone and a *linked worktree* (whose ``.git`` is a gitfile, + which git follows) both still answer normally, so pinning the clone down + costs nothing. + + ``--work-tree`` earns its place separately from ``--git-dir``, and the suite + goes red without it. ``core.worktree`` in the clone's own config points the + work tree at another directory, and ``--git-dir`` alone honours it — so + ``git status --porcelain`` compares the clone's index against *that* + directory. What comes back depends on what is in it, and only one of the two + outcomes is a fail-open. Both were run against git 2.55.0 on the fixture the + tests build (a clone on a pushed branch, ``README.md`` and ``feature.txt`` + tracked, an untracked ``an-hour-of-work.md`` in the clone): + + - the other directory does **not** hold HEAD's files — an empty directory is + the easiest case — and git prints ``" D README.md\\n D feature.txt"`` at + rc 0. That is a :class:`WouldLose` naming two files that are not missing, + about a directory nobody asked about: wrong, and worth fixing, but it is a + *refusal*, so it does not destroy anything. + - the other directory **mirrors HEAD** — a second checkout of the same + commit, which is what ``core.worktree`` is normally pointed at — and git + prints **nothing at rc 0**. A :class:`NothingToLose` on a clone holding an + hour of work that exists nowhere else. That is devlaunch#171's failure + class reached by a second route, and it is the fail-*open* one. + + So the mirrored shape is the one that makes ``--work-tree`` load-bearing, + and it is the one ``TestGitIsPinnedToItsWorkTreeToo`` builds in + ``test/test_workspace_state.py``. ``core.bare = true`` is the neighbouring + shape and fails closed as well (``fatal: this operation must be run in a + work tree``, a refusal). + + ``GIT_CEILING_DIRECTORIES`` was the other candidate and is not used: it + bounds discovery instead of switching it off, so it has to be an absolute + path that matches what git resolved the clone's parent to, and when it does + not match it fails *open* — back to the ancestor, silently. ``--git-dir`` + fails closed. + + ``cwd=repo`` is kept even though it no longer selects the repository, so + that ``git status`` keeps printing paths relative to the clone root exactly + as before. Only trailing newlines are trimmed, never leading whitespace. A full ``strip()`` here was wrong in a way that took real use to notice: the first @@ -64,31 +272,91 @@ def _git(repo: Path, *args: str) -> Optional[str]: :func:`_name_a_few` then reported ``ixi.lock``. Untracked entries start ``??`` and were unharmed, which is exactly why the tests missed it. """ + root = repo.resolve() try: result = subprocess.run( - ["git", *args], cwd=repo, capture_output=True, text=True, check=False, timeout=30 + ["git", f"--git-dir={root / '.git'}", f"--work-tree={root}", *args], + cwd=repo, + capture_output=True, + text=True, + check=False, + timeout=30, ) except (OSError, subprocess.SubprocessError) as e: logger.debug(f"git {' '.join(args)} in {repo}: {e}") - return None + return GitRefused(str(e)) if result.returncode != 0: - logger.debug(f"git {' '.join(args)} in {repo}: {result.stderr.strip()}") - return None - return result.stdout.rstrip("\n") + stderr = result.stderr.strip() + logger.debug(f"git {' '.join(args)} in {repo}: {stderr}") + return GitRefused(stderr or f"git {' '.join(args)} exited {result.returncode}") + return GitSaid(result.stdout.rstrip("\n")) def read_clone(clone: Path) -> CloneState: """Report what *clone* holds. The only function here that talks to git. - A directory that is not there, or is not a repository, holds nothing: there - is no work in it to lose. That is the truth about it rather than a special - case, and it is what lets a caller clear away a workspace whose clone was - already removed by hand. + A directory that is **not there** holds nothing: there is no work in it to + lose. That is the truth about it rather than a special case, and it is what + lets a caller clear away a workspace whose clone was already removed by + hand. + + A directory that *is* there and that git cannot read as a repository is a + different answer, and used to be given the same one. It holds whatever files + are in it, and with no repository to consult nothing has established that + they exist anywhere else — so it is a :class:`CouldNotTell`, and the delete + stops. See this module's docstring for what that cost before it did. + + A directory dl cannot even *look at* is a third answer, and ``Path.is_dir()`` + has no way to give it — which is why the ``os.stat`` below is written out + rather than left as ``clone.is_dir()``. ``is_dir()`` collapses every failure + into ``False``, and it does not even do that consistently across the + versions this package supports: up to and including Python 3.13 it swallows + ENOENT, ENOTDIR, EBADF and ELOOP and *re-raises* the rest, so a clone whose + parent is mode ``000`` raised ``PermissionError`` straight out of here + (``dl rm`` failed closed by crashing; ``dl --ls --json`` became a + traceback for the whole listing because of one workspace). On 3.14 the same + call returns ``False`` instead, which read as "not there, so nothing to + lose" — a clone that may be full of work, reported as free to delete, + because dl was not allowed to look. One sentinel each way, from the same + expression. + + The boundary is 3.14, and it is written from execution rather than from the + changelog: the mode-``000`` parent was run on this repo's own environments, + **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns ``False``**. + Patch levels between those were not run, so "3.13 and earlier" is the + minor-version claim those five interpreters support, not a claim about every + release. Every one of them is now in the ``ci`` matrix, 3.14 included; it + was not. The wrong boundary stood here for two rounds of review and what + corrected it was somebody running the mode-``000`` parent rather than + reading about it: this function's tests assert the same answer on every + version, which is the point of them, so they are green on both sides of the + line wherever this paragraph puts it. + + ``os.stat`` raises for all of them and the errno says which: ENOENT and + ENOTDIR mean there is no directory there to hold anything, and everything + else means dl was stopped before it could find out, which is exactly a + :class:`CouldNotTell`. The answer is now the same on every supported Python. + + ``ValueError`` is caught alongside ``OSError`` because ``os.stat`` is total + over ``OSError`` and not over ``ValueError``: a path with a NUL byte in it + — which a hand-edited or truncated ``metadata.json`` can put in a record — + is not an errno, it is a ``ValueError`` raised before the syscall, so no + errno arm can be written that catches it. Uncaught it takes down the whole + of ``dl --ls --json`` for one bad record, which is the exact harm the stat + guard was written to stop. """ - if not clone.is_dir(): - return CloneState(branch=None, unsaved=None) - branch = _git(clone, "rev-parse", "--abbrev-ref", "HEAD") - return CloneState(branch=branch or None, unsaved=_unsaved(clone, branch)) + try: + present = stat.S_ISDIR(os.stat(clone).st_mode) + except (FileNotFoundError, NotADirectoryError): + return CloneState(branch=None, unsaved=NothingToLose()) + except (OSError, ValueError) as e: + logger.debug(f"could not look at {clone}: {e}") + return CloneState(branch=None, unsaved=CouldNotTell(f"could not look at {clone}: {e}")) + if not present: + return CloneState(branch=None, unsaved=NothingToLose()) + head = _git(clone, "rev-parse", "--abbrev-ref", "HEAD") + branch = head.output or None if isinstance(head, GitSaid) else None + return CloneState(branch=branch, unsaved=_unsaved(clone, branch)) def _name_a_few(changed: List[str], limit: int = 3) -> str: @@ -111,8 +379,8 @@ def _name_a_few(changed: List[str], limit: int = 3) -> str: return ", ".join(names) -def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: - """What deleting *clone* would destroy, in words, or ``None`` if nothing. +def _unsaved(clone: Path, branch: Optional[str]) -> Unsaved: + """What deleting *clone* would destroy, in words — or that git could not say. Two kinds of loss, reported together because someone deciding whether to force a delete wants both: @@ -124,11 +392,28 @@ def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: *any* remote ref rather than this branch's upstream, so work that was pushed under another name, or merged and fetched back, is correctly not counted as lost. + + ``git status`` is asked first and doubles as the repository probe: with + ``--git-dir`` naming the clone (see :func:`_git`) it succeeds on any + repository git can read — including one with no commits yet — and refuses + on every unusable ``.git``. A refusal here is therefore not "clean", it is + :class:`CouldNotTell`, and so is a refusal from the ``git log`` below: once + the repository has been shown readable, a command that then fails has + failed for a reason nobody here can account for, and accounting for it by + saying "nothing to lose" is the bug this module exists to not have. + + *branch* being ``None`` after a successful ``status`` means HEAD names no + commit — a clone of an empty repository, or one checked out to a ref that + does not exist yet. There is no commit to be unpushed, so there is nothing + to ask ``git log`` about. """ - losses: List[str] = [] status = _git(clone, "status", "--porcelain") - if status: - changed = status.splitlines() + if isinstance(status, GitRefused): + return CouldNotTell(f"git could not read {clone}: {status.reason}") + + losses: List[str] = [] + if status.output: + changed = status.output.splitlines() losses.append(f"{len(changed)} uncommitted change(s) ({_name_a_few(changed)})") if branch: # Argument order is load-bearing: `--not` flips the sense of every ref @@ -136,15 +421,21 @@ def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: # --remotes ` excludes the branch as well and is silently always # empty — which would report every clone as safe to delete. unpushed = _git(clone, "log", "--oneline", branch, "--not", "--remotes") - if unpushed: - losses.append(f"{len(unpushed.splitlines())} unpushed commit(s)") - return " and ".join(losses) if losses else None + if isinstance(unpushed, GitRefused): + return CouldNotTell( + f"git could not list unpushed commits on {branch} in {clone}: {unpushed.reason}" + ) + if unpushed.output: + losses.append(f"{len(unpushed.output.splitlines())} unpushed commit(s)") + return WouldLose(" and ".join(losses)) if losses else NothingToLose() -def holds_unsaved_work(clone: Path) -> Optional[str]: - """What would be lost by deleting *clone*, or ``None`` if nothing would be. +def holds_unsaved_work(clone: Path) -> Unsaved: + """What would be lost by deleting *clone*, as far as git can be made to say. The guard `dl rm` consults. Thin on purpose: the interesting behaviour - is in :func:`read_clone`, and this is the name the guard reads by. + is in :func:`read_clone`, and this is the name the guard reads by. Total — + every path returns one of the three arms, and none of them means "go ahead" + by default. """ return read_clone(clone).unsaved diff --git a/test/test_workspace_state.py b/test/test_workspace_state.py index 454c88d7..ac821afb 100644 --- a/test/test_workspace_state.py +++ b/test/test_workspace_state.py @@ -13,13 +13,15 @@ The git tests use real repositories with a local bare as their remote -- a local path is a real git remote, so push, fetch and the remote-tracking refs all behave exactly as they do over ssh, with no network. Mocking git here would only -prove this file agrees with itself, and the one bug these tests caught before -release (an argument order that reported every clone as safe to delete) was -invisible to everything except a real `git log`. +prove this file agrees with itself, and the two bugs these tests caught (an +argument order that reported every clone as safe to delete, and git's discovery +walking up into an ancestor repository) were both invisible to everything except +a real git. """ import json import os +import shutil import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -27,7 +29,15 @@ import pytest from devlaunch.dl import main -from devlaunch.workspace_state import CloneState, holds_unsaved_work, read_clone +from devlaunch.workspace_state import ( + CloneState, + CouldNotTell, + NothingToLose, + WouldLose, + holds_unsaved_work, + read_clone, + unsaved_as_json, +) def git(*args: str, cwd: Path) -> str: @@ -54,11 +64,10 @@ def remote(tmp_path: Path) -> Path: return origin -@pytest.fixture -def clone(remote: Path, tmp_path: Path) -> Path: +def _make_clone(remote: Path, work: Path) -> Path: """A workspace clone on a pushed branch, as `dl` would leave one.""" - work = tmp_path / "ws" - git("clone", str(remote), str(work), cwd=tmp_path) + work.parent.mkdir(parents=True, exist_ok=True) + git("clone", str(remote), str(work), cwd=work.parent) git("checkout", "-b", "feature", cwd=work) (work / "feature.txt").write_text("work\n") commit(work, "feature") @@ -66,33 +75,92 @@ def clone(remote: Path, tmp_path: Path) -> Path: return work +@pytest.fixture +def clone(remote: Path, tmp_path: Path) -> Path: + return _make_clone(remote, tmp_path / "ws") + + +@pytest.fixture +def clone_in_the_cache(remote: Path, tmp_path: Path) -> Path: + """The same clone, at the path `dl` actually puts one: under its own cache. + + Ownership is established from the source path devpod kept, so this is a clone + `dl` can prove is its own with or without a metadata record of it. It is the + path `TestTheJsonListing` hands devpod as the workspace source. + """ + return _make_clone( + remote, tmp_path / "cache" / "devlaunch" / "repos" / "blooop" / "r" / "r-feature-aaa" + ) + + +@pytest.fixture +def ancestor(tmp_path: Path) -> Path: + """A repository that is clean, fully pushed, and ignores `.cache/`. + + The dotfiles-in-`$HOME` case, and the reason devlaunch#171 only shows itself + on a tidy host: git's discovery walking up out of a broken clone lands here, + and a repository with nothing to report answers "nothing to report". + """ + host = tmp_path / "host" + host.mkdir() + git("init", "-q", "-b", "main", ".", cwd=host) + (host / ".gitignore").write_text(".cache/\n") + commit(host, "seed") + origin = tmp_path / "host-origin.git" + git("init", "-q", "--bare", str(origin), cwd=tmp_path) + git("remote", "add", "origin", str(origin), cwd=host) + git("push", "-q", "-u", "origin", "main", cwd=host) + # The premise, asserted rather than assumed: if this repo were dirty or had + # an unpushed commit the guard would fire for the wrong reason and the bug + # these tests are about would be invisible. + assert git("status", "--porcelain", cwd=host) == "" + assert git("log", "--oneline", "main", "--not", "--remotes", cwd=host) == "" + return host + + +@pytest.fixture +def broken_clone_under_ancestor(ancestor: Path) -> Path: + """A clone whose `.git` is unusable, holding scratch work, nested in a repo. + + What an interrupted delete, a truncated write or a half-copied cache leaves + behind. The directory is there and holds a file that exists nowhere else. + """ + clone = ancestor / ".cache" / "devlaunch" / "ws" + clone.mkdir(parents=True) + (clone / ".git").mkdir() + (clone / ".git" / "HEAD").write_text("garbage\n") + (clone / "scratch.md").write_text("half a plan\n") + return clone + + class TestWhatACloneHolds: def test_a_pushed_branch_with_a_clean_tree_holds_nothing_unsaved(self, clone): - assert read_clone(clone) == CloneState(branch="feature", unsaved=None) + assert read_clone(clone) == CloneState(branch="feature", unsaved=NothingToLose()) def test_an_unpushed_commit_is_unsaved(self, clone): (clone / "more.txt").write_text("more\n") commit(clone, "more") unsaved = holds_unsaved_work(clone) - assert unsaved and "1 unpushed commit(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "1 unpushed commit(s)" in unsaved.description def test_uncommitted_changes_are_unsaved(self, clone): (clone / "feature.txt").write_text("edited\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "1 uncommitted change(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "1 uncommitted change(s)" in unsaved.description def test_untracked_files_are_unsaved_too(self, clone): # An agent's scratch notes are not less lost for never having been added. (clone / "notes.md").write_text("half a plan\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "uncommitted" in unsaved + assert isinstance(unsaved, WouldLose) and "uncommitted" in unsaved.description def test_both_kinds_of_loss_are_reported_together(self, clone): (clone / "more.txt").write_text("more\n") commit(clone, "more") (clone / "dirty.txt").write_text("dirty\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "uncommitted" in unsaved and "unpushed" in unsaved + assert isinstance(unsaved, WouldLose) + assert "uncommitted" in unsaved.description and "unpushed" in unsaved.description def test_a_branch_whose_commits_are_on_the_remote_under_another_name_is_saved( self, clone, remote @@ -102,18 +170,286 @@ def test_a_branch_whose_commits_are_on_the_remote_under_another_name_is_saved( # upstream is what gets this right. git("push", "origin", "feature:review/feature", cwd=clone) git("branch", "-m", "feature", "renamed", cwd=clone) - assert holds_unsaved_work(clone) is None + assert holds_unsaved_work(clone) == NothingToLose() def test_a_clone_that_is_not_there_holds_nothing(self, tmp_path): # A half-finished delete, or a directory removed by hand. There is no # work in it to lose, and nothing here may crash on it. - assert read_clone(tmp_path / "absent") == CloneState(branch=None, unsaved=None) + assert read_clone(tmp_path / "absent") == CloneState(branch=None, unsaved=NothingToLose()) + + def test_a_path_with_a_file_at_it_rather_than_a_clone_also_holds_nothing(self, tmp_path): + # Neither a clone nor a directory: the same answer as nothing at all, + # which is what this gave before the errno was read directly. + (tmp_path / "ws").write_text("not a clone\n") + assert read_clone(tmp_path / "ws") == CloneState(branch=None, unsaved=NothingToLose()) + + def test_a_clone_under_something_that_is_not_a_directory_holds_nothing(self, tmp_path): + # ENOTDIR rather than ENOENT: a parent component is a file. Still no + # clone at that path, so still nothing in it to lose. + (tmp_path / "file").write_text("not a directory\n") + assert read_clone(tmp_path / "file" / "ws") == CloneState( + branch=None, unsaved=NothingToLose() + ) - def test_a_directory_that_is_not_a_repository_holds_nothing(self, tmp_path): + def test_a_clone_nested_in_a_repository_still_answers_about_itself(self, ancestor, remote): + """A healthy clone under a repository is not confused with its ancestor. + + The other half of devlaunch#171: pinning the clone down must not cost + the ordinary answer. `dl`'s cache lives under `$XDG_CACHE_HOME`, which + on a great many machines is inside a dotfiles repository. + """ + work = ancestor / ".cache" / "devlaunch" / "ws" + git("clone", "-q", str(remote), str(work), cwd=ancestor) + git("checkout", "-q", "-b", "feature", cwd=work) + (work / "mine.txt").write_text("mine\n") + state = read_clone(work) + assert state.branch == "feature" + assert isinstance(state.unsaved, WouldLose) and "mine.txt" in state.unsaved.description + + +class TestWhenGitCannotBeAsked: + """devlaunch#171: a refusal is an answer of its own, and it is not "clean". + + `_git` used to run with `cwd=` alone -- no `--git-dir`, no `--work-tree`, no + ceiling -- so git's repository discovery walked up the parent chain. A clone + whose `.git` was unusable did not make git refuse; it made git answer about + an **ancestor** repository. On a tidy ancestor that answer is "nothing to + report", `holds_unsaved_work` returned `None`, and `None` meant delete + freely. The scratch file in the clone went with it. + + Two things had to change and both are pinned here: git is now asked about + one directory and cannot leave it, and "could not tell" is an arm of its own + that refuses the delete exactly as "would lose" does. + """ + + def test_an_unusable_git_is_could_not_tell_not_nothing_to_lose( + self, broken_clone_under_ancestor + ): + unsaved = holds_unsaved_work(broken_clone_under_ancestor) + assert isinstance(unsaved, CouldNotTell), unsaved + # And it must say so of *this* directory, not of the one git wandered + # into: the reason is what a person reads before deciding to force. + assert str(broken_clone_under_ancestor) in unsaved.reason + + def test_it_does_not_borrow_the_ancestors_branch_either(self, broken_clone_under_ancestor): + # The shipped bug reported `branch='main'` -- the ancestor's checked-out + # branch -- and `dl --ls --json` printed it as this clone's `checkedOut`. + assert read_clone(broken_clone_under_ancestor).branch is None + + def test_an_unusable_git_with_no_ancestor_at_all_is_also_could_not_tell(self, tmp_path): + # The same directory with nothing above it to walk into. git refuses + # either way now, so the answer does not depend on what the machine + # happens to have in a parent directory. + clone = tmp_path / "ws" + clone.mkdir() + (clone / ".git").mkdir() + (clone / ".git" / "HEAD").write_text("garbage\n") + (clone / "scratch.md").write_text("half a plan\n") + assert isinstance(holds_unsaved_work(clone), CouldNotTell) + + def test_a_directory_that_is_not_a_repository_cannot_be_judged(self, tmp_path): + """A present directory that is not a repository is not an empty one. + + This function used to answer `None` here, documented as "a directory + that is not there, or is not a repository, holds nothing". Half of that + is true and stays true -- a directory that is *not there* holds nothing, + which is what lets a caller clear away a workspace whose clone was + removed by hand. The other half was the bug: a directory that *is* + there and is not a repository holds whatever files are in it, and git, + having no repository to read, cannot say whether they exist anywhere + else. That is a refusal, and a refusal is not permission. + """ plain = tmp_path / "plain" plain.mkdir() (plain / "file.txt").write_text("not a repo\n") - assert read_clone(plain) == CloneState(branch=None, unsaved=None) + state = read_clone(plain) + assert state.branch is None + assert isinstance(state.unsaved, CouldNotTell) + + def test_a_half_removed_clone_is_could_not_tell(self, clone): + """An interrupted delete: a real clone with its object store gone. + + Named separately from the garbage-`.git` case because it is the one the + issue describes reaching in the wild, and because it is the shape where + the *files* are still all there to lose. + """ + (clone / "scratch.md").write_text("half a plan\n") + subprocess.run(["rm", "-rf", str(clone / ".git" / "objects")], check=True) + assert isinstance(holds_unsaved_work(clone), CouldNotTell) + + def test_a_readable_repo_whose_remote_refs_are_broken_is_could_not_tell(self, clone): + """The second refusal, which the first would otherwise hide. + + `git status` succeeds -- it never looks at remote-tracking refs -- so + the repository probe passes and the clone reads as clean right up until + `git log … --not --remotes` is asked, which refuses on a ref pointing at + an object that is not there. Answering "nothing to lose" on the strength + of the half that worked is the same bug in a narrower place: the unpushed + commits were never counted. A real broken ref, not a patched subprocess, + because the point is what git does. + """ + bogus = clone / ".git" / "refs" / "remotes" / "origin" / "bogus" + bogus.parent.mkdir(parents=True, exist_ok=True) + bogus.write_text("0123456789abcdef0123456789abcdef01234567\n") + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, CouldNotTell), unsaved + assert "unpushed commits" in unsaved.reason and "feature" in unsaved.reason + + def test_git_that_cannot_be_run_at_all_is_could_not_tell(self, clone): + # No git on PATH, a fork that fails: the process-level refusal, which + # never reaches a return code to inspect. + with patch("devlaunch.workspace_state.subprocess.run", side_effect=OSError("no git")): + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, CouldNotTell) and "no git" in unsaved.reason + + +class TestGitIsPinnedToItsWorkTreeToo: + """`--git-dir` names the repository; `--work-tree` names the files. + + The flags are not one belt and one brace. `core.worktree` in a clone's own + config points the work tree at another directory, and `--git-dir` alone + honours it -- so `git status --porcelain` compares the clone's index against + *that* directory. Dropping `--work-tree` and keeping `--git-dir` left the + rest of this suite green, so this class exists to make that mutation red. + + What the mutation does depends on what the other directory holds, and the + first version of this class pinned the wrong one of the two. Run against git + 2.55.0: + + - the other directory does not hold HEAD's files (an empty directory) and + git prints `" D README.md\\n D feature.txt"` at rc 0 -- a `WouldLose` + naming two files that are not missing. Wrong, but a *refusal*: it destroys + nothing, and a test that only checks which filenames appear is red under + the mutation for a reason that is not the safety property. + - the other directory mirrors HEAD (a second checkout of the same commit, + which is what `core.worktree` is normally pointed at) and git prints + nothing at rc 0 -- `NothingToLose()` on a clone holding an hour of work + that exists nowhere else. Devlaunch#171's failure class reached by a + second route, and the fail-*open* one. + + `test_a_clone_whose_other_work_tree_mirrors_head...` builds the second shape, + so it goes red on the arm rather than on a filename. Its neighbour + `core.bare = true` is not a third case: with it, `--git-dir` alone says + `fatal: this operation must be run in a work tree` at rc 128, which is a + refusal and therefore already safe. + """ + + @staticmethod + def _pointed_elsewhere(clone: Path, elsewhere: Path, mirror_head: bool) -> None: + elsewhere.mkdir() + if mirror_head: + # A checkout of the same commit, built from what git says is tracked + # rather than from a list written here, so it mirrors HEAD by + # construction and stays mirrored if the fixture gains a file. + for tracked in git("ls-files", cwd=clone).splitlines(): + (elsewhere / tracked).write_bytes((clone / tracked).read_bytes()) + git("config", "core.worktree", str(elsewhere), cwd=clone) + + def test_a_clone_whose_other_work_tree_mirrors_head_still_reports_its_own_work( + self, clone, tmp_path + ): + """The fail-open, and the assertion that matters is the arm. + + Without `--work-tree` git looks at the mirror, finds it identical to the + index, and says nothing at rc 0: `NothingToLose()` on the clone below, + which holds a file that exists nowhere else. `isinstance` is what goes + red; the filename is checked afterwards because a `WouldLose` about the + wrong directory would be a different defect. + """ + self._pointed_elsewhere(clone, tmp_path / "elsewhere", mirror_head=True) + (clone / "an-hour-of-work.md").write_text("half a plan\n") + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, WouldLose), unsaved + assert "an-hour-of-work.md" in unsaved.description + + def test_a_clean_clone_is_not_made_dirty_by_the_other_work_trees_absences( + self, clone, tmp_path + ): + # The other outcome, pinned so it cannot be mistaken for the one above: + # an empty other work tree makes `--git-dir` alone report HEAD's files as + # deleted, so this clone -- which is clean -- would be refused for work + # it does not hold. Also red under the mutation, and for the complaint a + # user would actually file. + self._pointed_elsewhere(clone, tmp_path / "elsewhere", mirror_head=False) + assert isinstance(holds_unsaved_work(clone), NothingToLose) + + +class TestADirectoryThatCannotBeLookedAt: + """Not knowing whether the clone is there is not knowing what it holds. + + A clone whose parent directory `dl` has no search permission on. This used + to be `clone.is_dir()`, which gave two different wrong answers depending on + which Python was running it: up to and including 3.13 it re-raised + `PermissionError`, so `dl rm` failed closed by crashing and + `dl --ls --json` became a traceback for the whole listing because of one + workspace; on 3.14 the same call returns `False`, which read as "not there, + so nothing to lose" -- a clone that may be full of work, reported as free to + delete. A version-independent assertion is the point of this class. + + The boundary is 3.14 and it was executed, not read off a changelog: the + mode-`000` parent below was run on this repo's own environments and + **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns `False`**. It + was written here as "3.13+" for two rounds of review, which is a whole minor + version out. No test here would have said so: what this class asserts is the + same on every version -- that is the point of it -- so it is green whichever + side of the boundary the prose names, and what corrected it was somebody + running the mode-`000` parent on 3.14 rather than reading about it. The `ci` + matrix in `.github/workflows/ci.yml` ran py310 through py313 only and now + includes the `default` environment, which is 3.14, so "this suite runs on + both sides of the boundary" is true of GitHub rather than only of whoever + ran it locally. + """ + + def test_a_clone_behind_a_closed_door_is_could_not_tell(self, tmp_path): + if os.geteuid() == 0: + pytest.skip("root is refused by nothing, so the closed door would open") + parent = tmp_path / "locked" + clone = parent / "ws" + clone.mkdir(parents=True) + (clone / "an-hour-of-work.md").write_text("half a plan\n") + parent.chmod(0o000) + try: + state = read_clone(clone) + finally: + parent.chmod(0o700) + assert isinstance(state.unsaved, CouldNotTell), state + assert str(clone) in state.unsaved.reason + + def test_a_recorded_path_that_is_not_a_path_at_all_is_could_not_tell(self, tmp_path): + # `os.stat` is total over `OSError` and not over `ValueError`: a NUL byte + # in the path is rejected before the syscall, and a hand-edited or + # truncated `metadata.json` is how one gets into a record. Uncaught it + # takes the whole of `dl --ls --json` down for one bad row, which is the + # harm the stat guard was written to stop. + state = read_clone(Path(f"{tmp_path / 'ws'}\0truncated")) + assert isinstance(state.unsaved, CouldNotTell), state + + +class TestTheAnswersAreTotal: + """Nothing may reach a caller as an answer the caller does not name. + + Three arms, and every place that reads them says which arm it is handling. + A fourth would raise here rather than being read as permission to delete -- + which is what the `Optional[str]` this replaced did with anything it had no + case for. + """ + + def test_an_arm_nobody_handles_is_refused_rather_than_rendered(self): + with pytest.raises(AssertionError, match="Unhandled unsaved-work answer"): + unsaved_as_json("nothing to lose, honest") # type: ignore[arg-type] + + def test_would_lose_cannot_be_built_with_nothing_to_say(self): + # "workspace holds ." reads as a bug in dl rather than as a reason to + # stop, and the arm for having nothing to report already exists. + with pytest.raises(ValueError): + WouldLose("") + + def test_each_arm_renders_as_one_key_that_names_it(self): + assert unsaved_as_json(NothingToLose()) == {"nothingToLose": True} + assert unsaved_as_json(WouldLose("1 unpushed commit(s)")) == { + "wouldLose": "1 unpushed commit(s)" + } + assert unsaved_as_json(CouldNotTell("git said no")) == {"couldNotTell": "git said no"} def _entry(workspace_id: str, source: Path) -> dict: @@ -134,13 +470,31 @@ def __init__(self, owner, repo, branch, local_path): def _clone_manager(records: dict) -> MagicMock: manager = MagicMock() manager.storage.get_worktree_by_workspace_id.side_effect = records.get + + def remove_workspace_by_id(workspace_id: str) -> bool: + """What `WorkspaceCloneManager.remove_workspace_by_id` does, and only that. + + Both halves, because the interesting one is the refusal: with no record + it removes nothing and returns False (`worktree/workspace_clone.py`), so + a workspace dl has no record for keeps its clone whatever `rm` returns. + A `MagicMock` returns a truthy `Mock` there and deletes nothing, which + makes "the clone is still on disk" true of every test for the wrong + reason. + """ + record = records.get(workspace_id) + if record is None: + return False + shutil.rmtree(record.local_path, ignore_errors=True) + return True + + manager.remove_workspace_by_id.side_effect = remove_workspace_by_id return manager class TestTheJsonListing: """`dl --ls --json`: the facts a cleanup tool decides from.""" - def _run(self, tmp_path, clone, capsys): + def _run(self, tmp_path, clone, capsys, recorded=True, foreign_recorded=False): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [ @@ -148,7 +502,17 @@ def _run(self, tmp_path, clone, capsys): _entry("someone-elses", tmp_path / "projects" / "other"), ] ) - records = {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} + records = ( + {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} if recorded else {} + ) + if foreign_recorded: + # dl wrote a record and the workspace still reads as someone else's. + # `is_devlaunch_clone`'s docstring names the way in: a `config.toml` + # pointing `repos_dir` outside the cache puts dl's clones somewhere + # `--purge` does not reach, so they are foreign by the only test that + # keeps ownership and deletion answering to the same directory -- + # and the record dl wrote for them outlives the config change. + records["someone-elses"] = FakeRecord("blooop", "other", "main", str(clone)) def devpod(args, **kwargs): if args[:1] == ["list"]: @@ -172,14 +536,105 @@ def test_it_reports_the_repo_branch_and_that_nothing_is_unsaved(self, tmp_path, assert ours["devlaunch"] is True assert ours["repo"] == "blooop/r" assert ours["branch"] == "feature" - assert ours["unsaved"] is None + assert ours["unsaved"] == {"nothingToLose": True} assert ours["state"] == "Stopped" def test_unsaved_work_is_reported_so_a_caller_can_leave_it_alone(self, tmp_path, clone, capsys): (clone / "scratch.txt").write_text("half-finished\n") _code, report = self._run(tmp_path, clone, capsys) ours = next(w for w in report if w["id"] == "r-feature-aaa") - assert ours["unsaved"] and "uncommitted" in ours["unsaved"] + assert "uncommitted" in ours["unsaved"]["wouldLose"] + + def test_a_clone_git_cannot_read_is_not_reported_as_nothing_to_lose( + self, tmp_path, broken_clone_under_ancestor, capsys + ): + """The JSON surface of devlaunch#171. + + One key, and the key says which kind of answer it is -- the shape + `disk` already uses, for the same reason. A caller cannot arrive at + "nothing would be lost" by reading a field that is absent, and the old + `null` cannot be mistaken for it either, because `null` here now means + only "not `dl`'s clone to inspect". + """ + _code, report = self._run(tmp_path, broken_clone_under_ancestor, capsys) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert "nothingToLose" not in ours["unsaved"] + assert str(broken_clone_under_ancestor) in ours["unsaved"]["couldNotTell"] + # And not the ancestor's branch, reported as this clone's. + assert ours["checkedOut"] is None + + def test_a_clone_dl_owns_but_has_no_record_for_is_still_inspected( + self, tmp_path, clone_in_the_cache, capsys + ): + """`unsaved: null` must mean "not `dl`'s clone", and only that. + + A clone under the cache that `dl` has no metadata record for -- a + metadata write that failed, a record pruned, a cache restored without + one -- is still `dl`'s clone: `devlaunch: true` says so, `--size` + measures it, and `--purge` deletes it. This used to gate on the record + while its neighbours gated on the clone directory, so that row read + `devlaunch: true` with a measured `disk` and `unsaved: null` beside + them, and `null` is documented as "no clone of `dl`'s here to inspect". + That is devlaunch#171's own sentinel surviving one layer out: `null` + carrying both "not mine" and "mine, never examined". Exactly the + divergence PR #165 closed for `disk`, which `unsaved` was left behind + by. + """ + (clone_in_the_cache / "an-hour-of-work.md").write_text("half a plan\n") + _code, report = self._run(tmp_path, clone_in_the_cache, capsys, recorded=False) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True + # No record, so nothing to report for the two fields that come from one. + assert ours["repo"] is None and ours["branch"] is None + # But the clone is there, and so is the work in it. + assert "an-hour-of-work.md" in ours["unsaved"]["wouldLose"] + assert ours["path"] == str(clone_in_the_cache) + assert ours["checkedOut"] == "feature" + # And on the same listing, `null` still means the one thing it means. + foreign = next(w for w in report if w["id"] == "someone-elses") + assert foreign["devlaunch"] is False and foreign["unsaved"] is None + + def test_a_clone_that_is_simply_gone_is_still_read_rather_than_assumed(self, tmp_path, capsys): + """An absent clone is an answer `read_clone` gives, not one a caller may skip. + + `dl rm` clears away a workspace whose clone was removed by hand + precisely because an absent directory is `NothingToLose()` rather than a + refusal, and this row has to say the same thing about it. Not calling + `read_clone` for a directory that is not on disk -- a plausible "don't + shell out to git for something that isn't there" edit, and the one + mutation of `dl.py`'s single-question restructure that the rest of this + class leaves green -- writes `unsaved: null` onto a row that says + `devlaunch: true` and names a `path` on the lines above it. That is + devlaunch#171's sentinel again: `null` meaning "mine, not examined" + rather than "not mine". + """ + gone = tmp_path / "cache" / "devlaunch" / "repos" / "blooop" / "r" / "r-feature-aaa" + assert not gone.exists() + _code, report = self._run(tmp_path, gone, capsys) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True + assert ours["path"] == str(gone) + assert ours["unsaved"] == {"nothingToLose": True} + # Nothing there to have a branch checked out, and no guess about one. + assert ours["checkedOut"] is None + + def test_one_unreachable_clone_does_not_take_the_whole_listing_down( + self, tmp_path, clone_in_the_cache, capsys + ): + # The harm `read_clone`'s stat guard prevents, at the surface it was + # visible from: one workspace behind a closed door used to raise out of + # the loop and the caller got a traceback instead of a listing. + if os.geteuid() == 0: + pytest.skip("root is refused by nothing, so the closed door would open") + clone_in_the_cache.parent.chmod(0o000) + try: + _code, report = self._run(tmp_path, clone_in_the_cache, capsys) + finally: + clone_in_the_cache.parent.chmod(0o700) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert "couldNotTell" in ours["unsaved"], ours["unsaved"] + # And the rest of the listing is still there. + assert next(w for w in report if w["id"] == "someone-elses")["devlaunch"] is False def test_a_workspace_devlaunch_did_not_make_says_so_instead_of_guessing( self, tmp_path, clone, capsys @@ -191,6 +646,38 @@ def test_a_workspace_devlaunch_did_not_make_says_so_instead_of_guessing( # And nothing inspected it: dl has no clone of its own to protect there. assert foreign["unsaved"] is None + def test_a_leftover_record_cannot_make_a_foreign_workspace_dls_own( + self, tmp_path, clone, capsys + ): + """`unsaved: null` iff `devlaunch: false`, on the input that can break it. + + The two fields used to come from two questions -- `is_devlaunch_clone` + for `devlaunch`, the metadata record for the directory `unsaved` + describes -- and the claim that they name the same set was a claim that + the two agree. They do not have to: a record can outlive the reason its + workspace counts as dl's, so a `devlaunch: false` row with a record + behind it reported a `wouldLose` about a directory the same row called + `path: null`, on a checkout dl has no business inspecting. Nothing here + caught that -- deleting the ownership gate on the record lookup left the + suite green. + + `mine` is now defined as "there is a clone dl owns here", and the record + can only move the row from one directory of dl's to another, so this row + cannot be reached by a record at all. + """ + _code, report = self._run(tmp_path, clone, capsys, foreign_recorded=True) + foreign = next(w for w in report if w["id"] == "someone-elses") + assert foreign["devlaunch"] is False + assert foreign["unsaved"] is None + # And none of the fields a record would have filled in, either -- above + # all `path`, which is what `unsaved` would have been about. + assert foreign["path"] is None + assert foreign["repo"] is None and foreign["branch"] is None + assert foreign["checkedOut"] is None + # The row that is dl's is unaffected: this is not "inspect nothing". + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True and ours["unsaved"] == {"nothingToLose": True} + def test_a_checked_out_branch_that_differs_from_the_record_is_reported_too( self, tmp_path, clone, capsys ): @@ -321,6 +808,11 @@ def test_the_two_renderings_measure_the_same_workspaces(self, tmp_path, capsys): ours = next(row for row in report if row["id"] == "r-feature-aaa") assert ours["devlaunch"] is True assert 2 * 1024 * 1024 <= ours["disk"]["exclusiveBytes"] < 3 * 1024 * 1024 + # And the field a cleanup tool must not ignore is answered on the same + # row, for the same reason: `unsaved: null` beside a measured `disk` on + # a row that says `devlaunch: true` is `null` meaning two things again. + # (This clone is a plain directory, so the answer is a refusal.) + assert "couldNotTell" in ours["unsaved"] _code, out = self._run(tmp_path, ["--ls", "--size"], capsys, recorded=False) row = next(line for line in out.splitlines() if line.startswith("r-feature-aaa")) @@ -338,12 +830,14 @@ def test_the_size_column_lines_up_with_its_heading(self, tmp_path, capsys): class TestTheDeleteGuard: """`dl rm` refuses to destroy the only copy of something.""" - def _run(self, tmp_path, clone, argv): + def _run(self, tmp_path, clone, argv, record_read_raises=None, recorded=True): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [_entry("r-feature-aaa", cache / "repos" / "blooop" / "r" / "r-feature-aaa")] ) - records = {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} + records = ( + {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} if recorded else {} + ) deleted = [] def devpod(args, **kwargs): @@ -356,10 +850,14 @@ def devpod(args, **kwargs): deleted.append(args[1]) return subprocess.CompletedProcess(args, 0, "", "") + manager = _clone_manager(records) + if record_read_raises is not None: + manager.storage.get_worktree_by_workspace_id.side_effect = record_read_raises + with ( patch("devlaunch.dl._get_cache_dir", return_value=cache), patch("devlaunch.dl.run_devpod", side_effect=devpod), - patch("devlaunch.dl._get_clone_manager", return_value=_clone_manager(records)), + patch("devlaunch.dl._get_clone_manager", return_value=manager), patch("devlaunch.dl.update_cache_background"), ): code = main(argv) @@ -391,6 +889,103 @@ def test_uncommitted_changes_stop_it_too(self, tmp_path, clone): assert code == 1 assert deleted == [] + def test_a_clone_git_cannot_read_stops_the_delete_too( + self, tmp_path, broken_clone_under_ancestor, caplog + ): + """devlaunch#171, at the surface it destroys things from. + + This is the whole ticket: on the shipped code this returned 0 and + deleted the workspace, because git had answered about the ancestor + repository and the answer was "nothing to report". "Could not tell" + must refuse exactly as "would lose" does -- and say `--force`, because + a refusal a person cannot get past is a bug of its own. + """ + code, deleted = self._run(tmp_path, broken_clone_under_ancestor, ["r-feature-aaa", "rm"]) + assert code == 1 + assert deleted == [] + assert "--force" in caplog.text + assert str(broken_clone_under_ancestor) in caplog.text + # The clone is still there, with the work still in it. + assert (broken_clone_under_ancestor / "scratch.md").exists() + + def test_force_still_gets_past_a_refusal_it_cannot_explain( + self, tmp_path, broken_clone_under_ancestor + ): + # The caller who means it is not blocked by dl declining to guess. + code, deleted = self._run( + tmp_path, broken_clone_under_ancestor, ["r-feature-aaa", "rm", "--force"] + ) + assert code == 0 + assert deleted == ["r-feature-aaa"] + + def test_a_workspace_whose_record_cannot_be_read_is_not_deleted_either( + self, tmp_path, clone, caplog + ): + """A record dl cannot read is not a record saying "nothing here". + + The same conflation one level up: `_unsaved_work_in` used to answer + `None` both for "this is not dl's workspace, it has no clone to + protect" and for "dl could not read its own metadata", and `None` meant + delete freely. + """ + code, deleted = self._run( + tmp_path, clone, ["r-feature-aaa", "rm"], record_read_raises=OSError("disk gone") + ) + assert code == 1 + assert deleted == [] + assert "--force" in caplog.text + + def test_an_answer_this_guard_does_not_name_stops_the_delete(self, tmp_path, clone): + # The exhaustiveness check, at the surface that destroys things. A + # fourth arm added later must arrive here as a crash, not as consent: + # the `if unsaved:` this replaced would have deleted on anything falsey + # and stopped on anything else, without either being a decision. + with patch("devlaunch.dl._unsaved_work_in", return_value="looks empty to me"): + with pytest.raises(AssertionError, match="Unhandled unsaved-work answer"): + self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + + def test_a_clone_dl_has_no_record_for_is_left_alone_rather_than_refused( + self, tmp_path, clone_in_the_cache, caplog + ): + """What the guard does *not* cover, pinned so README cannot overstate it. + + A clone under dl's cache with no metadata record. `dl --ls --json` + reports what it holds -- `wouldLose`, correctly -- but `rm` does not + refuse: the guard reads the record, there is none, and "dl has no record + of a clone here" is answered `NothingToLose`. So the workspace goes and + the clone stays, exit 0, no `--force` asked for. + + No work is destroyed, which is why this is not a fail-open and not fixed + here: the delete reads the same absent record and removes nothing + (`worktree/workspace_clone.py`). But it is not a refusal either, and + README said it was. What is asserted below is exactly what happens. + """ + (clone_in_the_cache / "an-hour-of-work.md").write_text("half a plan\n") + code, deleted = self._run( + tmp_path, clone_in_the_cache, ["r-feature-aaa", "rm"], recorded=False + ) + assert code == 0 + assert deleted == ["r-feature-aaa"] + assert "--force" not in caplog.text + # The work is still there, and now nothing dl records points at it. + assert (clone_in_the_cache / "an-hour-of-work.md").read_text() == "half a plan\n" + + def test_a_recorded_clone_is_removed_with_the_workspace(self, tmp_path, clone): + # The other half, so the assertion above is a fact about the record and + # not about a fake that never removes anything. + code, deleted = self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + assert code == 0 and deleted == ["r-feature-aaa"] + assert not clone.exists() + + def test_a_clone_already_removed_by_hand_is_still_deleted(self, tmp_path, clone): + # The reason the "not there" arm answers `NothingToLose` rather than + # refusing: clearing up after a half-finished delete must not need + # `--force`. + subprocess.run(["rm", "-rf", str(clone)], check=True) + code, deleted = self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + assert code == 0 + assert deleted == ["r-feature-aaa"] + class TestNamingWhatIsUnsaved: """A count cannot be judged; a name can. @@ -406,7 +1001,7 @@ class TestNamingWhatIsUnsaved: def test_the_changed_paths_are_named(self, clone): (clone / "pixi.lock").write_text("churned by the container's own build\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "pixi.lock" in unsaved + assert isinstance(unsaved, WouldLose) and "pixi.lock" in unsaved.description def test_a_modified_tracked_file_keeps_its_first_letter(self, clone): # The regression that took real use to find. `git status --porcelain` @@ -416,17 +1011,17 @@ def test_a_modified_tracked_file_keeps_its_first_letter(self, clone): # unharmed, which is why every test here passed while the feature was # printing nonsense. Asserted on the exact rendering, not on a substring. (clone / "feature.txt").write_text("edited by the container's build\n") - assert holds_unsaved_work(clone) == "1 uncommitted change(s) (feature.txt)" + assert holds_unsaved_work(clone) == WouldLose("1 uncommitted change(s) (feature.txt)") def test_a_long_list_is_cut_short_rather_than_dumped(self, clone): for i in range(6): (clone / f"file{i}.txt").write_text("x\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "6 uncommitted change(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "6 uncommitted change(s)" in unsaved.description # Three names and an ellipsis: enough to recognise, not a wall of text. - assert unsaved.count(",") == 3 and "…" in unsaved + assert unsaved.description.count(",") == 3 and "…" in unsaved.description def test_a_renamed_path_keeps_both_halves(self, clone): git("mv", "feature.txt", "renamed.txt", cwd=clone) unsaved = holds_unsaved_work(clone) - assert unsaved and "renamed.txt" in unsaved + assert isinstance(unsaved, WouldLose) and "renamed.txt" in unsaved.description