-
Notifications
You must be signed in to change notification settings - Fork 724
Implement reword_commit as an example of rebase engine usage #11452
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: graph-based-rebasing
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
8db0551 to
b6edf2b
Compare
0317278 to
e8cb47a
Compare
b6edf2b to
2d9fbda
Compare
e8cb47a to
885a370
Compare
227d540 to
e17d39c
Compare
885a370 to
d644db3
Compare
e17d39c to
0157fac
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 implements a new reword_commit operation as a demonstration of the new graph-based rebase engine. The implementation provides a simpler, more composable API for rewriting Git history compared to the legacy approach.
Key Changes:
- Introduces a new rebase engine API with an
Editorabstraction that operates on an in-memory repository - Implements the
rewordoperation using this new engine, which rewrites commit messages and rebases dependent commits - Adds feature flag support to allow gradual rollout, with both old and new implementations available
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-workspace/src/commit/reword.rs | Core implementation of the reword operation using the new graph-based rebase engine |
| crates/but-workspace/src/commit/mod.rs | Exports the reword function from the commit module |
| crates/but-workspace/src/lib.rs | Makes the commit module public for external use |
| crates/but-workspace/tests/workspace/commit/reword.rs | Comprehensive tests for rewording head, middle, and base commits |
| crates/but-workspace/tests/workspace/commit/mod.rs | Module declaration for reword tests |
| crates/but-workspace/tests/fixtures/scenario/reword-three-commits.sh | Test fixture script that sets up a three-commit scenario with branches |
| crates/but-rebase/src/graph_rebase/materialize.rs | Adds MaterializeOutcome struct to return commit mapping information |
| crates/but-api/src/commit.rs | API layer providing both oplog-enabled and oplog-disabled versions of reword_commit |
| crates/but-api/src/lib.rs | Adds commit module to the but-api crate |
| crates/gitbutler-tauri/src/main.rs | Registers the new reword_commit Tauri command |
| apps/desktop/src/lib/stacks/stackService.svelte.ts | Frontend integration with feature flag to switch between old and new implementations |
| apps/desktop/src/lib/config/uiFeatureFlags.ts | Adds useNewRebaseEngine feature flag |
| apps/desktop/src/components/profileSettings/ExperimentalSettings.svelte | UI toggle for the new rebase engine feature flag |
| but_workspace::commit::reword(&graph, &repo, commit_id, message.as_bstr()) | ||
| } | ||
|
|
||
| /// Apply `existing_branch` to the workspace in the repository that `ctx` refers to, or create the workspace with default name. |
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.
Copy-paste error in documentation: This docstring describes "Apply existing_branch to the workspace..." which is unrelated to rewording commits. This appears to be copied from another function and not updated.
The documentation should describe what reword_commit actually does, for example:
/// Rewords a commit message and updates the oplog.
///
/// Returns the ID of the newly renamed commit
| /// Apply `existing_branch` to the workspace in the repository that `ctx` refers to, or create the workspace with default name. | |
| /// Rewords a commit message and updates the oplog. |
| Ok(()) | ||
| } | ||
|
|
||
| /// Set a var ignoring the unsafty |
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.
Spelling error: "unsafty" should be "unsafety".
| /// Set a var ignoring the unsafty | |
| /// Set a var ignoring the unsafety |
| reword(&graph, &repo, id.detach(), b"New name".into())?; | ||
|
|
||
| // We end up with two divergent histories here. This is to be expected if we | ||
| // rewrite the very bottom commit in a repoository. |
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.
Spelling error: "repoository" should be "repository".
| // rewrite the very bottom commit in a repoository. | |
| // rewrite the very bottom commit in a repository. |
98565e9 to
a0c54a1
Compare
a0c54a1 to
498b7d4
Compare
|
Thanks for the write up! I plan to give this a proper read through as soon as I am back on Sunday. Just casually reading, completely disregarding the implementation, but just looking at the API as a future user of the editor, a few things stood out to me. Nothing is meant to be prescriptive, just ideas and food for thought.
|
|
Hey! All great feedback.
In the new world, the Graph is kind of the main source of truth, so it felt reasonable to say "take the graph and make it into something we can change". I don't mind the
Yes, something like this: gitbutler/crates/but-workspace/src/commit/insert_blank_commit.rs Lines 40 to 43 in bfdee46
Whether it should be I've already changed it to return an Err if it can't find it (compared to the PR description), so they now have cooresponding
I've thought quite a lot about this. One of the things that you and I talked about with the old rebase engine is we don't want things like re-treeing commits or things like that to be a part of the rebase engine. I agree that that we don't want to conflate the manipulation of commits with the history rewrite itself. That said, one of the pains I perceived when working with the old rebase engine was how it always felt like I needed to figure out "what is the most canonical way to fetch a commit for editing & then re-commit it". That is why I added these methods for reading a commit into an editable form & writing them to the odb again - so there is a clear standard way of doing it. They don't change the rebase steps in any way. It's also still up to the user of the Editor to actually manipulate things like the commit's trees.
This function behaves as the same The materialise then writes all the commits into the ODB, updates the references, and performs a checkout. The reason it's not Having this two step means you can either do |
In this PR I'm exploring what the first rendition of the new rebase engine might be to look like.
I'm doing this by implementing possibly the simplest operation we've got, and then I'll be scaling it up to some more complex operations, testing as I go along.
I want this to be an open RFC. This is a shed to be biked. A water cooler to be over cooled.
A break down of the
rewordfunction inreword.rsI'm going to interleave code followed by text to explain what each part is doing. If it doesn't make sense, then we probably need to switch up some portion of the interface. Please share!
For the signature of this "plumbing" function, it takes just the graph & repo which are the essentials now.
We don't need to take in any references or stack_ids, just getting the
targetcommit is enough to identify what we want to change.I can build the editor from the graph. It takes in a repo which it actually clones and turns into an in-memory repository. For any commits we rewrite in our business logic we want to use this. I've experimented a little bit with adding some functions to make the commit rewrites we want to do a little bit easier & handle things like signing correctly.
This line gets a reference or "selector" to where the target commit sits inside the editors internal graph structure.
Currently for a selector, we can either replace the thing it points to with another "step" kind, or we can insert a new "step" either below or above it.
I'm strongly considering changing
select_xxxto return a result & then havetry_select_xxxversions if the caller really wants to haveOptionsThe three lines here are concerned actually performing the manipulation where we change the commit's message.
In our app there a handful of ways of getting a commit object we can play around with and how to write a commit, with or without signing.
I really want there to be a single common way we do this with the rebase engine, so I added
find_commitandwrite_commit.find_commitis important to use anyway because if we're looking up a commit, then it should be from inside the editor's in memory repository in case anything relevant has been written there doing a 2 step rebase (example coming soon...). I considered having this take a selector or having this actually return the selector, but it seemed initially like more faff than benefit.Find commit is returning a
but_core::Commitwhich seems like an easy thing to modify, and has some useful methods around our specific headers so it seemed like a good type to be the mutable "thing" we edit.write_committhen takes the commit and writes it into the editor's in-memory repo. It takes aDateModewhich enables us to describe if and how to update committers and authors. I liked this because it means we don't have do the faff of getting the signature and then creating a timestamp in this function's body.This replace call doesn't perform any rebases itself. You can call
replaceandinsertand it's just manipulating an in-memory data structure, without any expensive operations.editor.rebase()is performing the actual rebase - without checking out any of the results. On a success it's giving us thisoutcomeobject which on it's own is opaque for now. Currently the only thing you can do with it is call.materialize()which actually checks out any changes. As use cases come up methods can be added to it which can describe the effects of the rebase. IE: did it cause any commits to become conflicted, ect... . Further, you will be able to create a new editor out of it for 2 step rebases.On a failure, this is currently returning an
anyhowerror, but I'm strongly considering doing something withthiserrorso error variants can be more easily matched over with detail..rebase()should only succeed if it the outcome can be checked out following the users preferences. Rules like disallowing pushed commits to be rebased should result in a failure variant..materialize()actually updates the references, any relevant metadata, and performs a safe checkout for us.The function in full:
The old world version of this implementation.
There are a few cases that currently aren't covered in the rebase engine, but by the time I'm done they should be functionally identical.
This is part 2 of 3 in a stack made with GitButler: