Updated branch creation implementation.#12664
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
This PR updates but-rebase’s branch creation/editor construction to build a step graph that more directly mirrors but-graph segment relationships, with fallback behavior when the segment graph is inconsistent with the commit graph.
Changes:
- Reworks
GraphExt::to_editor()to build per-segment step chains (segment ref → commit refs → pick) and then connect segments. - Adds preserved-parent handling for commits whose parents are not present in the traversed graph.
- Simplifies segment-parent discovery to return only parent commit IDs (dropping the prior “via reference” annotation).
Caleb-T-Owens
force-pushed
the
branch-creation-2.0
branch
from
c7302b4 to
de5f8e0
Compare
Caleb-T-Owens
force-pushed
the
branch-creation-2.0
branch
2 times, most recently
from
4fc6d0d to
88e7454
Compare
This new algorithm is more closely tied to the but graph. The old algirithm treated references like things that belonged to commits which made it hard to figure out which of the many permutations of inter-reference relations was the one we should express. This new algirithm has an intermediate step which is more similar to the but-graph represenation which allows us to take the but-graph relations _when_ it’s accurate, before falling back to true commit parantage.
Caleb-T-Owens
force-pushed
the
branch-creation-2.0
branch
from
88e7454 to
bbd812c
Compare
| let mut commits: Vec<Commit> = vec![]; | ||
| // References are ordered from child-most to parent-most | ||
| let mut references: BTreeMap<gix::ObjectId, Vec<gix::refs::FullName>> = BTreeMap::new(); | ||
|
|
||
| let mut head_refname = None; | ||
| let workspace_commit_id = self.managed_entrypoint_commit(repo)?.map(|c| c.id); | ||
| let mut commit_to_idx = HashMap::<gix::ObjectId, SegmentIndex>::new(); | ||
| let mut commit_to_pick_ix = HashMap::<gix::ObjectId, StepGraphIndex>::new(); |
There was a problem hiding this comment.
commit_to_idx is populated but never read. This will trigger an unused-variable warning (and can become a CI failure under clippy/deny-warnings). Remove it, or use it if it's intended for later logic.
| // reachable from the entrypoint TODO(CTO): Look into stopping at the | ||
| // common base |
There was a problem hiding this comment.
The TODO comment got collapsed into a single sentence (...entrypoint TODO(CTO): ...), which reads like two TODOs missing a newline. Split these into separate comment lines so it’s clear they are distinct items.
| // reachable from the entrypoint TODO(CTO): Look into stopping at the | |
| // common base | |
| // reachable from the entrypoint | |
| // TODO(CTO): Look into stopping at the common base |
| .collect::<Vec<_>>(); | ||
|
|
||
| for reference in refs { | ||
| references.push(reference.to_owned()); |
There was a problem hiding this comment.
In this loop, reference is already an owned FullName (it came from a .clone() earlier), but it’s cloned again via to_owned() when pushing into references. This adds extra allocations/clones for every ref; consider pushing/moving reference into references and cloning only for the Step::Reference node (or vice versa).
| references.push(reference.to_owned()); | |
| references.push(reference); |
This new algorithm is more closely tied to the but graph.
The old algirithm treated references like things that belonged to commits which made it hard to figure out which of the many permutations of inter-reference relations was the one we should express.
This new algirithm has an intermediate step which is more similar to the but-graph represenation which allows us to take the but-graph relations when it’s accurate, before falling back to true commit parantage.