Skip to content

Conversation

@Caleb-T-Owens
Copy link
Contributor

@Caleb-T-Owens Caleb-T-Owens commented Dec 4, 2025

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 reword function in reword.rs

I'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!

pub fn reword(
    graph: &but_graph::Graph,
    repo: &gix::Repository,
    target: gix::ObjectId,
    new_message: &BStr,
) -> Result<()> {

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 target commit is enough to identify what we want to change.

    let mut editor = graph.to_editor(repo)?;

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.

    let target_selector = editor
        .select_commit(target)
        .context("Failed to find target commit in editor")?;

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_xxx to return a result & then have try_select_xxx versions if the caller really wants to have Options

    let mut commit = editor.find_commit(target)?;
    commit.message = new_message.to_owned();
    let new_id = editor.write_commit(commit, DateMode::CommitterUpdateAuthorKeep)?;

The 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_commit and write_commit.

find_commit is 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::Commit which 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_commit then takes the commit and writes it into the editor's in-memory repo. It takes a DateMode which 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.

    editor.replace(&target_selector, Step::new_pick(new_id));

This replace call doesn't perform any rebases itself. You can call replace and insert and it's just manipulating an in-memory data structure, without any expensive operations.

    let outcome = editor.rebase()?;

editor.rebase() is performing the actual rebase - without checking out any of the results. On a success it's giving us this outcome object 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 anyhow error, but I'm strongly considering doing something with thiserror so 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.

    outcome.materialize()?;

    Ok(())
}

.materialize() actually updates the references, any relevant metadata, and performs a safe checkout for us.

The function in full:

pub fn reword(
    graph: &but_graph::Graph,
    repo: &gix::Repository,
    target: gix::ObjectId,
    new_message: &BStr,
) -> Result<()> {
    let mut editor = graph.to_editor(repo)?;
    let target_selector = editor
        .select_commit(target)
        .context("Failed to find target commit in editor")?;

    let mut commit = editor.find_commit(target)?;
    commit.message = new_message.to_owned();
    let new_id = editor.write_commit(commit, DateMode::CommitterUpdateAuthorKeep)?;

    editor.replace(&target_selector, Step::new_pick(new_id));

    let outcome = editor.rebase()?;
    outcome.materialize()?;

    Ok(())
}

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.

// changes a commit message for commit_oid, rebases everything above it, updates branch head if successful
pub(crate) fn update_commit_message(
    ctx: &Context,
    stack_id: StackId,
    commit_id: git2::Oid,
    message: &str,
) -> Result<git2::Oid> {
    if message.is_empty() {
        bail!("commit message can not be empty");
    }
    let vb_state = ctx.legacy_project.virtual_branches();
    let default_target = vb_state.get_default_target()?;
    let gix_repo = ctx.repo.get()?;

    let mut stack = vb_state.get_stack_in_workspace(stack_id)?;
    let branch_commit_oids = ctx.git2_repo.get()?.l(
        stack.head_oid(&gix_repo)?.to_git2(),
        LogUntil::Commit(default_target.sha),
        false,
    )?;

    if !branch_commit_oids.contains(&commit_id) {
        bail!("commit {commit_id} not in the branch");
    }

    let pushed_commit_oids = stack.upstream_head.map_or_else(
        || Ok(vec![]),
        |upstream_head| {
            ctx.git2_repo
                .get()?
                .l(upstream_head, LogUntil::Commit(default_target.sha), false)
        },
    )?;

    if pushed_commit_oids.contains(&commit_id) && !stack.allow_rebasing {
        // updating the message of a pushed commit will cause a force push that is not allowed
        bail!("force push not allowed");
    }

    let mut steps = stack.as_rebase_steps(ctx, &gix_repo)?;
    // Update the commit message
    for step in steps.iter_mut() {
        if let RebaseStep::Pick {
            commit_id: id,
            new_message,
        } = step
            && *id == commit_id.to_gix()
        {
            *new_message = Some(message.into());
        }
    }
    let merge_base = stack.merge_base(ctx)?;
    let mut rebase = but_rebase::Rebase::new(&gix_repo, Some(merge_base), None)?;
    rebase.rebase_noops(false);
    rebase.steps(steps)?;
    let output = rebase.rebase()?;

    let new_head = output.top_commit.to_git2();
    stack.set_stack_head(&vb_state, &gix_repo, new_head, None)?;
    stack.set_heads_from_rebase_output(ctx, output.references)?;

    crate::integration::update_workspace_commit(&vb_state, ctx, false)
        .context("failed to update gitbutler workspace")?;

    output
        .commit_mapping
        .iter()
        .find_map(|(_base, old, new)| (*old == commit_id.to_gix()).then_some(new.to_git2()))
        .ok_or(anyhow!(
            "Failed to find the updated commit id after rebasing"
        ))
}

