Rebase replace tests

Author: Caleb-T-Owens

crates/but-rebase/src/graph_rebase/creation.rs

@@ -9,12 +9,12 @@ use crate::graph_rebase::{Edge, Editor, Step, StepGraph, StepGraphIndex};
 /// Provides an extension for creating an Editor out of the segment graph
 pub trait GraphExt {
     /// Creates an editor.
-    fn create_editor(&self) -> Result<Editor>;
+    fn to_editor(&self) -> Result<Editor>;
 }
 
 impl GraphExt for Graph {
     /// Creates an editor out of the segment graph.
-    fn create_editor(&self) -> Result<Editor> {
+    fn to_editor(&self) -> Result<Editor> {
         // TODO(CTO): Look into traversing "in workspace" segments that are not reachable from the entrypoint
         // TODO(CTO): Look into stopping at the common base
         let entrypoint = self.lookup_entrypoint()?;

crates/but-rebase/src/graph_rebase/materialize.rs

@@ -8,8 +8,8 @@ use but_core::worktree::{
 
 impl SuccessfulRebase {
     /// Materializes a history rewrite
-    pub fn materialize(&self, repo: &gix::Repository) -> Result<()> {
-        for checkout in &self.checkouts {
+    pub fn materialize(self, repo: &gix::Repository) -> Result<()> {
+        for checkout in self.checkouts {
             // TODO(CTO): improve safe_checkout to allow for speculation
             safe_checkout(
                 checkout.old_head_id,

crates/but-rebase/src/graph_rebase/mod.rs

@@ -4,8 +4,6 @@
 //! one mess of git2 code to bring them all,
 //! and in the darknes bind them.
 
-use petgraph::graph::NodeIndex;
-
 mod creation;
 pub mod rebase;
 pub use creation::GraphExt;

crates/but-rebase/src/graph_rebase/rebase.rs

@@ -43,6 +43,8 @@ pub struct SuccessfulRebase {
 
 /// Represents the rebase output and the varying degrees of success it had.
 #[derive(Debug, Clone)]
+#[expect(clippy::large_enum_variant)]
+#[must_use = "Rebase does nothing to affect the commit graph unless materialized"]
 pub enum RebaseOutcome {
     /// The rebase
     Success(SuccessfulRebase),
@@ -51,15 +53,11 @@ pub enum RebaseOutcome {
     ///
     /// Holds the gix::ObjectId of the commit it failed to pick
     MergePickFailed(gix::ObjectId),
-    /// Symbolic reference was encountered. We should never be given these, and
-    /// the sematics of how to work with them are currently unclear, so the
-    /// rebase will be rejected if one is encountered.
-    SymbolicRefEncountered(gix::refs::FullName),
 }
 
 impl Editor {
     /// Perform the rebase
-    pub fn rebase(&self, repo: &gix::Repository) -> Result<RebaseOutcome> {
+    pub fn rebase(self, repo: &gix::Repository) -> Result<RebaseOutcome> {
         // First we want to get a list of nodes that can be reached by
         // traversing downwards from the heads that we care about.
         // Usually there would be just one "head" which is an index to access
@@ -166,7 +164,7 @@ impl Editor {
                                 }
                             }
                             gix::refs::TargetRef::Symbolic(name) => {
-                                return Ok(RebaseOutcome::SymbolicRefEncountered(name.to_owned()));
+                                bail!("Attempted to update the symbolic reference {}", name);
                             }
                         }
                     } else {

crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs

@@ -3,13 +3,7 @@ use but_rebase::graph_rebase::cherry_pick::{CherryPickOutcome, cherry_pick};
 use but_testsupport::visualize_tree;
 use gix::prelude::ObjectIdExt;
 
-use crate::utils::fixture_writable;
-
-fn set_var(key: &str, value: &str) {
-    unsafe {
-        std::env::set_var(key, value);
-    }
-}
+use crate::{graph_rebase::set_var, utils::fixture_writable};
 
 fn get_parents(id: &gix::Id) -> Result<Vec<gix::ObjectId>> {
     Ok(id

crates/but-rebase/tests/rebase/graph_rebase/editor_creation.rs

@@ -18,7 +18,7 @@ fn four_commits() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
 
     insta::assert_snapshot!(editor.steps_dot(), @r#"
     digraph {
@@ -53,7 +53,7 @@ fn merge_in_the_middle() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
 
     insta::assert_snapshot!(editor.steps_dot(), @r#"
     digraph {
@@ -103,7 +103,7 @@ fn three_branches_merged() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
 
     insta::assert_snapshot!(editor.steps_dot(), @r#"
     digraph {
@@ -161,7 +161,7 @@ fn many_references() -> Result<()> {
         └── ·35b8235 (⌂|1)
     ");
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
 
     insta::assert_snapshot!(editor.steps_dot(), @r#"
     digraph {

crates/but-rebase/tests/rebase/graph_rebase/mod.rs

@@ -1,3 +1,10 @@
 mod cherry_pick;
 mod editor_creation;
 mod rebase_identities;
+mod replace;
+
+pub fn set_var(key: &str, value: &str) {
+    unsafe {
+        std::env::set_var(key, value);
+    }
+}

crates/but-rebase/tests/rebase/graph_rebase/rebase_identities.rs

@@ -21,7 +21,7 @@ fn four_commits() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
     let outcome = editor.rebase(&repo)?;
     let RebaseOutcome::Success(outcome) = outcome else {
         bail!("Rebase failed");
@@ -50,7 +50,7 @@ fn merge_in_the_middle() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
     let outcome = editor.rebase(&repo)?;
     let RebaseOutcome::Success(outcome) = outcome else {
         bail!("Rebase failed");
@@ -83,7 +83,7 @@ fn three_branches_merged() -> Result<()> {
 
     let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
 
-    let editor = graph.create_editor()?;
+    let editor = graph.to_editor()?;
     let outcome = editor.rebase(&repo)?;
     let RebaseOutcome::Success(outcome) = outcome else {
         bail!("Rebase failed");

crates/but-rebase/tests/rebase/graph_rebase/replace.rs

@@ -0,0 +1,166 @@
+//! These tests exercise the replace operation.
+use anyhow::{Context, Result, bail};
+use but_graph::Graph;
+use but_rebase::graph_rebase::{GraphExt, Step, rebase::RebaseOutcome};
+use but_testsupport::{git_status, visualize_commit_graph_all, visualize_tree};
+
+use crate::{
+    graph_rebase::set_var,
+    utils::{fixture_writable, standard_options},
+};
+
+#[test]
+fn reword_a_commit() -> Result<()> {
+    set_var("GITBUTLER_CHANGE_ID", "1");
+    let (repo, _tmpdir, meta) = fixture_writable("merge-in-the-middle")?;
+
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * e8ee978 (HEAD -> with-inner-merge) on top of inner merge
+    *   2fc288c Merge branch 'B' into with-inner-merge
+    |\  
+    | * 984fd1c (B) C: new file with 10 lines
+    * | add59d2 (A) A: 10 lines on top
+    |/  
+    * 8f0d338 (tag: base, main) base
+    ");
+    insta::assert_snapshot!(git_status(&repo)?, @r"
+    ");
+
+    let head_tree = repo.head_tree()?.id;
+
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+
+    let mut editor = graph.to_editor()?;
+
+    // get the origional a
+    let a = repo.rev_parse_single("A")?.detach();
+
+    // reword commit a
+    let a_obj = repo.find_commit(a)?;
+    let mut a_obj = a_obj.decode()?;
+    a_obj.message = "A: a second coming".into();
+    let a_new = repo.write_object(a_obj)?.detach();
+
+    // select the origional a out of the graph
+    let a_selector = editor
+        .select_commit(a)
+        .context("Failed to find commit a in editor graph")?;
+    // replace it with the new one
+    editor.replace(&a_selector, Step::Pick { id: a_new });
+
+    let outcome = editor.rebase(&repo)?;
+    let RebaseOutcome::Success(outcome) = outcome else {
+        bail!("Rebase failed");
+    };
+    outcome.materialize(&repo)?;
+
+    assert_eq!(head_tree, repo.head_tree()?.id);
+
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * 4894b95 (HEAD -> with-inner-merge) on top of inner merge
+    *   af38519 Merge branch 'B' into with-inner-merge
+    |\  
+    | * 984fd1c (B) C: new file with 10 lines
+    * | 6de6b92 (A) A: a second coming
+    |/  
+    * 8f0d338 (tag: base, main) base
+    ");
+    insta::assert_snapshot!(git_status(&repo)?, @r"
+    ");
+
+    Ok(())
+}
+
+#[test]
+fn ammend_a_commit() -> Result<()> {
+    set_var("GITBUTLER_CHANGE_ID", "1");
+    let (repo, _tmpdir, meta) = fixture_writable("merge-in-the-middle")?;
+
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * e8ee978 (HEAD -> with-inner-merge) on top of inner merge
+    *   2fc288c Merge branch 'B' into with-inner-merge
+    |\  
+    | * 984fd1c (B) C: new file with 10 lines
+    * | add59d2 (A) A: 10 lines on top
+    |/  
+    * 8f0d338 (tag: base, main) base
+    ");
+    insta::assert_snapshot!(git_status(&repo)?, @r"
+    ");
+
+    let head_tree = repo.head_tree()?.id();
+    insta::assert_snapshot!(visualize_tree(head_tree), @r#"
+    f766d1f
+    ├── added-after-with-inner-merge:100644:861be1b "seq 10\n"
+    ├── file:100644:d78dd4f "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n"
+    └── new-file:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n"
+    "#);
+
+    let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?;
+
+    let mut editor = graph.to_editor()?;
+
+    // get the origional a
+    let a = repo.rev_parse_single("A")?;
+    insta::assert_snapshot!(visualize_tree(a), @r#"
+    0cc630c
+    └── file:100644:d78dd4f "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n"
+    "#);
+
+    // reword commit a
+    let mut a_obj = but_core::Commit::from_id(a)?;
+
+    let mut builder = repo.edit_tree(a_obj.tree)?;
+    let new_blob = repo.write_blob("I'm a new file :D\n")?;
+    builder.upsert("new-file.txt", gix::objs::tree::EntryKind::Blob, new_blob)?;
+    let tree = builder.write()?;
+
+    a_obj.tree = tree.detach();
+    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
+    let a_selector = editor
+        .select_commit(a.detach())
+        .context("Failed to find commit a in editor graph")?;
+    // replace it with the new one
+    editor.replace(&a_selector, Step::Pick { id: a_new });
+
+    let outcome = editor.rebase(&repo)?;
+    let RebaseOutcome::Success(outcome) = outcome else {
+        bail!("Rebase failed");
+    };
+    outcome.materialize(&repo)?;
+
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * 4a8642c (HEAD -> with-inner-merge) on top of inner merge
+    *   fa3d6b8 Merge branch 'B' into with-inner-merge
+    |\  
+    | * 984fd1c (B) C: new file with 10 lines
+    * | f1905a8 (A) A: a second coming
+    |/  
+    * 8f0d338 (tag: base, main) base
+    ");
+    insta::assert_snapshot!(git_status(&repo)?, @r"
+    ");
+
+    // A should include our extra blob
+    let a = repo.rev_parse_single("A")?;
+    insta::assert_snapshot!(visualize_tree(a), @r#"
+    0c482d4
+    ├── file:100644:d78dd4f "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n"
+    └── new-file.txt:100644:715faaf "I\'m a new file :D\n"
+    "#);
+
+    // New head tree should also include our extra blob
+    let new_head_tree = repo.head_tree()?.id();
+    insta::assert_snapshot!(visualize_tree(new_head_tree), @r#"
+    89042ca
+    ├── added-after-with-inner-merge:100644:861be1b "seq 10\n"
+    ├── file:100644:d78dd4f "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n"
+    ├── new-file:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n"
+    └── new-file.txt:100644:715faaf "I\'m a new file :D\n"
+    "#);
+
+    Ok(())
+}