Merge pull request #5052 from gitbutlerapp/Refactor-tree-updating

Extract tree updating into its own tested function

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

@@ -1,6 +1,10 @@
 use anyhow::{bail, Result};
+use gitbutler_branch::Branch;
+use gitbutler_cherry_pick::RepositoryExt;
 use gitbutler_command_context::CommandContext;
+use gitbutler_commit::commit_ext::CommitExt as _;
 use gitbutler_project::access::WorktreeWritePermission;
+use gitbutler_repo::rebase::cherry_rebase_group;
 use gitbutler_repo::RepositoryExt as _;
 
 use crate::VirtualBranchesExt as _;
@@ -61,6 +65,91 @@ pub(crate) fn checkout_branch_trees<'a>(
     }
 }
 
+pub struct BranchHeadAndTree {
+    /// This is a commit Oid.
+    ///
+    /// This should be used as the new head Oid for the branch.
+    pub head: git2::Oid,
+    /// This is a tree Oid.
+    ///
+    /// This should be used as the new tree Oid for the branch.
+    pub tree: git2::Oid,
+}
+
+/// Given a new head for a branch, this comptues how the tree should be
+/// rebased on top of the new head. If the rebased tree is conflicted, then
+/// the function will return a new head commit which is the conflicted
+/// tree commit, and the the tree oid will be the auto-resolved tree.
+///
+/// This does not mutate the branch, or update the virtual_branches.toml.
+/// You probably also want to call [`checkout_branch_trees`] after you have
+/// mutated the virtual_branches.toml.
+pub fn compute_updated_branch_head(
+    repository: &git2::Repository,
+    branch: &Branch,
+    new_head: git2::Oid,
+    fearless_rebasing: bool,
+) -> Result<BranchHeadAndTree> {
+    compute_updated_branch_head_for_commits(
+        repository,
+        branch.head,
+        branch.tree,
+        new_head,
+        fearless_rebasing,
+    )
+}
+
+/// Given a new head for a branch, this comptues how the tree should be
+/// rebased on top of the new head. If the rebased tree is conflicted, then
+/// the function will return a new head commit which is the conflicted
+/// tree commit, and the the tree oid will be the auto-resolved tree.
+///
+/// If you have access to a [`Branch`] object, it's probably preferable to
+/// use [`compute_updated_branch_head`] instead to prevent programmer error.
+///
+/// This does not mutate the branch, or update the virtual_branches.toml.
+/// You probably also want to call [`checkout_branch_trees`] after you have
+/// mutated the virtual_branches.toml.
+pub fn compute_updated_branch_head_for_commits(
+    repository: &git2::Repository,
+    old_head: git2::Oid,
+    old_tree: git2::Oid,
+    new_head: git2::Oid,
+    fearless_rebasing: bool,
+) -> Result<BranchHeadAndTree> {
+    let (author, committer) = repository.signatures()?;
+
+    let commited_tree = repository.commit_with_signature(
+        None,
+        &author,
+        &committer,
+        "Uncommited changes",
+        &repository.find_tree(old_tree)?,
+        &[&repository.find_commit(old_head)?],
+        Default::default(),
+    )?;
+
+    let rebased_tree =
+        cherry_rebase_group(repository, new_head, &[commited_tree], fearless_rebasing)?;
+    let rebased_tree = repository.find_commit(rebased_tree)?;
+
+    if rebased_tree.is_conflicted() {
+        let auto_tree_id = repository
+            .find_real_tree(&rebased_tree, Default::default())?
+            .id();
+
+        Ok(BranchHeadAndTree {
+            head: rebased_tree.id(),
+            tree: auto_tree_id,
+        })
+    } else {
+        Ok(BranchHeadAndTree {
+            head: new_head,
+            tree: rebased_tree.tree_id(),
+        })
+    }
+}
+
 // These possibly could be considered more "integration" tests, but there is no
 // need for `checkout_branch_trees` to be public, so it is tested here.
 #[cfg(test)]

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

@@ -43,7 +43,7 @@ pub use remote::{RemoteBranch, RemoteBranchData, RemoteCommit};
 
 pub mod conflicts;
 
-mod branch_trees;
+pub mod branch_trees;
 mod move_commits;
 mod reorder_commits;
 mod undo_commit;

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

@@ -1,12 +1,15 @@
 use anyhow::{bail, Context as _, Result};
-use gitbutler_branch::{signature, BranchId, SignaturePurpose};
-use gitbutler_cherry_pick::RepositoryExt as _;
+use gitbutler_branch::BranchId;
 use gitbutler_command_context::CommandContext;
-use gitbutler_commit::commit_ext::CommitExt as _;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{rebase::cherry_rebase_group, LogUntil, RepositoryExt as _};
 
