-
Notifications
You must be signed in to change notification settings - Fork 725
Creating steps #11195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Creating steps #11195
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
0251bf7 to
1e8dd78
Compare
c6608cf to
83041a9
Compare
0c1e750 to
95cd28b
Compare
My notesNote that swap is like Usage
Commit operations from usage
Trees are not created by the rebase engine unless it's by cherry-pickingOperation modality
ConsiderationsSelections
Excluded Selections
Tree changes or not during replace or insertion
Reference updates
Metadata updates
Worktree Updates
Multiple Workspace: Ignore
Shortest path to valueHow can we get something merged and used in the app that already allows to enable single-branch compatibility? Reword
Open Questions
Tasks
|
7cee094 to
120f675
Compare
|
Some tests have now been added that perform a "reword commit" operation and an "amend" operation & currently have an initial version of what the public interface might look like https://github.com/gitbutlerapp/gitbutler/blob/120f675a5c93347fa76765656db95ef7258dbd49/crates/but-rebase/tests/rebase/graph_rebase/replace.rs. Based on these tests, Byron and I caught up about public interface which has given me some areas to explore. The materialisation step is also something that I'm still working on. There are some cases around checkouts and worktrees that are not robustly considered. |
4dbd0d8 to
42cf963
Compare
e8cb47a to
885a370
Compare
885a370 to
d644db3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new graph-based rebase engine to modernize GitButler's commit manipulation operations. The new system operates on arbitrary commit graphs (including merge commits) rather than just linear commit sequences, providing better support for single branch mode and more robust history rewriting capabilities.
Key Changes
- New graph-based rebase engine (
crates/but-rebase/src/graph_rebase/) that abstracts history-rewrite logic, supporting operations on arbitrary commit graphs with merge commits - Enhanced cherry-pick functionality that handles N-to-M parent scenarios for merge commits
- Comprehensive test coverage with new test fixtures and identity/operation tests to validate the rebase engine behavior
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
crates/but-rebase/src/graph_rebase/mod.rs |
Core module defining the Step enum, Editor struct, and graph structures for the new rebase engine |
crates/but-rebase/src/graph_rebase/creation.rs |
Graph creation logic that converts segment graphs into editor step graphs |
crates/but-rebase/src/graph_rebase/mutate.rs |
Editor mutation operations (select, replace, insert) for manipulating rebase steps |
crates/but-rebase/src/graph_rebase/rebase.rs |
Core rebase execution logic with topological ordering and cherry-pick operations |
crates/but-rebase/src/graph_rebase/materialize.rs |
Materialization logic to persist rebase results to disk and update references |
crates/but-rebase/src/graph_rebase/cherry_pick.rs |
Generalized cherry-pick supporting N-to-M parent scenarios for merge commits |
crates/but-rebase/src/graph_rebase/commit.rs |
Helper utilities for finding and writing commits in the editor |
crates/but-rebase/src/graph_rebase/testing.rs |
Testing utilities for visualizing step graphs as DOT format |
crates/but-rebase/tests/rebase/graph_rebase/*.rs |
Comprehensive test suite covering identity, replace, insert, and cherry-pick operations |
crates/but-rebase/tests/fixtures/rebase.sh |
New test fixtures including cherry-pick scenarios and multiple reference scenarios |
crates/but-rebase/tests/rebase/main.rs |
Updated test utilities to return metadata from fixtures |
crates/but-rebase/src/lib.rs |
Exports the new graph_rebase module |
crates/but-rebase/Cargo.toml |
Added dependencies: but-graph, petgraph, but-meta (dev) |
crates/but-graph/Cargo.toml |
Moved petgraph to workspace dependencies |
Cargo.toml |
Added petgraph as workspace dependency with stable_graph feature |
crates/gitbutler-repo-actions/src/repository.rs |
Improved force push protection error message to include details |
crates/but-claude/src/bridge.rs |
Added ExecutionFailed variant to distinguish Claude availability states |
crates/but-rebase/src/cherry_pick.rs |
Made ConflictEntries fields pub(crate) for use in graph_rebase module |
| .collect::<Vec<_>>(), | ||
| ); | ||
|
|
||
| // A 1 to 1 mapping between the incoming graph and hte output graph |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "hte" should be "the"
| // A 1 to 1 mapping between the incoming graph and hte output graph | |
| // A 1 to 1 mapping between the incoming graph and the output graph |
|
|
||
| let mut editor = graph.to_editor(&repo)?; | ||
|
|
||
| // get the origional a |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "origional" should be "original"
| @@ -0,0 +1,115 @@ | |||
| //! Operations for mutation the editor | |||
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar error in module documentation: "mutation" should be "mutating" - should read "Operations for mutating the editor"
| //! Operations for mutation the editor | |
| //! Operations for mutating the editor |
| } | ||
|
|
||
| #[test] | ||
| fn cherry_pick_back_to_origional_parents_unconflicts() -> Result<()> { |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in test function name: "origional" should be "original"
| fn cherry_pick_back_to_origional_parents_unconflicts() -> Result<()> { | |
| fn cherry_pick_back_to_original_parents_unconflicts() -> Result<()> { |
| /// Represents in which order the `parent` fields should be written out | ||
| /// | ||
| /// A child commit should have edges that all have unique orders. In order | ||
| /// to achive that we can employ the following semantics. |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "achive" should be "achieve"
| /// to achive that we can employ the following semantics. | |
| /// to achieve that we can employ the following semantics. |
| unsafe { | ||
| std::env::set_var(key, value); | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unsafe block around std::env::set_var is marked as unsafe in newer Rust versions because it can cause data races in multi-threaded contexts. Consider using a safer approach like serial_test crate to ensure tests using environment variables don't run in parallel, or use a different mechanism to configure test behavior (e.g., passing configuration through function parameters).
| unsafe { | |
| std::env::set_var(key, value); | |
| } | |
| std::env::set_var(key, value); |
| pub(crate) repo: gix::Repository, | ||
| /// A mapping of any commits that were rewritten as part of the rebase | ||
| pub(crate) commit_mapping: HashMap<gix::ObjectId, gix::ObjectId>, | ||
| /// A mapping between the origional step graph and the new one |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "origional" should be "original"
| /// A mapping between the origional step graph and the new one | |
| /// A mapping between the original step graph and the new one |
| while let Some(candidate) = potential_parent_edges.pop() { | ||
| if let Step::Pick { .. } = graph[candidate.target()] { | ||
| parents.push(candidate.target()); | ||
| // Don't persue the children |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "persue" should be "pursue"
| // Don't persue the children | |
| // Don't pursue the children |
| a_obj.message = "A: a second coming".into(); | ||
| let a_new = repo.write_object(a_obj.inner)?.detach(); | ||
|
|
||
| // select the origional a out of the graph |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "origional" should be "original"
| // select the origional a out of the graph | |
| // select the original a out of the graph |
| /// A child commit should have edges that all have unique orders. In order | ||
| /// to achive that we can employ the following semantics. | ||
| /// | ||
| /// When replacing a given parent with N other parents, the first in that list takes the old parent's order, and the rest take the |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete comment: The comment ends mid-sentence with "the first in that list takes the old parent's order, and the rest take the". It appears the sentence is cut off and should be completed to explain what happens to the remaining items.
| /// When replacing a given parent with N other parents, the first in that list takes the old parent's order, and the rest take the | |
| /// When replacing a given parent with N other parents, the first in that list takes the old parent's order, and the rest take incremented orders following the original. This ensures that the ordering of parent edges remains unique and consistent. |
Graph based rebasing?
As a high level goal, we want to improve our existing operations to have the following qualities:
The vast majority of operations in GitButler don't live some combination of these ideals. One set of operations that we care to improve are the ones that involve manipulating commits in some way.
To name a few:
Each of these operations modify commits and since commits are not mutable, some portion of the git commit graph needs to be updated - and any cooresponding references need updated.
With each of these operations, we could do the history rewrites by hand each time - but that would expose a large amount of detail in the implementations.
In order to help simplify the current implementations of the listed operations, we have opted to break out the history-rewrite into it's own function, letting the body of these operations focus just on manipulating the specific commits that they are interested in.
Some examples of existing functions
Renaming commits
The current implementation can be found at https://github.com/gitbutlerapp/gitbutler/blob/master/crates/gitbutler-branch-actions/src/virtual.rs#L525 in full.
A high-level overview of this function is as follows:
git rebase -ioutput.This implementation doesn't check for workspace conflicts because it's not changing any trees.
Of note here, the old rebase engine has a "new_message" property in it's steps. It origionally had a "new_tree" property too, but the general concensus was that rewriting the specific really ought to be the concern of the operation's implementation, rather than the rebase-engine, so re-treeing was remove. I only presume that "new_message" wasn't removed due to it being less offencive & the relative effort required to remove it.
Reordering commits
The implementation can be seen in full at: https://github.com/gitbutlerapp/gitbutler/blob/master/crates/gitbutler-branch-actions/src/reorder.rs#L25.
A high-level overview of this function is as follows:
Strangly this doesn't look ahead to see if the workspace commit merge will succeed.
Moving changes between commits
The implementation can be seen in full at: https://github.com/gitbutlerapp/gitbutler/blob/master/crates/but-workspace/src/legacy/tree_manipulation/move_between_commits.rs#L58.
A high-level overview of this function is as follows:
The updating of the workspace is handled externally.
We do the rebase in two-steps in the destination_stack_id == source_stack_id case so we can make use of the rebased destination commit (if it was "above" the source commit).
How do we want to represent these operations going forwards?
Throughout the history of GitButler, we've always performed series of cherry-picks with differing utilities, where some might cherry pick an array of commits, and others might take a special range spec. The consolidation of history rewriting into the linear rebase engine however was a healthy step forward into standardizing how we build these operations that modify git history, and abstracted out excess detail from main buisness logic of the operations.
The current solution does have some downsides:
The over-all idea of abstracting out history-rewriting still seems to be a helpful concept. As such it seems reasonable to try and build a new rebase engine that addresses the above listed downsides, and adheres to the goals listed further above.
It seems to make sense to have something that can operate on an arbitrary commit graph, with some additional GitButler-specific introspections.
There is some prior art in reguards to rebasing commit graphs. Namely:
git rebase -i --with-merges- https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---rebase-mergesrebase-cousinsno-rebase-cousinsgit replay- https://git-scm.com/docs/git-replay - https://youtu.be/0JSsxRcs-aEA general flow
A general flow for some new system at a high level could be along the lines of:
Create an editor that gives us the ability to maniuplate the commits that get picked, similar to the old rebase engine.
Update the pick statements like the old rebase engine to use commits that were rewritten by the specific operation
Perform the rebase
This should be an output that describes the full output, including but not limited to
It should be transformable into a second editor to better support usages like moving changes between commits
We should ensure that we're not going to require a force push for users that care about that don't want that. Perhaps this is done through having a function that takes the rebase output, and performs extra validations, and the output of that function which performs extra validations is what we actually pass to the materializer.
Materialize the rebase
safe_checkoutWhat would the capabilities of a graph editor need to be?
In the examples of the old rebase engine that I went over, which are representative of the other operations mentioned, there are a small set of operations on the linear sets of commits:
It's possible that some of the raw operations that we want to perform, like amending a commit could be done without a full cherry-pick based rebase, but since we'd like to generally abstract rewriting history into one higher-level system, we instead need to be able to rebase an arbitrary commit graph.
Rebasing an arbitrary commit graph
For linear runs of commits - rebasing remains the the old cherry-pick operations. However, we need to consider what happens if we need to replace one or more parents of a merge commit. For a cherry pick, we can only operate with one "base" and one "ours" tree. For these cases we can find the merges of all the bases, and all the ours commit and use the resulting trees for our cherry-pick. This is already implemented and tested in the
but-graph/src/graph-rebase/cherry-pick.rswith hopfully a good amount of tests as examples of the behaviour.This is part 1 of 3 in a stack made with GitButler: