From ccbf3ba52b39ca2b092212dc184ff49190a1491b Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Fri, 4 Sep 2026 17:56:13 -0400 Subject: [PATCH] fix(review): honor workon.stackModel over stack auto-detection --- git-workon-review/src/acquire.rs | 24 ++++++-- git-workon-review/src/source.rs | 29 +++++----- git-workon-review/tests/suite/source.rs | 77 +++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 18 deletions(-) diff --git a/git-workon-review/src/acquire.rs b/git-workon-review/src/acquire.rs index e8f597d6..416899aa 100644 --- a/git-workon-review/src/acquire.rs +++ b/git-workon-review/src/acquire.rs @@ -6,7 +6,9 @@ //! git2 diffs and then a [`DiffModel`]. use git2::{DiffFindOptions, DiffOptions, Oid, Repository}; -use workon::{assemble_changesets, Changeset, ChangesetSpan, StackModel, UncommittedLayer}; +use workon::{ + assemble_changesets, Changeset, ChangesetSpan, StackModel, UncommittedLayer, WorkonConfig, +}; use crate::error::DiffError; use crate::model::DiffModel; @@ -200,10 +202,22 @@ pub fn diff_changesets( .collect() } +/// The stack provider in effect for `repo`: `workon.stackModel` when set, else auto-detect +/// (see [`WorkonConfig::stack_model`] for the precedence). Every stack-model decision in this +/// crate goes through here so an explicit config always beats [`StackModel::detect`]'s +/// Graphite-wins tiebreak, which otherwise blinds a gh-stack repo whose Graphite artifacts +/// linger in the common dir. A config read error or an unrecognized value falls back to +/// detection: the model only picks which metadata to read, so it is not worth failing over. +pub fn stack_model(repo: &Repository) -> StackModel { + WorkonConfig::new(repo) + .and_then(|config| config.stack_model(None)) + .unwrap_or_else(|_| StackModel::detect(repo)) +} + /// Resolve the changeset stack the review App opens on for the worktree whose `HEAD` is -/// `head_branch` (locked design decision: auto-detect Graphite, else a single uncommitted -/// changeset): the full Graphite stack -/// when one is active, or a single synthetic [`Changeset`] spanning the uncommitted worktree +/// `head_branch` (locked design decision: auto-detect a stack tool, else a single uncommitted +/// changeset): the full stack when a metadata-backed model (Graphite or gh-stack) is active +/// per [`stack_model`], or a single synthetic [`Changeset`] spanning the uncommitted worktree /// otherwise. /// /// This does NOT simply forward to [`workon::assemble_changesets`] with the detected @@ -219,7 +233,7 @@ pub fn resolve_changesets( repo: &Repository, head_branch: &str, ) -> Result, DiffError> { - match StackModel::detect(repo) { + match stack_model(repo) { model @ (StackModel::Graphite | StackModel::GhStack) => Ok(assemble_changesets( repo, head_branch, diff --git a/git-workon-review/src/source.rs b/git-workon-review/src/source.rs index 7ca2cb30..d0235f87 100644 --- a/git-workon-review/src/source.rs +++ b/git-workon-review/src/source.rs @@ -22,7 +22,7 @@ use workon::{ PrMetadata, StackModel, UncommittedLayer, WorkonError, }; -use crate::acquire::uncommitted_changeset; +use crate::acquire::{stack_model, uncommitted_changeset}; use crate::error::SourceError; /// Which dot form a [`Source::Range`] was spelled with — git-diff semantics differ (ADR-036). @@ -214,17 +214,17 @@ fn remote_branch_tip(repo: &Repository, remote: &str, branch: &str) -> Option Result, SourceError> { - let model = match StackModel::detect(repo) { + let model = match stack_model(repo) { StackModel::None | StackModel::Git => StackModel::Git, metadata @ (StackModel::Graphite | StackModel::GhStack) => metadata, }; @@ -250,8 +250,9 @@ fn map_assemble_err(branch: &str) -> impl Fn(WorkonError) -> SourceError + '_ { /// `` 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) +/// 1. A stack-tracked LOCAL branch (`text` names a local branch, qualified spellings like +/// `refs/heads/`/`heads/` included, AND that branch has a metadata row in the +/// active provider per [`stack_model`], Graphite or gh-stack) /// → 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`]. @@ -269,17 +270,19 @@ fn resolve_ref( 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 model = stack_model(repo); + if matches!(model, StackModel::Graphite | StackModel::GhStack) + && workon::current_stack(repo, &branch_name, model) + .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) + return assemble_changesets(repo, &branch_name, model, layer) .map_err(map_assemble_err(&branch_name)); } diff --git a/git-workon-review/tests/suite/source.rs b/git-workon-review/tests/suite/source.rs index bff3bcc9..6c132e43 100644 --- a/git-workon-review/tests/suite/source.rs +++ b/git-workon-review/tests/suite/source.rs @@ -468,3 +468,80 @@ fn empty_range_between_same_tag_prints_named_nothing_to_review_and_exits_zero() .success() .stderr(predicate::str::contains("nothing to review in v1..v1")); } + +// `workon.stackModel` must beat auto-detection. `StackModel::detect` picks Graphite whenever a +// `.graphite_repo_config` exists, so a gh-stack repo whose Graphite artifacts linger reads the +// wrong (empty) metadata and the stack branch falls through to git inference with no upstream. +// Each of the three stack-model decisions in the binary is pinned separately. + +fn lingering_graphite_over_gh_stack() -> Result> { + let fixture = FixtureBuilder::new() + .graphite_config(&["main"]) + .gh_stack(None, 1, "main", &["a", "b", "c"]) + .config("workon.stackModel", "gh-stack") + .build()?; + Ok(fixture) +} + +fn names(changesets: &[workon::Changeset]) -> Vec<&str> { + changesets.iter().map(|c| c.name.as_str()).collect() +} + +#[test] +fn stack_keyword_honors_stack_model_config_over_lingering_graphite_artifacts( +) -> Result<(), Box> { + let fixture = lingering_graphite_over_gh_stack()?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "b", Source::classify("stack"))?; + assert_eq!(names(&changesets), vec!["a", "b", "c"]); + Ok(()) +} + +#[test] +fn auto_detect_honors_stack_model_config_over_lingering_graphite_artifacts( +) -> Result<(), Box> { + let fixture = lingering_graphite_over_gh_stack()?; + let repo = fixture.repo()?; + + let changesets = resolve_changesets(repo, "b")?; + assert_eq!(names(&changesets), vec!["a", "b", "c"]); + Ok(()) +} + +#[test] +fn ref_on_gh_stack_tracked_branch_resolves_the_whole_stack_under_stack_model_config( +) -> Result<(), Box> { + let fixture = lingering_graphite_over_gh_stack()?; + let repo = fixture.repo()?; + + let changesets = resolve_source(repo, "some-other-branch", Source::classify("b"))?; + assert_eq!(names(&changesets), 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"]); + Ok(()) +} + +/// Without the config, detection's Graphite-wins tiebreak still applies and the gh-stack +/// branch has no Graphite row: the `stack` keyword reports the missing upstream. Documents the +/// trap the config escape hatch exists for. +#[test] +fn stack_keyword_without_stack_model_config_still_loses_to_lingering_graphite_artifacts( +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .graphite_config(&["main"]) + .gh_stack(None, 1, "main", &["a", "b", "c"]) + .build()?; + let repo = fixture.repo()?; + + let err = resolve_source(repo, "b", Source::classify("stack")).unwrap_err(); + assert!( + matches!(err, SourceError::NoUpstream { ref branch } if branch == "b"), + "{err:?}" + ); + Ok(()) +}