-use crate::{branch_trees::checkout_branch_trees, VirtualBranchesExt as _};
+use crate::{
+    branch_trees::{
+        checkout_branch_trees, compute_updated_branch_head_for_commits, BranchHeadAndTree,
+    },
+    VirtualBranchesExt as _,
+};
 
 /// Moves a commit up or down a stack by a certain offset.
 ///
@@ -119,44 +122,35 @@ fn inner_reorder_commit(
 
     ensure_offset_within_bounds(subject_commit, offset, branch_commits)?;
 
-    let author = signature(SignaturePurpose::Author)?;
-    let committer = signature(SignaturePurpose::Committer)?;
-    let tree_commit = repository.commit(
-        None,
-        &author,
-        &committer,
-        "Uncommited changes",
-        branch_tree,
-        &[&repository.find_commit(branch_commits[0])?],
-    )?;
-
-    let branch_commits = reorder_commit_list(subject_commit, offset, branch_commits)?;
+    let reordered_commits = reorder_commit_list(subject_commit, offset, branch_commits)?;
 
     // Rebase branch commits
     // We are passing all the commits to the cherry_rebase_group funcion, but
     // this is not a concern as it will verbaitm copy any commits that have
     // not had their parents changed.
-    let new_head_oid =
-        cherry_rebase_group(repository, base_commit, &branch_commits, succeeding_rebases)?;
+    let new_head_oid = cherry_rebase_group(
+        repository,
+        base_commit,
+        &reordered_commits,
+        succeeding_rebases,
+    )?;
 
-    // Rebase branch tree on top of new stack
-    let new_tree_commit =
-        cherry_rebase_group(repository, new_head_oid, &[tree_commit], succeeding_rebases)?;
-    let new_tree_commit = repository.find_commit(new_tree_commit)?;
+    // Calculate the new head and tree
+    let BranchHeadAndTree {
+        head: new_head_oid,
+        tree: new_tree_oid,
+    } = compute_updated_branch_head_for_commits(
+        repository,
+        branch_commits[0], // This function only operates on lists of 2 or greater
+        branch_tree.id(),
+        new_head_oid,
+        succeeding_rebases,
+    )?;
 
-    if new_tree_commit.is_conflicted() {
-        Ok(ReorderResult {
-            tree: repository
-                .find_real_tree(&new_tree_commit, Default::default())?
-                .id(),
-            head: new_tree_commit.id(),
-        })
-    } else {
-        Ok(ReorderResult {
-            tree: new_tree_commit.tree_id(),
-            head: new_head_oid,
-        })
-    }
+    Ok(ReorderResult {
+        head: new_head_oid,
+        tree: new_tree_oid,
+    })
 }
 
 fn reorder_commit_list(

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

@@ -1,10 +1,7 @@
-use anyhow::{anyhow, bail, Context, Result};
-use gitbutler_branch::{
-    signature, Branch, BranchId, SignaturePurpose, Target, VirtualBranchesHandle,
-};
+use anyhow::{anyhow, bail, Result};
+use gitbutler_branch::{Branch, BranchId, Target, VirtualBranchesHandle};
 use gitbutler_cherry_pick::RepositoryExt as _;
 use gitbutler_command_context::CommandContext;
-use gitbutler_commit::commit_ext::CommitExt;
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{
     rebase::{cherry_rebase_group, gitbutler_merge_commits},
@@ -12,7 +9,10 @@ use gitbutler_repo::{
 };
 use serde::{Deserialize, Serialize};
 
-use crate::{branch_trees::checkout_branch_trees, BranchManagerExt, VirtualBranchesExt as _};
+use crate::{
+    branch_trees::{checkout_branch_trees, compute_updated_branch_head, BranchHeadAndTree},
+    BranchManagerExt, VirtualBranchesExt as _,
+};
 
 #[derive(Serialize, PartialEq, Debug)]
 #[serde(tag = "type", content = "subject", rename_all = "camelCase")]
@@ -432,47 +432,24 @@ fn compute_resolutions(
                         target_branch_name,
                     )?;
 
-                    let head = repository.find_commit(virtual_branch.head)?;
-                    let tree = repository.find_tree(virtual_branch.tree)?;
-
-                    // Rebase tree
-                    let author_signature = signature(SignaturePurpose::Author)
-                        .context("Failed to get gitbutler signature")?;
-                    let committer_signature = signature(SignaturePurpose::Committer)
-                        .context("Failed to get gitbutler signature")?;
-                    let committed_tree = repository.commit(
-                        None,
-                        &author_signature,
-                        &committer_signature,
-                        "Uncommited changes",
-                        &tree,
-                        &[&head],
+                    // Get the updated tree oid
+                    let BranchHeadAndTree {
+                        head: new_head,
+                        tree: new_tree,
+                    } = compute_updated_branch_head(
+                        repository,
+                        virtual_branch,
+                        new_head.id(),
+                        true,
                     )?;
 
-                    // Rebase commited tree
-                    let new_commited_tree =
-                        cherry_rebase_group(repository, new_head.id(), &[committed_tree], true)?;
-                    let new_commited_tree = repository.find_commit(new_commited_tree)?;
-
-                    if new_commited_tree.is_conflicted() {
-                        Ok((
-                            virtual_branch.id,
-                            IntegrationResult::UpdatedObjects {
-                                head: new_commited_tree.id(),
-                                tree: repository
-                                    .find_real_tree(&new_commited_tree, Default::default())?
-                                    .id(),
-                            },
-                        ))
-                    } else {
-                        Ok((
-                            virtual_branch.id,
-                            IntegrationResult::UpdatedObjects {
-                                head: new_head.id(),
-                                tree: new_commited_tree.tree_id(),
-                            },
-                        ))
-                    }
+                    Ok((
+                        virtual_branch.id,
+                        IntegrationResult::UpdatedObjects {
+                            head: new_head,
+                            tree: new_tree,
+                        },
+                    ))
                 }
                 ResolutionApproach::Rebase => {
                     // Rebase the commits, then try rebasing the tree. If
@@ -497,47 +474,19 @@ fn compute_resolutions(
                         true,
                     )?;
 
-                    let head = repository.find_commit(virtual_branch.head)?;
-                    let tree = repository.find_tree(virtual_branch.tree)?;
+                    // Get the updated tree oid
+                    let BranchHeadAndTree {
+                        head: new_head,
+                        tree: new_tree,
+                    } = compute_updated_branch_head(repository, virtual_branch, new_head, true)?;
 
-                    // Rebase tree
-                    let author_signature = signature(SignaturePurpose::Author)
-                        .context("Failed to get gitbutler signature")?;
-                    let committer_signature = signature(SignaturePurpose::Committer)
-                        .context("Failed to get gitbutler signature")?;
-                    let committed_tree = repository.commit(
-                        None,
-                        &author_signature,
-                        &committer_signature,
-                        "Uncommited changes",
-                        &tree,
-                        &[&head],
-                    )?;
-
-                    // Rebase commited tree
-                    let new_commited_tree =
-                        cherry_rebase_group(repository, new_head, &[committed_tree], true)?;
-                    let new_commited_tree = repository.find_commit(new_commited_tree)?;
-
-                    if new_commited_tree.is_conflicted() {
-                        Ok((
-                            virtual_branch.id,
-                            IntegrationResult::UpdatedObjects {
-                                head: new_commited_tree.id(),
-                                tree: repository
-                                    .find_real_tree(&new_commited_tree, Default::default())?
-                                    .id(),
-                            },
-                        ))
-                    } else {
-                        Ok((
-                            virtual_branch.id,
-                            IntegrationResult::UpdatedObjects {
-                                head: new_head,
-                                tree: new_commited_tree.tree_id(),
-                            },
-                        ))
-                    }
+                    Ok((
+                        virtual_branch.id,
+                        IntegrationResult::UpdatedObjects {
+                            head: new_head,
+                            tree: new_tree,
+                        },
+                    ))
                 }
             }
         })
