diff --git a/CONTEXT.md b/CONTEXT.md index f7899f94..68571717 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -12,6 +12,16 @@ Terms used throughout the `git-workon` codebase. Implementation details do not b **Status filter** — a flag (`--dirty`, `--clean`, `--ahead`, `--behind`, `--gone`) that narrows a `list` or `find` result to worktrees in a specific state. Filters select **worktrees**: each check queries the working tree or branch-tracking state of a checked-out worktree. A metadata-only stack diff (`◯`) has no working tree and can never satisfy a status filter; it is excluded from any filtered result. See also: `StatusFilter`, `WorktreeDescriptor::is_dirty()`. +## Review + +**Changeset** — one reviewable unit in the review TUI: a node in a stack, a single inferred commit, or the uncommitted layer. Ordered base → head when part of a stack. See also: `workon::Changeset`. + +**Changeset span** — what a changeset covers: a resolved commit range (`base..head`) or the uncommitted working tree + index. _Avoid_: "changeset source" (renamed; "source" is the review-source concept below). + +**Review source** — the user's answer to "review *what?*": auto-detect (no argument), the `stack` keyword, the `uncommitted` keyword, a ref, a range, or a PR reference. Exact bare keywords win over same-named refs; a qualified spelling (`refs/heads/stack`) escapes. See also: [ADR-036](docs/adr/036-review-source-grammar.md). + +**Uncommitted layer** — the synthetic changeset spanning the dirty working tree + index. Appears in a review only when the review is focused where `HEAD` actually is, since uncommitted changes diff against `HEAD`. + ## Prune Candidate Reasons **BranchDeleted** — the local branch ref for the worktree no longer exists in the repository. Always a prune candidate regardless of flags. diff --git a/docs/adr/036-review-source-grammar.md b/docs/adr/036-review-source-grammar.md new file mode 100644 index 00000000..6028891c --- /dev/null +++ b/docs/adr/036-review-source-grammar.md @@ -0,0 +1,97 @@ +# 036 — Review Source: One Sniffed Positional, Shape-Aware Resolution + +Status: accepted (2026-07-09, M7 design session) + +## Context + +Through M6.5 the review binary's `Cli` is empty: it reviews only what auto-detect finds +(the Graphite stack when one is active, else a single uncommitted changeset). M7 makes it +review *anything* — the RFC's "review any source" — which forces three intertwined +decisions: how a source is spelled on the command line, what changeset(s) each spelling +resolves to, and what happens when resolution fails. This is the binary's entire +user-facing argument surface, so it is expensive to re-shape once muscle memory forms. + +Alternatives considered for the spelling: subcommands (`review pr 123`, `review range a b`) +— unambiguous but verbose and unlike git's rev-positional idiom; flags (`--pr`, `--range`) +— noisiest for a daily-driver tool, and sources are mutually exclusive so flags fight. + +## Decision + +**Grammar — one optional sniffed positional.** `git workon review []`. No argument +keeps auto-detect unchanged. An argument is classified by precedence: + +1. **PR reference** — any form `workon`'s own default command accepts (`123` excluded; + `pr-123`, `#123`, `pr#123`, GitHub URLs), via git-workon-lib `parse_pr_reference`. +2. **Keyword** — exact bare `stack` or `uncommitted`. +3. **Range** — contains `..` or `...`. +4. **Ref** — everything else, resolved via rev-parse. + +**Keywords win; qualify to escape.** Classification happens before rev-parse, so +`review stack` is deterministic regardless of repo state. A branch literally named +`stack` is reviewable via any qualified spelling (`refs/heads/stack`, `heads/stack`) — +only the exact bare word matches the keyword. + +**`stack` keyword** — "give me the real stack": Graphite metadata when active, otherwise +git-inference (the lib's already-built `StackModel::Git` arm: one changeset per commit in +`upstream..HEAD`). No metadata and no upstream is a real error, never a silent fall-through +to uncommitted — an explicit ask deserves an explicit failure. This ships the M5-deferred +`StackModel::Git` wiring, scoped to the one keyword that means it. + +**`uncommitted` keyword** — always the single uncommitted changeset (M2–M4 behavior), +even in a Graphite repo. + +**`` — shape-aware dispatch.** Match what a person most plausibly means per shape: + +- *Graphite-tracked branch* → the whole stack focused at that branch + (`assemble_changesets` already does exactly this; outline and `]c` nav come along). +- *Untracked branch* → one committed changeset, base = merge-base(upstream if set, else + repo trunk, else error) — "what this branch adds". +- *Bare commit-ish* (sha, tag, `HEAD~2`) → one changeset spanning just that commit + (`parent..ref`). + +**Ranges — git-diff semantics, both dot forms.** `a..b` → base `a`, head `b` (endpoint +trees, exactly a committed span). `a...b` → base merge-base(a,b), head `b` (the PR-style +"what did b add since diverging"). An empty side defaults to `HEAD`. One committed +changeset either way; git-diff muscle memory transfers unchanged. + +**PR — gh metadata + fetch, one changeset.** Reuse git-workon-lib `pr.rs` end-to-end: +`fetch_pr_metadata` (gh CLI) for base/head/title/fork detection, `fetch_branch` for the +objects — no worktree is created; review is read-only. Changeset = +`merge-base(base, head)..head` (GitHub's own three-dot PR diff), PR title carried into the +changeset. Requires gh + network, like `workon #123` today. + +**Uncommitted layer only when focused on real HEAD.** The layer rides along exactly when +the thing under review is where the working tree actually is: `stack`, and `` where +ref is the current `HEAD` branch. Every other source — range, commit, PR, untracked +branch, a tracked branch you're not standing on — is committed-only. Rationale: +uncommitted changes diff against `HEAD`; the lib's unconditional insert-after-current +would attach them to a branch they don't belong to. + +**Failures surface before the TUI.** Unresolvable ref, bad range endpoint, missing gh, PR +fetch failure, no-upstream: pre-TUI miette errors naming the offending source text, with a +hint where one exists. Never enter the TUI on a broken source; never fall back to +auto-detect (silently reviewing the wrong thing after a typo is the one surprise a review +tool must not have). A valid-but-empty source keeps "nothing to review" + exit 0, extended +to name the source. + +**Completion — keywords + local branches + tags.** Offline git2 ref enumeration only; +after a `..`/`...` prefix, complete the right-hand ref the same way. No PR-number +completion (network in the TAB hot path). This is M6's deferred sub-delegation trigger: +git-workon's dynamic completer now shells out to `COMPLETE= git-workon-review` for +post-subcommand words. + +**Rename `ChangesetSource` → `ChangesetSpan`.** Its doc comment already says "what a +Changeset spans"; the rename frees "source" for the user-facing concept every roadmap +document already uses. Safe while the M1–M6.5 tower is unmerged. + +## Consequences + +- The review binary gains its first real argument; the `Source` enum + (Auto | Stack | Uncommitted | Ref | Range | Pr) becomes the seam between CLI parse and + changeset resolution. +- Stack assembly for a non-HEAD tracked branch must suppress the uncommitted layer — a + lib-side knob or an acquire-side filter (execution detail, see the M7 plan). +- `review ` resolves through the untracked-branch arm via its upstream (unpushed + commits) — an acceptable edge, not a special case. +- Git-inference changesets become reachable from the binary for the first time; its + per-commit semantics get real exposure. diff --git a/docs/plans/review-any-source.md b/docs/plans/review-any-source.md new file mode 100644 index 00000000..df7e4e85 --- /dev/null +++ b/docs/plans/review-any-source.md @@ -0,0 +1,152 @@ +# Plan — Review Any Source (M7) + +Design locked 2026-07-09. Decisions live in **[ADR-036](../adr/036-review-source-grammar.md)** +(source grammar, per-shape resolution, error posture, completion scope, the +`ChangesetSpan` rename). This doc is the *execution* plan: what lands, in what order, how +each unit is verified. Read the ADR before implementing — this plan does not restate its +rationale. Glossary terms ("Review source", "Changeset span", "Uncommitted layer") are in +[CONTEXT.md](../../CONTEXT.md). + +Goal: `git workon review []` reviews *anything* — stack, uncommitted, ref, range, +PR — not just the auto-detected state. Read-only for committed sources (M5 semantics); +no-arg auto-detect behavior is byte-identical to today. + +## Scope (five tracks) + +1. **`ChangesetSpan` rename** — `workon::ChangesetSource` → `workon::ChangesetSpan` + (field `source` → `span`), mechanical across lib + review crates. +2. **Source classifier + keywords** — `Source` enum in the review crate; the binary's + `Cli` gains one optional positional (`[SOURCE]`); exact-bare-keyword precedence; + `stack` (Graphite → Git-inference → error) and `uncommitted` resolution; the + uncommitted-layer suppression seam in the lib. +3. **Rev sources** — `` shape-aware dispatch (tracked branch → focused stack; + untracked branch → merge-base changeset; commit-ish → single commit) and + `a..b` / `a...b` ranges (git-diff semantics, empty side = `HEAD`). +4. **PR source** — `parse_pr_reference` forms at top precedence; `fetch_pr_metadata` + + `fetch_branch` (fork-aware) → one committed changeset `merge-base(base,head)..head`, + PR title carried through. No worktree is created. +5. **Completion** — review-binary completer offers keywords + local branches + tags + (and the RHS after `..`/`...`); git-workon's completer sub-delegates post-subcommand + words to `COMPLETE= git-workon-review` (the M6-deferred shell-out). + +## Changeset partition (Graphite stack) + +Linear stack — each unit extends the classifier the previous one introduced. Base: +the current M3–M6.5 tower tip (`uc-roadmap-reprioritize`/`uc-pty-smoke`), or `main` once +the tower lands. Each unit is land-alone (green + valuable by itself) and +standalone-review (~≤400 non-mechanical lines). + +``` + + └─ m7-span-rename CS1 ── ChangesetSource → ChangesetSpan (mechanical) + └─ m7-source-keywords CS2 ── Source enum, positional arg, stack/uncommitted keywords + └─ m7-source-revs CS3 ── dispatch + ranges (grammar complete) + └─ m7-source-pr CS4 ── PR references via pr.rs + └─ m7-complete CS5 ── source completion + git-workon sub-delegation +``` + +Interim behavior is honest at every cut: before CS3, a ref/range argument fails the +keyword match and errors pre-TUI as an unresolvable source; before CS4, `pr-123` falls +through to the ref arm and errors the same way (named, hinted). + +## Per-changeset detail + +### CS1 — `m7-span-rename` (refactor, lib + review) + +- `refactor(lib): rename ChangesetSource to ChangesetSpan`. Type, `Changeset.source` + field → `Changeset.span`, doc comments, all use sites in `acquire.rs`/`app.rs`/tests. +- Purely mechanical; no behavior change. Verify: full workspace green, `grep -rn + ChangesetSource` returns nothing. + +### CS2 — `m7-source-keywords` (review crate + one lib seam) + +- New `source.rs` in the review lib: `Source` enum + (`Auto | Stack | Uncommitted | Ref(String) | Range{..} | Pr(PullRequest)`) with + `Source::classify(&str)` implementing the ADR precedence. In CS2 the classifier ships + with keyword + fallback-to-`Ref` arms only; `Ref` resolution errors as unresolvable + (real resolution is CS3). Classification is pure → unit-test exhaustively (keyword + exactness: `Stack` ≠ `stack` keyword? No — exact bare match is case-sensitive `stack`; + `refs/heads/stack` classifies as `Ref`). +- `Cli` gains `Option` positional `[SOURCE]`; `main.rs` routes + `None` → `Source::Auto` → existing `resolve_changesets` (unchanged path). +- `stack`: Graphite → `assemble_changesets(.., Graphite)`; else Git-inference + (`StackModel::Git` — first binary wiring); `NoUpstream` surfaces pre-TUI with a hint + (set an upstream, or `review uncommitted`). +- `uncommitted`: the single synthetic uncommitted changeset (extract today's + `resolve_changesets` fallback arm for reuse). +- **Lib seam**: `assemble_changesets` must be able to omit the uncommitted layer + (ADR-036: layer only when focused on real HEAD). Prefer an explicit parameter over a + post-filter — a post-filter must also repair the `current` flag, which is subtle. + CS2 introduces the seam (keywords always run with the layer *on*, since `stack` + reviews HEAD's stack); CS3 is the first caller that turns it off. +- New error variants in review `error.rs` per ADR-008 — **load `/docs errors` first**. +- Verify: fixture tests for both keyword resolutions in Graphite and plain-git repos + (sqlite + legacy metadata modes), error cases asserted with `NO_COLOR=1`. + +### CS3 — `m7-source-revs` (review crate + acquire) + +- `Ref` resolution, dispatched on shape (ADR-036): Graphite-tracked branch → + `assemble_changesets` focused there, uncommitted layer ON iff the ref is the actual + `HEAD` branch (first user of the CS2 lib seam); untracked branch → one committed + changeset, base = merge-base(upstream, else trunk, else error); commit-ish → + `parent..ref` (root commit: empty-tree base). +- `Range` resolution: split on `...` first, then `..`; empty side → `HEAD`; rev-parse + each endpoint; `...` → merge-base base. One committed changeset named after the + source text as typed. +- Empty-but-valid results extend the existing "nothing to review" to name the source. +- Verify: fixture matrix — tracked/untracked/commit/tag shapes; both dot forms; + `review ` == auto-detect output (layer present); reviewing a non-HEAD + tracked branch on a dirty tree asserts NO uncommitted layer and correct `current`. + +### CS4 — `m7-source-pr` (review crate, reuses lib `pr.rs`) + +- Classifier gains the PR arm at top precedence (`parse_pr_reference`; also accept + `pr-123` if the lib parser doesn't already — check first, extend the *lib parser* + if not, with its existing tests as the pattern). +- Resolution: `check_gh_available` → `fetch_pr_metadata` → `detect_pr_remote` / + `setup_fork_remote` → `fetch_branch` → merge-base(base, head) → one committed + changeset, `title` from PR metadata. Every failure pre-TUI, named, hinted. +- Verify: classification unit tests offline; resolution wiring behind the smallest + testable seam (metadata → changeset mapping fixture-tested with a local "remote"; + the gh-network path itself is exercised manually — record the manual check in the + changeset description). + +### CS5 — `m7-complete` (review crate + git-workon completer) + +- Review-binary completer: keywords + local branch names + tag names via git2 ref + enumeration; when the current word contains `..`/`...`, complete the RHS ref the + same way. Offline only; no PR numbers. +- git-workon side: the dynamic completer's external-subcommand arm shells out + `COMPLETE= git-workon-review -- ` for post-subcommand words + (M6 CS3 left this seam documented; remember `_CLAP_COMPLETE_INDEX`). +- Verify: completion integration tests per M6's pattern (`COMPLETE=` env protocol), + asserting keyword + ref candidates and the delegation path. + +## Traps / notes for the implementer + +- **Load `/docs testing` before any tests; `/docs errors` before error variants.** +- `FORCE_COLOR=3` is set in this environment — output-asserting tests pin `NO_COLOR=1`. +- Verify TUI behavior by instrumenting, never by grepping ratatui frames. +- The Git-inference arm (`assemble_git`) is lib-complete and lib-tested; CS2 only wires + it. Don't reimplement. +- `resolve_changesets`'s doc comment explains why auto-detect must NOT route plain-git + repos to `StackModel::Git` — that reasoning stays true; only the explicit `stack` + keyword takes the Git arm. +- Working-tree leftovers are user WIP — never stage `.claude/settings.json`, + `.claude/hooks/post-edit-rust.sh`, `docs/diagrams/agent-integration.md`, + `docs/recipes/agent-integration.md`. `git add` specific files, never `-A`/`-u`. +- Commits: Conventional, single line ≤72 chars, no body/footer. + +## Verification gates (green before any changeset is called done) + +```bash +NO_COLOR=1 cargo test --workspace +cargo clippy --workspace --all-targets --all-features -- -D warnings +cargo fmt --all -- --check +cargo run -p git-workon-review -- # manual: each source shape renders +``` + +## Acceptance (RFC M7) + +`git workon review ` / `` / `pr-123` renders the right changeset(s); +`git workon review ` completes sources. diff --git a/docs/rfc/workon-review.md b/docs/rfc/workon-review.md index be0c3a96..97cbc06b 100644 --- a/docs/rfc/workon-review.md +++ b/docs/rfc/workon-review.md @@ -140,7 +140,7 @@ The remaining roadmap is resequenced around the tool being **the author's own ev - **Prerequisite — Land M3–M6.5** (process, parallel to features; not a numbered milestone). QA the unmerged M3→M6.5 tower → merge to `main` → reliable install (a local build on PATH is enough to dogfood; the [ADR-033](../adr/033-review-crate-workspace-placement.md) release/homebrew "M3 flip" is a deferrable sub-decision). Gates real daily use regardless of features. QA checklist in memory `review-tui-priority-everyday-use` (`theme=auto` responsiveness, `theme=light` canvas, committed-changeset nav). -- **M7 — review any source.** A source selector — `stack | uncommitted | | | pr-####` — so the tool reviews *anything*, not just the auto-detected stack/uncommitted state. **Ordered first:** it is the tool's core *read* identity, read-only (low-risk), independent of the write verbs, and the M1/M5 lib already provides `assemble_changesets` + the `diff_changeset` router — mostly source-arg parse → resolve to changeset(s) → existing pipeline. PR support reuses git-workon-lib's `pr.rs`. Also **completes M6's deferred completion sub-delegation** (its trigger was exactly this arg gaining completion-worthy values). Acceptance: `git workon review ` / `` / `pr-123` renders the right changeset(s); `git workon review ` completes sources. +- **M7 — review any source.** A source selector — `stack | uncommitted | | | pr-####` — so the tool reviews *anything*, not just the auto-detected stack/uncommitted state. **Ordered first:** it is the tool's core *read* identity, read-only (low-risk), independent of the write verbs, and the M1/M5 lib already provides `assemble_changesets` + the `diff_changeset` router — mostly source-arg parse → resolve to changeset(s) → existing pipeline. PR support reuses git-workon-lib's `pr.rs`. Also **completes M6's deferred completion sub-delegation** (its trigger was exactly this arg gaining completion-worthy values). Acceptance: `git workon review ` / `` / `pr-123` renders the right changeset(s); `git workon review ` completes sources. Design locked 2026-07-09 — [ADR-036](../adr/036-review-source-grammar.md) (one sniffed positional, keyword-over-ref precedence, shape-aware `` dispatch, git-diff dot semantics, gh-backed PR resolution, uncommitted-layer-on-HEAD-only, fail-before-TUI, offline completion, `ChangesetSource`→`ChangesetSpan` rename); execution plan `docs/plans/review-any-source.md` (five changesets `m7-span-rename → m7-source-keywords → m7-source-revs → m7-source-pr → m7-complete`). - **M8 — commit operations.** Commit the staged changes without leaving the TUI — message editor (inline vs `$EDITOR`), Conventional-Commit-aware (enforced by `git-hooks/commit-msg`); **amend** the current commit; **fixup/absorb** staged changes into an earlier changeset in the stack. Closes the review→stage→**commit** loop — the acute daily-driver gap. Acceptance: stage in the TUI, commit/amend/fixup, verified against real git. diff --git a/git-workon-fixture/src/path_stub.rs b/git-workon-fixture/src/path_stub.rs index a179db06..f7f0fba1 100644 --- a/git-workon-fixture/src/path_stub.rs +++ b/git-workon-fixture/src/path_stub.rs @@ -66,6 +66,16 @@ impl PathStub { self.dir.path().join(format!("{name}.invocations.log")) } + /// Symlink a real executable (e.g. another workspace binary's `CARGO_BIN_EXE_*` path) into + /// the stub directory as `git-workon-`, so a test can drive genuine external-binary + /// behavior (not just canned `arg:`/`cwd:` stub output) through the same PATH-dispatch or + /// PATH-completion surface `command` exercises. + pub fn command_exe(self, name: &str, exe: &std::path::Path) -> Result { + let path = self.dir.path().join(format!("git-workon-{name}")); + symlink_exe(exe, &path)?; + Ok(self) + } + /// `PATH` value with the stub directory prepended to the current process's `PATH`, so a /// stub shadows nothing else already on `PATH` unless intended (see built-in precedence). pub fn path(&self) -> String { @@ -105,3 +115,15 @@ fn set_executable(path: &PathBuf) -> Result<()> { fn set_executable(_path: &PathBuf) -> Result<()> { Ok(()) } + +#[cfg(unix)] +fn symlink_exe(exe: &std::path::Path, link: &std::path::Path) -> Result<()> { + std::os::unix::fs::symlink(exe, link)?; + Ok(()) +} + +#[cfg(not(unix))] +fn symlink_exe(exe: &std::path::Path, link: &std::path::Path) -> Result<()> { + std::fs::copy(exe, link)?; + Ok(()) +} diff --git a/git-workon-lib/src/changeset.rs b/git-workon-lib/src/changeset.rs index b7d5158c..1a43dfa9 100644 --- a/git-workon-lib/src/changeset.rs +++ b/git-workon-lib/src/changeset.rs @@ -2,7 +2,7 @@ //! reviewable [`Changeset`]s for the worktree whose `HEAD` is a given branch. //! //! This is the substrate the review TUI (M2+) consumes. It stays **diff-free**: every -//! [`Changeset`] carries resolved `git2::Oid` rev pairs (or the [`ChangesetSource::Uncommitted`] +//! [`Changeset`] carries resolved `git2::Oid` rev pairs (or the [`ChangesetSpan::Uncommitted`] //! marker), never a parsed diff. Detecting uncommitted changes uses `repo.statuses`, never //! `repo.diff_*`. //! @@ -21,8 +21,12 @@ //! (oldest first), one [`Changeset`] per commit. //! //! In both metadata-bearing arms, a non-empty `repo.statuses` result inserts a -//! [`ChangesetSource::Uncommitted`] entry immediately after the current node, taking over -//! `current`. +//! [`ChangesetSpan::Uncommitted`] entry immediately after the current node, taking over +//! `current` — but only when the caller passes [`UncommittedLayer::Include`]. The layer +//! belongs only when the thing under review is where the working tree actually is; a caller +//! resolving a source that isn't real `HEAD` (a range, a commit, a PR, a tracked branch you're +//! not standing on) passes [`UncommittedLayer::Omit`] instead. See [`UncommittedLayer`]'s own +//! doc for the full rationale. use std::collections::HashSet; @@ -34,9 +38,14 @@ use crate::stack::{gh_stack, graphite, StackModel}; /// What a [`Changeset`] spans: a resolved commit range, or the working tree + index. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ChangesetSource { +pub enum ChangesetSpan { /// A committed range `base..head` — resolved OIDs only; the lib never diffs them itself. Committed { base: Oid, head: Oid }, + /// A committed range whose base is the empty tree: a root commit (no parent) reviewed on + /// its own, so every file in `head` renders as added. Only the review crate's `` + /// bare-commit-ish dispatch (ADR-036) constructs this — `assemble_graphite`/`assemble_git` + /// never do, since a stack node's base is always a real (or merge-base-derived) commit. + CommittedRoot { head: Oid }, /// Uncommitted working-tree + index changes relative to the current branch's head. Uncommitted, } @@ -46,12 +55,12 @@ pub enum ChangesetSource { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Changeset { /// Branch name for stack nodes; 8-hex abbreviated commit id for git-inference per-commit - /// changesets; the current branch name for [`ChangesetSource::Uncommitted`]. + /// changesets; the current branch name for [`ChangesetSpan::Uncommitted`]. pub name: String, /// The commit range (or uncommitted marker) this changeset covers. - pub source: ChangesetSource, + pub span: ChangesetSpan, /// PR title (from `.graphite_pr_info`) for Graphite nodes; commit summary for - /// git-inference nodes; `None` for [`ChangesetSource::Uncommitted`]. + /// git-inference nodes; `None` for [`ChangesetSpan::Uncommitted`]. pub title: Option, /// Exactly one entry in the returned `Vec` is current: the Uncommitted entry when /// present, otherwise the current branch's node (Graphite) or tip commit (Git). @@ -61,27 +70,52 @@ pub struct Changeset { pub needs_restack: bool, } +/// Whether [`assemble_changesets`] should insert the synthetic [`ChangesetSpan::Uncommitted`] +/// layer when the worktree has a dirty tree (see [`insert_uncommitted_layer`]). +/// +/// ADR-036: the layer only belongs when the thing under review is where the working tree +/// actually is (`stack`, or a `` that is the current `HEAD` branch) — every other source +/// (a range, a commit, a PR, an untracked branch, a tracked branch you're not standing on) is +/// committed-only, since uncommitted changes diff against `HEAD` and would otherwise attach to +/// a branch they don't belong to. An explicit parameter, not a post-filter: a post-filter would +/// also have to repair whichever node's `current` flag the inserted layer took over. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum UncommittedLayer { + /// Insert the layer when the tree is dirty (today's behavior). + Include, + /// Never insert the layer, regardless of tree state. + Omit, +} + /// Assemble the ordered (base → head) changesets for the worktree whose `HEAD` is /// `head_branch`, under the given [`StackModel`]. /// /// See the module docs for the per-model walk semantics. Errors distinguish a genuinely /// broken reference or stack-metadata snapshot (bad ref, unresolvable recorded revision, no /// upstream) from a valid empty result (`Ok(vec![])`, e.g. a trunk-only worktree under `Git` -/// with a clean tree). +/// with a clean tree). `uncommitted` controls whether a dirty tree gets the synthetic +/// [`ChangesetSpan::Uncommitted`] layer at all — see [`UncommittedLayer`]. pub fn assemble_changesets( repo: &Repository, head_branch: &str, model: StackModel, + uncommitted: UncommittedLayer, ) -> Result> { match model { StackModel::None => Ok(vec![]), - StackModel::Git => assemble_git(repo, head_branch), - StackModel::Graphite => { - assemble_from_metadata(repo, head_branch, &graphite::read_metadata(repo)?) - } - StackModel::GhStack => { - assemble_from_metadata(repo, head_branch, &gh_stack::read_metadata(repo)?) - } + StackModel::Git => assemble_git(repo, head_branch, uncommitted), + StackModel::Graphite => assemble_from_metadata( + repo, + head_branch, + &graphite::read_metadata(repo)?, + uncommitted, + ), + StackModel::GhStack => assemble_from_metadata( + repo, + head_branch, + &gh_stack::read_metadata(repo)?, + uncommitted, + ), } } @@ -91,12 +125,13 @@ fn assemble_from_metadata( repo: &Repository, head_branch: &str, meta: &StackMetadata, + uncommitted: UncommittedLayer, ) -> Result> { let trunks: HashSet = meta.trunks.iter().cloned().collect(); // Trunk or untracked head_branch: no stack metadata to walk, fall back to git-inference. if trunks.contains(head_branch) || !meta.parents.contains_key(head_branch) { - return assemble_git(repo, head_branch); + return assemble_git(repo, head_branch, uncommitted); } // head_branch is tracked but its own branch ref is gone: a genuinely broken state, distinct @@ -151,7 +186,7 @@ fn assemble_from_metadata( } changesets.push(Changeset { title: meta.pr_titles.get(&name).cloned(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: base_oid, head: head_oid, }, @@ -161,7 +196,9 @@ fn assemble_from_metadata( }); } - insert_uncommitted_layer(repo, head_branch, current_index, &mut changesets)?; + if uncommitted == UncommittedLayer::Include { + insert_uncommitted_layer(repo, head_branch, current_index, &mut changesets)?; + } Ok(changesets) } @@ -246,7 +283,11 @@ fn resolve_live_ancestor_tip(repo: &Repository, meta: &StackMetadata, start: &st /// Git-inference assembly: one [`Changeset`] per commit in `upstream(head_branch)..head_branch`, /// oldest first. -fn assemble_git(repo: &Repository, head_branch: &str) -> Result> { +fn assemble_git( + repo: &Repository, + head_branch: &str, + uncommitted: UncommittedLayer, +) -> Result> { let branch = repo.find_branch(head_branch, BranchType::Local)?; let upstream = branch.upstream().map_err(|_| ChangesetError::NoUpstream { branch: head_branch.to_string(), @@ -282,7 +323,7 @@ fn assemble_git(repo: &Repository, head_branch: &str) -> Result> let base = commit.parent_id(0).unwrap_or(oid); changesets.push(Changeset { name: short_id(oid), - source: ChangesetSource::Committed { base, head: oid }, + span: ChangesetSpan::Committed { base, head: oid }, title: commit.summary()?.map(str::to_string), current: false, needs_restack: false, @@ -297,7 +338,9 @@ fn assemble_git(repo: &Repository, head_branch: &str) -> Result> Some(last) }; - insert_uncommitted_layer(repo, head_branch, current_index, &mut changesets)?; + if uncommitted == UncommittedLayer::Include { + insert_uncommitted_layer(repo, head_branch, current_index, &mut changesets)?; + } Ok(changesets) } @@ -307,7 +350,7 @@ fn short_id(oid: Oid) -> String { oid.to_string()[..8].to_string() } -/// Insert a [`ChangesetSource::Uncommitted`] entry immediately after `current_index` (or at +/// Insert a [`ChangesetSpan::Uncommitted`] entry immediately after `current_index` (or at /// the end, if there is no committed current node) when `repo.statuses` reports any working /// tree or index changes. Demotes the previous current node's `current` flag. No-op on a /// clean tree. @@ -333,7 +376,7 @@ fn insert_uncommitted_layer( insert_at, Changeset { name: current_branch.to_string(), - source: ChangesetSource::Uncommitted, + span: ChangesetSpan::Uncommitted, title: None, current: true, needs_restack: false, diff --git a/git-workon-lib/src/pr.rs b/git-workon-lib/src/pr.rs index 79838f68..987fb5d2 100644 --- a/git-workon-lib/src/pr.rs +++ b/git-workon-lib/src/pr.rs @@ -553,11 +553,14 @@ pub fn setup_fork_remote(repo: &Repository, metadata: &PrMetadata) -> Result Result<()> { // Check if branch already exists locally let branch_ref = format!("refs/remotes/{}/{}", remote_name, branch); @@ -566,6 +569,18 @@ pub fn fetch_branch(repo: &Repository, remote_name: &str, branch: &str) -> Resul return Ok(()); } + fetch_branch_fresh(repo, remote_name, branch) +} + +/// Fetch `branch` from `remote_name`, making it available as +/// `refs/remotes/{remote_name}/{branch}`, always — force-updating the tracking ref to the +/// remote's current tip even if it already exists locally. +/// +/// Use this whenever a stale tracking ref would be wrong to review against (e.g. resolving a +/// PR's head and base for `git workon review`): the refspec is already force (`+`), so this +/// never fails on a diverged tracking ref, it just moves it. For the one-time +/// worktree-creation fetch where an existing ref is fine to leave alone, use [`fetch_branch`]. +pub fn fetch_branch_fresh(repo: &Repository, remote_name: &str, branch: &str) -> Result<()> { debug!("Fetching branch {} from remote {}", branch, remote_name); let refspec = format!( @@ -815,6 +830,55 @@ mod tests { assert_eq!(parse_merged_pr("not json"), None); } + /// `fetch_branch` skips a re-fetch once the tracking ref exists, even when the remote has + /// since moved — right for the one-time worktree-creation fetch, wrong for review, which is + /// why [`fetch_branch_fresh`] exists. Pins both: the existence short-circuit staying put, + /// and `fetch_branch_fresh` force-updating past it via the refspec's `+`. + #[test] + fn fetch_branch_fresh_updates_stale_tracking_ref_but_fetch_branch_does_not( + ) -> std::result::Result<(), Box> { + use git_workon_fixture::prelude::*; + + let upstream = FixtureBuilder::new() + .bare(true) + .default_branch("main") + .build()?; + let upstream_repo = upstream.repo()?; + let old_oid = upstream_repo.head()?.peel_to_commit()?.id(); + + let local = FixtureBuilder::new().remote("origin", &upstream).build()?; + let repo = local.repo()?; + + // First fetch: creates the tracking ref at the remote's current tip. + fetch_branch(repo, "origin", "main")?; + let tracking_ref = "refs/remotes/origin/main"; + assert_eq!(repo.find_reference(tracking_ref)?.target(), Some(old_oid)); + + // The remote moves. + let sig = git2::Signature::now("Test User", "test@example.com")?; + let old_commit = upstream_repo.find_commit(old_oid)?; + let tree = old_commit.tree()?; + let new_oid = upstream_repo.commit( + Some("refs/heads/main"), + &sig, + &sig, + "moved on main", + &tree, + &[&old_commit], + )?; + assert_ne!(new_oid, old_oid); + + // `fetch_branch` sees the ref already exists and leaves it stale. + fetch_branch(repo, "origin", "main")?; + assert_eq!(repo.find_reference(tracking_ref)?.target(), Some(old_oid)); + + // `fetch_branch_fresh` force-updates it to the remote's new tip. + fetch_branch_fresh(repo, "origin", "main")?; + assert_eq!(repo.find_reference(tracking_ref)?.target(), Some(new_oid)); + + Ok(()) + } + // Integration tests requiring gh CLI (marked with #[ignore]) #[test] #[ignore] diff --git a/git-workon-lib/tests/suite/changeset.rs b/git-workon-lib/tests/suite/changeset.rs index aec2ebd7..36dcaf9f 100644 --- a/git-workon-lib/tests/suite/changeset.rs +++ b/git-workon-lib/tests/suite/changeset.rs @@ -1,7 +1,8 @@ use git_workon_fixture::prelude::*; use std::error::Error; use workon::{ - assemble_changesets, ChangesetError, ChangesetSource, StackError, StackModel, WorkonError, + assemble_changesets, ChangesetError, ChangesetSpan, StackError, StackModel, UncommittedLayer, + WorkonError, }; // ── both-format parameterization (see tests/stack.rs) ──────────────────────── @@ -52,7 +53,8 @@ fn graphite_linear_order_current_and_titles(format: MetadataFormat) -> Result<() let b_tip = branch_tip(&fixture, "b")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "b", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "b", StackModel::Graphite, UncommittedLayer::Include)?; let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); assert_eq!(names, vec!["a", "b", "c"]); @@ -64,8 +66,8 @@ fn graphite_linear_order_current_and_titles(format: MetadataFormat) -> Result<() assert_eq!(current, vec!["b"]); let b_cs = changesets.iter().find(|c| c.name == "b").unwrap(); - match b_cs.source { - ChangesetSource::Committed { base, head } => { + match b_cs.span { + ChangesetSpan::Committed { base, head } => { assert_eq!(base, a_tip, "b's base must be a's recorded parent tip"); assert_eq!(head, b_tip, "b's head must be its live tip"); } @@ -100,7 +102,8 @@ fn graphite_fork_siblings_sorted_lexically(format: MetadataFormat) -> Result<(), .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "a", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "a", StackModel::Graphite, UncommittedLayer::Include)?; let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); // Descendant DFS sorts siblings lexically, not by creation order (zeta was added first). assert_eq!(names, vec!["a", "alpha", "zeta"]); @@ -118,10 +121,11 @@ fn graphite_all_at_one_commit_base_equals_head( .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "a", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "a", StackModel::Graphite, UncommittedLayer::Include)?; assert_eq!(changesets.len(), 1); - match changesets[0].source { - ChangesetSource::Committed { base, head } => { + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { assert_eq!(base, head, "no divergence yet: base must equal head") } _ => panic!("expected Committed"), @@ -141,7 +145,12 @@ fn graphite_ghost_mid_stack_skipped_children_present( .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "child", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "child", + StackModel::Graphite, + UncommittedLayer::Include, + )?; let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); assert_eq!(names, vec!["child"], "ghost must not appear in output"); assert!(changesets[0].current); @@ -163,7 +172,12 @@ fn graphite_untracked_parent_excluded_from_walk( .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "feat", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "feat", + StackModel::Graphite, + UncommittedLayer::Include, + )?; let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); assert_eq!(names, vec!["feat"], "untracked parent must not be emitted"); Ok(()) @@ -180,7 +194,8 @@ fn graphite_current_branch_missing_ref_errors( .build()?; let repo = fixture.repo()?; - let err = assemble_changesets(repo, "c", StackModel::Graphite).unwrap_err(); + let err = assemble_changesets(repo, "c", StackModel::Graphite, UncommittedLayer::Include) + .unwrap_err(); match err { WorkonError::Changeset(ChangesetError::UnresolvableBranch { branch }) => { assert_eq!(branch, "c") @@ -214,10 +229,15 @@ fn trap7_spans_stale_branch_revision_to_live_head( .create("commit2")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "feat-a", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "feat-a", + StackModel::Graphite, + UncommittedLayer::Include, + )?; assert_eq!(changesets.len(), 1); - match changesets[0].source { - ChangesetSource::Committed { base, head } => { + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { assert_eq!( base, main_tip, "base must be the recorded parentBranchRevision" @@ -242,7 +262,13 @@ fn trap7_bogus_parent_revision_errors(format: MetadataFormat) -> Result<(), Box< .build()?; let repo = fixture.repo()?; - let err = assemble_changesets(repo, "feat-a", StackModel::Graphite).unwrap_err(); + let err = assemble_changesets( + repo, + "feat-a", + StackModel::Graphite, + UncommittedLayer::Include, + ) + .unwrap_err(); match err { WorkonError::Changeset(ChangesetError::InvalidParentRevision { branch, revision }) => { assert_eq!(branch, "feat-a"); @@ -266,7 +292,13 @@ fn trap7_corrupt_sqlite_db_errors() -> Result<(), Box> { let db_path = repo.commondir().join(".graphite_metadata.db"); std::fs::write(&db_path, b"not a sqlite database")?; - let err = assemble_changesets(repo, "feat-a", StackModel::Graphite).unwrap_err(); + let err = assemble_changesets( + repo, + "feat-a", + StackModel::Graphite, + UncommittedLayer::Include, + ) + .unwrap_err(); assert!( matches!(err, WorkonError::Stack(StackError::GtParseFailed { .. })), "expected GtParseFailed, got {err:?}" @@ -293,7 +325,12 @@ fn needs_restack_true_when_parent_advances_post_build( .create("advance parent")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "child", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "child", + StackModel::Graphite, + UncommittedLayer::Include, + )?; let child_cs = changesets.iter().find(|c| c.name == "child").unwrap(); assert!( @@ -316,7 +353,8 @@ fn needs_restack_false_for_untouched_stack(format: MetadataFormat) -> Result<(), let fixture = linear_chain(format)?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "c", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "c", StackModel::Graphite, UncommittedLayer::Include)?; assert!( changesets.iter().all(|c| !c.needs_restack), "no branch advanced past what metadata recorded" @@ -337,11 +375,12 @@ fn needs_restack_false_with_empty_parent_revision_and_merge_base_fallback( let a_tip = branch_tip(&fixture, "a")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "a", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "a", StackModel::Graphite, UncommittedLayer::Include)?; assert_eq!(changesets.len(), 1); assert!(!changesets[0].needs_restack); - match changesets[0].source { - ChangesetSource::Committed { base, head } => { + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { assert_eq!(head, a_tip); assert_eq!(base, main_tip, "merge-base fallback resolves to main's tip"); } @@ -370,7 +409,8 @@ fn needs_restack_computed_for_ancestors_of_current( .create("advance main")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "b", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "b", StackModel::Graphite, UncommittedLayer::Include)?; let a_cs = changesets.iter().find(|c| c.name == "a").unwrap(); assert!( @@ -447,10 +487,11 @@ fn uncommitted_layer_absent_on_clean_tree(format: MetadataFormat) -> Result<(), .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "a", StackModel::Graphite)?; + let changesets = + assemble_changesets(repo, "a", StackModel::Graphite, UncommittedLayer::Include)?; assert_eq!(changesets.len(), 1); assert!(changesets[0].current); - assert_ne!(changesets[0].source, ChangesetSource::Uncommitted); + assert_ne!(changesets[0].span, ChangesetSpan::Uncommitted); Ok(()) } both_formats!(uncommitted_layer_absent_on_clean_tree); @@ -460,11 +501,16 @@ fn assert_uncommitted_inserted_after_current( current_branch: &str, ) -> Result<(), Box> { let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, current_branch, StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + current_branch, + StackModel::Graphite, + UncommittedLayer::Include, + )?; assert_eq!(changesets.len(), 2); assert_eq!(changesets[0].name, current_branch); assert!(!changesets[0].current, "branch node must drop current"); - assert_eq!(changesets[1].source, ChangesetSource::Uncommitted); + assert_eq!(changesets[1].span, ChangesetSpan::Uncommitted); assert_eq!(changesets[1].name, current_branch); assert!(changesets[1].current, "Uncommitted takes current"); Ok(()) @@ -485,7 +531,12 @@ fn graphite_falls_back_to_git_on_trunk(format: MetadataFormat) -> Result<(), Box .create("only commit")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "main", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "main", + StackModel::Graphite, + UncommittedLayer::Include, + )?; assert_eq!(changesets.len(), 1); assert_eq!(changesets[0].title.as_deref(), Some("only commit")); Ok(()) @@ -508,7 +559,12 @@ fn graphite_falls_back_to_git_on_untracked_branch( .create("untracked commit")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "feat-a", StackModel::Graphite)?; + let changesets = assemble_changesets( + repo, + "feat-a", + StackModel::Graphite, + UncommittedLayer::Include, + )?; assert_eq!(changesets.len(), 1); assert_eq!(changesets[0].title.as_deref(), Some("untracked commit")); Ok(()) @@ -528,7 +584,7 @@ fn git_inference_two_commits_oldest_first() -> Result<(), Box> { fixture.commit("main").file("b.txt", "2").create("second")?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "main", StackModel::Git)?; + let changesets = assemble_changesets(repo, "main", StackModel::Git, UncommittedLayer::Include)?; assert_eq!(changesets.len(), 2); assert_eq!(changesets[0].title.as_deref(), Some("first")); assert_eq!(changesets[1].title.as_deref(), Some("second")); @@ -541,11 +597,8 @@ fn git_inference_two_commits_oldest_first() -> Result<(), Box> { "name is an 8-hex abbreviated id" ); - match (&changesets[0].source, &changesets[1].source) { - ( - ChangesetSource::Committed { head: h0, .. }, - ChangesetSource::Committed { base: b1, .. }, - ) => { + match (&changesets[0].span, &changesets[1].span) { + (ChangesetSpan::Committed { head: h0, .. }, ChangesetSpan::Committed { base: b1, .. }) => { assert_eq!(*h0, first, "first commit's head is its own oid"); assert_eq!(*b1, *h0, "second's base is first's head"); } @@ -564,10 +617,10 @@ fn git_inference_dirty_tree_appends_uncommitted_as_current() -> Result<(), Box Result<(), Box Result<(), Box> { let fixture = FixtureBuilder::new().build()?; let repo = fixture.repo()?; - let err = assemble_changesets(repo, "main", StackModel::Git).unwrap_err(); + let err = + assemble_changesets(repo, "main", StackModel::Git, UncommittedLayer::Include).unwrap_err(); match err { WorkonError::Changeset(ChangesetError::NoUpstream { branch }) => { assert_eq!(branch, "main") @@ -607,7 +661,10 @@ fn none_model_always_returns_empty() -> Result<(), Box> { let fixture = linear_chain(MetadataFormat::Refs)?; let repo = fixture.repo()?; - assert_eq!(assemble_changesets(repo, "c", StackModel::None)?, vec![]); + assert_eq!( + assemble_changesets(repo, "c", StackModel::None, UncommittedLayer::Include)?, + vec![] + ); Ok(()) } @@ -623,7 +680,8 @@ fn gh_stack_linear_order_and_current() -> Result<(), Box> { .build()?; let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "b", StackModel::GhStack)?; + let changesets = + assemble_changesets(repo, "b", StackModel::GhStack, UncommittedLayer::Include)?; let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); assert_eq!(names, vec!["a", "b", "c"]); @@ -644,7 +702,13 @@ fn gh_stack_trap7_bogus_parent_revision_errors() -> Result<(), Box> { .build()?; let repo = fixture.repo()?; - let err = assemble_changesets(repo, "feat-a", StackModel::GhStack).unwrap_err(); + let err = assemble_changesets( + repo, + "feat-a", + StackModel::GhStack, + UncommittedLayer::Include, + ) + .unwrap_err(); match err { WorkonError::Changeset(ChangesetError::InvalidParentRevision { branch, revision }) => { assert_eq!(branch, "feat-a"); @@ -670,7 +734,12 @@ fn gh_stack_needs_restack_true_when_base_differs_from_parent_live_tip() -> Resul let repo = fixture.repo()?; - let changesets = assemble_changesets(repo, "feat-b", StackModel::GhStack)?; + let changesets = assemble_changesets( + repo, + "feat-b", + StackModel::GhStack, + UncommittedLayer::Include, + )?; let feat_a = changesets.iter().find(|c| c.name == "feat-a").unwrap(); assert!( !feat_a.needs_restack, diff --git a/git-workon-review/src/acquire.rs b/git-workon-review/src/acquire.rs index 00436525..e8904ae5 100644 --- a/git-workon-review/src/acquire.rs +++ b/git-workon-review/src/acquire.rs @@ -6,7 +6,7 @@ //! git2 diffs and then a [`DiffModel`]. use git2::{DiffFindOptions, DiffOptions, Oid, Repository}; -use workon::{assemble_changesets, Changeset, ChangesetSource, StackModel}; +use workon::{assemble_changesets, Changeset, ChangesetSpan, StackModel, UncommittedLayer}; use crate::error::DiffError; use crate::model::DiffModel; @@ -26,7 +26,7 @@ pub struct WorktreeDiffs { /// Diff `HEAD`'s tree against the index (staged), the index against the working tree /// (unstaged), and `HEAD`'s tree against the working tree directly (combined), for a -/// [`ChangesetSource::Uncommitted`] changeset. +/// [`ChangesetSpan::Uncommitted`] changeset. /// /// The unstaged and combined sides both set `include_untracked`/`recurse_untracked_dirs`/ /// `show_untracked_content` so untracked files carry real content in the model (git2 gives @@ -76,45 +76,130 @@ pub fn diff_uncommitted(repo: &Repository) -> Result { }) } -/// Diff `base`'s tree against `head`'s tree, for a [`ChangesetSource::Committed`] changeset — +/// Diff `base`'s tree against `head`'s tree, for a [`ChangesetSpan::Committed`] changeset — /// rename/copy detection runs via [`git2::Diff::find_similar`] so renamed files come back as /// [`crate::model::FileStatus::Renamed`] instead of a delete+add pair. pub fn diff_committed(repo: &Repository, base: Oid, head: Oid) -> Result { let base_tree = repo.find_commit(base)?.tree()?; let head_tree = repo.find_commit(head)?.tree()?; + diff_trees(repo, Some(&base_tree), &head_tree) +} + +/// Diff the empty tree against `head`'s tree, for a [`ChangesetSpan::CommittedRoot`] changeset +/// (a root commit reviewed on its own — every file in `head` renders as added). `git2` treats +/// `None` as the empty tree on either side of [`Repository::diff_tree_to_tree`], so no tree +/// object needs to be synthesized. +pub fn diff_committed_root(repo: &Repository, head: Oid) -> Result { + let head_tree = repo.find_commit(head)?.tree()?; + + diff_trees(repo, None, &head_tree) +} + +/// Shared tail of [`diff_committed`] and [`diff_committed_root`]: diff `base` (the empty tree +/// when `None`) against `head`, with rename/copy detection via [`git2::Diff::find_similar`] so +/// renamed files come back as [`crate::model::FileStatus::Renamed`] instead of a delete+add +/// pair. +fn diff_trees( + repo: &Repository, + base: Option<&git2::Tree>, + head: &git2::Tree, +) -> Result { let mut opts = DiffOptions::new(); opts.context_lines(3); - let mut diff = repo.diff_tree_to_tree(Some(&base_tree), Some(&head_tree), Some(&mut opts))?; + let mut diff = repo.diff_tree_to_tree(base, Some(head), Some(&mut opts))?; diff.find_similar(None)?; DiffModel::from_git2(&diff) } -/// The diff for one [`Changeset`], shaped by its [`ChangesetSource`]. +/// The diff for one [`Changeset`], shaped by its [`ChangesetSpan`]. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ChangesetDiff { Committed(DiffModel), Uncommitted(WorktreeDiffs), } -/// Diff `cs`, routing on its [`ChangesetSource`]. +/// Diff `cs`, routing on its [`ChangesetSpan`]. /// /// A changeset carrying a resolved-but-unreadable rev pair (a bad or garbage `Oid` — e.g. /// stale Graphite metadata pointing at a pruned commit) is a genuine failure, never an empty /// [`DiffModel`]: any underlying git2 error is reported as /// [`DiffError::ChangesetDiffFailed`]. pub fn diff_changeset(repo: &Repository, cs: &Changeset) -> Result { - match cs.source { - ChangesetSource::Committed { base, head } => diff_committed(repo, base, head) + match cs.span { + ChangesetSpan::Committed { base, head } => diff_committed(repo, base, head) + .map(ChangesetDiff::Committed) + .map_err(|err| changeset_diff_failed(&cs.name, err)), + ChangesetSpan::CommittedRoot { head } => diff_committed_root(repo, head) .map(ChangesetDiff::Committed) .map_err(|err| changeset_diff_failed(&cs.name, err)), - ChangesetSource::Uncommitted => diff_uncommitted(repo) + ChangesetSpan::Uncommitted => diff_uncommitted(repo) .map(ChangesetDiff::Uncommitted) .map_err(|err| changeset_diff_failed(&cs.name, err)), } } +/// Diff every changeset in `changesets`, returning the diffs in input order. +/// +/// The changesets are independent, and a review of a deep stack runs one [`diff_changeset`] +/// per node (a few ms of tree diff + rename detection each) — run sequentially their sum +/// gates the first frame at startup and every whole-stack refresh. So the work is striped +/// across `available_parallelism` threads. git2's `Repository` is `Send` but not `Sync`, so +/// each worker opens its own handle on the same on-disk repo instead of sharing `repo`. +/// +/// On any failure the first-failing changeset's error (by input order) is returned, matching +/// what the sequential loop this replaces would have surfaced. +pub fn diff_changesets( + repo: &Repository, + changesets: &[Changeset], +) -> Result, DiffError> { + let workers = std::thread::available_parallelism() + .map(std::num::NonZeroUsize::get) + .unwrap_or(1) + .min(changesets.len()); + if workers <= 1 { + return changesets + .iter() + .map(|cs| diff_changeset(repo, cs)) + .collect(); + } + + // Workers re-open at the workdir so the Uncommitted span's index/worktree diffs resolve + // against the same working tree as `repo`; the gitdir is the fallback for a bare repo + // (where only committed spans can occur). + let open_at = repo.workdir().unwrap_or_else(|| repo.path()).to_path_buf(); + + let chunk = changesets.len().div_ceil(workers); + let mut results: Vec>> = Vec::new(); + results.resize_with(changesets.len(), || None); + + std::thread::scope(|scope| { + for (cs_chunk, out_chunk) in changesets.chunks(chunk).zip(results.chunks_mut(chunk)) { + let open_at = &open_at; + scope.spawn(move || { + let repo = match Repository::open(open_at) { + Ok(repo) => repo, + Err(err) => { + // Every changeset in this chunk is undiffable without a handle; the + // first slot's error is the one input-order selection below reports. + out_chunk[0] = Some(Err(err.into())); + return; + } + }; + for (cs, out) in cs_chunk.iter().zip(out_chunk.iter_mut()) { + *out = Some(diff_changeset(&repo, cs)); + } + }); + } + }); + + results + .into_iter() + .map(|slot| slot.expect("every chunk fills its slots or errors its first slot")) + .collect() +} + /// Resolve the changeset stack the review App opens on for the worktree whose `HEAD` is /// `head_branch` (locked design decision M5-fork-7, "auto-detect"): the full Graphite stack /// when one is active, or a single synthetic [`Changeset`] spanning the uncommitted worktree @@ -134,16 +219,27 @@ pub fn resolve_changesets( head_branch: &str, ) -> Result, DiffError> { match StackModel::detect(repo) { - model @ (StackModel::Graphite | StackModel::GhStack) => { - Ok(assemble_changesets(repo, head_branch, model)?) - } - StackModel::None | StackModel::Git => Ok(vec![Changeset { - name: head_branch.to_string(), - source: ChangesetSource::Uncommitted, - title: None, - current: true, - needs_restack: false, - }]), + model @ (StackModel::Graphite | StackModel::GhStack) => Ok(assemble_changesets( + repo, + head_branch, + model, + UncommittedLayer::Include, + )?), + StackModel::None | StackModel::Git => Ok(vec![uncommitted_changeset(head_branch)]), + } +} + +/// The single synthetic [`ChangesetSpan::Uncommitted`] changeset for `head_branch` — always +/// `current`, no title, no restack question. Shared by [`resolve_changesets`]'s non-Graphite +/// fallback arm and the review binary's `uncommitted` keyword (`crate::source`), both of which +/// mean the same thing: "just diff the worktree." +pub fn uncommitted_changeset(head_branch: &str) -> Changeset { + Changeset { + name: head_branch.to_string(), + span: ChangesetSpan::Uncommitted, + title: None, + current: true, + needs_restack: false, } } diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index eda9a929..a4546454 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -13,7 +13,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::path::Path; use git2::Repository; -use workon::{Changeset, ChangesetSource}; +use workon::{Changeset, ChangesetSpan}; use crate::acquire::{ChangesetDiff, WorktreeDiffs}; use crate::align::{align_file, collapse_gaps, inline_rows, CellKind, DisplayRow, InlineRow, Row}; @@ -25,6 +25,7 @@ use crate::ops; use crate::outline::{self, OutlineChangeset, OutlineFile, OutlineItem, OutlineMode}; use crate::queue::{OpOutcome, StagingOp, StagingQueue}; use crate::refresh::{IndexSignature, RefreshCoordinator}; +use crate::source::{resolve_source, Source}; use crate::stage_op::{FileStagingOp, LineSelectionOp}; use crate::synthesis::LineSelection; use crate::wordiff::{word_diff_spans, Span}; @@ -312,12 +313,18 @@ impl FileView { /// not all of `App` — a `&self` method here would make the borrow checker treat the tree as /// blocking every OTHER field access (e.g. `&mut self.highlighter`) for its whole lifetime, even /// though the two never actually conflict. -fn old_side_tree_for(repo: &Repository, source: ChangesetSource) -> Option> { - match source { - ChangesetSource::Committed { base, .. } => { - repo.find_commit(base).and_then(|c| c.tree()).ok() - } - ChangesetSource::Uncommitted => repo.head().and_then(|h| h.peel_to_tree()).ok(), +fn old_side_tree_for(repo: &Repository, span: ChangesetSpan) -> Option> { + match span { + ChangesetSpan::Committed { base, .. } => repo.find_commit(base).and_then(|c| c.tree()).ok(), + // Root commit reviewed on its own: the old side is the empty tree. `treebuilder(None)` + // builds (and `write` persists, idempotently — git's well-known empty-tree object) an + // empty tree without needing a real parent commit to peel. + ChangesetSpan::CommittedRoot { .. } => repo + .treebuilder(None) + .and_then(|b| b.write()) + .and_then(|oid| repo.find_tree(oid)) + .ok(), + ChangesetSpan::Uncommitted => repo.head().and_then(|h| h.peel_to_tree()).ok(), } } @@ -330,12 +337,12 @@ fn old_side_tree_for(repo: &Repository, source: ChangesetSource) -> Option Option> { - match source { - ChangesetSource::Committed { head, .. } => { +fn new_side_tree_for(repo: &Repository, span: ChangesetSpan) -> Option> { + match span { + ChangesetSpan::Committed { head, .. } | ChangesetSpan::CommittedRoot { head } => { repo.find_commit(head).and_then(|c| c.tree()).ok() } - ChangesetSource::Uncommitted => None, + ChangesetSpan::Uncommitted => None, } } @@ -741,6 +748,25 @@ pub struct App { /// every key as a modal (mirroring [`Self::pending_confirm`]'s capture) — see its doc comment /// for the precedence between the two modals. pub help_visible: bool, + /// The `git workon review []` argument the session was launched with, set via + /// [`Self::set_review_source`] (M7 CS2 fix). `None` means the session was launched via + /// no-argument auto-detect (`crate::acquire::resolve_changesets`); `Some(source)` means an + /// explicit ask (`stack`, `uncommitted`, and later CS3/CS4's ref/range/PR variants) that + /// [`Self::refresh`] must re-resolve on every refresh, NEVER downgrade to auto-detect — a + /// setter (rather than a constructor parameter) so `App::from_changesets`'s signature, and + /// every existing test building through it, stays untouched. + review_source: Option, + /// CS4's idle-deferred load switch. `false` (the default) keeps every pre-CS4 + /// `open_current`/render-path behavior byte-identical, so the ~80 existing tests asserting + /// eager loads keep passing unchanged. `main.rs` turns this on via [`Self::set_defer_loads`] + /// right after construction; the event loop is what actually defers (see `tui.rs`'s + /// `OPEN_DEBOUNCE`). + defer_loads: bool, + /// Set when [`Self::open_current`] deferred its load (only possible while + /// [`Self::defer_loads`] is on) — the render path shows a placeholder instead of loading + /// while this is `true`, and the event loop calls [`Self::complete_pending_open`] once input + /// has been quiet for `OPEN_DEBOUNCE`. Read via [`Self::open_pending`]. + open_pending: bool, } /// A destructive staging op deferred behind a [`Confirm`], identified by index into [`App::files`] @@ -806,7 +832,7 @@ impl App { .unwrap_or_default(); let cs = Changeset { name, - source: ChangesetSource::Uncommitted, + span: ChangesetSpan::Uncommitted, title: None, current: true, needs_restack: false, @@ -875,6 +901,9 @@ impl App { refresh_coordinator, outline, help_visible: false, + review_source: None, + defer_loads: false, + open_pending: false, }; // Position the outline cursor on the changeset/file the lib marked `current` (the same // row `sync_outline_to_current` would reposition to after any diff-initiated nav) rather @@ -884,6 +913,15 @@ impl App { app } + /// Record the `[SOURCE]` argument the review session was launched with, so + /// [`Self::refresh`] re-resolves that same ask instead of silently falling back to + /// no-argument auto-detect (M7 CS2 fix). `main.rs` calls this right after + /// [`Self::from_changesets`] whenever a `[SOURCE]` argument was given; a no-argument launch + /// never calls it, leaving [`Self::review_source`] at its `None` default. + pub fn set_review_source(&mut self, source: Source) { + self.review_source = Some(source); + } + /// The current `.git/index`'s cheap fingerprint (mtime + size), or `None` if the read fails — /// tolerated rather than propagated, since a transient read error (e.g. a concurrent git /// process mid-write) must not crash the TUI or wedge the tick loop; the next tick just tries @@ -966,12 +1004,15 @@ impl App { } /// Whether the ACTIVE changeset is a committed range (`base..head`) rather than the - /// uncommitted worktree layer — derived from [`workon::ChangesetSource`] on every call rather + /// uncommitted worktree layer — derived from [`workon::ChangesetSpan`] on every call rather /// than cached (locked decision #2's "derive, don't store" mode gate). Drives every /// committed-mode guard: the mode-aware staging refusal, skipping combined attribution (no /// staged/unstaged sets exist to color by), and locking zoom to combined. pub fn is_committed(&self) -> bool { - matches!(self.cur().cs.source, ChangesetSource::Committed { .. }) + matches!( + self.cur().cs.span, + ChangesetSpan::Committed { .. } | ChangesetSpan::CommittedRoot { .. } + ) } /// Re-run [`crate::acquire::resolve_changesets`] against the CURRENT `HEAD` branch and @@ -1000,6 +1041,16 @@ impl App { /// /// On any assembly/diff error, leaves all existing state untouched and sets an error /// [`Notice`] instead (via [`Self::notify`]) — a failed refresh must never blank the review. + /// + /// Dispatches on [`Self::review_source`] (M7 CS2 fix): a no-argument launch (`None`) re-runs + /// today's auto-detect ([`crate::acquire::resolve_changesets`]); an explicit-source launch + /// (`Some`) re-runs [`crate::source::resolve_source`] against THAT source, never auto-detect + /// — every ref-shaped source variant (`Stack`, `Uncommitted`, `Ref`, `Range`) is offline, + /// so re-resolving on every refresh (manual `r` and the tick-driven index watcher alike) is + /// cheap and safe. Without this, both refresh triggers would silently swap an explicit + /// review (e.g. `uncommitted`) for the current `HEAD`'s auto-detected state. + /// [`Source::Pr`] is the one exception: it resolves over the network (gh metadata + fetch), + /// so refresh is a no-op for it — see the match arm below. pub fn refresh(&mut self) { let Some(head_branch) = self .repo @@ -1011,7 +1062,18 @@ impl App { return; }; - let changesets = match crate::acquire::resolve_changesets(&self.repo, &head_branch) { + let changesets = match &self.review_source { + None => crate::acquire::resolve_changesets(&self.repo, &head_branch) + .map_err(|err| err.to_string()), + // A PR review is committed-only: nothing it renders depends on the index/worktree + // state that refresh exists to pick up, and re-resolving would hit the network + // (gh metadata + fetch) on every tick-driven refresh. Remote freshness is a + // re-launch, not a refresh. + Some(Source::Pr(_)) => return, + Some(source) => resolve_source(&self.repo, &head_branch, source.clone()) + .map_err(|err| err.to_string()), + }; + let changesets = match changesets { Ok(cs) => cs, Err(err) => { self.notify(format!("refresh failed: {err}"), Severity::Error); @@ -1019,16 +1081,18 @@ impl App { } }; - let mut views = Vec::with_capacity(changesets.len()); - for cs in changesets { - match crate::acquire::diff_changeset(&self.repo, &cs) { - Ok(diff) => views.push(ChangesetView::from_changeset_diff(cs, diff)), - Err(err) => { - self.notify(format!("refresh failed: {err}"), Severity::Error); - return; - } + let diffs = match crate::acquire::diff_changesets(&self.repo, &changesets) { + Ok(diffs) => diffs, + Err(err) => { + self.notify(format!("refresh failed: {err}"), Severity::Error); + return; } - } + }; + let views: Vec = changesets + .into_iter() + .zip(diffs) + .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) + .collect(); // `resolve_changesets` always returns at least one changeset (a lone Uncommitted entry // when no stack is active), but stay defensive rather than index an empty `Vec` below. if views.is_empty() { @@ -1161,8 +1225,12 @@ impl App { } /// Read-only access to file `idx`'s already-loaded [`FileView`] for `role` (`None` if the role - /// has no change for the file, or it isn't loaded yet). - pub(crate) fn role_view_ref(&self, idx: usize, role: Role) -> Option<&FileView> { + /// has no change for the file, or it isn't loaded yet). `pub` (not `pub(crate)`) so the + /// separate `git-workon-review` bin crate's `tui.rs` tests can assert a file was — or, more + /// importantly, was NOT — loaded without visiting it (CS2's event-coalescing regression + /// test); read-only and does not touch `open_current`/`ensure_loaded`/`outline_move_by`'s + /// eager-load semantics. + pub fn role_view_ref(&self, idx: usize, role: Role) -> Option<&FileView> { self.views_for(role).get(idx).and_then(|v| v.as_ref()) } @@ -1256,18 +1324,18 @@ impl App { // Combined role. // Re-peeled per call rather than cached on `App`: for the uncommitted layer `HEAD` can // move between file loads, and the tree is cheap to re-peel either way. - // `self.cur().cs.source` is `Copy`, so reading it here borrows `self` only for this + // `self.cur().cs.span` is `Copy`, so reading it here borrows `self` only for this // sub-expression — `head_tree` itself ends up borrowing `self.repo` alone (via the free // `old_side_tree_for`), leaving `&mut self.highlighter` free below. A method tied to // `&self` would instead have bound the tree's lifetime to all of `self`. - let Some(head_tree) = old_side_tree_for(&self.repo, self.cur().cs.source) else { + let Some(head_tree) = old_side_tree_for(&self.repo, self.cur().cs.span) else { return; }; // New-side source mirrors the old side: `None` (worktree) for the uncommitted layer, // the changeset's `head` tree for a committed changeset. Same free-fn borrow dance as // `old_side_tree_for` — both trees borrow only `self.repo`, so `&mut self.highlighter` // stays free for `FileView::load`. - let new_tree = new_side_tree_for(&self.repo, self.cur().cs.source); + let new_tree = new_side_tree_for(&self.repo, self.cur().cs.span); let file = self.cur().diff.files[idx].clone(); let view = FileView::load( &self.repo, @@ -1336,10 +1404,81 @@ impl App { self.derive_scroll(); } + /// Turn CS4's idle-deferred load mode on/off. `main.rs` calls this with `true` right after + /// [`Self::from_changesets`], before the first [`Self::open_current`] — see the field's doc + /// comment. Exposed as a setter (rather than folded into construction) so every existing test + /// building through `from_changesets`/`App::new` keeps today's eager behavior untouched. + pub fn set_defer_loads(&mut self, on: bool) { + self.defer_loads = on; + } + + /// Whether CS4's idle-deferred load mode is on — see [`Self::set_defer_loads`]. + pub fn defer_loads(&self) -> bool { + self.defer_loads + } + + /// Whether [`Self::open_current`] deferred its load and it hasn't been completed yet — the + /// render path (in defer mode) and the event loop both read this: render to decide whether to + /// show the placeholder, the event loop to decide whether to shorten its poll timeout and to + /// call [`Self::complete_pending_open`] on the next idle tick. + pub fn open_pending(&self) -> bool { + self.open_pending + } + /// Load the current file's needed views and reset both panes to their first hunks. + /// + /// In [`Self::defer_loads`] mode a file whose views are NOT yet cached does not load here: + /// the open is marked pending and the panes reset anyway (the cursor falls back to row 0 + /// for the still-unloaded view, via [`Self::role_first_hunk`]'s `unwrap_or(0)` — harmless, + /// since the body renders a placeholder until [`Self::complete_pending_open`] runs). A file + /// whose views ARE cached takes the eager path even in defer mode: `ensure_loaded` is a + /// pure cache hit there, and deferring would only trade an instantly-renderable diff for a + /// placeholder flash lasting the debounce window — revisiting a file is the most common + /// navigation of all, and it must render immediately. Outside defer mode this is exactly + /// the pre-defer eager behavior. pub fn open_current(&mut self) { + if self.defer_loads && !self.current_views_cached() { + self.open_pending = true; + self.reset_panes(); + return; + } + self.ensure_loaded(self.current); + self.reset_panes(); + } + + /// Whether the view(s) the current file's effective zoom needs are already cached, making a + /// deferred open pointless (`ensure_loaded` would be a cache hit). Split checks EITHER pane: + /// a role with no change for the file stays legitimately `None` forever (see + /// [`Self::ensure_role_loaded`]), so requiring both would defer a one-role file every time. + /// A partially-cached split (one loadable pane in, one missing) takes the eager path and + /// loads the single missing pane synchronously — one file, cheap, and consistent with the + /// both-`None` gate the render placeholder uses. + fn current_views_cached(&self) -> bool { + match self.effective_zoom_for(self.current) { + EffectiveZoom::Single(role) => self.role_view_ref(self.current, role).is_some(), + EffectiveZoom::Split => { + self.role_view_ref(self.current, Role::Unstaged).is_some() + || self.role_view_ref(self.current, Role::Staged).is_some() + } + } + } + + /// Complete a deferred open, if one is pending: load the current file's needed views, then + /// reset both panes again so the cursor now derives from the REAL first-hunk row (rather than + /// the `0` fallback [`Self::open_current`] left it at). A no-op when nothing is pending — + /// idempotent, so the event loop can call this liberally (e.g. on every idle tick while + /// pending) without worrying about double-loading. + /// + /// Invariant this pins (the equivalence the tests assert): after this returns, `App` state is + /// byte-identical to what an eager [`Self::open_current`] would have produced for the same + /// current file. + pub fn complete_pending_open(&mut self) { + if !self.open_pending { + return; + } self.ensure_loaded(self.current); self.reset_panes(); + self.open_pending = false; } /// Cycle the requested zoom `Split → Combined → Unstaged → Staged → Split` (`z`). The new zoom @@ -1596,12 +1735,22 @@ impl App { /// Move the outline's own cursor by `delta` rows (`j`/`k` while the outline has focus), /// clamped into the current row list. Landing on a FILE row jumps the diff there - /// immediately (outline -> diff, per the locked design); landing on a HEADER row does NOT - /// jump — only [`Self::outline_confirm`] (`Enter`) jumps from a header, since a header's - /// "first file" isn't necessarily where a `j`/`k` scan through the stack should keep - /// stopping the diff. This calls [`Self::switch_changeset`] directly (not `next_file`/ - /// `goto_changeset`), so it does NOT re-trigger [`Self::sync_outline_to_current`] — see that - /// method's doc comment for why only the DIFF-initiated entry points do. + /// immediately (outline -> diff, per the locked design); a HEADER/DIR row itself never + /// causes a jump — only [`Self::outline_confirm`] (`Enter`) jumps from a header, since a + /// header's "first file" isn't necessarily where a `j`/`k` scan through the stack should + /// keep stopping the diff. This calls [`Self::switch_changeset`] directly (not + /// `next_file`/`goto_changeset`), so it does NOT re-trigger + /// [`Self::sync_outline_to_current`] — see that method's doc comment for why only the + /// DIFF-initiated entry points do. + /// + /// A multi-row `delta` is a coalesced burst of unit presses (the event loop merges + /// same-sign `j`/`k` runs — see `tui.rs`'s `update_batch`), so it must be + /// indistinguishable from the unit presses it stands for: N unit moves jump the diff at + /// every FILE row they cross, leaving it on the LAST one when the run stops on a + /// header/dir row. So a non-File landing scans back toward (but excluding) the starting + /// row for the last file crossed and jumps there. For a unit move that range is empty, + /// preserving the single-press rule above: bare `j`/`k` onto a header neither jumps nor + /// resets the diff. pub fn outline_move_by(&mut self, delta: i64) { let items = self.outline_items(); if items.is_empty() { @@ -1617,6 +1766,19 @@ impl App { } = &items[new_idx] { self.switch_changeset(*cs_idx, *file_idx); + } else if new_idx as i64 != cur { + let step = if delta > 0 { -1 } else { 1 }; + let mut idx = new_idx as i64 + step; + while idx != cur && (0..=max).contains(&idx) { + if let OutlineItem::File { + cs_idx, file_idx, .. + } = &items[idx as usize] + { + self.switch_changeset(*cs_idx, *file_idx); + break; + } + idx += step; + } } } @@ -2478,12 +2640,14 @@ fn current_cs_index(changesets: &[ChangesetView]) -> usize { /// base rev (7-char short-sha), or `"HEAD"` for the uncommitted layer (worktree ↔ `HEAD`, /// unchanged from M2–M4). fn base_label_for(cs: &Changeset) -> String { - match cs.source { - ChangesetSource::Committed { base, .. } => { + match cs.span { + ChangesetSpan::Committed { base, .. } => { let full = base.to_string(); full.chars().take(7).collect() } - ChangesetSource::Uncommitted => "HEAD".to_string(), + // No real base commit to abbreviate — the base is the empty tree. + ChangesetSpan::CommittedRoot { .. } => "(empty)".to_string(), + ChangesetSpan::Uncommitted => "HEAD".to_string(), } } @@ -2629,7 +2793,7 @@ pub(crate) mod test_support { mod tests { use git2::Repository; use git_workon_fixture::prelude::*; - use workon::{Changeset, ChangesetSource}; + use workon::{Changeset, ChangesetSpan}; use super::test_support::app_from_fixture; use super::{ @@ -2742,6 +2906,116 @@ mod tests { assert!(app.current_view_ref().is_none()); } + // ── CS4: idle-deferred loads ────────────────────────────────────────────── + + /// A twin pair: one `App` with `defer_loads` off (the eager baseline), one with it on. Both + /// built from independent copies of the SAME fixture so their diffs (and hunks) line up. + fn defer_and_eager_twins(fixture: &git_workon_fixture::fixture::Fixture) -> (App, App) { + let mut eager = app_from_fixture(fixture); + eager.open_current(); + + let mut deferred = app_from_fixture(fixture); + deferred.set_defer_loads(true); + deferred.open_current(); + + (deferred, eager) + } + + #[test] + fn open_current_defers_load_and_complete_matches_eager_open() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nold\nl10\nl11\nl12\n", + "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nnew\nl10\nl11\nl12\n", + ) + .build() + .unwrap(); + + let (mut deferred, eager) = defer_and_eager_twins(&fixture); + + // `open_current` under defer mode loads NOTHING and marks the open pending. + assert!( + deferred.current_view_ref().is_none(), + "deferred open_current must not have loaded the current view" + ); + assert!(deferred.open_pending(), "the open must be marked pending"); + + deferred.complete_pending_open(); + + assert!( + !deferred.open_pending(), + "complete_pending_open must clear the pending flag" + ); + assert_eq!( + deferred.cursor, eager.cursor, + "cursor must land on the same (first-hunk) row an eager open would have" + ); + assert_eq!(deferred.scroll, eager.scroll); + let deferred_view = deferred.current_view_ref().expect("view now loaded"); + let eager_view = eager.current_view_ref().expect("eager view loaded"); + assert_eq!(deferred_view.old_text(), eager_view.old_text()); + assert_eq!(deferred_view.new_text(), eager_view.new_text()); + assert_eq!(deferred_view.display.len(), eager_view.display.len()); + } + + #[test] + fn revisiting_a_cached_file_reopens_eagerly_without_a_pending_window() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "1\n2\n3\n", "1\nA\n3\n") + .unstaged_file("b.txt", "1\n2\n3\n", "1\nB\n3\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); // a.txt: uncached — defers + assert!(app.open_pending(), "an uncached file defers its open"); + app.complete_pending_open(); + + app.current = 1; + app.open_current(); // b.txt: uncached — defers + assert!(app.open_pending(), "a different uncached file still defers"); + app.complete_pending_open(); + + app.current = 0; + app.open_current(); // back to a.txt: cached — must NOT defer + assert!( + !app.open_pending(), + "revisiting a cached file must reopen eagerly — a pending window here would \ + flash the loading placeholder over an instantly-renderable diff" + ); + assert!( + app.current_view_ref().is_some(), + "the cached view is available the moment the open returns" + ); + } + + #[test] + fn complete_pending_open_is_a_no_op_when_nothing_pending() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("tracked.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); + app.complete_pending_open(); + assert!(!app.open_pending()); + + let cursor_before = app.cursor; + let scroll_before = app.scroll; + // Calling again with nothing pending must not touch cursor/scroll or reload anything. + app.complete_pending_open(); + assert!(!app.open_pending()); + assert_eq!(app.cursor, cursor_before); + assert_eq!(app.scroll, scroll_before); + } + // Hunk-nav helpers below operate purely over `DisplayRow` vectors — no fixture repo needed. fn ctx_row(n: usize) -> DisplayRow { @@ -3595,6 +3869,115 @@ mod tests { assert_eq!(app.zoom, Zoom::Combined, "refresh must not reset zoom"); } + /// M7 CS2 fix: a session launched with an explicit `[SOURCE]` argument must have `refresh` + /// re-resolve THAT source, never silently downgrade to no-argument auto-detect. A Graphite + /// stack is active (`assemble_changesets` would return the whole `a`/`b` stack for + /// auto-detect), but the session was launched with `uncommitted` — so both the manual `r` + /// key and the tick-driven index watcher must keep showing only the single uncommitted + /// changeset, not swap in the full stack. + #[test] + fn refresh_re_resolves_the_launched_source_instead_of_auto_detecting() { + use crate::source::Source; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .untracked_file("scratch.txt", "hi\n") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + // `App::refresh` re-derives the branch from the repo's ACTUAL `HEAD`, not from a name + // handed to `resolve_source` — so the fixture's checkout must really be on "b" for + // auto-detect (were the fix absent) to see the `a`/`b` stack, not `main`. + repo.set_head("refs/heads/b").unwrap(); + repo.checkout_head(None).unwrap(); + + let source = Source::Uncommitted; + let changesets = + crate::source::resolve_source(repo, "b", source.clone()).expect("resolve_source"); + assert_eq!( + changesets.len(), + 1, + "uncommitted keyword always resolves to exactly one changeset" + ); + let mut views = Vec::with_capacity(changesets.len()); + for cs in changesets { + let diff = crate::acquire::diff_changeset(repo, &cs).unwrap(); + views.push(ChangesetView::from_changeset_diff(cs, diff)); + } + + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, views); + app.set_review_source(source); + app.open_current(); + + app.refresh(); + + assert_eq!( + app.changeset_count(), + 1, + "refresh must keep reviewing only the uncommitted changeset, not the full \ + Graphite stack auto-detect would find" + ); + assert_eq!(app.cur().cs.span, ChangesetSpan::Uncommitted); + } + + /// A PR-sourced review must survive refresh untouched: re-resolving would hit the network + /// (gh + fetch), so [`App::refresh`] no-ops for [`Source::Pr`]. The fixture has no PR and no + /// gh — if refresh DID try to re-resolve, `resolve_pr` would fail and raise a "refresh + /// failed" notice; asserting no notice (and unchanged views) pins the no-op. + #[test] + fn refresh_is_a_no_op_for_a_pr_source() { + use crate::source::Source; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + fixture + .commit("main") + .file("a.txt", "one\n") + .create("first") + .unwrap(); + fixture + .commit("main") + .file("a.txt", "two\n") + .create("second") + .unwrap(); + let repo = fixture.repo().unwrap(); + + let head = repo.head().unwrap().peel_to_commit().unwrap(); + let base = head.parent(0).unwrap(); + let cs = workon::Changeset { + name: "pr-1".to_string(), + span: ChangesetSpan::Committed { + base: base.id(), + head: head.id(), + }, + title: Some("a pr".to_string()), + current: true, + needs_restack: false, + }; + let diff = crate::acquire::diff_changeset(repo, &cs).unwrap(); + let views = vec![ChangesetView::from_changeset_diff(cs, diff)]; + + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, views); + app.set_review_source(Source::Pr("pr-1".to_string())); + app.open_current(); + + app.refresh(); + + assert_eq!(app.changeset_count(), 1); + assert_eq!(app.cur().cs.name, "pr-1"); + assert!( + app.notice.is_none(), + "a PR-source refresh must no-op, not attempt (and fail) a network re-resolution" + ); + } + // ---- M4 index watcher (`on_tick`) ------------------------------------------------------- /// Stage `path` in the fixture's index, exactly as an external `git add` would — the write @@ -4482,8 +4865,8 @@ mod tests { assert_eq!(app.current_cs(), 0); assert_eq!(app.base_label, "HEAD"); assert!(matches!( - app.current_changeset().source, - ChangesetSource::Uncommitted + app.current_changeset().span, + ChangesetSpan::Uncommitted )); } @@ -4507,7 +4890,7 @@ mod tests { let repo = fixture.repo().unwrap(); let cs = Changeset { name: "main".to_string(), - source: ChangesetSource::Committed { base, head }, + span: ChangesetSpan::Committed { base, head }, title: None, current: true, needs_restack: false, @@ -4563,7 +4946,7 @@ mod tests { let repo = fixture.repo().unwrap(); let cs = Changeset { name: "main".to_string(), - source: ChangesetSource::Committed { base, head }, + span: ChangesetSpan::Committed { base, head }, title: None, current: true, needs_restack: false, @@ -4598,14 +4981,14 @@ mod tests { // Deliberately NOT current — listed first, so a naive "open index 0" would pick it. let not_current = Changeset { name: "not-current".to_string(), - source: ChangesetSource::Committed { base, head: base }, + span: ChangesetSpan::Committed { base, head: base }, title: None, current: false, needs_restack: false, }; let current = Changeset { name: "current".to_string(), - source: ChangesetSource::Committed { base, head }, + span: ChangesetSpan::Committed { base, head }, title: None, current: true, needs_restack: false, @@ -4663,7 +5046,7 @@ mod tests { let cs_a = Changeset { name: "cs-a".to_string(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: root, head: mid, }, @@ -4673,7 +5056,7 @@ mod tests { }; let cs_b = Changeset { name: "cs-b".to_string(), - source: ChangesetSource::Committed { base: mid, head }, + span: ChangesetSpan::Committed { base: mid, head }, title: None, current: false, needs_restack: false, @@ -4797,7 +5180,7 @@ mod tests { let cs_a = Changeset { name: "cs-a".to_string(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: root, head: mid, }, @@ -4807,7 +5190,7 @@ mod tests { }; let cs_b = Changeset { name: "cs-b".to_string(), - source: ChangesetSource::Committed { base: mid, head }, + span: ChangesetSpan::Committed { base: mid, head }, title: None, current: true, needs_restack: false, @@ -4977,14 +5360,14 @@ mod tests { let committed = Changeset { name: "committed".to_string(), - source: ChangesetSource::Committed { base, head }, + span: ChangesetSpan::Committed { base, head }, title: Some("Committed work".to_string()), current: false, needs_restack: false, }; let uncommitted = Changeset { name: "uncommitted".to_string(), - source: ChangesetSource::Uncommitted, + span: ChangesetSpan::Uncommitted, title: None, current: true, needs_restack: false, @@ -5120,7 +5503,7 @@ mod tests { let cs_a = Changeset { name: "cs-a".to_string(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: root, head: mid, }, @@ -5130,7 +5513,7 @@ mod tests { }; let cs_b = Changeset { name: "cs-b".to_string(), - source: ChangesetSource::Committed { base: mid, head }, + span: ChangesetSpan::Committed { base: mid, head }, title: None, current: true, needs_restack: true, @@ -5227,17 +5610,52 @@ mod tests { fn outline_move_by_on_a_header_row_does_not_jump_the_diff() { let mut app = two_committed_changesets_two_and_one_files(); app.outline.mode = OutlineMode::Stack; - app.outline.cursor = 0; // cs-a's header row - let cs_before = app.current_cs(); - let file_before = app.current; - // Header rows sit at indices 0 (cs-a) and 3 (cs-b) in Stack mode (header, a1, a2, - // header). Move onto the cs-b header without landing on a file row in between. - app.outline_move_by(3); + // header, b1). Park the diff on a2, cursor on its row. + app.outline.cursor = 2; + app.switch_changeset(0, 1); + app.cursor += 1; // nudge off the open position so a hidden re-open would be visible + let cursor_before = app.cursor; + + // A UNIT move onto the header: the header itself never jumps — and must not reset the + // diff's cursor either (a re-`switch_changeset` to the same file would). + app.outline_move_by(1); assert_eq!( (app.current_cs(), app.current), - (cs_before, file_before), - "landing the outline cursor on a header row must not move the diff" + (0, 1), + "a bare j onto a header row must not move the diff" + ); + assert_eq!(app.cursor, cursor_before, "...nor reset the diff cursor"); + } + + #[test] + fn coalesced_outline_burst_onto_a_header_matches_sequential_unit_moves() { + // A multi-row delta is CS2's coalesced stand-in for N unit presses, so the two must be + // indistinguishable — including which file the diff follows when the burst stops on a + // header row (the LAST file crossed, exactly where unit presses leave it). + let mut coalesced = two_committed_changesets_two_and_one_files(); + coalesced.outline.mode = OutlineMode::Stack; + coalesced.outline.cursor = 0; + coalesced.outline_move_by(3); // header -> a1 -> a2 -> cs-b header + + let mut sequential = two_committed_changesets_two_and_one_files(); + sequential.outline.mode = OutlineMode::Stack; + sequential.outline.cursor = 0; + for _ in 0..3 { + sequential.outline_move_by(1); + } + + assert_eq!(coalesced.outline.cursor, sequential.outline.cursor); + assert_eq!( + (coalesced.current_cs(), coalesced.current), + (sequential.current_cs(), sequential.current), + "a summed burst stopping on a header must leave the diff on the last file \ + crossed, like the unit presses it coalesces" + ); + assert_eq!( + (coalesced.current_cs(), coalesced.current), + (0, 1), + "...which is a2 here" ); } @@ -5351,7 +5769,7 @@ mod tests { let cs = Changeset { name: "cs".to_string(), - source: ChangesetSource::Committed { base: root, head }, + span: ChangesetSpan::Committed { base: root, head }, title: None, current: true, needs_restack: false, diff --git a/git-workon-review/src/error.rs b/git-workon-review/src/error.rs index 7fa8865e..a1f9edea 100644 --- a/git-workon-review/src/error.rs +++ b/git-workon-review/src/error.rs @@ -28,6 +28,11 @@ pub enum ReviewError { #[error(transparent)] #[diagnostic(transparent)] Apply(#[from] ApplyError), + + /// A `git workon review ` argument failed to resolve to changesets + #[error(transparent)] + #[diagnostic(transparent)] + Source(#[from] SourceError), } /// Errors building a [`crate::model::DiffModel`] from git2 structures, or acquiring one for a @@ -118,3 +123,81 @@ pub enum ApplyError { source: std::io::Error, }, } + +/// Errors resolving a `git workon review ` positional argument to changesets +/// (ADR-036: the classifier/resolver seam is [`crate::source::Source`]). +#[derive(Error, Diagnostic, Debug)] +pub enum SourceError { + /// The `stack` keyword found no Graphite metadata and `branch` has no upstream to infer a + /// git-only stack from. An explicit ask deserves an explicit failure — never a silent + /// fall-through to the uncommitted layer (ADR-036). + #[error("branch '{branch}' has no Graphite stack and no upstream to infer one from")] + #[diagnostic( + code(workon::review::stack_no_upstream), + help( + "set an upstream (git branch --set-upstream-to=/{branch}), \ + or run 'git workon review uncommitted'" + ) + )] + NoUpstream { branch: String }, + + /// Assembling the requested stack failed for a reason other than a missing upstream + /// (broken Graphite metadata, an unresolvable branch, a bad recorded parent revision). + #[error("failed to assemble the stack for '{branch}'")] + #[diagnostic(code(workon::review::stack_resolution_failed))] + StackResolutionFailed { + branch: String, + #[source] + source: workon::WorkonError, + }, + + /// A `` argument (or one side of a `Range`) doesn't rev-parse to anything reviewable — + /// a typo, a deleted branch, a garbage commit-ish. + #[error("cannot resolve '{text}' as a review source")] + #[diagnostic( + code(workon::review::unresolvable_source), + help( + "try 'stack', 'uncommitted', a branch/tag/commit, a..b / a...b range, \ + or a PR reference (pr-123, #123)" + ) + )] + UnresolvableSource { text: String }, + + /// An untracked (or remote-tracking) `` branch has neither an upstream nor a resolvable + /// trunk to compute "what this branch adds" from. + #[error("branch '{branch}' has no upstream and no trunk to compute a base from")] + #[diagnostic( + code(workon::review::no_base_for_branch), + help( + "set an upstream (git branch --set-upstream-to=/{branch}), \ + or ensure a trunk branch (main/master) exists" + ) + )] + NoBaseForBranch { branch: String }, + + /// `check_gh_available` found no working `gh` CLI — a PR reference can't resolve without it, + /// the same requirement `git workon #123`'s own PR workflow has. + #[error("'{text}' is a PR reference, but gh is not available")] + #[diagnostic( + code(workon::review::gh_unavailable), + help("install the gh CLI and run 'gh auth login', then retry") + )] + GhUnavailable { + text: String, + #[source] + source: workon::WorkonError, + }, + + /// Resolving a PR reference failed after `gh` was confirmed available: an unknown PR number, + /// `gh` not authenticated, a fork remote/fetch failure, or a missing base/head ref. + #[error("failed to resolve PR reference '{text}'")] + #[diagnostic( + code(workon::review::pr_resolution_failed), + help("check the PR number and that 'gh auth status' is logged in") + )] + PrResolutionFailed { + text: String, + #[source] + source: workon::WorkonError, + }, +} diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index fb07145c..b45d452d 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -28,6 +28,7 @@ pub mod outline; pub mod queue; pub mod refresh; pub mod render; +pub mod source; pub mod stage_op; pub mod synthesis; pub mod terminal_query; diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index 0db3e30e..6425cec1 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -1,20 +1,27 @@ mod tui; use clap::{CommandFactory, Parser}; +use clap_complete::engine::ArgValueCompleter; use clap_complete::env::CompleteEnv; use git2::Repository; use miette::{IntoDiagnostic, Result}; -use workon_review::acquire::{diff_changeset, resolve_changesets}; +use workon_review::acquire::{diff_changesets, resolve_changesets}; use workon_review::app::{App, ChangesetView, Severity}; use workon_review::config::{self, ReviewConfig}; use workon_review::keymap::Keymap; +use workon_review::source::{complete_source, resolve_source, Source}; use workon_review::terminal_query; use workon_review::theme::Palette; /// A TUI for reviewing changesets #[derive(Debug, Parser)] #[clap(about, author, bin_name = env!("CARGO_PKG_NAME"), version)] -struct Cli {} +struct Cli { + /// What to review: stack, uncommitted, a ref (branch/tag/commit), a..b / a...b range, or + /// a PR reference + #[arg(value_name = "SOURCE", add = ArgValueCompleter::new(complete_source))] + source: Option, +} fn main() -> Result<()> { // Respond to the `COMPLETE=` dynamic-completion protocol before anything else — mirrors @@ -22,7 +29,7 @@ fn main() -> Result<()> { // what lets git-workon delegate `git workon review ` completion here (M6 CS3). CompleteEnv::with_factory(Cli::command).complete(); - Cli::parse(); + let cli = Cli::parse(); let repo = Repository::discover(".").into_diagnostic()?; let branch = repo @@ -32,19 +39,45 @@ fn main() -> Result<()> { .into_diagnostic()? .to_string(); - // `resolve_changesets` is the M5 entry point (locked decision #7, auto-detect): the full - // Graphite stack when one is active, or a single synthetic uncommitted changeset otherwise - // — the latter keeps a non-Graphite repo byte-identical to M2–M4's `diff_uncommitted` path. - let changesets = resolve_changesets(&repo, &branch).into_diagnostic()?; - - let mut views = Vec::with_capacity(changesets.len()); - for cs in changesets { - let diff = diff_changeset(&repo, &cs).into_diagnostic()?; - views.push(ChangesetView::from_changeset_diff(cs, diff)); - } + // No `[SOURCE]` argument: the M5 auto-detect entry point (locked decision #7), unchanged — + // the full Graphite stack when one is active, or a single synthetic uncommitted changeset + // otherwise (keeps a non-Graphite repo byte-identical to M2–M4's `diff_uncommitted` path). + // A `[SOURCE]` argument routes through the ADR-036 classifier/resolver instead (M7 CS2/CS3). + // `source` is kept (not just the resolved changesets) so it can be handed to `App` below — + // `App::refresh` re-runs THIS same ask on every refresh rather than downgrading to + // auto-detect (M7 CS2 fix). + // + // Everything from here through the theme probe runs BEFORE the terminal is taken (CS5's + // splash enters the alternate screen further down, for the diff/build phase only). That + // ordering is deliberate, not incidental: + // - PR resolution fetches over the network, and auth-git2 may interactively PROMPT for an + // ssh passphrase / https credentials — inside raw-mode alternate screen the prompt would + // stair-step over the splash and leave the user typing blind. + // - "nothing to review" exits below without ever needing a tty (CI, test harnesses). + // - The `theme = auto` probe must own the tty while it converses, and its straggler flush + // discards ALL pending input — flushing before the alternate screen appears means nothing + // a user types at a visible TUI is ever eaten (a q typed right after the screen flips + // must quit, not vanish; see pty_smoke.rs's silent-terminal test). + // Resolve itself is milliseconds locally, so the splash still appears near-instantly for + // the launch that matters (a deep stack's diff work, below). + let source = cli.source.as_deref().map(Source::classify); + let changesets = match &source { + None => resolve_changesets(&repo, &branch).into_diagnostic()?, + Some(source) => resolve_source(&repo, &branch, source.clone()).into_diagnostic()?, + }; - if views.len() == 1 && views[0].file_count() == 0 { - eprintln!("nothing to review"); + // A resolved source can legitimately name zero changesets — `stack` on a branch that's + // caught up with its upstream and has a clean tree hits `assemble_git`'s empty-vec arm + // (see `git_inference_caught_up_and_clean_returns_empty` in git-workon-lib), same as the + // single-uncommitted-changeset case with nothing in it. Both are "nothing to review" + + // exit 0 (ADR-036). The file-count gate needs per-changeset counts, not views — checked + // against the resolved changesets' diffs only after they're built, so the empty case is + // detected on the cheap resolve data here first. + if changesets.is_empty() { + match cli.source.as_deref() { + Some(text) => eprintln!("nothing to review in {text}"), + None => eprintln!("nothing to review"), + } return Ok(()); } @@ -75,10 +108,73 @@ fn main() -> Result<()> { // would still be borrowing `repo` when `App::from_changesets` tries to move it below). let view_config = ReviewConfig::new(&repo).view_config(); + // After a probe, OSC replies from a slow terminal (e.g. one ssh round-trip away) may have + // straggled in while the theme was being derived above. Discard them now, BEFORE crossterm + // takes the terminal — parsed as input they become phantom keystrokes (`r` fires refreshes; + // `d` opens the discard confirm, which then swallows every key until Esc/n: the + // "unresponsive for ~30s with theme=auto" startup). Un-probed launches skip this so + // legitimate type-ahead survives. This MUST stay ahead of `Tui::acquire`: once the + // alternate screen is visible, a user's keystrokes are real input a flush must never eat. + if probed { + terminal_query::flush_pending_tty_input(); + } + + // CS5: take the terminal and show launch activity while the diffs build — on a deep stack + // this is the bulk of the launch, and until CS5 it left the terminal dead the whole time. + // Everything that could print, prompt, or flush is done (see the block comment above the + // resolve), so from here the terminal belongs to the TUI. `Tui`'s Drop restores it, so the + // `?`s below put the shell back before miette prints their error. + // + // An acquire FAILURE (no controlling tty — CI, a test harness, a bare pipe) is carried, not + // propagated here: a clean worktree's "nothing to review" is only detectable AFTER the diff + // below (resolve always yields at least the uncommitted changeset), and that exit must stay + // tty-free, exactly as it was when the terminal was only taken inside the run call. The + // error surfaces at the run call — the same logical point it always did. Splash failures on + // an acquired terminal are cosmetic (the run call will surface anything real) and ignored. + let mut tui = tui::Tui::acquire(); + if let Ok(tui) = tui.as_mut() { + let noun = if changesets.len() == 1 { + "changeset" + } else { + "changesets" + }; + let _ = tui.splash(&format!("diffing {} {noun}…", changesets.len())); + } + let diffs = diff_changesets(&repo, &changesets).into_diagnostic()?; + let views: Vec = changesets + .into_iter() + .zip(diffs) + .map(|(cs, diff)| ChangesetView::from_changeset_diff(cs, diff)) + .collect(); + + // The single-uncommitted-changeset case with nothing in it only shows up in the built + // views' file counts — the mirror of the resolve-level empty check above, and the same + // "nothing to review" + exit 0 (ADR-036), never a `views` list handed to + // `App::from_changesets`, which panics on empty input. Restore the terminal BEFORE + // printing: the message must land on the normal screen, not vanish with the alternate one. + // A tty-less launch has no terminal to restore — the message prints exactly as before CS5. + if views.is_empty() || (views.len() == 1 && views[0].file_count() == 0) { + if let Ok(tui) = tui.as_mut() { + tui.restore().into_diagnostic()?; + } + match cli.source.as_deref() { + Some(text) => eprintln!("nothing to review in {text}"), + None => eprintln!("nothing to review"), + } + return Ok(()); + } + // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here after // acquisition is done borrowing it. `App::from_changesets` opens on whichever changeset the // lib marked `current` (locked decision #6). let mut app = App::from_changesets(repo, views); + if let Some(source) = source { + app.set_review_source(source); + } + // CS4: defer file loads to the event loop's input-idle window rather than blocking here (or + // on any later selection change) — `app.open_current()` below marks the initial open pending + // instead of loading eagerly; see `tui::run`'s doc comment for the resulting startup contract. + app.set_defer_loads(true); // Apply CS7's view-config settings BEFORE `open_current`: `App::apply_view_config`'s setters // only set the raw layout/zoom/mode/width fields, and `open_current` is what derives @@ -96,16 +192,11 @@ fn main() -> Result<()> { app.notify(warnings.join("; "), Severity::Error); } - // After a probe, OSC replies from a slow terminal (e.g. one ssh round-trip away) may have - // straggled in while the changesets were being assembled above. Discard them now, right - // before crossterm takes the terminal — parsed as input they become phantom keystrokes - // (`r` fires refreshes; `d` opens the discard confirm, which then swallows every key until - // Esc/n: the "unresponsive for ~30s with theme=auto" startup). Un-probed launches skip this - // so legitimate type-ahead survives. - if probed { - terminal_query::flush_pending_tty_input(); - } - tui::run(&mut app, &keymap, &theme).into_diagnostic()?; + // A carried acquire failure surfaces HERE — the same logical point (running the TUI) it + // surfaced at before CS5 moved the terminal takeover ahead of the diff phase. + tui.into_diagnostic()? + .run(&mut app, &keymap, &theme) + .into_diagnostic()?; Ok(()) } diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index c909b51c..cfd053b4 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -726,6 +726,45 @@ fn render_gap_row( buf.set_line(area.x, y, &line, area.width); } +/// Whether file `idx` needs CS4's deferred-load placeholder instead of its real diff: either the +/// current open is still pending (set by [`App::open_current`] in defer mode — see its doc +/// comment), or it isn't pending but the view(s) its effective zoom needs haven't been loaded yet +/// (e.g. a force-completed OTHER file's load left this one's cache untouched). Under +/// [`EffectiveZoom::Split`] the placeholder shows only when NEITHER pane is loaded: a role with +/// no change for the file stays legitimately `None` forever (see `ensure_role_loaded`), so +/// gating on both panes would placeholder a one-role file for good. Once +/// [`App::complete_pending_open`] runs, every loadable pane is loaded, and a role-less pane +/// renders empty exactly as it did pre-CS4. +fn needs_deferred_placeholder(app: &App, idx: usize) -> bool { + if app.open_pending() { + return true; + } + match app.effective_zoom_for(idx) { + EffectiveZoom::Single(role) => app.role_view_ref(idx, role).is_none(), + EffectiveZoom::Split => { + app.role_view_ref(idx, Role::Unstaged).is_none() + && app.role_view_ref(idx, Role::Staged).is_none() + } + } +} + +/// Render CS4's deferred-load placeholder: a dim one-line paragraph naming the file, matching the +/// existing binary-file placeholder's style (see `render_body`'s binary arm) so the two read as +/// the same kind of "nothing to show yet" message. +fn render_loading_placeholder( + frame: &mut Frame, + app: &App, + area: Rect, + idx: usize, + theme: &Palette, +) { + let msg = format!("{} — loading…", app.files()[idx].path); + frame.render_widget( + Paragraph::new(msg).style(Style::default().fg(theme.dim)), + area, + ); +} + fn render_body(frame: &mut Frame, app: &mut App, area: Rect, theme: &Palette) { if app.files().is_empty() { frame.render_widget(Paragraph::new("(no changes)"), area); @@ -742,7 +781,17 @@ fn render_body(frame: &mut Frame, app: &mut App, area: Rect, theme: &Palette) { return; } - app.ensure_loaded(idx); + // CS4: in defer mode, selection changes never load — the diff body shows a placeholder until + // the event loop's idle window (`tui.rs`'s `OPEN_DEBOUNCE`) runs `complete_pending_open` + // between frames. Do NOT call `ensure_loaded` from this path in defer mode; outside defer mode + // (the default), behavior is unchanged. + if app.defer_loads() && needs_deferred_placeholder(app, idx) { + render_loading_placeholder(frame, app, area, idx, theme); + return; + } + if !app.defer_loads() { + app.ensure_loaded(idx); + } // The gate re-evaluates the effective zoom for the current file every frame (no caching — // ratatui relayout is free, per locked decision #3). @@ -1352,6 +1401,52 @@ mod tests { ); } + // ── CS4: idle-deferred loads ────────────────────────────────────────────── + + #[test] + fn defer_mode_shows_placeholder_and_does_not_load() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("tracked.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_defer_loads(true); + app.open_current(); // marks pending; does not load + + let buf = render_once(&mut app, 60, 10); + let content = buf_lines(&buf); + assert!( + content + .iter() + .any(|line| line.contains("tracked.txt") && line.contains("loading")), + "expected the CS4 loading placeholder, got:\n{}", + content.join("\n") + ); + assert!( + app.current_view_ref().is_none(), + "rendering in defer mode must not have triggered a load" + ); + } + + #[test] + fn non_defer_mode_still_loads_from_the_render_path() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("tracked.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + // `defer_loads` defaults off — render must still load eagerly, exactly like before CS4. + let _ = render_once(&mut app, 60, 10); + assert!( + app.current_view_ref().is_some(), + "non-defer mode must still load from the render path" + ); + } + #[test] fn deleted_file_renders_one_sided() { let fixture = FixtureBuilder::new() @@ -1988,7 +2083,7 @@ mod tests { /// (`mid..head`, one file, `current` + `needs_restack`). fn two_committed_changesets_app(fixture: &Fixture) -> App { use git2::Repository; - use workon::{Changeset, ChangesetSource}; + use workon::{Changeset, ChangesetSpan}; use crate::app::ChangesetView; @@ -2011,7 +2106,7 @@ mod tests { let cs_a = Changeset { name: "cs-a".to_string(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: root, head: mid, }, @@ -2021,7 +2116,7 @@ mod tests { }; let cs_b = Changeset { name: "cs-b".to_string(), - source: ChangesetSource::Committed { base: mid, head }, + span: ChangesetSpan::Committed { base: mid, head }, title: None, current: true, needs_restack: true, @@ -2141,7 +2236,7 @@ mod tests { // anything, it's a committed range. Assert the fix: the Add side renders the plain // (bright) pair. use git2::Repository; - use workon::{Changeset, ChangesetSource}; + use workon::{Changeset, ChangesetSpan}; use crate::app::ChangesetView; @@ -2165,7 +2260,7 @@ mod tests { let cs = Changeset { name: "main".to_string(), - source: ChangesetSource::Committed { base, head }, + span: ChangesetSpan::Committed { base, head }, title: None, current: true, needs_restack: false, @@ -2382,7 +2477,7 @@ mod tests { /// deliberately flat and never produce a directory row. fn changeset_with_nested_paths(fixture: &Fixture) -> App { use git2::Repository; - use workon::{Changeset, ChangesetSource}; + use workon::{Changeset, ChangesetSpan}; use crate::app::ChangesetView; @@ -2401,7 +2496,7 @@ mod tests { let cs = Changeset { name: "cs".to_string(), - source: ChangesetSource::Committed { base: root, head }, + span: ChangesetSpan::Committed { base: root, head }, title: None, current: true, needs_restack: false, diff --git a/git-workon-review/src/source.rs b/git-workon-review/src/source.rs new file mode 100644 index 00000000..e55b1f6e --- /dev/null +++ b/git-workon-review/src/source.rs @@ -0,0 +1,830 @@ +//! Classifying and resolving a `git workon review []` positional argument (ADR-036). +//! +//! [`Source::classify`] is pure — no repository access, deterministic regardless of repo +//! state — so a branch literally named `stack` only matches the keyword when spelled bare; +//! `refs/heads/stack` or `heads/stack` classify as [`Source::Ref`]. Resolution +//! ([`resolve_source`]) is where repo state comes in. +//! +//! CS3 wires real `` dispatch (shape-aware: Graphite-tracked branch, other branch, +//! bare commit-ish) and `Range` (`a..b` / `a...b`, git-diff semantics). CS4 wires `Pr`: any +//! form git-workon-lib's `parse_pr_reference` accepts (`pr-123`, `#123`, `pr#123`, GitHub URLs; +//! a bare number never matches — that spelling stays a `Ref`), reused end-to-end for +//! resolution too (`check_gh_available` → `fetch_pr_metadata` → fork-aware fetch → one +//! committed changeset). + +use std::ffi::OsStr; + +use clap_complete::engine::CompletionCandidate; +use git2::{BranchType, Oid, Repository}; +use workon::{ + assemble_changesets, get_default_branch, graphite_trunk, ChangesetError, ChangesetSpan, + PrMetadata, StackModel, UncommittedLayer, WorkonError, +}; + +use crate::acquire::uncommitted_changeset; +use crate::error::SourceError; + +/// Which dot form a [`Source::Range`] was spelled with — git-diff semantics differ (ADR-036). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RangeDots { + /// `a..b` — base `a`, head `b` (endpoint trees, exactly a committed span). + Two, + /// `a...b` — base `merge-base(a, b)`, head `b` (the PR-style "what did b add"). + Three, +} + +/// What a `git workon review ` argument was classified as (ADR-036 precedence). +/// +/// `classify` only ever runs on `Some(text)` — the no-argument case stays the existing +/// auto-detect path in `main.rs` and never constructs a `Source` at all. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Source { + /// A PR reference (`pr-123`, `#123`, `pr#123`, a GitHub PR URL — any form + /// `workon::parse_pr_reference` accepts). Carries the source text as typed, for the + /// changeset name; the PR number is re-derived from it at resolution time. + Pr(String), + /// The exact bare word `stack`. + Stack, + /// The exact bare word `uncommitted`. + Uncommitted, + /// `..` or `...` — either side may be empty, defaulting to `HEAD` + /// at resolution time (classification stays pure/repo-state-free). + Range { + base_text: String, + head_text: String, + dots: RangeDots, + }, + /// Everything else — a candidate ref, resolved by shape (CS3). + Ref(String), +} + +impl Source { + /// Classify `text` per ADR-036's precedence: PR reference first (checked via + /// `workon::parse_pr_reference`, pure string parsing — no network, no repo access), then + /// exact bare keyword, else a range (three-dot checked before two-dot, since `...` contains + /// `..`), else `Ref`. A malformed near-PR spelling (`pr-`, `pr-abc`) is `Ok(Err(_))` from the + /// lib parser, not `Ok(Some(_))` — it falls through to the normal precedence chain rather + /// than being force-classified as a broken PR, so it ultimately resolves (or fails) as a + /// `Ref` like any other typo. A bare number (`123`) never matches any of the lib parser's + /// accepted spellings, so it also falls through to `Ref` — no separate digit guard needed. + pub fn classify(text: &str) -> Source { + if let Ok(Some(_)) = workon::parse_pr_reference(text) { + return Source::Pr(text.to_string()); + } + match text { + "stack" => return Source::Stack, + "uncommitted" => return Source::Uncommitted, + _ => {} + } + if let Some((base_text, head_text)) = text.split_once("...") { + return Source::Range { + base_text: base_text.to_string(), + head_text: head_text.to_string(), + dots: RangeDots::Three, + }; + } + if let Some((base_text, head_text)) = text.split_once("..") { + return Source::Range { + base_text: base_text.to_string(), + head_text: head_text.to_string(), + dots: RangeDots::Two, + }; + } + Source::Ref(text.to_string()) + } +} + +/// Resolve a classified [`Source`] to the changesets it names, for the worktree whose `HEAD` +/// is `head_branch`. +/// +/// Unlike [`crate::acquire::resolve_changesets`] (the no-argument auto-detect path), every +/// arm here is an explicit ask: `Stack` never silently falls back to the uncommitted layer on +/// a missing upstream, and an unresolvable `Ref` is a named pre-TUI error, never a fallback to +/// auto-detect (ADR-036's "no surprise reviews" rule). +pub fn resolve_source( + repo: &Repository, + head_branch: &str, + source: Source, +) -> Result, SourceError> { + match source { + Source::Pr(text) => resolve_pr(repo, text), + Source::Stack => resolve_stack(repo, head_branch), + Source::Uncommitted => Ok(vec![uncommitted_changeset(head_branch)]), + Source::Range { + base_text, + head_text, + dots, + } => resolve_range(repo, &base_text, &head_text, dots), + Source::Ref(text) => resolve_ref(repo, head_branch, text), + } +} + +/// `Pr` resolution (ADR-036): reuse git-workon-lib's `pr.rs` PR workflow end-to-end, minus the +/// worktree-creation step — review only needs the PR's base and head fetched locally so their +/// merge-base span can be computed, never a branch or worktree. Every failure here is a named, +/// hinted pre-TUI error. The network round-trip (`check_gh_available`, `fetch_pr_metadata`, +/// `fetch_branch_fresh`) lives entirely in this function so [`pr_changeset_from_metadata`] can +/// stay a pure git2 mapping, fixture-testable without gh (the real gh path is exercised manually +/// — see the CS4 changeset description). +/// +/// Both refs are fetched with [`workon::fetch_branch_fresh`], not [`workon::fetch_branch`]: +/// review's whole point is freshness, and `fetch_branch`'s existence short-circuit (right for +/// its original one-time worktree-creation fetch) would leave a previously-fetched head stale +/// and — since `refs/remotes/{remote}/{base}` is virtually always already present — would never +/// refresh the base at all, corrupting the merge-base against a stale base tip. +fn resolve_pr(repo: &Repository, text: String) -> Result, SourceError> { + workon::check_gh_available().map_err(|source| SourceError::GhUnavailable { + text: text.clone(), + source, + })?; + + let fail = |source| SourceError::PrResolutionFailed { + text: text.clone(), + source, + }; + + // `classify` only builds `Source::Pr` from a `parse_pr_reference` `Ok(Some(_))`, so this + // re-parse is infallible in practice; treated as unresolvable rather than unwrapped in case + // a `Source::Pr` is ever constructed some other way. + let pr = workon::parse_pr_reference(&text) + .ok() + .flatten() + .ok_or_else(|| SourceError::UnresolvableSource { text: text.clone() })?; + + let metadata = workon::fetch_pr_metadata(pr.number).map_err(&fail)?; + + // `setup_fork_remote` already dispatches on `metadata.is_fork` (non-fork → `detect_pr_remote`), + // so only the fork case needs a second, separate lookup for the base remote — a fork's base + // is what the PR targets upstream, never the fork remote itself. + let head_remote = workon::setup_fork_remote(repo, &metadata).map_err(&fail)?; + let base_remote = if metadata.is_fork { + workon::detect_pr_remote(repo).map_err(&fail)? + } else { + head_remote.clone() + }; + + workon::fetch_branch_fresh(repo, &head_remote, &metadata.head_ref).map_err(&fail)?; + workon::fetch_branch_fresh(repo, &base_remote, &metadata.base_ref).map_err(&fail)?; + + pr_changeset_from_metadata(repo, &text, &metadata, &head_remote, &base_remote) +} + +/// Map fetched PR metadata to the one committed changeset review renders for it: +/// `merge-base(base tip, head tip)..head`, PR title carried through (ADR-036: "GitHub's own +/// three-dot PR diff"). Pure git2 — no gh, no fetch — assuming `head_remote`/`base_remote` +/// already have `metadata.head_ref`/`metadata.base_ref` as remote-tracking branches (true after +/// [`resolve_pr`]'s fetches, or hand-built in a fixture for testing this half without gh). +fn pr_changeset_from_metadata( + repo: &Repository, + text: &str, + metadata: &PrMetadata, + head_remote: &str, + base_remote: &str, +) -> Result, SourceError> { + let unresolvable = || SourceError::UnresolvableSource { + text: text.to_string(), + }; + + let head_oid = + remote_branch_tip(repo, head_remote, &metadata.head_ref).ok_or_else(unresolvable)?; + let base_tip = + remote_branch_tip(repo, base_remote, &metadata.base_ref).ok_or_else(unresolvable)?; + let base_oid = repo + .merge_base(base_tip, head_oid) + .map_err(|_| unresolvable())?; + + Ok(vec![workon::Changeset { + name: text.to_string(), + span: ChangesetSpan::Committed { + base: base_oid, + head: head_oid, + }, + title: Some(metadata.title.clone()), + current: true, + needs_restack: false, + }]) +} + +/// The tip commit of `refs/remotes/{remote}/{branch}`, or `None` if it isn't a remote-tracking +/// branch that resolves to a commit. +fn remote_branch_tip(repo: &Repository, remote: &str, branch: &str) -> Option { + repo.find_branch(&format!("{remote}/{branch}"), BranchType::Remote) + .ok() + .and_then(|b| b.get().target()) +} + +/// `stack` keyword resolution: stack metadata (Graphite or gh-stack) when active, otherwise the +/// git-inference arm (`StackModel::Git`, first wired into the binary here) — never a silent +/// downgrade to `StackModel::None`'s empty result, since the keyword is an explicit ask for the +/// real stack. +/// The uncommitted layer rides along (`UncommittedLayer::Include`): `stack` always means +/// "focused on real `HEAD`." +fn resolve_stack( + repo: &Repository, + head_branch: &str, +) -> Result, SourceError> { + let model = match StackModel::detect(repo) { + StackModel::None | StackModel::Git => StackModel::Git, + metadata @ (StackModel::Graphite | StackModel::GhStack) => metadata, + }; + + assemble_changesets(repo, head_branch, model, UncommittedLayer::Include) + .map_err(map_assemble_err(head_branch)) +} + +/// Maps an [`assemble_changesets`] failure to a [`SourceError`], for the branch named `branch`: +/// a missing upstream becomes [`SourceError::NoUpstream`], anything else +/// [`SourceError::StackResolutionFailed`]. +fn map_assemble_err(branch: &str) -> impl Fn(WorkonError) -> SourceError + '_ { + move |err| match err { + WorkonError::Changeset(ChangesetError::NoUpstream { branch }) => { + SourceError::NoUpstream { branch } + } + other => SourceError::StackResolutionFailed { + branch: branch.to_string(), + source: other, + }, + } +} + +/// `` resolution — shape-aware dispatch (ADR-036), checked in order: +/// +/// 1. A Graphite-tracked LOCAL branch (`text` names a local branch, qualified spellings like +/// `refs/heads/`/`heads/` included, AND that branch has a Graphite metadata row) +/// → the whole stack focused there, exactly like `stack` but pinned to `text`'s branch +/// instead of real `HEAD`. The uncommitted layer rides along only when the resolved branch +/// IS `head_branch` — this is the first caller to pass [`UncommittedLayer::Omit`]. +/// 2. Any other branch (an untracked local branch, or a remote-tracking branch like +/// `origin/foo`) → one committed changeset, "what this branch adds": base = +/// `merge-base(upstream, branch)` when a local branch has an upstream, else +/// `merge-base(trunk, branch)`. +/// 3. A bare commit-ish (sha, tag, `HEAD~2`) that rev-parses to a commit but isn't a branch → +/// one changeset spanning just that commit (`parent..ref`, or [`ChangesetSpan::CommittedRoot`] +/// for a parentless root commit). +/// 4. Nothing rev-parses → [`SourceError::UnresolvableSource`]. +fn resolve_ref( + repo: &Repository, + head_branch: &str, + text: String, +) -> Result, SourceError> { + if let Some(branch_name) = resolve_local_branch_name(repo, &text) { + if workon::current_stack(repo, &branch_name, StackModel::Graphite) + .ok() + .flatten() + .is_some() + { + let layer = if branch_name == head_branch { + UncommittedLayer::Include + } else { + UncommittedLayer::Omit + }; + return assemble_changesets(repo, &branch_name, StackModel::Graphite, layer) + .map_err(map_assemble_err(&branch_name)); + } + + // Untracked local branch: "what this branch adds" vs its upstream, else the trunk. + let branch = repo + .find_branch(&branch_name, BranchType::Local) + .map_err(|_| SourceError::UnresolvableSource { text: text.clone() })?; + let head_oid = branch + .get() + .target() + .ok_or_else(|| SourceError::UnresolvableSource { text: text.clone() })?; + let upstream_oid = branch.upstream().ok().and_then(|u| u.get().target()); + return one_changeset_from_branch(repo, &text, head_oid, upstream_oid); + } + + // Remote-tracking branch (e.g. "origin/foo"): no upstream concept of its own, base always + // comes from the trunk. + if let Ok(branch) = repo.find_branch(&text, BranchType::Remote) { + let head_oid = branch + .get() + .target() + .ok_or_else(|| SourceError::UnresolvableSource { text: text.clone() })?; + return one_changeset_from_branch(repo, &text, head_oid, None); + } + + // Bare commit-ish: sha, tag, `HEAD~2`, etc. — rev-parses to a commit but isn't a branch. + if let Some(head_oid) = revparse_to_commit(repo, &text) { + let commit = repo + .find_commit(head_oid) + .map_err(|_| SourceError::UnresolvableSource { text: text.clone() })?; + let span = match commit.parent_id(0) { + Ok(base) => ChangesetSpan::Committed { + base, + head: head_oid, + }, + // A root commit has no parent to diff against — the empty tree stands in. + Err(_) => ChangesetSpan::CommittedRoot { head: head_oid }, + }; + return Ok(vec![workon::Changeset { + name: text, + span, + title: None, + current: true, + needs_restack: false, + }]); + } + + Err(SourceError::UnresolvableSource { text }) +} + +/// `Range` resolution: rev-parse each endpoint (an empty side defaults to `HEAD`), then combine +/// per [`RangeDots`] — `a..b` spans the endpoints directly; `a...b` bases off their merge-base. +/// One committed changeset either way, named after the source text exactly as typed. Never a +/// candidate for the uncommitted layer (ADR-036: committed-only, like every source but `stack` +/// and a `` on real `HEAD`). +fn resolve_range( + repo: &Repository, + base_text: &str, + head_text: &str, + dots: RangeDots, +) -> Result, SourceError> { + let base_oid = resolve_endpoint(repo, base_text)?; + let head_oid = resolve_endpoint(repo, head_text)?; + + let base_oid = match dots { + RangeDots::Two => base_oid, + RangeDots::Three => { + repo.merge_base(base_oid, head_oid) + .map_err(|_| SourceError::UnresolvableSource { + text: format!("{base_text}...{head_text}"), + })? + } + }; + + let name = match dots { + RangeDots::Two => format!("{base_text}..{head_text}"), + RangeDots::Three => format!("{base_text}...{head_text}"), + }; + + Ok(vec![workon::Changeset { + name, + span: ChangesetSpan::Committed { + base: base_oid, + head: head_oid, + }, + title: None, + current: true, + needs_restack: false, + }]) +} + +/// Rev-parse one range endpoint, peeled to a commit; an empty `text` defaults to `HEAD` (ADR-036). +fn resolve_endpoint(repo: &Repository, text: &str) -> Result { + if text.is_empty() { + return repo + .head() + .and_then(|h| h.peel_to_commit()) + .map(|c| c.id()) + .map_err(|_| SourceError::UnresolvableSource { + text: "HEAD".to_string(), + }); + } + revparse_to_commit(repo, text).ok_or_else(|| SourceError::UnresolvableSource { + text: text.to_string(), + }) +} + +/// Rev-parse `text` and peel it to a commit, or `None` if it doesn't resolve to one. +fn revparse_to_commit(repo: &Repository, text: &str) -> Option { + repo.revparse_single(text) + .ok() + .and_then(|obj| obj.peel_to_commit().ok()) + .map(|c| c.id()) +} + +/// The resolved local branch name for `text`: an exact bare match, or a qualified spelling +/// (`refs/heads/`, `heads/`) that resolves to one — the same escapes ADR-036 gives +/// for the `stack`/`uncommitted` keywords, so `refs/heads/` still counts as +/// "focused on real `HEAD`" (compares equal to `head_branch` after unwrapping). +fn resolve_local_branch_name(repo: &Repository, text: &str) -> Option { + if repo.find_branch(text, BranchType::Local).is_ok() { + return Some(text.to_string()); + } + for prefix in ["refs/heads/", "heads/"] { + if let Some(name) = text.strip_prefix(prefix) { + if repo.find_branch(name, BranchType::Local).is_ok() { + return Some(name.to_string()); + } + } + } + None +} + +/// One committed changeset spanning "what `branch` (named `text`) adds": base = +/// `merge-base(upstream_oid, head_oid)` when `upstream_oid` is `Some` (a local branch with an +/// upstream), else `merge-base(trunk, head_oid)` where trunk is the Graphite trunk if known, +/// else the repo's default branch. Neither resolving is [`SourceError::NoBaseForBranch`]. +fn one_changeset_from_branch( + repo: &Repository, + text: &str, + head_oid: Oid, + upstream_oid: Option, +) -> Result, SourceError> { + let no_base = || SourceError::NoBaseForBranch { + branch: text.to_string(), + }; + + let base_target = match upstream_oid { + Some(oid) => oid, + None => trunk_commit_oid(repo).ok_or_else(no_base)?, + }; + let base_oid = repo + .merge_base(base_target, head_oid) + .map_err(|_| no_base())?; + + Ok(vec![workon::Changeset { + name: text.to_string(), + span: ChangesetSpan::Committed { + base: base_oid, + head: head_oid, + }, + title: None, + current: true, + needs_restack: false, + }]) +} + +/// The trunk branch's tip commit: the Graphite trunk if known, else the repo's default branch +/// (`init.defaultBranch`/`main`/`master`) — `None` if neither resolves to a real commit. +fn trunk_commit_oid(repo: &Repository) -> Option { + let name = graphite_trunk(repo).or_else(|| get_default_branch(repo).ok())?; + revparse_to_commit(repo, &name) +} + +/// Dynamic `[SOURCE]` completion candidates (ADR-036 "Completion" section, CS5): the `stack` / +/// `uncommitted` keywords, plus local branch and tag names via offline git2 ref enumeration — +/// never a PR number (network stays out of the TAB hot path). When `current` contains `..` or +/// `...`, only the right-hand side is a ref candidate; each is emitted prefixed with the +/// left-hand text (dots included) so the shell's own prefix filtering keeps working on the full +/// word (e.g. typing `main..fe` offers `main..feature-x`, not just `feature-x`). +/// +/// Failure-safe by construction: [`Repository::discover`] failing (not a repo, or any other git +/// error) simply skips the ref arms below, leaving keyword candidates — this must never panic or +/// surface an error into the completion path (a broken `TAB` is worse than an incomplete one). +pub fn complete_source(current: &OsStr) -> Vec { + let Some(current) = current.to_str() else { + return Vec::new(); + }; + + let (prefix, ref_prefix) = split_range_rhs(current); + let mut candidates = Vec::new(); + + // Keywords only make sense as the bare word itself — never after a `..`/`...` split. + if prefix.is_empty() { + for (keyword, help) in [ + ("stack", "Review the whole Graphite/git-inferred stack"), + ("uncommitted", "Review only uncommitted changes"), + ] { + if keyword.starts_with(ref_prefix) { + candidates.push(CompletionCandidate::new(keyword).help(Some(help.into()))); + } + } + } + + if let Ok(repo) = Repository::discover(".") { + for name in local_branch_and_tag_names(&repo) { + if name.starts_with(ref_prefix) { + candidates.push(CompletionCandidate::new(format!("{prefix}{name}"))); + } + } + } + + candidates +} + +/// Split `text` on its last dot-range separator (`...` checked before `..`, matching +/// [`Source::classify`]'s precedence): `(left-including-dots, right-hand-partial)`. No dots at +/// all yields `("", text)` — the whole word is the partial being completed. +fn split_range_rhs(text: &str) -> (&str, &str) { + if let Some(idx) = text.find("...") { + (&text[..idx + 3], &text[idx + 3..]) + } else if let Some(idx) = text.find("..") { + (&text[..idx + 2], &text[idx + 2..]) + } else { + ("", text) + } +} + +/// Local branch and tag names, offline (no network, no remote enumeration) — any git2 error +/// along the way degrades to whatever was already collected rather than propagating. +fn local_branch_and_tag_names(repo: &Repository) -> Vec { + let mut names = Vec::new(); + + if let Ok(branches) = repo.branches(Some(BranchType::Local)) { + for (branch, _) in branches.filter_map(Result::ok) { + if let Ok(Some(name)) = branch.name() { + names.push(name.to_string()); + } + } + } + + if let Ok(tags) = repo.tag_names(None) { + names.extend( + tags.iter() + .filter_map(|t| t.ok().flatten()) + .map(str::to_string), + ); + } + + names +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn classify_bare_stack_is_stack_keyword() { + assert_eq!(Source::classify("stack"), Source::Stack); + } + + #[test] + fn classify_bare_uncommitted_is_uncommitted_keyword() { + assert_eq!(Source::classify("uncommitted"), Source::Uncommitted); + } + + #[test] + fn classify_qualified_stack_ref_is_ref_not_keyword() { + assert_eq!( + Source::classify("refs/heads/stack"), + Source::Ref("refs/heads/stack".to_string()) + ); + assert_eq!( + Source::classify("heads/stack"), + Source::Ref("heads/stack".to_string()) + ); + } + + #[test] + fn classify_qualified_uncommitted_ref_is_ref_not_keyword() { + assert_eq!( + Source::classify("refs/heads/uncommitted"), + Source::Ref("refs/heads/uncommitted".to_string()) + ); + } + + #[test] + fn classify_is_case_sensitive() { + assert_eq!(Source::classify("Stack"), Source::Ref("Stack".to_string())); + assert_eq!( + Source::classify("Uncommitted"), + Source::Ref("Uncommitted".to_string()) + ); + assert_eq!(Source::classify("STACK"), Source::Ref("STACK".to_string())); + } + + #[test] + fn classify_arbitrary_text_is_ref() { + assert_eq!(Source::classify("main"), Source::Ref("main".to_string())); + assert_eq!( + Source::classify("deadbeef"), + Source::Ref("deadbeef".to_string()) + ); + } + + #[test] + fn classify_pr_dash_number_is_pr() { + assert_eq!(Source::classify("pr-123"), Source::Pr("pr-123".to_string())); + } + + #[test] + fn classify_hash_number_is_pr() { + assert_eq!(Source::classify("#123"), Source::Pr("#123".to_string())); + } + + #[test] + fn classify_pr_hash_number_is_pr() { + assert_eq!(Source::classify("pr#123"), Source::Pr("pr#123".to_string())); + } + + #[test] + fn classify_github_url_is_pr() { + let url = "https://github.com/owner/repo/pull/123"; + assert_eq!(Source::classify(url), Source::Pr(url.to_string())); + } + + #[test] + fn classify_bare_number_is_ref_not_pr() { + // ADR-036 explicitly excludes a bare number — it could be a branch or an abbreviated + // sha. `workon::parse_pr_reference` already requires a `#`/`pr-`/`pr#` prefix or a + // GitHub URL, so this falls through to `Ref` with no extra guard needed here. + assert_eq!(Source::classify("123"), Source::Ref("123".to_string())); + } + + #[test] + fn classify_malformed_pr_dash_is_ref_not_pr() { + // `pr-` and `pr-abc` look PR-shaped but don't carry a valid number — + // `parse_pr_reference` returns `Err`, not `Ok(Some(_))`, so classify falls through + // rather than force-classifying a broken PR reference. + assert_eq!(Source::classify("pr-"), Source::Ref("pr-".to_string())); + assert_eq!( + Source::classify("pr-abc"), + Source::Ref("pr-abc".to_string()) + ); + } + + #[test] + fn classify_empty_string_is_ref() { + assert_eq!(Source::classify(""), Source::Ref(String::new())); + } + + #[test] + fn classify_two_dot_range() { + assert_eq!( + Source::classify("a..b"), + Source::Range { + base_text: "a".to_string(), + head_text: "b".to_string(), + dots: RangeDots::Two, + } + ); + } + + #[test] + fn classify_three_dot_range() { + assert_eq!( + Source::classify("a...b"), + Source::Range { + base_text: "a".to_string(), + head_text: "b".to_string(), + dots: RangeDots::Three, + } + ); + } + + #[test] + fn classify_range_empty_sides() { + assert_eq!( + Source::classify("..main"), + Source::Range { + base_text: String::new(), + head_text: "main".to_string(), + dots: RangeDots::Two, + } + ); + assert_eq!( + Source::classify("main.."), + Source::Range { + base_text: "main".to_string(), + head_text: String::new(), + dots: RangeDots::Two, + } + ); + assert_eq!( + Source::classify(".."), + Source::Range { + base_text: String::new(), + head_text: String::new(), + dots: RangeDots::Two, + } + ); + } + + #[test] + fn classify_dotted_text_mixed_with_keyword_text_is_range() { + assert_eq!( + Source::classify("stack..main"), + Source::Range { + base_text: "stack".to_string(), + head_text: "main".to_string(), + dots: RangeDots::Two, + } + ); + } + + // ── CS5: SOURCE completion — `split_range_rhs` (the pure half of `complete_source`) ───── + + #[test] + fn split_range_rhs_no_dots_is_whole_word() { + assert_eq!(split_range_rhs("main"), ("", "main")); + assert_eq!(split_range_rhs(""), ("", "")); + } + + #[test] + fn split_range_rhs_two_dot_splits_after_dots() { + assert_eq!(split_range_rhs("main..fe"), ("main..", "fe")); + assert_eq!(split_range_rhs("main.."), ("main..", "")); + } + + #[test] + fn split_range_rhs_three_dot_wins_over_two_dot() { + assert_eq!(split_range_rhs("main...fe"), ("main...", "fe")); + } + + // ── CS4: PR metadata → changeset mapping (the gh-free half of `resolve_pr`) ───────────── + + /// [`pr_changeset_from_metadata`] is the pure git2 half of PR resolution — everything + /// downstream of `fetch_pr_metadata`/`fetch_branch`, which the real `gh` path can't exercise + /// in CI. This fixture stands in for "already fetched": a real (local, file-path) remote, + /// with `fetch_branch` itself used to populate the remote-tracking refs, so the only thing + /// not exercised here is the network round-trip to `gh` and to a non-local remote. + #[test] + fn pr_metadata_maps_to_merge_base_changeset_with_title( + ) -> Result<(), Box> { + use git_workon_fixture::prelude::*; + + // `RemoteSource::from(&Fixture)` only resolves to the bare `.git` dir when + // `fixture.repo()` itself reports bare — true for a bare fixture with NO worktree (a + // worktree checkout is never bare, even off a bare main repo). So this "remote" fixture + // stays worktree-free, and its two divergent branches are built directly with git2 + // rather than via `commit()` (which requires a checked-out worktree path). + let upstream = FixtureBuilder::new() + .bare(true) + .default_branch("main") + .build()?; + let upstream_repo = upstream.repo()?; + let base_commit = upstream_repo.head()?.peel_to_commit()?; + let sig = git2::Signature::now("Test User", "test@example.com")?; + + let mut main_tree = upstream_repo.treebuilder(None)?; + let a_blob = upstream_repo.blob(b"1")?; + main_tree.insert("a.txt", a_blob, 0o100_644)?; + let main_tree_oid = main_tree.write()?; + let main_tree = upstream_repo.find_tree(main_tree_oid)?; + let main_oid = upstream_repo.commit( + Some("refs/heads/main"), + &sig, + &sig, + "on main", + &main_tree, + &[&base_commit], + )?; + + let mut head_tree = upstream_repo.treebuilder(None)?; + let b_blob = upstream_repo.blob(b"1")?; + head_tree.insert("b.txt", b_blob, 0o100_644)?; + let head_tree_oid = head_tree.write()?; + let head_tree = upstream_repo.find_tree(head_tree_oid)?; + let head_oid = upstream_repo.commit( + Some("refs/heads/pr-head"), + &sig, + &sig, + "on pr-head", + &head_tree, + &[&base_commit], + )?; + + let local = FixtureBuilder::new().remote("origin", &upstream).build()?; + let repo = local.repo()?; + workon::fetch_branch(repo, "origin", "main")?; + workon::fetch_branch(repo, "origin", "pr-head")?; + + let metadata = PrMetadata { + number: 123, + title: "Add widget".to_string(), + author: "someone".to_string(), + head_ref: "pr-head".to_string(), + base_ref: "main".to_string(), + is_fork: false, + fork_owner: None, + fork_url: None, + }; + + let changesets = pr_changeset_from_metadata(repo, "pr-123", &metadata, "origin", "origin")?; + assert_eq!(changesets.len(), 1); + assert_eq!(changesets[0].name, "pr-123"); + assert_eq!(changesets[0].title.as_deref(), Some("Add widget")); + assert!(changesets[0].current); + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { + assert_eq!(head, head_oid); + let expected_base = repo.merge_base(main_oid, head_oid)?; + assert_eq!(base, expected_base); + } + other => panic!("expected Committed, got {other:?}"), + } + Ok(()) + } + + /// A missing remote-tracking ref (nothing fetched yet for that branch) is unresolvable, not + /// a panic — guards the "assumes already fetched" precondition documented on + /// [`pr_changeset_from_metadata`]. + #[test] + fn pr_metadata_with_unfetched_head_is_unresolvable() -> Result<(), Box> { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new().build()?; + let repo = fixture.repo()?; + + let metadata = PrMetadata { + number: 123, + title: "Add widget".to_string(), + author: "someone".to_string(), + head_ref: "pr-head".to_string(), + base_ref: "main".to_string(), + is_fork: false, + fork_owner: None, + fork_url: None, + }; + + let err = + pr_changeset_from_metadata(repo, "pr-123", &metadata, "origin", "origin").unwrap_err(); + match err { + SourceError::UnresolvableSource { text } => assert_eq!(text, "pr-123"), + other => panic!("expected UnresolvableSource, got {other:?}"), + } + Ok(()) + } +} diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index 0390e552..c10e9977 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -20,7 +20,9 @@ use crossterm::terminal::{ disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen, }; use ratatui::backend::CrosstermBackend; -use ratatui::Terminal; +use ratatui::style::{Modifier, Style}; +use ratatui::widgets::Paragraph; +use ratatui::{Frame, Terminal}; use workon_review::app::App; use workon_review::keymap::{Command, Dispatch, KeyPress, Keymap}; use workon_review::render; @@ -52,6 +54,30 @@ pub fn next_event(timeout: Duration) -> io::Result> { }) } +/// Cap on how many events [`drain_pending`] batches per iteration — leftover input past this +/// count is simply picked up by the next iteration's `next_event` call. +const MAX_DRAIN_BATCH: usize = 128; + +/// Drain all immediately-available terminal events into `batch`, mapping them exactly like +/// [`next_event`]'s read arm (key-press and resize map; release/repeat/mouse/paste/focus are +/// skipped, not pushed). Unlike calling `next_event(Duration::ZERO)` in a loop, a not-ready poll +/// here simply stops draining — it must NOT fabricate a `Tick`, since `next_event`'s `!poll` arm +/// exists solely to give the loop its regular redraw beat on a real timeout, and reusing it here +/// would inject a spurious tick at the end of every drain. +fn drain_pending(batch: &mut Vec) -> io::Result<()> { + while batch.len() < MAX_DRAIN_BATCH { + if !event::poll(Duration::ZERO)? { + break; + } + match event::read()? { + Event::Key(key) if key.kind == KeyEventKind::Press => batch.push(AppEvent::Key(key)), + Event::Resize(w, h) => batch.push(AppEvent::Resize(w, h)), + _ => {} + } + } + Ok(()) +} + /// The action a mapped key requests, independent of any [`App`] — kept separate from /// [`map_key`]'s dispatch so the mapping itself is unit-testable without building an `App`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -162,8 +188,45 @@ fn map_key( } } +/// Whether `action`'s effect READS the current [`App::current_view`]/cursor-space state +/// (cursor-space movement, staging, selection) rather than only changing WHICH file/changeset is +/// current. An action in the first group must force-complete any pending deferred open first (see +/// [`apply_action`]'s chokepoint) so it observes the same loaded view an eager `open_current` would +/// have produced — e.g. `j` then immediately `s` must stage the same hunk eager code would have. +/// +/// Exempt (returns `false`): every action that ends in its own fresh `open_current` (`NextFile`, +/// `PrevFile`, `NextChangeset`, `PrevChangeset`, `CycleZoom`, and the outline nav/confirm actions), +/// since those simply set a NEW pending open rather than needing the current one force-completed; +/// plus pure UI toggles/no-ops (`Refresh` rebuilds all views itself; `ToggleHelp`/`Quit`/`None` +/// touch no view state at all). +fn action_needs_loaded_view(action: Action) -> bool { + matches!( + action, + Action::MoveCursorBy(_) + | Action::ScrollTop + | Action::ScrollBottom + | Action::NextHunk + | Action::PrevHunk + | Action::StageHunk + | Action::StageFile + | Action::DiscardHunk + | Action::DiscardFile + | Action::StartSelection + | Action::ToggleSplitFocus + ) +} + /// Apply an [`Action`] to `app`. Returns `true` when the loop should exit. +/// +/// Chokepoint (CS4): before doing anything else, force-complete a pending deferred open for every +/// action [`action_needs_loaded_view`] flags — see that function's doc comment for the principle +/// and the exemption list. [`App::complete_pending_open`] is a no-op when nothing is pending, so +/// this costs nothing outside defer mode (where `open_pending` is never set) or when the debounce +/// window already completed the open on its own. fn apply_action(app: &mut App, action: Action) -> bool { + if action_needs_loaded_view(action) { + app.complete_pending_open(); + } match action { Action::Quit => return true, Action::ToggleHelp => app.toggle_help(), @@ -195,6 +258,43 @@ fn apply_action(app: &mut App, action: Action) -> bool { false } +/// The result of resolving a `Key` event through the non-modal cascade (see [`resolve_key`]): +/// either the key was fully handled inline (the selection-Esc guard cancelled the selection), or +/// it resolved to an [`Action`] still waiting to be applied. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KeyOutcome { + Handled, + Action(Action), +} + +/// Resolve one `Key` event to a [`KeyOutcome`], given the caller has already ruled out the two +/// modal cases (a pending discard confirm, the help overlay) — this is cases 3-5 of `update`'s +/// documented Esc-precedence cascade, extracted so [`update`] and [`update_batch`] share the exact +/// same resolution instead of duplicating it. +/// +/// Clears any showing footer notice as a side effect, exactly like `update`'s cases 3-5 do (the +/// confirm/help modals deliberately do not — that stays in their own arms, not here). +fn resolve_key( + app: &mut App, + keymap: &Keymap, + pending: &mut Vec, + key: KeyEvent, +) -> KeyOutcome { + if app.selection_anchor.is_some() && key.code == KeyCode::Esc && !app.outline_focused() { + app.clear_notice(); + app.cancel_selection(); + return KeyOutcome::Handled; + } + app.clear_notice(); + KeyOutcome::Action(map_key( + keymap, + pending, + key, + app.pane_height, + app.outline_focused(), + )) +} + /// Apply one [`AppEvent`] to `app`. Returns `true` when the loop should exit (q/Esc). Resize is a /// no-op — ratatui re-measures `body_area` every frame regardless. Tick drives /// [`App::on_tick`], the M4 index watcher's poll (see the module doc). @@ -226,7 +326,8 @@ fn apply_action(app: &mut App, action: Action) -> bool { /// 5. Otherwise the normal map applies, where Esc (like `q`) quits. /// /// A `Key` event clears any showing footer notice before applying its own action (cases 3-5); the -/// confirm and help modals (cases 1-2) deliberately do not. +/// confirm and help modals (cases 1-2) deliberately do not. Cases 3-5 are delegated to +/// [`resolve_key`], shared with [`update_batch`]. fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: AppEvent) -> bool { match event { AppEvent::Key(key) if app.pending_confirm.is_some() => { @@ -246,22 +347,10 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap } false } - AppEvent::Key(key) - if app.selection_anchor.is_some() - && key.code == KeyCode::Esc - && !app.outline_focused() => - { - app.clear_notice(); - app.cancel_selection(); - false - } - AppEvent::Key(key) => { - app.clear_notice(); - apply_action( - app, - map_key(keymap, pending, key, app.pane_height, app.outline_focused()), - ) - } + AppEvent::Key(key) => match resolve_key(app, keymap, pending, key) { + KeyOutcome::Handled => false, + KeyOutcome::Action(action) => apply_action(app, action), + }, AppEvent::Tick => { app.on_tick(); false @@ -270,6 +359,143 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap } } +/// The run kind and delta for an action [`update_batch`] can coalesce, or `None` for every +/// other action. The single source of truth for WHICH actions coalesce — `update_batch`'s +/// accumulate arm matches through this so the rule can't drift per action kind. +fn coalescable(action: Action) -> Option<(RunKind, i64)> { + match action { + Action::OutlineMoveBy(delta) => Some((RunKind::OutlineMoveBy, delta)), + Action::MoveCursorBy(delta) => Some((RunKind::MoveCursorBy, delta)), + _ => None, + } +} + +/// One in-flight coalesced nav run tracked by [`update_batch`]: a same-sign burst of either +/// outline moves or diff-cursor moves, deferred until a context-changing event forces a flush. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RunKind { + OutlineMoveBy, + MoveCursorBy, +} + +/// Apply and clear `run`, if one is open. `outline_move_by`/`move_cursor_by` both clamp at their +/// ends, so one call with the summed delta lands exactly where the equivalent sequence of unit +/// calls would (see [`update_batch`]'s doc comment for why this only holds for same-sign runs). +/// +/// `MoveCursorBy` reads cursor-space state exactly like [`apply_action`]'s `Action::MoveCursorBy` +/// arm does, and this is the OTHER path (besides `apply_action`) that can run it — CS2's +/// coalescing calls `App::move_cursor_by` directly rather than routing the flush through +/// `apply_action`, so the same force-completion has to happen here too (see the plan's chokepoint +/// note: whichever path applies `MoveCursorBy` must complete first). `OutlineMoveBy` needs no such +/// call: it ends in its own fresh `open_current`, exactly like `apply_action`'s exemption list. +fn flush_run(app: &mut App, run: &mut Option<(RunKind, i64)>) { + if let Some((kind, delta)) = run.take() { + match kind { + RunKind::OutlineMoveBy => app.outline_move_by(delta), + RunKind::MoveCursorBy => { + app.complete_pending_open(); + app.move_cursor_by(delta); + } + } + } +} + +/// Drain-and-coalesce entry point used by the event loop (`update` stays the single-event +/// primitive whose doc-comment cascade and tests are the spec — this delegates to it for +/// everything that isn't a coalescable nav key). +/// +/// Batches `events` (already drained by [`drain_pending`]) and merges same-sign runs of +/// `Action::OutlineMoveBy`/`Action::MoveCursorBy` into ONE deferred `App` call each, so +/// intermediate outline rows in a fast `j`/`k` burst are never opened (`render_body` only loads +/// the landing row, at draw time, via `App::ensure_loaded`). Returns `true` when the loop should +/// exit; remaining batched events after a quit are dropped. +/// +/// # Why coalescing same-sign runs is safe +/// +/// - Both `App::outline_move_by(delta)` and `App::move_cursor_by(delta)` clamp at the ends; for a +/// same-sign run, one call with the summed delta lands exactly where N unit calls land. Mixed +/// signs are NOT equivalent at a clamped boundary (`k` at row 0 then `j` = row 1, but summed +/// delta 0 = row 0 — a no-op) — hence a sign change always flushes the open run first. +/// - Applying a Move action never changes key-mapping context: it cannot toggle outline focus, +/// alter pane height (render sets it), open a modal, or change the keymap. So resolving key N+1 +/// before applying keys 1..N's deferred run is sound. Any action that COULD change context +/// (`ToggleOutline`, `ToggleHelp`, zoom, refresh, a modal, …) forces a flush before it is +/// applied, preserving strict ordering. +/// - `outline_move_by(sum)` opens at most ONE file — the landing row's, or for a header/dir +/// landing the last file the burst crossed (see its doc comment) — rather than one per row +/// crossed. That single jump is precisely what skips the intermediate loads. +fn update_batch( + app: &mut App, + keymap: &Keymap, + pending: &mut Vec, + events: Vec, +) -> bool { + let mut run: Option<(RunKind, i64)> = None; + + for event in events { + match event { + // The coalescable path: no modal is up, and this isn't the selection-Esc-cancel + // guard (that guard is a context change — an "Esc cascade" — so it falls to the + // catch-all arm below, which flushes first and delegates the whole event to + // `update`). Notice-clearing still happens per key via `resolve_key`. + AppEvent::Key(key) + if app.pending_confirm.is_none() + && !app.help_visible + && !(app.selection_anchor.is_some() + && key.code == KeyCode::Esc + && !app.outline_focused()) => + { + match resolve_key(app, keymap, pending, key) { + // A coalescable nav action extends the open run when it matches in kind + // and sign, else flushes and starts a fresh run — one arm for both kinds + // so the coalescing rule can't drift between them. + KeyOutcome::Action(action) if coalescable(action).is_some() => { + let Some((kind, delta)) = coalescable(action) else { + continue; // unreachable: the guard just matched + }; + match &mut run { + Some((k, acc)) if *k == kind && acc.signum() == delta.signum() => { + *acc += delta; + } + _ => { + flush_run(app, &mut run); + run = Some((kind, delta)); + } + } + } + // Any other resolved action (Quit, ToggleHelp, chord-pending `Action::None`, + // …) can change context, so flush first, then apply it directly — `resolve_key` + // already did the notice-clear and keymap resolution `update` would have done + // for this key, so applying here (rather than re-delegating to `update`) + // avoids resolving the same key twice. + KeyOutcome::Action(action) => { + flush_run(app, &mut run); + if apply_action(app, action) { + return true; + } + } + // The selection-Esc guard already ran inline inside `resolve_key`, but the + // outer match guard above rules this arm's condition out before we ever + // reach it — kept for exhaustiveness. + KeyOutcome::Handled => flush_run(app, &mut run), + } + } + // Any other event — Tick, Resize, a modal-captured key, or the selection-Esc-cancel + // guard — flushes the open run first, then is handled with `update`'s existing, + // unmodified semantics. + _ => { + flush_run(app, &mut run); + if update(app, keymap, pending, event) { + return true; + } + } + } + } + + flush_run(app, &mut run); + false +} + /// Open the controlling terminal (`/dev/tty`) for writing, falling back to stdout when there is /// none (a pipe/CI with no tty). The TUI renders here rather than to stdout so it stays usable /// inside a shell command substitution: the `workon` wrapper function captures `git workon`'s @@ -300,25 +526,93 @@ fn install_panic_hook() { })); } -/// Run the review TUI's terminal lifecycle and main loop against `app`. Callers must have -/// already loaded the initial file (`app.open_current()`) before calling this. -pub fn run(app: &mut App, keymap: &Keymap, theme: &Palette) -> io::Result<()> { - install_panic_hook(); - enable_raw_mode()?; - let mut out = terminal_writer(); - execute!(out, EnterAlternateScreen)?; - let backend = CrosstermBackend::new(out); - let mut terminal = Terminal::new(backend)?; +/// The acquired terminal: raw mode on, alternate screen entered, panic hook installed. +/// +/// Owning this as a value (rather than the old take-the-terminal-inside-`run` flow) is what lets +/// `main` show a splash frame BEFORE changeset acquisition — the terminal is live from the first +/// milliseconds of the launch, so resolve/diff work happens behind visible feedback instead of a +/// dead prompt. Restoration is idempotent and runs on [`Tui::restore`] or on drop, so every early +/// exit from `main` — "nothing to review", a `?`-propagated acquisition error — puts the shell +/// back before anything is printed to it. +pub struct Tui { + terminal: Terminal>>, + restored: bool, +} + +impl Tui { + /// Take over the terminal now: install the panic hook, enable raw mode, enter the alternate + /// screen. Call this before any slow launch work so [`Tui::splash`] can show it. + pub fn acquire() -> io::Result { + install_panic_hook(); + enable_raw_mode()?; + let mut out = terminal_writer(); + execute!(out, EnterAlternateScreen)?; + let backend = CrosstermBackend::new(out); + let terminal = Terminal::new(backend)?; + Ok(Self { + terminal, + restored: false, + }) + } - let result = event_loop(&mut terminal, app, keymap, theme); + /// Draw a one-line launch-activity frame (e.g. `resolving changesets…`). Deliberately + /// theme-free (`DIM` modifier, no palette colors): it renders before the theme is resolved — + /// resolving the theme first would put the up-to-800ms `theme=auto` terminal probe back in + /// front of the first visible frame, defeating the point. + pub fn splash(&mut self, msg: &str) -> io::Result<()> { + self.terminal.draw(|f| draw_splash(f, msg))?; + Ok(()) + } + + /// Run the main loop against `app`, then restore the terminal. Callers must have already + /// called `app.open_current()` — under CS4's deferred-load mode (`app.set_defer_loads(true)`, + /// `main.rs`'s default) that call marks the open PENDING rather than loading eagerly, so the + /// first frame shows CS4's placeholder for one `OPEN_DEBOUNCE` window instead of blocking on + /// the initial file's load; a caller that never turned defer mode on gets eager behavior. + pub fn run(&mut self, app: &mut App, keymap: &Keymap, theme: &Palette) -> io::Result<()> { + let result = event_loop(&mut self.terminal, app, keymap, theme); + let restored = self.restore(); + result.and(restored) + } + + /// Put the terminal back (raw mode off, leave the alternate screen, cursor shown). Idempotent + /// — a second call (including the one [`Drop`] always makes) is a no-op, so explicit callers + /// (the "nothing to review" exit, which must restore BEFORE its `eprintln`) and the drop + /// backstop coexist without double-restoring. + pub fn restore(&mut self) -> io::Result<()> { + if self.restored { + return Ok(()); + } + self.restored = true; + disable_raw_mode()?; + execute!(self.terminal.backend_mut(), LeaveAlternateScreen)?; + self.terminal.show_cursor() + } +} - disable_raw_mode()?; - execute!(terminal.backend_mut(), LeaveAlternateScreen)?; - terminal.show_cursor()?; +impl Drop for Tui { + /// Backstop restore for every exit path that doesn't call [`Tui::restore`] explicitly — most + /// importantly `main`'s `?` returns between `acquire` and `run`, whose errors miette prints + /// only after locals drop; without this they would print into the alternate screen. + fn drop(&mut self) { + let _ = self.restore(); + } +} - result +/// Render the splash frame's widget tree — split from [`Tui::splash`] so tests can drive it +/// against a `TestBackend` frame without acquiring a real terminal. +fn draw_splash(frame: &mut Frame<'_>, msg: &str) { + let para = Paragraph::new(msg).style(Style::default().add_modifier(Modifier::DIM)); + frame.render_widget(para, frame.area()); } +/// CS4's input-idle window: how long the loop waits with no new input before running a pending +/// deferred file open. Long enough that held-key autorepeat (~30-90ms between events on most +/// terminals) usually keeps re-arming the debounce and deferring the load past the whole burst; +/// short enough that releasing the key feels instant rather than laggy. Tunable if either edge +/// proves wrong in practice — there is nothing else load-bearing about this exact number. +const OPEN_DEBOUNCE: Duration = Duration::from_millis(80); + fn event_loop( terminal: &mut Terminal>, app: &mut App, @@ -335,8 +629,25 @@ fn event_loop( return Ok(()); } - if let Some(event) = next_event(Duration::from_millis(200))? { - quit = update(app, keymap, &mut pending, event); + // While an open is pending, poll on the short debounce window instead of the regular + // 200ms redraw beat, so the deferred load runs promptly once input goes quiet — a plain + // timeout (no new terminal event) is what "quiet" means here. This borrows the same + // `Tick` beat the M4 index watcher already polls on (see the module doc); the watcher + // occasionally running ~120ms early during a debounce window is harmless (its own doc + // comment already tolerates an "unseen" signature settling one tick late). + let timeout = if app.open_pending() { + OPEN_DEBOUNCE + } else { + Duration::from_millis(200) + }; + + if let Some(event) = next_event(timeout)? { + if matches!(event, AppEvent::Tick) && app.open_pending() { + app.complete_pending_open(); + } + let mut batch = vec![event]; + drain_pending(&mut batch)?; + quit = update_batch(app, keymap, &mut pending, batch); } } } @@ -809,7 +1120,7 @@ mod tests { /// `app_from_fixture`'s doc comment above for why the helpers can't be shared directly). fn two_committed_changesets_app(fixture: &git_workon_fixture::fixture::Fixture) -> App { use git2::Repository; - use workon::{Changeset, ChangesetSource}; + use workon::{Changeset, ChangesetSpan}; use workon_review::acquire::diff_changeset; use workon_review::app::ChangesetView; @@ -832,7 +1143,7 @@ mod tests { let cs_a = Changeset { name: "cs-a".to_string(), - source: ChangesetSource::Committed { + span: ChangesetSpan::Committed { base: root, head: mid, }, @@ -842,7 +1153,7 @@ mod tests { }; let cs_b = Changeset { name: "cs-b".to_string(), - source: ChangesetSource::Committed { base: mid, head }, + span: ChangesetSpan::Committed { base: mid, head }, title: None, current: true, needs_restack: false, @@ -1139,4 +1450,505 @@ mod tests { "the confirm arm must not have touched help_visible" ); } + + // ── CS2: coalesce buffered nav input ───────────────────────────────────── + + /// A single committed changeset with `n` distinct multi-line files ("f0.txt".."f{n-1}.txt"), + /// opened on file 0 — CS2's batching tests need several files so a coalesced outline jump has + /// intermediate rows to skip over, and several lines per file so a coalesced diff-cursor run + /// has room to move without immediately clamping. + fn many_files_app(fixture: &git_workon_fixture::fixture::Fixture, n: usize) -> App { + use git2::Repository; + use workon::{Changeset, ChangesetSpan}; + use workon_review::acquire::diff_changeset; + use workon_review::app::ChangesetView; + + let root = fixture + .commit("main") + .file("root.txt", "r\n") + .create("root") + .unwrap(); + let mut builder = fixture.commit("main"); + for i in 0..n { + builder = builder.file( + &format!("f{i}.txt"), + &format!("line-{i}-a\nline-{i}-b\nline-{i}-c\nline-{i}-d\nline-{i}-e\n"), + ); + } + let head = builder.create("head").unwrap(); + let repo = fixture.repo().unwrap(); + + let cs = Changeset { + name: "cs".to_string(), + span: ChangesetSpan::Committed { base: root, head }, + title: None, + current: true, + needs_restack: false, + }; + let view = + ChangesetView::from_changeset_diff(cs.clone(), diff_changeset(repo, &cs).unwrap()); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.open_current(); + app + } + + #[test] + fn batched_outline_jump_skips_intermediate_file_loads() { + use git_workon_fixture::prelude::*; + use workon_review::app::Role; + use workon_review::outline::OutlineMode; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = many_files_app(&fixture, 5); + app.set_outline_mode(OutlineMode::Flat); + app.toggle_outline(); // open + focus, cursor synced onto file 0's row (index 0 in Flat mode) + assert!(app.outline_focused()); + assert!( + app.role_view_ref(0, Role::Combined).is_some(), + "file 0 loaded by open_current" + ); + + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + // 4 outline-down keys: sequentially this would visit (and load) files 1, 2, 3, then land + // on 4 — coalescing must apply ONE outline_move_by(4), landing on file 4 directly. + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ]; + + let quit = update_batch(&mut app, &km, &mut pending, events); + + assert!(!quit); + assert_eq!( + app.outline_cursor(), + 4, + "the outline cursor lands on the final row" + ); + assert_eq!(app.current, 4, "the diff jumps to the landing file only"); + for skipped in 1..4 { + assert!( + app.role_view_ref(skipped, Role::Combined).is_none(), + "file {skipped} must never have been visited, so its view must not be loaded" + ); + } + assert!( + app.role_view_ref(4, Role::Combined).is_some(), + "the landing file's view IS loaded" + ); + } + + #[test] + fn batched_outline_jump_matches_sequential_moves() { + use git_workon_fixture::prelude::*; + use workon_review::outline::OutlineMode; + + let fixture_batch = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_batch = many_files_app(&fixture_batch, 5); + app_batch.set_outline_mode(OutlineMode::Flat); + app_batch.toggle_outline(); + + let fixture_seq = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_seq = many_files_app(&fixture_seq, 5); + app_seq.set_outline_mode(OutlineMode::Flat); + app_seq.toggle_outline(); + + let km = Keymap::defaults(); + let mut pending_batch: Vec = Vec::new(); + let mut pending_seq: Vec = Vec::new(); + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ]; + + update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); + for event in events { + update(&mut app_seq, &km, &mut pending_seq, event); + } + + assert_eq!(app_batch.outline_cursor(), app_seq.outline_cursor()); + assert_eq!(app_batch.current, app_seq.current); + } + + #[test] + fn mixed_direction_batch_matches_sequential_moves_including_at_a_clamp_boundary() { + use git_workon_fixture::prelude::*; + + // Mixed-sign run (j,j,j,k) starting away from any boundary. Each `App` gets its OWN + // fixture — `many_files_app` commits onto the fixture's `main`, so reusing one fixture + // across calls would have the second call's "head" commit re-add files the first call's + // "head" already committed, producing an empty diff for it. + let fixture_batch = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_batch = many_files_app(&fixture_batch, 1); + let fixture_seq = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_seq = many_files_app(&fixture_seq, 1); + let km = Keymap::defaults(); + let mut pending_batch: Vec = Vec::new(); + let mut pending_seq: Vec = Vec::new(); + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('k'))), + ]; + + update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); + for event in events { + update(&mut app_seq, &km, &mut pending_seq, event); + } + assert_eq!(app_batch.cursor, app_seq.cursor); + + // Clamp-boundary case: k then j starting at row 0 — a naive sum (0) would wrongly stay + // put; sequential unit calls land on row 1 (k clamps at 0, then j moves to 1). + let fixture_batch2 = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_batch2 = many_files_app(&fixture_batch2, 1); + let fixture_seq2 = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app_seq2 = many_files_app(&fixture_seq2, 1); + let mut pending_batch2: Vec = Vec::new(); + let mut pending_seq2: Vec = Vec::new(); + let boundary_events = vec![ + AppEvent::Key(key(KeyCode::Char('k'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ]; + + update_batch( + &mut app_batch2, + &km, + &mut pending_batch2, + boundary_events.clone(), + ); + for event in boundary_events { + update(&mut app_seq2, &km, &mut pending_seq2, event); + } + assert_eq!(app_batch2.cursor, app_seq2.cursor); + assert_eq!(app_batch2.cursor, 1, "k clamps at 0, then j moves to row 1"); + } + + #[test] + fn a_context_changing_key_mid_run_applies_moves_in_their_own_context() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = many_files_app(&fixture, 5); + // Default OutlineMode::Stack: row 0 is the header, row 1 is file 0 — so `o`'s + // sync-to-current lands the outline cursor on row 1, and a single outline `k` afterward + // lands on the header row (no file jump), leaving `app.cursor`/`app.current` observable. + assert!(!app.outline_open()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), // MoveCursorBy(1), outline unfocused + AppEvent::Key(key(KeyCode::Char('j'))), // MoveCursorBy(1), outline unfocused + AppEvent::Key(key(KeyCode::Char('o'))), // ToggleOutline: open + focus + AppEvent::Key(key(KeyCode::Char('k'))), // OutlineMoveBy(-1), outline now focused + ]; + + let quit = update_batch(&mut app, &km, &mut pending, events); + + assert!(!quit); + assert_eq!( + app.cursor, 2, + "the two j's before `o` must apply as diff-cursor moves in the OLD context" + ); + assert!( + app.outline_open() && app.outline_focused(), + "`o` toggles the outline open and focused" + ); + assert_eq!( + app.outline_cursor(), + 0, + "the k after `o` must apply as an outline move in the NEW context, landing on the \ + header row" + ); + assert_eq!( + app.current, 0, + "landing on the header row must not jump the diff" + ); + } + + #[test] + fn a_pending_confirm_disables_coalescing_and_batch_matches_sequential_updates() { + use git_workon_fixture::prelude::*; + use workon_review::app::PendingOp; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app_batch = app_from_fixture(&fixture); + app_batch.open_current(); + let mut app_seq = app_from_fixture(&fixture); + app_seq.open_current(); + + app_batch.request_confirm("Discard? (y/n)", PendingOp::DiscardFile { file_idx: 0 }); + app_seq.request_confirm("Discard? (y/n)", PendingOp::DiscardFile { file_idx: 0 }); + + let km = Keymap::defaults(); + let mut pending_batch: Vec = Vec::new(); + let mut pending_seq: Vec = Vec::new(); + let cursor_before = app_batch.cursor; + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), // swallowed by the confirm modal + AppEvent::Key(key(KeyCode::Char('n'))), // cancels the confirm + ]; + + update_batch(&mut app_batch, &km, &mut pending_batch, events.clone()); + for event in events { + update(&mut app_seq, &km, &mut pending_seq, event); + } + + assert_eq!( + app_batch.cursor, cursor_before, + "a captured key inside the modal must not run its normal action" + ); + assert!(app_batch.pending_confirm.is_none(), "n cancels the confirm"); + assert_eq!(app_batch.cursor, app_seq.cursor); + assert_eq!( + app_batch.pending_confirm.is_none(), + app_seq.pending_confirm.is_none() + ); + } + + #[test] + fn quit_mid_batch_drops_the_remaining_events_and_returns_true() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = many_files_app(&fixture, 1); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + let cursor_before = app.cursor; + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), // applies: cursor_before + 1 + AppEvent::Key(key(KeyCode::Char('q'))), // quits + AppEvent::Key(key(KeyCode::Char('j'))), // dropped: must never apply + ]; + + let quit = update_batch(&mut app, &km, &mut pending, events); + + assert!(quit, "q mid-batch must report quit"); + assert_eq!( + app.cursor, + cursor_before + 1, + "only the j before q must have applied" + ); + } + + #[test] + fn a_chord_split_across_two_batches_still_fires() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = many_files_app(&fixture, 3); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + assert_eq!(app.current, 0); + + // First batch: only the chord's first key arrives — held in `pending` across the drain + // boundary, exactly like a real terminal delivering the two keys in separate polls. + let quit1 = update_batch( + &mut app, + &km, + &mut pending, + vec![AppEvent::Key(key(KeyCode::Char(']')))], + ); + assert!(!quit1); + assert_eq!(pending, vec![KeyPress::from_event(key(KeyCode::Char(']')))]); + + // Second batch: the chord's second key completes it via the SAME `pending` buffer. + let quit2 = update_batch( + &mut app, + &km, + &mut pending, + vec![AppEvent::Key(key(KeyCode::Char('f')))], + ); + assert!(!quit2); + assert!(pending.is_empty()); + assert_eq!(app.current, 1, "]f must have fired NextFile"); + } + + // ── CS4: idle-deferred loads ────────────────────────────────────────────── + + #[test] + fn deferred_outline_burst_loads_nothing_until_completed() { + use git_workon_fixture::prelude::*; + use workon_review::app::Role; + use workon_review::outline::OutlineMode; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = many_files_app(&fixture, 5); + // `many_files_app` opens eagerly (defer mode isn't on yet) — file 0 is loaded before we + // flip the switch, exactly like a real session's startup open would be under CS4 (see + // `main.rs`, which turns defer mode on before its own initial `open_current`). + app.set_defer_loads(true); + app.set_outline_mode(OutlineMode::Flat); + app.toggle_outline(); // open + focus, cursor synced onto file 0's row + + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + let events = vec![ + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Char('j'))), + ]; + + let quit = update_batch(&mut app, &km, &mut pending, events); + + assert!(!quit); + assert_eq!(app.current, 4, "the outline jump still lands on file 4"); + assert!( + app.open_pending(), + "landing on file 4 in defer mode must mark the open pending, not load it" + ); + for f in 1..=4 { + assert!( + app.role_view_ref(f, Role::Combined).is_none(), + "file {f} must not be loaded — not even the landing file, until completed" + ); + } + + app.complete_pending_open(); + + assert!(!app.open_pending()); + assert!( + app.role_view_ref(4, Role::Combined).is_some(), + "completing the pending open loads only the landing file" + ); + } + + #[test] + fn force_completion_before_move_lets_stage_hit_the_eager_hunk() { + use git_workon_fixture::prelude::*; + + // Twin fixtures with identical content: one driven through defer mode (open_current + // defers, `j` must force-complete before moving, then `s` stages), the other through + // today's eager path — both must end up staging the exact same hunk. + let fixture_deferred = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let fixture_eager = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + + let mut app_deferred = app_from_fixture(&fixture_deferred); + app_deferred.set_defer_loads(true); + app_deferred.open_current(); + assert!( + app_deferred.open_pending(), + "open_current in defer mode must not load eagerly" + ); + + let mut app_eager = app_from_fixture(&fixture_eager); + app_eager.open_current(); + + let km = Keymap::defaults(); + let mut pending_deferred: Vec = Vec::new(); + let mut pending_eager: Vec = Vec::new(); + + // `j`: in defer mode this must force-complete the pending open (loading the view and + // re-deriving the cursor from the REAL first-hunk row) before applying the move — else + // the move would apply against the `0`-fallback cursor `reset_panes` left behind. + update( + &mut app_deferred, + &km, + &mut pending_deferred, + AppEvent::Key(key(KeyCode::Char('j'))), + ); + assert!( + !app_deferred.open_pending(), + "MoveCursorBy must force-complete the pending open" + ); + update( + &mut app_eager, + &km, + &mut pending_eager, + AppEvent::Key(key(KeyCode::Char('j'))), + ); + assert_eq!( + app_deferred.cursor, app_eager.cursor, + "post-completion cursor must match the eager path's cursor exactly" + ); + + // `s`: stages whatever hunk the (now-correct) cursor resolves to. + update( + &mut app_deferred, + &km, + &mut pending_deferred, + AppEvent::Key(key(KeyCode::Char('s'))), + ); + update( + &mut app_eager, + &km, + &mut pending_eager, + AppEvent::Key(key(KeyCode::Char('s'))), + ); + + let repo_deferred = fixture_deferred.repo().unwrap(); + let repo_eager = fixture_eager.repo().unwrap(); + repo_deferred.assert(predicate::repo::has_staged_file("a.txt")); + repo_eager.assert(predicate::repo::has_staged_file("a.txt")); + } + + // ── CS5: launch splash ──────────────────────────────────────────────────── + + #[test] + fn splash_renders_the_message() { + let backend = ratatui::backend::TestBackend::new(40, 3); + let mut terminal = Terminal::new(backend).unwrap(); + terminal + .draw(|f| draw_splash(f, "resolving changesets…")) + .unwrap(); + + let buffer = terminal.backend().buffer(); + let top_row: String = (0..buffer.area.width) + .map(|x| buffer[(x, 0)].symbol()) + .collect(); + assert!( + top_row.contains("resolving changesets…"), + "splash frame must show the launch-activity message, got: {top_row:?}" + ); + } } diff --git a/git-workon-review/tests/cli.rs b/git-workon-review/tests/cli.rs index fbec3c59..71813da7 100644 --- a/git-workon-review/tests/cli.rs +++ b/git-workon-review/tests/cli.rs @@ -28,22 +28,102 @@ fn help_shows_usage_and_succeeds() { .stdout(predicate::str::contains("git-workon-review")); } +/// Drive clap_complete's dynamic `COMPLETE=bash` protocol for `git-workon-review ` (mirrors +/// `git-workon/tests/completions.rs`'s `bash_candidates` helper) and return the emitted candidate +/// values, one per line (no `_CLAP_IFS` override means `write_complete` falls back to `\n`). +fn bash_candidates(cwd: &std::path::Path, word: &str) -> Vec { + let output = cargo_bin_cmd!("git-workon-review") + .env("COMPLETE", "bash") + .env("_CLAP_COMPLETE_INDEX", "1") + .current_dir(cwd) + .args(["--", "git-workon-review", word]) + .output() + .expect("completion invocation"); + String::from_utf8_lossy(&output.stdout) + .lines() + .map(str::to_string) + .filter(|s| !s.is_empty()) + .collect() +} + +/// ADR-036 "Completion" section (CS5): the `stack`/`uncommitted` keywords plus local branch and +/// tag names, offline, via git2 ref enumeration — this is the SOURCE positional's dynamic +/// completer, exercised through the same `COMPLETE=bash` protocol M6 wired the binary to answer. +#[test] +fn source_completion_offers_keywords_and_local_refs() { + let fixture = FixtureBuilder::new() + .default_branch("main") + .branch("feature-x") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + let head = repo.head().unwrap().peel_to_commit().unwrap(); + repo.tag_lightweight("v1", head.as_object(), false).unwrap(); + + let candidates = bash_candidates(repo.workdir().unwrap(), ""); + + assert!(candidates.contains(&"stack".to_string()), "{candidates:?}"); + assert!( + candidates.contains(&"uncommitted".to_string()), + "{candidates:?}" + ); + assert!( + candidates.contains(&"feature-x".to_string()), + "{candidates:?}" + ); + assert!(candidates.contains(&"v1".to_string()), "{candidates:?}"); +} + +/// A word containing `..`/`...` only completes the right-hand ref, reassembled with the +/// left-hand text (dots included) so shell prefix-matching keeps working on the whole word. +#[test] +fn source_completion_completes_range_rhs_with_lhs_prefix() { + let fixture = FixtureBuilder::new() + .default_branch("main") + .branch("feature-x") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + + let candidates = bash_candidates(repo.workdir().unwrap(), "main..fe"); + + assert!( + candidates.contains(&"main..feature-x".to_string()), + "{candidates:?}" + ); + // Never a bare ref without the `main..` prefix, and never a keyword after a dot-range. + assert!( + !candidates.contains(&"feature-x".to_string()), + "{candidates:?}" + ); + assert!(!candidates.contains(&"stack".to_string()), "{candidates:?}"); +} + /// The binary answers the `COMPLETE=` dynamic-completion protocol (clap_complete's /// `CompleteEnv`), so git-workon can delegate `git workon review ` completion to it (M6 CS3). -/// The review `Cli` has no args of its own yet, so the completer generates no candidates and exits -/// 2 ("no completion generated") — but the load-bearing contract is that `COMPLETE` mode -/// short-circuits into the completer *before* repository discovery or the TUI. Running from an -/// empty non-repo dir makes that concrete: an unwired binary would instead fail repo discovery; -/// getting clap_complete's own exit path proves the responder is in place. When M7 gives the -/// binary a real subcommand (`mcp`), upgrade this to assert that candidate. +/// A non-repo cwd degrades to keyword-only candidates (ADR-036: any git error → keywords only, +/// never a completion-path error) rather than failing repo discovery — the load-bearing contract +/// is that `COMPLETE` mode short-circuits into the completer *before* that discovery even runs. #[test] -fn responds_to_complete_env_protocol_before_repo_discovery() { +fn non_repo_cwd_completes_keywords_only_without_error() { let non_repo = assert_fs::TempDir::new().unwrap(); - let mut cmd = cargo_bin_cmd!("git-workon-review"); - cmd.env("COMPLETE", "bash") - .current_dir(&non_repo) - .args(["--", "git-workon-review", ""]) - .assert() - .code(2) - .stderr(predicate::str::contains("completion")); + + let candidates = bash_candidates(&non_repo, ""); + + assert!(candidates.contains(&"stack".to_string()), "{candidates:?}"); + assert!( + candidates.contains(&"uncommitted".to_string()), + "{candidates:?}" + ); + // No ref candidates from a non-repo cwd — only the two keywords (plus clap's own + // `--help`/`--version`, unrelated to the ref-enumeration arm under test here). + assert_eq!( + candidates + .iter() + .filter(|c| !c.starts_with('-')) + .cloned() + .collect::>(), + vec!["stack".to_string(), "uncommitted".to_string()], + "a non-repo cwd must never surface ref candidates" + ); } diff --git a/git-workon-review/tests/diff_model.rs b/git-workon-review/tests/diff_model.rs index ac081672..25c9bcf6 100644 --- a/git-workon-review/tests/diff_model.rs +++ b/git-workon-review/tests/diff_model.rs @@ -7,7 +7,7 @@ use git2::{BranchType, Oid, Repository}; use git_workon_fixture::prelude::*; -use workon::{assemble_changesets, Changeset, ChangesetSource, StackModel}; +use workon::{assemble_changesets, Changeset, ChangesetSpan, StackModel, UncommittedLayer}; use workon_review::acquire::{diff_changeset, diff_committed, diff_uncommitted, ChangesetDiff}; use workon_review::error::DiffError; use workon_review::model::{FileStatus, LineKind}; @@ -450,7 +450,8 @@ fn diff_changeset_over_real_graphite_stack() -> Result<(), Box Result<(), Box Result<(), Box> { + // A two-branch Graphite stack plus the uncommitted layer: exercises every span kind the + // parallel fan-out stripes across workers (Committed × 2, Uncommitted) in one call. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .unstaged_file("tracked.txt", "line1\nline2\n", "line1\nCHANGED\n") + .build()?; + let repo = fixture.repo()?; + + // Advance "a" and "b" independently so each committed changeset has its own diff. + let main_tip = repo + .find_branch("main", BranchType::Local)? + .get() + .target() + .unwrap(); + let a_head = commit_onto(repo, &repo.find_commit(main_tip)?, "a.txt", "from a\n"); + fixture.update_branch("a", a_head)?; + let b_head = commit_onto(repo, &repo.find_commit(a_head)?, "b.txt", "from b\n"); + fixture.update_branch("b", b_head)?; + + let changesets = + assemble_changesets(repo, "b", StackModel::Graphite, UncommittedLayer::Include)?; + assert!( + changesets.len() >= 3, + "expected two committed changesets plus the uncommitted layer" + ); + + let parallel = workon_review::acquire::diff_changesets(repo, &changesets)?; + + assert_eq!(parallel.len(), changesets.len()); + for (cs, got) in changesets.iter().zip(¶llel) { + let sequential = diff_changeset(repo, cs)?; + assert_eq!( + got, &sequential, + "parallel diff for '{}' must match the sequential one, in input order", + cs.name + ); + } + + Ok(()) +} + +#[test] +fn diff_changesets_surfaces_the_first_failing_changeset_by_input_order( +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build()?; + let repo = fixture.repo()?; + let head = repo.head()?.target().unwrap(); + + let good = Changeset { + name: "good".to_string(), + span: ChangesetSpan::CommittedRoot { head }, + title: None, + current: false, + needs_restack: false, + }; + let garbage = Oid::from_str("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")?; + let bad = |name: &str| Changeset { + name: name.to_string(), + span: ChangesetSpan::Committed { + base: garbage, + head, + }, + title: None, + current: true, + needs_restack: false, + }; + let changesets = vec![good, bad("bad-first"), bad("bad-second")]; + + let err = workon_review::acquire::diff_changesets(repo, &changesets) + .expect_err("a garbage base Oid must fail the whole acquisition"); + match err { + DiffError::ChangesetDiffFailed { name, .. } => assert_eq!( + name, "bad-first", + "the FIRST failing changeset by input order is the one reported" + ), + other => panic!("expected ChangesetDiffFailed, got {other:?}"), + } + + Ok(()) +} + #[test] fn diff_changeset_with_bad_base_oid_fails_never_empty() -> Result<(), Box> { let fixture = FixtureBuilder::new().build()?; @@ -476,7 +566,7 @@ fn diff_changeset_with_bad_base_oid_fails_never_empty() -> Result<(), Box String { + let mut src = String::with_capacity(lines * 40); + src.push_str(&format!("//! Generated fixture module {seed}.\n\n")); + let mut n = 0; + while src.lines().count() < lines { + src.push_str(&format!( + "pub fn item_{seed}_{n}(x: u64) -> u64 {{\n let y = x.wrapping_mul({n}) + {seed};\n y ^ (y >> 3)\n}}\n\n", + )); + n += 1; + } + src +} + +#[test] +#[ignore = "PTY smoke — run explicitly: cargo test -p git-workon-review --test pty_responsiveness -- --ignored"] +fn launch_reaches_the_tui_and_quits_promptly() { + // Theme pinned to dark so the `theme = auto` probe (and its deadline) stays out of this + // bound — the probe's own responsiveness is pty_smoke.rs's job. One unstaged change so the + // TUI actually opens; a plain (non-Graphite) repo keeps behavior identical whether or not + // the machine has `gt` on PATH — and `StackModel::detect` still runs `detect_gt` first, so + // a reintroduced subprocess spawn there is still inside the measured window. + let fixture = FixtureBuilder::new() + .config("workon.review.theme", "dark") + .unstaged_file("file.txt", "a\nb\nc\n", "a\nCHANGED\nc\n") + .build() + .expect("fixture"); + + let launched = Instant::now(); + let mut session = spawn_review(&fixture); + + // The alternate screen is the proof the launch reached the TUI — without this, an early + // error exit (or "nothing to review") would sail through the quit assertion trivially. + session + .expect("\x1b[?1049h") + .expect("TUI entered the alternate screen"); + + // `q` buffers in the PTY until the event loop polls input, so send it immediately: the + // elapsed spawn→exit time IS time-to-interactive plus one quit. + session.send("q").expect("send q"); + session.expect(expectrl::Eof).expect("app exited on q"); + + let elapsed = launched.elapsed(); + assert!( + elapsed < LAUNCH_RESPONSIVE, + "launch→quit took {elapsed:?} — something slow is blocking the launch path \ + (subprocess spawn? sequential stack diff? probe?)" + ); +} + +#[test] +#[ignore = "PTY smoke — run explicitly: cargo test -p git-workon-review --test pty_responsiveness -- --ignored"] +fn rapid_outline_nav_burst_stays_responsive() { + // Dozens of untracked multi-thousand-line Rust files: every outline row the burst crosses + // is a file whose (regressed) synchronous load would cost real tree-sitter work. + let mut builder = FixtureBuilder::new().config("workon.review.theme", "dark"); + let sources: Vec<(String, String)> = (0..BURST_FILES) + .map(|i| (format!("src_{i:02}.rs"), rust_source(i, BURST_FILE_LINES))) + .collect(); + for (path, content) in &sources { + builder = builder.untracked_file(path, content); + } + let fixture = builder.build().expect("fixture"); + + let mut session = spawn_review(&fixture); + session + .expect("\x1b[?1049h") + .expect("TUI entered the alternate screen"); + + // Buffer the whole interaction at once — focus the outline, sweep down across every file + // row, quit. This is the buffered-burst shape the coalescing fix exists for: healthy code + // merges the sweep into one outline move and quits before any deferred load fires; + // regressed code loads each file it crosses before it ever reaches the `q`. + let burst_sent = Instant::now(); + let mut input = String::from("o"); + input.push_str(&"j".repeat(BURST_FILES + 12)); // sweep past every file row, clamp at the end + input.push('q'); + session.send(&input).expect("send nav burst"); + session.expect(expectrl::Eof).expect("app exited on q"); + + let elapsed = burst_sent.elapsed(); + // Visible under `--nocapture`; also the number to check when triaging a failure. + eprintln!("burst→quit: {elapsed:?}"); + assert!( + elapsed < BURST_RESPONSIVE, + "burst→quit took {elapsed:?} — outline nav is loading files synchronously again \ + (input coalescing or idle-deferred loads regressed)" + ); +} diff --git a/git-workon-review/tests/pty_smoke.rs b/git-workon-review/tests/pty_smoke.rs index 070b2a65..5b72c5f0 100644 --- a/git-workon-review/tests/pty_smoke.rs +++ b/git-workon-review/tests/pty_smoke.rs @@ -22,6 +22,9 @@ #![cfg(unix)] +mod pty_support; +use pty_support::spawn_review; + use std::io::Write; use std::time::{Duration, Instant}; @@ -45,24 +48,6 @@ fn auto_theme_fixture() -> Fixture { .expect("fixture") } -/// Spawn the review binary in a PTY sized like a real terminal (an unsized PTY is 0×0 and -/// ratatui draws nothing), cwd'd into the fixture's worktree. -fn spawn_review(fixture: &Fixture) -> Session { - let repo = fixture.repo().expect("fixture repo"); - let workdir = repo.workdir().expect("fixture workdir").to_path_buf(); - - let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_git-workon-review")); - cmd.current_dir(workdir).env("TERM", "xterm-256color"); - - let mut session = expectrl::Session::spawn(cmd).expect("spawn in PTY"); - session - .get_process_mut() - .set_window_size(120, 40) - .expect("size PTY"); - session.set_expect_timeout(Some(Duration::from_secs(15))); - session -} - /// Play a well-behaved answering terminal: reply to all 16 `OSC 4` color queries plus /// `OSC 11`/`OSC 10`, then the DA1 sentinel. The replies deliberately contain the poison bytes /// of the round-2 wedge — `r`/`g`/`b` (refresh binding) and `d` hex digits (discard binding) — diff --git a/git-workon-review/tests/pty_support/mod.rs b/git-workon-review/tests/pty_support/mod.rs new file mode 100644 index 00000000..1253c867 --- /dev/null +++ b/git-workon-review/tests/pty_support/mod.rs @@ -0,0 +1,32 @@ +//! Shared PTY-test support for the `pty_smoke` and `pty_responsiveness` test binaries. +//! +//! A `tests//mod.rs` directory module so cargo does not build it as a test binary of its +//! own; each PTY suite declares `mod pty_support;`. Keeping the spawn setup in one place means +//! a change to the window size, `TERM`, or expect timeout applies to every PTY suite at once — +//! the two suites guard related regressions, so silent drift here would matter. + +use std::time::Duration; + +use expectrl::{ + session::{OsProcess, OsStream}, + Session, +}; +use git_workon_fixture::prelude::*; + +/// Spawn the review binary in a PTY sized like a real terminal (an unsized PTY is 0×0 and +/// ratatui draws nothing), cwd'd into the fixture's worktree. +pub fn spawn_review(fixture: &Fixture) -> Session { + let repo = fixture.repo().expect("fixture repo"); + let workdir = repo.workdir().expect("fixture workdir").to_path_buf(); + + let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_git-workon-review")); + cmd.current_dir(workdir).env("TERM", "xterm-256color"); + + let mut session = expectrl::Session::spawn(cmd).expect("spawn in PTY"); + session + .get_process_mut() + .set_window_size(120, 40) + .expect("size PTY"); + session.set_expect_timeout(Some(Duration::from_secs(15))); + session +} diff --git a/git-workon-review/tests/source.rs b/git-workon-review/tests/source.rs new file mode 100644 index 00000000..46e8e303 --- /dev/null +++ b/git-workon-review/tests/source.rs @@ -0,0 +1,467 @@ +//! Fixture tests for the M7 `Source` classifier + resolver (ADR-036): the `stack`/`uncommitted` +//! keywords (CS2), and `` shape-aware dispatch + `Range` resolution (CS3). Output +//! assertions pin `NO_COLOR=1` per the FORCE_COLOR trap this dev environment sets. + +use assert_cmd::cargo_bin_cmd; +use git2::ObjectType; +use git_workon_fixture::prelude::*; +use std::error::Error; +use workon::ChangesetSpan; +use workon_review::acquire::{diff_changeset, resolve_changesets, ChangesetDiff}; +use workon_review::error::SourceError; +use workon_review::source::{resolve_source, Source}; + +macro_rules! both_formats { + ($($name:ident),+ $(,)?) => {$( + mod $name { + use super::*; + #[test] fn refs() { super::$name(MetadataFormat::Refs).unwrap() } + #[test] fn sqlite() { super::$name(MetadataFormat::Sqlite).unwrap() } + } + )+}; +} + +both_formats!( + stack_keyword_in_graphite_repo_returns_full_stack, + uncommitted_keyword_in_graphite_repo_returns_single_uncommitted_changeset, +); + +fn stack_keyword_in_graphite_repo_returns_full_stack( + format: MetadataFormat, +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .metadata_format(format) + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .branch_metadata("c", "b") + .build()?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "b", Source::classify("stack"))?; + let names: Vec<&str> = changesets.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, vec!["a", "b", "c"]); + + let current: Vec<&str> = changesets + .iter() + .filter(|c| c.current) + .map(|c| c.name.as_str()) + .collect(); + assert_eq!(current, vec!["b"], "exactly the focused branch is current"); + Ok(()) +} + +fn uncommitted_keyword_in_graphite_repo_returns_single_uncommitted_changeset( + format: MetadataFormat, +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .metadata_format(format) + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .build()?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "b", Source::classify("uncommitted"))?; + assert_eq!(changesets.len(), 1, "always exactly one changeset"); + assert_eq!(changesets[0].span, ChangesetSpan::Uncommitted); + assert!(changesets[0].current); + assert_eq!( + changesets[0].name, "b", + "the uncommitted changeset is named after the focused branch, not the stack" + ); + Ok(()) +} + +#[test] +fn stack_keyword_in_plain_git_repo_with_upstream_returns_per_commit_changesets( +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .remote("origin", "https://example.com/origin.git") + .upstream("main", "origin/main") + .build()?; + fixture.commit("main").file("a.txt", "1").create("first")?; + fixture.commit("main").file("b.txt", "2").create("second")?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "main", Source::classify("stack"))?; + assert_eq!(changesets.len(), 2, "one changeset per commit"); + assert_eq!(changesets[0].title.as_deref(), Some("first")); + assert_eq!(changesets[1].title.as_deref(), Some("second")); + assert!(changesets[1].current); + Ok(()) +} + +#[test] +fn stack_keyword_with_no_upstream_errors() -> Result<(), Box> { + let fixture = FixtureBuilder::new().build()?; + let repo = fixture.repo()?; + + let err = resolve_source(repo, "main", Source::classify("stack")).unwrap_err(); + match err { + SourceError::NoUpstream { branch } => assert_eq!(branch, "main"), + other => panic!("expected NoUpstream, got {other:?}"), + } + Ok(()) +} + +/// `stack` on a branch that's caught up with its upstream and has a clean tree resolves to +/// zero changesets (`assemble_git`'s empty-vec arm, `git_inference_caught_up_and_clean_returns_empty` +/// in `git-workon-lib/tests/changeset.rs`) — end-to-end through the binary this must print +/// "nothing to review" and exit 0, exactly like the no-argument auto-detect path, not panic. +#[test] +fn stack_keyword_caught_up_and_clean_prints_nothing_to_review() { + let fixture = FixtureBuilder::new() + .remote("origin", "https://example.com/origin.git") + .upstream("main", "origin/main") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + + let mut cmd = cargo_bin_cmd!("git-workon-review"); + cmd.current_dir(workdir) + .env("NO_COLOR", "1") + .arg("stack") + .assert() + .success() + .stderr(predicate::str::contains("nothing to review")); +} + +/// The classifier/resolver seam CS2 introduces resolves `Ref` to a named, hinted pre-TUI +/// failure (real ref resolution is CS3) — end-to-end through the binary, so this doubles as +/// the CS2 manual smoke check ("a source shape renders or errors honestly"). Color is pinned +/// off: `FORCE_COLOR=3` is set in this dev environment and would otherwise leak ANSI codes +/// into the assertion. +#[test] +fn unresolvable_ref_source_prints_named_error_and_exits_nonzero() { + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + + let mut cmd = cargo_bin_cmd!("git-workon-review"); + cmd.current_dir(workdir) + .env("NO_COLOR", "1") + .arg("no-such-thing") + .assert() + .failure() + .stderr(predicate::str::contains( + "cannot resolve 'no-such-thing' as a review source", + )); +} + +// ── CS3: `` shape-aware dispatch + `Range` resolution ────────────────────────────────── + +both_formats!(ref_on_graphite_tracked_branch_that_is_head_matches_auto_detect,); + +/// A `` naming the Graphite-tracked branch that IS real `HEAD` must resolve identically to +/// auto-detect (ADR-036: the uncommitted layer rides along). A dirty tree (an untracked file) +/// makes the layer's presence in both outputs an actual assertion, not a vacuous one. +fn ref_on_graphite_tracked_branch_that_is_head_matches_auto_detect( + format: MetadataFormat, +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .metadata_format(format) + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .untracked_file("dirty.txt", "wip") + .build()?; + let repo = fixture.repo()?; + + let auto = resolve_changesets(repo, "b")?; + let via_ref = resolve_source(repo, "b", Source::classify("b"))?; + assert_eq!(auto, via_ref); + assert!( + auto.iter().any(|cs| cs.span == ChangesetSpan::Uncommitted), + "a dirty tree on real HEAD must carry the uncommitted layer" + ); + Ok(()) +} + +/// A `` naming a Graphite-tracked branch that is NOT real `HEAD` never gets the +/// uncommitted layer, even on a dirty tree — the dirty tree belongs to whatever branch is +/// actually checked out, not the reviewed one (ADR-036). +#[test] +fn ref_on_graphite_tracked_branch_that_is_not_head_omits_uncommitted_layer_on_dirty_tree( +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .graphite_config(&["main"]) + .branch_metadata("a", "main") + .branch_metadata("b", "a") + .untracked_file("dirty.txt", "wip") + .build()?; + let repo = fixture.repo()?; + + // head_branch is deliberately NOT "b": the real HEAD is some other branch entirely. + let changesets = resolve_source(repo, "some-other-branch", Source::classify("b"))?; + assert!( + !changesets + .iter() + .any(|cs| cs.span == ChangesetSpan::Uncommitted), + "reviewing a non-HEAD branch must never surface the uncommitted layer" + ); + let current: Vec<&str> = changesets + .iter() + .filter(|c| c.current) + .map(|c| c.name.as_str()) + .collect(); + assert_eq!(current, vec!["b"]); + Ok(()) +} + +/// An untracked local branch with an upstream resolves to one committed changeset spanning +/// `merge-base(upstream, branch)..branch` — "what this branch adds". +#[test] +fn untracked_branch_with_upstream_bases_on_merge_base_with_upstream() -> Result<(), Box> +{ + let fixture = FixtureBuilder::new() + .remote("origin", "https://example.com/origin.git") + .upstream("main", "origin/main") + .build()?; + // `.upstream()` pins `origin/main` to the branch's tip AT BUILD TIME (the root commit) — + // both commits below land after that, so the upstream-anchored merge-base is the root. + let base_oid = fixture.head()?.peel_to_commit()?.id(); + fixture.commit("main").file("a.txt", "1").create("first")?; + let head_oid = fixture.commit("main").file("b.txt", "2").create("second")?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "main", Source::classify("main"))?; + assert_eq!(changesets.len(), 1); + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { + assert_eq!(base, base_oid, "base is the upstream-anchored merge-base"); + assert_eq!(head, head_oid); + } + other => panic!("expected Committed, got {other:?}"), + } + assert_eq!(changesets[0].name, "main"); + Ok(()) +} + +/// An untracked local branch with NO upstream falls back to `merge-base(trunk, branch)`, where +/// trunk is the repo's default branch (no Graphite trunk configured here). +#[test] +fn untracked_branch_without_upstream_bases_on_merge_base_with_trunk() -> Result<(), Box> +{ + let fixture = FixtureBuilder::new() + .default_branch("main") + .worktree("feature") + .build()?; + fixture + .commit("main") + .file("a.txt", "1") + .create("on main")?; + let feature_head = fixture + .commit("feature") + .file("b.txt", "1") + .create("on feature")?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "feature", Source::classify("feature"))?; + assert_eq!(changesets.len(), 1); + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { + let main_tip = repo + .find_branch("main", git2::BranchType::Local)? + .get() + .target() + .unwrap(); + let expected_base = repo.merge_base(main_tip, feature_head)?; + assert_eq!(base, expected_base, "base is the trunk-anchored merge-base"); + assert_eq!(head, feature_head); + } + other => panic!("expected Committed, got {other:?}"), + } + Ok(()) +} + +/// An untracked branch with neither an upstream nor a resolvable trunk is a named error, not a +/// silent fallback. +#[test] +fn untracked_branch_with_no_upstream_and_no_trunk_errors() -> Result<(), Box> { + let fixture = FixtureBuilder::new().default_branch("solo").build()?; + let repo = fixture.repo()?; + + let err = resolve_source(repo, "solo", Source::classify("solo")).unwrap_err(); + match err { + SourceError::NoBaseForBranch { branch } => assert_eq!(branch, "solo"), + other => panic!("expected NoBaseForBranch, got {other:?}"), + } + Ok(()) +} + +/// A bare commit sha resolves to one changeset spanning `parent..sha`. +#[test] +fn commit_sha_resolves_to_parent_and_sha() -> Result<(), Box> { + let fixture = FixtureBuilder::new().build()?; + let parent_oid = fixture.head()?.peel_to_commit()?.id(); + let head_oid = fixture.commit("main").file("a.txt", "1").create("first")?; + let repo = fixture.repo()?; + + let sha = head_oid.to_string(); + let changesets = resolve_source(repo, "main", Source::classify(&sha))?; + assert_eq!(changesets.len(), 1); + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { + assert_eq!(base, parent_oid); + assert_eq!(head, head_oid); + } + other => panic!("expected Committed, got {other:?}"), + } + assert_eq!(changesets[0].name, sha); + assert!(changesets[0].current); + Ok(()) +} + +/// A tag resolves to the commit it points at, same as a bare sha. +#[test] +fn tag_resolves_to_tagged_commit() -> Result<(), Box> { + let fixture = FixtureBuilder::new().build()?; + let parent_oid = fixture.head()?.peel_to_commit()?.id(); + let head_oid = fixture.commit("main").file("a.txt", "1").create("first")?; + let repo = fixture.repo()?; + let tagged = repo.find_object(head_oid, Some(ObjectType::Commit))?; + repo.tag_lightweight("v1", &tagged, false)?; + + let changesets = resolve_source(repo, "main", Source::classify("v1"))?; + assert_eq!(changesets.len(), 1); + match changesets[0].span { + ChangesetSpan::Committed { base, head } => { + assert_eq!(base, parent_oid); + assert_eq!(head, head_oid); + } + other => panic!("expected Committed, got {other:?}"), + } + Ok(()) +} + +/// A root commit (no parent) reviewed on its own must still render — its base is the empty +/// tree, so every file in it shows as added. The commit is a genuine orphan (parents: &[]) so +/// it has no ancestry to fall back on, addressed only by its own sha. +#[test] +fn root_commit_renders_against_the_empty_tree() -> Result<(), Box> { + let fixture = FixtureBuilder::new().build()?; + let repo = fixture.repo()?; + + let sig = git2::Signature::now("Test User", "test@example.com")?; + let blob_oid = repo.blob(b"hello")?; + let mut builder = repo.treebuilder(None)?; + builder.insert("a.txt", blob_oid, 0o100_644)?; + let tree_oid = builder.write()?; + let tree = repo.find_tree(tree_oid)?; + let root_oid = repo.commit(None, &sig, &sig, "orphan root", &tree, &[])?; + + let sha = root_oid.to_string(); + let changesets = resolve_source(repo, "main", Source::classify(&sha))?; + assert_eq!(changesets.len(), 1); + assert_eq!( + changesets[0].span, + ChangesetSpan::CommittedRoot { head: root_oid } + ); + + match diff_changeset(repo, &changesets[0])? { + ChangesetDiff::Committed(model) => { + assert!(!model.files.is_empty(), "root commit must render its file") + } + other => panic!("expected a Committed diff, got {other:?}"), + } + Ok(()) +} + +/// `a..b` and `a...b` diverge after the branches actually diverge: two-dot bases on `a` itself, +/// three-dot bases on their merge-base. +#[test] +fn two_dot_and_three_dot_ranges_differ_after_divergence() -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .default_branch("main") + .worktree("feature") + .build()?; + fixture + .commit("main") + .file("a.txt", "1") + .create("on main")?; + fixture + .commit("feature") + .file("b.txt", "1") + .create("on feature")?; + let repo = fixture.repo()?; + + let main_tip = repo + .find_branch("main", git2::BranchType::Local)? + .get() + .target() + .unwrap(); + let feature_tip = repo + .find_branch("feature", git2::BranchType::Local)? + .get() + .target() + .unwrap(); + let expected_merge_base = repo.merge_base(main_tip, feature_tip)?; + + let two_dot = resolve_source(repo, "main", Source::classify("main..feature"))?; + let three_dot = resolve_source(repo, "main", Source::classify("main...feature"))?; + + match (&two_dot[0].span, &three_dot[0].span) { + ( + ChangesetSpan::Committed { base: b2, head: h2 }, + ChangesetSpan::Committed { base: b3, head: h3 }, + ) => { + assert_eq!(*h2, feature_tip); + assert_eq!(*h3, feature_tip); + assert_eq!(*b2, main_tip, "two-dot bases directly on the left endpoint"); + assert_eq!( + *b3, expected_merge_base, + "three-dot bases on the merge-base" + ); + assert_ne!(b2, b3, "two-dot and three-dot bases diverge"); + } + other => panic!("expected two Committed spans, got {other:?}"), + } + Ok(()) +} + +/// An empty side of a range defaults to `HEAD` at resolution time. +#[test] +fn range_empty_side_defaults_to_head() -> Result<(), Box> { + let fixture = FixtureBuilder::new().branch("old").build()?; + fixture + .commit("main") + .file("a.txt", "1") + .create("advance main")?; + let repo = fixture.repo()?; + let head_oid = repo.head()?.peel_to_commit()?.id(); + + let explicit = resolve_source(repo, "main", Source::classify("old..main"))?; + let defaulted = resolve_source(repo, "main", Source::classify("old.."))?; + // Names differ (source text as typed); spans must be identical — the empty side resolved + // to the exact same commit as writing `main` out explicitly. + assert_eq!(explicit[0].span, defaulted[0].span); + match defaulted[0].span { + ChangesetSpan::Committed { head, .. } => assert_eq!(head, head_oid), + other => panic!("expected Committed, got {other:?}"), + } + Ok(()) +} + +/// `review ..` is a valid-but-empty range: exit 0, "nothing to review" naming +/// the source text (ADR-036's empty-but-valid UX, extended in CS3 to name the source). +#[test] +fn empty_range_between_same_tag_prints_named_nothing_to_review_and_exits_zero() { + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = fixture.repo().unwrap(); + let head_oid = repo.head().unwrap().peel_to_commit().unwrap().id(); + let tagged = repo + .find_object(head_oid, Some(ObjectType::Commit)) + .unwrap(); + repo.tag_lightweight("v1", &tagged, false).unwrap(); + let workdir = repo.workdir().unwrap(); + + let mut cmd = cargo_bin_cmd!("git-workon-review"); + cmd.current_dir(workdir) + .env("NO_COLOR", "1") + .arg("v1..v1") + .assert() + .success() + .stderr(predicate::str::contains("nothing to review in v1..v1")); +} diff --git a/git-workon/src/completers.rs b/git-workon/src/completers.rs index 0018a2e7..74150c88 100644 --- a/git-workon/src/completers.rs +++ b/git-workon/src/completers.rs @@ -1,8 +1,8 @@ -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; use std::path::Path; use clap::builder::StyledStr; -use clap::Command; +use clap::{Command, CommandFactory}; use clap_complete::engine::{ArgValueCompleter, CompletionCandidate}; use workon::WorktreeDescriptor; @@ -93,6 +93,117 @@ fn augment_external_subcommands(cmd: Command) -> Command { }) } +/// First-party externals known to speak the `clap_complete` `COMPLETE=` responder protocol. +/// +/// Delegation (below) has to *execute* the external to get its completions — there is no way to +/// probe "does this binary support `COMPLETE=`" without running it, and a plain user script (the +/// external-subcommand surface explicitly supports those; see `dispatch.rs` and the +/// `PathStub::command` tests) ignores `COMPLETE` entirely and just runs, turning a TAB press into +/// an arbitrary side-effecting execution with its normal stdout misread as completion candidates. +/// ADR-036 only promises this delegation for the review binary, so the allowlist starts there. +/// A future git-config allowlist (`workon.*`, ADR-006) can let users opt other externals in +/// deliberately — extend this list (or make it configurable) when that lands. +const DELEGATED_EXTERNALS: &[&str] = &["review"]; + +/// Sub-delegate `git-workon ` completion to `git-workon-`'s own +/// `COMPLETE=` responder — the M6 CS3-deferred seam, wired here per ADR-036 CS5. +/// +/// `augment_external_subcommands` (above) only adds a bare stub `Command` for each PATH-discovered +/// external, with no argument definitions of its own — clap_complete's engine has no notion that +/// the stub actually stands in for a whole other program, so anything typed after the external's +/// name would otherwise complete against nothing (verified manually: `git workon review ` +/// offered only global flags before this). This function runs *before* `CompleteEnv`'s own +/// dispatch in `main`, so it can intercept that case and hand off instead. +/// +/// It re-derives the shell's word list and completion index the same way `clap_complete`'s own +/// bash/elvish adapters do (`_CLAP_COMPLETE_INDEX`; zsh/fish don't set that var and always mean +/// "the last word", so that's the fallback). If the first non-flag word after the program name +/// names a subcommand in `DELEGATED_EXTERNALS` (see its doc comment for why delegation is gated at +/// all) that also resolves to a `git-workon-` executable on `$PATH` (and isn't a known +/// built-in — a built-in's own `Cli` already completes itself) *and* the word actually being +/// completed sits after it, this re-invokes that executable under the identical protocol: the +/// leading ` ` words collapse into one placeholder word (`git-workon-`, +/// mirroring how the external is invoked for real by `dispatch::try_dispatch`) and the completion +/// index shifts down by however many leading words were collapsed away. Stdin is nulled for the +/// delegated process — an external that prompts on stdin must not be able to block the user's +/// shell on a TAB press. The external's stdout — already shell-formatted by its own `CompleteEnv` +/// responder — is copied through verbatim, and this process exits with the external's exit code. +/// +/// A no-op (returns without printing or exiting) whenever `COMPLETE` isn't set, there's no word +/// after the program name, the completing index lands on the subcommand slot itself (that's +/// still a top-level candidate list, not a delegation target), the leading word is a known +/// built-in, the leading word isn't in `DELEGATED_EXTERNALS`, or nothing matching is found on +/// `$PATH` — every one of those falls through to `CompleteEnv`'s normal dispatch in `main`. +pub fn try_delegate_external_completion() { + let Some(shell) = std::env::var_os("COMPLETE") else { + return; + }; + if shell.is_empty() || shell == "0" { + return; + } + + let args: Vec = std::env::args_os().collect(); + let Some(dash_dash) = args.iter().position(|a| a == "--") else { + return; + }; + let words = &args[dash_dash + 1..]; + if words.len() < 2 { + return; // nothing after the program-name word to delegate + } + + // Mirrors clap_complete's own env adapters: bash/elvish read `_CLAP_COMPLETE_INDEX`; zsh/fish + // always treat the last word as the one being completed. + let index = std::env::var("_CLAP_COMPLETE_INDEX") + .ok() + .and_then(|i| i.parse::().ok()) + .unwrap_or(words.len() - 1); + + // The first non-flag word after the program name (index 0) is the subcommand candidate. + let Some(subcmd_pos) = words[1..] + .iter() + .position(|w| !w.to_str().is_some_and(|s| s.starts_with('-'))) + .map(|i| i + 1) + else { + return; + }; + if index <= subcmd_pos { + return; // completing the subcommand slot itself, not a word after it + } + + let Some(name) = words[subcmd_pos].to_str() else { + return; + }; + let known = crate::dispatch::known_subcommands(&crate::cli::Cli::command()); + if known.contains(name) { + return; // a built-in owns this name; its own Cli completes it + } + if !DELEGATED_EXTERNALS.contains(&name) { + return; // protocol support isn't known/promised for this external; don't execute it + } + let Some(exe) = crate::dispatch::find_external(name) else { + return; // no matching external on PATH + }; + + let mut delegated_words: Vec = vec![OsString::from(format!("git-workon-{name}"))]; + delegated_words.extend(words[subcmd_pos + 1..].iter().cloned()); + let delegated_index = index - subcmd_pos; + + let output = std::process::Command::new(&exe) + .env("COMPLETE", &shell) + .env("_CLAP_COMPLETE_INDEX", delegated_index.to_string()) + .stdin(std::process::Stdio::null()) + .arg("--") + .args(&delegated_words) + .output(); + + let Ok(output) = output else { + std::process::exit(0); // fail closed: no candidates rather than a broken TAB + }; + use std::io::Write as _; + let _ = std::io::stdout().write_all(&output.stdout); + std::process::exit(output.status.code().unwrap_or(0)); +} + pub fn augment(cmd: Command) -> Command { let cmd = augment_external_subcommands(cmd); cmd.mut_arg("name", |a| { diff --git a/git-workon/src/main.rs b/git-workon/src/main.rs index fbd55a85..c7b2e12f 100644 --- a/git-workon/src/main.rs +++ b/git-workon/src/main.rs @@ -18,6 +18,12 @@ use crate::cmd::Run; use crate::json::worktree_to_json; fn main() -> Result<()> { + // Must run before `CompleteEnv`'s own dispatch: an external subcommand's stub `Command` (see + // `completers::augment`) carries no argument definitions, so `CompleteEnv` alone can't + // complete anything typed after it. This hands those words off to the external's own + // `COMPLETE=` responder instead (M6 CS3's deferred seam; ADR-036 CS5), and is a no-op in + // every other case (see its doc comment). + completers::try_delegate_external_completion(); CompleteEnv::with_factory(|| completers::augment(Cli::command())).complete(); dispatch::try_dispatch(&dispatch::known_subcommands(&Cli::command())); diff --git a/git-workon/tests/suite/completions.rs b/git-workon/tests/suite/completions.rs index f15c8441..2dc12608 100644 --- a/git-workon/tests/suite/completions.rs +++ b/git-workon/tests/suite/completions.rs @@ -109,6 +109,83 @@ fn tab_lists_external_subcommands_from_path() { ); } +/// The sibling `git-workon-review` binary, built alongside this test binary as part of the +/// workspace (`cargo test --workspace` / `-p git-workon` after a workspace build both produce +/// it). Located via the workspace's `target//` (one level up from this crate's +/// `CARGO_MANIFEST_DIR`) rather than pulled in as a Cargo dev-dependency, which would drag +/// `git-workon-review`'s whole tree-sitter-heavy dependency tree into every `git-workon` test +/// build for one delegation test. NOT `current_exe()`-relative: this repo shares cargo's +/// intermediate build artifacts across worktrees (a `build-dir` override in `.cargo/config.toml`), +/// so test binaries themselves live under that shared dir while final binaries still land in +/// this worktree's own `target//`. +fn review_binary_path() -> std::path::PathBuf { + let profile = if cfg!(debug_assertions) { + "debug" + } else { + "release" + }; + std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("workspace root") + .join("target") + .join(profile) + .join("git-workon-review") +} + +/// ADR-036 CS5 / M6 CS3's deferred seam: `git workon review ` shells out to the review +/// binary's own `COMPLETE=` responder rather than completing against review's argument-less stub +/// `Command` (`augment_external_subcommands`). Uses the *real* compiled `git-workon-review` +/// (via `PathStub::command_exe`, not the canned `arg:`/`cwd:` script `command` writes) so the +/// candidates asserted here — `stack`/`uncommitted` — are genuinely the review binary's own SOURCE +/// completer output, proving the index-shifted shell-out end to end. +#[test] +fn tab_after_review_subcommand_delegates_to_review_binary_completer() { + let review_exe = review_binary_path(); + assert!( + review_exe.is_file(), + "expected a sibling git-workon-review binary at {review_exe:?} \ + (built as part of a workspace build/test run)" + ); + + let stub = PathStub::new() + .unwrap() + .command_exe("review", &review_exe) + .unwrap(); + + // `git workon review ` — index 2 is the (empty) word after `review`. + let candidates = bash_candidates(&stub.path(), &["git-workon", "review", ""], 2); + + assert!( + candidates.iter().any(|c| c == "stack"), + "expected the review binary's own `stack` keyword candidate, delegated through: {candidates:?}" + ); + assert!( + candidates.iter().any(|c| c == "uncommitted"), + "expected the review binary's own `uncommitted` keyword candidate, delegated through: {candidates:?}" + ); +} + +/// Security regression: only `DELEGATED_EXTERNALS` (`completers.rs`) are re-invoked under the +/// `COMPLETE=` protocol. A non-allowlisted external is a plain user script that has no idea what +/// `COMPLETE` means — it would just run for real on every TAB press, its normal stdout misread as +/// completion candidates and its side effects fired. `PathStub::command`'s canned script prints +/// distinctive `arg:`/`cwd:` lines to stdout when executed; asserting those never appear (and the +/// process still exits cleanly, falling through to the stub top-level candidate) proves the stub +/// was never invoked. +#[test] +fn tab_after_non_allowlisted_external_does_not_execute_it() { + let stub = PathStub::new().unwrap().command("greet").unwrap(); + + let candidates = bash_candidates(&stub.path(), &["git-workon", "greet", ""], 2); + + assert!( + candidates + .iter() + .all(|c| !c.starts_with("arg:") && !c.starts_with("cwd:")), + "non-allowlisted external must never be executed for completion: {candidates:?}" + ); +} + #[test] fn external_subcommand_does_not_shadow_a_builtin_in_completion() { // A `git-workon-list` stub must not produce a duplicate `list` candidate — the built-in owns