Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/adr/028-provider-neutral-stack-metadata.md
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,20 @@ sees the same state regardless of which worktree wrote it.
as a dependency but no `serde` derive crate, and the write path in particular needs the raw
`Value` round-trip to preserve `id` and `pullRequest` on stack entries workon itself never
touches.
- **Merged-branch awareness.** `gh stack sync` leaves a merged branch's metadata row behind rather
than deleting it (its help text says the row is kept for rebase and display logic), and its
`--prune` cannot `git branch -D` a branch that a worktree has checked out, which is the normal
git-workon case. To reflect this, `gh_stack::read_metadata` reads `pullRequest.merged` into
`BranchMetadata.merged`, and `Stack` gains a `merged: HashSet<String>` field. `enumerate` drops
merged rows the way it drops ghosts, but first reparents each surviving branch past every merged
ancestor: sync merges a stack bottom-up, so without that step a merged bottom branch would take
the whole remaining stack down with it. `current` retains merged rows, same reasoning as ghost
retention. The CLI tree builder (`build_children` in `display.rs`) drops a merged branch that
has no worktree and splices its live children onto its parent, and renders a dim ` merged`
suffix on a merged branch that does have one. `list --json` gains an additive per-stack
`"merged"` array. **Graphite gap:** neither `.graphite_metadata.db` (its `state` column is
empty in practice) nor the `refs/branch-metadata/*` blobs carry PR merged state, so
`BranchMetadata.merged` is hardcoded `false` for Graphite.

## References

Expand Down
87 changes: 83 additions & 4 deletions git-workon-fixture/src/fixture_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ struct GhStackBranchSpec {
/// Ghost entries simulate a branch gh-stack still tracks but whose git ref was
/// deleted/merged — no local branch ref is created for these.
ghost: bool,
/// Writes `pullRequest: { number, merged: true }` instead of `pullRequest: null`,
/// simulating a branch `gh stack sync` has recorded as merged.
merged: bool,
}

/// One `stacks[]` entry queued by [`FixtureBuilder::gh_stack`]/[`FixtureBuilder::gh_stack_at`].
Expand Down Expand Up @@ -153,6 +156,13 @@ enum GhStackOp {
number: u64,
branch: String,
},
/// Flip a branch already queued by a prior [`GhStackOp::Stack`] on the `target` file's stack
/// numbered `number` to `merged: true`.
MergedBranch {
target: GhStackTarget,
number: u64,
branch: String,
},
/// Overwrite `target`'s file with raw bytes after all JSON writes — truncated, `v2`, or
/// plain garbage content for error-path tests.
Raw {
Expand Down Expand Up @@ -435,6 +445,7 @@ impl<'fixture> FixtureBuilder<'fixture> {
branch: branch.to_string(),
base: GhStackBase::ResolveParentTip,
ghost: false,
merged: false,
})
.collect(),
};
Expand Down Expand Up @@ -464,6 +475,7 @@ impl<'fixture> FixtureBuilder<'fixture> {
branch: branch.to_string(),
base: GhStackBase::Verbatim(base.to_string()),
ghost: false,
merged: false,
})
.collect(),
};
Expand Down Expand Up @@ -493,6 +505,27 @@ impl<'fixture> FixtureBuilder<'fixture> {
self
}

/// Mark an already-queued branch on `worktree`'s stack numbered `number` as merged —
/// writes `pullRequest: { number, merged: true }` for it instead of `pullRequest: null`,
/// simulating `gh stack sync` recording a merged PR.
///
/// A [`gh_stack`](Self::gh_stack)/[`gh_stack_at`](Self::gh_stack_at) call queueing `branch`
/// on this `(worktree, number)` must come first — `build()` panics otherwise, mirroring
/// [`gh_stack_ghost_branch`](Self::gh_stack_ghost_branch).
pub fn gh_stack_merged_branch(
mut self,
worktree: Option<&str>,
number: u64,
branch: &str,
) -> Self {
self.gh_stack_ops.push(GhStackOp::MergedBranch {
target: worktree.map(str::to_string),
number,
branch: branch.to_string(),
});
self
}

