diff --git a/README.md b/README.md index 9ec50d47..bfd0edd2 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,36 @@ 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`. +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. + 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 selector says so on its own screen: the diff --git a/rust/devlaunch-core/public-api.txt b/rust/devlaunch-core/public-api.txt index b9154a94..85827fec 100644 --- a/rust/devlaunch-core/public-api.txt +++ b/rust/devlaunch-core/public-api.txt @@ -1778,6 +1778,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..7dbba7fa 100644 --- a/rust/devlaunch-core/src/domain/workspace_id.rs +++ b/rust/devlaunch-core/src/domain/workspace_id.rs @@ -372,6 +372,100 @@ 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, 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 +/// 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); + } + // 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('-'); + 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 +/// 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 +1632,193 @@ 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")); + } + + #[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/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 b4673284..1acf6687 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 @@ -51,12 +66,13 @@ //! terminal. [`pick`] is the interactive half. use std::borrow::Cow; +use std::collections::{HashMap, HashSet}; 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. @@ -76,40 +92,199 @@ 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, + /// 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. +/// +/// 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 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 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()); + } + labels = drawn(&namings); + } workspaces .iter() - .zip(owners) - .map(|(workspace, owner)| Offer { - label: format!("{owner:width$} | {}", workspace.id), + .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, + })); + namings + .iter() + .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. +/// +/// **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()), + }, + 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 in &mut namings { + if matches!(naming.tail, Tail::Split { .. }) && drawn[&shared_key(naming)] > 1 { + naming.tail = Tail::Whole(naming.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. +/// +/// 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 } => { + 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 @@ -566,11 +741,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"}"#, @@ -578,7 +756,165 @@ 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_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": "https://github.com/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 + // 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"] ); } @@ -627,6 +963,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