This is part 2 of 3 in a stack made with GitButler:

@vercel
Copy link

vercel bot commented Dec 4, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
gitbutler-web Ignored Ignored Preview Dec 5, 2025 4:12pm

@Caleb-T-Owens Caleb-T-Owens mentioned this pull request Dec 4, 2025
@github-actions github-actions bot added rust Pull requests that update Rust code @gitbutler/desktop labels Dec 4, 2025
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch from 8db0551 to b6edf2b Compare December 4, 2025 13:35
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch from b6edf2b to 2d9fbda Compare December 4, 2025 13:53
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch 2 times, most recently from 227d540 to e17d39c Compare December 4, 2025 14:46
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch from e17d39c to 0157fac Compare December 5, 2025 13:06
@Byron Byron requested a review from Copilot December 5, 2025 14:47
Copilot finished reviewing on behalf of Byron December 5, 2025 14:51
Copy link

Copilot AI left a 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 Editor abstraction that operates on an in-memory repository
  • Implements the reword operation 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.
Copy link

Copilot AI Dec 5, 2025

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
Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
Ok(())
}

/// Set a var ignoring the unsafty
Copy link

Copilot AI Dec 5, 2025

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".

Suggested change
/// Set a var ignoring the unsafty
/// Set a var ignoring the unsafety

Copilot uses AI. Check for mistakes.
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.
Copy link

Copilot AI Dec 5, 2025

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".

Suggested change
// rewrite the very bottom commit in a repoository.
// rewrite the very bottom commit in a repository.

Copilot uses AI. Check for mistakes.
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch 2 times, most recently from 98565e9 to a0c54a1 Compare December 5, 2025 16:07
@Caleb-T-Owens Caleb-T-Owens force-pushed the reimplement-commit-reword branch from a0c54a1 to 498b7d4 Compare December 5, 2025 16:12
@krlvi
Copy link
Member

krlvi commented Dec 5, 2025

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.

graph.to_editor(repo)
Totally random: would it not make sense to express this more like Editor::create(graph, repo)? In theory this could also have been implemented on the repo, but we choose the graph as the 'main' object. But I wonder why?

editor.select_commit(target)
Would we be able to select references as well in the future? I wonder if it would make sense to have an editor.select which takes a typed input instead

editor.find_commit and editor.write_commit
For some reason, semantically it feels weird that this is the editor's responsibility. Would it make sense to either make the special in-memory repo that the editor uses available either when it's constructed or as a method?

editor.rebase()
I am wondering if "rebase" is the right top level verb here. Because how would this look like for reference modification type of operations?

outcome.materialize()
Another thought - why have two steps editor.rebase() which gives an outcome and then outcome.materialize().
What if it was just one function which takes a "dry run" boolean as an argument? This way, a user can run it with dry run, and if they are happy with the result, they run it a second time for real. Does this make sense?
My main thinking here is how to simplify the API

@Caleb-T-Owens
Copy link
Contributor Author

Hey! All great feedback.

Totally random: would it not make sense to express this more like Editor::create(graph, repo)? In theory this could also have been implemented on the repo, but we choose the graph as the 'main' object. But I wonder why?

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 Editor::create constructor at all, and I was already considering something like that since I might need a third argument for some project settings in some form.

editor.select_commit(target)

Yes, something like this:

let target_selector = match relative_to {
RelativeTo::Commit(id) => editor.select_commit(id)?,
RelativeTo::Reference(r) => editor.select_reference(r)?,
};
.

Whether it should be select(SelectionTarget::Commit(id)) or select_commit(id) is a matter of taste. I'm happy to change this up.

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 try_ variants as well.

editor.find_commit and editor.write_commit
For some reason, semantically it feels weird that this is the editor's responsibility. Would it make sense to either make the special in-memory repo that the editor uses available either when it's constructed or as a method?

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.

I am wondering if "rebase" is the right top level verb here. Because how would this look like for reference modification type of operations?

This function behaves as the same rebase() function of the old rebase engine. It does all the work, and in its output it has a list of all the references that need to be updated, added, or removed.

The materialise then writes all the commits into the ODB, updates the references, and performs a checkout.

The reason it's not rebase_and_materialize() (just for a descriptive name), is for operations like squashing commits and moving changes between commits. For these two operations in their existing implementations, they perform some manipulations of commits, then ask the rebase engine to do a rebase, and then they use that to change the steps some more and perform a final rebase & materialise the output.

Having this two step means you can either do .rebase().gimme_nother_editor() or .rebase().materialize() depending on the scenario.

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

Labels

@gitbutler/desktop rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants