Merge pull request #5060 from gitbutlerapp/Consistent-setting-of-stack-head-and-updating-series

Consistent setting of stack head and updating series

Cargo.lock

@@ -2314,6 +2314,7 @@ dependencies = [
  "gitbutler-reference",
  "gitbutler-repo",
  "gitbutler-stack",
+ "gitbutler-stack-api",
  "gitbutler-time",
  "serde",
 ]
@@ -2580,6 +2581,7 @@ dependencies = [
  "gitbutler-repo",
  "gitbutler-stack",
  "gitbutler-testsupport",
+ "gitbutler-time",
  "gix",
  "itertools 0.13.0",
  "rand 0.8.5",

crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs

@@ -217,13 +217,11 @@ impl BranchManager<'_> {
             },
         );
 
-        let branch = if let Ok(Some(mut branch)) =
+        let mut branch = if let Ok(Some(mut branch)) =
             vb_state.find_by_source_refname_where_not_in_workspace(target)
         {
             branch.upstream_head = upstream_branch.is_some().then_some(head_commit.id());
             branch.upstream = upstream_branch;
-            branch.tree = head_commit_tree.id();
-            branch.set_head(head_commit.id());
             branch.ownership = ownership;
             branch.order = order;
             branch.selected_for_changes = selected_for_changes;
@@ -247,7 +245,7 @@ impl BranchManager<'_> {
             )
         };
 
-        vb_state.set_branch(branch.clone())?;
+        branch.set_stack_head(self.ctx, head_commit.id(), Some(head_commit_tree.id()))?;
         self.ctx.add_branch_reference(&branch)?;
 
         match self.apply_branch(branch.id, perm) {
@@ -345,10 +343,11 @@ impl BranchManager<'_> {
                 )?
             };
 
-            branch.set_head(new_head.id());
-            branch.tree = repo.find_real_tree(&new_head, Default::default())?.id();
-
-            vb_state.set_branch(branch.clone())?;
+            branch.set_stack_head(
+                self.ctx,
+                new_head.id(),
+                Some(repo.find_real_tree(&new_head, Default::default())?.id()),
+            )?;
         }
 
         // apply the branch

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

@@ -6,6 +6,7 @@ use gitbutler_repo::{
     LogUntil, RepositoryExt as _,
 };
 use gitbutler_stack::StackId;
