From e1ee69a348512af68cef76478c842460383c9b5a Mon Sep 17 00:00:00 2001 From: Nikita Galaiko Date: Wed, 10 Jan 2024 12:37:23 +0100 Subject: [PATCH] fix integrating fully merge branch while being behind --- gitbutler-app/src/virtual_branches/base.rs | 106 ++++++++--------- gitbutler-app/tests/virtual_branches.rs | 126 ++++++++++++++++++++- 2 files changed, 173 insertions(+), 59 deletions(-) diff --git a/gitbutler-app/src/virtual_branches/base.rs b/gitbutler-app/src/virtual_branches/base.rs index 40f262e5f..d7244c73b 100644 --- a/gitbutler-app/src/virtual_branches/base.rs +++ b/gitbutler-app/src/virtual_branches/base.rs @@ -315,50 +315,61 @@ pub fn update_base_branch( branch.head, branch.id ))?; + let result_integrated_detected = + |mut branch: branch::Branch| -> Result> { + // branch head tree is the same as the new target tree. + // meaning we can safely use the new target commit as the branch head. + + branch.head = new_target_commit.id(); + + // it also means that the branch is fully integrated into the target. + // disconnect it from the upstream + branch.upstream = None; + branch.upstream_head = None; + + let non_commited_files = diff::trees( + &project_repository.git_repository, + &branch_head_tree, + &branch_tree, + )?; + if non_commited_files.is_empty() { + // if there are no commited files, then the branch is fully merged + // and we can delete it. + branch_writer.delete(&branch)?; + project_repository.delete_branch_reference(&branch)?; + Ok(None) + } else { + branch_writer.write(&mut branch)?; + Ok(Some(branch)) + } + }; + if branch_head_tree.id() == new_target_tree.id() { - // branch head tree is the same as the new target tree. - // meaning we can safely use the new target commit as the branch head. - - branch.head = new_target_commit.id(); - - // it also means that the branch is fully integrated into the target. - // disconnect it from the upstream - branch.upstream = None; - branch.upstream_head = None; - - let non_commited_files = diff::trees( - &project_repository.git_repository, - &branch_head_tree, - &branch_tree, - )?; - if non_commited_files.is_empty() { - // if there are no commited files, then the branch is fully merged - // and we can delete it. - branch_writer.delete(&branch)?; - project_repository.delete_branch_reference(&branch)?; - return Ok(None); - } - - branch_writer.write(&mut branch)?; - return Ok(Some(branch)); + return result_integrated_detected(branch); } // try to merge branch head with new target - let mut branch_merge_index = repo + let mut branch_tree_merge_index = repo .merge_trees(&old_target_tree, &branch_tree, &new_target_tree) .context(format!("failed to merge trees for branch {}", branch.id))?; - if branch_merge_index.has_conflicts() { + if branch_tree_merge_index.has_conflicts() { // branch tree conflicts with new target, unapply branch for now. we'll handle it later, when user applies it back. branch.applied = false; branch_writer.write(&mut branch)?; return Ok(Some(branch)); } + let branch_merge_index_tree_oid = branch_tree_merge_index.write_tree_to(repo)?; + + if branch_merge_index_tree_oid == new_target_tree.id() { + return result_integrated_detected(branch); + } + if branch.head == target.sha { // there are no commits on the branch, so we can just update the head to the new target and calculate the new tree branch.head = new_target_commit.id(); - branch.tree = branch_merge_index.write_tree_to(repo)?; + branch.tree = branch_merge_index_tree_oid; branch_writer.write(&mut branch)?; return Ok(Some(branch)); } @@ -388,7 +399,7 @@ pub fn update_base_branch( let ok_with_force_push = project_repository.project().ok_with_force_push; - if branch.upstream.is_some() && !ok_with_force_push { + let result_merge = |mut branch: branch::Branch| -> Result> { // branch was pushed to upstream, and user doesn't like force pushing. // create a merge commit to avoid the need of force pushing then. let branch_head_merge_tree = repo @@ -412,9 +423,13 @@ pub fn update_base_branch( .context("failed to commit merge")?; branch.head = new_target_head; - branch.tree = branch_merge_index.write_tree_to(repo)?; + branch.tree = branch_merge_index_tree_oid; branch_writer.write(&mut branch)?; - return Ok(Some(branch)); + Ok(Some(branch)) + }; + + if branch.upstream.is_some() && !ok_with_force_push { + return result_merge(branch); } // branch was not pushed to upstream yet. attempt a rebase, @@ -461,7 +476,7 @@ pub fn update_base_branch( // rebase worked out, rewrite the branch head rebase.finish(None).context("failed to finish rebase")?; branch.head = last_rebase_head; - branch.tree = branch_merge_index.write_tree_to(repo)?; + branch.tree = branch_merge_index_tree_oid; branch_writer.write(&mut branch)?; return Ok(Some(branch)); } @@ -469,32 +484,7 @@ pub fn update_base_branch( // rebase failed, do a merge commit rebase.abort().context("failed to abort rebase")?; - // get tree from merge_tree_oid - let merge_tree = repo - .find_tree(branch_head_merge_tree_oid) - .context("failed to find tree")?; - - // commit the merge tree oid - let new_branch_head = project_repository - .commit( - user, - format!( - "Merged {}/{} into {}", - target.branch.remote(), - target.branch.branch(), - branch.name - ) - .as_str(), - &merge_tree, - &[&branch_head_commit, &new_target_commit], - signing_key, - ) - .context("failed to commit merge")?; - - branch.head = new_branch_head; - branch.tree = branch_merge_index.write_tree_to(repo)?; - branch_writer.write(&mut branch)?; - Ok(Some(branch)) + result_merge(branch) }, ) .collect::>>()? diff --git a/gitbutler-app/tests/virtual_branches.rs b/gitbutler-app/tests/virtual_branches.rs index 42698e52a..5ea0f2312 100644 --- a/gitbutler-app/tests/virtual_branches.rs +++ b/gitbutler-app/tests/virtual_branches.rs @@ -1985,6 +1985,71 @@ mod update_base_branch { assert_eq!(branches.len(), 0); } } + + #[tokio::test] + async fn integrate_work_while_being_behind() { + let Test { + repository, + project_id, + controller, + .. + } = Test::default(); + + // make sure we have an undiscovered commit in the remote branch + { + fs::write(repository.path().join("file.txt"), "first").unwrap(); + let first_commit_oid = repository.commit_all("first"); + fs::write(repository.path().join("file.txt"), "second").unwrap(); + repository.commit_all("second"); + repository.push(); + repository.reset_hard(Some(first_commit_oid)); + } + + controller + .set_base_branch(&project_id, &"refs/remotes/origin/master".parse().unwrap()) + .await + .unwrap(); + + let branch_id = controller + .create_virtual_branch(&project_id, &branch::BranchCreateRequest::default()) + .await + .unwrap(); + + { + // open pr + fs::write(repository.path().join("file2.txt"), "new file").unwrap(); + controller + .create_commit(&project_id, &branch_id, "second", None, false) + .await + .unwrap(); + controller + .push_virtual_branch(&project_id, &branch_id, false) + .await + .unwrap(); + } + + controller + .unapply_virtual_branch(&project_id, &branch_id) + .await + .unwrap(); + + { + // merge pr + let branch = + controller.list_virtual_branches(&project_id).await.unwrap()[0].clone(); + repository.merge(&branch.upstream.as_ref().unwrap().name); + repository.fetch(); + } + + { + // fetch remote + controller.update_base_branch(&project_id).await.unwrap(); + + // just removes integrated branch + let branches = controller.list_virtual_branches(&project_id).await.unwrap(); + assert_eq!(branches.len(), 0); + } + } } mod applied_branch { @@ -2881,7 +2946,6 @@ mod update_base_branch { .unwrap(); let branch_id = { - // make a branch that conflicts with the remote branch, but doesn't know about it yet let branch_id = controller .create_virtual_branch(&project_id, &branch::BranchCreateRequest::default()) .await @@ -3071,6 +3135,66 @@ mod update_base_branch { assert_eq!(branches.len(), 0); } } + + #[tokio::test] + async fn integrate_work_while_being_behind() { + let Test { + repository, + project_id, + controller, + .. + } = Test::default(); + + // make sure we have an undiscovered commit in the remote branch + { + fs::write(repository.path().join("file.txt"), "first").unwrap(); + let first_commit_oid = repository.commit_all("first"); + fs::write(repository.path().join("file.txt"), "second").unwrap(); + repository.commit_all("second"); + repository.push(); + repository.reset_hard(Some(first_commit_oid)); + } + + controller + .set_base_branch(&project_id, &"refs/remotes/origin/master".parse().unwrap()) + .await + .unwrap(); + + let branch_id = controller + .create_virtual_branch(&project_id, &branch::BranchCreateRequest::default()) + .await + .unwrap(); + + { + // open pr + fs::write(repository.path().join("file2.txt"), "new file").unwrap(); + controller + .create_commit(&project_id, &branch_id, "second", None, false) + .await + .unwrap(); + controller + .push_virtual_branch(&project_id, &branch_id, false) + .await + .unwrap(); + } + + { + // merge pr + let branch = + controller.list_virtual_branches(&project_id).await.unwrap()[0].clone(); + repository.merge(&branch.upstream.as_ref().unwrap().name); + repository.fetch(); + } + + { + // fetch remote + controller.update_base_branch(&project_id).await.unwrap(); + + // just removes integrated branch + let branches = controller.list_virtual_branches(&project_id).await.unwrap(); + assert_eq!(branches.len(), 0); + } + } } }