diff --git a/.claude/skills/architecture/SKILL.md b/.claude/skills/architecture/SKILL.md index cb8edc6..ae627e3 100644 --- a/.claude/skills/architecture/SKILL.md +++ b/.claude/skills/architecture/SKILL.md @@ -11,7 +11,7 @@ Broad, global rules only. Implementation-level decisions, extension recipes, and ## Principles -1. **Ports & adapters.** Business logic lives in `flows/` and depends only on traits (`Git`, `HostingPlatform`, `Editor`). Concrete adapters are wired in `main.rs` — the only composition root. No subprocess calls outside adapter impls. +1. **Ports & adapters.** Business logic lives in `flows/` and depends only on traits (`Git`, `HostingPlatform`, `Editor`, `Prompter`). Concrete adapters are wired in `main.rs` — the only composition root. No subprocess calls outside adapter impls. 2. **Plug-and-play integrations.** Hosting providers are auto-detected from the remote and swappable; adding a provider or editor never touches flows. 3. **Integrate through the user's own CLIs** (`git`, `gh`, `az`) — never link libraries or call APIs directly. bflow inherits the user's auth, config, hooks, and signing. 4. **Minimal dependency budget.** Two crates today. A new dependency needs a very good reason; prefer hand-rolling small things. @@ -26,8 +26,9 @@ Broad, global rules only. Implementation-level decisions, extension recipes, and | Module | Responsibility | |---|---| -| `main.rs` | Composition root: builds adapters, preflight, cross-cutting lifecycle (stash/state/resume), dispatches `Action` once | -| `cli.rs` / `menu.rs` | The two interfaces; both resolve to `Action` | +| `main.rs` | Composition root: builds adapters, preflight, hands off to `lifecycle::run` | +| `lifecycle.rs` | Cross-cutting lifecycle (stash/state/resume ordering), dispatches `Action` once | +| `cli.rs` / `menu.rs` | The two interfaces; both resolve to `Action` (`action.rs`) | | `flows/` | Business logic per workflow | -| `git/`, `hosting/`, `editor.rs` | Ports (traits) + adapters (CLI impls); `hosting/detect.rs` picks the provider | +| `git/`, `hosting/`, `editor.rs`, `prompt.rs` | Ports (traits) + adapters (CLI impls); `hosting/detect.rs` picks the provider | | `state.rs`, `worktree.rs`, `version.rs` | Finish state, worktree feature, SemVer | diff --git a/.claude/skills/architecture/decisions.md b/.claude/skills/architecture/decisions.md index 7e4a701..37b228d 100644 --- a/.claude/skills/architecture/decisions.md +++ b/.claude/skills/architecture/decisions.md @@ -7,6 +7,7 @@ Catalog of deliberate choices — each entry: the choice, why, and the rejected - **New git operation:** add method to `Git` trait → impl in `GitCli` → add to `MockGit` in `tests/common/mod.rs`. - **New hosting provider:** new file in `hosting/` implementing `HostingPlatform` → `Provider` variant + parse arm in `detect.rs` → preflight arm in `main.rs::create_hosting`. Reuse `hosting::run_cli`. - **New command/flow:** clap variant in `cli.rs` → `Action` variant + menu entry gated by `BranchType` → flow fn in `flows/` taking `&dyn` deps → dispatch arm in `run_flow`. +- **New work-branch type** (e.g. `perf/`): variant on `BranchType` → entry in the `WORK_TYPES` table + arm in `work_kind` in `git/branch.rs` (the compiler forces both — `work_kind` has no wildcard) → clap `StartKind` variant + arms in `cli.rs::resolve_action` → `pr_template_keys` arm. Menus, parent-branch detection, and `commit_type` (kind name, unless special-cased like feature→feat) follow from the table automatically. ## Plug-in Points @@ -15,6 +16,7 @@ Catalog of deliberate choices — each entry: the choice, why, and the rejected | `Git` | `GitCli`; `MockGit` in tests | Built once in `main.rs` | | `HostingPlatform` | `GitHub` (`gh`), `AzureDevOps` (`az`); `MockHosting` | Auto-detected from origin remote URL (`hosting/detect.rs`) | | `Editor` | `CommandEditor` (any command); `MockEditor` | `bflow.worktree.editor` git config | +| `Prompter` | `MenuPrompter` (interactive select); `MockPrompter` (scripted) | Built once in `main.rs` | User-configurable without code changes: the worktree flow (`bflow.worktree.*` git config, local/global scope, `bflow worktree` wizard) and PR templates (`.github/pr-templates/bflow-.md`, most-specific-first, falling back to the repo's native template). @@ -70,7 +72,7 @@ User-configurable without code changes: the worktree flow (`bflow.worktree.*` gi - **`Git` is ~45 fine-grained primitives** so ordering logic lives in flows and mocks can record exact sequences. Rejected: coarse `finish_release()`-style methods (untestable ordering). - **`HostingPlatform` stays minimal (3 methods)**: `create_or_get_pr` is deliberately one method ("PR already open" is a normal resume outcome and the check-then-create dance is provider-specific); `open_url` is a default method (provider-agnostic, but on the trait as a test seam); the `template` param is a *path* (only `az` needs the contents read; `gh` takes `--body-file`). - **Detection: pure core, thin shell.** `resolve`/`parse_remote` are pure functions unit-tested without a repo (same pattern in `template.rs` and `devops.rs` helpers). Unknown host → GitHub (preserves pre-detection behavior for GitLab/Enterprise users), but an *explicit* `devops` override with an unparseable remote is a hard error — asymmetric on purpose. `Provider::AzureDevOps` carries `{org, project, repo}` so detection and construction are one step. ADO PR URLs are synthesized from parsed coordinates (`az`'s `webUrl` is unreliable); `validate_pr_id` rejects `az`'s literal `"None"`. -- **Library crate + thin binary exists solely so `tests/` can link the flows.** Consequence: visibility falls exactly on the test boundary — `pub mod` at the top, tight function-level privacy inside (`run_cli`, parsers, runners all private). Orchestration (ordering, stash, state lifecycle) stays in `main.rs`, not the library. +- **Library crate + thin binary exists solely so `tests/` can link the flows.** Consequence: visibility falls exactly on the test boundary — `pub mod` at the top, tight function-level privacy inside (`run_cli`, parsers, runners all private). Orchestration (ordering, stash, state lifecycle) lives in `lifecycle::run` in the library, taking `&dyn` ports, so the reject → stash → write-state → dispatch contract and the three-way stash-pop policy are mock-tested (`tests/lifecycle_test.rs`); `main.rs` keeps only adapter construction, preflight, and the `bflow worktree` dispatch. *(Amended: this previously read "orchestration stays in main.rs" — that made the state-before-mutation invariant untestable, and a resume/`--base` bug slipped through this layer.)* - **PR templates are repo files** (`.github/pr-templates/bflow-.md`, specific → group → default → the repo's own native template → empty). The fix family shares one `bflow-fix.md` group key. Native-template path lists are per-provider knowledge, intentionally not shared. ## Testing Strategy @@ -78,7 +80,7 @@ User-configurable without code changes: the worktree flow (`bflow.worktree.*` gi - **Call-recording mock: one `Vec` of `format!`-encoded calls; assertions are exact sequences** (23-element scripts in `finish_release_test.rs`) — a reorder of merge-before-tag is a test failure. Strings, not a typed `Call` enum: expectations read as a runnable script and diff legibly. - **One configurable `MockGit` with knob fields (state axes of git: what exists, what's merged, what's pushed), not a mock family.** Targeted failure injection (`fail_nth_merge` distinguishes "main merge conflicted" from "develop merge conflicted"); `RefCell` interior mutability keeps the trait `&self` so test bookkeeping never infects production signatures. Stateful where flows read back their own writes (stash save→find), dumb constants where nothing branches on the value. - **Negative assertions prove guards fire before damage** (no `checkout:main` in the log after a gate error; `rev_list_count_result = 99` proves the RC gate didn't even run on resume). Idempotent-resume tests build a fully-completed world and assert *zero* mutating calls. -- **No real git anywhere in tests** (flows push/delete/PR — a sandbox would need a fake remote and stubs, and be slow on Windows CI). The one filesystem exception is `state.rs` round-trip tests. Tests split by *reachability*, not dogma: public behavior in `tests/` (one file per flow), private logic inline `#[cfg(test)]`. Scenario builders stay local per test file (DAMP over DRY); the tiny `stash_test.rs` deliberately pins the mock's own recording contract. +- **No real git anywhere in tests** (flows push/delete/PR — a sandbox would need a fake remote and stubs, and be slow on Windows CI). The one filesystem exception is `state.rs` round-trip tests. Tests split by *reachability*, not dogma: public behavior in `tests/` (one file per flow), private logic inline `#[cfg(test)]`. Scenario builders stay local per test file (DAMP over DRY); the tiny `mock_contract_test.rs` deliberately pins the mock's own recording contract. ## Delivery & CI diff --git a/README.md b/README.md index f92cff4..21f0edc 100644 --- a/README.md +++ b/README.md @@ -568,8 +568,11 @@ jobs: ``` src/ -├── main.rs — Entry point, preflight checks, dispatch +├── main.rs — Composition root: builds adapters, preflight, hands off to lifecycle ├── lib.rs — Library root, re-exports all modules +├── action.rs — Action enum: the single currency both interfaces resolve into +├── cli.rs — CLI subcommands (clap); resolves to Action +├── lifecycle.rs — Resume lookup, stash/state ordering contract, dispatch ├── git/ │ ├── mod.rs — Git trait + CLI implementation │ └── branch.rs — Branch type detection and parsing @@ -585,12 +588,14 @@ src/ │ └── finish_hotfix.rs — Finish hotfix with auto-tag, propagate to open releases (idempotent) ├── state.rs — Persisted finish state for conflict recovery ├── version.rs — SemVer parsing and bumping -├── menu.rs — Interactive menus via crossterm +├── menu.rs — Interactive menus via crossterm; implements Prompter +├── prompt.rs — Prompter trait: interactive selection as a port ├── editor.rs — Editor trait for opening worktrees -└── worktree.rs — Worktree config, path resolution, and setup wizard +├── worktree.rs — Worktree config, path resolution, and setup wizard +└── test_support.rs — Shared helpers for inline unit tests (test builds only) ``` -The `Git` and `HostingPlatform` traits enable multiple hosting providers (GitHub and Azure DevOps today, GitLab/Bitbucket-ready) and testability. +The `Git`, `HostingPlatform`, `Editor`, and `Prompter` traits keep every flow fully mockable and make hosting providers swappable (GitHub and Azure DevOps today, GitLab/Bitbucket-ready). ## License diff --git a/src/action.rs b/src/action.rs new file mode 100644 index 0000000..ebc98e3 --- /dev/null +++ b/src/action.rs @@ -0,0 +1,75 @@ +//! The single currency both interfaces resolve into: the menu (`menu.rs`) and +//! the CLI subcommands (`cli.rs`) each produce an `Action`, and nothing +//! downstream knows which interface ran. Lives in its own module so neither +//! interface has to import the other. + +use crate::flows::start::ReleaseType; + +#[derive(Debug, PartialEq)] +pub enum Action { + StartWorkBranch { prefix: String, name: String, from: String, no_checkout: bool, no_worktree: bool }, + StartRelease(Option), + StartReleaseFix { name: String, no_checkout: bool, no_worktree: bool }, + StartHotfixFix { name: String, no_checkout: bool, no_worktree: bool }, + FinishWorkBranch { breaking: Option, base: Option }, + FinishReleaseFix, + FinishRelease, + FinishHotfix, + FinishHotfixFix, + AbortFinish, + BumpVersion, + SyncWithDevelop, +} + +impl Action { + pub fn is_start(&self) -> bool { + matches!( + self, + Action::StartWorkBranch { .. } + | Action::StartRelease(_) + | Action::StartReleaseFix { .. } + | Action::StartHotfixFix { .. } + ) + } + + pub fn no_checkout(&self) -> bool { + match self { + Action::StartWorkBranch { no_checkout, .. } => *no_checkout, + Action::StartReleaseFix { no_checkout, .. } => *no_checkout, + Action::StartHotfixFix { no_checkout, .. } => *no_checkout, + _ => false, + } + } + + /// Whether this action is a named-work-branch start eligible for the worktree + /// flow. Deliberately excludes `StartRelease`, unlike `is_start`. + pub fn worktree_eligible(&self) -> bool { + matches!( + self, + Action::StartWorkBranch { .. } + | Action::StartReleaseFix { .. } + | Action::StartHotfixFix { .. } + ) + } + + pub fn no_worktree(&self) -> bool { + match self { + Action::StartWorkBranch { no_worktree, .. } => *no_worktree, + Action::StartReleaseFix { no_worktree, .. } => *no_worktree, + Action::StartHotfixFix { no_worktree, .. } => *no_worktree, + _ => false, + } + } +} + +/// Branch-name validation shared by the CLI (`--name`) and the interactive +/// prompt — same rules, same message, whichever interface ran. +pub fn validate_branch_name(input: &str) -> Result<(), String> { + if input.is_empty() { + return Err("Name cannot be empty".to_string()); + } + if input.contains("..") || input.contains('~') || input.contains('^') || input.contains(':') || input.contains('\\') { + return Err("Invalid branch name. Avoid special characters (.. ~ ^ : \\)".to_string()); + } + Ok(()) +} diff --git a/src/cli.rs b/src/cli.rs index a827f31..3d9be32 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,7 +1,7 @@ use clap::{Args, Subcommand}; use crate::git::branch::BranchType; use crate::flows::start::ReleaseType; -use crate::menu::{self, Action}; +use crate::action::{validate_branch_name, Action}; #[derive(Subcommand)] pub enum Commands { @@ -141,7 +141,7 @@ pub enum StartKind { } fn start_work_branch(prefix: &str, name: String, base: String, no_checkout: bool, no_worktree: bool) -> Result { - menu::validate_branch_name(&name)?; + validate_branch_name(&name)?; Ok(Action::StartWorkBranch { prefix: prefix.to_string(), name, from: base, no_checkout, no_worktree }) } @@ -180,14 +180,14 @@ pub fn resolve_action(command: Commands, branch_type: &BranchType, worktree_enab Ok(Action::StartRelease(release_type)) } StartKind::ReleaseFix { name, opts } => { - menu::validate_branch_name(&name)?; + validate_branch_name(&name)?; if !auto_discovers_target(&opts, worktree_enabled) { require_release_branch(branch_type)?; } Ok(Action::StartReleaseFix { name, no_checkout: opts.no_checkout, no_worktree: opts.no_worktree }) } StartKind::HotfixFix { name, opts } => { - menu::validate_branch_name(&name)?; + validate_branch_name(&name)?; if !auto_discovers_target(&opts, worktree_enabled) && !matches!(branch_type, BranchType::Main | BranchType::Hotfix { .. }) { diff --git a/src/flows/finish_hotfix.rs b/src/flows/finish_hotfix.rs index c21e31c..21788c0 100644 --- a/src/flows/finish_hotfix.rs +++ b/src/flows/finish_hotfix.rs @@ -1,117 +1,50 @@ -use crate::flows::resume_hint; +use crate::flows::{delete_source_branch, merge_into, push_if_needed, push_tag_if_missing, resume_hint, tag_if_missing}; use crate::git::Git; use crate::version::SemVer; pub fn finish_hotfix(git: &dyn Git, major: u32, minor: u32, patch: u32) -> Result<(), String> { - let hotfix_branch = format!("hotfix/{major}.{minor}.{patch}"); let version = SemVer::new(major, minor, patch); + let hotfix_branch = version.hotfix_branch(); let tag = version.tag_name(); println!("Finishing hotfix {hotfix_branch}..."); - // Merge into main - if !git.is_ancestor(&hotfix_branch, "main")? { - println!("Merging into main..."); - git.checkout("main")?; - git.pull("origin/main")?; - git.merge(&hotfix_branch, &format!("chore: merge hotfix {version} into main")) - .map_err(|e| format!("{e}\n{}", resume_hint(&hotfix_branch)))?; - } else { - println!("↷ skipped: merge into main (already merged)"); - } - - // Tag - if !git.tag_exists(&tag)? { - println!("Tagging: {tag}"); - git.create_tag(&tag, &format!("chore: hotfix {version}"))?; - } else { - println!("↷ skipped: tag {tag} (already exists)"); - } - - // Push main - if !git.is_pushed("main")? { - git.push("main")?; - } else { - println!("↷ skipped: push main (already up to date)"); - } - - // Push tag - if !git.remote_tag_exists(&tag)? { - git.push_tag(&tag)?; - } else { - println!("↷ skipped: push tag {tag} (already pushed)"); - } - - // Merge into develop - if !git.is_ancestor(&hotfix_branch, "develop")? { - println!("Merging into develop..."); - git.checkout("develop")?; - git.pull("origin/develop")?; - git.merge(&hotfix_branch, &format!("chore: merge hotfix {version} into develop")) - .map_err(|e| format!("{e}\n{}", resume_hint(&hotfix_branch)))?; - } else { - println!("↷ skipped: merge into develop (already merged)"); - } - - // Push develop - if !git.is_pushed("develop")? { - git.push("develop")?; - } else { - println!("↷ skipped: push develop (already up to date)"); - } - - // Propagate into every open release branch - let mut release_branches: Vec = git - .list_branches_matching("release/*")? - .into_iter() - .filter(|b| b.starts_with("release/") && !b.starts_with("release-fix/")) - .collect(); + merge_into(git, &hotfix_branch, "main", + &format!("chore: merge hotfix {version} into main"), + &resume_hint(&hotfix_branch))?; + tag_if_missing(git, &tag, &format!("chore: hotfix {version}"))?; + push_if_needed(git, "main")?; + push_tag_if_missing(git, &tag)?; + + merge_into(git, &hotfix_branch, "develop", + &format!("chore: merge hotfix {version} into develop"), + &resume_hint(&hotfix_branch))?; + push_if_needed(git, "develop")?; + + // Propagate into every open release branch — re-sorted here even though the + // trait contract already promises sorted output: deterministic replay on + // resume is a crash-safety invariant, so the flow enforces it rather than + // trusting the adapter (test-pinned with a deliberately unsorted mock). + let mut release_branches = super::branches_with_prefix(git, "release")?; release_branches.sort(); release_branches.dedup(); for release in &release_branches { - if git.is_ancestor(&hotfix_branch, release)? { - println!("↷ skipped: merge into {release} (already merged)"); - } else { - println!("Merging into {release}..."); - git.checkout(release)?; - git.pull(&format!("origin/{release}"))?; - git.merge( - &hotfix_branch, - &format!("chore: merge hotfix {version} into {release}"), - ).map_err(|e| format!( - "{e}\n\ - Hotfix {version} was merged into main and develop, but propagation into {release} failed.\n\ + merge_into(git, &hotfix_branch, release, + &format!("chore: merge hotfix {version} into {release}"), + &format!( + "Hotfix {version} was merged into main and develop, but propagation into {release} failed.\n\ {}\n\ (After all releases are updated, run 'bflow bump' on each to cut a fresh RC for staging.)", resume_hint(&hotfix_branch) ))?; - } - if !git.is_pushed(release)? { - git.push(release)?; - } else { - println!("↷ skipped: push {release} (already up to date)"); - } + // The push sits outside the merge guard so merged-but-unpushed crashes + // still push on resume (see decisions.md). + push_if_needed(git, release)?; } - // Cleanup println!("Cleaning up hotfix branch..."); - // On a resume path that skipped the develop merge, HEAD may still be on the - // hotfix branch — git refuses to delete the currently checked-out branch, so - // switch off it first. `main` is always safe (the hotfix is now in main). - if git.current_branch()? == hotfix_branch { - git.checkout("main")?; - } - if git.local_branch_exists(&hotfix_branch)? { - git.delete_branch_local(&hotfix_branch)?; - } else { - println!("↷ skipped: delete local {hotfix_branch} (already gone)"); - } - if git.remote_branch_exists(&hotfix_branch)? { - git.delete_branch_remote(&hotfix_branch)?; - } else { - println!("↷ skipped: delete remote {hotfix_branch} (already gone)"); - } + delete_source_branch(git, &hotfix_branch)?; if release_branches.is_empty() { println!("Hotfix {version} complete."); diff --git a/src/flows/finish_release.rs b/src/flows/finish_release.rs index b5847e2..af9d3f0 100644 --- a/src/flows/finish_release.rs +++ b/src/flows/finish_release.rs @@ -1,15 +1,21 @@ -use crate::flows::resume_hint; +use crate::flows::{delete_source_branch, merge_into, push_if_needed, push_tag_if_missing, resume_hint, tag_if_missing}; use crate::git::Git; use crate::version::SemVer; +/// Highest `v{major}.{minor}.0-rc.N` tag on `branch`, or `None` when the branch +/// has no matching RC tag. Callers turn `None` into their own per-command error. +fn latest_rc(git: &dyn Git, branch: &str, major: u32, minor: u32) -> Result, String> { + let tags = git.tags_on_branch(branch)?; + Ok(tags.iter().filter_map(|t| SemVer::parse(t)) + .filter(|v| v.major == major && v.minor == minor && v.patch == 0 && v.is_rc()) + .max()) +} + pub fn bump_version(git: &dyn Git, major: u32, minor: u32) -> Result<(), String> { let release = SemVer::new(major, minor, 0); let branch = release.release_branch(); - let tags = git.tags_on_branch(&branch)?; - let latest = tags.iter().filter_map(|t| SemVer::parse(t)) - .filter(|v| v.major == major && v.minor == minor && v.patch == 0 && v.is_rc()) - .max() + let latest = latest_rc(git, &branch, major, minor)? .ok_or_else(|| format!("No RC tags found on branch {branch}. Run 'bflow start release' first."))?; let next = latest.bump_rc(); @@ -30,7 +36,7 @@ pub fn sync_with_develop(git: &dyn Git, major: u32, minor: u32) -> Result<(), St println!("Merging {release_branch} into develop..."); git.checkout("develop")?; - git.pull("origin/develop")?; + git.ff_merge("origin/develop")?; git.merge(&release_branch, &format!("chore: sync release {release} with develop"))?; git.push("develop")?; @@ -44,10 +50,7 @@ pub fn finish_release(git: &dyn Git, major: u32, minor: u32) -> Result<(), Strin let release = SemVer::new(major, minor, 0); let release_branch = release.release_branch(); - let tags = git.tags_on_branch(&release_branch)?; - let latest_rc = tags.iter().filter_map(|t| SemVer::parse(t)) - .filter(|v| v.major == major && v.minor == minor && v.patch == 0 && v.is_rc()) - .max() + let latest_rc = latest_rc(git, &release_branch, major, minor)? .ok_or_else(|| "No RC tag found on this release branch. Run 'bflow bump' first.".to_string())?; let release_version = latest_rc.to_release(); @@ -55,7 +58,9 @@ pub fn finish_release(git: &dyn Git, major: u32, minor: u32) -> Result<(), Strin println!("Finishing release {release_branch} (tag: {tag})..."); - // Merge into main (with RC gate) + // Merge into main — inline rather than merge_into(): the RC gate must run + // inside the not-yet-merged branch, so a resume past the main merge never + // re-evaluates it (negative-tested in finish_release_test.rs). if !git.is_ancestor(&release_branch, "main")? { let latest_rc_tag = latest_rc.tag_name(); let commits_past_rc = git.rev_list_count(&latest_rc_tag, &release_branch)?; @@ -69,71 +74,24 @@ pub fn finish_release(git: &dyn Git, major: u32, minor: u32) -> Result<(), Strin } println!("Merging into main..."); git.checkout("main")?; - git.pull("origin/main")?; + git.ff_merge("origin/main")?; git.merge(&release_branch, &format!("chore: merge release {release} into main")) .map_err(|e| format!("{e}\n{}", resume_hint(&release_branch)))?; } else { println!("↷ skipped: merge into main (already merged)"); } - // Tag main - if !git.tag_exists(&tag)? { - println!("Tagging main: {tag}"); - git.create_tag(&tag, &format!("chore: release {release_version}"))?; - } else { - println!("↷ skipped: tag {tag} (already exists)"); - } + tag_if_missing(git, &tag, &format!("chore: release {release_version}"))?; + push_if_needed(git, "main")?; + push_tag_if_missing(git, &tag)?; - // Push main - if !git.is_pushed("main")? { - git.push("main")?; - } else { - println!("↷ skipped: push main (already up to date)"); - } - - // Push tag - if !git.remote_tag_exists(&tag)? { - git.push_tag(&tag)?; - } else { - println!("↷ skipped: push tag {tag} (already pushed)"); - } + merge_into(git, &release_branch, "develop", + &format!("chore: merge release {release} into develop"), + &resume_hint(&release_branch))?; + push_if_needed(git, "develop")?; - // Merge into develop - if !git.is_ancestor(&release_branch, "develop")? { - println!("Merging into develop..."); - git.checkout("develop")?; - git.pull("origin/develop")?; - git.merge(&release_branch, &format!("chore: merge release {release} into develop")) - .map_err(|e| format!("{e}\n{}", resume_hint(&release_branch)))?; - } else { - println!("↷ skipped: merge into develop (already merged)"); - } - - // Push develop - if !git.is_pushed("develop")? { - git.push("develop")?; - } else { - println!("↷ skipped: push develop (already up to date)"); - } - - // Cleanup println!("Cleaning up release branch..."); - // On a resume path that skipped the develop merge, HEAD may still be on the - // release branch — git refuses to delete the currently checked-out branch, so - // switch off it first. `main` is always safe (the release is now in main). - if git.current_branch()? == release_branch { - git.checkout("main")?; - } - if git.local_branch_exists(&release_branch)? { - git.delete_branch_local(&release_branch)?; - } else { - println!("↷ skipped: delete local {release_branch} (already gone)"); - } - if git.remote_branch_exists(&release_branch)? { - git.delete_branch_remote(&release_branch)?; - } else { - println!("↷ skipped: delete remote {release_branch} (already gone)"); - } + delete_source_branch(git, &release_branch)?; println!("Release {release_version} complete."); Ok(()) diff --git a/src/flows/finish_work.rs b/src/flows/finish_work.rs index a87acc2..9881283 100644 --- a/src/flows/finish_work.rs +++ b/src/flows/finish_work.rs @@ -1,30 +1,32 @@ +use std::path::Path; + use crate::git::Git; use crate::git::branch::BranchType; use crate::hosting::HostingPlatform; -use crate::menu; +use crate::prompt::Prompter; +use crate::version::SemVer; -fn push_and_create_pr(git: &dyn Git, hosting: &dyn HostingPlatform, base: &str, title: &str, branch_type: &BranchType) -> Result<(), String> { +/// `template` is the pre-resolved PR template path (resolved at the composition +/// root, anchored to the repo root) — flows never probe the filesystem. +fn push_and_create_pr(git: &dyn Git, hosting: &dyn HostingPlatform, base: &str, title: &str, template: Option<&Path>) -> Result<(), String> { let current = git.current_branch()?; println!("Pushing branch: {current}"); git.push(¤t)?; - let template = crate::hosting::template::resolve(branch_type); - if let Some(path) = &template { + if let Some(path) = template { println!("Using PR template: {}", path.display()); } println!("Creating PR: {title} → {base}"); - let url = hosting.create_or_get_pr(¤t, base, title, template.as_deref().and_then(|p| p.to_str()))?; + let url = hosting.create_or_get_pr(¤t, base, title, template.and_then(|p| p.to_str()))?; println!("PR: {url}"); hosting.open_url(&url)?; Ok(()) } -fn detect_parent_branch(git: &dyn Git, current: &str) -> Result { - let work_prefixes = ["feature/", "fix/", "chore/", "docs/", "refactor/"]; - +fn detect_parent_branch(git: &dyn Git, prompter: &dyn Prompter, current: &str) -> Result { let remote_branches = git.list_remote_branches()?; let mut candidates: Vec<(String, u32)> = Vec::new(); @@ -32,9 +34,10 @@ fn detect_parent_branch(git: &dyn Git, current: &str) -> Result if branch == current { continue; } - let is_work = work_prefixes.iter().any(|p| branch.starts_with(p)); - let is_develop = branch == "develop"; - if !is_work && !is_develop { + // Candidate parents are develop and work branches — decided by the + // taxonomy in BranchType, not a local prefix list. + let parsed = BranchType::parse(branch); + if parsed != BranchType::Develop && !parsed.is_work_branch() { continue; } let base = match git.merge_base(current, branch) { @@ -79,11 +82,11 @@ fn detect_parent_branch(git: &dyn Git, current: &str) -> Result }); let labels: Vec<&str> = candidates.iter().map(|(name, _)| name.as_str()).collect(); - let idx = menu::show_select("PR target branch", &labels)?; + let idx = prompter.select("PR target branch", &labels)?; Ok(candidates[idx].0.clone()) } -pub fn finish_work_branch(git: &dyn Git, hosting: &dyn HostingPlatform, branch_type: &BranchType, breaking: Option, base: Option) -> Result<(), String> { +pub fn finish_work_branch(git: &dyn Git, hosting: &dyn HostingPlatform, prompter: &dyn Prompter, branch_type: &BranchType, breaking: Option, base: Option, template: Option<&Path>) -> Result<(), String> { let commit_type = branch_type.commit_type().ok_or("Cannot finish: not on a work branch")?; let name = branch_type.name().ok_or("Cannot finish: branch has no name")?; let current = git.current_branch()?; @@ -99,14 +102,14 @@ pub fn finish_work_branch(git: &dyn Git, hosting: &dyn HostingPlatform, branch_t } base } - None => detect_parent_branch(git, ¤t)?, + None => detect_parent_branch(git, prompter, ¤t)?, }; let bang = match breaking { // Explicit flag always honored, for any work type Some(b) => b, // Prompt only for types that are commonly breaking - None if commonly_breaking(commit_type) => prompt_breaking_change()?, + None if commonly_breaking(commit_type) => prompt_breaking_change(prompter)?, None => false, }; @@ -116,30 +119,30 @@ pub fn finish_work_branch(git: &dyn Git, hosting: &dyn HostingPlatform, branch_t format!("{commit_type}: {name}") }; - push_and_create_pr(git, hosting, &base, &title, branch_type) + push_and_create_pr(git, hosting, &base, &title, template) } fn commonly_breaking(commit_type: &str) -> bool { matches!(commit_type, "feat" | "fix" | "refactor") } -fn prompt_breaking_change() -> Result { - let idx = menu::show_select("Contains breaking changes?", &["no", "yes"])?; +fn prompt_breaking_change(prompter: &dyn Prompter) -> Result { + let idx = prompter.select("Contains breaking changes?", &["no", "yes"])?; Ok(idx == 1) } -pub fn finish_release_fix(git: &dyn Git, hosting: &dyn HostingPlatform, branch_type: &BranchType) -> Result<(), String> { +pub fn finish_release_fix(git: &dyn Git, hosting: &dyn HostingPlatform, branch_type: &BranchType, template: Option<&Path>) -> Result<(), String> { let BranchType::ReleaseFix { major, minor, patch, name } = branch_type else { return Err("Cannot finish: not on a release-fix branch".to_string()); }; let title = format!("fix: {}", name.replace('-', " ")); - push_and_create_pr(git, hosting, &format!("release/{major}.{minor}.{patch}"), &title, branch_type) + push_and_create_pr(git, hosting, &SemVer::new(*major, *minor, *patch).release_branch(), &title, template) } -pub fn finish_hotfix_fix(git: &dyn Git, hosting: &dyn HostingPlatform, branch_type: &BranchType) -> Result<(), String> { +pub fn finish_hotfix_fix(git: &dyn Git, hosting: &dyn HostingPlatform, branch_type: &BranchType, template: Option<&Path>) -> Result<(), String> { let BranchType::HotfixFix { major, minor, patch, name } = branch_type else { return Err("Cannot finish: not on a hotfix-fix branch".to_string()); }; let title = format!("fix: {}", name.replace('-', " ")); - push_and_create_pr(git, hosting, &format!("hotfix/{major}.{minor}.{patch}"), &title, branch_type) + push_and_create_pr(git, hosting, &SemVer::new(*major, *minor, *patch).hotfix_branch(), &title, template) } diff --git a/src/flows/mod.rs b/src/flows/mod.rs index aac1a3b..4cb4313 100644 --- a/src/flows/mod.rs +++ b/src/flows/mod.rs @@ -3,6 +3,88 @@ pub mod finish_work; pub mod finish_release; pub mod finish_hotfix; +use crate::git::Git; + +/// Open `{prefix}/*` branches (e.g. `release/*`), in the order git returns them. +/// The glob itself can never match `{prefix}-fix/*` (`release-` ≠ `release/`); +/// the prefix filter keeps that guarantee for mocks, which return their +/// configured list regardless of the pattern. +pub(crate) fn branches_with_prefix(git: &dyn Git, prefix: &str) -> Result, String> { + let matching = git.list_branches_matching(&format!("{prefix}/*"))?; + let with_slash = format!("{prefix}/"); + Ok(matching.into_iter().filter(|b| b.starts_with(&with_slash)).collect()) +} + +// --- Idempotent finish steps ----------------------------------------------- +// Shared scaffolding of the release/hotfix finish flows: each step is guarded +// by a real-state predicate and prints a visible "↷ skipped:" line on resume +// (see decisions.md, Resume & Idempotency). Commit messages and conflict help +// stay caller-supplied — wording is per-flow UX, not shared knowledge. The two +// flows themselves are deliberately NOT unified: the RC gate, release +// propagation, and messaging genuinely differ. + +/// Merge `source` into `target` (checkout, ff-sync to origin, no-ff merge), +/// skipped entirely when `source` is already an ancestor of `target`. +pub(crate) fn merge_into(git: &dyn Git, source: &str, target: &str, message: &str, conflict_help: &str) -> Result<(), String> { + if git.is_ancestor(source, target)? { + println!("↷ skipped: merge into {target} (already merged)"); + return Ok(()); + } + println!("Merging into {target}..."); + git.checkout(target)?; + git.ff_merge(&format!("origin/{target}"))?; + git.merge(source, message).map_err(|e| format!("{e}\n{conflict_help}")) +} + +/// Push `branch` unless origin already has its HEAD. +pub(crate) fn push_if_needed(git: &dyn Git, branch: &str) -> Result<(), String> { + if git.is_pushed(branch)? { + println!("↷ skipped: push {branch} (already up to date)"); + return Ok(()); + } + git.push(branch) +} + +/// Create annotated tag `tag` unless it already exists locally. +pub(crate) fn tag_if_missing(git: &dyn Git, tag: &str, message: &str) -> Result<(), String> { + if git.tag_exists(tag)? { + println!("↷ skipped: tag {tag} (already exists)"); + return Ok(()); + } + println!("Tagging: {tag}"); + git.create_tag(tag, message) +} + +/// Push `tag` unless origin already has it. +pub(crate) fn push_tag_if_missing(git: &dyn Git, tag: &str) -> Result<(), String> { + if git.remote_tag_exists(tag)? { + println!("↷ skipped: push tag {tag} (already pushed)"); + return Ok(()); + } + git.push_tag(tag) +} + +/// Delete the finished source branch locally and remotely, both idempotent. +/// Switches to `main` first when HEAD is still on the branch — a resume that +/// skipped the develop merge leaves it there, git refuses to delete the +/// checked-out branch, and `main` is always safe (the work is merged there). +pub(crate) fn delete_source_branch(git: &dyn Git, branch: &str) -> Result<(), String> { + if git.current_branch()? == branch { + git.checkout("main")?; + } + if git.local_branch_exists(branch)? { + git.delete_branch_local(branch)?; + } else { + println!("↷ skipped: delete local {branch} (already gone)"); + } + if git.remote_branch_exists(branch)? { + git.delete_branch_remote(branch)?; + } else { + println!("↷ skipped: delete remote {branch} (already gone)"); + } + Ok(()) +} + /// Guidance appended to a merge conflict during a release/hotfix finish. /// /// Resume is branch-scoped: bflow only continues an interrupted finish when you diff --git a/src/flows/start.rs b/src/flows/start.rs index 71d016b..8c95fef 100644 --- a/src/flows/start.rs +++ b/src/flows/start.rs @@ -1,5 +1,5 @@ use crate::git::Git; -use crate::menu; +use crate::prompt::Prompter; use crate::version::SemVer; use crate::worktree::{open_worktree, WorktreeContext}; @@ -9,10 +9,18 @@ pub enum ReleaseType { Minor, } +/// An active worktree context leaves the current checkout untouched, exactly +/// like `--no-checkout`. This is the flows' single derivation of that rule +/// (pinned by the worktree tests: passing no_checkout=false with a worktree +/// context must still take the no-checkout path). +fn effective_no_checkout(no_checkout: bool, worktree: &Option>) -> bool { + no_checkout || worktree.is_some() +} + pub fn start_work_branch(git: &dyn Git, prefix: &str, name: &str, from: &str, no_checkout: bool, worktree: Option>) -> Result<(), String> { let branch = format!("{prefix}/{name}"); println!("Creating branch: {branch}"); - let effective_no_checkout = no_checkout || worktree.is_some(); + let effective_no_checkout = effective_no_checkout(no_checkout, &worktree); if effective_no_checkout { git.create_branch_no_checkout(&branch, from) } else { @@ -32,21 +40,18 @@ pub fn start_work_branch(git: &dyn Git, prefix: &str, name: &str, from: &str, no Ok(()) } -pub fn start_release(git: &dyn Git, release_type: Option) -> Result<(), String> { - resolve_or_create_release(git, release_type)?; +pub fn start_release(git: &dyn Git, prompter: &dyn Prompter, release_type: Option) -> Result<(), String> { + resolve_or_create_release(git, prompter, release_type)?; Ok(()) } pub fn start_release_fix(git: &dyn Git, name: &str, no_checkout: bool, worktree: Option>) -> Result<(), String> { - let effective_no_checkout = no_checkout || worktree.is_some(); + let effective_no_checkout = effective_no_checkout(no_checkout, &worktree); let release_branch = if effective_no_checkout { - let branches = git.list_branches_matching("release/*")?; - let release_branches: Vec<&String> = branches.iter() - .filter(|b| b.starts_with("release/") && !b.starts_with("release-fix/")) - .collect(); - release_branches.first() + super::branches_with_prefix(git, "release")? + .first() .ok_or("No release branch found. Create one with 'bflow start release' first.")? - .to_string() + .clone() } else { let current = git.current_branch()?; if current.strip_prefix("release/").is_none() { @@ -72,7 +77,7 @@ pub fn start_release_fix(git: &dyn Git, name: &str, no_checkout: bool, worktree: } pub fn start_hotfix_fix(git: &dyn Git, name: &str, no_checkout: bool, worktree: Option>) -> Result<(), String> { - let effective_no_checkout = no_checkout || worktree.is_some(); + let effective_no_checkout = effective_no_checkout(no_checkout, &worktree); let hotfix_branch = resolve_or_create_hotfix(git, effective_no_checkout)?; let version = hotfix_branch.strip_prefix("hotfix/").unwrap(); let branch = format!("hotfix-fix/{version}/{name}"); @@ -90,11 +95,8 @@ pub fn start_hotfix_fix(git: &dyn Git, name: &str, no_checkout: bool, worktree: Ok(()) } -fn resolve_or_create_release(git: &dyn Git, release_type: Option) -> Result { - let branches = git.list_branches_matching("release/*")?; - let release_branches: Vec<&String> = branches.iter() - .filter(|b| b.starts_with("release/") && !b.starts_with("release-fix/")) - .collect(); +fn resolve_or_create_release(git: &dyn Git, prompter: &dyn Prompter, release_type: Option) -> Result { + let release_branches = super::branches_with_prefix(git, "release")?; if let Some(branch) = release_branches.first() { println!("Using existing release branch: {branch}"); @@ -108,7 +110,7 @@ fn resolve_or_create_release(git: &dyn Git, release_type: Option) - Some(ReleaseType::Minor) => latest.bump_minor(), None => { let has_breaking = detect_breaking_changes(git, &latest); - prompt_release_type(&latest, has_breaking)? + prompt_release_type(prompter, &latest, has_breaking)? } }; @@ -173,7 +175,7 @@ pub(crate) fn message_is_breaking(msg: &str) -> bool { false } -fn prompt_release_type(latest: &SemVer, has_breaking: bool) -> Result { +fn prompt_release_type(prompter: &dyn Prompter, latest: &SemVer, has_breaking: bool) -> Result { let major_label = format!("major (v{} → v{})", latest, latest.bump_major()); let minor_label = format!("minor (v{} → v{})", latest, latest.bump_minor()); @@ -184,7 +186,7 @@ fn prompt_release_type(latest: &SemVer, has_breaking: bool) -> Result Result Result { - let branches = git.list_branches_matching("hotfix/*")?; - let hotfix_branches: Vec<&String> = branches.iter() - .filter(|b| b.starts_with("hotfix/") && !b.starts_with("hotfix-fix/")) - .collect(); + let hotfix_branches = super::branches_with_prefix(git, "hotfix")?; if let Some(branch) = hotfix_branches.first() { println!("Using existing hotfix branch: {branch}"); diff --git a/src/git/branch.rs b/src/git/branch.rs index 162a1d9..4b41368 100644 --- a/src/git/branch.rs +++ b/src/git/branch.rs @@ -15,6 +15,24 @@ pub enum BranchType { } impl BranchType { + /// Work-branch taxonomy — the single source of truth. `parse`, the menus, + /// and parent-branch detection all derive from this table; `work_kind` is + /// its compile-checked inverse. To add a work-branch type, follow the + /// "New work-branch type" recipe in decisions.md. + const WORK_TYPES: [(&'static str, fn(String) -> Self); 5] = [ + ("feature", |name| Self::Feature { name }), + ("fix", |name| Self::Fix { name }), + ("chore", |name| Self::Chore { name }), + ("docs", |name| Self::Docs { name }), + ("refactor", |name| Self::Refactor { name }), + ]; + + /// The work-branch kind names ("feature", "fix", …) in canonical order. + /// Branch prefix and menu label are the kind name itself. + pub fn work_kinds() -> [&'static str; 5] { + Self::WORK_TYPES.map(|(kind, _)| kind) + } + pub fn parse(branch: &str) -> Self { match branch { "main" | "master" => return Self::Main, @@ -22,14 +40,8 @@ impl BranchType { _ => {} } - for (prefix, constructor) in [ - ("feature/", Self::new_feature as fn(String) -> Self), - ("fix/", Self::new_fix as fn(String) -> Self), - ("chore/", Self::new_chore as fn(String) -> Self), - ("docs/", Self::new_docs as fn(String) -> Self), - ("refactor/", Self::new_refactor as fn(String) -> Self), - ] { - if let Some(name) = branch.strip_prefix(prefix) { + for (kind, constructor) in Self::WORK_TYPES { + if let Some(name) = branch.strip_prefix(kind).and_then(|rest| rest.strip_prefix('/')) { if !name.is_empty() { return constructor(name.to_string()); } @@ -71,14 +83,31 @@ impl BranchType { Self::Other } - pub fn commit_type(&self) -> Option<&'static str> { + /// Kind name ("feature", …) for work branches; the inverse of the + /// `WORK_TYPES` constructors. Deliberately exhaustive (no wildcard): + /// adding a `BranchType` variant fails compilation here until its kind — + /// or `None` — is decided, so parent-branch detection and the menus can + /// never silently miss a new type. + pub fn work_kind(&self) -> Option<&'static str> { match self { - Self::Feature { .. } => Some("feat"), + Self::Feature { .. } => Some("feature"), Self::Fix { .. } => Some("fix"), Self::Chore { .. } => Some("chore"), Self::Docs { .. } => Some("docs"), Self::Refactor { .. } => Some("refactor"), - _ => None, + Self::Main | Self::Develop + | Self::Release { .. } | Self::ReleaseFix { .. } + | Self::Hotfix { .. } | Self::HotfixFix { .. } + | Self::Other => None, + } + } + + /// Conventional-commit type for work branches: the kind name itself, + /// except the "feature" kind commits as "feat". + pub fn commit_type(&self) -> Option<&'static str> { + match self.work_kind()? { + "feature" => Some("feat"), + kind => Some(kind), } } @@ -92,7 +121,7 @@ impl BranchType { } pub fn is_work_branch(&self) -> bool { - matches!(self, Self::Feature { .. } | Self::Fix { .. } | Self::Chore { .. } | Self::Docs { .. } | Self::Refactor { .. }) + self.work_kind().is_some() } /// Branch types whose finish has a fixed merge/PR target, so `--base` never applies. @@ -117,12 +146,6 @@ impl BranchType { } } - fn new_feature(name: String) -> Self { Self::Feature { name } } - fn new_fix(name: String) -> Self { Self::Fix { name } } - fn new_chore(name: String) -> Self { Self::Chore { name } } - fn new_docs(name: String) -> Self { Self::Docs { name } } - fn new_refactor(name: String) -> Self { Self::Refactor { name } } - fn parse_major_minor_patch(s: &str) -> Option<(u32, u32, u32)> { let parts: Vec<&str> = s.splitn(3, '.').collect(); if parts.len() != 3 { return None; } @@ -134,6 +157,27 @@ impl BranchType { mod tests { use super::*; + #[test] + fn work_kinds_round_trip_through_parse() { + // The WORK_TYPES table and work_kind() are two directions of one + // mapping; this pins that they can never drift apart. + for kind in BranchType::work_kinds() { + let bt = BranchType::parse(&format!("{kind}/x")); + assert_eq!(bt.work_kind(), Some(kind), "kind {kind} must round-trip"); + assert!(bt.is_work_branch(), "kind {kind} must be a work branch"); + } + } + + #[test] + fn commit_type_is_kind_name_except_feature() { + assert_eq!(BranchType::parse("feature/x").commit_type(), Some("feat")); + assert_eq!(BranchType::parse("fix/x").commit_type(), Some("fix")); + assert_eq!(BranchType::parse("chore/x").commit_type(), Some("chore")); + assert_eq!(BranchType::parse("docs/x").commit_type(), Some("docs")); + assert_eq!(BranchType::parse("refactor/x").commit_type(), Some("refactor")); + assert_eq!(BranchType::parse("release/1.2.0").commit_type(), None); + } + #[test] fn pr_template_keys_for_work_branches() { assert_eq!(BranchType::parse("feature/x").pr_template_keys(), Some(("feature", "feature"))); diff --git a/src/git/mod.rs b/src/git/mod.rs index 24a712e..7a21adf 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -15,8 +15,10 @@ pub trait Git { fn push_tag(&self, tag: &str) -> Result<()>; fn create_tag(&self, tag: &str, message: &str) -> Result<()>; fn merge(&self, branch: &str, message: &str) -> Result<()>; - fn pull(&self, branch: &str) -> Result<()>; + fn ff_merge(&self, branch: &str) -> Result<()>; fn list_tags(&self) -> Result>; + /// Contract: returns a sorted, deduplicated list — consumers pick + /// candidates by position (`.first()`), so order must be deterministic. fn list_branches_matching(&self, pattern: &str) -> Result>; fn is_working_tree_clean(&self) -> Result; fn delete_branch_local(&self, branch: &str) -> Result<()>; @@ -25,8 +27,6 @@ pub trait Git { fn list_remote_branches(&self) -> Result>; fn merge_base(&self, a: &str, b: &str) -> Result; fn rev_list_count(&self, from: &str, to: &str) -> Result; - fn stash_push(&self) -> Result<()>; - fn stash_pop(&self) -> Result<()>; fn commit_messages(&self, from: &str, to: &str) -> Result>; // Idempotency primitives @@ -39,7 +39,6 @@ pub trait Git { fn is_mid_merge(&self) -> Result; fn has_unmerged_paths(&self) -> Result; fn git_dir(&self) -> Result; - fn rev_parse(&self, refname: &str) -> Result; /// URL of the `origin` remote (`git remote get-url origin`). Errors when the /// remote does not exist; callers decide how to handle a missing remote. @@ -98,6 +97,12 @@ impl GitCli { } } + /// Resolve a ref name to its SHA. Only needed internally (`is_pushed` + /// compares local vs remote SHAs), so not part of the `Git` port. + fn rev_parse(&self, refname: &str) -> Result { + self.run(&["rev-parse", refname]) + } + /// Run a `git config --get`-style command that uses exit 1 to mean "key not set". /// Returns `Ok(None)` on exit 1, `Ok(Some(value))` on exit 0, and an error otherwise. fn run_config(&self, args: &[&str]) -> Result> { @@ -129,7 +134,7 @@ impl Git for GitCli { fn push_tag(&self, tag: &str) -> Result<()> { self.run(&["push", "origin", tag]).map(|_| ()) } fn create_tag(&self, tag: &str, message: &str) -> Result<()> { self.run(&["tag", "-a", tag, "-m", message]).map(|_| ()) } fn merge(&self, branch: &str, message: &str) -> Result<()> { self.run(&["merge", branch, "--no-ff", "-m", message]).map(|_| ()) } - fn pull(&self, branch: &str) -> Result<()> { self.run(&["merge", branch, "--ff-only"]).map(|_| ()) } + fn ff_merge(&self, branch: &str) -> Result<()> { self.run(&["merge", branch, "--ff-only"]).map(|_| ()) } fn list_tags(&self) -> Result> { let output = self.run(&["tag", "--list"])?; Ok(output.lines().map(|s| s.to_string()).filter(|s| !s.is_empty()).collect()) @@ -141,13 +146,15 @@ impl Git for GitCli { "for-each-ref", "--format=%(refname:short)", &ref_pattern, &local_pattern, ])?; - Ok(output + // Sorted + deduped per the trait contract. + let mut branches: Vec = output .lines() .map(|s| s.trim_start_matches("origin/").to_string()) .filter(|s| !s.is_empty()) - .collect::>() - .into_iter() - .collect()) + .collect(); + branches.sort(); + branches.dedup(); + Ok(branches) } fn is_working_tree_clean(&self) -> Result { let output = self.run(&["status", "--porcelain"])?; @@ -175,8 +182,6 @@ impl Git for GitCli { let output = self.run(&["rev-list", "--count", &range])?; output.parse::().map_err(|e| format!("Failed to parse rev-list count: {e}")) } - fn stash_push(&self) -> Result<()> { self.run(&["stash", "push", "-u"]).map(|_| ()) } - fn stash_pop(&self) -> Result<()> { self.run(&["stash", "pop"]).map(|_| ()) } fn commit_messages(&self, from: &str, to: &str) -> Result> { let range = format!("{from}..{to}"); // Use NUL byte as separator — guaranteed not to appear in commit messages @@ -237,9 +242,6 @@ impl Git for GitCli { let s = self.run(&["rev-parse", "--git-dir"])?; Ok(PathBuf::from(s)) } - fn rev_parse(&self, refname: &str) -> Result { - self.run(&["rev-parse", refname]) - } fn remote_url(&self) -> Result { self.run(&["remote", "get-url", "origin"]) diff --git a/src/hosting/devops.rs b/src/hosting/devops.rs index dcb087f..341a325 100644 --- a/src/hosting/devops.rs +++ b/src/hosting/devops.rs @@ -1,4 +1,4 @@ -use super::{run_cli, HostingPlatform, Result}; +use super::{resolve_body_file, run_cli, HostingPlatform, Result}; pub struct AzureDevOps { org: String, @@ -82,8 +82,6 @@ impl HostingPlatform for AzureDevOps { return Ok(self.pr_url(validate_pr_id(&existing)?)); } - // A bflow-resolved template (branch-specific/group/default) wins; otherwise fall - // back to the repository's own default PR template, then to an empty body. let default_paths = [ ".azuredevops/pull_request_template.md", ".azuredevops/PULL_REQUEST_TEMPLATE.md", @@ -92,9 +90,7 @@ impl HostingPlatform for AzureDevOps { "PULL_REQUEST_TEMPLATE.md", "docs/pull_request_template.md", ]; - let body_file = template - .map(|p| p.to_string()) - .or_else(|| default_paths.iter().find(|p| std::path::Path::new(p).exists()).map(|p| p.to_string())); + let body_file = resolve_body_file(template, &default_paths); let body = match body_file { Some(path) => std::fs::read_to_string(&path) .map_err(|e| format!("Failed to read PR template '{path}': {e}"))?, diff --git a/src/hosting/github.rs b/src/hosting/github.rs index 1a5f4e3..057a684 100644 --- a/src/hosting/github.rs +++ b/src/hosting/github.rs @@ -1,4 +1,4 @@ -use super::{run_cli, HostingPlatform, Result}; +use super::{resolve_body_file, run_cli, HostingPlatform, Result}; pub struct GitHub; @@ -16,13 +16,19 @@ impl GitHub { impl HostingPlatform for GitHub { fn create_or_get_pr(&self, head: &str, base: &str, title: &str, template: Option<&str>) -> Result { - let existing = self.run_gh(&["pr", "view", head, "--json", "url,state", "--jq", "select(.state == \"OPEN\") | .url"]); - if let Ok(url) = existing { - if !url.is_empty() { return Ok(url); } + match self.run_gh(&["pr", "view", head, "--json", "url,state", "--jq", "select(.state == \"OPEN\") | .url"]) { + Ok(url) if !url.is_empty() => return Ok(url), + // A closed/merged PR filters to empty output — create a new one. + Ok(_) => {} + // gh exits non-zero both when no PR exists (normal here) and on real + // failures (auth expiry, network). Only the former may be swallowed. + Err(e) if e.contains("no pull requests found") => {} + Err(e) => return Err(format!( + "Could not check for an existing PR: {e}\n\ + If authentication expired, run 'gh auth login', then re-run 'bflow finish'." + )), } - // A bflow-resolved template (branch-specific/group/default) wins; otherwise fall - // back to the repository's own default PR template, then to an empty body. let git_default_paths = [ ".github/PULL_REQUEST_TEMPLATE.md", ".github/pull_request_template.md", @@ -30,9 +36,7 @@ impl HostingPlatform for GitHub { "pull_request_template.md", "docs/pull_request_template.md", ]; - let body_file = template - .map(|p| p.to_string()) - .or_else(|| git_default_paths.iter().find(|p| std::path::Path::new(p).exists()).map(|p| p.to_string())); + let body_file = resolve_body_file(template, &git_default_paths); if let Some(path) = body_file { self.run_gh(&["pr", "create", "--head", head, "--base", base, "--title", title, "--body-file", &path]) diff --git a/src/hosting/mod.rs b/src/hosting/mod.rs index f26df9f..5c66d73 100644 --- a/src/hosting/mod.rs +++ b/src/hosting/mod.rs @@ -31,6 +31,36 @@ fn run_cli(program: &str, args: &[impl AsRef]) -> Result { } } +/// PR-body precedence shared by all providers: a bflow-resolved template wins, +/// else the first existing native default-template path, else `None` (empty +/// body). The native path *lists* stay per-provider knowledge on purpose. +fn resolve_body_file(template: Option<&str>, native_paths: &[&str]) -> Option { + template + .map(|p| p.to_string()) + .or_else(|| { + native_paths.iter() + .find(|p| std::path::Path::new(p).exists()) + .map(|p| p.to_string()) + }) +} + +#[cfg(test)] +mod tests { + use super::resolve_body_file; + + #[test] + fn bflow_template_wins_over_native_paths() { + // The native path need not even exist for the template to win. + let result = resolve_body_file(Some(".github/pr-templates/bflow-fix.md"), &["nope.md"]); + assert_eq!(result, Some(".github/pr-templates/bflow-fix.md".to_string())); + } + + #[test] + fn no_template_and_no_existing_native_path_is_empty_body() { + assert_eq!(resolve_body_file(None, &["definitely/not/a/real/path.md"]), None); + } +} + /// Open a URL in the OS default browser (platform dispatch, provider-agnostic). pub fn open_in_browser(url: &str) -> Result<()> { use std::process::Command; diff --git a/src/hosting/template.rs b/src/hosting/template.rs index 950d888..1be1639 100644 --- a/src/hosting/template.rs +++ b/src/hosting/template.rs @@ -14,9 +14,12 @@ use crate::git::branch::BranchType; const DIR: &str = ".github/pr-templates"; -/// Resolve the PR template for `branch_type` against the conventional repo location. -pub fn resolve(branch_type: &BranchType) -> Option { - resolve_in(Path::new(DIR), branch_type) +/// Resolve the PR template for `branch_type` against the conventional location +/// inside `repo_root`. Anchoring to the repo root (not the process CWD) keeps +/// resolution working from subdirectories; called from the composition root so +/// flows never probe the filesystem themselves. +pub fn resolve(repo_root: &Path, branch_type: &BranchType) -> Option { + resolve_in(&repo_root.join(DIR), branch_type) } /// Resolution against an explicit directory — kept separate so tests can point at a @@ -36,23 +39,27 @@ fn resolve_in(dir: &Path, branch_type: &BranchType) -> Option { #[cfg(test)] mod tests { use super::*; - use std::env; use std::fs; - use std::sync::atomic::{AtomicU64, Ordering}; - - static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); fn tmp_dir() -> PathBuf { - let n = TMP_COUNTER.fetch_add(1, Ordering::SeqCst); - let dir = env::temp_dir().join(format!("bflow-template-test-{}-{n}", std::process::id())); - fs::create_dir_all(&dir).unwrap(); - dir + crate::test_support::tmp_dir("bflow-template-test") } fn touch(dir: &Path, name: &str) { fs::write(dir.join(name), "body").unwrap(); } + #[test] + fn resolve_is_anchored_to_the_repo_root() { + let root = tmp_dir(); + let dir = root.join(".github/pr-templates"); + fs::create_dir_all(&dir).unwrap(); + touch(&dir, "bflow-default.md"); + let bt = BranchType::parse("feature/foo"); + assert_eq!(resolve(&root, &bt), Some(dir.join("bflow-default.md"))); + fs::remove_dir_all(&root).ok(); + } + #[test] fn specific_wins_over_group_and_default() { let dir = tmp_dir(); diff --git a/src/lib.rs b/src/lib.rs index e2e18b0..a1fcc79 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,9 +1,14 @@ +pub mod action; pub mod cli; pub mod editor; pub mod flows; pub mod git; pub mod hosting; +pub mod lifecycle; pub mod menu; +pub mod prompt; pub mod state; +#[cfg(test)] +pub(crate) mod test_support; pub mod version; pub mod worktree; diff --git a/src/lifecycle.rs b/src/lifecycle.rs new file mode 100644 index 0000000..ceb5aae --- /dev/null +++ b/src/lifecycle.rs @@ -0,0 +1,354 @@ +//! Cross-cutting lifecycle of a bflow invocation: resume lookup, action +//! resolution, the reject → stash → write-state → dispatch ordering contract, +//! state clearing, and the three-way stash-pop policy (see decisions.md, +//! State & Crash-Safety). Lives in the library — not `main.rs` — so the +//! crash-safety ordering is enforced by tests (`tests/lifecycle_test.rs`), +//! not by a comment. `main.rs` keeps only adapter construction and preflight. + +use crate::action::Action; +use crate::cli::{resolve_action, Commands}; +use crate::editor::Editor; +use crate::flows::{finish_hotfix, finish_release, finish_work, start}; +use crate::git::branch::BranchType; +use crate::git::Git; +use crate::hosting::HostingPlatform; +use crate::menu; +use crate::prompt::Prompter; +use crate::state::{current_timestamp, FinishKind, FinishState}; +use crate::worktree::{WorktreeConfig, WorktreeContext}; + +pub fn run( + git: &dyn Git, + hosting: &dyn HostingPlatform, + prompter: &dyn Prompter, + editor: &dyn Editor, + wt_config: &WorktreeConfig, + command: Option, +) -> Result<(), String> { + let branch_name = git.current_branch()?; + let git_dir = git.git_dir()?; + + // One-time upgrade of any pre-2.4 global state file into the per-branch folder. + FinishState::migrate_legacy(&git_dir)?; + + let branch_type = BranchType::parse(&branch_name); + + // Resume context: an in-progress finish only resumes when you are standing on + // the source branch that started it. From develop/main/feature branches there + // is no resume — bflow behaves normally — so a stalled finish never hijacks + // other work. To continue after a conflict you switch back to the source + // branch and re-run 'bflow finish'. + let resume_state = match finish_identity(&branch_type) { + Some((kind, major, minor, patch)) => FinishState::load(&git_dir, kind, major, minor, patch)?, + None => None, + }; + + // Resolve the action up-front so we can decide whether to fetch / stash / etc. + let action = resolve_action_with_state(command, &branch_type, &branch_name, resume_state.as_ref(), wt_config.enabled)?; + + // --abort short-circuits before any state-changing operation, even if the repo + // is mid-merge — abort is itself a recovery action. + if matches!(action, Action::AbortFinish) { + return handle_abort(&git_dir, resume_state); + } + + // Mid-merge / unmerged-paths preflight (all other paths). + if git.is_mid_merge()? || git.has_unmerged_paths()? { + return Err(unresolved_merge_message(resume_state.as_ref())); + } + + println!("Fetching latest..."); + git.fetch()?; + + // Optional worktree flow: when enabled (and not opted out) for an eligible start, + // treat it like --no-checkout so the current working tree is left untouched and the + // new branch is free to be checked out in its own worktree. + let worktree_active = wt_config.enabled && !action.no_worktree() && action.worktree_eligible(); + + let no_checkout = action.no_checkout() || worktree_active; + let is_finish_with_state = matches!(action, Action::FinishRelease | Action::FinishHotfix); + let needs_stash = !no_checkout && branch_type != BranchType::Other && !git.is_working_tree_clean()?; + + // Reject dirty-tree finishes BEFORE any side effects (stash, state write). + // Start actions and resumes get to stash/inherit; everything else must be clean. + if needs_stash && !action.is_start() && resume_state.is_none() { + return Err("Working tree is not clean. Commit your changes before finishing.".to_string()); + } + + // Stash if needed. On resume, inherit the prior stash ref from state. + let stash_msg = if resume_state.is_some() { + resume_state.as_ref().and_then(|s| s.stash_ref.clone()) + } else if needs_stash { + println!("Stashing uncommitted changes..."); + let msg = format!("bflow-finish:{branch_name}:{}", current_timestamp()); + git.stash_push_with_message(&msg)?; + Some(msg) + } else { + None + }; + + // Write state file BEFORE the first side effect of a release/hotfix finish. + if is_finish_with_state && resume_state.is_none() { + write_state_for_action(&action, &branch_type, &git_dir, stash_msg.clone())?; + } + + let result = run_flow(git, hosting, prompter, &branch_type, &branch_name, &action, no_checkout, worktree_active, wt_config, editor, resume_state.as_ref()); + + // Lifecycle: clear state on success of a release/hotfix finish. Both a fresh + // finish and a resume run on the source branch, so its identity is available. + if result.is_ok() && (is_finish_with_state || resume_state.is_some()) { + if let Some((kind, major, minor, patch)) = finish_identity(&branch_type) { + FinishState::clear(&git_dir, kind, major, minor, patch)?; + } + } + + // Stash pop policy: + // - On success: always pop (changes restored). + // - On failure of a release/hotfix finish: leave stash for resume. + // - On failure of any other action: pop (preserves prior bflow behavior of + // restoring the user's working tree even on errors). + let keep_stash_for_resume = result.is_err() && (is_finish_with_state || resume_state.is_some()); + if let Some(msg) = &stash_msg { + if keep_stash_for_resume { + eprintln!("Your uncommitted changes remain stashed as '{msg}'. They will be restored on a successful 'bflow finish' resume (or after 'bflow finish --abort')."); + } else { + println!("Restoring uncommitted changes..."); + match git.find_stash_by_message(msg) { + Ok(Some(stash_ref)) => { + if let Err(e) = git.stash_pop_ref(&stash_ref) { + eprintln!("Warning: Failed to restore stashed changes: {e}"); + eprintln!("Your changes are saved in git stash ({stash_ref}). Run 'git stash pop {stash_ref}' to restore them."); + } + } + Ok(None) => {} // already gone + Err(e) => eprintln!("Warning: Could not look up stash by message: {e}"), + } + } + } + + result +} + +/// Decide which Action to run given the parsed command, current branch, and +/// any resume state. Resume state takes precedence over branch-based dispatch +/// for `bflow finish` (and the default interactive path) — a develop-merge +/// conflict leaves HEAD on develop, where the branch-eligibility check would +/// otherwise reject the resume with "Nothing to finish on this branch." +pub fn resolve_action_with_state( + command: Option, + branch_type: &BranchType, + branch_name: &str, + resume_state: Option<&FinishState>, + worktree_enabled: bool, +) -> Result { + // `--abort` wins unconditionally and never errors based on branch type. + if let Some(Commands::Finish { abort: true, .. }) = &command { + return Ok(Action::AbortFinish); + } + + // For `bflow finish` (or the default interactive path), an in-progress finish + // state takes precedence over branch-based dispatch. This state is only ever + // present when standing on the source branch (resume is branch-scoped), so it + // covers the case where a develop-merge conflict was resolved and the user has + // switched back to the release/hotfix branch to continue. + // An explicit --base never applies here: resume state only exists on + // release/hotfix source branches (see finish_identity), whose finishes have a + // fixed target. Skip the resume shortcut so resolve_action rejects the flag + // instead of silently ignoring it. + let has_explicit_base = matches!(&command, Some(Commands::Finish { base: Some(_), .. })); + let is_finish_or_default = matches!(command, Some(Commands::Finish { .. }) | None); + if is_finish_or_default && !has_explicit_base { + if let Some(state) = resume_state { + eprintln!( + "↻ Resuming in-progress {} finish for {} (started_at={}). Use 'bflow finish --abort' to discard.", + state.kind.as_str(), + state.source_branch(), + state.started_at, + ); + return Ok(match state.kind { + FinishKind::Release => Action::FinishRelease, + FinishKind::Hotfix => Action::FinishHotfix, + }); + } + } + + // No resume state (or a non-finish command): fall through to normal dispatch. + match command { + None => menu::show_menu(branch_type, branch_name), + Some(cmd) => resolve_action(cmd, branch_type, worktree_enabled), + } +} + +/// Map a source branch to the (kind, version) identity of its finish state. +/// Only release and hotfix branches carry finish state; everything else (develop, +/// main, feature/*) yields None and therefore never resumes. +fn finish_identity(branch_type: &BranchType) -> Option<(FinishKind, u32, u32, u32)> { + match branch_type { + BranchType::Release { major, minor, patch } => { + Some((FinishKind::Release, *major, *minor, *patch)) + } + BranchType::Hotfix { major, minor, patch } => { + Some((FinishKind::Hotfix, *major, *minor, *patch)) + } + _ => None, + } +} + +fn write_state_for_action( + action: &Action, + branch_type: &BranchType, + git_dir: &std::path::Path, + stash_ref: Option, +) -> Result<(), String> { + // The action decides *whether* state is written; the branch supplies the + // identity via the same finish_identity mapping the resume lookup uses, + // so the two can never encode the branch→identity rule differently. + let expected_kind = match action { + Action::FinishRelease => FinishKind::Release, + Action::FinishHotfix => FinishKind::Hotfix, + _ => return Ok(()), + }; + let Some((kind, major, minor, patch)) = finish_identity(branch_type) else { + return Ok(()); + }; + if kind != expected_kind { + return Ok(()); + } + FinishState { kind, major, minor, patch, started_at: current_timestamp(), stash_ref }.save(git_dir) +} + +fn handle_abort(git_dir: &std::path::Path, state: Option) -> Result<(), String> { + match state { + None => { + println!("No in-progress finish to abort."); + Ok(()) + } + Some(s) => { + println!("Aborting in-progress {} finish for {} (started_at={}).", + s.kind.as_str(), s.source_branch(), s.started_at); + FinishState::clear(git_dir, s.kind, s.major, s.minor, s.patch)?; + if let Some(msg) = &s.stash_ref { + println!("Your original uncommitted changes are still stashed as '{msg}'."); + println!("Run 'git stash list' to find it, then 'git stash pop ' to restore."); + } + Ok(()) + } + } +} + +fn unresolved_merge_message(resume_state: Option<&FinishState>) -> String { + let mut msg = String::from( + "Unresolved merge in progress. Resolve conflicts, run 'git commit', then re-run 'bflow finish'." + ); + if let Some(s) = resume_state { + msg.push_str(&format!( + "\n(In-progress {} finish for {} is waiting for resume.)", + s.kind.as_str(), s.source_branch(), + )); + } + msg +} + +#[allow(clippy::too_many_arguments)] +fn run_flow( + git: &dyn Git, + hosting: &dyn HostingPlatform, + prompter: &dyn Prompter, + branch_type: &BranchType, + branch_name: &str, + action: &Action, + skip_current_branch_sync: bool, + worktree_active: bool, + wt_config: &WorktreeConfig, + editor: &dyn Editor, + resume_state: Option<&FinishState>, +) -> Result<(), String> { + // Fast-forward the current branch to origin when the flow will operate on + // this checkout and we're not resuming (on resume the user may be on + // main/develop after conflict resolution — syncing that branch is harmless + // but produces noisy output). + if !skip_current_branch_sync && resume_state.is_none() { + if let Err(e) = git.ff_merge(&format!("origin/{branch_name}")) { + if !e.contains("not something we can merge") { + return Err(e); + } + } + } + + match action { + Action::StartWorkBranch { prefix, name, from, no_checkout, .. } => { + let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; + start::start_work_branch(git, prefix, name, from, *no_checkout, wt)?; + } + Action::StartRelease(release_type) => { + start::start_release(git, prompter, *release_type)?; + } + Action::StartReleaseFix { name, no_checkout, .. } => { + let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; + start::start_release_fix(git, name, *no_checkout, wt)?; + } + Action::StartHotfixFix { name, no_checkout, .. } => { + let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; + start::start_hotfix_fix(git, name, *no_checkout, wt)?; + } + Action::FinishWorkBranch { breaking, base } => { + let template = resolve_pr_template(git, branch_type)?; + finish_work::finish_work_branch(git, hosting, prompter, branch_type, *breaking, base.clone(), template.as_deref())?; + } + Action::FinishReleaseFix => { + let template = resolve_pr_template(git, branch_type)?; + finish_work::finish_release_fix(git, hosting, branch_type, template.as_deref())?; + } + Action::FinishHotfixFix => { + let template = resolve_pr_template(git, branch_type)?; + finish_work::finish_hotfix_fix(git, hosting, branch_type, template.as_deref())?; + } + Action::BumpVersion => { + let BranchType::Release { major, minor, .. } = branch_type else { + unreachable!("BumpVersion action only from Release branch"); + }; + finish_release::bump_version(git, *major, *minor)?; + } + Action::SyncWithDevelop => { + let BranchType::Release { major, minor, .. } = branch_type else { + unreachable!("SyncWithDevelop action only from Release branch"); + }; + finish_release::sync_with_develop(git, *major, *minor)?; + } + Action::FinishRelease => { + // On resume, prefer the state's version (we may not be on the release branch). + let (major, minor) = if let Some(s) = resume_state { + (s.major, s.minor) + } else { + let BranchType::Release { major, minor, .. } = branch_type else { + unreachable!("FinishRelease action only from Release branch"); + }; + (*major, *minor) + }; + finish_release::finish_release(git, major, minor)?; + } + Action::FinishHotfix => { + let (major, minor, patch) = if let Some(s) = resume_state { + (s.major, s.minor, s.patch) + } else { + let BranchType::Hotfix { major, minor, patch } = branch_type else { + unreachable!("FinishHotfix action only from Hotfix branch"); + }; + (*major, *minor, *patch) + }; + finish_hotfix::finish_hotfix(git, major, minor, patch)?; + } + Action::AbortFinish => { + unreachable!("AbortFinish is handled before run_flow"); + } + } + + Ok(()) +} + +/// Resolve the PR template at the composition boundary, anchored to the repo +/// root — resolution keeps working from subdirectories, and flows never probe +/// the filesystem themselves (they receive the resolved path as a parameter). +fn resolve_pr_template(git: &dyn Git, branch_type: &BranchType) -> Result, String> { + Ok(crate::hosting::template::resolve(&git.repo_root()?, branch_type)) +} diff --git a/src/main.rs b/src/main.rs index 5c3f587..cf5cc1a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,19 +2,17 @@ use std::process::{Command, ExitCode}; use clap::Parser; -use bflow::cli::{Commands, WorktreeAction, resolve_action}; +use bflow::cli::{Commands, WorktreeAction}; use bflow::git::GitCli; use bflow::git::Git; -use bflow::git::branch::BranchType; use bflow::hosting::detect::{self, Provider}; use bflow::hosting::devops::AzureDevOps; use bflow::hosting::github::GitHub; use bflow::hosting::HostingPlatform; -use bflow::menu::{self, Action}; -use bflow::flows::{start, finish_work, finish_release, finish_hotfix}; -use bflow::state::{FinishState, FinishKind, current_timestamp}; -use bflow::editor::{CommandEditor, Editor}; -use bflow::worktree::{self, WorktreeConfig, WorktreeContext}; +use bflow::lifecycle; +use bflow::menu::MenuPrompter; +use bflow::editor::CommandEditor; +use bflow::worktree::{self, WorktreeConfig}; #[derive(Parser)] #[command(name = "bflow", version, about = "Beans GitFlow - customized gitflow workflow CLI")] @@ -33,6 +31,8 @@ fn main() -> ExitCode { ExitCode::SUCCESS } +/// Composition root: build adapters, run preflight, hand off to the lifecycle +/// (which lives in the library so its crash-safety ordering is testable). fn run(command: Option) -> Result<(), String> { check_command_exists("git")?; let git = GitCli::new(); @@ -47,331 +47,14 @@ fn run(command: Option) -> Result<(), String> { }; // Provider detection reads the origin remote, so the repo check comes first. - let branch_name = git.current_branch().map_err(|_| { - "Not in a git repository.".to_string() - })?; + git.current_branch().map_err(|_| "Not in a git repository.".to_string())?; let hosting = create_hosting(&git)?; - let git_dir = git.git_dir()?; - - // One-time upgrade of any pre-2.4 global state file into the per-branch folder. - FinishState::migrate_legacy(&git_dir)?; - - let branch_type = BranchType::parse(&branch_name); - - // Resume context: an in-progress finish only resumes when you are standing on - // the source branch that started it. From develop/main/feature branches there - // is no resume — bflow behaves normally — so a stalled finish never hijacks - // other work. To continue after a conflict you switch back to the source - // branch and re-run 'bflow finish'. - let resume_state = match finish_identity(&branch_type) { - Some((kind, major, minor, patch)) => FinishState::load(&git_dir, kind, major, minor, patch)?, - None => None, - }; - - // Load the worktree config BEFORE resolving the action: the release-fix/ - // hotfix-fix branch-type gate must know whether the worktree flow (which - // auto-discovers the target branch, like --no-checkout) will apply. let wt_config = WorktreeConfig::load(&git)?; - - // Resolve the action up-front so we can decide whether to fetch / stash / etc. - let action = resolve_action_with_state(command, &branch_type, &branch_name, resume_state.as_ref(), wt_config.enabled)?; - - // --abort short-circuits before any state-changing operation, even if the repo - // is mid-merge — abort is itself a recovery action. - if matches!(action, Action::AbortFinish) { - return handle_abort(&git_dir, resume_state); - } - - // Mid-merge / unmerged-paths preflight (all other paths). - if git.is_mid_merge()? || git.has_unmerged_paths()? { - return Err(unresolved_merge_message(resume_state.as_ref())); - } - - println!("Fetching latest..."); - git.fetch()?; - - // Optional worktree flow: when enabled (and not opted out) for an eligible start, - // treat it like --no-checkout so the current working tree is left untouched and the - // new branch is free to be checked out in its own worktree. let editor = CommandEditor::new(wt_config.editor.clone()); - let worktree_active = wt_config.enabled && !action.no_worktree() && action.worktree_eligible(); - - let no_checkout = action.no_checkout() || worktree_active; - let is_finish_with_state = matches!(action, Action::FinishRelease | Action::FinishHotfix); - let needs_stash = !no_checkout && branch_type != BranchType::Other && !git.is_working_tree_clean()?; - - // Reject dirty-tree finishes BEFORE any side effects (stash, state write). - // Start actions and resumes get to stash/inherit; everything else must be clean. - if needs_stash && !action.is_start() && resume_state.is_none() { - return Err("Working tree is not clean. Commit your changes before finishing.".to_string()); - } - - // Stash if needed. On resume, inherit the prior stash ref from state. - let stash_msg = if resume_state.is_some() { - resume_state.as_ref().and_then(|s| s.stash_ref.clone()) - } else if needs_stash { - println!("Stashing uncommitted changes..."); - let msg = format!("bflow-finish:{branch_name}:{}", current_timestamp()); - git.stash_push_with_message(&msg)?; - Some(msg) - } else { - None - }; + let prompter = MenuPrompter; - // Write state file BEFORE the first side effect of a release/hotfix finish. - if is_finish_with_state && resume_state.is_none() { - write_state_for_action(&action, &branch_type, &git_dir, stash_msg.clone())?; - } - - let result = run_flow(&git, &*hosting, &branch_type, &branch_name, &action, no_checkout, worktree_active, &wt_config, &editor, resume_state.as_ref()); - - // Lifecycle: clear state on success of a release/hotfix finish. Both a fresh - // finish and a resume run on the source branch, so its identity is available. - if result.is_ok() && (is_finish_with_state || resume_state.is_some()) { - if let Some((kind, major, minor, patch)) = finish_identity(&branch_type) { - FinishState::clear(&git_dir, kind, major, minor, patch)?; - } - } - - // Stash pop policy: - // - On success: always pop (changes restored). - // - On failure of a release/hotfix finish: leave stash for resume. - // - On failure of any other action: pop (preserves prior bflow behavior of - // restoring the user's working tree even on errors). - let keep_stash_for_resume = result.is_err() && (is_finish_with_state || resume_state.is_some()); - if let Some(msg) = &stash_msg { - if keep_stash_for_resume { - eprintln!("Your uncommitted changes remain stashed as '{msg}'. They will be restored on a successful 'bflow finish' resume (or after 'bflow finish --abort')."); - } else { - println!("Restoring uncommitted changes..."); - match git.find_stash_by_message(msg) { - Ok(Some(stash_ref)) => { - if let Err(e) = git.stash_pop_ref(&stash_ref) { - eprintln!("Warning: Failed to restore stashed changes: {e}"); - eprintln!("Your changes are saved in git stash ({stash_ref}). Run 'git stash pop {stash_ref}' to restore them."); - } - } - Ok(None) => {} // already gone - Err(e) => eprintln!("Warning: Could not look up stash by message: {e}"), - } - } - } - - result -} - -/// Decide which Action to run given the parsed command, current branch, and -/// any resume state. Resume state takes precedence over branch-based dispatch -/// for `bflow finish` (and the default interactive path) — a develop-merge -/// conflict leaves HEAD on develop, where the branch-eligibility check would -/// otherwise reject the resume with "Nothing to finish on this branch." -fn resolve_action_with_state( - command: Option, - branch_type: &BranchType, - branch_name: &str, - resume_state: Option<&FinishState>, - worktree_enabled: bool, -) -> Result { - // `--abort` wins unconditionally and never errors based on branch type. - if let Some(Commands::Finish { abort: true, .. }) = &command { - return Ok(Action::AbortFinish); - } - - // For `bflow finish` (or the default interactive path), an in-progress finish - // state takes precedence over branch-based dispatch. This state is only ever - // present when standing on the source branch (resume is branch-scoped), so it - // covers the case where a develop-merge conflict was resolved and the user has - // switched back to the release/hotfix branch to continue. - // An explicit --base never applies here: resume state only exists on - // release/hotfix source branches (see finish_identity), whose finishes have a - // fixed target. Skip the resume shortcut so resolve_action rejects the flag - // instead of silently ignoring it. - let has_explicit_base = matches!(&command, Some(Commands::Finish { base: Some(_), .. })); - let is_finish_or_default = matches!(command, Some(Commands::Finish { .. }) | None); - if is_finish_or_default && !has_explicit_base { - if let Some(state) = resume_state { - eprintln!( - "↻ Resuming in-progress {} finish for {} (started_at={}). Use 'bflow finish --abort' to discard.", - state.kind.as_str(), - state.source_branch(), - state.started_at, - ); - return Ok(match state.kind { - FinishKind::Release => Action::FinishRelease, - FinishKind::Hotfix => Action::FinishHotfix, - }); - } - } - - // No resume state (or a non-finish command): fall through to normal dispatch. - match command { - None => menu::show_menu(branch_type, branch_name), - Some(cmd) => resolve_action(cmd, branch_type, worktree_enabled), - } -} - -/// Map a source branch to the (kind, version) identity of its finish state. -/// Only release and hotfix branches carry finish state; everything else (develop, -/// main, feature/*) yields None and therefore never resumes. -fn finish_identity(branch_type: &BranchType) -> Option<(FinishKind, u32, u32, u32)> { - match branch_type { - BranchType::Release { major, minor, patch } => { - Some((FinishKind::Release, *major, *minor, *patch)) - } - BranchType::Hotfix { major, minor, patch } => { - Some((FinishKind::Hotfix, *major, *minor, *patch)) - } - _ => None, - } -} - -fn write_state_for_action( - action: &Action, - branch_type: &BranchType, - git_dir: &std::path::Path, - stash_ref: Option, -) -> Result<(), String> { - let state = match (action, branch_type) { - (Action::FinishRelease, BranchType::Release { major, minor, patch }) => FinishState { - kind: FinishKind::Release, - major: *major, minor: *minor, patch: *patch, - started_at: current_timestamp(), - stash_ref, - }, - (Action::FinishHotfix, BranchType::Hotfix { major, minor, patch }) => FinishState { - kind: FinishKind::Hotfix, - major: *major, minor: *minor, patch: *patch, - started_at: current_timestamp(), - stash_ref, - }, - _ => return Ok(()), - }; - state.save(git_dir) -} - -fn handle_abort(git_dir: &std::path::Path, state: Option) -> Result<(), String> { - match state { - None => { - println!("No in-progress finish to abort."); - Ok(()) - } - Some(s) => { - println!("Aborting in-progress {} finish for {} (started_at={}).", - s.kind.as_str(), s.source_branch(), s.started_at); - FinishState::clear(git_dir, s.kind, s.major, s.minor, s.patch)?; - if let Some(msg) = &s.stash_ref { - println!("Your original uncommitted changes are still stashed as '{msg}'."); - println!("Run 'git stash list' to find it, then 'git stash pop ' to restore."); - } - Ok(()) - } - } -} - -fn unresolved_merge_message(resume_state: Option<&FinishState>) -> String { - let mut msg = String::from( - "Unresolved merge in progress. Resolve conflicts, run 'git commit', then re-run 'bflow finish'." - ); - if let Some(s) = resume_state { - msg.push_str(&format!( - "\n(In-progress {} finish for {} is waiting for resume.)", - s.kind.as_str(), s.source_branch(), - )); - } - msg -} - -#[allow(clippy::too_many_arguments)] -fn run_flow( - git: &GitCli, - hosting: &dyn HostingPlatform, - branch_type: &BranchType, - branch_name: &str, - action: &Action, - no_checkout: bool, - worktree_active: bool, - wt_config: &WorktreeConfig, - editor: &dyn Editor, - resume_state: Option<&FinishState>, -) -> Result<(), String> { - // Pull the current branch when it's a real bflow branch and we're not resuming - // a flow (on resume the user may be on main/develop after conflict resolution — - // pulling that branch is harmless but produces noisy output). - if !no_checkout && resume_state.is_none() { - if let Err(e) = git.pull(&format!("origin/{branch_name}")) { - if !e.contains("not something we can merge") { - return Err(e); - } - } - } - - match action { - Action::StartWorkBranch { prefix, name, from, no_checkout, .. } => { - let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; - start::start_work_branch(git, prefix, name, from, *no_checkout, wt)?; - } - Action::StartRelease(release_type) => { - start::start_release(git, *release_type)?; - } - Action::StartReleaseFix { name, no_checkout, .. } => { - let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; - start::start_release_fix(git, name, *no_checkout, wt)?; - } - Action::StartHotfixFix { name, no_checkout, .. } => { - let wt = if worktree_active { Some(WorktreeContext { config: wt_config, editor }) } else { None }; - start::start_hotfix_fix(git, name, *no_checkout, wt)?; - } - Action::FinishWorkBranch { breaking, base } => { - finish_work::finish_work_branch(git, hosting, branch_type, *breaking, base.clone())?; - } - Action::FinishReleaseFix => { - finish_work::finish_release_fix(git, hosting, branch_type)?; - } - Action::FinishHotfixFix => { - finish_work::finish_hotfix_fix(git, hosting, branch_type)?; - } - Action::BumpVersion => { - let BranchType::Release { major, minor, .. } = branch_type else { - unreachable!("BumpVersion action only from Release branch"); - }; - finish_release::bump_version(git, *major, *minor)?; - } - Action::SyncWithDevelop => { - let BranchType::Release { major, minor, .. } = branch_type else { - unreachable!("SyncWithDevelop action only from Release branch"); - }; - finish_release::sync_with_develop(git, *major, *minor)?; - } - Action::FinishRelease => { - // On resume, prefer the state's version (we may not be on the release branch). - let (major, minor) = if let Some(s) = resume_state { - (s.major, s.minor) - } else { - let BranchType::Release { major, minor, .. } = branch_type else { - unreachable!("FinishRelease action only from Release branch"); - }; - (*major, *minor) - }; - finish_release::finish_release(git, major, minor)?; - } - Action::FinishHotfix => { - let (major, minor, patch) = if let Some(s) = resume_state { - (s.major, s.minor, s.patch) - } else { - let BranchType::Hotfix { major, minor, patch } = branch_type else { - unreachable!("FinishHotfix action only from Hotfix branch"); - }; - (*major, *minor, *patch) - }; - finish_hotfix::finish_hotfix(git, major, minor, patch)?; - } - Action::AbortFinish => { - unreachable!("AbortFinish is handled before run_flow"); - } - } - - Ok(()) + lifecycle::run(&git, &*hosting, &prompter, &editor, &wt_config, command) } /// Detect the hosting provider for this repo and return a ready-to-use, @@ -415,43 +98,3 @@ fn check_command_exists(cmd: &str) -> Result<(), String> { .map_err(|_| format!("'{cmd}' is not installed or not in PATH."))?; Ok(()) } - -#[cfg(test)] -mod tests { - use super::*; - - fn release_state() -> FinishState { - FinishState { - kind: FinishKind::Release, - major: 2, - minor: 5, - patch: 0, - started_at: "0".to_string(), - stash_ref: None, - } - } - - #[test] - fn resume_state_resumes_finish_without_base() { - let branch_type = BranchType::Release { major: 2, minor: 5, patch: 0 }; - let state = release_state(); - let cmd = Some(Commands::Finish { breaking: None, base: None, abort: false }); - - let action = resolve_action_with_state(cmd, &branch_type, "release/2.5.0", Some(&state), false).unwrap(); - - assert!(matches!(action, Action::FinishRelease)); - } - - #[test] - fn explicit_base_rejected_even_when_resume_state_exists() { - // Regression: the fixed-target --base guard lives in resolve_action, which the - // resume early-return used to skip — silently swallowing an invalid --base. - let branch_type = BranchType::Release { major: 2, minor: 5, patch: 0 }; - let state = release_state(); - let cmd = Some(Commands::Finish { breaking: None, base: Some("develop".to_string()), abort: false }); - - let err = resolve_action_with_state(cmd, &branch_type, "release/2.5.0", Some(&state), false).unwrap_err(); - - assert!(err.contains("--base"), "Expected the fixed-target --base error, got: {err}"); - } -} diff --git a/src/menu.rs b/src/menu.rs index 50552ae..2ebbddf 100644 --- a/src/menu.rs +++ b/src/menu.rs @@ -5,69 +5,17 @@ use crossterm::{ style::{self, Stylize}, terminal, }; -use crate::flows::start::ReleaseType; +use crate::action::{validate_branch_name, Action}; use crate::git::branch::BranchType; +use crate::prompt::Prompter; -#[derive(Debug, Clone, Copy)] -pub enum DevelopOption { - StartFeature, StartFix, StartChore, StartDocs, StartRefactor, StartRelease, -} - -impl DevelopOption { - pub fn label(&self) -> &'static str { - match self { - Self::StartFeature => "start feature", - Self::StartFix => "start fix", - Self::StartChore => "start chore", - Self::StartDocs => "start docs", - Self::StartRefactor => "start refactor", - Self::StartRelease => "start release", - } - } - - pub fn branch_prefix(&self) -> &'static str { - match self { - Self::StartFeature => "feature", - Self::StartFix => "fix", - Self::StartChore => "chore", - Self::StartDocs => "docs", - Self::StartRefactor => "refactor", - Self::StartRelease => unreachable!(), - } - } - - const ALL: [Self; 6] = [Self::StartFeature, Self::StartFix, Self::StartChore, Self::StartDocs, Self::StartRefactor, Self::StartRelease]; -} - -#[derive(Debug, Clone, Copy)] -pub enum WorkBranchOption { - Finish, StartFeature, StartFix, StartChore, StartDocs, StartRefactor, -} - -impl WorkBranchOption { - pub fn label(&self, branch_type: &str) -> String { - match self { - Self::Finish => format!("finish {branch_type}"), - Self::StartFeature => "start feature".to_string(), - Self::StartFix => "start fix".to_string(), - Self::StartChore => "start chore".to_string(), - Self::StartDocs => "start docs".to_string(), - Self::StartRefactor => "start refactor".to_string(), - } - } +/// The real `Prompter`: the interactive select menu on stderr. +pub struct MenuPrompter; - pub fn branch_prefix(&self) -> &'static str { - match self { - Self::StartFeature => "feature", - Self::StartFix => "fix", - Self::StartChore => "chore", - Self::StartDocs => "docs", - Self::StartRefactor => "refactor", - Self::Finish => unreachable!(), - } +impl Prompter for MenuPrompter { + fn select(&self, prompt: &str, items: &[&str]) -> Result { + show_select(prompt, items) } - - const ALL: [Self; 6] = [Self::Finish, Self::StartFeature, Self::StartFix, Self::StartChore, Self::StartDocs, Self::StartRefactor]; } #[derive(Debug, Clone, Copy)] @@ -88,6 +36,26 @@ impl ReleaseOption { const ALL: [Self; 4] = [Self::FinishRelease, Self::StartReleaseFix, Self::BumpVersion, Self::SyncWithDevelop]; } +/// Enables raw mode on construction; restores the terminal (cursor visible, raw +/// mode off) on drop. Every exit path — success, error, Ctrl-C/Esc abort — runs +/// the same cleanup structurally, so the documented "no raw-mode leak" invariant +/// is enforced by the type, not by hand-written cleanup at each return. +struct TerminalGuard; + +impl TerminalGuard { + fn enter(context: &str) -> Result { + terminal::enable_raw_mode().map_err(|e| format!("{context}: {e}"))?; + Ok(Self) + } +} + +impl Drop for TerminalGuard { + fn drop(&mut self) { + let _ = execute!(io::stderr(), cursor::Show); + let _ = terminal::disable_raw_mode(); + } +} + fn render_menu(out: &mut io::Stderr, items: &[&str], selected: usize) -> io::Result<()> { for (i, item) in items.iter().enumerate() { let number = i + 1; @@ -129,27 +97,16 @@ pub fn show_select(prompt: &str, items: &[&str]) -> Result { style::Print("\n"), ).map_err(|e| format!("Menu error: {e}"))?; - terminal::enable_raw_mode().map_err(|e| format!("Menu error: {e}"))?; + let _guard = TerminalGuard::enter("Menu error")?; - // Hide cursor during selection - execute!(out, cursor::Hide).map_err(|e| { - let _ = terminal::disable_raw_mode(); - format!("Menu error: {e}") - })?; + // Hide cursor during selection; the guard re-shows it on every exit path. + execute!(out, cursor::Hide).map_err(|e| format!("Menu error: {e}"))?; // Initial render - render_menu(&mut out, items, selected).map_err(|e| { - let _ = execute!(out, cursor::Show); - let _ = terminal::disable_raw_mode(); - format!("Menu error: {e}") - })?; + render_menu(&mut out, items, selected).map_err(|e| format!("Menu error: {e}"))?; let result = loop { - let ev = event::read().map_err(|e| { - let _ = execute!(out, cursor::Show); - let _ = terminal::disable_raw_mode(); - format!("Menu error: {e}") - })?; + let ev = event::read().map_err(|e| format!("Menu error: {e}"))?; // On Windows, crossterm emits Press + Release events; only handle Press let Event::Key(KeyEvent { kind: KeyEventKind::Press, code, modifiers, .. }) = ev else { @@ -158,8 +115,6 @@ pub fn show_select(prompt: &str, items: &[&str]) -> Result { match (code, modifiers) { (KeyCode::Char('c'), KeyModifiers::CONTROL) | (KeyCode::Esc, _) => { - let _ = execute!(out, cursor::Show); - let _ = terminal::disable_raw_mode(); return Err("Aborted".to_string()); } (KeyCode::Enter, _) => { @@ -194,42 +149,20 @@ pub fn show_select(prompt: &str, items: &[&str]) -> Result { } // Redraw: move cursor up to start of menu, then re-render - let reposition = (|| -> io::Result<()> { - if items.len() > 1 { - execute!(out, cursor::MoveUp((items.len() - 1) as u16))?; - } - execute!(out, cursor::MoveToColumn(0))?; - Ok(()) - })(); - if let Err(e) = reposition { - let _ = execute!(out, cursor::Show); - let _ = terminal::disable_raw_mode(); - return Err(format!("Menu error: {e}")); + if items.len() > 1 { + execute!(out, cursor::MoveUp((items.len() - 1) as u16)) + .map_err(|e| format!("Menu error: {e}"))?; } - render_menu(&mut out, items, selected).map_err(|e| { - let _ = execute!(out, cursor::Show); - let _ = terminal::disable_raw_mode(); - format!("Menu error: {e}") - })?; + execute!(out, cursor::MoveToColumn(0)).map_err(|e| format!("Menu error: {e}"))?; + render_menu(&mut out, items, selected).map_err(|e| format!("Menu error: {e}"))?; }; - // Cleanup: show cursor, disable raw mode, move past menu - let _ = execute!(out, cursor::Show, style::Print("\r\n")); - let _ = terminal::disable_raw_mode(); + // Move past the menu; the guard restores cursor + raw mode on drop. + let _ = execute!(out, style::Print("\r\n")); Ok(result) } -pub fn validate_branch_name(input: &str) -> Result<(), String> { - if input.is_empty() { - return Err("Name cannot be empty".to_string()); - } - if input.contains("..") || input.contains('~') || input.contains('^') || input.contains(':') || input.contains('\\') { - return Err("Invalid branch name. Avoid special characters (.. ~ ^ : \\)".to_string()); - } - Ok(()) -} - /// Print `prompt` and read a line of input in raw mode. Shared scaffolding for /// `prompt_name`/`prompt_line`: prompt printing, raw-mode lifecycle, the /// Windows Press-filter, Ctrl-C/Esc abort, Enter, and backspace handling all @@ -245,13 +178,10 @@ fn read_raw_line(prompt: &str, transform: impl Fn(&str, char) -> Option) - style::Print(format!("{prompt}: ")), ).map_err(|e| format!("Input error: {e}"))?; - terminal::enable_raw_mode().map_err(|e| format!("Input error: {e}"))?; + let _guard = TerminalGuard::enter("Input error")?; let result = loop { - let ev = event::read().map_err(|e| { - let _ = terminal::disable_raw_mode(); - format!("Input error: {e}") - })?; + let ev = event::read().map_err(|e| format!("Input error: {e}"))?; // On Windows, crossterm emits Press + Release events; only handle Press let Event::Key(KeyEvent { kind: KeyEventKind::Press, code, modifiers, .. }) = ev else { @@ -260,7 +190,6 @@ fn read_raw_line(prompt: &str, transform: impl Fn(&str, char) -> Option) - match (code, modifiers) { (KeyCode::Char('c'), KeyModifiers::CONTROL) | (KeyCode::Esc, _) => { - let _ = terminal::disable_raw_mode(); return Err("Aborted".to_string()); } (KeyCode::Enter, _) => break input, @@ -279,8 +208,8 @@ fn read_raw_line(prompt: &str, transform: impl Fn(&str, char) -> Option) - } }; + // Move to the next line; the guard disables raw mode on drop. let _ = execute!(out, cursor::MoveToNextLine(1)); - let _ = terminal::disable_raw_mode(); Ok(result) } @@ -325,42 +254,40 @@ pub fn show_menu(branch_type: &BranchType, current_branch: &str) -> Result { - let labels: Vec<&str> = DevelopOption::ALL.iter().map(|o| o.label()).collect(); - let idx = show_select("What would you like to do?", &labels)?; - let option = DevelopOption::ALL[idx]; - match option { - DevelopOption::StartRelease => Ok(Action::StartRelease(None)), - other => { - let name = prompt_name(&format!("Name for {} branch", other.branch_prefix()))?; - Ok(Action::StartWorkBranch { prefix: other.branch_prefix().to_string(), name, from: "develop".to_string(), no_checkout: false, no_worktree: false }) + // "start " for every work-branch kind, then "start release". + let kinds = BranchType::work_kinds(); + let mut labels: Vec = kinds.iter().map(|k| format!("start {k}")).collect(); + labels.push("start release".to_string()); + let label_refs: Vec<&str> = labels.iter().map(String::as_str).collect(); + let idx = show_select("What would you like to do?", &label_refs)?; + match kinds.get(idx) { + Some(kind) => { + let name = prompt_name(&format!("Name for {kind} branch"))?; + Ok(Action::StartWorkBranch { prefix: kind.to_string(), name, from: "develop".to_string(), no_checkout: false, no_worktree: false }) } + None => Ok(Action::StartRelease(None)), } } BranchType::Feature { .. } | BranchType::Fix { .. } | BranchType::Chore { .. } | BranchType::Docs { .. } | BranchType::Refactor { .. } => { - let branch_type_label = match branch_type { - BranchType::Feature { .. } => "feature", - BranchType::Fix { .. } => "fix", - BranchType::Chore { .. } => "chore", - BranchType::Docs { .. } => "docs", - BranchType::Refactor { .. } => "refactor", - _ => unreachable!(), - }; - let labels: Vec = WorkBranchOption::ALL.iter().map(|o| o.label(branch_type_label)).collect(); - let label_refs: Vec<&str> = labels.iter().map(|s| s.as_str()).collect(); + let current_kind = branch_type.work_kind() + .expect("this match arm only accepts work branches"); + // "finish ", then "start " for every kind. + let kinds = BranchType::work_kinds(); + let mut labels: Vec = vec![format!("finish {current_kind}")]; + labels.extend(kinds.iter().map(|k| format!("start {k}"))); + let label_refs: Vec<&str> = labels.iter().map(String::as_str).collect(); let idx = show_select("What would you like to do?", &label_refs)?; - let option = WorkBranchOption::ALL[idx]; - match option { - WorkBranchOption::Finish => Ok(Action::FinishWorkBranch { breaking: None, base: None }), - other => { - let name = prompt_name(&format!("Name for {} branch", other.branch_prefix()))?; - let current_label = format!("{current_branch} (current)"); - let base_options: &[&str] = &[¤t_label, "develop"]; - let base_idx = show_select("Base branch", base_options)?; - let from = if base_idx == 0 { current_branch.to_string() } else { "develop".to_string() }; - Ok(Action::StartWorkBranch { prefix: other.branch_prefix().to_string(), name, from, no_checkout: false, no_worktree: false }) - } + if idx == 0 { + return Ok(Action::FinishWorkBranch { breaking: None, base: None }); } + let kind = kinds[idx - 1]; + let name = prompt_name(&format!("Name for {kind} branch"))?; + let current_label = format!("{current_branch} (current)"); + let base_options: &[&str] = &[¤t_label, "develop"]; + let base_idx = show_select("Base branch", base_options)?; + let from = if base_idx == 0 { current_branch.to_string() } else { "develop".to_string() }; + Ok(Action::StartWorkBranch { prefix: kind.to_string(), name, from, no_checkout: false, no_worktree: false }) } BranchType::ReleaseFix { .. } => { show_select("What would you like to do?", &["finish release fix"])?; @@ -390,60 +317,3 @@ pub fn show_menu(branch_type: &BranchType, current_branch: &str) -> Result Err("Not on a recognized gitflow branch. Switch to main or develop first.".to_string()), } } - -#[derive(Debug, PartialEq)] -pub enum Action { - StartWorkBranch { prefix: String, name: String, from: String, no_checkout: bool, no_worktree: bool }, - StartRelease(Option), - StartReleaseFix { name: String, no_checkout: bool, no_worktree: bool }, - StartHotfixFix { name: String, no_checkout: bool, no_worktree: bool }, - FinishWorkBranch { breaking: Option, base: Option }, - FinishReleaseFix, - FinishRelease, - FinishHotfix, - FinishHotfixFix, - AbortFinish, - BumpVersion, - SyncWithDevelop, -} - -impl Action { - pub fn is_start(&self) -> bool { - matches!( - self, - Action::StartWorkBranch { .. } - | Action::StartRelease(_) - | Action::StartReleaseFix { .. } - | Action::StartHotfixFix { .. } - ) - } - - pub fn no_checkout(&self) -> bool { - match self { - Action::StartWorkBranch { no_checkout, .. } => *no_checkout, - Action::StartReleaseFix { no_checkout, .. } => *no_checkout, - Action::StartHotfixFix { no_checkout, .. } => *no_checkout, - _ => false, - } - } - - /// Whether this action is a named-work-branch start eligible for the worktree - /// flow. Deliberately excludes `StartRelease`, unlike `is_start`. - pub fn worktree_eligible(&self) -> bool { - matches!( - self, - Action::StartWorkBranch { .. } - | Action::StartReleaseFix { .. } - | Action::StartHotfixFix { .. } - ) - } - - pub fn no_worktree(&self) -> bool { - match self { - Action::StartWorkBranch { no_worktree, .. } => *no_worktree, - Action::StartReleaseFix { no_worktree, .. } => *no_worktree, - Action::StartHotfixFix { no_worktree, .. } => *no_worktree, - _ => false, - } - } -} diff --git a/src/prompt.rs b/src/prompt.rs new file mode 100644 index 0000000..cccb76a --- /dev/null +++ b/src/prompt.rs @@ -0,0 +1,9 @@ +//! Port for interactive selection. Flows depend on this trait instead of the +//! terminal-UI module, so business logic that needs a user decision stays +//! runnable against mocks without a TTY. The real implementation is +//! `menu::MenuPrompter`, wired in `main.rs` like the other adapters. + +pub trait Prompter { + /// Present `items` and return the index of the chosen one. + fn select(&self, prompt: &str, items: &[&str]) -> Result; +} diff --git a/src/state.rs b/src/state.rs index a615033..6d705d9 100644 --- a/src/state.rs +++ b/src/state.rs @@ -1,6 +1,8 @@ use std::fs; use std::path::{Path, PathBuf}; +use crate::version::SemVer; + /// Folder under `.git/` holding one state file per in-progress finish. pub const STATE_DIR_NAME: &str = "bflow-finish"; /// Pre-2.4 single global state file, migrated on startup if found. @@ -42,9 +44,10 @@ pub struct FinishState { impl FinishState { pub fn source_branch(&self) -> String { + let version = SemVer::new(self.major, self.minor, self.patch); match self.kind { - FinishKind::Release => format!("release/{}.{}.{}", self.major, self.minor, self.patch), - FinishKind::Hotfix => format!("hotfix/{}.{}.{}", self.major, self.minor, self.patch), + FinishKind::Release => version.release_branch(), + FinishKind::Hotfix => version.hotfix_branch(), } } @@ -188,20 +191,9 @@ pub fn current_timestamp() -> String { #[cfg(test)] mod tests { use super::*; - use std::env; - use std::sync::atomic::{AtomicU64, Ordering}; - - static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); fn tmp_dir() -> PathBuf { - let n = TMP_COUNTER.fetch_add(1, Ordering::SeqCst); - let dir = env::temp_dir().join(format!( - "bflow-state-test-{}-{}-{n}", - std::process::id(), - current_timestamp(), - )); - fs::create_dir_all(&dir).unwrap(); - dir + crate::test_support::tmp_dir("bflow-state-test") } fn release(major: u32, minor: u32, patch: u32) -> FinishState { diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 0000000..b896d8d --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,18 @@ +//! Shared infrastructure for inline unit tests — the one thing DAMP says to +//! DRY (mechanics, not scenarios). Compiled only for `cargo test`. + +use std::fs; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; + +static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); + +/// Create a unique, empty temp directory. `prefix` names the test area (e.g. +/// `bflow-state-test`); pid + a process-wide counter make the path unique +/// across parallel tests without any extra dependency. +pub(crate) fn tmp_dir(prefix: &str) -> PathBuf { + let n = TMP_COUNTER.fetch_add(1, Ordering::SeqCst); + let dir = std::env::temp_dir().join(format!("{prefix}-{}-{n}", std::process::id())); + fs::create_dir_all(&dir).unwrap(); + dir +} diff --git a/src/worktree.rs b/src/worktree.rs index 0fa1611..19cd158 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -25,6 +25,12 @@ fn scope_label(local: bool) -> &'static str { if local { "local (this repo)" } else { "global (all repos)" } } +/// An editor value that is blank or `none` (any case) means "don't open an editor". +fn editor_disabled(editor: &str) -> bool { + let editor = editor.trim(); + editor.is_empty() || editor.eq_ignore_ascii_case("none") +} + /// User configuration for the optional worktree flow, read from `bflow.worktree.*` /// git config keys. pub struct WorktreeConfig { @@ -120,8 +126,8 @@ pub fn open_worktree(git: &dyn Git, editor: &dyn Editor, config: &WorktreeConfig println!("Creating worktree: {}", path.display()); git.add_worktree(&path, branch)?; - let editor_cmd = config.editor.trim(); - if !editor_cmd.is_empty() && !editor_cmd.eq_ignore_ascii_case("none") { + if !editor_disabled(&config.editor) { + let editor_cmd = config.editor.trim(); println!("Opening in editor: {editor_cmd}"); if let Err(e) = editor.open(&path) { eprintln!("Warning: {e}. Worktree is ready at {}.", path.display()); @@ -172,7 +178,7 @@ pub fn show_status(git: &dyn Git) -> Result<()> { let cfg = WorktreeConfig::load(git)?; println!("Worktree flow configuration"); println!(" enabled : {}", cfg.enabled); - if cfg.editor.trim().is_empty() || cfg.editor.eq_ignore_ascii_case("none") { + if editor_disabled(&cfg.editor) { println!(" editor : {} (won't open an editor)", cfg.editor); } else { println!(" editor : {}", cfg.editor); diff --git a/tests/action_test.rs b/tests/action_test.rs index 4991b78..dad5abf 100644 --- a/tests/action_test.rs +++ b/tests/action_test.rs @@ -1,4 +1,4 @@ -use bflow::menu::Action; +use bflow::action::Action; #[test] fn start_actions_return_true() { diff --git a/tests/cli_test.rs b/tests/cli_test.rs index 60f2144..d2ac145 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -1,7 +1,7 @@ use bflow::cli::{Commands, StartKind, StartOptions, resolve_action}; use bflow::flows::start::ReleaseType; use bflow::git::branch::BranchType; -use bflow::menu::Action; +use bflow::action::Action; // --- Start work branch tests --- diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 073b917..7f3bb0f 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,9 +1,16 @@ +// Shared mocks for the integration-test suites. Each test crate compiles its +// own copy of this module and rarely uses every mock, so item-level dead_code +// warnings here are pure noise — silenced module-wide. +#![allow(dead_code)] + use std::cell::RefCell; -use std::collections::{HashMap, HashSet}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::path::{Path, PathBuf}; use bflow::editor::Editor; use bflow::git::Git; use bflow::hosting::HostingPlatform; +use bflow::prompt::Prompter; pub struct MockGit { pub calls: RefCell>, @@ -13,7 +20,11 @@ pub struct MockGit { pub branches_matching: Vec, pub remote_branches: Vec, pub merge_base_result: String, + /// Per-(a, b) merge bases; falls back to `merge_base_result` when absent. + pub merge_bases: HashMap<(String, String), String>, pub rev_list_count_result: u32, + /// Per-(from, to) counts; falls back to `rev_list_count_result` when absent. + pub rev_list_counts: HashMap<(String, String), u32>, pub commit_messages: Vec, /// Refs (the `to` arg) that should fail with an error. Used to simulate missing branches. pub fail_commit_messages_for: Vec, @@ -36,6 +47,8 @@ pub struct MockGit { pub pushed_branches: HashSet, pub mid_merge: bool, pub unmerged_paths: bool, + /// What `is_working_tree_clean` reports (defaults to clean). + pub working_tree_clean: bool, pub git_dir: PathBuf, /// Stash messages currently in the stash list (most recent first). pub stashes: RefCell>, @@ -57,7 +70,9 @@ impl MockGit { branches_matching: Vec::new(), remote_branches: Vec::new(), merge_base_result: "abc123".to_string(), + merge_bases: HashMap::new(), rev_list_count_result: 0, + rev_list_counts: HashMap::new(), commit_messages: Vec::new(), fail_commit_messages_for: Vec::new(), fail_nth_merge: None, @@ -70,6 +85,7 @@ impl MockGit { pushed_branches: HashSet::new(), mid_merge: false, unmerged_paths: false, + working_tree_clean: true, git_dir: PathBuf::from(".git"), stashes: RefCell::new(Vec::new()), config: HashMap::new(), @@ -134,8 +150,8 @@ impl Git for MockGit { Ok(()) } - fn pull(&self, branch: &str) -> Result<(), String> { - self.calls.borrow_mut().push(format!("pull:{branch}")); + fn ff_merge(&self, branch: &str) -> Result<(), String> { + self.calls.borrow_mut().push(format!("ff_merge:{branch}")); Ok(()) } @@ -151,7 +167,7 @@ impl Git for MockGit { fn is_working_tree_clean(&self) -> Result { self.calls.borrow_mut().push("is_working_tree_clean".to_string()); - Ok(true) + Ok(self.working_tree_clean) } fn delete_branch_local(&self, branch: &str) -> Result<(), String> { @@ -176,22 +192,17 @@ impl Git for MockGit { fn merge_base(&self, a: &str, b: &str) -> Result { self.calls.borrow_mut().push(format!("merge_base:{a}:{b}")); - Ok(self.merge_base_result.clone()) + Ok(self.merge_bases + .get(&(a.to_string(), b.to_string())) + .cloned() + .unwrap_or_else(|| self.merge_base_result.clone())) } fn rev_list_count(&self, from: &str, to: &str) -> Result { self.calls.borrow_mut().push(format!("rev_list_count:{from}:{to}")); - Ok(self.rev_list_count_result) - } - - fn stash_push(&self) -> Result<(), String> { - self.calls.borrow_mut().push("stash_push".to_string()); - Ok(()) - } - - fn stash_pop(&self) -> Result<(), String> { - self.calls.borrow_mut().push("stash_pop".to_string()); - Ok(()) + Ok(*self.rev_list_counts + .get(&(from.to_string(), to.to_string())) + .unwrap_or(&self.rev_list_count_result)) } fn commit_messages(&self, from: &str, to: &str) -> Result, String> { @@ -247,11 +258,6 @@ impl Git for MockGit { Ok(self.git_dir.clone()) } - fn rev_parse(&self, refname: &str) -> Result { - self.calls.borrow_mut().push(format!("rev_parse:{refname}")); - Ok("abc123".to_string()) - } - fn remote_url(&self) -> Result { self.calls.borrow_mut().push("remote_url".to_string()); Ok(self.remote_url.clone()) @@ -369,3 +375,47 @@ impl Editor for MockEditor { } } } + +/// Scripted `Prompter`: records every select as `select:{prompt}:[items]` and +/// answers from a queue. An unscripted select is an error, so a test proves a +/// flow never prompted simply by not scripting anything. +pub struct MockPrompter { + pub calls: RefCell>, + pub selections: RefCell>, +} + +impl MockPrompter { + pub fn new() -> Self { + Self { calls: RefCell::new(Vec::new()), selections: RefCell::new(VecDeque::new()) } + } + + pub fn scripted(selections: &[usize]) -> Self { + let p = Self::new(); + p.selections.borrow_mut().extend(selections.iter().copied()); + p + } + + pub fn calls(&self) -> Vec { + self.calls.borrow().clone() + } +} + +impl Prompter for MockPrompter { + fn select(&self, prompt: &str, items: &[&str]) -> Result { + self.calls.borrow_mut().push(format!("select:{prompt}:[{}]", items.join(", "))); + self.selections.borrow_mut().pop_front() + .ok_or_else(|| format!("MockPrompter: unscripted select('{prompt}')")) + } +} + +static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); + +/// Unique temp directory for integration tests that need a fake `.git` dir +/// (state files). Mirrors the lib's #[cfg(test)] helper, which integration +/// tests cannot link. +pub fn tmp_dir(prefix: &str) -> PathBuf { + let n = TMP_COUNTER.fetch_add(1, Ordering::SeqCst); + let dir = std::env::temp_dir().join(format!("{prefix}-{}-{n}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + dir +} diff --git a/tests/finish_hotfix_test.rs b/tests/finish_hotfix_test.rs index 5dfd585..45ec0a1 100644 --- a/tests/finish_hotfix_test.rs +++ b/tests/finish_hotfix_test.rs @@ -22,7 +22,7 @@ fn finish_hotfix_full_sequence() { assert_eq!(git.calls(), vec![ "is_ancestor:hotfix/1.0.1:main", "checkout:main", - "pull:origin/main", + "ff_merge:origin/main", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into main", "tag_exists:v1.0.1", "create_tag:v1.0.1:chore: hotfix 1.0.1", @@ -32,7 +32,7 @@ fn finish_hotfix_full_sequence() { "push_tag:v1.0.1", "is_ancestor:hotfix/1.0.1:develop", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into develop", "is_pushed:develop", "push:develop", @@ -54,7 +54,7 @@ fn finish_hotfix_different_version() { assert_eq!(git.calls(), vec![ "is_ancestor:hotfix/2.3.4:main", "checkout:main", - "pull:origin/main", + "ff_merge:origin/main", "merge:hotfix/2.3.4:chore: merge hotfix 2.3.4 into main", "tag_exists:v2.3.4", "create_tag:v2.3.4:chore: hotfix 2.3.4", @@ -64,7 +64,7 @@ fn finish_hotfix_different_version() { "push_tag:v2.3.4", "is_ancestor:hotfix/2.3.4:develop", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:hotfix/2.3.4:chore: merge hotfix 2.3.4 into develop", "is_pushed:develop", "push:develop", @@ -87,7 +87,7 @@ fn finish_hotfix_propagates_to_open_release_branch() { assert_eq!(git.calls(), vec![ "is_ancestor:hotfix/1.0.1:main", "checkout:main", - "pull:origin/main", + "ff_merge:origin/main", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into main", "tag_exists:v1.0.1", "create_tag:v1.0.1:chore: hotfix 1.0.1", @@ -97,14 +97,14 @@ fn finish_hotfix_propagates_to_open_release_branch() { "push_tag:v1.0.1", "is_ancestor:hotfix/1.0.1:develop", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into develop", "is_pushed:develop", "push:develop", "list_branches_matching:release/*", "is_ancestor:hotfix/1.0.1:release/1.2.0", "checkout:release/1.2.0", - "pull:origin/release/1.2.0", + "ff_merge:origin/release/1.2.0", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into release/1.2.0", "is_pushed:release/1.2.0", "push:release/1.2.0", @@ -132,13 +132,13 @@ fn finish_hotfix_propagates_to_multiple_release_branches_in_sorted_order() { "list_branches_matching:release/*", "is_ancestor:hotfix/1.0.1:release/1.5.0", "checkout:release/1.5.0", - "pull:origin/release/1.5.0", + "ff_merge:origin/release/1.5.0", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into release/1.5.0", "is_pushed:release/1.5.0", "push:release/1.5.0", "is_ancestor:hotfix/1.0.1:release/2.0.0", "checkout:release/2.0.0", - "pull:origin/release/2.0.0", + "ff_merge:origin/release/2.0.0", "merge:hotfix/1.0.1:chore: merge hotfix 1.0.1 into release/2.0.0", "is_pushed:release/2.0.0", "push:release/2.0.0", diff --git a/tests/finish_release_test.rs b/tests/finish_release_test.rs index 284da24..dfaa8c2 100644 --- a/tests/finish_release_test.rs +++ b/tests/finish_release_test.rs @@ -90,7 +90,7 @@ fn sync_with_develop_merges_and_returns_to_current() { assert_eq!(git.calls(), vec![ "current_branch", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:release/1.1.0:chore: sync release 1.1.0 with develop", "push:develop", "checkout:release/1.1.0", @@ -109,7 +109,7 @@ fn finish_release_creates_clean_tag_from_rc() { "is_ancestor:release/1.1.0:main", "rev_list_count:v1.1.0-rc.2:release/1.1.0", "checkout:main", - "pull:origin/main", + "ff_merge:origin/main", "merge:release/1.1.0:chore: merge release 1.1.0 into main", "tag_exists:v1.1.0", "create_tag:v1.1.0:chore: release 1.1.0", @@ -119,7 +119,7 @@ fn finish_release_creates_clean_tag_from_rc() { "push_tag:v1.1.0", "is_ancestor:release/1.1.0:develop", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:release/1.1.0:chore: merge release 1.1.0 into develop", "is_pushed:develop", "push:develop", @@ -143,7 +143,7 @@ fn finish_release_single_rc() { "is_ancestor:release/2.0.0:main", "rev_list_count:v2.0.0-rc.1:release/2.0.0", "checkout:main", - "pull:origin/main", + "ff_merge:origin/main", "merge:release/2.0.0:chore: merge release 2.0.0 into main", "tag_exists:v2.0.0", "create_tag:v2.0.0:chore: release 2.0.0", @@ -153,7 +153,7 @@ fn finish_release_single_rc() { "push_tag:v2.0.0", "is_ancestor:release/2.0.0:develop", "checkout:develop", - "pull:origin/develop", + "ff_merge:origin/develop", "merge:release/2.0.0:chore: merge release 2.0.0 into develop", "is_pushed:develop", "push:develop", diff --git a/tests/finish_work_test.rs b/tests/finish_work_test.rs index b0f3e01..6da7a84 100644 --- a/tests/finish_work_test.rs +++ b/tests/finish_work_test.rs @@ -1,6 +1,6 @@ mod common; -use common::{MockGit, MockHosting}; +use common::{MockGit, MockHosting, MockPrompter}; use bflow::flows::finish_work::{finish_release_fix, finish_hotfix_fix, finish_work_branch}; use bflow::git::branch::BranchType; @@ -11,7 +11,7 @@ fn finish_release_fix_pushes_and_creates_pr() { let hosting = MockHosting::new(); let branch_type = BranchType::ReleaseFix { major: 1, minor: 1, patch: 0, name: "login-bug".to_string() }; - finish_release_fix(&git, &hosting, &branch_type).unwrap(); + finish_release_fix(&git, &hosting, &branch_type, None).unwrap(); assert_eq!(git.calls(), vec![ "current_branch", @@ -31,7 +31,7 @@ fn finish_hotfix_fix_pushes_and_creates_pr() { let hosting = MockHosting::new(); let branch_type = BranchType::HotfixFix { major: 1, minor: 0, patch: 1, name: "crash-fix".to_string() }; - finish_hotfix_fix(&git, &hosting, &branch_type).unwrap(); + finish_hotfix_fix(&git, &hosting, &branch_type, None).unwrap(); assert_eq!(git.calls(), vec![ "current_branch", @@ -52,7 +52,7 @@ fn finish_release_fix_with_custom_pr_url() { hosting.pr_url = "https://github.com/org/repo/pull/42".to_string(); let branch_type = BranchType::ReleaseFix { major: 2, minor: 0, patch: 0, name: "typo".to_string() }; - finish_release_fix(&git, &hosting, &branch_type).unwrap(); + finish_release_fix(&git, &hosting, &branch_type, None).unwrap(); assert_eq!(hosting.calls(), vec![ "create_or_get_pr:release-fix/2.0.0/typo:release/2.0.0:fix: typo", @@ -69,7 +69,7 @@ fn finish_work_branch_feature_non_breaking() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "login".to_string() }; - finish_work_branch(&git, &hosting, &branch_type, Some(false), None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].starts_with("create_or_get_pr:feature/login:")); @@ -83,7 +83,7 @@ fn finish_work_branch_feature_breaking() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "remove-api".to_string() }; - finish_work_branch(&git, &hosting, &branch_type, Some(true), None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(true), None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].ends_with(":feat!: remove-api"), @@ -97,7 +97,7 @@ fn finish_work_branch_chore_breaking_honored() { let hosting = MockHosting::new(); let branch_type = BranchType::Chore { name: "drop-node-16".to_string() }; - finish_work_branch(&git, &hosting, &branch_type, Some(true), None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(true), None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].ends_with(":chore!: drop-node-16"), @@ -112,7 +112,7 @@ fn finish_work_branch_docs_defaults_to_non_breaking() { let branch_type = BranchType::Docs { name: "readme".to_string() }; // No flag (None) — docs should NOT prompt, should default to non-breaking - finish_work_branch(&git, &hosting, &branch_type, None, None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, None, None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].ends_with(":docs: readme"), @@ -127,7 +127,7 @@ fn finish_work_branch_with_explicit_base_skips_detection() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "login".to_string() }; - finish_work_branch(&git, &hosting, &branch_type, Some(false), Some("develop".to_string())).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), Some("develop".to_string()), None).unwrap(); let git_calls = git.calls(); assert!(!git_calls.contains(&"list_remote_branches".to_string()), @@ -147,7 +147,7 @@ fn finish_work_branch_with_local_only_base_errors() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "login".to_string() }; - let err = finish_work_branch(&git, &hosting, &branch_type, Some(false), Some("feature/auth".to_string())).unwrap_err(); + let err = finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), Some("feature/auth".to_string()), None).unwrap_err(); assert!(err.contains("feature/auth") && err.contains("origin"), "Error should name the branch and origin, got: {err}"); @@ -166,7 +166,7 @@ fn finish_work_branch_with_base_equal_to_current_errors() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "login".to_string() }; - let err = finish_work_branch(&git, &hosting, &branch_type, Some(false), Some("feature/login".to_string())).unwrap_err(); + let err = finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), Some("feature/login".to_string()), None).unwrap_err(); assert!(err.contains("feature/login"), "Error should name the branch, got: {err}"); assert!(hosting.calls().is_empty(), "No PR should be created when base == current"); @@ -179,7 +179,7 @@ fn finish_work_branch_with_unknown_base_errors() { let hosting = MockHosting::new(); let branch_type = BranchType::Feature { name: "login".to_string() }; - let err = finish_work_branch(&git, &hosting, &branch_type, Some(false), Some("no-such-branch".to_string())).unwrap_err(); + let err = finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), Some("no-such-branch".to_string()), None).unwrap_err(); assert!(err.contains("no-such-branch"), "Error should name the missing branch, got: {err}"); assert!(hosting.calls().is_empty(), "No PR should be created for an unknown base"); @@ -195,7 +195,7 @@ fn finish_work_branch_single_candidate_finishes_without_menu() { let branch_type = BranchType::Feature { name: "login".to_string() }; // Passing proves show_select was never reached: it has no TTY here. - finish_work_branch(&git, &hosting, &branch_type, Some(false), None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].starts_with("create_or_get_pr:feature/login:develop:"), @@ -209,9 +209,97 @@ fn finish_work_branch_fix_breaking() { let hosting = MockHosting::new(); let branch_type = BranchType::Fix { name: "auth".to_string() }; - finish_work_branch(&git, &hosting, &branch_type, Some(true), None).unwrap(); + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(true), None, None).unwrap(); let calls = hosting.calls(); assert!(calls[0].ends_with(":fix!: auth"), "Expected 'fix!: auth', got: {}", calls[0]); } + +#[test] +fn finish_work_branch_passes_resolved_template_to_hosting() { + let mut git = MockGit::new(); + git.current_branch = "feature/login".to_string(); + let hosting = MockHosting::new(); + let branch_type = BranchType::Feature { name: "login".to_string() }; + let template = std::path::Path::new(".github/pr-templates/bflow-feature.md"); + + finish_work_branch(&git, &hosting, &MockPrompter::new(), &branch_type, Some(false), None, Some(template)).unwrap(); + + let calls = hosting.calls(); + assert!(calls[0].ends_with(":template=.github/pr-templates/bflow-feature.md"), + "template path must reach the hosting platform verbatim, got: {}", calls[0]); +} + +// --- Parent-branch candidate ordering (reachable now that prompting goes +// --- through the Prompter port; previously required a TTY) --- + +/// Wire up a candidate: merge base with `current`, our distance since +/// divergence, and the candidate's own commit count since divergence. +fn add_candidate(git: &mut MockGit, current: &str, branch: &str, base: &str, ours: u32, theirs: u32) { + git.merge_bases.insert((current.to_string(), branch.to_string()), base.to_string()); + git.rev_list_counts.insert((base.to_string(), current.to_string()), ours); + git.rev_list_counts.insert((base.to_string(), branch.to_string()), theirs); +} + +#[test] +fn parent_candidates_sorted_by_merge_distance_ascending() { + let mut git = MockGit::new(); + git.current_branch = "feature/child".to_string(); + git.remote_branches = vec!["develop".to_string(), "feature/near".to_string()]; + // develop diverged 5 commits ago, feature/near only 2 — nearest first. + add_candidate(&mut git, "feature/child", "develop", "base-d", 5, 0); + add_candidate(&mut git, "feature/child", "feature/near", "base-n", 2, 0); + let hosting = MockHosting::new(); + let prompter = MockPrompter::scripted(&[0]); + let branch_type = BranchType::Feature { name: "child".to_string() }; + + finish_work_branch(&git, &hosting, &prompter, &branch_type, Some(false), None, None).unwrap(); + + assert_eq!(prompter.calls(), vec!["select:PR target branch:[feature/near, develop]"]); + assert!(hosting.calls()[0].starts_with("create_or_get_pr:feature/child:feature/near:"), + "choosing index 0 must target the nearest candidate, got: {}", hosting.calls()[0]); +} + +#[test] +fn parent_candidates_tie_prefers_develop_then_alphabetical() { + let mut git = MockGit::new(); + git.current_branch = "feature/child".to_string(); + git.remote_branches = vec![ + "feature/bbb".to_string(), + "develop".to_string(), + "feature/aaa".to_string(), + ]; + // All three candidates at the same distance. + add_candidate(&mut git, "feature/child", "feature/bbb", "base-b", 3, 0); + add_candidate(&mut git, "feature/child", "develop", "base-d", 3, 0); + add_candidate(&mut git, "feature/child", "feature/aaa", "base-a", 3, 0); + let hosting = MockHosting::new(); + let prompter = MockPrompter::scripted(&[0]); + let branch_type = BranchType::Feature { name: "child".to_string() }; + + finish_work_branch(&git, &hosting, &prompter, &branch_type, Some(false), None, None).unwrap(); + + assert_eq!(prompter.calls(), + vec!["select:PR target branch:[develop, feature/aaa, feature/bbb]"]); +} + +#[test] +fn parent_detection_excludes_child_branches_and_skips_menu_for_single_candidate() { + let mut git = MockGit::new(); + git.current_branch = "feature/parent".to_string(); + git.remote_branches = vec!["develop".to_string(), "feature/stacked".to_string()]; + add_candidate(&mut git, "feature/parent", "develop", "base-d", 4, 0); + // feature/stacked has MORE commits since divergence than we do — it + // branched from us, so it must not be offered as a PR target. + add_candidate(&mut git, "feature/parent", "feature/stacked", "base-s", 1, 6); + let hosting = MockHosting::new(); + let prompter = MockPrompter::new(); // unscripted: any select would error + let branch_type = BranchType::Feature { name: "parent".to_string() }; + + finish_work_branch(&git, &hosting, &prompter, &branch_type, Some(false), None, None).unwrap(); + + assert!(prompter.calls().is_empty(), "single surviving candidate must be auto-selected"); + assert!(hosting.calls()[0].starts_with("create_or_get_pr:feature/parent:develop:"), + "child branch must be excluded, got: {}", hosting.calls()[0]); +} diff --git a/tests/lifecycle_test.rs b/tests/lifecycle_test.rs new file mode 100644 index 0000000..f1436a7 --- /dev/null +++ b/tests/lifecycle_test.rs @@ -0,0 +1,250 @@ +mod common; + +use std::path::PathBuf; + +use common::{tmp_dir, MockEditor, MockGit, MockHosting, MockPrompter}; +use bflow::action::Action; +use bflow::cli::Commands; +use bflow::git::branch::BranchType; +use bflow::lifecycle::{resolve_action_with_state, run}; +use bflow::state::{FinishKind, FinishState}; +use bflow::worktree::WorktreeConfig; + +// The lifecycle (reject → stash → write-state → dispatch → clear/pop) used to +// live in main.rs, where tests/ could not link it — the system's most +// safety-critical ordering was enforced by a comment. These tests pin it. + +fn wt_config() -> WorktreeConfig { + WorktreeConfig { enabled: false, editor: "code".to_string(), base_path: None } +} + +fn finish_cmd() -> Option { + Some(Commands::Finish { breaking: None, base: None, abort: false }) +} + +/// A MockGit standing on release/2.5.0 with one RC tag and a fake `.git` dir, +/// ready for a `bflow finish`. +fn release_git() -> MockGit { + let mut git = MockGit::new(); + git.current_branch = "release/2.5.0".to_string(); + git.git_dir = tmp_dir("bflow-lifecycle-test"); + git.tags_on_branch = vec!["v2.5.0-rc.1".to_string()]; + git.existing_local_branches.insert("release/2.5.0".to_string()); + git.existing_remote_branches.insert("release/2.5.0".to_string()); + git +} + +fn state_path(git_dir: &PathBuf) -> PathBuf { + FinishState::path(git_dir, FinishKind::Release, 2, 5, 0) +} + +fn run_lifecycle(git: &MockGit, command: Option) -> Result<(), String> { + let hosting = MockHosting::new(); + let prompter = MockPrompter::new(); + let editor = MockEditor::new(); + run(git, &hosting, &prompter, &editor, &wt_config(), command) +} + +// --- State-before-mutation ordering --- + +#[test] +fn crashed_finish_leaves_resumable_state_on_disk() { + let mut git = release_git(); + git.fail_nth_merge = Some(1); // conflict on the main merge, mid-flow + + let result = run_lifecycle(&git, finish_cmd()); + + assert!(result.is_err(), "merge conflict must surface"); + let state = FinishState::load(&git.git_dir, FinishKind::Release, 2, 5, 0) + .unwrap() + .expect("state must have been written BEFORE the first mutating git call, so a crash mid-flow leaves a resumable record"); + assert_eq!(state.source_branch(), "release/2.5.0"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +#[test] +fn successful_finish_clears_state() { + let git = release_git(); + + run_lifecycle(&git, finish_cmd()).unwrap(); + + assert!(!state_path(&git.git_dir).exists(), + "state must be cleared after a successful finish"); + assert!(git.calls().iter().any(|c| c.starts_with("merge:release/2.5.0:")), + "the finish flow must actually have run"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +#[test] +fn dirty_tree_finish_rejected_before_any_side_effect() { + let mut git = release_git(); + git.working_tree_clean = false; + + let err = run_lifecycle(&git, finish_cmd()).unwrap_err(); + + assert!(err.contains("Working tree is not clean"), "got: {err}"); + let calls = git.calls(); + assert!(!calls.iter().any(|c| c.starts_with("stash_push_with_message")), + "nothing may be stashed before the reject; calls: {calls:?}"); + assert!(!state_path(&git.git_dir).exists(), + "no state may be written before the reject"); + assert!(!calls.iter().any(|c| c.starts_with("merge:")), + "no mutation may run; calls: {calls:?}"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +#[test] +fn mid_merge_preflight_blocks_and_names_pending_resume() { + let mut git = release_git(); + git.mid_merge = true; + // An in-progress finish is waiting. + FinishState { + kind: FinishKind::Release, major: 2, minor: 5, patch: 0, + started_at: "1".to_string(), stash_ref: None, + }.save(&git.git_dir).unwrap(); + + let err = run_lifecycle(&git, finish_cmd()).unwrap_err(); + + assert!(err.contains("Unresolved merge in progress"), "got: {err}"); + assert!(err.contains("release/2.5.0"), "must name the waiting finish; got: {err}"); + assert!(!git.calls().iter().any(|c| c == "fetch"), + "preflight must block before fetch"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +// --- Stash policy (dirty start: stash before mutation, pop on success) --- + +#[test] +fn dirty_start_stashes_before_mutating_and_pops_on_success() { + let mut git = MockGit::new(); + git.current_branch = "develop".to_string(); + git.git_dir = tmp_dir("bflow-lifecycle-test"); + git.working_tree_clean = false; + let start = { + use bflow::cli::{StartKind, StartOptions}; + Some(Commands::Start { kind: StartKind::Feature { + name: "login".to_string(), + base: "develop".to_string(), + opts: StartOptions { no_checkout: false, no_worktree: false }, + }}) + }; + + run_lifecycle(&git, start).unwrap(); + + let calls = git.calls(); + let stash_idx = calls.iter().position(|c| c.starts_with("stash_push_with_message:bflow-finish:develop:")) + .expect("dirty start must stash"); + let create_idx = calls.iter().position(|c| c.starts_with("create_branch:feature/login:")) + .expect("branch must be created"); + let pop_idx = calls.iter().position(|c| c.starts_with("stash_pop_ref:")) + .expect("stash must be popped on success"); + assert!(stash_idx < create_idx, "stash must precede the first mutation; calls: {calls:?}"); + assert!(pop_idx > create_idx, "pop must follow the flow; calls: {calls:?}"); + assert!(!state_path(&git.git_dir).exists(), + "start actions never write finish state"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +// --- Resume --- + +#[test] +fn resume_runs_flow_from_state_and_clears_it_on_success() { + // Fully-completed world: resume should re-drive the flow (all steps skip), + // then clear the state file. + let mut git = release_git(); + git.ancestors.insert(("release/2.5.0".to_string(), "main".to_string())); + git.ancestors.insert(("release/2.5.0".to_string(), "develop".to_string())); + git.existing_tags.insert("v2.5.0".to_string()); + git.existing_remote_tags.insert("v2.5.0".to_string()); + git.pushed_branches.insert("main".to_string()); + git.pushed_branches.insert("develop".to_string()); + FinishState { + kind: FinishKind::Release, major: 2, minor: 5, patch: 0, + started_at: "1".to_string(), stash_ref: None, + }.save(&git.git_dir).unwrap(); + + run_lifecycle(&git, finish_cmd()).unwrap(); + + let calls = git.calls(); + assert!(!calls.iter().any(|c| c.starts_with("merge:")), + "resume of a completed finish must not re-merge; calls: {calls:?}"); + assert!(!state_path(&git.git_dir).exists(), + "state must be cleared after a successful resume"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +#[test] +fn failed_resume_keeps_state_for_the_next_attempt() { + let mut git = release_git(); + git.fail_nth_merge = Some(1); + FinishState { + kind: FinishKind::Release, major: 2, minor: 5, patch: 0, + started_at: "1".to_string(), stash_ref: None, + }.save(&git.git_dir).unwrap(); + + let result = run_lifecycle(&git, finish_cmd()); + + assert!(result.is_err()); + assert!(state_path(&git.git_dir).exists(), + "a failed resume must keep the state file for the next attempt"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +// --- Abort --- + +#[test] +fn abort_clears_state_without_touching_the_repo() { + let mut git = release_git(); + git.mid_merge = true; // abort must work even mid-merge + FinishState { + kind: FinishKind::Release, major: 2, minor: 5, patch: 0, + started_at: "1".to_string(), stash_ref: Some("bflow-finish:release/2.5.0:1".to_string()), + }.save(&git.git_dir).unwrap(); + + run_lifecycle(&git, Some(Commands::Finish { breaking: None, base: None, abort: true })).unwrap(); + + assert!(!state_path(&git.git_dir).exists(), "abort must discard the state file"); + let calls = git.calls(); + assert!(!calls.iter().any(|c| c == "fetch"), "abort must not fetch"); + assert!(!calls.iter().any(|c| c.starts_with("merge:") || c.starts_with("checkout:") + || c.starts_with("stash_pop_ref:")), + "abort must not mutate the repo or auto-pop the stash; calls: {calls:?}"); + std::fs::remove_dir_all(&git.git_dir).ok(); +} + +// --- Action resolution (moved from main.rs with the lifecycle) --- + +fn release_state() -> FinishState { + FinishState { + kind: FinishKind::Release, + major: 2, + minor: 5, + patch: 0, + started_at: "0".to_string(), + stash_ref: None, + } +} + +#[test] +fn resume_state_resumes_finish_without_base() { + let branch_type = BranchType::Release { major: 2, minor: 5, patch: 0 }; + let state = release_state(); + let cmd = Some(Commands::Finish { breaking: None, base: None, abort: false }); + + let action = resolve_action_with_state(cmd, &branch_type, "release/2.5.0", Some(&state), false).unwrap(); + + assert!(matches!(action, Action::FinishRelease)); +} + +#[test] +fn explicit_base_rejected_even_when_resume_state_exists() { + // Regression: the fixed-target --base guard lives in resolve_action, which the + // resume early-return used to skip — silently swallowing an invalid --base. + let branch_type = BranchType::Release { major: 2, minor: 5, patch: 0 }; + let state = release_state(); + let cmd = Some(Commands::Finish { breaking: None, base: Some("develop".to_string()), abort: false }); + + let err = resolve_action_with_state(cmd, &branch_type, "release/2.5.0", Some(&state), false).unwrap_err(); + + assert!(err.contains("--base"), "Expected the fixed-target --base error, got: {err}"); +} diff --git a/tests/mock_contract_test.rs b/tests/mock_contract_test.rs new file mode 100644 index 0000000..4790941 --- /dev/null +++ b/tests/mock_contract_test.rs @@ -0,0 +1,14 @@ +mod common; + +use common::MockGit; +use bflow::git::Git; + +// Pins the mock's own recording contract (see decisions.md, Testing Strategy): +// the call string format is part of every exact-sequence expectation. + +#[test] +fn create_branch_no_checkout_records_call() { + let git = MockGit::new(); + git.create_branch_no_checkout("feature/test", "develop").unwrap(); + assert_eq!(git.calls(), vec!["create_branch_no_checkout:feature/test:develop"]); +} diff --git a/tests/start_test.rs b/tests/start_test.rs index 79c1620..0b1bf43 100644 --- a/tests/start_test.rs +++ b/tests/start_test.rs @@ -1,6 +1,6 @@ mod common; -use common::{MockEditor, MockGit}; +use common::{MockEditor, MockGit, MockPrompter}; use bflow::flows::start::{start_work_branch, start_release, start_release_fix, start_hotfix_fix, ReleaseType, detect_breaking_changes}; use bflow::version::SemVer; use bflow::worktree::{WorktreeConfig, WorktreeContext}; @@ -44,7 +44,7 @@ fn start_release_creates_new_when_no_release_exists_with_tags() { git.branches_matching = vec![]; // no existing release branches git.tags = vec!["v1.0.0".to_string()]; - start_release(&git, Some(ReleaseType::Minor)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Minor)).unwrap(); assert_eq!(git.calls(), vec![ "list_branches_matching:release/*", @@ -63,7 +63,7 @@ fn start_release_creates_new_when_no_release_exists_no_tags() { git.branches_matching = vec![]; git.tags = vec![]; - start_release(&git, Some(ReleaseType::Minor)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Minor)).unwrap(); assert_eq!(git.calls(), vec![ "list_branches_matching:release/*", @@ -81,7 +81,7 @@ fn start_release_checks_out_existing_release_branch() { let mut git = MockGit::new(); git.branches_matching = vec!["release/1.1.0".to_string()]; - start_release(&git, Some(ReleaseType::Minor)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Minor)).unwrap(); assert_eq!(git.calls(), vec![ "list_branches_matching:release/*", @@ -210,7 +210,7 @@ fn start_release_falls_back_to_rc_tags_when_no_clean_tags() { git.branches_matching = vec![]; git.tags = vec!["v1.1.0-rc.1".to_string(), "v1.1.0-rc.2".to_string()]; - start_release(&git, Some(ReleaseType::Minor)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Minor)).unwrap(); // Should use 1.1.0 (from RC tags) as base, bump to 1.2 assert_eq!(git.calls(), vec![ @@ -230,7 +230,7 @@ fn start_release_major_bumps_major_version() { git.branches_matching = vec![]; git.tags = vec!["v1.5.0".to_string()]; - start_release(&git, Some(ReleaseType::Major)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Major)).unwrap(); assert_eq!(git.calls(), vec![ "list_branches_matching:release/*", @@ -249,7 +249,7 @@ fn start_release_ignores_rc_tags_when_determining_next_version() { git.branches_matching = vec![]; git.tags = vec!["v1.0.0".to_string(), "v1.1.0-rc.1".to_string(), "v1.1.0-rc.2".to_string()]; - start_release(&git, Some(ReleaseType::Minor)).unwrap(); + start_release(&git, &MockPrompter::new(), Some(ReleaseType::Minor)).unwrap(); assert_eq!(git.calls(), vec![ "list_branches_matching:release/*", diff --git a/tests/stash_test.rs b/tests/stash_test.rs deleted file mode 100644 index ecc46f3..0000000 --- a/tests/stash_test.rs +++ /dev/null @@ -1,25 +0,0 @@ -mod common; - -use common::MockGit; -use bflow::git::Git; - -#[test] -fn stash_push_records_call() { - let git = MockGit::new(); - git.stash_push().unwrap(); - assert_eq!(git.calls(), vec!["stash_push"]); -} - -#[test] -fn stash_pop_records_call() { - let git = MockGit::new(); - git.stash_pop().unwrap(); - assert_eq!(git.calls(), vec!["stash_pop"]); -} - -#[test] -fn create_branch_no_checkout_records_call() { - let git = MockGit::new(); - git.create_branch_no_checkout("feature/test", "develop").unwrap(); - assert_eq!(git.calls(), vec!["create_branch_no_checkout:feature/test:develop"]); -}