From 82406810f0304ef61d4cbad6172d0ccb092812d0 Mon Sep 17 00:00:00 2001 From: Caleb Owens Date: Mon, 7 Oct 2024 16:10:34 +0200 Subject: [PATCH] Extract tree updating into its own tested function --- .../src/branch_trees.rs | 89 +++++++++++ crates/gitbutler-branch-actions/src/lib.rs | 2 +- .../src/reorder_commits.rs | 64 ++++---- .../src/upstream_integration.rs | 120 +++++---------- .../tests/virtual_branches/branch_trees.rs | 138 ++++++++++++++++++ .../tests/virtual_branches/mod.rs | 1 + 6 files changed, 293 insertions(+), 121 deletions(-) create mode 100644 crates/gitbutler-branch-actions/tests/virtual_branches/branch_trees.rs diff --git a/crates/gitbutler-branch-actions/src/branch_trees.rs b/crates/gitbutler-branch-actions/src/branch_trees.rs index 7cdf3b7af..dd67c4826 100644 --- a/crates/gitbutler-branch-actions/src/branch_trees.rs +++ b/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 { + 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 { + 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)] diff --git a/crates/gitbutler-branch-actions/src/lib.rs b/crates/gitbutler-branch-actions/src/lib.rs index 49b8e3cd6..c818ad03a 100644 --- a/crates/gitbutler-branch-actions/src/lib.rs +++ b/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; diff --git a/crates/gitbutler-branch-actions/src/reorder_commits.rs b/crates/gitbutler-branch-actions/src/reorder_commits.rs index 567741dc5..9aec72d8f 100644 --- a/crates/gitbutler-branch-actions/src/reorder_commits.rs +++ b/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( diff --git a/crates/gitbutler-branch-actions/src/upstream_integration.rs b/crates/gitbutler-branch-actions/src/upstream_integration.rs index e271e2e42..5649c132e 100644 --- a/crates/gitbutler-branch-actions/src/upstream_integration.rs +++ b/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; diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/branch_trees.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/branch_trees.rs new file mode 100644 index 000000000..d23d9bcbe --- /dev/null +++ b/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() + ); + } +} diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs index cfb1c4c2f..152fc96d5 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs +++ b/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;