Remove Merge Step in but-rebase to remove gitbutler-repo dependency.

There is no use of it yet and it doesn't look like it's needed
since `Pick` can re-merge merge-commits.

With the dependency gone, the new `but-rebase::cherry_pick` can be used
in `gitbutler-repo` to assure all tests still work.

From there certain crucial tests can be ported to `but-rebase` as
cherry-pick tests.

Author: Byron

Cargo.lock

@@ -13,11 +13,9 @@ gix = { workspace = true, features = ["revision", "merge"]}
 anyhow.workspace = true
 tracing.workspace = true
 but-core.workspace = true
-gitbutler-repo.workspace = true
 gitbutler-oxidize.workspace = true
 gitbutler-error.workspace = true
 bstr.workspace = true
-git2.workspace = true
 tempfile.workspace = true
 serde = { version = "1.0.217", features = ["derive"] }
 toml.workspace = true
@@ -26,6 +24,3 @@ toml.workspace = true
 but-testsupport.workspace = true
 insta = "1.42.1"
 but-core = { workspace = true, features = ["testing"] }
-# for stable hashes in `gitbuter-` crates while we use them.
-# TODO: remove once `gitbutler-repo` isn't needed anymore.
-gitbutler-commit = { workspace = true, features = ["testing"] }

crates/but-rebase/Cargo.toml