+use gitbutler_stack_api::StackExt;
 
 use crate::{
     branch_trees::{
@@ -66,11 +67,7 @@ pub fn integrate_upstream_commits(
 
     let mut branch = branch.clone();
 
-    branch.set_head(head);
-    branch.tree = tree;
-
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, head, Some(tree))?;
 
     checkout_branch_trees(ctx, perm)?;
 

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

@@ -12,6 +12,7 @@ use gitbutler_operating_modes::OPEN_WORKSPACE_REFS;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{LogUntil, RepositoryExt};
 use gitbutler_stack::{Stack, VirtualBranchesHandle};
+use gitbutler_stack_api::StackExt;
 use tracing::instrument;
 
 use crate::{branch_manager::BranchManagerExt, conflicts, VirtualBranchesExt};
@@ -368,8 +369,6 @@ fn verify_head_is_clean(ctx: &CommandContext, perm: &mut WorktreeWritePermission
         .context("failed to create virtual branch")?;
 
     // rebasing the extra commits onto the new branch
-    let vb_state = ctx.project().virtual_branches();
-    // let mut head = new_branch.head;
     let mut head = new_branch.head();
     for commit in extra_commits {
         let new_branch_head = ctx
@@ -401,11 +400,7 @@ fn verify_head_is_clean(ctx: &CommandContext, perm: &mut WorktreeWritePermission
                 rebased_commit_oid
             ))?;
 
-        new_branch.set_head(rebased_commit.id());
-        new_branch.tree = rebased_commit.tree_id();
-        vb_state
-            .set_branch(new_branch.clone())
-            .context("failed to write branch")?;
+        new_branch.set_stack_head(ctx, rebased_commit.id(), Some(rebased_commit.tree_id()))?;
 
         head = rebased_commit.id();
     }

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

@@ -8,6 +8,7 @@ use gitbutler_commit::commit_ext::CommitExt;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{rebase::cherry_rebase_group, LogUntil, RepositoryExt};
 use gitbutler_stack::{OwnershipClaim, StackId};
+use gitbutler_stack_api::StackExt;
 use std::collections::HashMap;
 
 /// moves commit from the branch it's in to the top of the target branch
@@ -122,11 +123,9 @@ pub(crate) fn move_commit(
 
     // reset the source branch to the parent commit
     // and update the destination branch head
-    source_branch.set_head(source_branch_head_parent.id());
-    vb_state.set_branch(source_branch.clone())?;
+    source_branch.set_stack_head(ctx, source_branch_head_parent.id(), None)?;
 
-    destination_branch.set_head(new_destination_head_oid);
-    vb_state.set_branch(destination_branch.clone())?;
+    destination_branch.set_stack_head(ctx, new_destination_head_oid, None)?;
 
     checkout_branch_trees(ctx, perm)?;
 

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

@@ -3,6 +3,7 @@ use gitbutler_command_context::CommandContext;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{rebase::cherry_rebase_group, LogUntil, RepositoryExt as _};
 use gitbutler_stack::StackId;
+use gitbutler_stack_api::StackExt;
 
 use crate::{
     branch_trees::{
@@ -62,11 +63,7 @@ pub(crate) fn reorder_commit(
         &repository.find_tree(branch.tree)?,
     )?;
 
-    branch.tree = tree;
-    branch.set_head(head);
-
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, head, Some(tree))?;
 
     checkout_branch_trees(ctx, perm)?;
 

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

@@ -3,6 +3,7 @@ use gitbutler_command_context::CommandContext;
 use gitbutler_commit::commit_ext::CommitExt as _;
 use gitbutler_repo::{rebase::cherry_rebase_group, LogUntil, RepositoryExt as _};
 use gitbutler_stack::{Stack, StackId};
+use gitbutler_stack_api::StackExt;
 
 use crate::VirtualBranchesExt as _;
 
@@ -28,9 +29,7 @@ pub(crate) fn undo_commit(
 
     let new_head_commit = inner_undo_commit(ctx.repository(), branch.head(), commit_oid)?;
 
-    branch.set_head(new_head_commit);
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, new_head_commit, None)?;
 
     crate::integration::update_workspace_commit(&vb_state, ctx)
         .context("failed to update gitbutler workspace")?;

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

@@ -7,6 +7,7 @@ use gitbutler_repo::{
     LogUntil, RepoActionsExt as _, RepositoryExt as _,
 };
 use gitbutler_stack::{Stack, StackId, Target, VirtualBranchesHandle};
+use gitbutler_stack_api::StackExt;
 use serde::{Deserialize, Serialize};
 
 use crate::{
@@ -320,10 +321,7 @@ pub(crate) fn integrate_upstream(
                 continue;
             };
 
-            branch.set_head(*head);
-            branch.tree = *tree;
-
-            virtual_branches_state.set_branch(branch.clone())?;
+            branch.set_stack_head(command_context, *head, Some(*tree))?;
         }
 
         // checkout_branch_trees won't checkout anything if there are no

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

@@ -752,9 +752,7 @@ pub(crate) fn reset_branch(
     // what hunks were released by this reset, and assign them to this branch.
     let old_head = get_workspace_head(ctx)?;
 
-    branch.set_head(target_commit_id);
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, target_commit_id, None)?;
 
     let updated_head = get_workspace_head(ctx)?;
     let repo = ctx.repository();
@@ -916,14 +914,7 @@ pub fn commit(
     }
 
     let vb_state = ctx.project().virtual_branches();
-    branch.tree = tree_oid;
-    branch.set_head(commit_oid);
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
-    if let Err(e) = branch.set_stack_head(ctx, commit_oid) {
-        // TODO: Make this error out when this functionality is stable
-        tracing::warn!("failed to set stack head: {:?}", e);
-    }
+    branch.set_stack_head(ctx, commit_oid, Some(tree_oid))?;
 
     crate::integration::update_workspace_commit(&vb_state, ctx)
         .context("failed to update gitbutler workspace")?;
@@ -1330,8 +1321,7 @@ pub(crate) fn move_commit_file(
 
     // if there are no upstream commits (the "to" commit was the branch head), then we're done
     if upstream_commits.is_empty() {
-        target_branch.set_head(commit_oid);
-        vb_state.set_branch(target_branch.clone())?;
+        target_branch.set_stack_head(ctx, commit_oid, None)?;
         crate::integration::update_workspace_commit(&vb_state, ctx)?;
         return Ok(commit_oid);
     }
@@ -1342,8 +1332,7 @@ pub(crate) fn move_commit_file(
 
     // if that rebase worked, update the branch head and the gitbutler workspace
     if let Some(new_head) = new_head {
-        target_branch.set_head(new_head);
-        vb_state.set_branch(target_branch.clone())?;
+        target_branch.set_stack_head(ctx, new_head, None)?;
         crate::integration::update_workspace_commit(&vb_state, ctx)?;
         Ok(commit_oid)
     } else {
@@ -1458,8 +1447,7 @@ pub(crate) fn amend(
         .l(target_branch.head(), LogUntil::Commit(amend_commit.id()))?;
     // if there are no upstream commits, we're done
     if upstream_commits.is_empty() {
-        target_branch.set_head(commit_oid);
-        vb_state.set_branch(target_branch.clone())?;
+        target_branch.set_stack_head(ctx, commit_oid, None)?;
         crate::integration::update_workspace_commit(&vb_state, ctx)?;
         return Ok(commit_oid);
     }
@@ -1469,8 +1457,7 @@ pub(crate) fn amend(
     let new_head = cherry_rebase(ctx, commit_oid, amend_commit.id(), last_commit)?;
 
     if let Some(new_head) = new_head {
-        target_branch.set_head(new_head);
-        vb_state.set_branch(target_branch.clone())?;
+        target_branch.set_stack_head(ctx, new_head, None)?;
         crate::integration::update_workspace_commit(&vb_state, ctx)?;
         Ok(commit_oid)
     } else {
@@ -1509,14 +1496,14 @@ pub(crate) fn insert_blank_commit(
 
     if commit.id() == branch.head() && offset < 0 {
         // inserting before the first commit
-        branch.set_head(blank_commit_oid);
+        branch.set_stack_head(ctx, blank_commit_oid, None)?;
         crate::integration::update_workspace_commit(&vb_state, ctx)
             .context("failed to update gitbutler workspace")?;
     } else {
         // rebase all commits above it onto the new commit
         match cherry_rebase(ctx, blank_commit_oid, commit.id(), branch.head()) {
             Ok(Some(new_head)) => {
-                branch.set_head(new_head);
+                branch.set_stack_head(ctx, new_head, None)?;
                 crate::integration::update_workspace_commit(&vb_state, ctx)
                     .context("failed to update gitbutler workspace")?;
             }
@@ -1526,8 +1513,6 @@ pub(crate) fn insert_blank_commit(
             }
         }
     }
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
 
     Ok(())
 }
@@ -1613,9 +1598,7 @@ pub(crate) fn squash(ctx: &CommandContext, branch_id: StackId, commit_id: git2::
     match cherry_rebase_group(ctx.repository(), new_commit_oid, &ids_to_rebase) {
         Ok(new_head_id) => {
             // save new branch head
-            branch.set_head(new_head_id);
-            branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-            vb_state.set_branch(branch.clone())?;
+            branch.set_stack_head(ctx, new_head_id, None)?;
 
             crate::integration::update_workspace_commit(&vb_state, ctx)
                 .context("failed to update gitbutler workspace")?;
@@ -1694,9 +1677,7 @@ pub(crate) fn update_commit_message(
     let new_head_id = cherry_rebase_group(ctx.repository(), new_commit_oid, &ids_to_rebase)
         .map_err(|err| err.context("rebase error"))?;
     // save new branch head
-    branch.set_head(new_head_id);
-    branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, new_head_id, None)?;
 
     crate::integration::update_workspace_commit(&vb_state, ctx)
         .context("failed to update gitbutler workspace")?;

crates/gitbutler-branch-actions/tests/extra/mod.rs

@@ -21,6 +21,7 @@ use gitbutler_commit::{commit_ext::CommitExt, commit_headers::CommitHeadersV2};
 use gitbutler_reference::{Refname, RemoteRefname};
 use gitbutler_repo::RepositoryExt;
 use gitbutler_stack::{BranchOwnershipClaims, Target, VirtualBranchesHandle};
+use gitbutler_stack_api::StackExt;
 use gitbutler_testsupport::{commit_all, virtual_branches::set_test_target, Case, Suite};
 use pretty_assertions::assert_eq;
 
@@ -844,8 +845,7 @@ fn merge_vbranch_upstream_clean_rebase() -> Result<()> {
         .expect("failed to create virtual branch");
 
     branch.upstream = Some(remote_branch.clone());
-    branch.set_head(last_push);
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, last_push, None)?;
 
     // create the branch
     let (branches, _) = internal::list_virtual_branches(ctx, guard.write_permission())?;
@@ -973,8 +973,7 @@ fn merge_vbranch_upstream_conflict() -> Result<()> {
         .create_virtual_branch(&BranchCreateRequest::default(), guard.write_permission())
         .expect("failed to create virtual branch");
     branch.upstream = Some(remote_branch.clone());
-    branch.set_head(last_push);
-    vb_state.set_branch(branch.clone())?;
+    branch.set_stack_head(ctx, last_push, None)?;
 
     internal::update_branch(
         ctx,

crates/gitbutler-edit-mode/Cargo.toml

@@ -22,4 +22,5 @@ gitbutler-oplog.workspace = true
 gitbutler-diff.workspace = true
 gitbutler-stack.workspace = true
 gitbutler-cherry-pick.workspace = true
+gitbutler-stack-api.workspace = true
 serde.workspace = true

crates/gitbutler-edit-mode/src/lib.rs

@@ -24,6 +24,7 @@ use gitbutler_project::access::{WorktreeReadPermission, WorktreeWritePermission}
 use gitbutler_reference::{ReferenceName, Refname};
 use gitbutler_repo::{rebase::cherry_rebase, RepositoryExt};
 use gitbutler_stack::{Stack, VirtualBranchesHandle};
+use gitbutler_stack_api::StackExt;
 
 pub mod commands;
 
@@ -227,12 +228,7 @@ pub(crate) fn save_and_return_to_workspace(
         tree: new_branch_tree,
     } = compute_updated_branch_head(repository, &virtual_branch, new_branch_head)?;
 
-    virtual_branch.set_head(new_branch_head);
-    virtual_branch.tree = new_branch_tree;
-    virtual_branch.updated_timestamp_ms = gitbutler_time::time::now_ms();
-    vb_state
-        .set_branch(virtual_branch)
-        .context("Failed to update vbstate")?;
+    virtual_branch.set_stack_head(ctx, new_branch_head, Some(new_branch_tree))?;
 
     // Switch branch to gitbutler/workspace
     repository

crates/gitbutler-stack-api/Cargo.toml

@@ -19,6 +19,7 @@ gitbutler-patch-reference.workspace = true
 gitbutler-reference.workspace = true
 gitbutler-repo.workspace = true
 gitbutler-commit.workspace = true
+gitbutler-time.workspace = true
 gitbutler-stack.workspace = true
 
 [[test]]

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

@@ -108,8 +108,17 @@ pub trait StackExt {
     ) -> Result<()>;
 
     /// Updates the most recent series of the stack to point to a new patch (commit or change ID).
-    /// This is a helper function that is equivalent to `update_series` with the target update set.
-    fn set_stack_head(&mut self, ctx: &CommandContext, commit_id: git2::Oid) -> Result<()>;
+    /// This will set the
+    /// - `head` of the stack to the new commit
+    /// - the target of the most recent series to the new commit
+    /// - the timestamp of the stack to the current time
+    /// - the tree of the stack to the new tree (if provided)
+    fn set_stack_head(
+        &mut self,
+        ctx: &CommandContext,
+        commit_id: git2::Oid,
+        tree: Option<git2::Oid>,
+    ) -> Result<()>;
 
     /// Pushes the reference (branch) to the Stack remote as derived from the default target.
     /// This operation will error out if the target has no push remote configured.
@@ -287,12 +296,20 @@ impl StackExt for Stack {
         state.set_branch(self.clone())
     }
 
-    fn set_stack_head(&mut self, ctx: &CommandContext, commit_id: git2::Oid) -> Result<()> {
+    fn set_stack_head(
+        &mut self,
+        ctx: &CommandContext,
+        commit_id: git2::Oid,
+        tree: Option<git2::Oid>,
+    ) -> Result<()> {
         if !self.initialized() {
             return Err(anyhow!("Stack has not been initialized"));
         }
-        if self.head() != commit_id {
-            bail!("The commit {} is not the head of the stack", commit_id);
+        self.updated_timestamp_ms = gitbutler_time::time::now_ms();
+        #[allow(deprecated)] // this is the only place where this is allowed
+        self.set_head(commit_id);
+        if let Some(tree) = tree {
+            self.tree = tree;
         }
         let commit = ctx.repository().find_commit(commit_id)?;
         let patch = if let Some(change_id) = commit.change_id() {
@@ -591,9 +608,14 @@ fn commit_by_branch_id_and_change_id<'a>(
 ) -> Result<git2::Commit<'a>> {
     // Find the commit with the change id
     let merge_base = ctx.repository().merge_base(stack_head, target_sha)?;
-    let commit = ctx
-        .repository()
-        .log(stack_head, LogUntil::Commit(merge_base))?
+
+    let commits = if stack_head == merge_base {
+        vec![ctx.repository().find_commit(stack_head)?]
+    } else {
+        ctx.repository()
+            .log(stack_head, LogUntil::Commit(merge_base))?
+    };
+    let commit = commits
         .iter()
         .map(|c| c.id())
         .find(|c| {

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

@@ -723,30 +723,31 @@ fn list_series_two_heads_different_commit() -> Result<()> {
 }
 
 #[test]
-fn set_stack_head_commit_from_other_stack() -> Result<()> {
+fn set_stack_head_commit_invalid() -> Result<()> {
     let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
     let mut test_ctx = test_ctx(&ctx)?;
     let result = test_ctx
         .branch
-        .set_stack_head(&ctx, test_ctx.other_commits.first().unwrap().id());
+        .set_stack_head(&ctx, git2::Oid::zero(), None);
     assert!(result.is_err());
     Ok(())
 }
 
 #[test]
-fn set_stack_head_commit_not_head() -> Result<()> {
+fn set_stack_head() -> Result<()> {
     let (ctx, _temp_dir) = command_ctx("multiple-commits")?;
     let mut test_ctx = test_ctx(&ctx)?;
-    let result = test_ctx
-        .branch
-        .set_stack_head(&ctx, test_ctx.commits.get(1).unwrap().id());
-    assert!(result.is_err());
+    let commit = test_ctx.other_commits.last().unwrap();
+    let result = test_ctx.branch.set_stack_head(&ctx, commit.id(), None);
+    assert!(result.is_ok());
+    let result = test_ctx.branch.list_series(&ctx)?;
     assert_eq!(
-        result.err().unwrap().to_string(),
-        format!(
-            "The commit {} is not the head of the stack",
-            test_ctx.commits[1].id()
-        )
+        result.first().unwrap().head.target,
+        CommitOrChangeId::ChangeId(commit.change_id().unwrap())
+    );
+    assert_eq!(
+        test_ctx.branch.head(),
+        test_ctx.other_commits.last().unwrap().id()
     );
     Ok(())
 }

crates/gitbutler-stack/src/stack.rs

@@ -129,6 +129,9 @@ impl Stack {
         self.head
     }
 
+    #[deprecated(
+        note = "DO NOT USE THIS DIRECTLY, use `stack_ext::StackExt::set_stack_head` instead."
+    )]
     pub fn set_head(&mut self, head: git2::Oid) {
         self.head = head;
     }