Merge pull request #12797 from gitbutlerapp/docs-for-move-mutations

estib-vega · web-flow · commit d4dc9cb143a5 · 2026-03-12T10:26:20.000+01:00
docs: Add some more docs for move commit and branch
diff --git a/crates/but-api/src/branch.rs b/crates/but-api/src/branch.rs
@@ -177,7 +177,7 @@ fn move_branch_impl(
     let (_guard, repo, mut workspace, _) = ctx.workspace_mut_and_db()?;
     let editor = workspace.graph.to_editor(&repo)?;
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
-        but_workspace::branch::move_branch(&workspace, editor, subject_branch, target_branch)?;
+        but_workspace::branch::move_branch(editor, &workspace, subject_branch, target_branch)?;
 
     let materialization = rebase.materialize()?;
     if let Some((ws_meta, ref_name)) = ws_meta.zip(workspace.ref_name()) {
diff --git a/crates/but-api/src/commit.rs b/crates/but-api/src/commit.rs
@@ -657,7 +657,7 @@ pub fn commit_move_only(
     let anchor_selector = editor.select_commit(anchor_commit_id)?;
 
     let rebase =
-        but_workspace::commit::move_commit(&ws, editor, subject_commit_id, anchor_selector, side)?;
+        but_workspace::commit::move_commit(editor, &ws, subject_commit_id, anchor_selector, side)?;
 
     let materialized = rebase.materialize()?;
     ws.refresh_from_head(&repo, &meta)?;
@@ -706,8 +706,8 @@ pub fn commit_move_to_branch_only(
     let anchor_selector = editor.select_reference(anchor_ref)?;
 
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         subject_commit_id,
         anchor_selector,
         InsertSide::Below,
diff --git a/crates/but-workspace/src/branch/move_branch.rs b/crates/but-workspace/src/branch/move_branch.rs
@@ -30,6 +30,12 @@ pub(super) mod function {
 
     /// Move a branch between stacks in the `workspace`.
     ///
+    /// `editor` is assumed to have been generated from the given `workspace`
+    /// and therefore aligned.
+    ///
+    /// `workspace` - Used for getting the surrounding context of the commit being moved.
+    ///     In the future, we should not rely on the projection and do it fully on the graph.
+    ///
     /// `subject_branch_name` is the full reference name of the branch to move.
     ///
     /// `target_branch_name` is the full reference name of the branch to move the subject
@@ -38,23 +44,13 @@ pub(super) mod function {
     /// Returns:
     /// - Successful rebase
     pub fn move_branch(
-        workspace: &but_graph::projection::Workspace,
         mut editor: Editor,
+        workspace: &but_graph::projection::Workspace,
         subject_branch_name: &FullNameRef,
         target_branch_name: &FullNameRef,
     ) -> anyhow::Result<Outcome> {
-        let Some(source) = workspace.find_segment_and_stack_by_refname(subject_branch_name) else {
-            bail!(
-                "Couldn't find branch to move in workspace with reference name: {subject_branch_name}"
-            );
-        };
-
-        let Some(destination) = workspace.find_segment_and_stack_by_refname(target_branch_name)
-        else {
-            bail!(
-                "Couldn't find target branch to move in workspace with reference name: {target_branch_name}"
-            );
-        };
+        let (source, destination) =
+            retrieve_branches_and_containers(workspace, subject_branch_name, target_branch_name)?;
 
         let Some(workspace_head) = workspace.tip_commit().map(|commit| commit.id) else {
             bail!("Couldn't find workspace head.")
@@ -123,6 +119,56 @@ pub(super) mod function {
             ws_meta,
         })
     }
+
+    /// A segment and its container stack.
+    type WorkspaceSegmentContext<'a> = (
+        &'a but_graph::projection::Stack,
+        &'a but_graph::projection::StackSegment,
+    );
+
+    /// Determine the surrounding context of the subject and target branches.
+    ///
+    /// Currently, this looks into the workspace projection in order to determine **where to take the branch from and to**.
+    ///
+    /// ### The issue
+    /// It's impossible to know for sure what is the exact intention of 'moving a branch' inside a complex git graph.
+    /// Any commit, can have N children and M parents. 'Moving' it somewhere else can imply:
+    /// - Disconnecting all parents and children, and inserting it somewhere else.
+    /// - Disconnecting the first parent and all children, and then inserting.
+    /// - Disconnecting *some* parents and *some* children, and then inserting it.
+    ///
+    /// This condition holds for every commit in a branch.
+    ///
+    /// ### The GitButler assumption
+    /// In the context of a GitButler workspace (as of this writing), we want to disconnect the branch (segment) from
+    /// the stack, and insert it on top of another. In graph terms, this means that we:
+    /// - Disconnect the reference node from the base segment (the branch under the subject or the target base)
+    /// - Disconnect the last commit node of the child segment (the branch over the subject or the workspace commit)
+    /// - Nothing else. Other parentage and children are kept, since this is what we care about in a GB workspace world.
+    ///
+    /// ### What the future holds
+    /// In the future, where we're not afraid of complex graphs, we've figured out UX and data wrangling,
+    /// the concept of a segment might not hold, and hence we'll have to figure out a better way of determining
+    /// what to cut (e.g. letting the clients decide what to cut).
+    fn retrieve_branches_and_containers<'a>(
+        workspace: &'a but_graph::projection::Workspace,
+        subject_branch_name: &FullNameRef,
+        target_branch_name: &FullNameRef,
+    ) -> anyhow::Result<(WorkspaceSegmentContext<'a>, WorkspaceSegmentContext<'a>)> {
+        let Some(source) = workspace.find_segment_and_stack_by_refname(subject_branch_name) else {
+            bail!(
+                "Couldn't find branch to move in workspace with reference name: {subject_branch_name}"
+            );
+        };
+
+        let Some(destination) = workspace.find_segment_and_stack_by_refname(target_branch_name)
+        else {
+            bail!(
+                "Couldn't find target branch to move in workspace with reference name: {target_branch_name}"
+            );
+        };
+        Ok((source, destination))
+    }
 }
 
 /// Get the right disconnect parameters for the given subject segment and source stack.
diff --git a/crates/but-workspace/src/commit/move_commit.rs b/crates/but-workspace/src/commit/move_commit.rs
@@ -9,21 +9,31 @@ pub(crate) mod function {
 
     /// Move a commit.
     ///
+    /// `editor` is assumed to have been generated from the given `workspace`
+    /// and therefore aligned.
+    ///
+    /// `workspace` - Used for getting the surrounding context of the commit being moved.
+    ///     In the future, we should not rely on the projection and do it fully on the graph.
+    ///
+    /// `subject_commit` - The commit to be moved.
+    ///
+    /// `anchor` - A git graph node selector to move the subject commit relative to.
+    ///
+    /// `side` - The side relative to the anchor at which to insert the subject commit.
+    ///
     /// The subject commit will be detached from the source segment, and inserted relative
     /// to a given anchor (branch or commit).
     pub fn move_commit(
-        workspace: &but_graph::projection::Workspace,
         mut editor: Editor,
+        workspace: &but_graph::projection::Workspace,
         subject_commit: impl ToCommitSelector,
         anchor: impl ToSelector,
         side: InsertSide,
     ) -> anyhow::Result<SuccessfulRebase> {
         let (subject_commit_selector, subject_commit) =
             editor.find_selectable_commit(subject_commit)?;
 
-        let Some(subject) = workspace.find_commit_and_containers(&subject_commit.id) else {
-            bail!("Failed to find the commit to move in the workspace.");
-        };
+        let subject = retrieve_commit_and_containers(workspace, &subject_commit)?;
 
         let (source_stack, source_segment, _) = subject;
 
@@ -50,16 +60,53 @@ pub(crate) mod function {
             index_of_subject_commit,
         )?;
 
+        // Step 2: Disconnect
         editor.disconnect_segment_from(
             commit_delimiter.clone(),
             child_to_disconnect,
             parent_to_disconnect,
             false,
         )?;
+
+        // Step 3: Insert
         editor.insert_segment(anchor, commit_delimiter, side)?;
         editor.rebase()
     }
 
+    /// Determine the surrounding context of the commit to be moved
+    ///
+    /// Currently, this looks into the workspace projection in order to determine **where to take the commit from**.
+    ///
+    /// ### The issue
+    /// It's impossible to know for sure what is the exact intention of 'moving a commit' inside a complex git graph.
+    /// A commit, can have N children and M parents. 'Moving' it somewhere else can imply:
+    /// - Disconnecting all parents and children, and inserting it somewhere else.
+    /// - Disconnecting the first parent and all children, and then inserting.
+    /// - Disconnecting *some* parents and *some* children, and then inserting it.
+    ///
+    /// ### The GitButler assumption
+    /// In the context of a GitButler workspace (as of this writing), we want to disconnect the commit from the linear
+    /// segments and move them to another position in the same or other segment. That way, any other parents and
+    /// children that are not part of the source segment are kept.
+    ///
+    /// ### What the future holds
+    /// In the future, where we're not afraid of complex graphs, we've figured out UX and data wrangling,
+    /// the concept of a segment might not hold, and hence we'll have to figure out a better way of determining
+    /// what to cut (e.g. letting the clients decide what to cut).
+    fn retrieve_commit_and_containers<'a>(
+        workspace: &'a but_graph::projection::Workspace,
+        subject_commit: &but_core::CommitOwned,
+    ) -> anyhow::Result<(
+        &'a but_graph::projection::Stack,
+        &'a but_graph::projection::StackSegment,
+        &'a but_graph::projection::StackCommit,
+    )> {
+        let Some(subject) = workspace.find_commit_and_containers(&subject_commit.id) else {
+            bail!("Failed to find the commit to move in the workspace.");
+        };
+        Ok(subject)
+    }
+
     /// Determine which children to disconnect, based on the position of the subject commit in the segment.
     fn determine_child_selector(
         editor: &Editor,
diff --git a/crates/but-workspace/tests/workspace/branch/move_branch.rs b/crates/but-workspace/tests/workspace/branch/move_branch.rs
@@ -43,8 +43,8 @@ fn move_top_branch_to_top_of_another_stack() -> anyhow::Result<()> {
     // Put C on top of A
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/C".try_into()?,
             "refs/heads/A".try_into()?,
         )?;
@@ -115,8 +115,8 @@ fn move_bottom_branch_to_top_of_another_stack() -> anyhow::Result<()> {
     let editor = ws.graph.to_editor(&repo)?;
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/B".try_into()?,
             "refs/heads/A".try_into()?,
         )?;
@@ -188,8 +188,8 @@ fn move_single_branch_to_top_of_another_stack() -> anyhow::Result<()> {
     // Put A on top of C
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/A".try_into()?,
             "refs/heads/C".try_into()?,
         )?;
@@ -258,8 +258,8 @@ fn reorder_branch_in_stack() -> anyhow::Result<()> {
     // Put B on top of C
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/B".try_into()?,
             "refs/heads/C".try_into()?,
         )?;
@@ -331,8 +331,8 @@ fn insert_branch_in_the_middle_of_a_stack() -> anyhow::Result<()> {
     // Put A on top of B, and below C
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/A".try_into()?,
             "refs/heads/B".try_into()?,
         )?;
@@ -393,8 +393,8 @@ fn move_empty_branch() -> anyhow::Result<()> {
     // Put B on top of A
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/B".try_into()?,
             "refs/heads/A".try_into()?,
         )?;
@@ -449,8 +449,8 @@ fn move_branch_on_top_of_empty_branch() -> anyhow::Result<()> {
     // Put A on top of B
     let but_workspace::branch::move_branch::Outcome { rebase, ws_meta } =
         but_workspace::branch::move_branch(
-            &ws,
             editor,
+            &ws,
             "refs/heads/A".try_into()?,
             "refs/heads/B".try_into()?,
         )?;
diff --git a/crates/but-workspace/tests/workspace/commit/move_commit.rs b/crates/but-workspace/tests/workspace/commit/move_commit.rs
@@ -47,8 +47,8 @@ fn move_top_commit_to_top_of_another_stack() -> anyhow::Result<()> {
 
     // Put C commit at the top of A
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         c_commit_selector,
         a_commit_selector,
         InsertSide::Above,
@@ -141,8 +141,8 @@ fn move_bottom_commit_to_top_of_another_stack() -> anyhow::Result<()> {
 
     // Put B commit at the top of A
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         b_commit_selector,
         a_commit_selector,
         InsertSide::Above,
@@ -237,8 +237,8 @@ fn move_top_commit_to_bottom_of_another_stack() -> anyhow::Result<()> {
 
     // Put C commit below the A commit
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         c_commit_selector,
         a_commit_selector,
         InsertSide::Below,
@@ -331,8 +331,8 @@ fn move_bottom_commit_to_bottom_of_another_stack() -> anyhow::Result<()> {
 
     // Put B commit below the A commit
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         b_commit_selector,
         a_commit_selector,
         InsertSide::Below,
@@ -426,8 +426,8 @@ fn move_single_commit_to_the_top_of_another_branch() -> anyhow::Result<()> {
 
     // Put A commit at the top of the branch C
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         a_commit_selector,
         c_commit_selector,
         InsertSide::Above,
@@ -514,8 +514,8 @@ fn move_single_commit_to_the_bottom_of_another_branch() -> anyhow::Result<()> {
 
     // Put A commit below the B commit
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         a_commit_selector,
         b_commit_selector,
         InsertSide::Below,
@@ -601,8 +601,8 @@ fn move_commit_to_empty_branch() -> anyhow::Result<()> {
 
     // Put A commit in branch B
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         a_commit_selector,
         b_ref_selector,
         InsertSide::Below,
@@ -670,8 +670,8 @@ fn move_commit_in_non_managed_workspace() -> anyhow::Result<()> {
 
     // Put commit three at the top of branch two
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         three_commit_selector,
         two_ref_selector,
         InsertSide::Below,
@@ -744,8 +744,8 @@ fn reorder_commit_in_non_managed_workspace() -> anyhow::Result<()> {
 
     // Put commit three below commit two
     let rebase = but_workspace::commit::move_commit(
-        &ws,
         editor,
+        &ws,
         three_commit_selector,
         two_commit_selector,
         InsertSide::Below,
