From fb7f1cb9b2d5509e1803e7bed84ebd8d259c9b0d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 18:48:44 +0000 Subject: [PATCH 1/3] feat: the selector draws a workspace id apart instead of whole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A picker row was `{owner} | {id}`, where the id is `--`. Two of those three parts are what somebody choosing a workspace is looking for and the third is machinery: the eight-character syllable suffix exists to keep two branches from sharing an id, and reading it is no part of picking one. Rows are now `{owner} | {repo} | {ref}`. Both new halves come off the source devpod already reported, so the picker still opens no records and reads no config: `/repos/ //` names the repo, and `workspace_id::ref_slug_of` strips the repo prefix and the suffix off the id to leave the ref-slug. That inverse lives beside the derivation it undoes, because a caller spelling out the suffix width or the repo cap for itself would be the second derivation to disagree with the first. Three columns and not `owner/repo@ref`, which reads like a spec `dl` would accept and is not one: `slug` collapses `/` and `-` alike, so a retyped `feature-auth` row can address the other branch. And the elision is never unconditional. `chosen` maps a picked row back to its workspace by the row's own text with the first match winning, so two rows drawn the same would be one workspace acted on in place of another — `dl rm` among the verbs this picker opens for. Where a split would collide, both rows go back to their whole ids, which puts the suffix on screen in exactly the case it is doing work. `public-api.txt` is edited by hand for the two added functions, copied from the `owner_of` line they are shaped after; CI regenerates it. --- README.md | 34 +- rust/devlaunch-core/public-api.txt | 2 + .../devlaunch-core/src/domain/workspace_id.rs | 220 +++++++++++ rust/devlaunch-core/src/flows/listing.rs | 95 ++++- rust/dl/src/select.rs | 363 ++++++++++++++++-- 5 files changed, 658 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 7ef5e29a..0724bf4e 100644 --- a/README.md +++ b/README.md @@ -87,11 +87,35 @@ one. It draws the search bar on the top line with the matches reading downward f first match is the row nearest what you are typing. A reserved verb wins over a workspace name of the same spelling: `dl stop` opens the selector to stop something, it does not look for a workspace called `stop`. -For the verbs that finish on their own — `up`, `stop`, `rm`, `code` and `dotfiles` — the selector -takes more than one row: TAB marks any number and Enter applies the verb to each in turn, so -`dl rm` can clear five dead workspaces in one visit. The forms that end in an interactive session -(`dl`, `dl -- `, `restart`, `recreate`, `reset`) take exactly one, since several of those -would just be sessions queued behind each other's exit. +Each row is `owner | repo | branch`, aligned into columns: + +``` +blooop | devlaunch | main +blooop | devlaunch | picker-columns +kinisi-robotics | kinisi_ros | ags-devcontainer-tooling-su +- | myproject +``` + +That is the [workspace id](#workspace-ids) read apart, with the hashed suffix left +off — it is there to keep two branches from sharing an id, and reading it is no part +of choosing a workspace. The owner is not in the id at all, so a fork and its +upstream used to be two rows spelled the same. A workspace `dl` did not clone has no +owner or repo to read out of it, so it keeps whatever name devpod has for it and a +dash where the owner would go. + +**The right-hand column is the branch as the id spells it, which is not always the +branch.** It is slugged, so `feature/auth` reads as `feature-auth`, and a long one is +shortened — the third row above is really `ags-devcontainer-tooling-support`. That is +why the row is three columns and not `owner/repo@branch`: the latter reads like +something you could retype, and retyping a slugged branch name can address a +different workspace. To act on what you picked, pick it — the row carries the id +underneath. + +Two branches can therefore share the middle and right columns: `feature/auth` and +`feature-auth` read alike. When that happens **both** rows go back to their full ids, +suffix and all, because the row's own text is how `dl` knows which workspace you +picked — two rows reading the same would be one workspace deleted in place of +another. The suffix appears exactly where it is doing work. ### Examples diff --git a/rust/devlaunch-core/public-api.txt b/rust/devlaunch-core/public-api.txt index e76315c7..bfffc8ec 100644 --- a/rust/devlaunch-core/public-api.txt +++ b/rust/devlaunch-core/public-api.txt @@ -1736,6 +1736,8 @@ pub fn devlaunch_core::flows::listing::enriched_listing(&mut devlaunch_core::flo pub fn devlaunch_core::flows::listing::flatten_repos(&indexmap::map::IndexMap>) -> alloc::vec::Vec pub fn devlaunch_core::flows::listing::json_document(&[devlaunch_core::flows::listing::ListedWorkspace]) -> serde_json::value::Value pub fn devlaunch_core::flows::listing::owner_of(&devlaunch_core::clients::devpod::Workspace, &std::path::Path) -> core::option::Option +pub fn devlaunch_core::flows::listing::ref_slug_of(&devlaunch_core::clients::devpod::Workspace, &std::path::Path) -> core::option::Option +pub fn devlaunch_core::flows::listing::repo_of(&devlaunch_core::clients::devpod::Workspace, &std::path::Path) -> core::option::Option pub fn devlaunch_core::flows::listing::workspace_table(&mut devlaunch_core::flows::listing::CommandContext<'_>, &std::path::Path, devlaunch_core::flows::listing::Sizes) -> core::result::Result pub mod devlaunch_core::flows::migration pub enum devlaunch_core::flows::migration::Listing diff --git a/rust/devlaunch-core/src/domain/workspace_id.rs b/rust/devlaunch-core/src/domain/workspace_id.rs index fe78cd43..f1a4e665 100644 --- a/rust/devlaunch-core/src/domain/workspace_id.rs +++ b/rust/devlaunch-core/src/domain/workspace_id.rs @@ -372,6 +372,92 @@ fn fit_ref(git_ref: &str, room: usize) -> String { .to_string() } +/// The `` an id carries, for a workspace derived for *repo*. +/// +/// The display-side inverse of [`WorkspaceId::value`]'s readable half. It lives +/// here because that is where the halves were joined: a caller that spelled out +/// the suffix width or the repo cap for itself would be a second derivation to +/// disagree with the first, which is defect #4 of devlaunch#55 — one rule, two +/// derivations — in the direction nothing has written yet. +/// +/// **The repo has to come from outside, because the id does not say where its own +/// first boundary is.** Both slugs may hold dashes, so `devlaunch-main-zovomobo` +/// reads equally well as repo `devlaunch` with ref `main` and as repo +/// `devlaunch-main` with no ref at all. The caller that has a repo to pass is the +/// one reading dl's own clone layout, `/repos///`, which +/// names it. +/// +/// `None` for anything that does not read as one, and every arm of that is a +/// workspace a caller should show whole instead: an id with no syllable suffix on +/// it, a repo whose slug is not the prefix under either spelling, or nothing left +/// between the two. The suffix check is what makes this answer `None` for a name +/// dl did not derive rather than cutting eight characters off the end of it. +/// +/// **What comes back is a slug, and a slug is not a ref.** [`slug`] collapses `/` +/// and `-` alike, and [`fit_ref`] drops whole segments before it truncates +/// characters — so `feature/auth` and `feature-auth` both read back as +/// `feature-auth`, and a long ref reads back short. Nothing may hand the result to +/// [`WorkspaceId::new`] and expect the workspace it came from: it is a label to +/// read, and the id remains the only thing that addresses anything. +pub(crate) fn ref_slug_of<'a>(id: &'a str, repo: &str) -> Option<&'a str> { + let body = without_suffix(id)?; + let repo_slug = slug(repo); + if repo_slug.is_empty() { + // `value` joins with the empty part dropped, so an id for a repo whose + // slug is empty carries no repo part and no separator for one. + return non_empty(body); + } + // The full spelling first and the cut one second. `value` cuts the repo slug + // to REPO_SLUG_LENGTH only when the id would otherwise overflow, so both are + // live — and trying the cut one first would eat the front of the ref for + // every repo whose slug is inside the cap. + let cut = head(&repo_slug, REPO_SLUG_LENGTH).trim_matches('-'); + let rest = after_part(body, &repo_slug).or_else(|| after_part(body, cut))?; + non_empty(rest) +} + +/// *body* with `-` taken off the front, or `None` if it does not start that +/// way. +/// +/// The separator is required, which is what keeps a repo slug that is merely a +/// *prefix* of a longer one from matching: `dev` does not strip `devlaunch-main`. +fn after_part<'a>(body: &'a str, part: &str) -> Option<&'a str> { + body.strip_prefix(part)?.strip_prefix('-') +} + +/// An id with its identity suffix and the separator in front of it removed. +/// +/// `None` unless the last [`SUFFIX_LENGTH`] characters really are a syllable +/// suffix and a `-` precedes them. Checking the syllables rather than just +/// counting characters is what makes this a parse: `some-hand-made-ws` is not an +/// id this module derived, and cutting its last eight characters off would answer +/// a confident lie where `None` is the truth. +fn without_suffix(id: &str) -> Option<&str> { + let cut = id.len().checked_sub(SUFFIX_LENGTH)?; + if !is_syllables(id.get(cut..)?) { + return None; + } + id.get(..cut)?.strip_suffix('-') +} + +/// Whether *text* is exactly what [`syllable_suffix`] emits: [`SYLLABLES`] +/// consonant-vowel pairs drawn from the two tables. +/// +/// Byte-wise, which is sound because both tables are ASCII: a non-ASCII character +/// cannot be in either, so it fails the test rather than splitting a character. +fn is_syllables(text: &str) -> bool { + let bytes = text.as_bytes(); + bytes.len() == SUFFIX_LENGTH + && bytes + .chunks(2) + .all(|pair| CONSONANTS.contains(&pair[0]) && VOWELS.contains(&pair[1])) +} + +/// *text* unless it is empty, so "nothing was left" is one answer and not two. +fn non_empty(text: &str) -> Option<&str> { + (!text.is_empty()).then_some(text) +} + /// SHA-256 (FIPS 180-4), because the frozen suffix is defined by this digest. /// The `sha2` crate rather than a hand-rolled compression function: the golden /// ids pin the output either way, and crypto primitives are the one place @@ -1538,4 +1624,138 @@ mod tests { assert_eq!(parsed.repo(), "devlaunch"); assert_eq!(parsed.git_ref(), "feature/auth"); } + + // -------------------------------------------------- reading an id apart + + /// The id a triple derives, so these read against the real derivation rather + /// than against a hand-spelled string that could drift from it. + fn derived(owner: &str, repo: &str, git_ref: &str) -> String { + WorkspaceId::new(owner, repo, git_ref) + .expect("a safe triple") + .value() + } + + #[test] + fn an_id_gives_up_its_ref_slug_when_the_repo_is_known() { + // The whole point of the inverse: the readable half of an id is the two + // things a person is looking for, and the repo is what says where the + // boundary between them falls. + let id = derived("blooop", "devlaunch", "main"); + + assert_eq!(id, "devlaunch-main-zovomobo"); + assert_eq!(ref_slug_of(&id, "devlaunch"), Some("main")); + } + + #[test] + fn a_repo_slug_cut_to_the_cap_is_still_recognised() { + // `value` cuts the repo slug to REPO_SLUG_LENGTH when the id would + // otherwise overflow, so the prefix in the id is not always `slug(repo)` — + // and a reader that only tried the full spelling would answer `None` for + // every workspace of a long-named repository. + let repo = "a-very-long-repository-name-indeed"; + let id = derived("blooop", repo, "main"); + + assert_eq!(id, "a-very-long-reposito-main-mafedavi"); + assert!(slug(repo).len() > REPO_SLUG_LENGTH, "the cap has to bite"); + assert_eq!(ref_slug_of(&id, repo), Some("main")); + } + + #[test] + fn the_full_repo_spelling_is_tried_before_the_cut_one() { + // Order matters and only shows up on a repo whose slug is *inside* the cap: + // `head(slug, 20)` of a shorter slug is the slug itself, so both candidates + // agree — but a reader that tried a *cut* candidate first on a repo like + // `devlaunch` would strip fewer characters than the id spent and hand back + // a ref-slug with the tail of the repo name still on the front. + let id = derived("blooop", "devlaunch", "feature/auth"); + + assert_eq!(ref_slug_of(&id, "devlaunch"), Some("feature-auth")); + // The same id read against a repo it was not derived for: the prefix does + // not match under either spelling, so there is no ref to report. + assert_eq!(ref_slug_of(&id, "wayfinder"), None); + } + + #[test] + fn a_long_ref_reads_back_as_the_slug_the_id_kept_and_not_as_the_ref() { + // The caveat the doc comment leads with, pinned: `fit_ref` drops whole + // middle segments, so what comes back is legible and is *not* the ref. A + // caller that handed this to `WorkspaceId::new` would derive a different + // workspace, which is why nothing does. + let git_ref = "dependabot/github_actions/codecov/codecov-action-6"; + let id = derived("blooop", "devlaunch", git_ref); + + assert_eq!( + ref_slug_of(&id, "devlaunch"), + Some("dependabot-codecov-action-6") + ); + assert_ne!(ref_slug_of(&id, "devlaunch"), Some(git_ref)); + } + + #[test] + fn two_refs_that_slug_alike_read_back_alike() { + // Defect #1 of devlaunch#55, in the one place it survives: `slug` collapses + // `/` and `-`, so these two branches are two workspaces with one readable + // part between them. The ids differ — that is what the suffix is for — and a + // caller drawing only the readable part has to notice, because the string + // it is about to print does not distinguish them. + let over = derived("blooop", "devlaunch", "feature/auth"); + let under = derived("blooop", "devlaunch", "feature-auth"); + + assert_ne!(over, under); + assert_eq!(ref_slug_of(&over, "devlaunch"), Some("feature-auth")); + assert_eq!(ref_slug_of(&under, "devlaunch"), Some("feature-auth")); + } + + #[test] + fn a_name_this_module_did_not_derive_is_refused_rather_than_cut() { + // The check that makes this a parse instead of a substring operation. + // Without it every one of these would answer a confident lie: eight + // characters off the end of a name that never had a suffix on it. + for name in [ + // No syllables: `made-ws` is not four consonant-vowel pairs. + "some-hand-made-ws", + // Right shape, wrong tables: `q` and `u` are in neither. + "devlaunch-main-qulaquli", + // Nothing but a suffix, so there is no separator and no repo part. + "zovomobo", + // Shorter than a suffix. + "ws", + "", + ] { + assert_eq!(ref_slug_of(name, "devlaunch"), None, "{name}"); + } + } + + #[test] + fn a_multibyte_name_is_refused_without_splitting_a_character() { + // `checked_sub` counts bytes, so a name whose last bytes are the middle of a + // character would panic on a naive slice. `str::get` answering `None` on a + // boundary that is not one is what keeps this total — and the tables are + // ASCII, so no non-ASCII name could have been an id anyway. + assert_eq!(ref_slug_of("devlaunch-main-zzzzzzé", "devlaunch"), None); + assert_eq!(ref_slug_of("é", "devlaunch"), None); + } + + #[test] + fn an_id_with_no_ref_part_left_answers_nothing_rather_than_an_empty_label() { + // A ref whose slug is empty leaves `-`, so there is a repo + // prefix and a suffix and nothing between them. `None` rather than + // `Some("")`, so a caller has one answer to handle and not two. + let id = derived("blooop", "devlaunch", "_"); + + assert_eq!(id, "devlaunch-sasevapo"); + assert_eq!(ref_slug_of(&id, "devlaunch"), None); + } + + #[test] + fn a_repo_whose_slug_is_empty_leaves_the_ref_alone() { + // The mirror case: `value` drops the empty repo part *and* its separator, so + // the id is `-` and there is no prefix to strip. A reader + // that insisted on one would answer `None` for a workspace it can describe + // perfectly well. + let id = derived("blooop", "_", "main"); + + assert_eq!(id, "main-gakebofi"); + assert_eq!(ref_slug_of(&id, "_"), Some("main")); + } } diff --git a/rust/devlaunch-core/src/flows/listing.rs b/rust/devlaunch-core/src/flows/listing.rs index 10effe22..b074b5f6 100644 --- a/rust/devlaunch-core/src/flows/listing.rs +++ b/rust/devlaunch-core/src/flows/listing.rs @@ -63,6 +63,7 @@ use crate::clients::devpod::{self, ContainerState, ListingUnreadable, Workspace, use crate::clients::git::{Git, GitAnswer}; use crate::domain::metadata::MetadataStorage; use crate::domain::model::WorktreeInfo; +use crate::domain::workspace_id; use crate::domain::workspace_state::{self, CloneState, CouldNotTell, NonEmpty, Unsaved}; use crate::flows::disk_usage::{self, DiskUsage}; use crate::runner::Runner; @@ -262,32 +263,94 @@ pub fn owner_of(workspace: &Workspace, cache_dir: &Path) -> Option { WorkspaceSource::GitRepository(url) => { parse_owner_repo_from_url(url).map(|(owner, _repo)| owner.to_owned()) } - WorkspaceSource::LocalFolder(path) if is_devlaunch_clone(workspace, cache_dir) => { - owner_of_clone(Path::new(path), &workspace.id, cache_dir) + // The guards live in `layout_of_clone`, so a local folder that is not one of + // dl's clones answers `None` there rather than here. + WorkspaceSource::LocalFolder(_) => { + layout_of_clone(workspace, cache_dir).map(|(owner, _repo)| owner) } - WorkspaceSource::LocalFolder(_) - | WorkspaceSource::UnreadableLocalFolder(_) - | WorkspaceSource::Unrecognised(_) => None, + WorkspaceSource::UnreadableLocalFolder(_) | WorkspaceSource::Unrecognised(_) => None, } } -/// dl's clone layout read backwards: the grandparent of a leaf named for the -/// workspace it holds. +/// The repo a workspace is a clone of, as dl's own layout names it. +/// +/// Only ever answered for a clone dl made, and for the same reason [`owner_of`] +/// only reads an owner out of one: the name lives in the path +/// `/repos///`, which is the only place devpod's own answer +/// carries it. The same guards apply, because both questions are now answered by +/// the one reading in [`layout_of_clone`] — so a directory merely *shaped* like the +/// layout names no repo. +/// +/// **A git source answers `None` even though its URL names a repo.** A workspace +/// dl did not clone was named by +/// [`source_workspace_id`](crate::domain::workspace_id), which hashes a source with +/// no ref in it at all — so [`ref_slug_of`] has nothing to answer for it either, +/// and a repo on its own would be half a reading. +/// +/// The *directory* and not the id's prefix: the id spells `kinisi_ros` as +/// `kinisi-ros`, because [`slug`](crate::domain::workspace_id) turns `_` into `-` +/// and may cut what is left to fit — and the underscore is the repository's real +/// name. +/// +/// binary surface — not part of the frozen wf API (#251 §7) +pub fn repo_of(workspace: &Workspace, cache_dir: &Path) -> Option { + layout_of_clone(workspace, cache_dir).map(|(_owner, repo)| repo) +} + +/// The ref-slug a workspace's id carries, read against the repo it was derived +/// for. +/// +/// Answered exactly when [`repo_of`] is, minus the ids that do not read apart — +/// which is what makes the pair safe to ask separately: a caller wanting both +/// takes them together and treats one answer without the other as neither. +/// +/// **What comes back is a slug, and a slug is not a ref.** +/// [`ref_slug_of`](crate::domain::workspace_id::ref_slug_of) has the whole of that +/// caveat: `feature/auth` and `feature-auth` read back alike, and a long ref reads +/// back short. It is a label to read, and the id remains the only thing that +/// addresses a workspace. /// -/// That grandparent has to be a directory dl put there, which is what the last -/// check is for — inside the cache and not the cache itself. Without it a -/// directory a user keeps *in* dl's cache, opened by path, hands back whatever -/// component sits two above its leaf: `dl ~/.cache/devlaunch/scratch/myproject` -/// would be credited to an owner called `devlaunch`. -fn owner_of_clone(path: &Path, workspace_id: &str, cache_dir: &Path) -> Option { - if path.file_name()?.to_str()? != workspace_id { +/// binary surface — not part of the frozen wf API (#251 §7) +pub fn ref_slug_of(workspace: &Workspace, cache_dir: &Path) -> Option { + let (_owner, repo) = layout_of_clone(workspace, cache_dir)?; + workspace_id::ref_slug_of(&workspace.id, &repo).map(str::to_owned) +} + +/// dl's clone layout read backwards: the two directories above a leaf named for +/// the workspace it holds, as `(owner, repo)`. +/// +/// The source has to be a local folder dl put under its cache +/// ([`is_devlaunch_clone`]), the leaf has to be named for *this* workspace, and the +/// owner directory has to be one dl put there — inside the cache and not the cache +/// itself. Without that last check a directory a user keeps *in* dl's cache, opened +/// by path, hands back whatever component sits two above its leaf: +/// `dl ~/.cache/devlaunch/scratch/myproject` would be credited to an owner called +/// `devlaunch`. +/// +/// One reading for all three callers rather than one each. Every answer comes out +/// of the same three path components, so the guards cannot be applied to one +/// question and forgotten on the next — the shape of defect the id derivation was +/// rebuilt to make unrepresentable (devlaunch#55). +fn layout_of_clone(workspace: &Workspace, cache_dir: &Path) -> Option<(String, String)> { + let WorkspaceSource::LocalFolder(path) = &workspace.source else { + return None; + }; + if !is_devlaunch_clone(workspace, cache_dir) { return None; } - let owner_dir = path.parent()?.parent()?; + let path = Path::new(path); + if path.file_name()?.to_str()? != workspace.id { + return None; + } + let repo_dir = path.parent()?; + let owner_dir = repo_dir.parent()?; if owner_dir == cache_dir || !owner_dir.starts_with(cache_dir) { return None; } - Some(owner_dir.file_name()?.to_str()?.to_owned()) + Some(( + owner_dir.file_name()?.to_str()?.to_owned(), + repo_dir.file_name()?.to_str()?.to_owned(), + )) } // --------------------------------------------------------------------------- diff --git a/rust/dl/src/select.rs b/rust/dl/src/select.rs index c687d6e6..27e916d1 100644 --- a/rust/dl/src/select.rs +++ b/rust/dl/src/select.rs @@ -13,16 +13,31 @@ //! //! **The columns are not Python's.** `dl.py::fuzzy_select_workspace` drew //! `{id} | {kind} | {detail}`, where the last two came from `describe_source`; this -//! draws `{owner} | {id}`. Both of the columns that went were answering questions -//! nobody standing at this picker is asking: `kind` reads `local` for every -//! workspace dl makes, since dl always hands devpod a path, and `detail` is the -//! clone directory dl chose and manages — a long, mechanically derived path whose -//! own last component is already the id in the column beside it. What is *missing* -//! from an id is the owner: an id is `--` -//! ([`devlaunch_core::domain::workspace_id`]) and carries no owner at all, so a -//! fork and its upstream are two rows spelled the same. [`owner_of`] derives it -//! from the source devpod already reported — no records opened for it — and the -//! column is padded to a common width so the ids line up under each other. +//! draws `{owner} | {repo} | {ref}`. Both of the columns that went were answering +//! questions nobody standing at this picker is asking: `kind` reads `local` for +//! every workspace dl makes, since dl always hands devpod a path, and `detail` is +//! the clone directory dl chose and manages — a long, mechanically derived path +//! whose own last component is already the id. +//! +//! **What replaced the id is the id, read apart.** An id is +//! `--` ([`devlaunch_core::domain::workspace_id`]), +//! and two of those three parts are what a person at this picker is looking for +//! while the third is machinery: the suffix is eight characters of hash, there to +//! keep two branches from sharing a name, and reading it is no part of choosing a +//! workspace. The owner is missing from the id altogether, so a fork and its +//! upstream were two rows spelled the same. All three come out of the source +//! devpod already reported — [`owner_of`], [`repo_of`] and [`ref_slug_of`] read +//! dl's own clone layout, `/repos///`, with no records +//! opened and no config read — and each column but the last is padded to a common +//! width so the rows line up under each other. +//! +//! Two things this deliberately does not do. It does not draw `owner/repo@ref`: +//! that reads as a spec `dl` would accept, and a ref-slug is not a ref — `slug` +//! collapses `/` and `-` alike, so the row for `feature/auth` would invite a +//! retype that addresses `feature-auth` instead. And it never *only* elides: where +//! a split would leave two rows drawn the same, both go back to their whole ids, +//! because the row's own text is what [`chosen`] maps back to a workspace (see +//! [`named`]). //! //! **One deliberate departure from Python's picker: it can take several rows.** //! Python's `iterfzf(..., multi=False)` answered one workspace always. Here the @@ -43,12 +58,13 @@ //! terminal. [`pick`] is the interactive half. use std::borrow::Cow; +use std::collections::HashMap; use std::path::Path; use std::sync::Arc; use devlaunch_core::clients::devpod::Workspace; use devlaunch_core::domain::workspace_state::NonEmpty; -use devlaunch_core::flows::listing::owner_of; +use devlaunch_core::flows::listing::{owner_of, ref_slug_of, repo_of}; use skim::prelude::*; /// One row the picker offers, and the workspace it stands for. @@ -68,40 +84,151 @@ pub(crate) struct Offer { /// reads as a column that failed to draw, where a dash reads as an answer. const NO_OWNER: &str = "-"; +/// One row's naming, before padding turns it into a label. +struct Naming { + owner: String, + tail: Tail, +} + +/// What a row says after its owner. +/// +/// A sum and not a repo beside an optional ref, because the two are not +/// independently absent: dl's own layout names both halves or neither, and a row +/// holding one of them would be a column with the wrong thing under it. +enum Tail { + /// The id read apart into the repo it was derived for and the ref-slug it + /// carries. + Split { repo: String, git_ref: String }, + /// devpod's workspace name, whole — for a workspace dl did not clone, and for + /// one whose split would not have been unique (see [`named`]). + Whole(String), +} + /// Every workspace devpod listed, in devpod's order, as the picker shows it. /// /// No filtering of any kind: the picker is a view of `dl --ls`, so a workspace /// devlaunch did not create and one whose source it cannot read are both offered. /// -/// The owner column is padded to the widest owner *in this list*, which is why the -/// labels are built here in one pass over all of them rather than one workspace at -/// a time: alignment is a fact about the set of rows, not about any one of them. +/// Both columns before the last are padded to the widest entry *in this list*, +/// which is why the labels are built here in one pass over all of them rather than +/// one workspace at a time: alignment is a fact about the set of rows, not about +/// any one of them. The repo column is measured over the rows that have one, so a +/// listing of nothing but foreign workspaces is not padded out around a column it +/// has not got. /// -/// `cache_dir` is where dl keeps its clones, and it is what [`owner_of`] reads a -/// clone's owner out of the layout with — the same directory `--purge` decides -/// ownership by, so the two cannot disagree about which workspaces are dl's. +/// `cache_dir` is where dl keeps its clones, and it is what [`owner_of`], +/// [`repo_of`] and [`ref_slug_of`] read a clone's layout with — the same directory +/// `--purge` decides ownership by, so they cannot disagree about which workspaces +/// are dl's. pub(crate) fn offered(workspaces: &[Workspace], cache_dir: &Path) -> Vec { - let owners: Vec = workspaces - .iter() - .map(|workspace| owner_of(workspace, cache_dir).unwrap_or_else(|| NO_OWNER.to_owned())) - .collect(); - // Characters and not bytes: a non-ASCII owner is one column per character on - // the terminal, and `{:width$}` counts the same way. - let width = owners - .iter() - .map(|owner| owner.chars().count()) - .max() - .unwrap_or(0); + let namings = named(workspaces, cache_dir); + let owner_width = widest(namings.iter().map(|naming| naming.owner.as_str())); + let repo_width = widest(namings.iter().filter_map(|naming| match &naming.tail { + Tail::Split { repo, .. } => Some(repo.as_str()), + Tail::Whole(_) => None, + })); workspaces .iter() - .zip(owners) - .map(|(workspace, owner)| Offer { - label: format!("{owner:width$} | {}", workspace.id), + .zip(namings) + .map(|(workspace, naming)| Offer { + label: label(&naming, owner_width, repo_width), workspace_id: workspace.id.clone(), }) .collect() } +/// How every row wants to be named, with any split that would not have been +/// unique put back together. +/// +/// **The second pass is not cosmetic.** [`chosen`] maps a picked row back to its +/// workspace by the row's own text, first match winning, so two rows drawn the same +/// are a row that selects the other workspace — and `dl rm` is one of the verbs +/// this picker opens for. A ref-slug is a lossy reading of a ref ([`slug`] collapses +/// `/` and `-` alike, and a long ref loses whole segments), so one repo really can +/// hold two branches that read apart identically: `feature/auth` and `feature-auth` +/// are devlaunch#55's own example, and they are two workspaces. +/// +/// Falling back to the whole id is what settles it, because that is the string the +/// ids were given a hashed suffix to make unique in the first place. It also puts +/// the suffix on screen in exactly the case it is doing work, and nowhere else. +/// +/// [`slug`]: devlaunch_core::domain::workspace_id +fn named(workspaces: &[Workspace], cache_dir: &Path) -> Vec { + let mut namings: Vec = workspaces + .iter() + .map(|workspace| Naming { + owner: owner_of(workspace, cache_dir).unwrap_or_else(|| NO_OWNER.to_owned()), + tail: match ( + repo_of(workspace, cache_dir), + ref_slug_of(workspace, cache_dir), + ) { + // Both or neither: the two are one reading of one layout, and a repo + // with no ref beside it would be a column with the wrong thing under + // it. `listing` answers them separately only so that neither has to + // return a pair. + (Some(repo), Some(git_ref)) => Tail::Split { repo, git_ref }, + _ => Tail::Whole(workspace.id.clone()), + }, + }) + .collect(); + let mut drawn: HashMap = HashMap::new(); + for naming in &namings { + *drawn.entry(shared_key(naming)).or_default() += 1; + } + for (naming, workspace) in namings.iter_mut().zip(workspaces) { + if matches!(naming.tail, Tail::Split { .. }) && drawn[&shared_key(naming)] > 1 { + naming.tail = Tail::Whole(workspace.id.clone()); + } + } + namings +} + +/// What two rows drawn the same have in common, with the field boundaries kept. +/// +/// NUL-delimited for the reason [`syllable_suffix`] joins on it: without a +/// delimiter no key could tell the repo `a` with ref `bc` from the repo `ab` with +/// ref `c`, and the padding a label is drawn with is not in the key at all — a +/// collision is between what the rows *say*, not how wide they were printed. +/// +/// [`syllable_suffix`]: devlaunch_core::domain::workspace_id +fn shared_key(naming: &Naming) -> String { + match &naming.tail { + Tail::Split { repo, git_ref } => format!("{}\0{repo}\0{git_ref}", naming.owner), + // An id is unique within one devpod listing, so a whole-name row can never + // share its key — it is counted anyway rather than skipped, so that the one + // pass answers for every row and the fallback below has nothing to special- + // case. + Tail::Whole(name) => format!("{}\0{name}", naming.owner), + } +} + +/// One row, padded into the label skim draws and [`chosen`] reads back. +/// +/// A split row takes three columns and a whole-name row takes two, so the name +/// runs on through the space a ref would have occupied. That is deliberate rather +/// than a column left blank: a foreign workspace has no repo, and a dash under a +/// repo heading would be inventing the same answer twice. +/// +/// The two shapes cannot collide, because a devpod workspace name holds no spaces +/// — so nothing drawn with two separators can equal anything drawn with one. +fn label(naming: &Naming, owner_width: usize, repo_width: usize) -> String { + match &naming.tail { + Tail::Split { repo, git_ref } => { + format!( + "{owner:owner_width$} | {repo:repo_width$} | {git_ref}", + owner = naming.owner + ) + } + Tail::Whole(name) => format!("{owner:owner_width$} | {name}", owner = naming.owner), + } +} + +/// The width of the widest of *texts*, in the characters a terminal draws and +/// `{:width$}` counts — not the bytes a non-ASCII owner or repo would measure. +fn widest<'a>(texts: impl Iterator) -> usize { + texts.map(|text| text.chars().count()).max().unwrap_or(0) +} + /// How many rows one picker run may take. /// /// Decided by the verb the selector was opened for @@ -505,11 +632,14 @@ mod tests { } #[test] - fn a_clone_of_dls_own_is_offered_under_the_owner_its_directory_names() { + fn a_clone_of_dls_own_is_read_apart_into_owner_repo_and_ref() { // The row a user of `dl` actually sees: every workspace `dl owner/repo` // makes is a clone at `/repos///` handed - // to devpod as a path, so the owner is read back out of the layout — no - // records opened, no config read, no disk touched. + // to devpod as a path, so the owner and the repo are read back out of the + // layout and the ref off the id — no records opened, no config read, no disk + // touched. The eight-character suffix does not appear: it is what keeps two + // branches from sharing an id, and choosing a workspace never involves + // reading it. let workspaces = listed(&one( "devlaunch-main-zovomobo", r#"{"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-zovomobo"}"#, @@ -517,7 +647,130 @@ mod tests { assert_eq!( offered(&workspaces, cache())[0].label, - "blooop | devlaunch-main-zovomobo" + "blooop | devlaunch | main" + ); + } + + #[test] + fn a_repo_whose_slug_the_id_cut_is_still_read_apart() { + // The id's repo part is cut to twenty characters when the id would overflow, + // so the prefix in the id is not the repo directory's name. The *directory* + // is what the column shows, because that is the repository's actual name and + // the cut is an artefact of the id's length budget. + let workspaces = listed(&one( + "a-very-long-reposito-main-mafedavi", + r#"{"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/a-very-long-repository-name-indeed/a-very-long-reposito-main-mafedavi"}"#, + )); + + assert_eq!( + offered(&workspaces, cache())[0].label, + "blooop | a-very-long-repository-name-indeed | main" + ); + } + + #[test] + fn two_rows_that_would_be_drawn_alike_go_back_to_their_whole_ids() { + // `feature/auth` and `feature-auth` are two branches, two workspaces and two + // ids — and one ref-slug, because `slug` collapses `/` and `-` alike + // (devlaunch#55, defect #1). Drawn apart they would be the same row twice, + // and `chosen` maps a picked row back by its text with the first match + // winning: marking the second would remove the first. `dl rm` is one of the + // verbs this picker opens for, so that is a workspace deleted for a + // legibility win. + // + // Both rows go back to the whole id — not just the second — because there is + // no first: the collision is between what the two rows say, and neither has a + // better claim on the shorter spelling. + let workspaces = listed( + r#"[ + {"id": "devlaunch-feature-auth-poliseno", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-feature-auth-poliseno"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "devlaunch-feature-auth-nesatabe", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-feature-auth-nesatabe"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"} + ]"#, + ); + + let offers = offered(&workspaces, cache()); + + assert_eq!( + offers.iter().map(|offer| &offer.label).collect::>(), + [ + "blooop | devlaunch-feature-auth-poliseno", + "blooop | devlaunch-feature-auth-nesatabe", + ] + ); + // And the property the fallback exists for: each row still maps back to its + // own workspace. + assert_eq!( + chosen(&offers, vec![offers[1].label.clone()]), + Pick::Chose(one_id("devlaunch-feature-auth-nesatabe")) + ); + } + + #[test] + fn a_collision_only_pulls_in_the_rows_that_collide() { + // The fallback is scoped to the rows drawn alike. A third workspace of the + // same repository keeps its split row, because nothing else is drawn like it + // — so one ambiguous pair does not put the suffix back on a whole listing. + let workspaces = listed( + r#"[ + {"id": "devlaunch-feature-auth-poliseno", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-feature-auth-poliseno"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "devlaunch-feature-auth-nesatabe", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-feature-auth-nesatabe"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "devlaunch-main-zovomobo", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-zovomobo"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"} + ]"#, + ); + + assert_eq!( + offered(&workspaces, cache()) + .iter() + .map(|offer| offer.label.clone()) + .collect::>(), + [ + "blooop | devlaunch-feature-auth-poliseno", + "blooop | devlaunch-feature-auth-nesatabe", + "blooop | devlaunch | main", + ] + ); + } + + #[test] + fn the_same_repo_name_under_two_owners_is_two_rows_that_read_apart() { + // A fork and its upstream: one repository name, one branch, two workspaces. + // The ids differ only in the suffix that is no longer drawn, so the owner + // column is the whole of what tells the rows apart — and it is enough, so + // neither row falls back. + let workspaces = listed( + r#"[ + {"id": "devlaunch-main-zovomobo", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-zovomobo"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "devlaunch-main-dedavevi", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/someone/devlaunch/devlaunch-main-dedavevi"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"} + ]"#, + ); + + assert_eq!( + offered(&workspaces, cache()) + .iter() + .map(|offer| offer.label.clone()) + .collect::>(), + ["blooop | devlaunch | main", "someone | devlaunch | main"] ); } @@ -566,6 +819,46 @@ mod tests { assert_eq!(offered(&workspaces, cache())[0].label, "- | myproject"); } + #[test] + fn the_repo_column_is_padded_over_the_rows_that_have_one() { + // Alignment across the two row shapes. The repo column is measured over the + // split rows only, so a foreign workspace's name does not widen a column it + // has no entry in — it runs on through the space a ref would have taken, + // which is what a row with nothing to put in two columns should do. + // + // `kinisi_ros` also pins the column on the *directory* rather than the id's + // prefix: the id spells it `kinisi-ros`, because `slug` turns `_` into `-`, + // and the underscore is the repository's real name. + let workspaces = listed( + r#"[ + {"id": "devlaunch-main-zovomobo", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-zovomobo"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "kinisi-ros-main-zivefoti", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/kinisi-robotics/kinisi_ros/kinisi-ros-main-zivefoti"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "a-very-long-workspace-name-of-its-own", + "source": {"localFolder": "/home/dev/myproject"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"} + ]"#, + ); + + assert_eq!( + offered(&workspaces, cache()) + .iter() + .map(|offer| offer.label.clone()) + .collect::>(), + [ + "blooop | devlaunch | main", + "kinisi-robotics | kinisi_ros | main", + "- | a-very-long-workspace-name-of-its-own", + ] + ); + } + #[test] fn the_owner_column_is_padded_so_the_ids_line_up() { // Alignment is a fact about the list, not about one row: every owner is From ec5c8821dba9b13be5b94c86ab46dd925ec5da5a Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 19:33:51 +0000 Subject: [PATCH 2/3] fix: a workspace named after another row's columns drew the same row `chosen` finds a picked row by matching its text with the first match winning, so two rows drawn alike act on one workspace. The key pass caught the split-versus-split case; it cannot see the cross-shape one, because a whole-name row keyed `\0` and a split row keyed `\0\0` never share a key even when they draw the same string. A workspace dl did not clone, named `devlaunch | main`, drew `blooop | devlaunch | main` beside a clone of blooop/devlaunch@main -- and picking it removed the clone. The comment claiming the shapes could not collide argued it from the names devpod permits, which is the borrowed guarantee `sanitize_title` declines to rest on next door. `offered` now establishes it locally: draw the labels, and if any two are equal, every remaining split goes back to its id, which leaves one shape and one column that is unique per row. --- rust/dl/src/select.rs | 104 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 12 deletions(-) diff --git a/rust/dl/src/select.rs b/rust/dl/src/select.rs index 27e916d1..6fc040f1 100644 --- a/rust/dl/src/select.rs +++ b/rust/dl/src/select.rs @@ -58,7 +58,7 @@ //! terminal. [`pick`] is the interactive half. use std::borrow::Cow; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::Path; use std::sync::Arc; @@ -88,6 +88,10 @@ const NO_OWNER: &str = "-"; struct Naming { owner: String, tail: Tail, + /// The workspace's own id, kept beside the tail because it is what every + /// fallback falls back *to* — and a fallback that had to go looking for it + /// again could look in the wrong row. + id: String, } /// What a row says after its owner. @@ -121,22 +125,62 @@ enum Tail { /// `--purge` decides ownership by, so they cannot disagree about which workspaces /// are dl's. pub(crate) fn offered(workspaces: &[Workspace], cache_dir: &Path) -> Vec { - let namings = named(workspaces, cache_dir); + let mut namings = named(workspaces, cache_dir); + let mut labels = drawn(&namings); + if !all_distinct(&labels) { + // A collision the key pass could not see, because the two rows that made it + // have different key shapes: a whole-name row drawn ` | ` can + // equal a split row drawn ` | | ` when the name is those + // last two columns. Rare, and not rare enough to argue away — the argument + // would have to be about which names devpod permits, and a row this picker + // can act on is not the place to borrow another program's validation. + // + // Every remaining split goes back to its id, which leaves one shape and a + // last column that is unique per row. Bluntness is affordable here: nothing + // but a name built out of another row's columns ever reaches it. + for naming in &mut namings { + naming.tail = Tail::Whole(naming.id.clone()); + } + labels = drawn(&namings); + } + workspaces + .iter() + .zip(labels) + .map(|(workspace, label)| Offer { + label, + workspace_id: workspace.id.clone(), + }) + .collect() +} + +/// Every row's label, padded against the widest entry in each column. +/// +/// Both columns before the last are padded to the widest entry *in this list*, +/// which is why the labels are drawn from all of them at once rather than one row at +/// a time: alignment is a fact about the set of rows, not about any one of them. The +/// repo column is measured over the rows that have one, so a listing of nothing but +/// foreign workspaces is not padded out around a column it has not got. +fn drawn(namings: &[Naming]) -> Vec { let owner_width = widest(namings.iter().map(|naming| naming.owner.as_str())); let repo_width = widest(namings.iter().filter_map(|naming| match &naming.tail { Tail::Split { repo, .. } => Some(repo.as_str()), Tail::Whole(_) => None, })); - workspaces + namings .iter() - .zip(namings) - .map(|(workspace, naming)| Offer { - label: label(&naming, owner_width, repo_width), - workspace_id: workspace.id.clone(), - }) + .map(|naming| label(naming, owner_width, repo_width)) .collect() } +/// Whether no two of *labels* are the same string. +/// +/// The property [`chosen`] needs and cannot check for itself: it finds a picked row +/// by matching its text, first match winning, so two rows drawn alike are a row that +/// acts on the other one's workspace. +fn all_distinct(labels: &[String]) -> bool { + labels.iter().collect::>().len() == labels.len() +} + /// How every row wants to be named, with any split that would not have been /// unique put back together. /// @@ -169,15 +213,16 @@ fn named(workspaces: &[Workspace], cache_dir: &Path) -> Vec { (Some(repo), Some(git_ref)) => Tail::Split { repo, git_ref }, _ => Tail::Whole(workspace.id.clone()), }, + id: workspace.id.clone(), }) .collect(); let mut drawn: HashMap = HashMap::new(); for naming in &namings { *drawn.entry(shared_key(naming)).or_default() += 1; } - for (naming, workspace) in namings.iter_mut().zip(workspaces) { + for naming in &mut namings { if matches!(naming.tail, Tail::Split { .. }) && drawn[&shared_key(naming)] > 1 { - naming.tail = Tail::Whole(workspace.id.clone()); + naming.tail = Tail::Whole(naming.id.clone()); } } namings @@ -209,8 +254,8 @@ fn shared_key(naming: &Naming) -> String { /// than a column left blank: a foreign workspace has no repo, and a dash under a /// repo heading would be inventing the same answer twice. /// -/// The two shapes cannot collide, because a devpod workspace name holds no spaces -/// — so nothing drawn with two separators can equal anything drawn with one. +/// Nothing here keeps two rows apart. Whether the labels are distinct is a fact +/// about the whole list, and [`offered`] is where it is established. fn label(naming: &Naming, owner_width: usize, repo_width: usize) -> String { match &naming.tail { Tail::Split { repo, git_ref } => { @@ -711,6 +756,41 @@ mod tests { ); } + #[test] + fn a_split_row_cannot_be_drawn_the_same_as_a_whole_name_row() { + // The cross-shape collision the two-column and three-column rows can make + // between them: a workspace dl did not clone keeps whatever name devpod has + // for it, and if that name is the middle and right columns of some *other* + // row, the two rows are the same text. `chosen` then maps both to whichever + // came first, so picking the second one acts on the first -- and `dl rm` is + // one of the verbs this picker opens for. + // + // The collision key cannot see this one: the two rows have different key + // shapes, so counting keys finds no duplicate. + let workspaces = listed( + r#"[ + {"id": "devlaunch-main-zovomobo", + "source": {"localFolder": "/home/dev/.cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-zovomobo"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"}, + {"id": "devlaunch | main", + "source": {"gitRepository": "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/blooop/devlaunch.git"}, + "lastUsed": "x", "provider": {"name": "docker"}, + "ide": {"name": "none"}, "context": "default"} + ]"#, + ); + + let offers = offered(&workspaces, cache()); + + let labels: Vec<&String> = offers.iter().map(|offer| &offer.label).collect(); + assert_ne!(labels[0], labels[1], "two rows drawn alike: {labels:?}"); + // And the property that matters: each row still reaches its own workspace. + assert_eq!( + chosen(&offers, vec![offers[1].label.clone()]), + Pick::Chose(one_id("devlaunch | main")) + ); + } + #[test] fn a_collision_only_pulls_in_the_rows_that_collide() { // The fallback is scoped to the rows drawn alike. A third workspace of the From 10d3bd6099fd4022cf8ec196a1a27833d7e19827 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 19:39:23 +0000 Subject: [PATCH 3/3] fix: an id two repo spellings explain read back as the wrong branch `value` cuts the repo slug to twenty characters only when the id would otherwise overflow, so a reader has two spellings to try. Trying the full one first is right far more often, and it is not always right: for repo `aaaaaaaaaaaaaaaaaaaa-bbbb` at branch `bbbb-cccccccccc` the id used the cut spelling, and the full spelling also explains the string -- so the ref column read `cccccccccc`, a branch of that name being exactly what the row then claimed to be. Both readings really are derivable there, so there is no right answer to pick: it needs a repo slug over the cap with a dash at exactly the cap and a ref beginning with the segment after it. So an id both spellings explain, disagreeing, is refused, and the caller draws the id whole -- which is what `None` already meant. Conservative in one direction: some ids for such a repo are only derivable one way and are refused anyway. Recovering those means re-deriving the cut rule from a ref that has already been fitted, kept in step with `value` forever, for a prettier column on repositories named like that. A repo slug with no dash at the cap is read apart under exactly one spelling however long it is, which the test pins. --- .../devlaunch-core/src/domain/workspace_id.rs | 81 ++++++++++++++++--- rust/dl/src/select.rs | 9 ++- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/rust/devlaunch-core/src/domain/workspace_id.rs b/rust/devlaunch-core/src/domain/workspace_id.rs index f1a4e665..7dbba7fa 100644 --- a/rust/devlaunch-core/src/domain/workspace_id.rs +++ b/rust/devlaunch-core/src/domain/workspace_id.rs @@ -389,9 +389,10 @@ fn fit_ref(git_ref: &str, room: usize) -> String { /// /// `None` for anything that does not read as one, and every arm of that is a /// workspace a caller should show whole instead: an id with no syllable suffix on -/// it, a repo whose slug is not the prefix under either spelling, or nothing left -/// between the two. The suffix check is what makes this answer `None` for a name -/// dl did not derive rather than cutting eight characters off the end of it. +/// it, a repo whose slug is not the prefix under either spelling, nothing left +/// between the two, or an id that *both* spellings explain and disagree about. The +/// suffix check is what makes this answer `None` for a name dl did not derive rather +/// than cutting eight characters off the end of it. /// /// **What comes back is a slug, and a slug is not a ref.** [`slug`] collapses `/` /// and `-` alike, and [`fit_ref`] drops whole segments before it truncates @@ -407,13 +408,20 @@ pub(crate) fn ref_slug_of<'a>(id: &'a str, repo: &str) -> Option<&'a str> { // slug is empty carries no repo part and no separator for one. return non_empty(body); } - // The full spelling first and the cut one second. `value` cuts the repo slug - // to REPO_SLUG_LENGTH only when the id would otherwise overflow, so both are - // live — and trying the cut one first would eat the front of the ref for - // every repo whose slug is inside the cap. + // Both spellings `value` can have used, because it cuts the repo slug to + // REPO_SLUG_LENGTH only when the id would otherwise overflow. let cut = head(&repo_slug, REPO_SLUG_LENGTH).trim_matches('-'); - let rest = after_part(body, &repo_slug).or_else(|| after_part(body, cut))?; - non_empty(rest) + match (after_part(body, &repo_slug), after_part(body, cut)) { + // Both explain the id and they disagree about where the boundary is, so + // nothing here knows which spelling produced it: it takes a repo slug over + // the cap with a dash at exactly the cap, and a ref beginning with the + // segment after it. Answering one of them is how a row shows a branch that + // is not the branch, so it answers neither and the caller draws the id + // whole. + (Some(under), Some(over)) if under != over => None, + (Some(rest), _) | (None, Some(rest)) => non_empty(rest), + (None, None) => None, + } } /// *body* with `-` taken off the front, or `None` if it does not start that @@ -1758,4 +1766,59 @@ mod tests { assert_eq!(id, "main-gakebofi"); assert_eq!(ref_slug_of(&id, "_"), Some("main")); } + + #[test] + fn an_id_two_repo_spellings_both_explain_is_refused_rather_than_guessed() { + // A repo slug over the cap with a dash at exactly the cap, and a branch + // starting with the segment after it. Both readings derive this very id: + // + // repo `…-bbbb` untruncated, ref slug `cccccccccc` + // repo cut to the twenty a's, ref slug `bbbb-cccccccccc` <- the real one + // + // Nothing in the id says which, because the cut is applied on a length the + // ref has already been fitted to and neither reading overruns it. Reading it + // one way and answering confidently is how a row shows a branch that is not + // the branch, so it answers `None` and the caller draws the id whole. + let repo = "aaaaaaaaaaaaaaaaaaaa-bbbb"; + let id = derived("o", repo, "bbbb-cccccccccc"); + + assert_eq!(id, "aaaaaaaaaaaaaaaaaaaa-bbbb-cccccccccc-vekozazi"); + assert!(slug(repo).len() > REPO_SLUG_LENGTH, "the cap has to bite"); + assert_eq!(ref_slug_of(&id, repo), None); + } + + #[test] + fn only_a_repo_slug_with_a_dash_at_the_cap_can_be_read_two_ways() { + // The refusal above is conservative, and this is the whole of what it costs. + // Two spellings can only both match when the cut lands on a `-`: the cut + // reading needs a `-` at the cap in the *body*, and the full reading needs + // the same position in the *slug*, so a repo slug without one there is read + // apart under exactly one spelling however long it is. + // + // For this repo that means some ids a cleverer reader could resolve are + // refused too — `main` below is only derivable under the full spelling, + // since the cut one would not have overflowed. Recovering it means + // re-deriving the cut rule from a ref that has already been fitted, which is + // arithmetic this module would have to keep in step with `value` forever, for + // a prettier column on repositories named like this one. + let dashed = "aaaaaaaaaaaaaaaaaaaa-bbbb"; + assert_eq!(ref_slug_of(&derived("o", dashed, "main"), dashed), None); + + // Only one spelling matches here, so it is answered: `bbbb` is not the front + // of this ref, so there is nothing for the full spelling to strip. + let cut = derived("o", dashed, "release/9999999999999999999999999176"); + assert_eq!(cut, "aaaaaaaaaaaaaaaaaaaa-release-999999999-dobakero"); + assert_eq!(ref_slug_of(&cut, dashed), Some("release-999999999")); + + // And a repo slug just as far over the cap with no dash at it is unaffected, + // which is every long repository name that is not this shape. + let plain = "aaaaaaaaaaaaaaaaaaaaabbbb"; + assert!(slug(plain).len() > REPO_SLUG_LENGTH, "the cap has to bite"); + assert_eq!( + ref_slug_of(&derived("o", plain, "main"), plain), + Some("main") + ); + let plain_cut = derived("o", plain, "release/9999999999999999999999999176"); + assert_eq!(ref_slug_of(&plain_cut, plain), Some("release-999999999")); + } } diff --git a/rust/dl/src/select.rs b/rust/dl/src/select.rs index 6fc040f1..e2823571 100644 --- a/rust/dl/src/select.rs +++ b/rust/dl/src/select.rs @@ -135,9 +135,12 @@ pub(crate) fn offered(workspaces: &[Workspace], cache_dir: &Path) -> Vec // would have to be about which names devpod permits, and a row this picker // can act on is not the place to borrow another program's validation. // - // Every remaining split goes back to its id, which leaves one shape and a - // last column that is unique per row. Bluntness is affordable here: nothing - // but a name built out of another row's columns ever reaches it. + // Every remaining split goes back to its id, which is what this can offer + // and not a promise of distinctness: two workspaces of one id in two devpod + // contexts draw one row whatever is done here, because an id is unique per + // context and nothing in an `Offer` carries the context. That predates the + // columns — ` | ` collided the same way — and closing it means + // addressing a workspace by more than its id. for naming in &mut namings { naming.tail = Tail::Whole(naming.id.clone()); }