Skip to content

Order stacks by the workspace commit's parents, drop parent_order#14452

Closed
mtsgrd wants to merge 2 commits into
masterfrom
order-stacks-by-parent-index
Closed

Order stacks by the workspace commit's parents, drop parent_order#14452
mtsgrd wants to merge 2 commits into
masterfrom
order-stacks-by-parent-index

Conversation

@mtsgrd

@mtsgrd mtsgrd commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Stack display order now follows the workspace commit's real parent array instead of a cached parent_order edge weight — which had gone stale (it held metadata order), so octopus/multi-stack workspaces showed the wrong order.

The parent array is the single source of stack order. Ordering happens once, at graph-build time, so nothing downstream re-sorts: the graph's first-parent traversal, its debug display, and the but st lane order all read the same already-correct order off the edges.

1. Drop parent_order, read the real parents

  • display reads the actual parent_ids; Edge.parent_order, its accessor, and the rebuild_edges_in_parent_order heuristic are all gone
  • preferred_parent_order_by_commit re-keyed to segment ids — still steers the merge-walk to the entrypoint through diamonds, just no weight
  • first parent from parent_ids[0]; but-rebase derives its step order the same way

2. Order once, at graph-build time

  • but-graph orders the ws-commit's outgoing edges by the real parent array in post.rs: stack_entry_commit walks first-parent edges (skipping empties) to find each lane's entry, then sorts by (by_parent, by_metadata) — metadata order stays as a secondary tie-break for lanes the parent array can't tell apart (e.g. two branches sharing one base)
  • the workspace projection's re-sort is deleted — it reads order straight off the edges; the invariant now holds by construction
  • remerged_workspace_commit_v2 (the non-graph writer) sorts its parent list by stack.order instead of HashMap::values(), so the real app produces parents in a deterministic order matching what the display reads

3. Empty branches

  • give each empty branch its own real ws-commit parent, so two empties on distinct bases stay distinct instead of collapsing (one base ↔ one lane); a second empty on an already-used base walks back to a distinct unused base (concise out-of-commits error)
  • but-rebase gives commit-less empties distinct, increasing Edge.order values from a counter, so the step graph never has tied parent orders and the old tie-break v.reverse() is removed
  • follow the first-parent edge by graph order (not dst id) when walking the excluded side, so upstream commits aren't miscounted for empty stacks
  • resolve each branch's own base after integrated-commit pruning instead of falling back to the workspace merge base

4. Fixtures + snapshots

  • fixture helpers check out the first arg as the first parent deterministically; one-empty scenarios list the empty branch first (a no-op git merge of an ancestor would otherwise silently drop it)
  • the three-stacks move scenario gives its stacks distinct bases, so emptied stacks resolve to different parents instead of collapsing onto one
  • multi-stack display flips to the true order across but-graph / but-workspace / but — snapshots re-accepted; the move/squash commit-order asserts are updated for the new order

✅ but-graph / but-workspace / but-rebase / but green; cargo fmt, clippy -D warnings, and pnpm begood all clean.

Note

The construction layer can still collapse two empty branches that genuinely share one git base onto a single ws-commit parent (the array then has fewer slots than stacks). This PR sidesteps that with distinct-base fixtures; the long-term fix is to have move/remerge materialise empties as distinct parents even on a shared base. Tracked in the branch handover.

5. Drive-by: first_ordered_parent (but-rebase)

Reference resolution in materialize/commit/reword/rebase only needs the first parent, but called collect_ordered_parents (a full pruned DFS over every parent) and took .first(). Added first_ordered_parent, which shares the DFS but stops at the first commit. No behaviour change; the all-parents call sites are unchanged.

Copilot AI review requested due to automatic review settings June 25, 2026 19:35
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jun 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates workspace stack display ordering so it is derived from the workspace commit’s parent array (parent_order) instead of relying on petgraph edge iteration/insertion order, and refreshes many snapshot-based tests accordingly.

Changes:

  • Order workspace stacks by parent_order on outgoing edges from the workspace tip segment.
  • Reframe/rename move_branch order assertions to treat the workspace commit’s parent array as the source of truth (not metadata).
  • Update numerous insta snapshots across workspace/graph tests to reflect the new ordering.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/but-workspace/tests/workspace/upstream_integration.rs Updates workspace rendering snapshots to reflect the new parent-array-derived stack order.
crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs Updates expected ref_info stack/segment ordering and associated snapshot IDs/names.
crates/but-workspace/tests/workspace/commit/squash_commits.rs Refreshes workspace graph snapshots for squash scenarios under the new stack ordering.
crates/but-workspace/tests/workspace/commit/move_commit.rs Refreshes snapshots for commit-move scenarios to match parent-array stack ordering.
crates/but-workspace/tests/workspace/commit/discard_commit.rs Refreshes snapshots for discard scenarios to match new ordering.
crates/but-workspace/tests/workspace/branch/tear_off_branch.rs Refreshes snapshots for tear-off scenarios with updated stack ordering.
crates/but-workspace/tests/workspace/branch/move_branch.rs Renames/reframes ordering tests and updates snapshots/assertions to match parent-array authority.
crates/but-workspace/tests/workspace/branch/create_reference.rs Updates snapshots for reference creation scenarios under new ordering.
crates/but-workspace/tests/workspace/branch/apply_unapply.rs Updates snapshots for apply/unapply/checkout scenarios under new ordering.
crates/but-graph/tests/graph/init/with_workspace.rs Updates graph-workspace snapshots reflecting the new stack order behavior.
crates/but-graph/src/projection/workspace/init.rs Implements stack-top ordering using parent_order from outgoing edges of the workspace tip segment.