@@ -549,6 +498,7 @@ fn compute_resolutions(
 #[cfg(test)]
 mod test {
     use gitbutler_branch::BranchOwnershipClaims;
+    use gitbutler_commit::commit_ext::CommitExt as _;
     use gitbutler_testsupport::testing_repository::TestingRepository;
     use uuid::Uuid;
 

crates/gitbutler-branch-actions/tests/virtual_branches/branch_trees.rs

@@ -0,0 +1,138 @@
+use gitbutler_branch::{Branch, BranchOwnershipClaims};
+use uuid::Uuid;
+
+/// Makes a Branch struct with a bunch of default values.
+///
+/// This assumes that the only relevant properties for your test are the head
+/// and tree Oids.
+fn make_branch(head: git2::Oid, tree: git2::Oid) -> Branch {
+    Branch {
+        id: Uuid::new_v4().into(),
+        name: "branchy branch".into(),
+        notes: "bla bla bla".into(),
+        source_refname: None,
+        upstream: None,
+        upstream_head: None,
+        created_timestamp_ms: 69420,
+        updated_timestamp_ms: 69420,
+        tree,
+        head,
+        ownership: BranchOwnershipClaims::default(),
+        order: 0,
+        selected_for_changes: None,
+        allow_rebasing: true,
+        in_workspace: true,
+        not_in_workspace_wip_change_id: None,
+        heads: Default::default(),
+    }
+}
+
+#[cfg(test)]
+mod compute_updated_branch_head {
+    use super::*;
+    use gitbutler_branch_actions::branch_trees::{compute_updated_branch_head, BranchHeadAndTree};
+    use gitbutler_cherry_pick::RepositoryExt as _;
+    use gitbutler_commit::commit_ext::CommitExt;
+    use gitbutler_testsupport::testing_repository::{
+        assert_commit_tree_matches, assert_tree_matches, TestingRepository,
+    };
+
+    /// When the head ID is the same as the branch ID, we should return the same Oids.
+    #[test]
+    fn head_id_is_the_same() {
+        let test_repository = TestingRepository::open();
+
+        let base_commit = test_repository.commit_tree(None, &[("foo.txt", "foo")]);
+        let head = test_repository.commit_tree(Some(&base_commit), &[("foo.txt", "bar")]);
+        let tree = test_repository.commit_tree(Some(&head), &[("foo.txt", "baz")]);
+
+        let branch = make_branch(head.id(), tree.tree_id());
+
+        let BranchHeadAndTree { head, tree } =
+            compute_updated_branch_head(&test_repository.repository, &branch, head.id(), true)
+                .unwrap();
+
+        assert_eq!(head, branch.head);
+        assert_eq!(tree, branch.tree);
+    }
+
+    /// When the head ID is different from the branch ID, we should rebase the
+    /// tree on top of it.
+    ///
+    /// This test is set up such that the tree won't be conflicted.
+    ///
+    /// We expect to see the head commit match what we passed in as the new
+    /// head, and the tree should rebased on top of that new head.
+    #[test]
+    fn head_id_is_different() {
+        let test_repository = TestingRepository::open();
+
+        let base_commit = test_repository.commit_tree(None, &[("foo.txt", "foo")]);
+        let head = test_repository.commit_tree(Some(&base_commit), &[("foo.txt", "bar")]);
+        let tree =
+            test_repository.commit_tree(Some(&head), &[("foo.txt", "bar"), ("bar.txt", "baz")]);
+
+        let branch = make_branch(head.id(), tree.tree_id());
+
+        let new_head = test_repository.commit_tree(Some(&base_commit), &[("foo.txt", "new")]);
+
+        let BranchHeadAndTree { head, tree } =
+            compute_updated_branch_head(&test_repository.repository, &branch, new_head.id(), true)
+                .unwrap();
+
+        assert_eq!(head, new_head.id());
+        assert_tree_matches(
+            &test_repository.repository,
+            &test_repository.repository.find_tree(tree).unwrap(),
+            &[("foo.txt", b"new"), ("bar.txt", b"baz")],
+        );
+    }
+
+    /// When the head ID is different from the branch ID and the new head will
+    /// conflict with the tree.
+    ///
+    /// In this case we should expect to receive a new head commit that is the
+    /// conflicted result of the rebase, and the tree will the the
+    /// auto-resolved tree of that new head commit.
+    #[test]
+    fn tree_conflicts() {
+        let test_repository = TestingRepository::open();
+
+        let base_commit = test_repository.commit_tree(None, &[("foo.txt", "foo")]);
+        let head = test_repository.commit_tree(Some(&base_commit), &[("foo.txt", "bar")]);
+        let tree = test_repository.commit_tree(Some(&head), &[("foo.txt", "baz")]);
+
+        let branch = make_branch(head.id(), tree.tree_id());
+
+        let new_head = test_repository.commit_tree(Some(&base_commit), &[("foo.txt", "new")]);
+
+        let BranchHeadAndTree { head, tree } =
+            compute_updated_branch_head(&test_repository.repository, &branch, new_head.id(), true)
+                .unwrap();
+
+        let new_new_head = test_repository.repository.find_commit(head).unwrap();
+        assert!(new_new_head.is_conflicted());
+        assert_eq!(new_new_head.parent(0).unwrap().id(), new_head.id());
+
+        assert_commit_tree_matches(
+            &test_repository.repository,
+            &new_new_head,
+            &[
+                (".auto-resolution/foo.txt", b"new"), // Auto-resolves to new_head
+                (".conflict-base-0/foo.txt", b"bar"), // head is the base
+                (".conflict-side-0/foo.txt", b"new"), // new_head is the ours side
+                (".conflict-side-1/foo.txt", b"baz"), // tree is the theris side
+            ],
+        );
+
+        // Tree should be the auto-resolved tree.
+        assert_eq!(
+            tree,
+            test_repository
+                .repository
+                .find_real_tree(&new_new_head, Default::default())
+                .unwrap()
+                .id()
+        );
+    }
+}

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

@@ -57,6 +57,7 @@ impl Test {
 
 mod amend;
 mod apply_virtual_branch;
+mod branch_trees;
 mod create_commit;
 mod create_virtual_branch_from_branch;
 mod init;