From 23701e3639bc78c0749bd85345da1d674595157d Mon Sep 17 00:00:00 2001 From: Joshua Smith Date: Wed, 16 Sep 2026 11:20:27 +0100 Subject: [PATCH] feat: `--from ` cuts a new branch from a base you name `dl owner/repo@fix/123` creates the branch if it is not there, and what it cut from was settled with no way to ask for anything else: the default branch's remote ref, freshly fetched. `--from` replaces the answer `WorkspaceCloneManager::ensure_branch` gives and nothing else. Carried forward onto upstream's v0.49.0 rework of that method (312005c, 1a8dc37, 8da1a86) rather than replayed from the old fork's diff: upstream's current `ensure_branch` already matches the shape the old patch assumed, so the seam it hooks into (a new `ensure_branch_from` arm, taken before the no-flag fetch/default-branch dance) needed no rediscovery. `prepare_cold_from` threads `from` through the lock the same way `prepare_cold` always has, and `prepare_cold` itself becomes a `None` call of it so the module's forty-odd existing test call sites stay untouched. An unresolvable base is a refusal naming the base, and creates no branch: falling back to the default would be the same "invented base" failure the no-flag path already forbids, arriving through a new door. A branch that already exists is a refusal that names the branch, not the base -- existence is decided before the base is resolved, so ignoring the flag there would look like it worked while the workspace opens on whatever the branch already is. Like `--claude-profile` and unlike `--devcontainer`: a base describes an event that happened once, and a workspace that re-applied one on every later launch would be claiming to re-cut a branch it did not. `Launch::from_ref` rides one call and is never read back off a record. `aid` gained no `--from` of its own, but `DL_VALUE_OPTIONS` had to learn that dl's `--from` takes a value, or `aid --from develop owner/repo fix it` reads "develop" as the prompt's first word. `completions/dl.bash` carries the same fact in three tables, each pinned by `completion_tables`. `render_select` picks up the same `#[allow(clippy::too_many_arguments)]` its sibling `render_workspace` already carries, for the trio of independent launch modifiers. README's `--from` paragraph is restored (it was pulled earlier for describing a flag the binary did not yet have), and the public-api snapshots carry the four new additive rows (`Launch::from_ref` twice, `EnsureBranchError`'s two new variants), inserted by anchoring on content rather than copying a full regeneration -- a local nightly renders unrelated toolchain noise (`core::io::error::Error` vs `std::io::error::Error`, blanket-impl reordering) that has nothing to do with this change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AdSFnBdxie6TosHVmjLY28 --- README.md | 12 + rust/aid/src/rewrite.rs | 8 +- rust/devlaunch-core/completions/dl.bash | 10 +- rust/devlaunch-core/public-api.api.txt | 2 + rust/devlaunch-core/public-api.rest.txt | 4 + rust/devlaunch-core/src/flows/launch.rs | 31 +- .../src/flows/workspace_clone.rs | 345 +++++++++++++++++- rust/dl/src/cli.rs | 90 ++++- rust/dl/src/commands.rs | 28 ++ rust/dl/src/launch.rs | 4 +- rust/dl/src/lib.rs | 3 + rust/dl/src/render.rs | 8 + 12 files changed, 531 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 359743d9..1812bb6e 100644 --- a/README.md +++ b/README.md @@ -372,6 +372,18 @@ credential stops the launch and says so rather than falling back to your default the whole point of naming one. [docs/workspace-tools.md](docs/workspace-tools.md) has the precedence order and what a profile does not change. +`--from ` cuts a new branch from that ref instead of from the repository's default branch: + +```bash +dl blooop/devlaunch@fix/123 --from develop +``` + +It only means something when the branch does not exist yet. Given a branch that is already there, +`dl` refuses rather than ignoring the flag, since silently opening the existing branch would look +like the base had been honoured. Like `--claude-profile`, and unlike `--devcontainer`, it is per +launch and is **not** stored with the workspace: a base names a one-time event, not something +every later launch should redo. + `dl --help` is the complete reference and is kept in step with the binary by a test. ## aid: an agent instead of a shell diff --git a/rust/aid/src/rewrite.rs b/rust/aid/src/rewrite.rs index 620aacbe..297c1a5e 100644 --- a/rust/aid/src/rewrite.rs +++ b/rust/aid/src/rewrite.rs @@ -329,7 +329,13 @@ const REMOTE_CONTROL_NO: &[&str] = &["0", "false", "off", "no"]; /// `aid --claude-profile work` still lists its session under whichever account the /// container is signed in to. Two credentials, and this one is the token forwarded /// into the session. -const DL_VALUE_OPTIONS: &[&str] = &["--devcontainer", "--claude-profile"]; +/// +/// **`--from` is listed for the same reason and no other: `aid` has no `--from` +/// of its own** (that is a separate change), but a line typed as `aid --from +/// develop owner/repo fix it` still has to read "develop" as the base rather +/// than as the first word of the prompt, so dl's own grammar is recognised here +/// exactly as `--claude-profile`'s is. +const DL_VALUE_OPTIONS: &[&str] = &["--devcontainer", "--claude-profile", "--from"]; /// The modifier the suffix options take, peeled only in their company. /// diff --git a/rust/devlaunch-core/completions/dl.bash b/rust/devlaunch-core/completions/dl.bash index 49516065..5e0bc8a9 100644 --- a/rust/devlaunch-core/completions/dl.bash +++ b/rust/devlaunch-core/completions/dl.bash @@ -69,9 +69,9 @@ _dl_completion() { # The retired spellings (--stop, --autorm) are absent by rule rather than by # hand: the grammar marks them `hide = true`, and the test drops every hidden # flag, so a spelling this build only still answers for is never offered. - local global_opts="--ls --install --refresh --prune --reconcile --purge --herdr-shell --herdr-setup --herdr-env --herdr-workspace --rm --devcontainer --claude-profile --claude-profiles --help -h --version" + local global_opts="--ls --install --refresh --prune --reconcile --purge --herdr-shell --herdr-setup --herdr-env --herdr-workspace --rm --devcontainer --claude-profile --from --claude-profiles --help -h --version" if [[ "$cmd" == aid ]]; then - global_opts="--claude --codex --gemini --model --effort --devcontainer --claude-profile --help -h --version" + global_opts="--claude --codex --gemini --model --effort --devcontainer --claude-profile --from --help -h --version" fi # Workspace subcommands @@ -82,7 +82,7 @@ _dl_completion() { local ws_cmds="up stop kill rm rme code restart recreate reset dotfiles --rm --" # Options that take a value; a variant name, a profile name or a path follows. - local value_opts="--devcontainer --claude-profile --herdr-workspace" + local value_opts="--devcontainer --claude-profile --herdr-workspace --from" # aid's own value-taking flags, from `AGENT_VALUE_OPTIONS`. Nothing completes # their values: the models and efforts are each agent's to list, they change @@ -115,7 +115,7 @@ _dl_completion() { # would mean a second exception rather than a wider `spec_follows` -- the # thing that follows is not a spec, and the branch below that handles `./` # is inside the spec position. - local spec_follows="--rm --devcontainer --claude-profile" + local spec_follows="--rm --devcontainer --claude-profile --from" if [[ "$cmd" == aid ]]; then # aid's own, from `parse_aid_args`: it reads an agent flag, a remote # control flag or a dl value option and keeps looking for the spec. The @@ -124,7 +124,7 @@ _dl_completion() { # takes a value, so on `aid --unknown-taking-a-value foo owner/repo` it # calls `foo` the spec, and completing a slot aid itself cannot place is # worse than completing nothing. - spec_follows="--claude --codex --gemini --remote-control --remote --no-remote-control --no-remote --model --effort --devcontainer --claude-profile" + spec_follows="--claude --codex --gemini --remote-control --remote --no-remote-control --no-remote --model --effort --devcontainer --claude-profile --from" fi if [[ "${prev}" == "--herdr-workspace" ]]; then diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index bb4a77bf..206773f7 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -396,6 +396,7 @@ pub fn devlaunch_core::flows::kept_copies::KeptCopies::fmt(&self, &mut core::fmt impl core::marker::StructuralPartialEq for devlaunch_core::flows::kept_copies::KeptCopies pub struct devlaunch_core::api::Launch<'a, 'r, 'l> impl<'a, 'r, 'l> devlaunch_core::flows::launch::Launch<'a, 'r, 'l> +pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::from_ref(self, core::option::Option) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::new(&'a mut devlaunch_core::flows::listing::CommandContext<'r>, &'a mut devlaunch_core::flows::lifecycle::Refresh<'l>, &'a mut dyn devlaunch_core::flows::launch::ColdMachinery<'r>, &'a dyn devlaunch_core::flows::launch::Provision, &'a devlaunch_core::flows::launch::Host, &'a mut dyn core::ops::function::FnMut(&str), &'a mut dyn devlaunch_core::notices::Notices) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::recognised_as(self, core::option::Option) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::run(&mut self, &str, &devlaunch_core::flows::launch::LaunchVerb, core::option::Option<&devlaunch_core::domain::spec::DevcontainerPath>) -> core::result::Result @@ -719,6 +720,7 @@ pub fn devlaunch_core::flows::launch::Host::fmt(&self, &mut core::fmt::Formatter impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::Host pub struct devlaunch_core::flows::launch::Launch<'a, 'r, 'l> impl<'a, 'r, 'l> devlaunch_core::flows::launch::Launch<'a, 'r, 'l> +pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::from_ref(self, core::option::Option) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::new(&'a mut devlaunch_core::flows::listing::CommandContext<'r>, &'a mut devlaunch_core::flows::lifecycle::Refresh<'l>, &'a mut dyn devlaunch_core::flows::launch::ColdMachinery<'r>, &'a dyn devlaunch_core::flows::launch::Provision, &'a devlaunch_core::flows::launch::Host, &'a mut dyn core::ops::function::FnMut(&str), &'a mut dyn devlaunch_core::notices::Notices) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::recognised_as(self, core::option::Option) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::run(&mut self, &str, &devlaunch_core::flows::launch::LaunchVerb, core::option::Option<&devlaunch_core::domain::spec::DevcontainerPath>) -> core::result::Result diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 99d4e061..18ec8c10 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -3177,7 +3177,11 @@ impl core::marker::StructuralPartialEq for devlaunch_core::flows::session_manage pub fn devlaunch_core::flows::session_manager::pane_destination(&dyn devlaunch_runner::Runner) -> devlaunch_core::flows::session_manager::PaneDestination pub mod devlaunch_core::flows::workspace_clone pub enum devlaunch_core::flows::workspace_clone::EnsureBranchError +pub devlaunch_core::flows::workspace_clone::EnsureBranchError::BaseNotResolved +pub devlaunch_core::flows::workspace_clone::EnsureBranchError::BaseNotResolved::base: alloc::string::String pub devlaunch_core::flows::workspace_clone::EnsureBranchError::Branch(devlaunch_core::flows::branch_manager::BranchError) +pub devlaunch_core::flows::workspace_clone::EnsureBranchError::BranchAlreadyExists +pub devlaunch_core::flows::workspace_clone::EnsureBranchError::BranchAlreadyExists::branch: alloc::string::String pub devlaunch_core::flows::workspace_clone::EnsureBranchError::WrongRepoLock(devlaunch_core::flows::repo_manager::WrongRepoLock) impl core::clone::Clone for devlaunch_core::flows::workspace_clone::EnsureBranchError pub fn devlaunch_core::flows::workspace_clone::EnsureBranchError::clone(&self) -> devlaunch_core::flows::workspace_clone::EnsureBranchError diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 0d124cb1..f46eb95a 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -4473,14 +4473,16 @@ pub(crate) fn prepare( cold: &mut dyn ColdMachinery<'_>, workspace: &WorkspaceId, remote_url: &str, + from: Option<&str>, notices: &mut dyn Notices, ) -> Result { let opened = cold.open().map_err(NotPrepared::Cold)?; - let prepared = opened.clones.prepare_cold( + let prepared = opened.clones.prepare_cold_from( opened.storage, workspace.owner(), workspace.repo(), workspace.git_ref(), + from, remote_url, &mut as_cache(notices), ); @@ -4749,6 +4751,8 @@ pub struct Launch<'a, 'r, 'l> { /// What the caller already knows this workspace is, for a launch that names it /// by id. See [`Self::recognised_as`]. recognised: Option, + /// `--from `, for this launch and no other. See [`Self::from_ref`]. + from: Option, } impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { @@ -4772,6 +4776,7 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { claude_seen: ClaudeSeen::new(), notices, recognised: None, + from: None, } } @@ -4805,6 +4810,21 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { self } + /// `--from `: cut a new branch from `base` instead of the default + /// branch. + /// + /// Per launch, like [`Host::with_claude_profile`] and unlike + /// `--devcontainer`: a base describes an event that happened once, not what + /// the workspace *is*, so it rides this one call and is never read back off + /// a record. Reaches only [`Launch::place_triple`]'s cold arm -- a launch + /// that resolves warm, or a bare workspace name, has no branch left to cut + /// and this is not consulted. + #[must_use] + pub fn from_ref(mut self, base: Option) -> Self { + self.from = base; + self + } + /// Run one launch. pub fn run( &mut self, @@ -4938,7 +4958,13 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { Ok(Ok(placement)) } Resolution::Cold { workspace } => { - match prepare(self.cold, &workspace, &remote_url, &mut *self.notices) { + match prepare( + self.cold, + &workspace, + &remote_url, + self.from.as_deref(), + &mut *self.notices, + ) { Ok(placement) => Ok(Ok(placement)), Err(error) => Ok(Err(LaunchRefusal::NotPrepared { owner, @@ -10911,6 +10937,7 @@ mod tests { &mut MetadataWillNotOpen, &workspace, "git@github.com:blooop/devlaunch.git", + None, &mut no_notices(), ); diff --git a/rust/devlaunch-core/src/flows/workspace_clone.rs b/rust/devlaunch-core/src/flows/workspace_clone.rs index 9933dff6..e3283dc9 100644 --- a/rust/devlaunch-core/src/flows/workspace_clone.rs +++ b/rust/devlaunch-core/src/flows/workspace_clone.rs @@ -188,6 +188,20 @@ pub enum EnsureBranchError { /// The branch could not be created, which is where an empty cache is /// discovered: it is the first step that actually consults it. Branch(BranchError), + /// `--from ` named a branch that already exists. The flag only means + /// something when a branch is being cut, and ignoring it here would look + /// like it worked while the workspace opens on whatever the branch already + /// is — so this names the branch rather than the base. + BranchAlreadyExists { + branch: String, + }, + /// `--from ` named a base this launch could not resolve as a + /// freshly-fetched remote ref. Refused rather than substituting the default + /// branch, which would be the "invented base" failure the no-flag path is + /// already guarded against, arriving through a new door. + BaseNotResolved { + base: String, + }, } /// Why a workspace clone could not be prepared. @@ -534,6 +548,13 @@ impl<'r> WorkspaceCloneManager<'r> { /// through preparing: `dl --prune` weighing or removing a clone still being /// filled, or two launches of different branches of one repository interleaving /// their steps. Atomicity and legibility, not speed. + /// + /// The bare form: no `--from`. Kept as its own call so this module's forty-odd + /// existing call sites are untouched by `--from`'s plumbing rather than each + /// growing a `None`; production now always goes through + /// [`WorkspaceCloneManager::prepare_cold_from`], so only this module's own + /// tests reach this one. + #[cfg_attr(not(test), allow(dead_code))] pub(crate) fn prepare_cold( &self, storage: &mut MetadataStorage, @@ -542,6 +563,27 @@ impl<'r> WorkspaceCloneManager<'r> { branch: &str, remote_url: &str, notices: &mut dyn Notices, + ) -> Result { + self.prepare_cold_from(storage, owner, repo, branch, None, remote_url, notices) + } + + /// [`WorkspaceCloneManager::prepare_cold`], with `--from ` threaded to + /// the one place that decides what a new branch is cut from. + /// + /// Split out rather than adding the parameter to `prepare_cold` itself so + /// every existing call of it — every one of them a `None` — is untouched + /// rather than a `None` added at each: the no-flag path is provably the same + /// call it always was. + #[allow(clippy::too_many_arguments)] + pub(crate) fn prepare_cold_from( + &self, + storage: &mut MetadataStorage, + owner: &str, + repo: &str, + branch: &str, + from: Option<&str>, + remote_url: &str, + notices: &mut dyn Notices, ) -> Result { // Derived here rather than passed in, and derived before the lock: it is // the parse boundary for the triple, and an unsafe ref should be refused @@ -550,7 +592,7 @@ impl<'r> WorkspaceCloneManager<'r> { WorkspaceId::new(owner, repo, branch).map_err(PrepareColdError::UnsafeTriple)?; let mut stage = timing::stage(timing::Stage::HostPrep); - let prepared = self.prepare_cold_under_lock(storage, &workspace, remote_url, notices); + let prepared = self.prepare_cold_under_lock(storage, &workspace, from, remote_url, notices); if prepared.is_err() { stage.fail(); } @@ -566,6 +608,7 @@ impl<'r> WorkspaceCloneManager<'r> { &self, storage: &mut MetadataStorage, workspace: &WorkspaceId, + from: Option<&str>, remote_url: &str, notices: &mut dyn Notices, ) -> Result { @@ -579,7 +622,7 @@ impl<'r> WorkspaceCloneManager<'r> { .clone_if_missing(&lock, storage, owner, repo, remote_url, notices)?; let base = self - .ensure_branch(&lock, storage, owner, repo, branch, notices) + .ensure_branch(&lock, storage, owner, repo, branch, from, notices) .map_err(PrepareColdError::Branch)?; if let BranchBase::Stale { base: from, reason } = &base { // The one consequence-stating notice for the whole degraded family: @@ -630,6 +673,7 @@ impl<'r> WorkspaceCloneManager<'r> { /// Takes a [`RepoLock`] rather than acquiring one: the fetch and the branch /// creation both write refs in the shared bare repository, and two processes /// doing so at once trip over git's own ref locks. + #[allow(clippy::too_many_arguments)] pub(crate) fn ensure_branch( &self, lock: &RepoLock, @@ -637,12 +681,21 @@ impl<'r> WorkspaceCloneManager<'r> { owner: &str, repo: &str, branch: &str, + from: Option<&str>, notices: &mut dyn Notices, ) -> Result { lock.require(owner, repo) .map_err(EnsureBranchError::WrongRepoLock)?; let bare = self.repo_manager.bare_dir(owner, repo); + // `--from` replaces the default branch as the new branch's start point + // and nothing else, so it is its own arm rather than a third case folded + // into the match below: every line under it is the sequence of git calls + // this method has always made when `from` is `None`, untouched. + if let Some(from_ref) = from { + return self.ensure_branch_from(&bare, owner, repo, branch, from_ref, notices); + } + let outcome = self.repo_manager.fetch_ref(owner, repo, branch, notices); let default = self.resolve_default_branch(storage, owner, repo, notices); @@ -710,6 +763,87 @@ impl<'r> WorkspaceCloneManager<'r> { Ok(base) } + /// [`WorkspaceCloneManager::ensure_branch`]'s `--from ` arm. + /// + /// Two refusals guard this, in order, and both are refusals rather than a + /// quiet fallback to the default branch: doing that would be the "invented + /// base" failure [`EnsureBranchError::Branch`]'s sibling test already forbids + /// for the no-flag path, arriving through a new door. + /// + /// **The branch already exists.** Checked before `from_ref` is resolved at + /// all, because ignoring `--from` here would look like it worked while the + /// workspace opens on whatever the branch already is — the operator is owed + /// "that branch exists", not a base that quietly did nothing. Read off a + /// fresh fetch of `branch` when the remote can be asked, and off the cache's + /// own refs when it cannot: an offline launch has no remote answer, and what + /// the cache already holds locally is the next best fact rather than a guess. + /// + /// **`from_ref` cannot be resolved.** Only once `branch` is confirmed new: + /// `from_ref` is fetched exactly as the default branch would have been, and + /// anything short of [`FetchOutcome::Updated`] means this launch cannot prove + /// the base is current, so it is named in the refusal rather than + /// substituted for. + fn ensure_branch_from( + &self, + bare: &Path, + owner: &str, + repo: &str, + branch: &str, + from_ref: &str, + notices: &mut dyn Notices, + ) -> Result { + let branch_outcome = self.repo_manager.fetch_ref(owner, repo, branch, notices); + let already_exists = match &branch_outcome { + Ok(FetchOutcome::Updated) => true, + Ok(FetchOutcome::RefMissingOnRemote) => false, + // The remote could not be asked, so the only fact left to decide + // this from is whatever the cache already holds locally — the same + // fact the no-`--from` path reads for this exact bare cache when a + // fetch fails. + Ok(FetchOutcome::Failed { .. }) | Err(_) => { + self.branch_manager.local_branch_exists(bare, branch) + } + }; + if already_exists { + return Err(EnsureBranchError::BranchAlreadyExists { + branch: branch.to_owned(), + }); + } + + let base = match self.repo_manager.fetch_ref(owner, repo, from_ref, notices) { + Ok(FetchOutcome::Updated) => BranchBase::Fresh, + Ok(FetchOutcome::RefMissingOnRemote) => { + return Err(EnsureBranchError::BaseNotResolved { + base: from_ref.to_owned(), + }); + } + Ok(FetchOutcome::Failed { reason }) => { + notices.say(CacheNotice::RefNotFetched { + owner: owner.to_owned(), + repo: repo.to_owned(), + branch: from_ref.to_owned(), + reason: NotRefreshed::FetchFailed { reason }, + }); + return Err(EnsureBranchError::BaseNotResolved { + base: from_ref.to_owned(), + }); + } + Err(_unsafe_name) => { + return Err(EnsureBranchError::BaseNotResolved { + base: from_ref.to_owned(), + }); + } + }; + + let request = EnsureBranch::in_cache(bare, branch, from_ref); + let ensured = self + .branch_manager + .ensure_branch_exists(request) + .map_err(EnsureBranchError::Branch)?; + say_branch(&ensured, request.branch, request.remote, notices); + Ok(base) + } + /// The repository's default branch, or why it could not be named. /// /// An empty answer is folded into [`DefaultBranch::Unknown`] here rather than @@ -1982,6 +2116,177 @@ mod tests { } } + // ============================================================ --from + + #[test] + fn a_new_branch_with_from_is_cut_from_that_base_freshly_fetched_not_the_default() { + // Two guards in one test, because they are checked against the same two + // git calls: the base is fetched (not assumed), and the branch is created + // from it (not from the default). Scripted so both are false unless the + // implementation does both: an unscripted `show-ref` answers "there" by + // default, which would skip the branch creation and hide either bug. + let mut cache = a_cache(); + given_cached_repo(&mut cache); + let fake = FakeGit::new() + .with_script( + [ + "git", + "fetch", + "origin", + "+refs/heads/newbranch:refs/heads/newbranch", + ], + Response::failed( + 128, + "fatal: couldn't find remote ref refs/heads/newbranch\n", + ), + ) + .with_script( + ["git", "show-ref", "--verify", "refs/heads/newbranch"], + Response::exited(1), + ); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + + let (base, _) = ensure_branch_from_with(&manager, &mut cache, "newbranch", "develop"); + + assert_eq!(base.expect("ensured"), BranchBase::Fresh); + let argvs = fake.argvs(); + let issued = as_strs(&argvs); + assert!( + issued.contains(&vec![ + "git", + "fetch", + "origin", + "+refs/heads/develop:refs/heads/develop" + ]), + "the base was not fetched fresh: {issued:?}" + ); + assert!( + issued.contains(&vec!["git", "branch", "newbranch", "develop"]), + "the new branch was not cut from --from's base: {issued:?}" + ); + assert!( + !issued.iter().any(|argv| argv.contains(&"main")), + "the default branch was consulted even though --from named a base: {issued:?}" + ); + } + + #[test] + fn an_unresolvable_from_is_refused_and_creates_no_branch() { + let mut cache = a_cache(); + given_cached_repo(&mut cache); + let fake = FakeGit::new() + .with_script( + [ + "git", + "fetch", + "origin", + "+refs/heads/newbranch:refs/heads/newbranch", + ], + Response::failed( + 128, + "fatal: couldn't find remote ref refs/heads/newbranch\n", + ), + ) + .with_script( + [ + "git", + "fetch", + "origin", + "+refs/heads/ghost:refs/heads/ghost", + ], + Response::failed(128, "fatal: couldn't find remote ref refs/heads/ghost\n"), + ); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + + let (base, _) = ensure_branch_from_with(&manager, &mut cache, "newbranch", "ghost"); + + assert_eq!( + base, + Err(EnsureBranchError::BaseNotResolved { + base: "ghost".to_owned() + }) + ); + let argvs = fake.argvs(); + let issued = as_strs(&argvs); + assert!( + !issued.iter().any(|argv| argv.contains(&"branch")), + "a branch was created despite an unresolvable base: {issued:?}" + ); + } + + #[test] + fn from_against_a_branch_that_already_exists_names_the_branch_not_the_base() { + let mut cache = a_cache(); + given_cached_repo(&mut cache); + // Unscripted: the fetch of "existing" answers success by default, which is + // what a branch the remote already has looks like. + let fake = FakeGit::new(); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + + let (base, _) = ensure_branch_from_with(&manager, &mut cache, "existing", "otherbase"); + + assert_eq!( + base, + Err(EnsureBranchError::BranchAlreadyExists { + branch: "existing".to_owned() + }) + ); + let argvs = fake.argvs(); + let issued = as_strs(&argvs); + assert!( + !issued.iter().any(|argv| argv.contains(&"otherbase")), + "the base was fetched even though the branch already exists, so a refusal \ + here would wrongly look like the base's fault: {issued:?}" + ); + } + + #[test] + fn from_is_absent_from_what_the_workspace_stores() { + // The absence a "does the flag work" test cannot make: a record this + // launch wrote, read back, and compared byte-for-byte against its own + // serialisation for a base string that would show up immediately if any + // field ever carried it. + let mut cache = a_cache(); + given_cached_repo(&mut cache); + let fake = FakeGit::new() + .with_script( + [ + "git", + "fetch", + "origin", + "+refs/heads/nb-from:refs/heads/nb-from", + ], + Response::failed(128, "fatal: couldn't find remote ref refs/heads/nb-from\n"), + ) + .with_script( + ["git", "show-ref", "--verify", "refs/heads/nb-from"], + Response::exited(1), + ); + let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); + + manager + .prepare_cold_from( + &mut cache.storage, + "owner", + "repo", + "nb-from", + Some("a-base-that-must-not-be-stored"), + REMOTE_URL, + &mut ignoring(), + ) + .expect("prepared"); + + let recorded = cache + .storage + .get_worktree("owner", "repo", "nb-from") + .expect("a record"); + let serialised = serde_json::to_string(&recorded).expect("a record serialises"); + assert!( + !serialised.contains("a-base-that-must-not-be-stored"), + "the base leaked into the stored record: {serialised}" + ); + } + #[test] fn every_step_of_the_workspace_reports_what_git_said_or_its_exit_status() { // Four failures, each of which reported "…: None" when an uncaptured stderr @@ -2125,8 +2430,39 @@ mod tests { .hold_repo_lock("owner", "repo") .expect("the lock"); let mut notices = ignoring(); - let base = - manager.ensure_branch(&lock, &cache.storage, "owner", "repo", branch, &mut notices); + let base = manager.ensure_branch( + &lock, + &cache.storage, + "owner", + "repo", + branch, + None, + &mut notices, + ); + (base, notices) + } + + /// [`ensure_branch_with`], with `--from `. + fn ensure_branch_from_with( + manager: &WorkspaceCloneManager<'_>, + cache: &mut Cache, + branch: &str, + from: &str, + ) -> (Result, Vec) { + let lock = manager + .repo_manager() + .hold_repo_lock("owner", "repo") + .expect("the lock"); + let mut notices = ignoring(); + let base = manager.ensure_branch( + &lock, + &cache.storage, + "owner", + "repo", + branch, + Some(from), + &mut notices, + ); (base, notices) } @@ -2464,6 +2800,7 @@ mod tests { "owner", "other", "main", + None, &mut ignoring(), ) .expect_err("a token for one repository cannot vouch for another"); diff --git a/rust/dl/src/cli.rs b/rust/dl/src/cli.rs index a667f07b..1c292bae 100644 --- a/rust/dl/src/cli.rs +++ b/rust/dl/src/cli.rs @@ -434,6 +434,7 @@ pub(crate) enum Command { verb: Verb, devcontainer: Option, claude_profile: Option, + from: Option, }, /// A workspace, and what to do with it. Workspace { @@ -441,6 +442,7 @@ pub(crate) enum Command { verb: Verb, devcontainer: Option, claude_profile: Option, + from: Option, }, } @@ -515,6 +517,14 @@ pub(crate) enum GrammarError { /// --claude-profile work` names a real workspace and a flag that does nothing to /// it. See `claude_profile_ignored` in `crate::commands`. ClaudeProfileNotAllowed { command: &'static str }, + /// `--from` on a command that opens no workspace. + /// + /// Refused here for the same reason [`GrammarError::ClaudeProfileNotAllowed`] + /// is: a global command has no workspace, so it has no branch for the flag + /// to mean anything about. Merely *reported* for a workspace verb that cuts + /// no branch (`stop`, `kill`, `rm`, `rme`) -- see `from_ignored` in + /// `crate::commands`. + FromNotAllowed { command: &'static str }, /// `--rm` on a command the flag is not defined for. /// /// Refused rather than ignored, and `code` is why. `dl code --rm` @@ -700,6 +710,14 @@ pub(crate) struct Cli { /// with the workspace, so a workspace never forwards an account chosen weeks ago. #[arg(long = "claude-profile", value_name = "NAME")] claude_profile: Option, + /// Cut a new branch from this ref instead of the default branch. Only means + /// something when the branch does not exist yet -- a branch that is already + /// there refuses rather than ignoring the flag. Per launch, like + /// `--claude-profile`: unlike `--devcontainer` it is not stored with the + /// workspace, since a base describes an event that happened once rather than + /// what the workspace is. + #[arg(long, value_name = "REF")] + from: Option, /// Delete the workspace once the session ends, like `docker run --rm`. Only /// for the two forms that hand one over: `dl ` and `dl -- `. /// Stops at work that is nowhere else, exactly as the `rm` verb does. @@ -992,6 +1010,9 @@ fn global_command(cli: &Cli, chosen: Chosen) -> Result { if cli.claude_profile.is_some() && !matches!(chosen, Chosen::HerdrShellReady) { return Err(GrammarError::ClaudeProfileNotAllowed { command: name }); } + if cli.from.is_some() { + return Err(GrammarError::FromNotAllowed { command: name }); + } if cli.rm { return Err(GrammarError::RmNotAllowed { command: name }); } @@ -1099,6 +1120,9 @@ fn workspace_command(cli: Cli, argv: &[String]) -> Result // directory component, and owns the refusal, so the grammar does not get a // second opinion about what a profile name may be. let claude_profile = cli.claude_profile.clone(); + // Carried as typed, same as `claude_profile`: the ref is resolved against + // the remote at launch time, not validated as a name here. + let from = cli.from.clone(); if cli.yes { return Err(GrammarError::ModifierNotAllowed { modifier: "--yes", @@ -1146,6 +1170,7 @@ fn workspace_command(cli: Cli, argv: &[String]) -> Result }, devcontainer, claude_profile, + from, }); } ForcePlace::VerbSlot { target } => { @@ -1241,12 +1266,14 @@ fn workspace_command(cli: Cli, argv: &[String]) -> Result verb, devcontainer, claude_profile, + from, }, Some(target) => Command::Workspace { target, verb, devcontainer, claude_profile, + from, }, }) } @@ -1274,7 +1301,12 @@ fn devcontainer_of(cli: &Cli) -> Result, GrammarError> /// A second copy of a fact about [`Cli`], and `the_value_flags_are_the_ones_clap_takes_values_for` /// is the test that diffs it against clap's own parser rather than leaving it to be /// kept true by hand. -const VALUE_FLAGS: [&str; 3] = ["--devcontainer", "--claude-profile", "--herdr-workspace"]; +const VALUE_FLAGS: [&str; 4] = [ + "--devcontainer", + "--claude-profile", + "--herdr-workspace", + "--from", +]; /// The argv `wants_startup_cache_refresh` is asked about. /// @@ -1404,6 +1436,7 @@ mod tests { verb, devcontainer: None, claude_profile: None, + from: None, } } @@ -1421,6 +1454,7 @@ mod tests { verb: Verb::Stop, devcontainer: None, claude_profile: None, + from: None, }, ), (&["stop", "ws"], workspace("ws", Verb::Stop)), @@ -1510,6 +1544,7 @@ mod tests { verb: remove_and_exit(false), devcontainer: None, claude_profile: None, + from: None, }) ); assert!(remove_and_exit(false).several_at_once()); @@ -1726,6 +1761,7 @@ mod tests { verb: attach(), devcontainer: None, claude_profile: None, + from: None, }) ); } @@ -1945,6 +1981,7 @@ mod tests { verb: Verb::Stop, devcontainer: None, claude_profile: None, + from: None, }) ); } @@ -2039,6 +2076,7 @@ mod tests { verb: attach(), devcontainer: None, claude_profile: Some("work".to_owned()), + from: None, }) ); assert_eq!( @@ -2047,6 +2085,7 @@ mod tests { verb: attach(), devcontainer: None, claude_profile: Some("work".to_owned()), + from: None, }) ); } @@ -2064,6 +2103,54 @@ mod tests { verb: attach(), devcontainer: None, claude_profile: Some("../../etc".to_owned()), + from: None, + }) + ); + } + + // ============================================================= --from + + #[test] + fn from_is_refused_on_a_command_that_opens_no_workspace() { + // The same split `--claude-profile` and `--devcontainer` draw: a global + // command has no workspace, so it has no branch for `--from` to mean + // anything about. + for command in [ + "--ls", + "--prune", + "--purge", + "--reconcile", + "--install", + "--refresh", + "--version", + ] { + assert_eq!( + parse(&[command, "--from", "develop"]), + Err(GrammarError::FromNotAllowed { command }), + "{command}" + ); + } + } + + #[test] + fn from_rides_the_workspace_forms_that_open_a_session() { + assert_eq!( + parse(&["ws", "--from", "develop"]), + Ok(Command::Workspace { + target: "ws".to_owned(), + verb: attach(), + devcontainer: None, + claude_profile: None, + from: Some("develop".to_owned()), + }) + ); + assert_eq!( + parse(&["--from", "develop"]), + Ok(Command::Select { + verb: attach(), + devcontainer: None, + claude_profile: None, + from: Some("develop".to_owned()), }) ); } @@ -2261,6 +2348,7 @@ mod tests { verb: Verb::Attach { rm: RmOnExit::Yes }, devcontainer: None, claude_profile: None, + from: None, }) ); } diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index c2db1b8a..38a67532 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -165,6 +165,10 @@ pub(crate) fn dispatch( // agent authenticated as a different account than the agent, // which is the one way this feature could mislead quietly. claude_profile: profile.or(claude_profile), + // A base is a one-time event and a pane has no branch left + // to cut anyway: this reattaches to a workspace the sibling + // already opened. + from: None, }, None, ); @@ -184,6 +188,7 @@ pub(crate) fn dispatch( verb, devcontainer, claude_profile, + from, } => { let after = verb.after_removal(); let ending = render_select( @@ -194,6 +199,7 @@ pub(crate) fn dispatch( verb, devcontainer.as_ref(), claude_profile.as_deref(), + from.as_deref(), ); hangup::after_the_command(after, ending) } @@ -202,6 +208,7 @@ pub(crate) fn dispatch( verb, devcontainer, claude_profile, + from, } => { // The one place a typed target exists before anything has read it, // which is why the pull request rewrite happens here and nowhere @@ -227,6 +234,7 @@ pub(crate) fn dispatch( verb, devcontainer.as_ref(), claude_profile.as_deref(), + from.as_deref(), // A target named on the command line is resolved by the launch // itself; only the picker arrives knowing more than it says. None, @@ -764,6 +772,7 @@ fn render_workspace<'r>( verb: Verb, devcontainer: Option<&DevcontainerPath>, claude_profile: Option<&str>, + from: Option<&str>, recognised: Option, resume: Option, ) -> Ending { @@ -781,16 +790,19 @@ fn render_workspace<'r>( Family::Stop => { devcontainer_ignored(devcontainer.is_some(), word); claude_profile_ignored(claude_profile.is_some(), word); + from_ignored(from.is_some(), word); render_stop(runner, context, refresh, &mut cold, target) } Family::Kill => { devcontainer_ignored(devcontainer.is_some(), word); claude_profile_ignored(claude_profile.is_some(), word); + from_ignored(from.is_some(), word); render_kill(runner, context, cache, refresh, &mut cold, target, word) } Family::Remove { force } => { devcontainer_ignored(devcontainer.is_some(), word); claude_profile_ignored(claude_profile.is_some(), word); + from_ignored(from.is_some(), word); render_remove( runner, context, @@ -814,6 +826,7 @@ fn render_workspace<'r>( &launched, devcontainer, claude_profile, + from, recognised, resume, ); @@ -954,6 +967,12 @@ fn claude_profile_ignored(given: bool, verb: &str) { } } +fn from_ignored(given: bool, verb: &str) { + if given { + eprintln!("Ignoring --from: '{verb}' cuts no branch."); + } +} + fn devcontainer_ignored(given: bool, verb: &str) { if given { eprintln!("Ignoring --devcontainer: it does not apply to '{verb}'."); @@ -1613,6 +1632,13 @@ fn render_reconcile( /// /// A pick that never came is Python's ending exactly: the help on stdout and exit 1 /// (`dl.py` 4457-4462). The help is clap's (**row 3**). +// Same trio of launch modifiers as `render_workspace` above, which carries this +// allow for the same reason: `--devcontainer`, `--claude-profile` and `--from` +// are one group in the grammar (completion_tables calls them "the launch +// modifiers the grammar leaves over") but they are three independent `Option`s +// at every call site, and grouping them into a struct here alone would leave +// the two siblings spelling one concept two ways. +#[allow(clippy::too_many_arguments)] fn render_select<'r>( runner: &'r dyn Runner, context: &mut CommandContext<'r>, @@ -1621,6 +1647,7 @@ fn render_select<'r>( verb: Verb, devcontainer: Option<&DevcontainerPath>, claude_profile: Option<&str>, + from: Option<&str>, ) -> Ending { let workspaces = match context.workspaces() { Err(refused) => return refuse_listing(&refused), @@ -1659,6 +1686,7 @@ fn render_select<'r>( verb.clone(), devcontainer, claude_profile, + from, // The picker knows what it drew: this row's clone said it is // this triple, and the launch it is about to start knows only // the id. See `Launch::recognised_as`. diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index 38f04ac1..439dc26f 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -139,6 +139,7 @@ pub(crate) fn render_launch<'r>( verb: &LaunchVerb, devcontainer: Option<&DevcontainerPath>, claude_profile: Option<&str>, + from_ref: Option<&str>, recognised: Option, resume: Option, ) -> Ran { @@ -196,7 +197,8 @@ pub(crate) fn render_launch<'r>( &mut forward, &mut said, ) - .recognised_as(recognised); + .recognised_as(recognised) + .from_ref(from_ref.map(str::to_owned)); launch.run(target, verb, devcontainer) }; ran(outcome, cache) diff --git a/rust/dl/src/lib.rs b/rust/dl/src/lib.rs index f40e2f81..63e1886e 100644 --- a/rust/dl/src/lib.rs +++ b/rust/dl/src/lib.rs @@ -703,6 +703,9 @@ fn grammar_refusal(refused: &cli::GrammarError) -> String { cli::GrammarError::ClaudeProfileNotAllowed { command } => { format!("--claude-profile means nothing for {command}: it forwards no Claude login.") } + cli::GrammarError::FromNotAllowed { command } => { + format!("--from means nothing for {command}: it opens no workspace.") + } // The two forms it *does* apply to are named, and so is what to type to // delete a workspace now: somebody who reached for `--rm` on another verb // wants the workspace gone at some point, and this sentence is where they diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 83db1efe..603f3edc 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -3711,6 +3711,14 @@ fn prepare_cold_failure(refused: &PrepareColdError) -> String { PrepareColdError::Clone(error) => clone_failure(error), PrepareColdError::Branch(EnsureBranchError::WrongRepoLock(wrong)) => wrong_repo_lock(wrong), PrepareColdError::Branch(EnsureBranchError::Branch(error)) => branch_failure(error), + PrepareColdError::Branch(EnsureBranchError::BranchAlreadyExists { branch }) => format!( + "--from means nothing for '{branch}': it already exists, so there is no branch \ + left to cut." + ), + PrepareColdError::Branch(EnsureBranchError::BaseNotResolved { base }) => format!( + "--from {base}: could not fetch '{base}' from the remote, so there is nothing to \ + cut the new branch from." + ), PrepareColdError::Workspace(error) => workspace_failure(error), } }