Comment thread crates/but-graph/src/projection/workspace/init.rs Outdated
@mtsgrd mtsgrd marked this pull request as draft June 25, 2026 19:51
@mtsgrd mtsgrd changed the title Order stacks by the workspace commit's parents [wip] Order stacks by the workspace commit's parents Jun 25, 2026
@mtsgrd mtsgrd changed the title [wip] Order stacks by the workspace commit's parents Order stacks by the workspace commit's parents, drop parent_order Jun 26, 2026
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch 2 times, most recently from 0375359 to a68169f Compare June 26, 2026 17:32
@github-actions github-actions Bot added the CLI The command-line program `but` label Jun 26, 2026
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch 2 times, most recently from aca6bec to 749331f Compare June 27, 2026 20:06
@mtsgrd mtsgrd marked this pull request as ready for review June 27, 2026 20:39
Copilot AI review requested due to automatic review settings June 27, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 43 out of 50 changed files in this pull request and generated 1 comment.

Comment thread crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs Outdated
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 749331f to 9ed607b Compare June 28, 2026 11:26
Copilot AI review requested due to automatic review settings June 28, 2026 14:01
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 9ed607b to dffa882 Compare June 28, 2026 14:01
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from dffa882 to 40e7a25 Compare June 28, 2026 14:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 44 out of 51 changed files in this pull request and generated 3 comments.

Comment thread crates/but-graph/src/init/mod.rs Outdated
Comment thread crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs Outdated
Comment thread crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs Outdated
Copilot AI review requested due to automatic review settings June 28, 2026 14:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 44 out of 51 changed files in this pull request and generated 2 comments.

Comment thread crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs Outdated
Comment thread crates/but-graph/src/init/post.rs
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 40e7a25 to 6c88d11 Compare June 28, 2026 14:27
Copilot AI review requested due to automatic review settings June 28, 2026 17:13
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 6c88d11 to 928d17e Compare June 28, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 44 out of 51 changed files in this pull request and generated 2 comments.

Comment thread crates/but-graph/src/projection/workspace/init.rs Outdated
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 928d17e to c921cda Compare June 28, 2026 17:31
Copilot AI review requested due to automatic review settings June 28, 2026 19:17
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from c921cda to 1b82a0d Compare June 28, 2026 19:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 56 changed files in this pull request and generated 1 comment.

Comment thread crates/but-graph/src/init/post.rs Outdated
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 1b82a0d to 1ede0ec Compare June 28, 2026 19:38
@mtsgrd mtsgrd marked this pull request as draft June 29, 2026 11:56
mtsgrd and others added 2 commits July 4, 2026 04:26
Two things, both in service of correct stack order:

- Drop the cached `parent_order` edge weight and read the workspace
  merge commit's real parent array instead, so display order always
  matches the actual parents.
- Give empty branches their own base: each keeps a real workspace-commit
  parent of its own, so two empty branches on distinct bases stay
  distinct instead of collapsing together.

The parent array is now the single source of stack order. The ordering
happens once, at graph-build time, so no consumer re-sorts: the graph's
first-parent traversal, its debug display, and the workspace projection
all read the same already-correct order off the edges.

Details:
- but-graph: remove parent_order; order the ws-commit's outgoing edges
  by the real parent array at build time (post.rs), and delete the
  projection's re-sort — it now reads the order directly off the edges.
- but-graph: follow the first-parent edge by graph order (not dst id)
  when walking the excluded side, so upstream commits aren't miscounted
  for empty stacks.
- but-rebase editor: order reparented edges by the child's own parents,
  so rebuilt merge commits keep their parents in sequence (tear-off and
  insert). Commit-less empty branches get distinct, increasing edge
  orders from a counter, so the step graph needs no tie-break reversal.
- but-rebase: add first_ordered_parent so reference resolution stops at
  the first commit parent instead of collecting every parent just to take
  the first (materialize / commit / reword / rebase).
- gitbutler-branch-actions: when the merge base already carries an empty
  branch, walk back to a distinct unused base for the new one (concise
  out-of-commits error).
- but-graph projection: resolve each branch's own base after
  integrated-commit pruning instead of falling back to the workspace
  merge base.
- Update test fixtures and snapshots (distinct bases in the three-stacks
  move scenario so emptied stacks don't collapse onto one parent).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`create_workspace_commit_once` makes the first argument the workspace
commit's first parent; with `A B` the merge of B (A's ancestor) is a no-op
and no workspace commit is created at all. `B A` reproduces the upstream
fixture's shape ([base, A]) — the same adjustment ws-with-empty-stack.sh
already received.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mtsgrd mtsgrd force-pushed the order-stacks-by-parent-index branch from 1ede0ec to 6fcec19 Compare July 5, 2026 13:02
@mtsgrd

mtsgrd commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of a consolidated draft PR from commit-graph-experiment — this branch's commits are being folded into that single-branch series (link to follow).

@mtsgrd

mtsgrd commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated draft PR: #14623

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI The command-line program `but` rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants