From 5dda273f910c2d4976dfb12bd451243b3a03013b Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 19:10:31 +0100 Subject: [PATCH 1/2] fix: a commit only an unpushed local tag reaches was read as nothing to lose The delete guard excludes `refs/tags/*` from the unpushed question, because a tag the remote carries on a branch it no longer has would otherwise read as hundreds of unpushed commits on every clone of that repository forever (#485, fixed in #486). The exclusion was right about that case and silently wrong about its neighbour: tag before a rewrite, move the branch off the tag, and the commit under it exists in one place on earth while `dl rm` deletes the clone without asking and `--prune` without printing. Nothing inside a clone can tell those two apart, because no remote-tracking ref carries a tag. What can is the bare mirror the clone was made from, which fetches `+refs/tags/*:refs/tags/*` forced and pruned: a tag it holds at the same object is a tag the remote had at the last sweep, and a tag it has not got, or holds at another object, was typed here. Those come back into the query by name, so the commits only they reach are counted like any other unpushed work, and a local tag pointing at a commit the remote already has still costs nothing. The bare path is threaded through the resolver that already answers which directory a record's clone is in: `ClonePathResolver::bare_path`, answered in production by `resolve_bare_path` beside `resolve_clone_path`, and named off the record's repository rather than off the clone directory. `--prune` passes the same mirror it already computes while walking the cache. Where there is none to ask, every tag counts, which keeps the clone: `BareCache` is two arms rather than an `Option<&Path>` so every caller that cannot name one has to write the word. No fourth `Unsaved` arm: its three arms are the three documented keys of `dl --ls --json`. What changed is what counts as unpushed, not the vocabulary. Closes #487. --- CHANGELOG.md | 27 +++ docs/cleanup.md | 42 +++- rust/devlaunch-core/public-api.rest.txt | 3 + rust/devlaunch-core/src/clients/git.rs | 125 +++++++++-- rust/devlaunch-core/src/clients/git/tests.rs | 102 ++++++++- .../src/domain/workspace_state.rs | 114 +++++++++- .../src/domain/workspace_state/tests.rs | 202 ++++++++++++++++-- rust/devlaunch-core/src/flows/lifecycle.rs | 62 ++++-- rust/devlaunch-core/src/flows/listing.rs | 49 ++++- .../src/flows/workspace_clone.rs | 62 +++++- rust/dl/tests/lifecycle.rs | 25 +++ rust/dl/tests/lifecycle_scenario.py | 33 +++ 12 files changed, 763 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2a1a9a4..3f7e783c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -183,6 +183,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `git fetch` over ssh; with a connection master now open on `dl`'s own hottest path it is the common one. +- **A commit that only an unpushed local tag reaches is no longer read as nothing + to lose.** The delete guard asks what a clone holds that exists nowhere else, + and it excluded `refs/tags/*` from the question outright, because a tag your + remote carries on a branch it no longer has would otherwise read as hundreds of + unpushed commits on every clone of that repository, forever. That exclusion was + right about the case it was written for and silently wrong about its neighbour: + tag before a rewrite, move the branch off the tag, and the commit under + `backup-before-rebase` exists in exactly one place on earth while `dl rm` + deletes the clone without asking and `dl --prune` without printing. + + Nothing inside a clone can tell those two apart, because no remote-tracking ref + carries a tag: `refs/tags/` holds no mark saying which name arrived in a fetch. + What knows is the bare mirror `dl` keeps under `repos///.bare`, + which fetches tags forced and pruned and is what every workspace clone is made + from. So the guard compares the two. A tag the mirror holds at the same object + came off the remote and still costs nothing; a tag it has not got, or holds at + another object, was typed here, and the commits only that tag reaches are + counted like any other unpushed work. A local tag pointing at a commit the + remote already has is asked about and counts for nothing, which is the whole + point of asking rather than counting. + + No network, one extra local `for-each-ref` per clone, and none at all for a + repository with no tags. Where there is no mirror to ask, every tag counts, + which keeps the clone: a clone kept costs disk, and the other direction costs + the only copy of somebody's work. [docs/cleanup.md](docs/cleanup.md) has the + table. + - **Sixty-six citations that pointed at nothing now point at something, and a guard keeps it that way.** Comments across `rust/` name the test that pins the behaviour they describe, which is most of what makes them worth reading. diff --git a/docs/cleanup.md b/docs/cleanup.md index 59483f82..ebe28f98 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -630,16 +630,38 @@ the answer covers every local branch, every worktree's HEAD including detached ones, and the stash, which is one ref per clone and holds work that exists nowhere else either. -It does **not** reach local tags, and that is the one exclusion (#485). A tag your -remote carries, but which no remote *branch* reaches any more, would otherwise read -as unpushed, and a repository that tags releases on branches it then deletes has -those by the hundred. One does: 265 commits reachable only from its tags, which is -what stood between six of the eight workspaces on a host and being deleted, at 265 -to 269 unpushed commits apiece and none of it real. A guard in that state is not a clone kept for the price of some disk. It is a -guard that has to be `--force`d past to delete anything, until `--force` is what you -type without reading, over the clone that did hold an hour of work as readily as -over this one. What the exclusion gives up is a commit reachable only from a local -tag, with no branch, worktree HEAD or stash in the clone naming it too. +Tags are the one ref kind the answer has to think about, and both directions of +getting it wrong have a ticket. A tag your remote carries, but which no remote +*branch* reaches any more, must not read as unpushed: a repository that tags +releases on branches it then deletes has those by the hundred, and one does, at +265 commits reachable only from its tags. That was what stood between six of the +eight workspaces on a host and being deleted, at 265 to 269 unpushed commits +apiece and none of it real (#485). A guard in that state is not a clone kept for +the price of some disk. It is a guard that has to be `--force`d past to delete +anything, until `--force` is what you type without reading, over the clone that did +hold an hour of work as readily as over this one. But a tag you typed here and +never pushed is the opposite case, and the backup habit reaches it in two commands: +tag before a rewrite, move the branch off the tag, and that commit exists in one +place on earth (#487). + +A clone cannot tell the two apart on its own, because no remote-tracking ref +carries a tag: nothing in `refs/tags/` says which name arrived in a fetch. What +knows is the bare mirror under `repos///.bare`, which `dl` fetches +tags into and clones the workspace from. So the rule is a comparison, and it costs +no network: + +| The tag in your clone | Counted as work at risk? | +| --- | --- | +| The mirror has it, at the same object | No. It came off the remote. | +| The mirror has not got it | Yes. Nothing but this clone has ever seen it. | +| The mirror has the name, at another object | Yes. It was moved or retyped here, and what it used to reach may be nowhere else. | +| There is no mirror to ask | Yes, every tag. | + +The last row is the same principle the whole guard is built on: a check that +cannot establish safety fails towards keeping the clone. It is reached by a clone +`dl` has no record for and by a cache directory that has been deleted out from +under a workspace, and in both the answer is a clone kept, which costs disk and +nothing 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 diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 93e7324d..c6caf94a 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1826,6 +1826,7 @@ impl<'a, 'r> devlaunch_core::flows::lifecycle::CloneDirectories<'a, 'r> pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'a, 'r>::of(&'a devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'r>) -> Self pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'a, 'r>::take_notices(&self) -> alloc::vec::Vec impl devlaunch_core::flows::listing::ClonePathResolver for devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_> +pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_>::bare_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_>::clone_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option pub struct devlaunch_core::flows::lifecycle::ClonePlacement impl devlaunch_core::flows::lifecycle::ClonePlacement @@ -2185,8 +2186,10 @@ impl core::fmt::Debug for devlaunch_core::flows::listing::WorkspaceOwnership pub fn devlaunch_core::flows::listing::WorkspaceOwnership::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::flows::listing::WorkspaceOwnership pub trait devlaunch_core::flows::listing::ClonePathResolver +pub fn devlaunch_core::flows::listing::ClonePathResolver::bare_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option pub fn devlaunch_core::flows::listing::ClonePathResolver::clone_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option impl devlaunch_core::flows::listing::ClonePathResolver for devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_> +pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_>::bare_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option pub fn devlaunch_core::flows::lifecycle::CloneDirectories<'_, '_>::clone_path(&self, &devlaunch_core::domain::model::WorktreeInfo) -> core::option::Option pub fn devlaunch_core::flows::listing::describe_source(&devlaunch_core::clients::devpod::WorkspaceSource) -> devlaunch_core::flows::listing::SourceDescription pub fn devlaunch_core::flows::listing::discover_repos_from_workspaces(&devlaunch_core::clients::git::Git<'_>, &[devlaunch_core::clients::devpod::Workspace]) -> devlaunch_core::flows::listing::RepoDiscovery diff --git a/rust/devlaunch-core/src/clients/git.rs b/rust/devlaunch-core/src/clients/git.rs index 42c4b074..88437365 100644 --- a/rust/devlaunch-core/src/clients/git.rs +++ b/rust/devlaunch-core/src/clients/git.rs @@ -81,6 +81,17 @@ const READ_LOCAL_REFS: Duration = Duration::from_secs(2); /// Every git-lfs pointer file starts with this; see the git-lfs pointer spec. const LFS_POINTER_PREFIX: &[u8] = b"version https://git-lfs"; +/// The one tag query, asked of a clone and of a bare in turn. +/// +/// Written once because the answers are compared to each other: two spellings of +/// the same question would be two formats to keep in step, and a mismatch reads +/// as "every tag is local" rather than as a bug. +const TAG_REFS_QUERY: [&str; 3] = [ + "for-each-ref", + "--format=%(objectname) %(refname)", + "refs/tags/", +]; + /// What git answered, or that it did not. /// /// `Said` carries the output shaped the way the verb that asked for it needs — @@ -513,30 +524,72 @@ impl<'r> Git<'r> { /// `--exclude` binds to the `--all` that follows it and drops the tags out of /// it alone, so every other ref `--all` reaches is still asked about: local /// branches, every worktree's HEAD including detached ones, and `refs/stash`. - /// What is given up is one shape of work, and #487 is the ticket for it: a - /// commit reachable *only* from a local tag, with no branch, worktree HEAD or - /// stash in the clone naming it as well. Tag before a rewrite, move the branch - /// off it, and that clone now reads as nothing to lose. The two cases are not - /// distinguishable from inside the clone, because remote-tracking refs carry no - /// tags: there is no local mark saying which of `refs/tags/*` arrived in a - /// fetch. What does know is the bare cache this clone came from, which is a - /// path this seam is not given. + /// + /// **The tags that are local come back in by name, and #487 is why.** A + /// blanket exclusion gives up one shape of work: a commit reachable *only* + /// from a tag typed in this clone, with no branch, worktree HEAD or stash + /// naming it as well. Tag before a rewrite, move the branch off it, and that + /// clone read as nothing to lose. The two cases are not distinguishable from + /// inside the clone, because remote-tracking refs carry no tags: there is no + /// local mark saying which of `refs/tags/*` arrived in a fetch. What knows is + /// the bare cache the clone came from, so *local_tags* is the answer to that + /// comparison — see [`Git::tags_in_clone`] — and each one is named as an + /// ordinary positive ref. + /// + /// Naming them *after* `--all` and *before* `--not` is the same argument-order + /// rule as above: they are refs being asked about, so they belong on the + /// positive side. Every name is a full `refs/tags/…`, which is unambiguous + /// against a branch of the same name and cannot be read as an option. /// /// Answers on a clone with no refs at all, where there is nothing to be /// unpushed: git exits 0 with no output rather than refusing, so a clone of an /// empty repository needs no gate here and does not get one. - pub(crate) fn unpushed_commits(&self, clone: &Path) -> GitAnswer { - self.about( - clone, - &[ - "log", - "--oneline", - "--exclude=refs/tags/*", - "--all", - "--not", - "--remotes", - ], - ) + pub(crate) fn unpushed_commits( + &self, + clone: &Path, + local_tags: &[String], + ) -> GitAnswer { + let mut args: Vec<&str> = vec!["log", "--oneline", "--exclude=refs/tags/*", "--all"]; + args.extend(local_tags.iter().map(String::as_str)); + args.extend(["--not", "--remotes"]); + self.about(clone, &args) + } + + /// Every tag in *clone*, with the object each one names. + /// + /// Half of #487's comparison. The other half is [`Git::tags_in_bare`], and a + /// tag that appears in both at the same object is a tag the remote had at the + /// last sweep — [`Git::fetch_all`] copies `refs/tags/*` into the bare forced + /// and pruned, and `git clone` copies them from there into the workspace, so + /// the two agree by construction for everything that came off the remote. + /// + /// `%(objectname)` rather than a peeled commit, so an annotated tag is + /// compared as the tag object it is. A fetch copies that object whole, so the + /// two sides match; a tag *retyped* here at the same commit does not, and + /// counts as local — which is the safe direction. + /// + /// Pinned to the clone by [`Git::about`] like every other question asked of a + /// workspace, so an unusable `.git` refuses rather than answering about an + /// ancestor repository (devlaunch#171). + pub(crate) fn tags_in_clone(&self, clone: &Path) -> GitAnswer> { + self.about(clone, &TAG_REFS_QUERY).map(tag_refs_in) + } + + /// Every tag in the bare cache at *bare*, with the object each one names. + /// + /// `--git-dir` and no work tree, because a bare has none, and no cwd, because + /// the directory may not be there: a bare that is missing, half-removed or not + /// a repository is `fatal: not a git repository` — a refusal — rather than git + /// discovering some other repository from wherever dl happens to be standing. + /// The caller reads that refusal as "nothing to compare against", which counts + /// every tag in the clone as local. + pub(crate) fn tags_in_bare(&self, bare: &Path) -> GitAnswer> { + let mut argv = vec![format!("--git-dir={}", bare.display())]; + argv.extend(TAG_REFS_QUERY.iter().map(|arg| (*arg).to_owned())); + let spec = + SpawnSpec::new(Invocation::new(PROGRAM).with_args(argv)).with_timeout(ABOUT_ONE_REPO); + self.captured("for-each-ref", &spec) + .map(|stdout| tag_refs_in(stdout.trim_end_matches('\n').to_owned())) } // ------------------------------------------------------- the bare cache @@ -1282,6 +1335,38 @@ fn nul_separated(output: &str) -> Vec { .collect() } +/// One tag, as the ref it is: the full refname and the object that ref names. +/// +/// A pair rather than a name alone, because #487's question is not "does the bare +/// have a tag of this name" but "does it have *this* tag": a name the two sides +/// hold at different objects is a tag that was moved or retyped here, and what it +/// used to reach may exist nowhere else. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct TagRef { + /// The full refname, `refs/tags/v1` — never the short name, so it is + /// unambiguous as a revision argument and can never read as an option. + pub(crate) name: String, + pub(crate) object: String, +} + +/// The tags in [`TAG_REFS_QUERY`] output, one per ` ` line. +/// +/// A line in any other shape is dropped rather than guessed at, and a dropped +/// line costs nothing here: the tag it named is then absent from *both* readings +/// where the sides agree, and absent from the bare's alone where they do not, +/// which counts the clone's tag as local. Both are the safe direction. +fn tag_refs_in(output: String) -> Vec { + output + .lines() + .filter_map(|line| line.split_once(' ')) + .filter(|(object, name)| !object.is_empty() && name.starts_with("refs/tags/")) + .map(|(object, name)| TagRef { + name: name.to_owned(), + object: object.to_owned(), + }) + .collect() +} + /// The branch names in `git ls-remote --heads` output. /// /// Each line is `\trefs/heads/`. Python has two readings of this — diff --git a/rust/devlaunch-core/src/clients/git/tests.rs b/rust/devlaunch-core/src/clients/git/tests.rs index b1550861..58e08f51 100644 --- a/rust/devlaunch-core/src/clients/git/tests.rs +++ b/rust/devlaunch-core/src/clients/git/tests.rs @@ -99,7 +99,7 @@ fn every_pinned_verb_names_its_repository_twice_and_keeps_the_clone_as_cwd() { let verbs: [&dyn Fn() -> GitAnswer; 3] = [ &|| git.head_branch(dir.path()), &|| git.status_porcelain(dir.path()), - &|| git.unpushed_commits(dir.path()), + &|| git.unpushed_commits(dir.path(), &[]), ]; for verb in verbs { fake.forget_calls(); @@ -133,7 +133,7 @@ fn the_pinned_verbs_ask_exactly_what_python_asked() { assert_eq!(strs(&argv(&fake))[3..], ["status", "--porcelain"]); fake.forget_calls(); - git.unpushed_commits(dir.path()); + git.unpushed_commits(dir.path(), &[]); // Order is load-bearing twice over. `--not` flips every ref after it, so // `--all` comes first: `log --oneline --not --remotes --all` is silently // always empty, which would report every clone as safe to delete. And @@ -150,6 +150,98 @@ fn the_pinned_verbs_ask_exactly_what_python_asked() { "--remotes" ] ); + + // And a local tag goes back in by name, between the ref set it was excluded + // from and the `--not` that would flip it (#487). Full refnames, so neither + // git nor a branch of the same name can read them as anything else. + fake.forget_calls(); + git.unpushed_commits(dir.path(), &["refs/tags/backup".to_owned()]); + assert_eq!( + strs(&argv(&fake))[3..], + [ + "log", + "--oneline", + "--exclude=refs/tags/*", + "--all", + "refs/tags/backup", + "--not", + "--remotes" + ] + ); +} + +#[test] +fn the_two_tag_queries_ask_the_same_question_of_each_side() { + // The clone's tags and the bare's are compared to each other, so the two + // spellings have to be one: a format that drifted would read as "every tag is + // local", which is safe but permanently noisy (#485's shape). + let (dir, root) = a_clone(); + let fake = ScriptedRunner::new(); + let git = Git::new(&fake); + + git.tags_in_clone(dir.path()); + assert_eq!( + strs(&argv(&fake))[1..], + [ + format!("--git-dir={}", root.join(".git").display()).as_str(), + format!("--work-tree={}", root.display()).as_str(), + "for-each-ref", + "--format=%(objectname) %(refname)", + "refs/tags/", + ] + ); + + // The bare is named by `--git-dir` alone: it has no work tree, and no cwd is + // set, so a directory that is not there refuses rather than letting git + // discover whatever repository dl happens to be standing in. + fake.forget_calls(); + git.tags_in_bare(Path::new("/cache/.bare")); + assert_eq!( + strs(&argv(&fake))[1..], + [ + "--git-dir=/cache/.bare", + "for-each-ref", + "--format=%(objectname) %(refname)", + "refs/tags/", + ] + ); + assert_eq!(cwd(&fake), None); + assert_eq!(timeout(&fake), Some(Duration::from_secs(30))); +} + +#[test] +fn a_tag_listing_is_read_as_the_pairs_it_is() { + let fake = ScriptedRunner::new().with_script( + ["git"], + Response::stdout(concat!( + "aaa refs/tags/v1\n", + "bbb refs/tags/release/2\n", + "\n", + "not-a-tag-line\n", + "ccc refs/heads/main\n", + )), + ); + let git = Git::new(&fake); + + let GitAnswer::Said(tags) = git.tags_in_bare(Path::new("/cache/.bare")) else { + panic!("git answered"); + }; + + // The two tag lines, and nothing else: a blank line, a line in another shape + // and a ref that is not a tag are all dropped rather than guessed at. + assert_eq!( + tags, + [ + TagRef { + name: "refs/tags/v1".to_owned(), + object: "aaa".to_owned(), + }, + TagRef { + name: "refs/tags/release/2".to_owned(), + object: "bbb".to_owned(), + }, + ] + ); } #[test] @@ -1016,9 +1108,11 @@ fn nothing_here_spawns_more_than_once_per_verb() { git.ls_remote("url", &[]); git.head_branch(Path::new("/ws")); git.status_porcelain(Path::new("/ws")); - git.unpushed_commits(Path::new("/ws")); + git.unpushed_commits(Path::new("/ws"), &[]); + git.tags_in_clone(Path::new("/ws")); + git.tags_in_bare(Path::new("/cache/.bare")); - assert_eq!(fake.call_count(), 28, "one spawn per verb, 28 verbs"); + assert_eq!(fake.call_count(), 30, "one spawn per verb, 30 verbs"); assert!( fake.calls() .iter() diff --git a/rust/devlaunch-core/src/domain/workspace_state.rs b/rust/devlaunch-core/src/domain/workspace_state.rs index 46f77cb8..5fda5902 100644 --- a/rust/devlaunch-core/src/domain/workspace_state.rs +++ b/rust/devlaunch-core/src/domain/workspace_state.rs @@ -71,7 +71,41 @@ use std::path::{Path, PathBuf}; -use crate::clients::git::{Git, GitAnswer}; +use crate::clients::git::{Git, GitAnswer, TagRef}; + +/// The bare cache a clone was made from, when there is one to consult. +/// +/// The one thing on the host that can say which of a clone's `refs/tags/*` came +/// off the remote: dl's mirror fetches `+refs/tags/*:refs/tags/*` forced and +/// pruned, `git clone` copies those tags into the workspace, so a tag in both at +/// the same object is a tag the remote had at the last sweep. A tag the bare has +/// not got is one somebody typed here. +/// +/// Two arms rather than an `Option<&Path>`, because the absent case is an +/// *answer* with a direction and not a missing argument. [`Self::Unknown`] counts +/// every tag in the clone as local, which is the fail-towards-keeping side: a +/// clone kept costs disk, and the other way costs the only copy of somebody's +/// work. Every caller that cannot name a bare has to write the word. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum BareCache<'a> { + /// dl's mirror of this clone's repository is at this path. + At(&'a Path), + /// There is no mirror to compare against — no record, an unusable one, or a + /// clone dl did not make. + Unknown, +} + +impl<'a> BareCache<'a> { + /// The one conversion from "a path, or not" — the shape every resolver + /// answers in — so the direction the absent case fails in is decided here + /// rather than at each call site. + pub(crate) fn of(bare: Option<&'a Path>) -> Self { + match bare { + Some(path) => Self::At(path), + None => Self::Unknown, + } + } +} /// What deleting a clone would destroy, as far as git can be made to say. /// @@ -348,7 +382,11 @@ pub(crate) struct CloneState { /// fewer thing to remember. Uncaught, either takes down the whole of /// `dl --ls --json` for one bad record, which is the exact harm this guard exists /// to stop. -pub(crate) fn read_clone(git: &Git<'_>, clone: &Path) -> CloneState { +/// +/// *bare* is dl's mirror of the repository this clone came from, and it is here +/// for one question only: which of the clone's tags arrived in a fetch (#487). +/// See [`BareCache`] for why not naming one is safe and what it costs. +pub(crate) fn read_clone(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> CloneState { let present = match std::fs::metadata(clone) { Ok(metadata) => metadata.is_dir(), // No directory there, so nothing in it to lose. ENOTDIR is a parent @@ -387,7 +425,7 @@ pub(crate) fn read_clone(git: &Git<'_>, clone: &Path) -> CloneState { GitAnswer::Said(head) if !head.is_empty() => Some(head), _ => None, }; - let unsaved = unsaved(git, clone); + let unsaved = unsaved(git, clone, bare); CloneState { branch, unsaved } } @@ -396,8 +434,8 @@ pub(crate) fn read_clone(git: &Git<'_>, clone: &Path) -> CloneState { /// The guard `dl rm` consults. Thin on purpose: the interesting behaviour is /// in [`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. -pub(crate) fn holds_unsaved_work(git: &Git<'_>, clone: &Path) -> Unsaved { - read_clone(git, clone).unsaved +pub(crate) fn holds_unsaved_work(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> Unsaved { + read_clone(git, clone, bare).unsaved } /// Name the first few changed paths from `git status --porcelain` lines. @@ -451,7 +489,7 @@ fn path_in(line: &str) -> Option<&str> { /// clone with no refs at all git exits 0 with no output, so the gate bought /// nothing, and on a clone whose HEAD is unborn but which carries an orphan branch /// it hid the one thing there was to find. -fn unsaved(git: &Git<'_>, clone: &Path) -> Unsaved { +fn unsaved(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> Unsaved { let status = match git.status_porcelain(clone) { GitAnswer::Said(status) => status, GitAnswer::Refused(refused) => { @@ -466,7 +504,16 @@ fn unsaved(git: &Git<'_>, clone: &Path) -> Unsaved { if let Some(changed) = NonEmpty::of(status.lines().map(str::to_owned)) { losses.push(Loss::Uncommitted(changed)); } - match git.unpushed_commits(clone) { + let local_tags = match local_tags(git, clone, bare) { + GitAnswer::Said(tags) => tags, + GitAnswer::Refused(refused) => { + return Unsaved::CouldNotTell(CouldNotTell::UnpushedNotListed { + clone: clone.to_path_buf(), + reason: refused.reason().to_owned(), + }); + } + }; + match git.unpushed_commits(clone, &local_tags) { GitAnswer::Said(unpushed) => { if let Some(commits) = NonEmpty::of(unpushed.lines().map(str::to_owned)) { losses.push(Loss::Unpushed(commits)); @@ -485,5 +532,58 @@ fn unsaved(git: &Git<'_>, clone: &Path) -> Unsaved { } } +/// The clone's tags that the bare cache does not vouch for, as full refnames. +/// +/// #487's whole answer. The unpushed question excludes `refs/tags/*` wholesale +/// because a tag the remote carries is not work in danger (#485/#486); these are +/// the tags it should not have excluded, and they go back into the question by +/// name. +/// +/// Three things decide a tag is local, and all three fail towards keeping: +/// +/// - the bare has not got a tag by that name; +/// - it has one by that name pointing at a different object, which means this one +/// was moved or retyped here and what it used to reach may be nowhere else; +/// - there is no bare to ask — [`BareCache::Unknown`], or a bare that refused — +/// in which case every tag in the clone is local as far as anything here has +/// established. +/// +/// The bare is asked only when the clone has a tag to ask about, so a repository +/// with no tags pays one `for-each-ref` and a missing bare costs it nothing. +/// +/// A refusal from the *clone* is a refusal of the whole answer, for the reason +/// [`unsaved`] gives: the repository has already been shown readable by +/// `git status`, so a question it then refuses has failed for a reason nobody +/// here can account for, and "no local tags" would be accounting for it. +fn local_tags(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> GitAnswer> { + let here = match git.tags_in_clone(clone) { + GitAnswer::Said(tags) => tags, + GitAnswer::Refused(refused) => return GitAnswer::Refused(refused), + }; + if here.is_empty() { + return GitAnswer::Said(Vec::new()); + } + let fetched = match bare { + BareCache::At(bare) => match git.tags_in_bare(bare) { + GitAnswer::Said(tags) => tags, + // The bare is gone, half-removed, or not a repository. Nothing has + // established that any of these tags came off a remote. + GitAnswer::Refused(_) => Vec::new(), + }, + BareCache::Unknown => Vec::new(), + }; + GitAnswer::Said( + here.into_iter() + .filter(|tag| !vouched_for(tag, &fetched)) + .map(|tag| tag.name) + .collect(), + ) +} + +/// Whether the bare holds *tag* under the same name at the same object. +fn vouched_for(tag: &TagRef, fetched: &[TagRef]) -> bool { + fetched.iter().any(|cached| cached == tag) +} + #[cfg(test)] mod tests; diff --git a/rust/devlaunch-core/src/domain/workspace_state/tests.rs b/rust/devlaunch-core/src/domain/workspace_state/tests.rs index a8295152..22b4b3b8 100644 --- a/rust/devlaunch-core/src/domain/workspace_state/tests.rs +++ b/rust/devlaunch-core/src/domain/workspace_state/tests.rs @@ -184,16 +184,33 @@ impl Fixture { // ------------------------------------------------------------- the subject -/// [`read_clone`] against real git. +/// [`read_clone`] against real git, with no bare cache named. +/// +/// [`BareCache::Unknown`] is the honest default for a clone built by hand in a +/// temp directory, and it is invisible to every test with no tags in its clone — +/// which is all of them but the four #485 and #487 are about. Those name the bare +/// through [`read_against`] / [`held_against`], because *which* bare is the whole +/// question there. fn read(clone: &Path) -> CloneState { - let runner = ProcessRunner::new(); - read_clone(&Git::new(&runner), clone) + read_against(clone, BareCache::Unknown) } -/// [`holds_unsaved_work`] against real git. +/// [`holds_unsaved_work`] against real git, with no bare cache named. fn held(clone: &Path) -> Unsaved { + read(clone).unsaved +} + +/// [`read_clone`] against real git, told where dl's mirror of the remote is. +fn read_against(clone: &Path, bare: BareCache<'_>) -> CloneState { + let runner = ProcessRunner::new(); + read_clone(&Git::new(&runner), clone, bare) +} + +/// [`holds_unsaved_work`] against real git, told where dl's mirror of the remote +/// is. +fn held_against(clone: &Path, bare: &Path) -> Unsaved { let runner = ProcessRunner::new(); - holds_unsaved_work(&Git::new(&runner), clone) + holds_unsaved_work(&Git::new(&runner), clone, BareCache::At(bare)) } /// The description of a `WouldLose`, or a failure naming the arm that came back. @@ -383,21 +400,26 @@ fn a_tag_no_remote_branch_reaches_any_more_is_not_unsaved_work() { "and no branch, local or remote-tracking, is" ); - assert_eq!(held(&clone), Unsaved::NothingToLose); + assert_eq!( + held_against(&clone, &fixture.remote), + Unsaved::NothingToLose, + "the bare has this tag at this object, so it is not work in danger" + ); } #[test] -fn a_commit_only_an_unpushed_local_tag_reaches_is_given_up() { - // The case the tag exclusion answers wrongly, and #487 is the ticket for it. - // It is asserted rather than left to the doc comment because it is a cost this - // module agreed to pay: a clone can hold a commit that exists nowhere else and - // still read as nothing to lose, so `dl rm` deletes it without asking and +fn a_commit_only_an_unpushed_local_tag_reaches_is_unsaved() { + // devlaunch#487, and the reason it was a data-loss ticket rather than a + // tidiness one: the blanket `--exclude=refs/tags/*` that answered #485 also + // gave away this case, where a clone holds a commit that exists nowhere else + // and still read as nothing to lose — so `dl rm` deleted it without asking and // `--prune` without printing. // - // Reached by the ordinary backup-tag habit, which is why it is worth a ticket: - // tag before a rewrite, then move the branch out from under the tag. Nothing - // in the clone but `refs/tags/backup` reaches the commit, and no remote has - // ever seen it. When #487 closes, this test is the one that changes. + // Reached by the ordinary backup-tag habit, which is why it had to be fixed + // rather than recorded: tag before a rewrite, then move the branch out from + // under the tag. Nothing in the clone but `refs/tags/backup` reaches the + // commit, and the bare has never heard of it — which is exactly how the guard + // now tells this case from #485's. let fixture = Fixture::new(); let clone = fixture.clone(); write(&clone.join("an-hour.txt"), "an hour of work\n"); @@ -416,10 +438,138 @@ fn a_commit_only_an_unpushed_local_tag_reaches_is_given_up() { "" ); + // And the bare, which is what the answer now turns on. assert_eq!( - held(&clone), + git(&fixture.remote, &["tag", "--list"]), + "", + "the mirror has no tag at all, so this one was typed here" + ); + + assert_eq!( + would_lose(&held_against(&clone, &fixture.remote)), + "1 unpushed commit(s)" + ); +} + +#[test] +fn a_local_tag_the_bare_holds_at_another_object_is_unsaved() { + // The middle case, and the one a name-only comparison would get wrong: the + // bare has a tag by this name, so "does the mirror have `v1`" says yes — but it + // has it at the commit the remote published, and this clone moved it onto a + // commit rewritten here. What the local `v1` reaches exists nowhere else, and + // moving a tag is not a way to lose it. + let fixture = Fixture::new(); + let clone = fixture.clone(); + git(&clone, &["tag", "v1"]); + git(&clone, &["push", "-q", "origin", "v1"]); + write(&clone.join("rewritten.txt"), "an hour of work\n"); + commit(&clone, "rewritten"); + git(&clone, &["tag", "-f", "v1"]); + git(&clone, &["reset", "-q", "--hard", "origin/feature"]); + + // The premise, asserted rather than assumed: both sides have `v1`, and they + // disagree about what it names. + assert_eq!(git(&fixture.remote, &["tag", "--list"]), "v1"); + assert_ne!( + git(&clone, &["rev-parse", "refs/tags/v1"]), + git(&fixture.remote, &["rev-parse", "refs/tags/v1"]) + ); + + assert_eq!( + would_lose(&held_against(&clone, &fixture.remote)), + "1 unpushed commit(s)" + ); +} + +#[test] +fn a_local_tag_on_a_commit_the_remote_already_has_is_not_a_loss() { + // The other direction, and what keeps the fix from becoming #485 again by a + // narrower route: a tag the bare has not got is *asked about*, not counted. Its + // commits are on the remote, so there is nothing to lose, and the answer is + // the same as if the tag were not there. + let fixture = Fixture::new(); + let clone = fixture.clone(); + git(&clone, &["tag", "reviewed"]); + + assert_eq!(git(&fixture.remote, &["tag", "--list"]), ""); + assert_eq!( + held_against(&clone, &fixture.remote), + Unsaved::NothingToLose + ); +} + +#[test] +fn with_no_bare_to_compare_against_every_tag_counts() { + // Principle 1 of map #444: where a check cannot prove safety, it fails towards + // keeping. This is #485's own fixture — a released tag the remote carries, on a + // branch both sides have deleted — asked with no mirror named, and the answer + // has to be the refusal. A clone kept costs disk; the other direction costs the + // only copy of somebody's work. + let fixture = Fixture::new(); + let clone = fixture.clone(); + git(&clone, &["checkout", "-q", "-b", "release"]); + write(&clone.join("release.txt"), "shipped\n"); + commit(&clone, "release"); + git(&clone, &["push", "-q", "origin", "release"]); + git(&clone, &["tag", "v1"]); + git(&clone, &["push", "-q", "origin", "v1"]); + git(&clone, &["push", "-q", "origin", ":release"]); + git(&clone, &["checkout", "-q", "feature"]); + git(&clone, &["branch", "-qD", "release"]); + git(&clone, &["remote", "prune", "origin"]); + + assert_eq!( + held_against(&clone, &fixture.remote), Unsaved::NothingToLose, - "the state #487 exists to fix: if this now reports a loss, delete the test" + "with the mirror named it is #485's answer" + ); + assert_eq!( + would_lose(&held(&clone)), + "1 unpushed commit(s)", + "and without one, the same clone is kept" + ); +} + +#[test] +fn a_bare_that_is_not_a_repository_counts_every_tag_too() { + // The same fail-towards-keeping, reached by the shape that actually happens: + // the mirror is named but is gone, half-removed, or was never cloned. A + // refusal from it is not an empty tag list — that reading would let a deleted + // cache directory quietly authorise a delete. + let fixture = Fixture::new(); + let clone = fixture.clone(); + git(&clone, &["tag", "v1"]); + git(&clone, &["push", "-q", "origin", "v1"]); + write(&clone.join("later.txt"), "an hour of work\n"); + commit(&clone, "later"); + git(&clone, &["tag", "backup"]); + git(&clone, &["reset", "-q", "--hard", "origin/feature"]); + + assert_eq!( + would_lose(&held_against(&clone, &fixture.path("no-such-bare"))), + "1 unpushed commit(s)" + ); +} + +#[test] +fn a_clone_with_no_tags_never_asks_the_bare() { + // The bare is one more spawn per clone in `dl --ls`, and a repository with no + // tags has nothing to compare, so it is not asked. Scripted rather than + // arranged with real git, because what this asserts is the argv that was never + // built. + let dir = tempfile::tempdir().expect("a temp dir"); + let fake = ScriptedRunner::new().with_script(["git"], Response::stdout("")); + let bare = dir.path().join("mirror.git"); + + let unsaved = holds_unsaved_work(&Git::new(&fake), dir.path(), BareCache::At(&bare)); + + assert_eq!(unsaved, Unsaved::NothingToLose); + let asked = fake.argvs(); + assert!( + !asked + .iter() + .any(|argv| argv.iter().any(|arg| arg.contains("mirror.git"))), + "the mirror was asked about anyway: {asked:?}" ); } @@ -476,7 +626,11 @@ fn a_clone_that_is_not_there_is_not_asked_about_either() { let dir = tempfile::tempdir().expect("a temp dir"); let fake = ScriptedRunner::new(); - let state = read_clone(&Git::new(&fake), &dir.path().join("absent")); + let state = read_clone( + &Git::new(&fake), + &dir.path().join("absent"), + BareCache::Unknown, + ); assert_eq!(state.unsaved, Unsaved::NothingToLose); assert_eq!(fake.call_count(), 0); @@ -798,7 +952,7 @@ fn git_that_cannot_be_run_at_all_is_could_not_tell() { let clone = fixture.clone(); let fake = ScriptedRunner::new().with_script(["git"], Response::ProgramNotFound); - let state = read_clone(&Git::new(&fake), &clone); + let state = read_clone(&Git::new(&fake), &clone, BareCache::Unknown); let reason = could_not_tell(&state.unsaved); assert!( @@ -818,10 +972,14 @@ fn a_refused_status_is_never_read_as_a_clean_tree() { let dir = tempfile::tempdir().expect("a temp dir"); assert_eq!( - holds_unsaved_work(&Git::new(&clean), dir.path()), + holds_unsaved_work(&Git::new(&clean), dir.path(), BareCache::Unknown), Unsaved::NothingToLose ); - could_not_tell(&holds_unsaved_work(&Git::new(&refused), dir.path())); + could_not_tell(&holds_unsaved_work( + &Git::new(&refused), + dir.path(), + BareCache::Unknown, + )); } // ------------------------------------------- git is pinned to its work tree @@ -987,7 +1145,7 @@ fn a_would_lose_with_nothing_to_say_has_no_representation() { let nothing_changed = ScriptedRunner::new().with_script(["git"], Response::stdout("")); let dir = tempfile::tempdir().expect("a temp dir"); assert_eq!( - holds_unsaved_work(&Git::new(¬hing_changed), dir.path()), + holds_unsaved_work(&Git::new(¬hing_changed), dir.path(), BareCache::Unknown), Unsaved::NothingToLose ); } diff --git a/rust/devlaunch-core/src/flows/lifecycle.rs b/rust/devlaunch-core/src/flows/lifecycle.rs index 83187526..fb67e8b5 100644 --- a/rust/devlaunch-core/src/flows/lifecycle.rs +++ b/rust/devlaunch-core/src/flows/lifecycle.rs @@ -83,7 +83,7 @@ use crate::clients::git::Git; use crate::domain::locks::{self, LockError}; use crate::domain::metadata::{self, MetadataStorage, RecordUpdate, WorktreeFilter}; use crate::domain::model::{SweepNote, SweepTrouble, WorktreeInfo}; -use crate::domain::workspace_state::{self, CouldNotTell, Losses, NonEmpty, Unsaved}; +use crate::domain::workspace_state::{self, BareCache, CouldNotTell, Losses, NonEmpty, Unsaved}; use crate::flows::completion_cache; use crate::flows::disk_usage::{self, DiskUsage}; use crate::flows::kept_copies::{self, KeptCopies}; @@ -986,6 +986,10 @@ impl ClonePathResolver for CloneDirectories<'_, '_> { self.clones .resolve_clone_path(record, &mut *self.notices.borrow_mut()) } + + fn bare_path(&self, record: &WorktreeInfo) -> Option { + self.clones.resolve_bare_path(record) + } } /// What deleting `workspace_id` would destroy, as far as dl can establish. @@ -2031,6 +2035,23 @@ pub(crate) enum CloneStatus { }, } +/// The repository one clone directory belongs to. +/// +/// Three fields that are one fact and are always read together: the names the +/// scan reports the clone under, and the mirror beside it. Grouped because they +/// arrive together at both call sites, from the same walk over +/// `repos//`, and because passing the mirror of a *different* +/// repository would be a guard reading the wrong tags. +#[derive(Clone, Copy)] +pub(crate) struct RepoAt<'a> { + pub(crate) owner: &'a str, + pub(crate) repo: &'a str, + /// dl's mirror for this repository, whether or not it is on disk. A path that + /// is not there refuses when asked, and the guard reads that as "no mirror", + /// which counts every tag in the clone as local (#487). + pub(crate) bare: &'a Path, +} + /// Which arm `clone` is, asked in the order that fails towards keeping it. /// /// devpod's own listing is consulted first, and by containment rather than by the @@ -2046,17 +2067,23 @@ pub(crate) enum CloneStatus { /// /// The unsaved probe and the disk walk run last and only on the arm that could be /// removed. Together they are the expensive half of a scan (593 ms of git over 37 -/// clones on the reference host, plus a walk with no ceiling), and asking them -/// about a directory no answer could affect is time spent to learn nothing. +/// clones on the reference host, measured before the tag comparison #487 added, +/// plus a walk with no ceiling), and asking them about a directory no answer could +/// affect is time spent to learn nothing. +/// +/// The bare is named rather than looked up, and it is this repository's own: every +/// clone this walk reaches is a subdirectory of `repos//`, so the +/// mirror beside them is the one they were cloned from. `--prune` deletes clones +/// without being asked twice, which is exactly the surface #487 was about. pub(crate) fn clone_status( git: &Git<'_>, clone: &Path, - owner: &str, - repo: &str, + of: RepoAt<'_>, locations: &WorkspaceLocations, record_for: &HashMap, listed_at: &HashMap, ) -> CloneStatus { + let RepoAt { owner, repo, bare } = of; if let Some(workspace_id) = locations.holder(clone) { return CloneStatus::Referenced { workspace_id: workspace_id.to_owned(), @@ -2077,7 +2104,7 @@ pub(crate) fn clone_status( }; } CloneStatus::Orphaned { - unsaved: workspace_state::holds_unsaved_work(git, clone), + unsaved: workspace_state::holds_unsaved_work(git, clone, BareCache::At(bare)), usage: disk_usage::exclusive_usage(clone), } } @@ -2418,12 +2445,8 @@ pub fn prune_plan( let (Some(owner), Some(repo)) = (leaf_of(&owner_dir), leaf_of(&repo_dir)) else { continue; }; - let bare = canonical( - &clones - .repo_manager() - .bare_dir(&owner, &repo) - .to_string_lossy(), - ); + let bare_path = clones.repo_manager().bare_dir(&owner, &repo); + let bare = canonical(&bare_path.to_string_lossy()); let _lock = clones .repo_manager() .hold_repo_lock(&owner, &repo) @@ -2440,8 +2463,11 @@ pub fn prune_plan( let status = clone_status( &git, &clone, - &owner, - &repo, + RepoAt { + owner: &owner, + repo: &repo, + bare: &bare_path, + }, locations, &record_for, &listed_at, @@ -2697,12 +2723,16 @@ pub fn prune_clones( .repo_manager() .hold_repo_lock(&owner, &repo) .map_err(PruneError::Lock)?; + let bare_path = clones.repo_manager().bare_dir(&owner, &repo); for reclaimable in reclaimables { let status = clone_status( &git, &reclaimable.path, - &owner, - &repo, + RepoAt { + owner: &owner, + repo: &repo, + bare: &bare_path, + }, &locations, &record_for, &listed_at, diff --git a/rust/devlaunch-core/src/flows/listing.rs b/rust/devlaunch-core/src/flows/listing.rs index af29a8f6..271e281c 100644 --- a/rust/devlaunch-core/src/flows/listing.rs +++ b/rust/devlaunch-core/src/flows/listing.rs @@ -65,7 +65,9 @@ use crate::clients::devpod::{ use crate::clients::git::{Git, GitAnswer}; use crate::domain::metadata::MetadataStorage; use crate::domain::model::{SweepNote, WorktreeInfo}; -use crate::domain::workspace_state::{self, CloneState, CouldNotTell, NonEmpty, Unsaved}; +use crate::domain::workspace_state::{ + self, BareCache, CloneState, CouldNotTell, NonEmpty, Unsaved, +}; use crate::flows::disk_usage::{self, DiskUsage}; use crate::runner::Runner; use crate::timing; @@ -772,6 +774,24 @@ fn file_name(path: &Path) -> Option { /// empty answer. pub trait ClonePathResolver { fn clone_path(&self, record: &WorktreeInfo) -> Option; + + /// The bare cache this record's clone was made from, when dl can name one. + /// + /// Asked for one thing: which of the clone's tags came off the remote and + /// which were typed in the clone (#487). A clone's own refs cannot say — no + /// remote-tracking ref carries a tag — so the mirror dl fetched them into is + /// the only local record of it. + /// + /// Named off the record's repository rather than off the clone directory, + /// because the clone may sit at a recorded path from an older layout while + /// `.bare` has not moved, and because the repository is the thing the mirror + /// belongs to. + /// + /// A required method rather than one defaulting to `None`: `None` counts every + /// tag as unpushed, which is the safe direction but also the one #486 was + /// filed about, so a resolver that cannot name a bare should say so on + /// purpose. + fn bare_path(&self, record: &WorktreeInfo) -> Option; } /// Everything the enriched listing reads besides devpod. @@ -960,8 +980,11 @@ fn enriched_row( let path = record .and_then(|record| view.clones.clone_path(record)) .unwrap_or_else(|| measured.clone()); + // No record is no repository to name a mirror for, and the row falls back + // to the safe reading: every tag in that clone counts as local. + let bare = record.and_then(|record| view.clones.bare_path(record)); DevlaunchClone { - state: workspace_state::read_clone(git, &path), + state: workspace_state::read_clone(git, &path, BareCache::of(bare.as_deref())), path, recorded: record.map(Recorded::of), } @@ -1079,7 +1102,11 @@ pub(crate) fn unsaved_work_in(git: &Git<'_>, view: &DlView<'_>, workspace_id: &s return Unsaved::NothingToLose; }; match view.clones.clone_path(record) { - Some(clone) => workspace_state::holds_unsaved_work(git, &clone), + Some(clone) => workspace_state::holds_unsaved_work( + git, + &clone, + BareCache::of(view.clones.bare_path(record).as_deref()), + ), None => Unsaved::CouldNotTell(CouldNotTell::DirectoryUnknown { workspace_id: workspace_id.to_owned(), }), @@ -1429,6 +1456,18 @@ mod tests { .join(id.value()), ) } + + /// The mirror is named off the repository, as `resolve_bare_path` names + /// it: a sibling of every clone of that repository, wherever the clone + /// itself was recorded. + fn bare_path(&self, record: &WorktreeInfo) -> Option { + Some( + self.repos_dir + .join(&record.owner) + .join(&record.repo) + .join(".bare"), + ) + } } fn established_absent(path: &Path) -> bool { @@ -1450,6 +1489,10 @@ mod tests { fn clone_path(&self, _record: &WorktreeInfo) -> Option { None } + + fn bare_path(&self, _record: &WorktreeInfo) -> Option { + None + } } // ---------------------------------------------------------------- helpers diff --git a/rust/devlaunch-core/src/flows/workspace_clone.rs b/rust/devlaunch-core/src/flows/workspace_clone.rs index d0c287ab..0c490b87 100644 --- a/rust/devlaunch-core/src/flows/workspace_clone.rs +++ b/rust/devlaunch-core/src/flows/workspace_clone.rs @@ -44,7 +44,7 @@ use super::branch_manager::{ }; use super::repo_manager::{ CacheNotice, Cleanup, CloneError, CloneIfMissingError, FetchOutcome, NotRefreshed, - RemoveTreeError, RepoLock, RepositoryManager, TreeRemoval, WrongRepoLock, clone_dir, + RemoveTreeError, RepoLock, RepositoryManager, TreeRemoval, WrongRepoLock, bare_dir, clone_dir, remove_tree, }; use crate::clients::git::{self, Git}; @@ -479,6 +479,33 @@ impl<'r> WorkspaceCloneManager<'r> { } } + /// The bare cache a record's clone was made from, or `None` if dl cannot name + /// one. + /// + /// The delete guard's second question since #487, and the only local record of + /// which of a clone's `refs/tags/*` came off the remote: the mirror fetches + /// tags forced and pruned, and `git clone` copies them from it, so a tag in + /// both at the same object was on the remote at the last sweep. + /// + /// Named off the record's **repository**, not off the clone directory. `.bare` + /// is a sibling of every clone dl makes, so the two agree on the ordinary + /// layout; they part on a record whose `local_path` was written by an older + /// build or edited by hand, and there the repository is the right answer, + /// because that is the mirror the clone's `origin` still points at + /// (`flows::migration` relies on the same fact). + /// + /// `None` for a triple the name rules refuse, which is the same refusal + /// [`WorkspaceCloneManager::resolve_clone_path`] gives and reaches the guard as + /// "no mirror to compare against": every tag in the clone then counts as + /// local, so a hand-edited record makes the guard *keep* a clone rather than + /// reach past its repository into another one. No notice is said here — this + /// runs beside `resolve_clone_path`, which says one for the same record. + pub(crate) fn resolve_bare_path(&self, recorded: &WorktreeInfo) -> Option { + validate_ref_name(&recorded.owner, NamePart::Owner).ok()?; + validate_ref_name(&recorded.repo, NamePart::Repo).ok()?; + Some(bare_dir(&self.repos_dir, &recorded.owner, &recorded.repo)) + } + /// Whether a workspace clone is on disk. /// /// Only this module's tests ask; the flows above resolve the path and look. @@ -3101,6 +3128,39 @@ mod tests { WorktreeInfo::new("owner", "repo", branch, local_path, &leaf(branch)) } + #[test] + fn the_mirror_is_named_off_the_repository_wherever_the_clone_was_recorded() { + // #487's second question. The clone may sit at a path an older build wrote, + // and `.bare` does not move with it: the mirror belongs to the repository, + // and it is the one the clone's `origin` still points at. + let cache = a_cache(); + let fake = FakeGit::new(); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + let elsewhere = cache.dir.path().join("somewhere/else/entirely"); + + assert_eq!( + manager.resolve_bare_path(&a_record("nb4", elsewhere)), + Some(bare_dir(&cache.repos_dir, "owner", "repo")) + ); + } + + #[test] + fn a_record_whose_repository_the_name_rules_refuse_names_no_mirror() { + // The same hand-edited `metadata.json` the test below is about, asked of the + // mirror instead of the clone. Answering a path built from an unsafe name + // would point the guard at whatever directory that name walked to, and the + // tags it found there would count as *vouched for* — a wrong answer in the + // deleting direction. `None` reaches the guard as "no mirror", which counts + // every tag in the clone as local and keeps it. + let cache = a_cache(); + let fake = FakeGit::new(); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + let mut recorded = a_record("nb4", clone_dir(&cache.repos_dir, "owner", "repo", "nb4")); + recorded.repo = "../../elsewhere".to_owned(); + + assert_eq!(manager.resolve_bare_path(&recorded), None); + } + #[test] fn a_record_pointing_somewhere_stale_resolves_to_the_directory_that_holds_the_work() { // Reproduced before the fix: the guard answered "nothing to lose" about the diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 135b2904..1e61ac9d 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -945,6 +945,31 @@ fn a_clone_whose_last_tag_the_remote_carries_too_is_deleted_like_any_other() { ); } +#[test] +fn a_commit_only_a_local_tag_reaches_stops_the_delete() { + // devlaunch#487 at the binary boundary, and the other side of the test above: + // the same shape of clone, clean tree, nothing on any branch, and one commit + // that only a tag reaches. The tag was typed here and never pushed, so the + // commit exists in one place on earth and `dl rm` has to say so rather than + // delete it. + // + // What tells the two worlds apart is the cache's mirror, which is the only + // thing on the host that knows which of a clone's tags came off a remote: it + // carries `v1` in the world above and has never heard of `backup` in this one. + let world = World::with(&["--local-tag"]); + let clone = "cache/devlaunch/repos/blooop/devlaunch/devlaunch-local-tag"; + + let run = world.dl(&["devlaunch-local-tag", "rm"]); + + run.exited(1); + assert_eq!( + run.err, + "devlaunch-local-tag holds 1 unpushed commit(s). Push or commit it, or run: dl \ + devlaunch-local-tag rm --force\n" + ); + assert!(world.exists(clone), "the refusal deleted the clone anyway"); +} + #[test] fn a_pushed_tag_does_not_hide_a_commit_that_really_is_nowhere_else() { // The other half of #485: the same clone with one commit of its own is still diff --git a/rust/dl/tests/lifecycle_scenario.py b/rust/dl/tests/lifecycle_scenario.py index 6260fb25..59e76c74 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -101,6 +101,12 @@ TAGGED_LEAF = "devlaunch-tagged-release" TAGGED_WS = "devlaunch-tagged-release" +# --local-tag: a recorded clone holding a commit that only a tag typed in the clone +# reaches. The tag was never pushed and the cache's mirror has never heard of it, +# so the commit under it exists in exactly one place on earth (devlaunch#487). +LOCAL_TAG_LEAF = "devlaunch-local-tag" +LOCAL_TAG_WS = "devlaunch-local-tag" + # --sealed-cache: the cache root itself refuses, so a purge removes what it can and # names what it could not. Skipped under root, which can unlink anything. @@ -333,6 +339,12 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: git(tagged, "checkout", "-q", "-B", "main", "origin/main") git(tagged, "branch", "-qD", "release") git(tagged, "remote", "prune", "origin") + # And the cache sweeps it, which is what dl's own fetch does + # (`+refs/tags/*:refs/tags/*`, forced and pruned). Without this the mirror + # would be a snapshot from before the release and the guard would rightly + # read `v1` as a tag nobody but this clone has (#487) — true of the + # directory on disk, and not the state #485 is about. + git(bare, "fetch", "-q", "origin", "+refs/tags/*:refs/tags/*", "--prune") worktrees["blooop/devlaunch/release"] = _record( "blooop", "devlaunch", "release", tagged, TAGGED_WS ) @@ -340,6 +352,26 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: TAGGED_WS, {"localFolder": str(tagged)}, OLDER, "Stopped" ) + if "local-tag" in wanted: + # devlaunch#487, and the habit that reaches it: tag before a rewrite, then + # move the branch out from under the tag. `refs/tags/backup` is the only ref + # in the clone that reaches the commit, the tag was never pushed, and the + # cache's mirror has no tag by that name at all. Same shape as the world + # above, opposite answer, and the mirror is the only thing that tells them + # apart. + held = _clone(root, origin, repos / "blooop" / "devlaunch" / LOCAL_TAG_LEAF, "rewrite") + (held / "an-hour.txt").write_text("an hour of work\n", encoding="utf-8") + git(held, "add", "-A") + git(held, "commit", "-q", "-m", "about to be rewritten") + git(held, "tag", "backup") + git(held, "reset", "-q", "--hard", "origin/main") + worktrees["blooop/devlaunch/rewrite"] = _record( + "blooop", "devlaunch", "rewrite", held, LOCAL_TAG_WS + ) + workspaces[LOCAL_TAG_WS] = _workspace( + LOCAL_TAG_WS, {"localFolder": str(held)}, OLDER, "Stopped" + ) + if "not-a-clone" in wanted: # A recorded clone directory that is there and is not a repository git can # read — an interrupted delete, or a `.git` a container wrote as another @@ -551,6 +583,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "stranded-clone", "unpushed", "tagged-release", + "local-tag", "sealed-cache", "symlinked-cache", "v1-cache", From 53dd4dc3366a50489e9a3fae7c329eb88b7d8af6 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 21:27:05 +0100 Subject: [PATCH 2/2] fix: say which tag is keeping the clone, not just that something is The refusal the tag comparison produces was "holds 1 unpushed commit(s). Push or commit it, or run: dl rm --force", and for every tag-driven instance of it that advice cannot be taken: the commit is already committed, and where the mirror is merely behind the remote it is already pushed too. What is missing is the tag in dl's cache, which no amount of pushing supplies and which the sentence did not mention. That is the state docs/cleanup.md argues against two paragraphs above the one this adds: a guard that has to be --force'd past to clear, until --force is what gets typed without reading. So the answer now carries what makes the sentence actionable. A second local query, asked only once a refusal exists and only when a local tag was named in the one that produced it, isolates the commits nothing but those tags reach: the same ref algebra with the sides swapped. Its result is a subset of the first by construction, so both counts can be said at once without either being a second measurement: holds 2 unpushed commit(s), 1 reachable only from local tag(s) (backup) Both numbers, because they answer different questions: how much would be lost, and how much of it pushing cannot clear. A commit on a branch is not blamed on a tag, and a tag reaching nothing a branch does not reach is named nowhere, so the plain sentence survives for the case where it was right. `ByLocalTags` holds the tags and the commits owed to them, both `NonEmpty`, so "some tags reached no commits" has no representation and the absent attribution is an absent value. A refusal from this query is *not* a `CouldNotTell` and it is the one place this module bends that rule: by then the loss is established and the clone is kept either way, so no failed question here can be read as permission. Only the naming half is lost. Also pins `TagRef`'s equality on both name and object, since `vouched_for` is that equality: derived on the name alone the whole comparison would degrade to "the mirror has heard of this name", which fails in the deleting direction and which every fixture where the two agree would pass. --- CHANGELOG.md | 11 ++ docs/cleanup.md | 24 +++ rust/devlaunch-core/public-api.rest.txt | 13 +- rust/devlaunch-core/src/clients/git.rs | 30 ++++ rust/devlaunch-core/src/clients/git/tests.rs | 61 +++++++- .../src/domain/workspace_state.rs | 142 ++++++++++++++++-- .../src/domain/workspace_state/tests.rs | 132 +++++++++++++++- rust/devlaunch-core/src/flows/lifecycle.rs | 7 +- rust/dl/tests/lifecycle.rs | 9 +- 9 files changed, 403 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f7e783c..9cf8af84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -210,6 +210,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the only copy of somebody's work. [docs/cleanup.md](docs/cleanup.md) has the table. + **And where a tag is the reason, the refusal now names it**, because "push or + commit it" is advice you have already taken for a commit that is under a tag: + `holds 1 unpushed commit(s), 1 reachable only from local tag(s) + (backup-before-rebase)`. Both counts, since the smaller one is how much of the + refusal pushing cannot clear. It matters in the case where the mirror is merely + *behind* the remote, which is otherwise baffling: a release you did push, on a + branch that has since gone, reads as unpushed until the next sweep, and now the + name in the message is the thing that tells you so. A commit on a branch is not + blamed on a tag, and a tag reaching nothing a branch does not reach is named + nowhere. + - **Sixty-six citations that pointed at nothing now point at something, and a guard keeps it that way.** Comments across `rust/` name the test that pins the behaviour they describe, which is most of what makes them worth reading. diff --git a/docs/cleanup.md b/docs/cleanup.md index ebe28f98..8bc50905 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -663,6 +663,30 @@ cannot establish safety fails towards keeping the clone. It is reached by a clon under a workspace, and in both the answer is a clone kept, which costs disk and nothing else. +**Where a tag is the reason, the refusal says so and names it**, because the +sentence a refusal ends with has to be one you can act on: + +``` +$ dl blooop/repo@feature rm +error: devlaunch-repo-feature-xyz holds 1 unpushed commit(s), 1 reachable only + from local tag(s) (backup-before-rebase). + Push or commit it, or run: dl blooop/repo@feature rm --force +``` + +Both counts, because they answer different questions: how much would be lost, and +how much of it pushing cannot clear. A commit under a tag is already committed, so +"push or commit it" is advice you have already taken, and the tag's name is what +tells you which case you are in. `backup-before-rebase` is the work being saved. +`v0.26.0` is a release you did push, whose tag has not reached the mirror yet, +which is what a `dl --refresh` fixes and what nothing else on the machine would +have told you. + +The count that is not attributed to a tag is left alone: a commit on a branch +needs no explaining, so a clone holding one of each reads `2 unpushed commit(s), 1 +reachable only from local tag(s) (backup)`, and a tag sitting on a commit some +branch also holds is named nowhere, because it explains nothing about why the +clone is being kept. + 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 diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index c6caf94a..cf9d034d 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -782,7 +782,9 @@ pub fn devlaunch_core::domain::workspace_state::CouldNotTell::fmt(&self, &mut co impl core::marker::StructuralPartialEq for devlaunch_core::domain::workspace_state::CouldNotTell pub enum devlaunch_core::domain::workspace_state::Loss pub devlaunch_core::domain::workspace_state::Loss::Uncommitted(devlaunch_core::domain::workspace_state::NonEmpty) -pub devlaunch_core::domain::workspace_state::Loss::Unpushed(devlaunch_core::domain::workspace_state::NonEmpty) +pub devlaunch_core::domain::workspace_state::Loss::Unpushed +pub devlaunch_core::domain::workspace_state::Loss::Unpushed::by_tags: core::option::Option +pub devlaunch_core::domain::workspace_state::Loss::Unpushed::commits: devlaunch_core::domain::workspace_state::NonEmpty impl core::clone::Clone for devlaunch_core::domain::workspace_state::Loss pub fn devlaunch_core::domain::workspace_state::Loss::clone(&self) -> devlaunch_core::domain::workspace_state::Loss impl core::cmp::Eq for devlaunch_core::domain::workspace_state::Loss @@ -803,6 +805,15 @@ pub fn devlaunch_core::domain::workspace_state::Unsaved::eq(&self, &devlaunch_co impl core::fmt::Debug for devlaunch_core::domain::workspace_state::Unsaved pub fn devlaunch_core::domain::workspace_state::Unsaved::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::domain::workspace_state::Unsaved +pub struct devlaunch_core::domain::workspace_state::ByLocalTags +impl core::clone::Clone for devlaunch_core::domain::workspace_state::ByLocalTags +pub fn devlaunch_core::domain::workspace_state::ByLocalTags::clone(&self) -> devlaunch_core::domain::workspace_state::ByLocalTags +impl core::cmp::Eq for devlaunch_core::domain::workspace_state::ByLocalTags +impl core::cmp::PartialEq for devlaunch_core::domain::workspace_state::ByLocalTags +pub fn devlaunch_core::domain::workspace_state::ByLocalTags::eq(&self, &devlaunch_core::domain::workspace_state::ByLocalTags) -> bool +impl core::fmt::Debug for devlaunch_core::domain::workspace_state::ByLocalTags +pub fn devlaunch_core::domain::workspace_state::ByLocalTags::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_core::domain::workspace_state::ByLocalTags pub struct devlaunch_core::domain::workspace_state::NonEmpty impl devlaunch_core::domain::workspace_state::NonEmpty pub fn devlaunch_core::domain::workspace_state::NonEmpty::describe(&self) -> alloc::string::String diff --git a/rust/devlaunch-core/src/clients/git.rs b/rust/devlaunch-core/src/clients/git.rs index 88437365..2d773be4 100644 --- a/rust/devlaunch-core/src/clients/git.rs +++ b/rust/devlaunch-core/src/clients/git.rs @@ -575,6 +575,36 @@ impl<'r> Git<'r> { self.about(clone, &TAG_REFS_QUERY).map(tag_refs_in) } + /// Of the commits [`Git::unpushed_commits`] counted, the ones *only* these + /// local tags reach. + /// + /// The same ref algebra as that query with the sides swapped: the tags are the + /// whole positive set, and everything else — the remotes and every local ref + /// that is not a tag — is subtracted. So a commit here is on no remote, on no + /// branch, in no worktree HEAD and in no stash, and the only thing naming it + /// is a tag this clone's mirror does not vouch for. + /// + /// It is asked for the *sentence*, not for the decision: the refusal is + /// already decided by the query above, and this says which tags to name in it. + /// "1 unpushed commit(s)" tells someone to push or commit something they have + /// already committed, and where the mirror is merely behind, already pushed + /// too; "reachable only from local tag(s) (backup)" is the same fact said in + /// a way that can be acted on. + /// + /// The result is a subset of the other query's by construction, which is what + /// lets the two counts be reported in one sentence without either being a + /// second measurement of the same thing. + pub(crate) fn commits_only_tags_reach( + &self, + clone: &Path, + local_tags: &[String], + ) -> GitAnswer { + let mut args: Vec<&str> = vec!["log", "--oneline"]; + args.extend(local_tags.iter().map(String::as_str)); + args.extend(["--not", "--remotes", "--exclude=refs/tags/*", "--all"]); + self.about(clone, &args) + } + /// Every tag in the bare cache at *bare*, with the object each one names. /// /// `--git-dir` and no work tree, because a bare has none, and no cwd, because diff --git a/rust/devlaunch-core/src/clients/git/tests.rs b/rust/devlaunch-core/src/clients/git/tests.rs index 58e08f51..832eb6d3 100644 --- a/rust/devlaunch-core/src/clients/git/tests.rs +++ b/rust/devlaunch-core/src/clients/git/tests.rs @@ -170,6 +170,32 @@ fn the_pinned_verbs_ask_exactly_what_python_asked() { ); } +#[test] +fn the_attribution_query_is_the_unpushed_one_with_the_sides_swapped() { + // The tags are the whole positive set and everything else is subtracted, so a + // commit it returns is on no remote, no branch, no worktree HEAD and no stash. + // `--exclude` still binds to the `--all` that follows it, which is why that + // pair stays adjacent on the negative side rather than being split up. + let (dir, _root) = a_clone(); + let fake = ScriptedRunner::new(); + let git = Git::new(&fake); + + git.commits_only_tags_reach(dir.path(), &["refs/tags/backup".to_owned()]); + + assert_eq!( + strs(&argv(&fake))[3..], + [ + "log", + "--oneline", + "refs/tags/backup", + "--not", + "--remotes", + "--exclude=refs/tags/*", + "--all" + ] + ); +} + #[test] fn the_two_tag_queries_ask_the_same_question_of_each_side() { // The clone's tags and the bare's are compared to each other, so the two @@ -209,6 +235,38 @@ fn the_two_tag_queries_ask_the_same_question_of_each_side() { assert_eq!(timeout(&fake), Some(Duration::from_secs(30))); } +#[test] +fn a_tag_is_its_object_as_well_as_its_name() { + // `vouched_for` in the delete guard is `fetched.iter().any(|cached| cached == + // tag)`, so this equality *is* the safety rule: derived on the name alone it + // would degrade to "the mirror has heard of this name", which silently excuses + // a tag the clone moved onto a commit that exists nowhere else. That failure + // has no test of its own that could catch it, because every fixture where the + // two agree passes either way. Hence one here, on the equality itself. + let same = TagRef { + name: "refs/tags/v1".to_owned(), + object: "aaa".to_owned(), + }; + let moved = TagRef { + name: "refs/tags/v1".to_owned(), + object: "bbb".to_owned(), + }; + let renamed = TagRef { + name: "refs/tags/v2".to_owned(), + object: "aaa".to_owned(), + }; + + assert_eq!(same, same.clone()); + assert_ne!( + same, moved, + "the same name at another object is another tag" + ); + assert_ne!( + same, renamed, + "and the same object under another name is too" + ); +} + #[test] fn a_tag_listing_is_read_as_the_pairs_it_is() { let fake = ScriptedRunner::new().with_script( @@ -1111,8 +1169,9 @@ fn nothing_here_spawns_more_than_once_per_verb() { git.unpushed_commits(Path::new("/ws"), &[]); git.tags_in_clone(Path::new("/ws")); git.tags_in_bare(Path::new("/cache/.bare")); + git.commits_only_tags_reach(Path::new("/ws"), &[]); - assert_eq!(fake.call_count(), 30, "one spawn per verb, 30 verbs"); + assert_eq!(fake.call_count(), 31, "one spawn per verb, 31 verbs"); assert!( fake.calls() .iter() diff --git a/rust/devlaunch-core/src/domain/workspace_state.rs b/rust/devlaunch-core/src/domain/workspace_state.rs index 5fda5902..4eeba4f0 100644 --- a/rust/devlaunch-core/src/domain/workspace_state.rs +++ b/rust/devlaunch-core/src/domain/workspace_state.rs @@ -248,9 +248,71 @@ pub enum Loss { Uncommitted(NonEmpty), /// Commits no remote-tracking ref contains, one `git log --oneline` line /// each. - Unpushed(NonEmpty), + Unpushed { + commits: NonEmpty, + /// The share of those commits that nothing but a local tag reaches, when + /// there is such a share. + /// + /// `None` is "no tag is why this clone is being kept", and it is the + /// ordinary case: an unpushed commit on a branch needs no explaining, + /// because the sentence the reader gets already tells them what to do + /// about it. `Some` is the case where that sentence is wrong, so the + /// answer carries what makes it right instead of the reader guessing. + by_tags: Option, + }, } +/// The local tags an unpushed count is owed to, and the commits owed to them. +/// +/// Both halves are [`NonEmpty`], so this value cannot say "some tags reached no +/// commits" or "some commits were reached by no tags": it exists exactly when a +/// tag this clone's mirror does not vouch for is the only thing naming a commit, +/// which is the one case where "push or commit it" is advice the reader cannot +/// act on. Where nothing is owed to a tag there is no value, not an empty one. +/// +/// Two shapes reach it and the sentence does not distinguish them, deliberately. +/// The mirror has never heard of the tag (#487's own case: tagged here, never +/// pushed) and the mirror is simply behind the remote (the tag was pushed from +/// this clone and no sweep has run since). Naming the tag settles both without +/// this having to know which: a reader who recognises `v0.26.0` as a release they +/// pushed learns the cache is stale, and a reader who sees `backup-before-rebase` +/// learns the thing that saves their work. +#[derive(Clone, Debug, PartialEq, Eq)] +// binary surface — not part of the frozen wf API (#251 §7) +pub struct ByLocalTags { + /// Short tag names, `backup` rather than `refs/tags/backup`, because this is + /// what a person types and reads. + tags: NonEmpty, + /// The `git log --oneline` lines only those tags reach. + commits: NonEmpty, +} + +impl ByLocalTags { + /// The attribution, or nothing when no commit is owed to a tag. + /// + /// Takes both sides through [`NonEmpty::of`], so the empty case is the absent + /// value rather than a value that describes nothing. *tags* arrive as full + /// refnames, which is what the query names them by, and are shortened here so + /// there is one place that knows the prefix. + pub(crate) fn of( + tags: impl IntoIterator, + commits: impl IntoIterator, + ) -> Option { + Some(Self { + tags: NonEmpty::of(tags.into_iter().map(|reference| { + reference + .strip_prefix(REFS_TAGS) + .unwrap_or(&reference) + .to_owned() + }))?, + commits: NonEmpty::of(commits)?, + }) + } +} + +/// The prefix every tag refname carries, stripped for display. +const REFS_TAGS: &str = "refs/tags/"; + impl Loss { /// Python's phrasing, exactly: this text reaches a person through /// `dl rm`'s refusal and a tool through `--ls --json`'s `wouldLose`, so @@ -262,7 +324,23 @@ impl Loss { changed.len(), name_a_few(changed, NAME_AT_MOST) ), - Self::Unpushed(commits) => format!("{} unpushed commit(s)", commits.len()), + Self::Unpushed { + commits, + by_tags: None, + } => format!("{} unpushed commit(s)", commits.len()), + // Both counts, because they are different facts and the smaller one is + // the actionable half: it says how much of the refusal "push it" cannot + // clear. Where every unpushed commit is a tag's, the two numbers agree + // and the sentence is merely emphatic rather than wrong. + Self::Unpushed { + commits, + by_tags: Some(by_tags), + } => format!( + "{} unpushed commit(s), {} reachable only from local tag(s) ({})", + commits.len(), + by_tags.commits.len(), + first_few(&by_tags.tags, NAME_AT_MOST), + ), } } } @@ -450,15 +528,28 @@ pub(crate) fn holds_unsaved_work(git: &Git<'_>, clone: &Path, bare: BareCache<'_ /// path starts at offset 3; a rename reads `old -> new`, and the whole field is /// kept rather than split, because both halves are the news. fn name_a_few(changed: &NonEmpty, limit: usize) -> String { - let mut names: Vec<&str> = changed - .iter() - .take(limit) - .filter_map(|line| path_in(line)) - .collect(); - if changed.len() > limit { - names.push("…"); + let named = changed.iter().filter_map(|line| path_in(line)); + // Counted from the porcelain lines rather than from `named`, so a line too + // short to hold a path still counts towards "there are more than these". + cut_short(named, changed.len(), limit) +} + +/// The first few of *names*, with an ellipsis when there were more. +/// +/// The tag half of the same rule [`name_a_few`] applies to changed paths, sharing +/// its one implementation: two truncation rules that drifted would be two +/// sentences claiming to elide the same way. +fn first_few(names: &NonEmpty, limit: usize) -> String { + cut_short(names.iter().map(String::as_str), names.len(), limit) +} + +/// *limit* of the names, then `…` when *total* is larger. +fn cut_short<'a>(names: impl Iterator, total: usize, limit: usize) -> String { + let mut kept: Vec<&str> = names.take(limit).collect(); + if total > limit { + kept.push("…"); } - names.join(", ") + kept.join(", ") } /// The path field of one porcelain line, or nothing when the line is too short to @@ -516,7 +607,8 @@ fn unsaved(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> Unsaved { match git.unpushed_commits(clone, &local_tags) { GitAnswer::Said(unpushed) => { if let Some(commits) = NonEmpty::of(unpushed.lines().map(str::to_owned)) { - losses.push(Loss::Unpushed(commits)); + let by_tags = owed_to_tags(git, clone, &local_tags); + losses.push(Loss::Unpushed { commits, by_tags }); } } GitAnswer::Refused(refused) => { @@ -532,6 +624,34 @@ fn unsaved(git: &Git<'_>, clone: &Path, bare: BareCache<'_>) -> Unsaved { } } +/// Which of the unpushed commits nothing but a local tag reaches, if any. +/// +/// Asked only once there is a refusal to explain, and only when a local tag was +/// named in the question that produced it, so a clone with nothing to lose pays +/// nothing and a clone with no tags pays nothing. +/// +/// **A refusal here is not a [`Unsaved::CouldNotTell`], and that is the one place +/// this module bends its own rule.** Everywhere else a git command that fails +/// after the repository has been shown readable takes the whole answer with it, +/// because the alternative is accounting for the failure as "nothing to lose" — +/// permission, granted by a question that did not work. Nothing of the sort is +/// available here: the loss is already established and the clone is already being +/// kept. All that is lost is the half of the sentence that says which tag, so the +/// answer degrades to the sentence it had before (#487) rather than to a worse +/// arm. Failing the whole reading would trade a plainer refusal for a vaguer one. +fn owed_to_tags(git: &Git<'_>, clone: &Path, local_tags: &[String]) -> Option { + if local_tags.is_empty() { + return None; + } + let GitAnswer::Said(only_tags) = git.commits_only_tags_reach(clone, local_tags) else { + return None; + }; + ByLocalTags::of( + local_tags.iter().cloned(), + only_tags.lines().map(str::to_owned), + ) +} + /// The clone's tags that the bare cache does not vouch for, as full refnames. /// /// #487's whole answer. The unpushed question excludes `refs/tags/*` wholesale diff --git a/rust/devlaunch-core/src/domain/workspace_state/tests.rs b/rust/devlaunch-core/src/domain/workspace_state/tests.rs index 22b4b3b8..f1042b4e 100644 --- a/rust/devlaunch-core/src/domain/workspace_state/tests.rs +++ b/rust/devlaunch-core/src/domain/workspace_state/tests.rs @@ -445,9 +445,12 @@ fn a_commit_only_an_unpushed_local_tag_reaches_is_unsaved() { "the mirror has no tag at all, so this one was typed here" ); + // The whole sentence, tag named. "1 unpushed commit(s)" alone tells the reader + // to push or commit something they have already committed; the tag's name is + // what turns the refusal into the thing that saves the work. assert_eq!( would_lose(&held_against(&clone, &fixture.remote)), - "1 unpushed commit(s)" + "1 unpushed commit(s), 1 reachable only from local tag(s) (backup)" ); } @@ -477,7 +480,7 @@ fn a_local_tag_the_bare_holds_at_another_object_is_unsaved() { assert_eq!( would_lose(&held_against(&clone, &fixture.remote)), - "1 unpushed commit(s)" + "1 unpushed commit(s), 1 reachable only from local tag(s) (v1)" ); } @@ -523,9 +526,13 @@ fn with_no_bare_to_compare_against_every_tag_counts() { Unsaved::NothingToLose, "with the mirror named it is #485's answer" ); + // And it names the tag, which is what makes a mirror that is merely *behind* + // diagnosable rather than baffling: a reader who recognises `v1` as a release + // they pushed learns the cache is stale, where "push or commit it" tells them + // to do a thing they have already done. assert_eq!( would_lose(&held(&clone)), - "1 unpushed commit(s)", + "1 unpushed commit(s), 1 reachable only from local tag(s) (v1)", "and without one, the same clone is kept" ); } @@ -545,12 +552,117 @@ fn a_bare_that_is_not_a_repository_counts_every_tag_too() { git(&clone, &["tag", "backup"]); git(&clone, &["reset", "-q", "--hard", "origin/feature"]); + // Both tags are named, and that is the honest answer rather than a loose one: + // with no mirror readable, nothing has established which of them the remote + // has, so both are candidates for the commit nothing else reaches. Narrowing + // the list would mean claiming knowledge this run does not have. assert_eq!( would_lose(&held_against(&clone, &fixture.path("no-such-bare"))), - "1 unpushed commit(s)" + "1 unpushed commit(s), 1 reachable only from local tag(s) (backup, v1)" + ); +} + +#[test] +fn a_branch_commit_is_not_blamed_on_a_tag_that_happens_to_be_there() { + // The over-claim this could have shipped instead. The clone holds two unpushed + // commits for two different reasons: one on a branch, which "push it" really + // does clear, and one only a local tag reaches, which it does not. The sentence + // has to carry both numbers, because a reader told "2 unpushed commit(s), + // 2 reachable only from local tag(s)" would go looking for a tag that explains + // the branch commit and find none. + let fixture = Fixture::new(); + let clone = fixture.clone(); + git(&clone, &["checkout", "-q", "-b", "wip"]); + write(&clone.join("branch-work.txt"), "an hour of work\n"); + commit(&clone, "branch work"); + git(&clone, &["checkout", "-q", "feature"]); + write(&clone.join("rewritten.txt"), "another hour\n"); + commit(&clone, "about to be rewritten"); + git(&clone, &["tag", "backup"]); + git(&clone, &["reset", "-q", "--hard", "origin/feature"]); + + assert_eq!( + would_lose(&held_against(&clone, &fixture.remote)), + "2 unpushed commit(s), 1 reachable only from local tag(s) (backup)" ); } +#[test] +fn a_tag_that_reaches_nothing_of_its_own_is_not_named_in_the_refusal() { + // The other half of the same honesty. A local tag sitting on a commit some + // branch also holds explains nothing about why this clone is being kept, and + // naming it would send the reader after the wrong ref. The unpushed commit here + // is the branch's, and the sentence stays the plain one. + let fixture = Fixture::new(); + let clone = fixture.clone(); + write(&clone.join("more.txt"), "more\n"); + commit(&clone, "more"); + git(&clone, &["tag", "sits-on-the-branch"]); + + assert_eq!( + would_lose(&held_against(&clone, &fixture.remote)), + "1 unpushed commit(s)", + "the tag reaches nothing the branch does not" + ); +} + +#[test] +fn a_long_list_of_tags_is_cut_short_like_every_other_list() { + // One truncation rule, shared with the changed-paths list rather than written + // twice: a stale mirror on a repository that tags releases makes every tag + // local, and a refusal that dumped four hundred names would be unreadable in + // exactly the case a person most needs to read it. + let fixture = Fixture::new(); + let clone = fixture.clone(); + write(&clone.join("an-hour.txt"), "an hour of work\n"); + commit(&clone, "about to be rewritten"); + for n in 0..5 { + git(&clone, &["tag", &format!("backup-{n}")]); + } + git(&clone, &["reset", "-q", "--hard", "origin/feature"]); + + let description = would_lose(&held_against(&clone, &fixture.remote)); + + assert!( + description.starts_with("1 unpushed commit(s), 1 reachable only from local tag(s) ("), + "{description:?}" + ); + assert!( + description.ends_with("(backup-0, backup-1, backup-2, …)"), + "three names and an ellipsis: {description:?}" + ); +} + +#[test] +fn the_attribution_is_given_up_rather_than_the_refusal_when_git_will_not_say() { + // The one place this module does *not* turn a refusal into `CouldNotTell`, and + // the reason that is safe: by the time this is asked the loss is established + // and the clone is being kept either way, so no failed question here can be + // read as permission. Only the half of the sentence naming the tag is lost. + // Asserted on the decision itself rather than through a fixture, because the + // two `git log` calls share an argv prefix and cannot be scripted apart. + let refused = ScriptedRunner::new().with_script(["git"], Response::failed(128, "fatal: nope")); + + assert_eq!( + owed_to_tags( + &Git::new(&refused), + Path::new("/ws"), + &["refs/tags/backup".to_owned()] + ), + None + ); +} + +#[test] +fn no_local_tags_is_no_question_asked() { + // The cheap path, pinned as a spawn count: a clone whose every tag the mirror + // vouches for pays nothing for the sentence it does not need. + let fake = ScriptedRunner::new(); + + assert_eq!(owed_to_tags(&Git::new(&fake), Path::new("/ws"), &[]), None); + assert_eq!(fake.call_count(), 0); +} + #[test] fn a_clone_with_no_tags_never_asks_the_bare() { // The bare is one more spawn per clone in `dl --ls`, and a repository with no @@ -1114,9 +1226,10 @@ fn each_arm_renders_as_one_key_that_names_it() { r#"{"nothingToLose":true}"# ); assert_eq!( - Unsaved::WouldLose(Losses::one(Loss::Unpushed(NonEmpty::one( - "abc123 more".to_owned() - )))) + Unsaved::WouldLose(Losses::one(Loss::Unpushed { + commits: NonEmpty::one("abc123 more".to_owned()), + by_tags: None, + })) .as_json() .to_string(), r#"{"wouldLose":"1 unpushed commit(s)"}"# @@ -1176,7 +1289,10 @@ fn two_of_the_three_answers_refuse_a_delete() { for (unsaved, may_delete) in [ (Unsaved::NothingToLose, true), ( - Unsaved::WouldLose(Losses::one(Loss::Unpushed(NonEmpty::one("abc".to_owned())))), + Unsaved::WouldLose(Losses::one(Loss::Unpushed { + commits: NonEmpty::one("abc".to_owned()), + by_tags: None, + })), false, ), ( diff --git a/rust/devlaunch-core/src/flows/lifecycle.rs b/rust/devlaunch-core/src/flows/lifecycle.rs index fb67e8b5..071ef503 100644 --- a/rust/devlaunch-core/src/flows/lifecycle.rs +++ b/rust/devlaunch-core/src/flows/lifecycle.rs @@ -5713,9 +5713,10 @@ mod tests { #[test] fn work_saved_nowhere_else_stops_the_delete_and_names_what_it_is() { - let losses = Losses::one(workspace_state::Loss::Unpushed(NonEmpty::one( - "abc1234 later".to_owned(), - ))); + let losses = Losses::one(workspace_state::Loss::Unpushed { + commits: NonEmpty::one("abc1234 later".to_owned()), + by_tags: None, + }); let guarded = guard_removal( "ws", Unsaved::WouldLose(losses.clone()), diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 1e61ac9d..e679ed92 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -962,10 +962,15 @@ fn a_commit_only_a_local_tag_reaches_stops_the_delete() { let run = world.dl(&["devlaunch-local-tag", "rm"]); run.exited(1); + // The tag is named, and that is the half of this sentence a person can act on. + // "Push or commit it" is advice for a commit that is neither pushed nor + // committed; this one is committed, and where the mirror is merely behind it is + // pushed as well. `backup` is what tells the reader which of those they are + // looking at, and in the #487 case it is the name of the thing being saved. assert_eq!( run.err, - "devlaunch-local-tag holds 1 unpushed commit(s). Push or commit it, or run: dl \ - devlaunch-local-tag rm --force\n" + "devlaunch-local-tag holds 1 unpushed commit(s), 1 reachable only from local tag(s) \ + (backup). Push or commit it, or run: dl devlaunch-local-tag rm --force\n" ); assert!(world.exists(clone), "the refusal deleted the clone anyway"); }