Merge pull request #5112 from gitbutlerapp/stack-update-target-commit

Adds replace_head method on the Stack trait

crates/gitbutler-stack-api/src/stack_ext.rs

@@ -4,6 +4,7 @@ use anyhow::anyhow;
 use anyhow::bail;
 use anyhow::Context;
 use anyhow::Result;
+use git2::Commit;
 use gitbutler_command_context::CommandContext;
 use gitbutler_commit::commit_ext::CommitExt;
 use gitbutler_patch_reference::{CommitOrChangeId, PatchReference};
@@ -134,6 +135,23 @@ pub trait StackExt {
     /// This operation will compute the current list of local and remote commits that belong to each series.
     /// The first entry is the newest in the Stack (i.e the top of the stack).
     fn list_series(&self, ctx: &CommandContext) -> Result<Vec<Series>>;
+
+    /// Updates all heads in the stack that point to the `from` commit to point to the `to` commit.
+    /// If there is nothing pointing to the `from` commit, this operation is a no-op.
+    /// If the `from` and `to` commits have the same change_id, this operation is also a no-op.
+    ///
+    /// In the case that the `from` commit is the head of the stack, this operation delegates to `set_stack_head`.
+    ///
+    /// Every time a commit/patch is moved / removed / updated, this method needs to be invoked to maintain the integrity of the stack.
+    /// Typically in this case the `to` Commit would be `from`'s parent.
+    ///
+    /// The `to` commit must be between the Stack head and it's merge base otherwise this operation will error out.
+    fn replace_head(
+        &mut self,
+        ctx: &CommandContext,
+        from: &Commit<'_>,
+        to: &Commit<'_>,
+    ) -> Result<()>;
 }
 
 /// Request to update a PatchReference.
@@ -434,7 +452,6 @@ impl StackExt for Stack {
         )
     }
 
-    // todo: remote commits are not being populated yet
     fn list_series(&self, ctx: &CommandContext) -> Result<Vec<Series>> {
         if !self.initialized() {
             return Err(anyhow!("Stack has not been initialized"));
@@ -491,6 +508,67 @@ impl StackExt for Stack {
         }
         Ok(all_series)
     }
+
+    fn replace_head(
+        &mut self,
+        ctx: &CommandContext,
+        from: &Commit<'_>,
+        to: &Commit<'_>,
+    ) -> Result<()> {
+        if !self.initialized() {
+            return Err(anyhow!("Stack has not been initialized"));
+        }
+        // find all heads matching the 'from' target (there can be multiple heads pointing to the same commit)
+        let matching_heads = self
+            .heads
+            .iter()
+            .filter(|h| match from.change_id() {
+                Some(change_id) => h.target == CommitOrChangeId::ChangeId(change_id.clone()),
+                None => h.target == CommitOrChangeId::CommitId(from.id().to_string()),
+            })
+            .cloned()
+            .collect_vec();
+
+        if from.change_id() == to.change_id() {
+            // there is nothing to do
+            return Ok(());
+        }
+
+        let state = branch_state(ctx);
+        let mut updated_heads: Vec<PatchReference> = vec![];
+
+        for head in matching_heads {
+            if self.heads.last().cloned() == Some(head.clone()) {
+                // the head is the stack head - update it accordingly
+                self.set_stack_head(ctx, to.id(), None)?;
+            } else {
+                // new head target from the 'to' commit
+                let new_target = match to.change_id() {
+                    Some(change_id) => CommitOrChangeId::ChangeId(change_id.to_string()),
+                    None => CommitOrChangeId::CommitId(to.id().to_string()),
+                };
+                let mut new_head = head.clone();
+                new_head.target = new_target;
+                // validate the updated head
+                validate_target(&new_head, ctx, self.head(), &state)?;
+                // add it to the list of updated heads
+                updated_heads.push(new_head);
+            }
+        }
+
+        if !updated_heads.is_empty() {
+            for updated_head in updated_heads {
+                if let Some(head) = self.heads.iter_mut().find(|h| h.name == updated_head.name) {
+                    // find set the corresponding head in the mutable self
+                    *head = updated_head;
+                }
+            }
+            self.updated_timestamp_ms = gitbutler_time::time::now_ms();
+            // update the persistent state
+            state.set_branch(self.clone())?;
+        }
+        Ok(())
+    }
 }
 
 /// Validates that the commit in the reference target

crates/gitbutler-stack-api/tests/mod.rs