@@ -33,39 +33,13 @@ pub(crate) mod function {
     use serde::Serialize;
     use std::collections::HashSet;
     use std::path::PathBuf;
-    use tracing::instrument;
 
-    /// Place `commits_to_rebase` onto `base` in-order, i.e. `base -> 0 -> 1 -> N`, so that that last
-    /// commit in `commits_to_rebase` is the last commit to rebase.
-    /// If `commits_to_rebase` is empty, `base` is returned unaltered.
+    /// Place `commit_to_rebase` onto `base`.
     ///
     /// `pick_mode` and `empty_commit` control how to deal with no-ops and epty commits.
+    /// Returns the id of the cherry-picked commit.
     ///
-    /// Returns the id of the top-most, rebased commit.
-    ///
-    /// Note that each rewritten commit will have headers injected, among which is a change id.
-    #[instrument(level = tracing::Level::DEBUG, skip(repo, commits_to_rebase))]
-    pub fn cherry_pick_many(
-        repo: &gix::Repository,
-        base: gix::ObjectId,
-        commits_to_rebase: impl IntoIterator<Item = gix::ObjectId>,
-        pick_mode: PickMode,
-        empty_commit: EmptyCommit,
-    ) -> anyhow::Result<gix::ObjectId> {
-        let mut cursor = but_core::Commit::from_id(base.attach(repo))?;
-        for to_rebase_id in commits_to_rebase {
-            let to_rebase = but_core::Commit::from_id(to_rebase_id.attach(repo))?;
-            cursor = but_core::Commit::from_id(cherry_pick_one_inner(
-                cursor,
-                to_rebase,
-                pick_mode,
-                empty_commit,
-            )?)?;
-        }
-        Ok(cursor.id.detach())
-    }
-
-    /// Like [`cherry_pick_many()`], but only handles a single `commit_to_rebase` onto `base`.
+    /// Note that the rewritten commit will have headers injected, among which is a change id.
     pub fn cherry_pick_one(
         repo: &gix::Repository,
         base: gix::ObjectId,

crates/but-rebase/src/cherry_pick.rs

@@ -5,14 +5,15 @@
 
 use crate::commit::CommitterMode;
 use anyhow::{anyhow, bail, Context, Ok, Result};
-use bstr::{BString, ByteSlice};
+use bstr::BString;
 use gix::objs::Exists;
 use gix::prelude::ObjectIdExt;
+use tracing::instrument;
 
 /// Types for use with cherry-picking
 pub mod cherry_pick;
 use crate::cherry_pick::{EmptyCommit, PickMode};
-pub use cherry_pick::function::{cherry_pick_many, cherry_pick_one};
+pub use cherry_pick::function::cherry_pick_one;
 
 /// Utilities to create commits (and deal with signing)
 pub mod commit;
@@ -32,14 +33,6 @@ pub enum RebaseStep {
         //       for multi-branch rebasing. It would keep the previous cursor, to allow the last
         //       branch to contain a pick of the merge commit on top, which it can then correctly re-merge.
     },
-    /// Merge an existing commit and it's parents producing a new merge commit.
-    // TODO: figure out what this is used for, and add tests for it/port it over.
-    Merge {
-        /// Id of an already existing commit
-        commit_id: gix::ObjectId,
-        /// Message to use for newly produced commit
-        new_message: BString,
-    },
     /// Squashes an existing commit into the one in the first `Pick` or `Merge` RebaseStep that precedes it.
     ///
     /// If there are neither `Pick` nor `Merge` steps preceding this operation (e.g. only `Reference` steps), the execution will halt with an error.
@@ -63,7 +56,6 @@ impl RebaseStep {
     fn commit_id(&self) -> Option<&gix::oid> {
         match self {
             RebaseStep::Pick { commit_id, .. }
-            | RebaseStep::Merge { commit_id, .. }
             | RebaseStep::SquashIntoPreceding { commit_id, .. } => Some(commit_id),
             RebaseStep::Reference { .. } => None,
         }
@@ -145,12 +137,17 @@ impl<'repo> Rebase<'repo> {
         if self.steps.is_empty() {
             return Err(anyhow!("No rebase steps provided"));
         }
+        let pick_mode = if self.rebase_noops {
+            PickMode::Unconditionally
+        } else {
+            PickMode::SkipIfNoop
+        };
         rebase(
             self.repo,
             self.base,
             self.base_substitute,
             std::mem::take(&mut self.steps),
-            self.rebase_noops,
+            pick_mode,
         )
     }
 }
@@ -177,12 +174,6 @@ impl Rebase<'_> {
             } => {
                 self.assure_unique_step_and_existing_non_base(commit_id, "Picked")?;
             }
-            RebaseStep::Merge {
-                commit_id,
-                new_message: _,
-            } => {
-                self.assure_unique_step_and_existing_non_base(commit_id, "Merge")?;
-            }
             RebaseStep::SquashIntoPreceding {
                 commit_id,
                 new_message: _,
@@ -222,12 +213,13 @@ impl Rebase<'_> {
     }
 }
 
+#[instrument(level = tracing::Level::DEBUG, skip(repo))]
 fn rebase(
     repo: &gix::Repository,
     base: Option<gix::ObjectId>,
     base_substitute: Option<gix::ObjectId>,
     steps: Vec<RebaseStep>,
-    rebase_noops: bool,
+    pick_mode: PickMode,
 ) -> Result<RebaseOutput> {
     let (mut references, mut commit_mapping) = (
         vec![],
@@ -270,11 +262,6 @@ fn rebase(
                 } else {
                     match &mut cursor {
                         Some(cursor) => {
-                            let pick_mode = if rebase_noops {
-                                PickMode::Unconditionally
-                            } else {
-                                PickMode::SkipIfNoop
-                            };
                             let mut new_commit = cherry_pick_one(
                                 repo,
                                 *cursor,
@@ -302,21 +289,6 @@ fn rebase(
                 }
                 last_seen_commit = Some(commit_id);
             }
-            RebaseStep::Merge {
-                commit_id,
-                new_message,
-            } => {
-                let Some(cursor) = &mut cursor else {
-                    bail!("Can't merge without history present yet");
-                };
-                last_seen_commit = Some(commit_id);
-                *cursor = gitbutler_repo::rebase::merge_commits(
-                    repo,
-                    *cursor,
-                    commit_id,
-                    &new_message.to_str_lossy(),
-                )?;
-            }
             RebaseStep::SquashIntoPreceding {
                 commit_id,
                 new_message,

crates/but-rebase/src/lib.rs

@@ -33,21 +33,6 @@ fn non_existing_commit_in_pick_step() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[test]
-fn non_existing_commit_in_merge_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([RebaseStep::Merge {
-        commit_id: non_existing_commit(),
-        new_message: "merge commit".into(),
-    }]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "An object with id eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee could not be found"
-    );
-    Ok(())
-}
-
 #[test]
 fn non_existing_commit_in_fixup_step() -> anyhow::Result<()> {
     let (repo, commits) = four_commits()?;
@@ -78,21 +63,6 @@ fn using_base_in_pick_step() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[test]
-fn using_base_in_merge_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([RebaseStep::Merge {
-        commit_id: commits.base,
-        new_message: "merge commit".into(),
-    }]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Merge commit cannot be the base commit"
-    );
-    Ok(())
-}
-
 #[test]
 fn using_base_in_fixup_step() -> anyhow::Result<()> {
     let (repo, commits) = four_commits()?;
@@ -129,27 +99,6 @@ fn using_picked_commit_in_a_pick_step() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[test]
-fn using_merged_commit_in_a_pick_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([
-        RebaseStep::Merge {
-            commit_id: commits.a,
-            new_message: "merge commit".into(),
-        },
-        RebaseStep::Pick {
-            commit_id: commits.a,
-            new_message: None,
-        },
-    ]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Picked commit already exists in a previous step"
-    );
-    Ok(())
-}
-
 #[test]
 fn using_fixup_commit_in_a_pick_step() -> anyhow::Result<()> {
     let (repo, commits) = four_commits()?;
@@ -175,73 +124,6 @@ fn using_fixup_commit_in_a_pick_step() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[test]
-fn using_picked_commit_in_a_merge_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([
-        RebaseStep::Pick {
-            commit_id: commits.a,
-            new_message: None,
-        },
-        RebaseStep::Merge {
-            commit_id: commits.a,
-            new_message: "merge commit".into(),
-        },
-    ]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Picked commit already exists in a previous step"
-    );
-    Ok(())
-}
-
-#[test]
-fn using_merged_commit_in_a_merge_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([
-        RebaseStep::Merge {
-            commit_id: commits.a,
-            new_message: "merge commit".into(),
-        },
-        RebaseStep::Merge {
-            commit_id: commits.a,
-            new_message: "merge commit".into(),
-        },
-    ]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Picked commit already exists in a previous step"
-    );
-    Ok(())
-}
-
-#[test]
-fn using_fixup_commit_in_a_merge_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([
-        RebaseStep::Pick {
-            commit_id: commits.a,
-            new_message: None,
-        },
-        RebaseStep::SquashIntoPreceding {
-            commit_id: commits.b,
-            new_message: None,
-        },
-        RebaseStep::Merge {
-            commit_id: commits.b,
-            new_message: "merge commit".into(),
-        },
-    ]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Picked commit already exists in a previous step"
-    );
-    Ok(())
-}
-
 #[test]
 fn using_picked_commit_in_a_fixup_step() -> anyhow::Result<()> {
     let (repo, commits) = four_commits()?;
@@ -263,27 +145,6 @@ fn using_picked_commit_in_a_fixup_step() -> anyhow::Result<()> {
     Ok(())
 }
 
-#[test]
-fn using_merged_commit_in_a_fixup_step() -> anyhow::Result<()> {
-    let (repo, commits) = four_commits()?;
-    let mut builder = Rebase::new(&repo, commits.base, None)?;
-    let result = builder.steps([
-        RebaseStep::Merge {
-            commit_id: commits.a,
-            new_message: "merge commit".into(),
-        },
-        RebaseStep::SquashIntoPreceding {
-            commit_id: commits.a,
-            new_message: None,
-        },
-    ]);
-    assert_eq!(
-        result.unwrap_err().to_string(),
-        "Picked commit already exists in a previous step"
-    );
-    Ok(())
-}
-
 #[test]
 fn using_fixup_commit_in_a_fixup_step() -> anyhow::Result<()> {
     let (repo, commits) = four_commits()?;