/// Overwrite `worktree`'s gh-stack file with raw `bytes` after all other gh-stack writes —
/// for truncated-read, `schemaVersion: 2`, and garbage-content error-path tests.
pub fn raw_gh_stack(mut self, worktree: Option<&str>, bytes: Vec<u8>) -> Self {
Expand Down Expand Up @@ -913,12 +946,22 @@ impl<'fixture> FixtureBuilder<'fixture> {
}
}

fn branch_ref_json(branch: &str, head: String, base: String) -> serde_json::Value {
fn branch_ref_json(
branch: &str,
head: String,
base: String,
merged: bool,
) -> serde_json::Value {
let pull_request = if merged {
serde_json::json!({ "number": 1, "merged": true })
} else {
serde_json::Value::Null
};
serde_json::json!({
"branch": branch,
"head": head,
"base": base,
"pullRequest": serde_json::Value::Null,
"pullRequest": pull_request,
})
}

Expand Down Expand Up @@ -980,8 +1023,44 @@ impl<'fixture> FixtureBuilder<'fixture> {
branch: branch.clone(),
base: GhStackBase::ResolveParentTip,
ghost: true,
merged: false,
});
}
GhStackOp::MergedBranch {
target,
number,
branch,
} => {
let i = target_index(&stacks_by_target, target).unwrap_or_else(|| {
panic!(
"gh_stack_merged_branch({target:?}, {number}, {branch:?}): \
no prior gh_stack/gh_stack_at queued a stack numbered {number} \
for this target"
)
});
let entries = &mut stacks_by_target[i].1;
let spec = entries
.iter_mut()
.find(|s| s.number == *number)
.unwrap_or_else(|| {
panic!(
"gh_stack_merged_branch({target:?}, {number}, {branch:?}): \
no queued stack numbered {number} for this target"
)
});
let branch_spec = spec
.branches
.iter_mut()
.find(|b| &b.branch == branch)
.unwrap_or_else(|| {
panic!(
"gh_stack_merged_branch({target:?}, {number}, {branch:?}): \
no queued branch {branch:?} on stack numbered {number} \
for this target"
)
});
branch_spec.merged = true;
}
_ => {}
}
}
Expand All @@ -1008,13 +1087,13 @@ impl<'fixture> FixtureBuilder<'fixture> {
}
};
let head = resolve_tip(&b.branch).unwrap_or_default();
branch_ref_json(&b.branch, head, base)
branch_ref_json(&b.branch, head, base, b.merged)
})
.collect();
serde_json::json!({
"id": format!("id-{}", spec.number),
"number": spec.number,
"trunk": branch_ref_json(&spec.trunk, trunk_tip, String::new()),
"trunk": branch_ref_json(&spec.trunk, trunk_tip, String::new(), false),
"branches": branches_json,
})
})
Expand Down
7 changes: 6 additions & 1 deletion git-workon-lib/src/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub(crate) mod gh_stack;
pub(crate) mod graphite;
pub(crate) mod metadata;

use std::collections::{BTreeMap, HashMap};
use std::collections::{BTreeMap, HashMap, HashSet};

use git2::Repository;

Expand Down Expand Up @@ -147,6 +147,10 @@ pub struct Stack {
/// `(trunk, sorted diff set)` and does not consult this field. Always `None` for
/// [`StackModel::Graphite`] and [`StackModel::Git`], which have no numbering concept.
pub number: Option<u64>,
/// Member branches (a subset of [`Stack::diffs`]) whose PR has merged, per
/// `BranchMetadata::merged`. Always empty for [`StackModel::Graphite`] — see
/// [`graphite::read_branch_metadata`]'s docs for why merged state isn't observable there.
pub merged: HashSet<String>,
}

/// Return all stacks present in metadata, one per connected component.
Expand Down Expand Up @@ -403,6 +407,7 @@ mod tests {
current: current.to_string(),
parents: HashMap::new(),
number: None,
merged: HashSet::new(),
}
}

Expand Down
73 changes: 66 additions & 7 deletions git-workon-lib/src/stack/gh_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,11 @@ use super::metadata::{self, BranchMetadata, StackMetadata};
use super::Stack;
use crate::error::StackError;