@@ -752,6 +752,264 @@ fn set_stack_head() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn replace_head_single() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let top_of_stack = test_ctx.branch.heads.last().unwrap().target.clone();
+    let from_head = PatchReference {
+        name: "from_head".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    test_ctx.branch.add_series(&ctx, from_head, None)?;
+    // replace with previous head
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &test_ctx.commits[1], &test_ctx.commits[0]);
+    assert!(result.is_ok());
+    // the heads is update to point to the new commit
+    assert_eq!(
+        test_ctx.branch.heads[0].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[0].change_id().unwrap())
+    );
+    // the top of the stack is not changed
+    assert_eq!(test_ctx.branch.heads.last().unwrap().target, top_of_stack);
+    // the state was persisted
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_head_single_with_mergebase() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let top_of_stack = test_ctx.branch.heads.last().unwrap().target.clone();
+    let from_head = PatchReference {
+        name: "from_head".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    test_ctx.branch.add_series(&ctx, from_head, None)?;
+    // replace with merge base
+    let merge_base = ctx.repository().find_commit(
+        ctx.repository()
+            .merge_base(test_ctx.branch.head(), test_ctx.default_target.sha)?,
+    )?;
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &test_ctx.commits[1], &merge_base);
+    assert!(result.is_ok());
+    // the heads is update to point to the new commit
+    // this time its a commit id
+    assert_eq!(
+        test_ctx.branch.heads[0].target,
+        CommitOrChangeId::CommitId(merge_base.id().to_string())
+    );
+    // the top of the stack is not changed
+    assert_eq!(test_ctx.branch.heads.last().unwrap().target, top_of_stack);
+    // the state was persisted
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_head_with_inavlid_commit_error() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let from_head = PatchReference {
+        name: "from_head".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    test_ctx.branch.add_series(&ctx, from_head, None)?;
+    let stack = test_ctx.branch.clone();
+    let result =
+        test_ctx
+            .branch
+            .replace_head(&ctx, &test_ctx.commits[1], &test_ctx.other_commits[0]); //in another stack
+    assert!(result.is_err());
+    // is unmodified
+    assert_eq!(stack, test_ctx.branch);
+    // same in persistence
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_head_with_same_noop() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let from_head = PatchReference {
+        name: "from_head".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    test_ctx.branch.add_series(&ctx, from_head, None)?;
+    let stack = test_ctx.branch.clone();
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &test_ctx.commits[1], &test_ctx.commits[1]);
+    assert!(result.is_ok());
+    // is unmodified
+    assert_eq!(stack, test_ctx.branch);
+    // same in persistence
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_no_head_noop() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let stack = test_ctx.branch.clone();
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &test_ctx.commits[1], &test_ctx.commits[0]);
+    assert!(result.is_ok());
+    // is unmodified
+    assert_eq!(stack, test_ctx.branch);
+    // same in persistence
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_non_member_commit_noop_noerror() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let stack = test_ctx.branch.clone();
+    let result =
+        test_ctx
+            .branch
+            .replace_head(&ctx, &test_ctx.other_commits[0], &test_ctx.commits[0]);
+    assert!(result.is_ok());
+    // is unmodified
+    assert_eq!(stack, test_ctx.branch);
+    Ok(())
+}
+
+#[test]
+fn replace_top_of_stack_single() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let initial_head = ctx.repository().find_commit(test_ctx.branch.head())?;
+
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &initial_head, &test_ctx.commits[1]);
+    assert!(result.is_ok());
+    // the heads is update to point to the new commit
+    assert_eq!(
+        test_ctx.branch.heads[0].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap())
+    );
+    assert_eq!(test_ctx.branch.head(), test_ctx.commits[1].id());
+    assert_eq!(test_ctx.branch.heads.len(), 1);
+    // the state was persisted
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_head_multiple() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let top_of_stack = test_ctx.branch.heads.last().unwrap().target.clone();
+    let from_head_1 = PatchReference {
+        name: "from_head_1".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    let from_head_2 = PatchReference {
+        name: "from_head_2".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    // both references point to the same commit
+    test_ctx.branch.add_series(&ctx, from_head_1, None)?;
+    test_ctx
+        .branch
+        .add_series(&ctx, from_head_2, Some("from_head_1".into()))?;
+    // replace the commit
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &test_ctx.commits[1], &test_ctx.commits[0]);
+    assert!(result.is_ok());
+    // both heads are  updated to point to the new commit
+    assert_eq!(
+        test_ctx.branch.heads[0].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[0].change_id().unwrap())
+    );
+    assert_eq!(
+        test_ctx.branch.heads[1].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[0].change_id().unwrap())
+    );
+    // the top of the stack is not changed
+    assert_eq!(test_ctx.branch.heads.last().unwrap().target, top_of_stack);
+    // the state was persisted
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
+#[test]
+fn replace_head_top_of_stack_multiple() -> Result<()> {
+    let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
+    let mut test_ctx = test_ctx(&ctx)?;
+    let initial_head = ctx.repository().find_commit(test_ctx.branch.head())?;
+    let extra_head = PatchReference {
+        name: "extra_head".into(),
+        target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()),
+        description: None,
+    };
+    // an extra head just beneath the top of the stack
+    test_ctx.branch.add_series(&ctx, extra_head, None)?;
+    // replace top of stack the commit
+    let result = test_ctx
+        .branch
+        .replace_head(&ctx, &initial_head, &test_ctx.commits[1]);
+    assert!(result.is_ok());
+    // both heads are  updated to point to the new commit
+    assert_eq!(
+        test_ctx.branch.heads[0].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap())
+    );
+    assert_eq!(
+        test_ctx.branch.heads[1].target,
+        CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap())
+    );
+    assert_eq!(test_ctx.branch.head(), test_ctx.commits[1].id());
+    // order is the same
+    assert_eq!(test_ctx.branch.heads[0].name, "extra_head");
+    // the state was persisted
+    assert_eq!(
+        test_ctx.branch,
+        test_ctx.handle.get_branch(test_ctx.branch.id)?
+    );
+    Ok(())
+}
+
 fn command_ctx(name: &str) -> Result<(CommandContext, TempDir)> {
     gitbutler_testsupport::writable::fixture("stacking.sh", name)
 }