Merge pull request #5324 from gitbutlerapp/kv-branch-1

reorder_stack: fix reordering between series

crates/gitbutler-branch-actions/src/reorder.rs

@@ -1,5 +1,7 @@
+use std::collections::HashMap;
+
 use anyhow::{bail, Context, Result};
-use git2::Oid;
+use git2::{Commit, Oid};
 use gitbutler_command_context::CommandContext;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::rebase::cherry_rebase_group;
@@ -46,25 +48,6 @@ pub fn reorder_stack(
     let old_head = repo.find_commit(stack.head())?;
     let merge_base = repo.merge_base(default_target_commit.id(), stack.head())?;
 
-    let mut update_pairs = vec![];
-    let mut previous = merge_base;
-    for series in new_order.series.iter().rev() {
-        let new_head = series.commit_ids.first();
-        let current = all_series
-            .iter()
-            .find(|s| s.head.name == series.name)
-            .unwrap();
-        let old_head = current.local_commits.last().unwrap();
-
-        let new_head_oid = if let Some(new_head) = new_head {
-            *new_head
-        } else {
-            previous
-        };
-        update_pairs.push((old_head.id(), new_head_oid));
-        previous = new_head_oid
-    }
-
     let ids_to_rebase = new_order
         .series
         .iter()
@@ -78,19 +61,23 @@ pub fn reorder_stack(
         tree: new_tree_oid,
     } = compute_updated_branch_head_for_commits(repo, old_head.id(), old_head.tree_id(), new_head)?;
 
-    // Set the series heads accordingly
-    for (current_oid, new_oid) in update_pairs {
-        let from_commit = repo.find_commit(current_oid)?;
-        let to_commit = repo.find_commit(new_oid)?;
-        // println!(
-        //     "Replacing {} with {}",
-        //     from_commit.message().unwrap(),
-        //     to_commit.message().unwrap()
-        // );
-        stack.replace_head(ctx, &from_commit, &to_commit)?;
-    }
-
+    // Ensure the stack head is set to the new oid after rebasing
     stack.set_stack_head(ctx, new_head_oid, Some(new_tree_oid))?;
+
+    let mut new_heads: HashMap<String, Commit<'_>> = HashMap::new();
+    let mut previous = merge_base;
+    for series in new_order.series.iter().rev() {
+        let commit = if let Some(commit_id) = series.commit_ids.first() {
+            repo.find_commit(*commit_id)?
+        } else {
+            repo.find_commit(previous)?
+        };
+        previous = commit.id();
+        new_heads.insert(series.name.clone(), commit);
+    }
+    // Set the series heads accordingly in one go
+    stack.set_all_heads(ctx, new_heads)?;
+
     checkout_branch_trees(ctx, perm)?;
     crate::integration::update_workspace_commit(&state, ctx)
         .context("failed to update gitbutler workspace")?;
@@ -176,8 +163,19 @@ impl StackOrder {
                 bail!("Commit '{}' does not exist in the stack", commit_id);
             }
         }
+
         // Ensure the new order is not a noop
-        if new_order_commit_ids == current_order_commit_ids {
+        if self
+            .series
+            .iter()
+            .map(|s| s.commit_ids.clone())
+            .collect_vec()
+            == current_order
+                .series
+                .iter()
+                .map(|s| s.commit_ids.clone())
+                .collect_vec()
+        {
             bail!("The new order is the same as the current order");
         }
 

crates/gitbutler-branch-actions/tests/fixtures/reorder.sh

@@ -38,3 +38,43 @@ git clone remote multiple-commits
   echo change6 >> file
   $CLI branch commit my_stack -m "commit 6"
 )
+
+git clone remote multiple-commits-small
+(cd multiple-commits-small
+  git config user.name "Author"
+  git config user.email "author@example.com"
+
+  git branch existing-branch
+  $CLI project add --switch-to-workspace "$(git rev-parse --symbolic-full-name @{u})"
+
+  $CLI branch create --set-default other_stack
+  echo change0 >> other_file
+  $CLI branch commit other_stack -m "commit 0"
+
+  $CLI branch create --set-default my_stack
+  echo change1 >> file
+  $CLI branch commit my_stack -m "commit 1"
+
+  $CLI branch series my_stack -s "top-series"
+  echo change4 >> file
+  $CLI branch commit my_stack -m "commit 2"
+)
+
+git clone remote multiple-commits-empty-top
+(cd multiple-commits-empty-top
+  git config user.name "Author"
+  git config user.email "author@example.com"
+
+  git branch existing-branch
+  $CLI project add --switch-to-workspace "$(git rev-parse --symbolic-full-name @{u})"
+
+  $CLI branch create --set-default other_stack
+  echo change0 >> other_file
+  $CLI branch commit other_stack -m "commit 0"
+
+  $CLI branch create --set-default my_stack
+  echo change1 >> file
+  $CLI branch commit my_stack -m "commit 1"
+
+  $CLI branch series my_stack -s "top-series"
+)

crates/gitbutler-branch-actions/tests/reorder.rs

