diff --git a/src/app/mod.rs b/src/app/mod.rs index 3a0c4d2..2546285 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -852,9 +852,9 @@ mod tests { selected_task_from_side_panel_rows, side_panel_rows_from, sorted_categories_with_indexes, }; use super::workflows::{ - build_attach_popup_lines, parse_existing_branch_name, popup_style_from_theme, - reconcile_startup_tasks, repo_match_candidates, repo_selection_command_id, - repo_selection_usage_map, resolve_repo_for_creation, tmux_hex_color, + build_attach_popup_lines, popup_style_from_theme, reconcile_startup_tasks, + repo_match_candidates, repo_selection_command_id, repo_selection_usage_map, + resolve_repo_for_creation, tmux_hex_color, }; use super::*; @@ -1172,25 +1172,6 @@ mod tests { assert_eq!(ranked.first().copied(), Some(1)); } - #[test] - fn parse_existing_branch_name_detects_git_branch_collision() { - let detail = - "stderr: Preparing worktree (new branch 'c')\nfatal: a branch named 'c' already exists"; - assert_eq!(parse_existing_branch_name(detail), Some("c".to_string())); - } - - #[test] - fn create_task_error_dialog_state_branch_collision_is_concise() { - let err = anyhow::anyhow!( - "worktree creation failed: failed to create worktree `/home/cc/.opencode-kanban-worktrees/test/c-2` for branch `c` from `main`: git command failed in /home/cc/codes/playgrounds/test: git worktree add -b c /home/cc/.opencode-kanban-worktrees/test/c-2 main\nstdout:\nstderr: Preparing worktree (new branch 'c')\nfatal: a branch named 'c' already exists" - ); - - let dialog = create_task_error_dialog_state(&err); - assert_eq!(dialog.title, "Branch already exists"); - assert!(dialog.detail.contains("Branch `c` already exists")); - assert!(!dialog.detail.contains("git worktree add -b")); - } - #[test] fn resolve_repo_for_creation_accepts_fuzzy_existing_repo_query() -> Result<()> { let db = Database::open(":memory:")?; diff --git a/src/app/runtime.rs b/src/app/runtime.rs index e4c5028..bdc642d 100644 --- a/src/app/runtime.rs +++ b/src/app/runtime.rs @@ -5,8 +5,9 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use crate::git::{ - git_check_branch_up_to_date, git_create_worktree, git_detect_default_branch, git_fetch, - git_is_valid_repo, git_remove_worktree, git_resolve_remote_ref, git_set_upstream, + git_check_branch_up_to_date, git_create_worktree, git_create_worktree_from_existing_branch, + git_detect_default_branch, git_fetch, git_is_valid_repo, git_local_branch_exists, + git_remove_worktree, git_resolve_remote_ref, git_set_upstream, }; use crate::process::command; use crate::tmux::{ @@ -120,6 +121,7 @@ pub trait CreateTaskRuntime { fn git_fetch(&self, repo_path: &Path) -> Result<()>; fn git_resolve_remote_ref(&self, repo_path: &Path, source: &str) -> Result; fn git_validate_branch(&self, repo_path: &Path, branch_name: &str) -> Result<()>; + fn git_local_branch_exists(&self, repo_path: &Path, branch_name: &str) -> bool; fn git_check_branch_up_to_date(&self, repo_path: &Path, base_ref: &str) -> Result<()>; fn git_create_worktree( &self, @@ -128,6 +130,12 @@ pub trait CreateTaskRuntime { branch_name: &str, base_ref: &str, ) -> Result<()>; + fn git_create_worktree_from_existing_branch( + &self, + repo_path: &Path, + worktree_path: &Path, + branch_name: &str, + ) -> Result<()>; fn git_set_upstream(&self, repo_path: &Path, branch: &str, remote_source: &str) -> Result<()>; fn git_remove_worktree(&self, repo_path: &Path, worktree_path: &Path) -> Result<()>; fn tmux_session_exists(&self, session_name: &str) -> bool; @@ -238,6 +246,10 @@ impl CreateTaskRuntime for RealCreateTaskRuntime { git_check_branch_up_to_date(repo_path, base_ref) } + fn git_local_branch_exists(&self, repo_path: &Path, branch_name: &str) -> bool { + git_local_branch_exists(repo_path, branch_name) + } + fn git_create_worktree( &self, repo_path: &Path, @@ -248,6 +260,15 @@ impl CreateTaskRuntime for RealCreateTaskRuntime { git_create_worktree(repo_path, worktree_path, branch_name, base_ref) } + fn git_create_worktree_from_existing_branch( + &self, + repo_path: &Path, + worktree_path: &Path, + branch_name: &str, + ) -> Result<()> { + git_create_worktree_from_existing_branch(repo_path, worktree_path, branch_name) + } + fn git_remove_worktree(&self, repo_path: &Path, worktree_path: &Path) -> Result<()> { git_remove_worktree(repo_path, worktree_path) } diff --git a/src/app/workflows/create_task.rs b/src/app/workflows/create_task.rs index 296a3ed..5ef1fa5 100644 --- a/src/app/workflows/create_task.rs +++ b/src/app/workflows/create_task.rs @@ -115,30 +115,8 @@ pub(crate) fn create_task_pipeline_with_runtime( .git_validate_branch(&repo_path, &branch) .context("branch validation failed")?; - let mut base_ref = if state.base_input.trim().is_empty() { - runtime.git_detect_default_branch(&repo_path) - } else { - state.base_input.trim().to_string() - }; - - if state.base_is_remote { - runtime - .git_fetch(&repo_path) - .context("failed to fetch origin; no task was created")?; - base_ref = runtime - .git_resolve_remote_ref(&repo_path, &base_ref) - .context("selected origin branch is no longer available; no task was created")?; - } else if let Err(err) = runtime.git_fetch(&repo_path) { - let message = format!("fetch from origin failed, continuing offline: {err:#}"); - tracing::warn!("{message}"); - warning = Some(message); - } - - if state.ensure_base_up_to_date { - runtime - .git_check_branch_up_to_date(&repo_path, &base_ref) - .context("base branch check failed")?; - } + let reuse_existing_branch = !state.branch_input.trim().is_empty() + && runtime.git_local_branch_exists(&repo_path, &branch); let worktrees_root = worktrees_root_for_repo(&repo_path); fs::create_dir_all(&worktrees_root).with_context(|| { @@ -149,16 +127,54 @@ pub(crate) fn create_task_pipeline_with_runtime( })?; let derived_worktree_path = derive_worktree_path(&worktrees_root, &repo_path, &branch); - runtime - .git_create_worktree(&repo_path, &derived_worktree_path, &branch, &base_ref) - .context("worktree creation failed")?; + if reuse_existing_branch { + runtime + .git_create_worktree_from_existing_branch( + &repo_path, + &derived_worktree_path, + &branch, + ) + .context("worktree creation failed")?; + } else { + let mut base_ref = if state.base_input.trim().is_empty() { + runtime.git_detect_default_branch(&repo_path) + } else { + state.base_input.trim().to_string() + }; + + if state.base_is_remote { + runtime + .git_fetch(&repo_path) + .context("failed to fetch origin; no task was created")?; + base_ref = runtime + .git_resolve_remote_ref(&repo_path, &base_ref) + .context( + "selected origin branch is no longer available; no task was created", + )?; + } else if let Err(err) = runtime.git_fetch(&repo_path) { + let message = format!("fetch from origin failed, continuing offline: {err:#}"); + tracing::warn!("{message}"); + warning = Some(message); + } - if state.base_is_remote - && let Err(error) = runtime.git_set_upstream(&derived_worktree_path, &branch, &base_ref) - { - let _ = runtime.git_remove_worktree(&repo_path, &derived_worktree_path); - return Err(error) - .context("worktree was created but upstream tracking could not be configured"); + if state.ensure_base_up_to_date { + runtime + .git_check_branch_up_to_date(&repo_path, &base_ref) + .context("base branch check failed")?; + } + + runtime + .git_create_worktree(&repo_path, &derived_worktree_path, &branch, &base_ref) + .context("worktree creation failed")?; + + if state.base_is_remote + && let Err(error) = + runtime.git_set_upstream(&derived_worktree_path, &branch, &base_ref) + { + let _ = runtime.git_remove_worktree(&repo_path, &derived_worktree_path); + return Err(error) + .context("worktree was created but upstream tracking could not be configured"); + } } (repo, branch, repo_path, derived_worktree_path, true) @@ -507,7 +523,7 @@ mod tests { use crate::db::Database; use crate::types::Repo; use anyhow::Result; - use std::cell::RefCell; + use std::cell::{Cell, RefCell}; use std::collections::HashMap; use std::path::{Path, PathBuf}; use tempfile::TempDir; @@ -516,8 +532,12 @@ mod tests { struct FakeCreateRuntime { fetch_error: Option, resolve_error: Option, + existing_branch: Cell, + reuse_error: Cell, fetched: RefCell, created: RefCell, + reused: RefCell, + session_created: Cell, upstream: RefCell>, } @@ -526,8 +546,12 @@ mod tests { Self { fetch_error: fetch_error.map(str::to_string), resolve_error: resolve_error.map(str::to_string), + existing_branch: Cell::new(false), + reuse_error: Cell::new(false), fetched: RefCell::new(false), created: RefCell::new(false), + reused: RefCell::new(false), + session_created: Cell::new(false), upstream: RefCell::new(Vec::new()), } } @@ -562,6 +586,9 @@ mod tests { fn git_validate_branch(&self, _: &Path, _: &str) -> Result<()> { Ok(()) } + fn git_local_branch_exists(&self, _: &Path, _: &str) -> bool { + self.existing_branch.get() + } fn git_check_branch_up_to_date(&self, _: &Path, _: &str) -> Result<()> { Ok(()) } @@ -569,6 +596,19 @@ mod tests { *self.created.borrow_mut() = true; Ok(()) } + fn git_create_worktree_from_existing_branch( + &self, + _: &Path, + _: &Path, + _: &str, + ) -> Result<()> { + *self.created.borrow_mut() = true; + *self.reused.borrow_mut() = true; + if self.reuse_error.get() { + anyhow::bail!("branch is already checked out"); + } + Ok(()) + } fn git_set_upstream(&self, _: &Path, branch: &str, source: &str) -> Result<()> { self.upstream .borrow_mut() @@ -582,6 +622,7 @@ mod tests { false } fn tmux_create_session(&self, _: &str, _: &Path, _: Option<&str>) -> Result<()> { + self.session_created.set(true); Ok(()) } fn tmux_apply_task_status_bar( @@ -705,6 +746,53 @@ mod tests { assert!(local_runtime.upstream.borrow().is_empty()); } + #[test] + fn existing_local_branch_skips_base_and_upstream_handling() { + let (_temp, db, repo) = pipeline_fixture(); + let category = db.list_categories().expect("categories")[0].id; + let runtime = FakeCreateRuntime::new(Some("fetch must not run"), None); + runtime.existing_branch.set(true); + + create_task_pipeline_with_runtime( + &db, + &mut vec![repo.clone()], + category, + &pipeline_state(Path::new(&repo.path), true), + None, + &runtime, + ) + .expect("existing branch task"); + + assert!(*runtime.created.borrow()); + assert!(*runtime.reused.borrow()); + assert!(!*runtime.fetched.borrow()); + assert!(runtime.upstream.borrow().is_empty()); + assert_eq!(db.list_tasks().expect("tasks").len(), 1); + } + + #[test] + fn existing_local_branch_failure_stops_before_session_and_task_creation() { + let (_temp, db, repo) = pipeline_fixture(); + let category = db.list_categories().expect("categories")[0].id; + let runtime = FakeCreateRuntime::new(None, None); + runtime.existing_branch.set(true); + runtime.reuse_error.set(true); + + let error = create_task_pipeline_with_runtime( + &db, + &mut vec![repo.clone()], + category, + &pipeline_state(Path::new(&repo.path), false), + None, + &runtime, + ) + .expect_err("checked-out branch should fail"); + + assert!(error.to_string().contains("worktree creation failed")); + assert!(!runtime.session_created.get()); + assert_eq!(db.list_tasks().expect("tasks").len(), 0); + } + #[test] fn resolve_create_task_branch_rejects_empty_branch_and_title() { let err = resolve_create_task_branch("", "").expect_err("empty branch+title must fail"); diff --git a/src/app/workflows/errors.rs b/src/app/workflows/errors.rs index a1dbda5..cfb1182 100644 --- a/src/app/workflows/errors.rs +++ b/src/app/workflows/errors.rs @@ -3,15 +3,6 @@ use crate::app::ErrorDialogState; pub(crate) fn create_task_error_dialog_state(err: &anyhow::Error) -> ErrorDialogState { let detail = format!("{err:#}"); - if let Some(branch) = parse_existing_branch_name(&detail) { - return ErrorDialogState { - title: "Branch already exists".to_string(), - detail: format!( - "Branch `{branch}` already exists in this repository, so a new worktree branch cannot be created.\n\nChoose a different branch name, or delete/rename the existing local branch and try again." - ), - }; - } - let title = if detail.contains("worktree creation failed") { "Worktree creation failed".to_string() } else if detail.contains("tmux session creation failed") { @@ -22,16 +13,3 @@ pub(crate) fn create_task_error_dialog_state(err: &anyhow::Error) -> ErrorDialog ErrorDialogState { title, detail } } - -pub(crate) fn parse_existing_branch_name(detail: &str) -> Option { - detail.lines().find_map(|line| { - let trimmed = line.trim(); - let rest = trimmed.strip_prefix("fatal: a branch named '")?; - let (branch_name, _) = rest.split_once("' already exists")?; - if branch_name.is_empty() { - None - } else { - Some(branch_name.to_string()) - } - }) -} diff --git a/src/app/workflows/mod.rs b/src/app/workflows/mod.rs index 14fa6fb..33d10fb 100644 --- a/src/app/workflows/mod.rs +++ b/src/app/workflows/mod.rs @@ -14,6 +14,4 @@ pub(crate) use create_task::{ repo_match_candidates, repo_selection_command_id, resolve_repo_for_creation, }; pub(crate) use errors::create_task_error_dialog_state; -#[cfg(test)] -pub(crate) use errors::parse_existing_branch_name; pub(crate) use recovery::reconcile_startup_tasks; diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 0c73c93..01ce27a 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -534,83 +534,95 @@ fn task_create(db: &Database, project: &str, args: TaskCreateArgs) -> CliResult< .context("branch validation failed") .map_err(classify_db_error)?; - let (worktree_path, remove_worktree_on_failure) = if let Some(existing_dir_raw) = - args.existing_dir.as_deref() - { - let existing_dir = PathBuf::from(existing_dir_raw.trim()); - if existing_dir_raw.trim().is_empty() { - return Err(usage_error( - "EXISTING_DIR_REQUIRED", - "existing directory cannot be empty", - )); - } - if !existing_dir.exists() { - return Err(not_found_error( - "EXISTING_DIR_NOT_FOUND", - format!( - "existing directory '{}' does not exist", - existing_dir.display() - ), - )); - } - if !existing_dir.is_dir() { - return Err(usage_error( - "EXISTING_DIR_INVALID", - format!( - "existing directory '{}' is not a folder", - existing_dir.display() - ), - )); - } - if !CreateTaskRuntime::git_is_valid_repo(&runtime, &existing_dir) { - return Err(usage_error( - "EXISTING_DIR_NOT_GIT_REPO", - format!( - "existing directory '{}' is not a git repository", - existing_dir.display() - ), - )); - } - - let canonical = fs::canonicalize(&existing_dir) - .context("failed to canonicalize existing directory") - .map_err(classify_db_error)?; - (canonical, false) - } else { - let base_ref = repo - .default_base - .clone() - .filter(|value| !value.trim().is_empty()) - .unwrap_or_else(|| CreateTaskRuntime::git_detect_default_branch(&runtime, &repo_path)); - - if let Err(err) = CreateTaskRuntime::git_fetch(&runtime, &repo_path) { - warn!( - repo = %repo.path, - error = %err, - "fetch from origin failed, continuing offline" - ); - } - - CreateTaskRuntime::git_check_branch_up_to_date(&runtime, &repo_path, &base_ref) - .context("base branch check failed") - .map_err(classify_db_error)?; - - let worktrees_root = worktrees_root_for_repo(&repo_path); - fs::create_dir_all(&worktrees_root).map_err(runtime_error)?; - let derived_worktree_path = derive_worktree_path(&worktrees_root, &repo_path, branch); + let (worktree_path, remove_worktree_on_failure) = + if let Some(existing_dir_raw) = args.existing_dir.as_deref() { + let existing_dir = PathBuf::from(existing_dir_raw.trim()); + if existing_dir_raw.trim().is_empty() { + return Err(usage_error( + "EXISTING_DIR_REQUIRED", + "existing directory cannot be empty", + )); + } + if !existing_dir.exists() { + return Err(not_found_error( + "EXISTING_DIR_NOT_FOUND", + format!( + "existing directory '{}' does not exist", + existing_dir.display() + ), + )); + } + if !existing_dir.is_dir() { + return Err(usage_error( + "EXISTING_DIR_INVALID", + format!( + "existing directory '{}' is not a folder", + existing_dir.display() + ), + )); + } + if !CreateTaskRuntime::git_is_valid_repo(&runtime, &existing_dir) { + return Err(usage_error( + "EXISTING_DIR_NOT_GIT_REPO", + format!( + "existing directory '{}' is not a git repository", + existing_dir.display() + ), + )); + } - CreateTaskRuntime::git_create_worktree( - &runtime, - &repo_path, - &derived_worktree_path, - branch, - &base_ref, - ) - .context("worktree creation failed") - .map_err(classify_db_error)?; + let canonical = fs::canonicalize(&existing_dir) + .context("failed to canonicalize existing directory") + .map_err(classify_db_error)?; + (canonical, false) + } else { + let worktrees_root = worktrees_root_for_repo(&repo_path); + fs::create_dir_all(&worktrees_root).map_err(runtime_error)?; + let derived_worktree_path = derive_worktree_path(&worktrees_root, &repo_path, branch); + + if CreateTaskRuntime::git_local_branch_exists(&runtime, &repo_path, branch) { + CreateTaskRuntime::git_create_worktree_from_existing_branch( + &runtime, + &repo_path, + &derived_worktree_path, + branch, + ) + .context("worktree creation failed") + .map_err(classify_db_error)?; + } else { + let base_ref = repo + .default_base + .clone() + .filter(|value| !value.trim().is_empty()) + .unwrap_or_else(|| { + CreateTaskRuntime::git_detect_default_branch(&runtime, &repo_path) + }); + + if let Err(err) = CreateTaskRuntime::git_fetch(&runtime, &repo_path) { + warn!( + repo = %repo.path, + error = %err, + "fetch from origin failed, continuing offline" + ); + } + + CreateTaskRuntime::git_check_branch_up_to_date(&runtime, &repo_path, &base_ref) + .context("base branch check failed") + .map_err(classify_db_error)?; + + CreateTaskRuntime::git_create_worktree( + &runtime, + &repo_path, + &derived_worktree_path, + branch, + &base_ref, + ) + .context("worktree creation failed") + .map_err(classify_db_error)?; + } - (derived_worktree_path, true) - }; + (derived_worktree_path, true) + }; let project_slug = if project == projects::DEFAULT_PROJECT { None diff --git a/src/git/mod.rs b/src/git/mod.rs index b1ea940..c83bcc7 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -182,23 +182,7 @@ pub fn git_create_worktree( branch_name: &str, base_ref: &str, ) -> Result<()> { - let check_output = run_git_output(repo_path, ["check-ref-format", "--branch", branch_name]) - .with_context(|| format!("failed to validate branch name `{branch_name}`"))?; - if !check_output.status.success() { - let stdout = String::from_utf8_lossy(&check_output.stdout) - .trim() - .to_string(); - let stderr = String::from_utf8_lossy(&check_output.stderr) - .trim() - .to_string(); - bail!("invalid branch name `{branch_name}`\nstdout: {stdout}\nstderr: {stderr}"); - } - - if worktree_path.exists() { - bail!("worktree path already exists: {}", worktree_path.display()); - } - - let worktree_path_str = worktree_path.to_string_lossy().to_string(); + let worktree_path_str = validate_worktree_request(repo_path, worktree_path, branch_name)?; run_git( repo_path, [ @@ -218,6 +202,48 @@ pub fn git_create_worktree( }) } +pub fn git_create_worktree_from_existing_branch( + repo_path: &Path, + worktree_path: &Path, + branch_name: &str, +) -> Result<()> { + let worktree_path_str = validate_worktree_request(repo_path, worktree_path, branch_name)?; + run_git( + repo_path, + ["worktree", "add", &worktree_path_str, branch_name], + ) + .with_context(|| { + format!( + "failed to create worktree `{}` from existing branch `{branch_name}`", + worktree_path.display() + ) + }) +} + +fn validate_worktree_request( + repo_path: &Path, + worktree_path: &Path, + branch_name: &str, +) -> Result { + let check_output = run_git_output(repo_path, ["check-ref-format", "--branch", branch_name]) + .with_context(|| format!("failed to validate branch name `{branch_name}`"))?; + if !check_output.status.success() { + let stdout = String::from_utf8_lossy(&check_output.stdout) + .trim() + .to_string(); + let stderr = String::from_utf8_lossy(&check_output.stderr) + .trim() + .to_string(); + bail!("invalid branch name `{branch_name}`\nstdout: {stdout}\nstderr: {stderr}"); + } + + if worktree_path.exists() { + bail!("worktree path already exists: {}", worktree_path.display()); + } + + Ok(worktree_path.to_string_lossy().to_string()) +} + pub fn git_remove_worktree(repo_path: &Path, worktree_path: &Path) -> Result<()> { let worktree_path_str = worktree_path.to_string_lossy().to_string(); run_git( @@ -415,7 +441,7 @@ fn sanitize_slug(input: &str, fallback: &str) -> String { } } -fn branch_exists(repo_path: &Path, branch_name: &str) -> bool { +pub fn git_local_branch_exists(repo_path: &Path, branch_name: &str) -> bool { run_git_output( repo_path, [ @@ -426,6 +452,10 @@ fn branch_exists(repo_path: &Path, branch_name: &str) -> bool { ], ) .is_ok() +} + +fn branch_exists(repo_path: &Path, branch_name: &str) -> bool { + git_local_branch_exists(repo_path, branch_name) || run_git_output( repo_path, [ @@ -549,6 +579,37 @@ mod tests { assert!(worktrees.contains(worktree.to_string_lossy().as_ref())); } + #[test] + fn test_create_worktree_from_existing_local_branch() { + let repo = + TestRepo::new_with_origin_main("existing-worktree").expect("repo should be created"); + repo.git(["branch", "feature/existing"]) + .expect("branch should be created"); + let worktree = repo.temp.path().join("wt-existing"); + + assert!(git_local_branch_exists(repo.path(), "feature/existing")); + git_create_worktree_from_existing_branch(repo.path(), &worktree, "feature/existing") + .expect("worktree should reuse existing branch"); + + assert!(worktree.exists()); + let branch = run_git_stdout(&worktree, ["branch", "--show-current"]) + .expect("worktree branch should be readable"); + assert_eq!(branch.trim(), "feature/existing"); + } + + #[test] + fn test_existing_branch_already_checked_out_does_not_create_worktree() { + let repo = + TestRepo::new_with_origin_main("occupied-worktree").expect("repo should be created"); + let worktree = repo.temp.path().join("wt-occupied"); + + let error = git_create_worktree_from_existing_branch(repo.path(), &worktree, "main") + .expect_err("occupied branch should fail"); + + assert!(error.to_string().contains("existing branch `main`")); + assert!(!worktree.exists()); + } + #[test] fn test_remove_worktree_and_delete_branch() { let repo = diff --git a/src/ui.rs b/src/ui.rs index e531d76..c5aaf79 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -1663,7 +1663,7 @@ fn render_new_task_dialog( &state.branch_input, state.focused_field == NewTaskField::Branch, theme, - Some("auto-generated if empty"), + Some("reuses a local branch; generated if empty"), ); app.interaction_map.register_click( InteractionLayer::Dialog,