/// One `branchRef` entry (`{ branch, head, base, pullRequest }`), pulled from a raw
/// `serde_json::Value` rather than a derived struct (this crate has no `serde` derive
/// dependency, only `serde_json`; see `graphite.rs` for the same raw-`Value` convention).
/// `head` and `pullRequest` are read from the file but not carried into [`StackMetadata`]:
/// assembly uses the live tip for `head`, and `pullRequest` has no `StackMetadata` field.
/// Both matter to the write path (added in a later changeset), which round-trips the raw
/// `Value` to preserve them.
#[derive(Debug)]
struct GhStackBranchRef {
branch: String,
base: String,
merged: bool,
}

impl GhStackBranchRef {
Expand All @@ -82,6 +76,11 @@ impl GhStackBranchRef {
.and_then(|v| v.as_str())
.unwrap_or_default()
.to_string(),
merged: value
.get("pullRequest")
.and_then(|pr| pr.get("merged"))
.and_then(|v| v.as_bool())
.unwrap_or(false),
})
}
}
Expand Down Expand Up @@ -329,6 +328,7 @@ pub(crate) fn read_metadata(repo: &Repository) -> Result<StackMetadata, StackErr
.or_insert(BranchMetadata {
parent: parent.clone(),
parent_revision,
merged: branch_ref.merged,
});
if entry.number != 0 {
stack_numbers
Expand Down Expand Up @@ -1045,6 +1045,65 @@ mod tests {
assert_eq!(stacks[0].diffs, vec!["feat-a", "feat-b"]);
}

#[test]
fn merged_true_is_read_from_pull_request() {
let fixture = FixtureBuilder::new()
.bare(true)
.default_branch("main")
.worktree("main")
.branch("feat-a")
.branch("feat-b")
.raw_gh_stack(
None,
br#"{"schemaVersion": 1, "stacks": [{"number": 1, "trunk": {"branch": "main", "head": "", "base": ""}, "branches": [{"branch": "feat-a", "head": "", "base": "", "pullRequest": {"number": 1, "merged": true}}, {"branch": "feat-b", "head": "", "base": "", "pullRequest": null}]}]}"#.to_vec(),
)
.build()
.unwrap();
let repo = fixture.repo().unwrap();

let meta = read_metadata(repo).unwrap();
assert!(meta.parents["feat-a"].merged);
assert!(!meta.parents["feat-b"].merged, "sibling must stay unmerged");
}

#[test]
fn missing_pull_request_reads_merged_false() {
let fixture = FixtureBuilder::new()
.bare(true)
.default_branch("main")
.worktree("main")
.branch("feat-a")
.raw_gh_stack(
None,
br#"{"schemaVersion": 1, "stacks": [{"number": 1, "trunk": {"branch": "main", "head": "", "base": ""}, "branches": [{"branch": "feat-a", "head": "", "base": ""}]}]}"#.to_vec(),
)
.build()
.unwrap();
let repo = fixture.repo().unwrap();

let meta = read_metadata(repo).unwrap();
assert!(!meta.parents["feat-a"].merged);
}

#[test]
fn null_pull_request_reads_merged_false() {
let fixture = FixtureBuilder::new()
.bare(true)
.default_branch("main")
.worktree("main")
.branch("feat-a")
.raw_gh_stack(
None,
br#"{"schemaVersion": 1, "stacks": [{"number": 1, "trunk": {"branch": "main", "head": "", "base": ""}, "branches": [{"branch": "feat-a", "head": "", "base": "", "pullRequest": null}]}]}"#.to_vec(),
)
.build()
.unwrap();
let repo = fixture.repo().unwrap();

let meta = read_metadata(repo).unwrap();
assert!(!meta.parents["feat-a"].merged);
}

#[test]
fn ghost_retained_by_current_stack_and_pruned_by_enumerate() {
let fixture = FixtureBuilder::new()
Expand Down
7 changes: 7 additions & 0 deletions git-workon-lib/src/stack/graphite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ fn read_branch_metadata_from_sqlite(
BranchMetadata {
parent,
parent_revision,
// No observable merged-state field: the sqlite `state` column is empty in
// practice and `.graphite_pr_info` carries no merge marker. Known gap — see
// ADR-028.
merged: false,
},
);
}
Expand Down Expand Up @@ -222,6 +226,9 @@ fn read_branch_metadata_from_refs(
BranchMetadata {
parent: parent.to_string(),
parent_revision,
// Same known gap as the sqlite path above: no merged-state field exists
// in `refs/branch-metadata/*` blobs either.
merged: false,
},
);
}
Expand Down
Loading