@@ -164,6 +164,116 @@ fn reorder_series_head_to_another_series() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn reorder_stack_head_to_another_series() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let test_ctx = test_ctx(&ctx)?;
+    let order = order(vec![
+        vec![
+            test_ctx.top_commits["commit 5"],
+            test_ctx.top_commits["commit 4"],
+        ],
+        vec![
+            test_ctx.top_commits["commit 6"], // from the top series
+            test_ctx.bottom_commits["commit 3"],
+            test_ctx.bottom_commits["commit 2"],
+            test_ctx.bottom_commits["commit 1"],
+        ],
+    ]);
+    reorder_stack(ctx.project(), test_ctx.stack.id, order.clone())?;
+    let commits = vb_commits(&ctx);
+
+    // Verify the commit messages and ids in the second (top) series - top-series
+    assert_eq!(commits[0].msgs(), vec!["commit 5", "commit 4"]);
+    for i in 0..2 {
+        assert_ne!(commits[0].ids()[i], order.series[0].commit_ids[i]); // all in the top series are rebased
+    }
+
+    // Verify the commit messages and ids in the first (bottom) series
+    assert_eq!(
+        commits[1].msgs(),
+        vec!["commit 6", "commit 3", "commit 2", "commit 1"]
+    );
+    assert_ne!(commits[1].ids()[0], order.series[1].commit_ids[0]);
+    assert_eq!(commits[1].ids()[1], order.series[1].commit_ids[1]);
+    assert_eq!(commits[1].ids()[2], order.series[1].commit_ids[2]);
+    assert_eq!(commits[1].ids()[3], order.series[1].commit_ids[3]);
+    Ok(())
+}
+
+#[test]
+fn reorder_stack_making_top_empty_series() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits-small")?;
+    let test_ctx = test_ctx(&ctx)?;
+    let order = order(vec![
+        vec![],
+        vec![
+            test_ctx.top_commits["commit 2"], // from the top series
+            test_ctx.bottom_commits["commit 1"],
+        ],
+    ]);
+    reorder_stack(ctx.project(), test_ctx.stack.id, order.clone())?;
+    let commits = vb_commits(&ctx);
+
+    // Verify the commit messages and ids in the second (top) series - top-series
+    assert!(commits[0].msgs().is_empty());
+    assert!(commits[0].ids().is_empty());
+
+    // Verify the commit messages and ids in the first (bottom) series
+    assert_eq!(commits[1].msgs(), vec!["commit 2", "commit 1"]);
+    assert_eq!(commits[1].ids(), order.series[1].commit_ids); // nothing was rebased
+    Ok(())
+}
+
+#[test]
+fn reorder_stack_making_bottom_empty_series() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits-small")?;
+    let test_ctx = test_ctx(&ctx)?;
+    let order = order(vec![
+        vec![
+            test_ctx.top_commits["commit 2"],
+            test_ctx.bottom_commits["commit 1"], // from the bottom series
+        ],
+        vec![],
+    ]);
+    reorder_stack(ctx.project(), test_ctx.stack.id, order.clone())?;
+    let commits = vb_commits(&ctx);
+
+    // Verify the commit messages and ids in the second (top) series - top-series
+    assert_eq!(commits[0].msgs(), vec!["commit 2", "commit 1"]);
+    assert_eq!(commits[0].ids(), order.series[0].commit_ids); // nothing was rebased
+
+    // Verify the commit messages and ids in the first (bottom) series
+    assert!(commits[1].msgs().is_empty());
+    assert!(commits[1].ids().is_empty());
+
+    Ok(())
+}
+
+#[test]
+fn reorder_stack_into_empty_top() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits-empty-top")?;
+    let test_ctx = test_ctx(&ctx)?;
+    let order = order(vec![
+        vec![
+            test_ctx.bottom_commits["commit 1"], // from the bottom series
+        ],
+        vec![],
+    ]);
+    reorder_stack(ctx.project(), test_ctx.stack.id, order.clone())?;
+    let commits = vb_commits(&ctx);
+
+    // Verify the commit messages and ids in the second (top) series - top-series
+    assert_eq!(commits[0].msgs(), vec!["commit 1"]);
+    assert_eq!(commits[0].ids(), order.series[0].commit_ids); // nothing was rebased
+
+    // Verify the commit messages and ids in the first (bottom) series
+    assert!(commits[1].msgs().is_empty());
+    assert!(commits[1].ids().is_empty());
+
+    Ok(())
+}
+
 fn order(series: Vec<Vec<Oid>>) -> StackOrder {
     StackOrder {
         series: vec![

crates/gitbutler-stack/src/lib.rs

@@ -1,3 +1,4 @@
+#![warn(clippy::indexing_slicing)]
 mod file_ownership;
 mod ownership;
 mod stack;

crates/gitbutler-stack/src/stack.rs

@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::collections::HashSet;
 use std::str::FromStr;
 
 use anyhow::anyhow;
@@ -603,6 +604,32 @@ impl Stack {
         Ok(())
     }
 
+    /// Sets the stack heads to the provided commits.
+    /// This is useful multiple heads are updated and the intermediate states are not valid while the final state is.
+    pub fn set_all_heads(
+        &mut self,
+        ctx: &CommandContext,
+        new_heads: HashMap<String, Commit<'_>>,
+    ) -> Result<()> {
+        let state = branch_state(ctx);
+
+        // same heads, just differente commits
+        if self.heads.iter().map(|h| &h.name).collect::<HashSet<_>>()
+            != new_heads.keys().collect::<HashSet<_>>()
+        {
+            return Err(anyhow!("The new head names do not match the current heads"));
+        }
+        let stack_head = self.head();
+        for head in self.heads.iter_mut() {
+            validate_target(head, ctx.repository(), stack_head, &state)?;
+            if let Some(commit) = new_heads.get(&head.name).cloned() {
+                head.target = commit.clone().into();
+            }
+        }
+        state.set_branch(self.clone())?;
+        Ok(())
+    }
+
     pub fn set_legacy_compatible_stack_reference(&mut self, ctx: &CommandContext) -> Result<()> {
         // self.upstream is only set if this is a branch that was created & manipulated by the legacy flow
         let legacy_refname = match self.upstream.clone().map(|r| r.branch().to_owned()) {