insert segment: Insert as first parents#14167
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts graph_rebase segment insertion to consistently place inserted segments on the first-parent lane (and, when inserting above, to make the target the first parent), addressing merge-lane visibility issues (GB-1525). It also adds/updates regression tests and a dedicated workspace scenario fixture to validate merge commit reordering behavior.
Changes:
- Update
Editor::insert_segment_into()to prepend insertion-location parents as first parents, preserving existing parents as merge-side parents. - Add workspace commit-move regression tests for reordering a merge commit above/below while keeping child commits visible.
- Update
insert_segmentsnapshots and add a new scenario fixture for GB-1525.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/but-rebase/src/graph_rebase/mutate.rs |
Introduces prepend_edges_to_parents() and changes parent-edge ordering semantics during segment insertion. |
crates/but-rebase/tests/rebase/graph_rebase/insert_segment.rs |
Updates snapshots to reflect the new parent ordering behavior in segment insertion tests. |
crates/but-workspace/tests/workspace/commit/move_commit.rs |
Adds regression tests verifying merge commit reordering preserves the visible first-parent lane. |
crates/but-workspace/tests/fixtures/scenario/gb-1525-reorder-merge-commit.sh |
Adds a new fixture repo topology used by the workspace regression tests. |
estib-vega
force-pushed
the
insert-segment-improvement
branch
from
4750645 to
cf47b84
Compare
estib-vega
force-pushed
the
insert-segment-improvement
branch
from
cf47b84 to
7030a8e
Compare
estib-vega
force-pushed
the
insert-segment-improvement
branch
from
7030a8e to
4160e2b
Compare
estib-vega
requested review from
Byron,
Caleb-T-Owens and
jonathantanmy2
as code owners
estib-vega
force-pushed
the
insert-segment-improvement
branch
from
4160e2b to
1e231cb
Compare
| fn add_edges_to_parents( | ||
| &mut self, | ||
| child_node: petgraph::prelude::NodeIndex, | ||
| new_parent_nodes: impl IntoIterator<Item = petgraph::prelude::NodeIndex>, | ||
| parent_reparenting_order: ParentReparentingOrder, | ||
| ) { |
There was a problem hiding this comment.
It seems like this function both deduplicates the list of new parents, and the list of old parents, as well as removing duplicate existing parents if they now show up in the new parents.
git doesn't allow you to create duplicate parents through the CLI, but it does support it in it's data model, and the stance from the git discord server was that it's probably fine to make such a monstrosity.
I'm wondering if the de-duplication is really needed, or if it might end up causing more confusion in the future
There was a problem hiding this comment.
Interesting, I didn't know that the data model could handle duplicated parents.
I can take it out
Always insert the segments as first parents when inserting below. If insertng above, inserts the target as the first parent
estib-vega
force-pushed
the
insert-segment-improvement
branch
from
1e231cb to
8bc345e
Compare
| /// Controls where reparented insertion-location parents are ordered relative to | ||
| /// existing parents on the segment. | ||
| #[derive(Debug, Clone)] | ||
| pub enum ParentReparentingOrder { | ||
| /// Put reparented insertion-location parents before existing segment parents. | ||
| Prepend, | ||
| /// Put reparented insertion-location parents after existing segment parents. | ||
| Append, | ||
| } |
| assert_eq!( | ||
| parent_subjects(&repo, "HEAD~3")?, | ||
| vec!["base".to_string(), "update parent.txt (2)".to_string()], | ||
| "moving the merge commit below main should make main's first parent the merge commit's first parent" |
By default, insert the segments as first parents when inserting below. If
insertng above, inserts the target as the first parent.
Fixes GB-1525
Implementation notes:
ParentReparentingOrderso advanced callers can choose whether reparented parents are prepended or appended.insert_segmentkeeps the default behavior simple by usingPrepend.