Don't use an outcome status for now

Author: Caleb-T-Owens

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

@@ -1,5 +1,6 @@
 //! Operations for mutation the editor
 
+use anyhow::{Result, anyhow};
 use petgraph::{Direction, visit::EdgeRef};
 
 use crate::graph_rebase::{Edge, Editor, Selector, Step};
@@ -18,7 +19,25 @@ pub enum InsertSide {
 /// Operations for mutating the commit graph
 impl Editor {
     /// Get a selector to a particular commit in the graph
-    pub fn select_commit(&self, target: gix::ObjectId) -> Option<Selector> {
+    pub fn select_commit(&self, target: gix::ObjectId) -> Result<Selector> {
+        match self.try_select_commit(target) {
+            Some(selector) => Ok(selector),
+            None => Err(anyhow!("Failed to find commit {target} in rebase editor")),
+        }
+    }
+
+    /// Get a selector to a particular reference in the graph
+    pub fn select_reference(&self, target: &gix::refs::FullNameRef) -> Result<Selector> {
+        match self.try_select_reference(target) {
+            Some(selector) => Ok(selector),
+            None => Err(anyhow!(
+                "Failed to find reference {target} in rebase editor"
+            )),
+        }
+    }
+
+    /// Get a selector to a particular commit in the graph
+    pub fn try_select_commit(&self, target: gix::ObjectId) -> Option<Selector> {
         for node_idx in self.graph.node_indices() {
             if let Step::Pick { id, .. } = self.graph[node_idx]
                 && id == target
@@ -31,7 +50,7 @@ impl Editor {
     }
 
     /// Get a selector to a particular reference in the graph
-    pub fn select_reference(&self, target: &gix::refs::FullNameRef) -> Option<Selector> {
+    pub fn try_select_reference(&self, target: &gix::refs::FullNameRef) -> Option<Selector> {
         for node_idx in self.graph.node_indices() {
             if let Step::Reference { refname } = &self.graph[node_idx]
                 && target == refname.as_ref()

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

@@ -31,23 +31,9 @@ pub struct SuccessfulRebase {
     pub(crate) checkouts: Vec<Checkouts>,
 }
 
-/// 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),
-    /// The graph rebase failed because we encountered a situation where we
-    /// couldn't merge bases.
-    ///
-    /// Holds the gix::ObjectId of the commit it failed to pick
-    MergePickFailed(gix::ObjectId),
-}
-
 impl Editor {
     /// Perform the rebase
-    pub fn rebase(self) -> Result<RebaseOutcome> {
+    pub fn rebase(self) -> Result<SuccessfulRebase> {
         // 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
@@ -115,7 +101,8 @@ impl Editor {
                         }
                         CherryPickOutcome::FailedToMergeBases => {
                             // Exit early - the rebase failed because it encountered a commit it couldn't pick
-                            return Ok(RebaseOutcome::MergePickFailed(id));
+                            // TODO(CTO): Detect if this was the merge commit itself & signal that seperatly
+                            bail!("Failed to merge bases for commit {id}");
                         }
                     }
                 }
@@ -214,14 +201,14 @@ impl Editor {
             }
         }
 
-        Ok(RebaseOutcome::Success(SuccessfulRebase {
+        Ok(SuccessfulRebase {
             repo: self.repo,
             ref_edits,
             commit_mapping,
             graph_mapping,
             graph: output_graph,
             checkouts: self.checkouts.to_owned(),
-        }))
+        })
     }
 }
 

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

@@ -1,7 +1,7 @@
 //! These tests exercise the insert operation.
-use anyhow::{Context, Result, bail};
+use anyhow::{Context, Result};
 use but_graph::Graph;
-use but_rebase::graph_rebase::{GraphExt, Step, mutate::InsertSide, rebase::RebaseOutcome};
+use but_rebase::graph_rebase::{GraphExt, Step, mutate::InsertSide};
 use but_testsupport::{git_status, visualize_commit_graph_all};
 
 use crate::{
@@ -54,9 +54,6 @@ fn insert_below_merge_commit() -> Result<()> {
     );
 
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
@@ -120,9 +117,6 @@ fn insert_above_commit_with_two_children() -> Result<()> {
     );
 
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
@@ -140,3 +134,9 @@ fn insert_above_commit_with_two_children() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+#[ignore]
+fn inserts_violating_fp_protection_should_cause_rebase_failure() -> Result<()> {
+    panic!("Branch protection hasn't been implemented dyet");
+}

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

@@ -1,6 +1,7 @@
 mod cherry_pick;
 mod editor_creation;
 mod insert;
+mod multiple_operations;
 mod rebase_identities;
 mod replace;
 

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

@@ -0,0 +1,8 @@
+//! These tests exercise the replace operation.
+use anyhow::Result;
+
+#[test]
+#[ignore = "non-legacy reordering is not implemented, and should be considered a requirement for any operations that want to work in SBM"]
+fn reorder_two_references() -> Result<()> {
+    panic!("Reference orders are not updated");
+}

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

@@ -1,8 +1,8 @@
 /// These tests demonstrate that if none of the steps are changed, the same
 /// graphs are returned.
-use anyhow::{Result, bail};
+use anyhow::Result;
 use but_graph::Graph;
-use but_rebase::graph_rebase::{GraphExt, rebase::RebaseOutcome};
+use but_rebase::graph_rebase::GraphExt;
 use but_testsupport::{graph_workspace, visualize_commit_graph_all};
 
 use crate::utils::{fixture_writable, standard_options};
@@ -23,9 +23,6 @@ fn four_commits() -> Result<()> {
 
     let editor = graph.to_editor(&repo)?;
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     assert_eq!(visualize_commit_graph_all(&repo)?, before);
@@ -59,9 +56,6 @@ fn four_commits_with_short_traversal() -> Result<()> {
 
     let editor = graph.to_editor(&repo)?;
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     assert_eq!(visualize_commit_graph_all(&repo)?, before);
@@ -88,9 +82,6 @@ fn merge_in_the_middle() -> Result<()> {
 
     let editor = graph.to_editor(&repo)?;
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     assert_eq!(visualize_commit_graph_all(&repo)?, before);
@@ -121,9 +112,6 @@ fn three_branches_merged() -> Result<()> {
 
     let editor = graph.to_editor(&repo)?;
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     assert_eq!(visualize_commit_graph_all(&repo)?, before);

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

@@ -1,7 +1,7 @@
 //! These tests exercise the replace operation.
-use anyhow::{Context, Result, bail};
+use anyhow::{Context, Result};
 use but_graph::Graph;
-use but_rebase::graph_rebase::{GraphExt, Step, rebase::RebaseOutcome};
+use but_rebase::graph_rebase::{GraphExt, Step};
 use but_testsupport::{git_status, visualize_commit_graph_all, visualize_tree};
 
 use crate::{
@@ -55,9 +55,6 @@ fn reword_a_commit() -> Result<()> {
     );
 
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     assert_eq!(head_tree, repo.head_tree()?.id);
@@ -139,9 +136,6 @@ fn ammend_a_commit() -> Result<()> {
     );
 
     let outcome = editor.rebase()?;
-    let RebaseOutcome::Success(outcome) = outcome else {
-        bail!("Rebase failed");
-    };
     outcome.materialize()?;
 
     insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
@@ -176,3 +170,9 @@ fn ammend_a_commit() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+#[ignore]
+fn replaces_violating_fp_protection_should_cause_rebase_failure() -> Result<()> {
+    panic!("Branch protection hasn't been implemented dyet");
+}