From c3f6db382a0b9008b3843ca4cb5054b997e4f434 Mon Sep 17 00:00:00 2001 From: Caleb Owens Date: Mon, 30 Sep 2024 01:53:21 +0200 Subject: [PATCH] Add more tests to undo commit --- .../gitbutler-branch-actions/src/actions.rs | 2 +- .../src/branch_manager/branch_creation.rs | 2 +- crates/gitbutler-branch-actions/src/lib.rs | 1 + .../src/undo_commit.rs | 171 ++++++++++++++++++ .../gitbutler-branch-actions/src/virtual.rs | 51 ------ 5 files changed, 174 insertions(+), 53 deletions(-) create mode 100644 crates/gitbutler-branch-actions/src/undo_commit.rs diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index 9c2f00da9..0cacebc26 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/crates/gitbutler-branch-actions/src/actions.rs @@ -298,7 +298,7 @@ pub fn undo_commit(project: &Project, branch_id: BranchId, commit_oid: git2::Oid assure_open_workspace_mode(&ctx).context("Undoing a commit requires open workspace mode")?; let mut guard = project.exclusive_worktree_access(); let snapshot_tree = ctx.project().prepare_snapshot(guard.read_permission()); - let result: Result<()> = vbranch::undo_commit(&ctx, branch_id, commit_oid) + let result: Result<()> = crate::undo_commit::undo_commit(&ctx, branch_id, commit_oid) .map(|_| ()) .map_err(Into::into); let _ = snapshot_tree.and_then(|snapshot_tree| { diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs index 6e2ef8efa..028f4d3ca 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs @@ -452,7 +452,7 @@ impl BranchManager<'_> { if let Some(headers) = potential_wip_commit.gitbutler_headers() { if headers.change_id == wip_commit_to_unapply { - branch = vbranch::undo_commit(self.ctx, branch.id, branch.head)?; + branch = crate::undo_commit::undo_commit(self.ctx, branch.id, branch.head)?; } } diff --git a/crates/gitbutler-branch-actions/src/lib.rs b/crates/gitbutler-branch-actions/src/lib.rs index 0c9ee1408..722b0a3a1 100644 --- a/crates/gitbutler-branch-actions/src/lib.rs +++ b/crates/gitbutler-branch-actions/src/lib.rs @@ -45,6 +45,7 @@ pub mod conflicts; mod branch_trees; mod reorder_commits; +mod undo_commit; mod author; mod status; diff --git a/crates/gitbutler-branch-actions/src/undo_commit.rs b/crates/gitbutler-branch-actions/src/undo_commit.rs new file mode 100644 index 000000000..a8985b146 --- /dev/null +++ b/crates/gitbutler-branch-actions/src/undo_commit.rs @@ -0,0 +1,171 @@ +use anyhow::{bail, Context as _, Result}; +use gitbutler_branch::{Branch, BranchId}; +use gitbutler_command_context::CommandContext; +use gitbutler_commit::commit_ext::CommitExt as _; +use gitbutler_repo::{rebase::cherry_rebase_group, LogUntil, RepositoryExt as _}; + +use crate::VirtualBranchesExt as _; + +/// Removes a commit from a branch by rebasing all commits _except_ for it +/// onto it's parent. +/// +/// if successful, it will update the branch head to the new head commit. +/// +/// It intentionally does **not** update the branch tree. It is a feature +/// of the operation that the branch tree will not be updated as it allows +/// the user to then re-commit the changes if they wish. +/// +/// This may create conflicted commits above the commit that is getting +/// undone. +pub(crate) fn undo_commit( + ctx: &CommandContext, + branch_id: BranchId, + commit_oid: git2::Oid, +) -> Result { + let vb_state = ctx.project().virtual_branches(); + let succeeding_rebases = ctx.project().succeeding_rebases; + + let mut branch = vb_state.get_branch_in_workspace(branch_id)?; + + let new_head_commit = inner_undo_commit( + ctx.repository(), + branch.head, + commit_oid, + succeeding_rebases, + )?; + + branch.head = new_head_commit; + branch.updated_timestamp_ms = gitbutler_time::time::now_ms(); + vb_state.set_branch(branch.clone())?; + + crate::integration::update_workspace_commit(&vb_state, ctx) + .context("failed to update gitbutler workspace")?; + + Ok(branch) +} + +fn inner_undo_commit( + repository: &git2::Repository, + branch_head_commit: git2::Oid, + commit_to_remove: git2::Oid, + succeeding_rebases: bool, +) -> Result { + let commit_to_remove = repository.find_commit(commit_to_remove)?; + + if commit_to_remove.is_conflicted() { + bail!("Can not undo a conflicted commit"); + } + + // if commit is the head, just set head to the parent + if branch_head_commit == commit_to_remove.id() { + return Ok(commit_to_remove.parent(0)?.id()); + }; + + let commits_to_rebase = + repository.l(branch_head_commit, LogUntil::Commit(commit_to_remove.id()))?; + + let new_head = cherry_rebase_group( + repository, + commit_to_remove.parent_id(0)?, + &commits_to_rebase, + succeeding_rebases, + )?; + + Ok(new_head) +} + +#[cfg(test)] +mod test { + #[cfg(test)] + mod inner_undo_commit { + use gitbutler_commit::commit_ext::CommitExt as _; + use gitbutler_repo::rebase::gitbutler_merge_commits; + use gitbutler_testsupport::testing_repository::{ + assert_commit_tree_matches, TestingRepository, + }; + + use crate::undo_commit::inner_undo_commit; + + #[test] + fn undoing_conflicted_commit_errors() { + let test_repository = TestingRepository::open(); + + let a = test_repository.commit_tree(None, &[("foo.txt", "foo")]); + let b = test_repository.commit_tree(Some(&a), &[("bar.txt", "bar")]); + let c = test_repository.commit_tree(Some(&a), &[("bar.txt", "baz")]); + + let conflicted_commit = + gitbutler_merge_commits(&test_repository.repository, b, c, "", "").unwrap(); + + // Branch looks like "A -> ConflictedCommit" + + let result = inner_undo_commit( + &test_repository.repository, + conflicted_commit.id(), + conflicted_commit.id(), + true, + ); + + assert!( + result.is_err(), + "Should error when trying to undo a conflicted commit" + ); + } + + #[test] + fn undoing_head_commit() { + let test_repository = TestingRepository::open(); + + let a = test_repository.commit_tree(None, &[("foo.txt", "foo")]); + let b = test_repository.commit_tree(Some(&a), &[("bar.txt", "bar")]); + let c = test_repository.commit_tree(Some(&b), &[("baz.txt", "baz")]); + + let new_head = + inner_undo_commit(&test_repository.repository, c.id(), c.id(), true).unwrap(); + + assert_eq!(new_head, b.id(), "The new head should be C's parent"); + } + + #[test] + fn undoing_commits_may_create_conflicts() { + let test_repository = TestingRepository::open(); + + let a = test_repository.commit_tree(None, &[("foo.txt", "foo")]); + let b = test_repository.commit_tree(Some(&a), &[("foo.txt", "bar")]); + let c = test_repository.commit_tree(Some(&b), &[("foo.txt", "baz")]); + + // By dropping the "B" commit, we're effectively cherry-picking + // C onto A, which in tern is merge trees of: + // Base: B (content bar) + // Ours: A (content foo) + // Theirs: C (content baz) + // + // As the theirs and ours both are different to the base, it ends up + // conflicted. + let new_head = + inner_undo_commit(&test_repository.repository, c.id(), b.id(), true).unwrap(); + + let new_head_commit: git2::Commit = + test_repository.repository.find_commit(new_head).unwrap(); + + assert!(new_head_commit.is_conflicted(), "Should be conflicted"); + + assert_commit_tree_matches( + &test_repository.repository, + &new_head_commit, + &[ + (".auto-resolution/foo.txt", b"foo"), + (".conflict-base-0/foo.txt", b"bar"), // B is the base + (".conflict-side-0/foo.txt", b"foo"), // "Ours" is A + (".conflict-side-1/foo.txt", b"baz"), // "Theirs" is C + ], + ); + + assert_eq!( + new_head_commit.parent_id(0).unwrap(), + a.id(), + "A should be C prime's parent" + ); + } + } +} diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index e06bed322..ac720c720 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -1642,57 +1642,6 @@ pub(crate) fn insert_blank_commit( Ok(()) } -// remove a commit in a branch by rebasing all commits _except_ for it onto it's parent -// if successful, it will update the branch head to the new head commit -pub(crate) fn undo_commit( - ctx: &CommandContext, - branch_id: BranchId, - commit_oid: git2::Oid, -) -> Result { - let vb_state = ctx.project().virtual_branches(); - - let mut branch = vb_state.get_branch_in_workspace(branch_id)?; - let commit = ctx - .repository() - .find_commit(commit_oid) - .context("failed to find commit")?; - - if commit.is_conflicted() { - bail!("Can not undo a conflicted commit"); - } - - let new_commit_oid; - - if branch.head == commit_oid { - // if commit is the head, just set head to the parent - new_commit_oid = commit.parent(0).context("failed to find parent")?.id(); - } else { - // if commit is not the head, rebase all commits above it onto it's parent - let parent_commit_oid = commit.parent(0).context("failed to find parent")?.id(); - - match cherry_rebase(ctx, parent_commit_oid, commit_oid, branch.head) { - Ok(Some(new_head)) => { - new_commit_oid = new_head; - } - Ok(None) => bail!("no rebase happened"), - Err(err) => { - return Err(err).context("rebase failed"); - } - } - } - - if new_commit_oid != commit_oid { - branch.head = new_commit_oid; - branch.updated_timestamp_ms = gitbutler_time::time::now_ms(); - vb_state.set_branch(branch.clone())?; - - crate::integration::update_workspace_commit(&vb_state, ctx) - .context("failed to update gitbutler workspace")?; - } - - Ok(branch) -} - /// squashes a commit from a virtual branch into its parent. pub(crate) fn squash( ctx: &CommandContext,