From 5bb8c25aca7e931bda06ed472a8c91f057ab349f Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Thu, 2 May 2024 17:27:24 +0200 Subject: [PATCH] Fix bug in updating base branch - drop "double lock across branches" test because it can't happen - save correct tree when updating branch states - use correct integration commit when updating base branch --- .../src/virtual_branches/base.rs | 351 +++++++++--------- .../src/virtual_branches/virtual.rs | 2 +- .../suite/virtual_branches/create_commit.rs | 75 ---- .../virtual_branches/update_base_branch.rs | 80 +++- 4 files changed, 257 insertions(+), 251 deletions(-) diff --git a/crates/gitbutler-core/src/virtual_branches/base.rs b/crates/gitbutler-core/src/virtual_branches/base.rs index fdebb0db9..c2f0fa172 100644 --- a/crates/gitbutler-core/src/virtual_branches/base.rs +++ b/crates/gitbutler-core/src/virtual_branches/base.rs @@ -5,7 +5,9 @@ use serde::Serialize; use super::{ branch, errors, - integration::{update_gitbutler_integration, GITBUTLER_INTEGRATION_REFERENCE}, + integration::{ + get_workspace_head, update_gitbutler_integration, GITBUTLER_INTEGRATION_REFERENCE, + }, target, BranchId, RemoteCommit, VirtualBranchHunk, VirtualBranchesHandle, }; use crate::{ @@ -337,14 +339,10 @@ pub fn update_base_branch( .peel_to_commit() .context(format!("failed to peel branch {} to commit", target.branch))?; - // if the target has not changed, do nothing if new_target_commit.id() == target.sha { return Ok(()); } - // ok, target has changed, so now we need to merge it into our current work and update our branches - - // get tree from new target let new_target_tree = new_target_commit .tree() .context("failed to get new target commit tree")?; @@ -358,195 +356,200 @@ pub fn update_base_branch( ))?; let vb_state = VirtualBranchesHandle::new(&project_repository.project().gb_dir()); + let integration_commit = get_workspace_head(&vb_state, project_repository)?; // try to update every branch - let updated_vbranches = super::get_status_by_branch(project_repository, None)? - .0 - .into_iter() - .map(|(branch, _)| branch) - .map( - |mut branch: branch::Branch| -> Result> { - let branch_tree = repo.find_tree(branch.tree)?; + let updated_vbranches = + super::get_status_by_branch(project_repository, Some(&integration_commit))? + .0 + .into_iter() + .map(|(branch, _)| branch) + .map( + |mut branch: branch::Branch| -> Result> { + let branch_tree = repo.find_tree(branch.tree)?; - let branch_head_commit = repo.find_commit(branch.head).context(format!( - "failed to find commit {} for branch {}", - branch.head, branch.id - ))?; - let branch_head_tree = branch_head_commit.tree().context(format!( - "failed to find tree for commit {} for branch {}", - branch.head, branch.id - ))?; + let branch_head_commit = repo.find_commit(branch.head).context(format!( + "failed to find commit {} for branch {}", + branch.head, branch.id + ))?; + let branch_head_tree = branch_head_commit.tree().context(format!( + "failed to find tree for commit {} for 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. + 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. + vb_state.remove_branch(branch.id)?; + project_repository.delete_branch_reference(&branch)?; + Ok(None) + } else { + vb_state.set_branch(branch.clone())?; + Ok(Some(branch)) + } + }; + + if branch_head_tree.id() == new_target_tree.id() { + return result_integrated_detected(branch); + } + + // try to merge branch head with new target + 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_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; + vb_state.set_branch(branch.clone())?; + 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_tree_oid; + vb_state.set_branch(branch.clone())?; + return Ok(Some(branch)); + } - // 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 mut branch_head_merge_index = repo + .merge_trees(&old_target_tree, &branch_head_tree, &new_target_tree) + .context(format!( + "failed to merge head tree for branch {}", + branch.id + ))?; - 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. - vb_state.remove_branch(branch.id)?; - project_repository.delete_branch_reference(&branch)?; - Ok(None) - } else { + if branch_head_merge_index.has_conflicts() { + // branch commits conflict with new target, make sure the branch is + // unapplied. conflicts witll be dealt with when applying it back. + branch.applied = false; + vb_state.set_branch(branch.clone())?; + return Ok(Some(branch)); + } + + // branch commits do not conflict with new target, so lets merge them + let branch_head_merge_tree_oid = branch_head_merge_index + .write_tree_to(repo) + .context(format!( + "failed to write head merge index for {}", + branch.id + ))?; + + let ok_with_force_push = project_repository.project().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 + .find_tree(branch_head_merge_tree_oid) + .context("failed to find tree")?; + + let new_target_head = project_repository + .commit( + user, + format!( + "Merged {}/{} into {}", + target.branch.remote(), + target.branch.branch(), + branch.name + ) + .as_str(), + &branch_head_merge_tree, + &[&branch_head_commit, &new_target_commit], + signing_key, + ) + .context("failed to commit merge")?; + + branch.head = new_target_head; + branch.tree = branch_merge_index_tree_oid; vb_state.set_branch(branch.clone())?; Ok(Some(branch)) - } - }; + }; - if branch_head_tree.id() == new_target_tree.id() { - return result_integrated_detected(branch); - } + if branch.upstream.is_some() && !ok_with_force_push { + return result_merge(branch); + } - // try to merge branch head with new target - 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_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; - vb_state.set_branch(branch.clone())?; - 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_tree_oid; - vb_state.set_branch(branch.clone())?; - return Ok(Some(branch)); - } - - let mut branch_head_merge_index = repo - .merge_trees(&old_target_tree, &branch_head_tree, &new_target_tree) - .context(format!( - "failed to merge head tree for branch {}", - branch.id - ))?; - - if branch_head_merge_index.has_conflicts() { - // branch commits conflict with new target, make sure the branch is - // unapplied. conflicts witll be dealt with when applying it back. - branch.applied = false; - vb_state.set_branch(branch.clone())?; - return Ok(Some(branch)); - } - - // branch commits do not conflict with new target, so lets merge them - let branch_head_merge_tree_oid = branch_head_merge_index - .write_tree_to(repo) - .context(format!( - "failed to write head merge index for {}", - branch.id - ))?; - - let ok_with_force_push = project_repository.project().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 - .find_tree(branch_head_merge_tree_oid) - .context("failed to find tree")?; - - let new_target_head = project_repository - .commit( - user, - format!( - "Merged {}/{} into {}", - target.branch.remote(), - target.branch.branch(), - branch.name - ) - .as_str(), - &branch_head_merge_tree, - &[&branch_head_commit, &new_target_commit], - signing_key, + // branch was not pushed to upstream yet. attempt a rebase, + let (_, committer) = project_repository.git_signatures(user)?; + let mut rebase_options = git2::RebaseOptions::new(); + rebase_options.quiet(true); + rebase_options.inmemory(true); + let mut rebase = repo + .rebase( + Some(branch.head), + Some(new_target_commit.id()), + None, + Some(&mut rebase_options), ) - .context("failed to commit merge")?; + .context("failed to rebase")?; - branch.head = new_target_head; - branch.tree = branch_merge_index_tree_oid; - vb_state.set_branch(branch.clone())?; - Ok(Some(branch)) - }; + let mut rebase_success = true; + // check to see if these commits have already been pushed + let mut last_rebase_head = branch.head; + while rebase.next().is_some() { + let index = rebase + .inmemory_index() + .context("failed to get inmemory index")?; + if index.has_conflicts() { + rebase_success = false; + break; + } - if branch.upstream.is_some() && !ok_with_force_push { - return result_merge(branch); - } - - // branch was not pushed to upstream yet. attempt a rebase, - let (_, committer) = project_repository.git_signatures(user)?; - let mut rebase_options = git2::RebaseOptions::new(); - rebase_options.quiet(true); - rebase_options.inmemory(true); - let mut rebase = repo - .rebase( - Some(branch.head), - Some(new_target_commit.id()), - None, - Some(&mut rebase_options), - ) - .context("failed to rebase")?; - - let mut rebase_success = true; - // check to see if these commits have already been pushed - let mut last_rebase_head = branch.head; - while rebase.next().is_some() { - let index = rebase - .inmemory_index() - .context("failed to get inmemory index")?; - if index.has_conflicts() { - rebase_success = false; - break; + if let Ok(commit_id) = rebase.commit(None, &committer.clone().into(), None) + { + last_rebase_head = commit_id.into(); + } else { + rebase_success = false; + break; + } } - if let Ok(commit_id) = rebase.commit(None, &committer.clone().into(), None) { - last_rebase_head = commit_id.into(); - } else { - rebase_success = false; - break; + if rebase_success { + // 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_tree_oid; + vb_state.set_branch(branch.clone())?; + return Ok(Some(branch)); } - } - if rebase_success { - // 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_tree_oid; - vb_state.set_branch(branch.clone())?; - return Ok(Some(branch)); - } + // rebase failed, do a merge commit + rebase.abort().context("failed to abort rebase")?; - // rebase failed, do a merge commit - rebase.abort().context("failed to abort rebase")?; - - result_merge(branch) - }, - ) - .collect::>>()? - .into_iter() - .flatten() - .collect::>(); + result_merge(branch) + }, + ) + .collect::>>()? + .into_iter() + .flatten() + .collect::>(); // ok, now all the problematic branches have been unapplied // now we calculate and checkout new tree for the working directory diff --git a/crates/gitbutler-core/src/virtual_branches/virtual.rs b/crates/gitbutler-core/src/virtual_branches/virtual.rs index 98130a004..bd5e70109 100644 --- a/crates/gitbutler-core/src/virtual_branches/virtual.rs +++ b/crates/gitbutler-core/src/virtual_branches/virtual.rs @@ -1963,7 +1963,7 @@ fn get_applied_status( if !project_repository.is_resolving() { let vb_state = VirtualBranchesHandle::new(&project_repository.project().gb_dir()); for (vbranch, files) in &mut hunks_by_branch { - vbranch.tree = write_tree(project_repository, integration_commit, files)?; + vbranch.tree = write_tree(project_repository, &vbranch.head, files)?; vb_state .set_branch(vbranch.clone()) .context(format!("failed to write virtual branch {}", vbranch.name))?; diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs b/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs index 164c4ac9b..48c32729d 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/create_commit.rs @@ -2,7 +2,6 @@ use gitbutler_core::{ id::Id, virtual_branches::{Branch, VirtualBranch}, }; -use uuid::Uuid; use super::*; @@ -280,80 +279,6 @@ async fn should_double_lock() { assert_eq!(locks[1].commit_id, commit_2); } -// This test only validates that locks are detected across virtual branches, it does -// not make any assertions about how said hunk is handled or what branch owns it. -// TODO(mg): Figure out why we can't reduce line count down to three? -#[tokio::test] -async fn should_double_lock_across_branches() { - let Test { - project_id, - controller, - repository, - .. - } = &Test::default(); - - let mut lines = gen_file(repository, "file.txt", 5); - commit_and_push_initial(repository); - - controller - .set_base_branch(project_id, &"refs/remotes/origin/master".parse().unwrap()) - .await - .unwrap(); - - let branch_1_id = controller - .create_virtual_branch(project_id, &branch::BranchCreateRequest::default()) - .await - .unwrap(); - - lines[0] = "change 1".to_string(); - write_file(repository, "file.txt", &lines); - - let commit_1 = controller - .create_commit(project_id, &branch_1_id, "commit 1", None, false) - .await - .unwrap(); - - // TODO(mg): Make `create_commit` clean up ownership of committed hunks. - // TODO(mg): Needed because next hunk overlaps with previous ownership. - get_virtual_branch(controller, project_id, branch_1_id).await; - - let branch_2_id = controller - .create_virtual_branch( - project_id, - &branch::BranchCreateRequest { - selected_for_changes: Some(true), - ..Default::default() - }, - ) - .await - .unwrap(); - - lines[4] = "change 2".to_string(); - write_file(repository, "file.txt", &lines); - - let commit_2 = controller - .create_commit(project_id, &branch_2_id, "commit 2", None, false) - .await - .unwrap(); - - lines[2] = "change3".to_string(); - write_file(repository, "file.txt", &lines); - - let branch_1 = get_virtual_branch(controller, project_id, branch_1_id).await; - let locks = &branch_1.files[0].hunks[0].locked_to.clone().unwrap(); - assert_eq!(locks.len(), 2); - assert_eq!(locks[0].commit_id, commit_1); - assert_eq!(locks[1].commit_id, commit_2); - assert_eq!( - locks[0].branch_id, - Uuid::parse_str(&branch_1_id.to_string()).unwrap() - ); - assert_eq!( - locks[1].branch_id, - Uuid::parse_str(&branch_2_id.to_string()).unwrap() - ); -} - fn write_file(repository: &TestProject, path: &str, lines: &[String]) { fs::write(repository.path().join(path), lines.join("\n")).unwrap() } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/update_base_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/update_base_branch.rs index 216773d72..550e0f9a2 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/update_base_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/update_base_branch.rs @@ -1576,7 +1576,7 @@ mod applied_branch { .unwrap(); controller - .create_commit(project_id, &branch_id, "third", None, false) + .create_commit(project_id, &branch_id, "fourth", None, false) .await .unwrap(); @@ -1929,4 +1929,82 @@ mod applied_branch { assert_eq!(branches.len(), 0); } } + + // Ensure integrated branch is dropped and that a commit on another + // branch does not lead to the introduction of gost/phantom diffs. + #[tokio::test] + async fn should_not_get_confused_by_commits_in_other_branches() { + let Test { + repository, + project_id, + controller, + .. + } = &Test::default(); + + fs::write(repository.path().join("file-1.txt"), "one").unwrap(); + let first_commit_oid = repository.commit_all("first"); + fs::write(repository.path().join("file-2.txt"), "two").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_1_id = controller + .create_virtual_branch(project_id, &branch::BranchCreateRequest::default()) + .await + .unwrap(); + + fs::write(repository.path().join("file-3.txt"), "three").unwrap(); + controller + .create_commit(project_id, &branch_1_id, "third", None, false) + .await + .unwrap(); + + let branch_2_id = controller + .create_virtual_branch( + project_id, + &branch::BranchCreateRequest { + selected_for_changes: Some(true), + ..Default::default() + }, + ) + .await + .unwrap(); + + fs::write(repository.path().join("file-4.txt"), "four").unwrap(); + + controller + .create_commit(project_id, &branch_2_id, "fourth", None, false) + .await + .unwrap(); + + controller + .push_virtual_branch(project_id, &branch_2_id, false, None) + .await + .unwrap(); + + let branch = controller + .list_virtual_branches(project_id) + .await + .unwrap() + .0[1] + .clone(); + + repository.merge(&branch.upstream.as_ref().unwrap().name); + repository.fetch(); + + // TODO(mg): Figure out why test fails without listing first. + controller.list_virtual_branches(project_id).await.unwrap(); + controller.update_base_branch(project_id).await.unwrap(); + + // Verify we have only the first branch left, and that no files + // are present. + let (branches, _) = controller.list_virtual_branches(project_id).await.unwrap(); + assert_eq!(branches.len(), 1); + assert_eq!(branches[0].files.len(), 0); + } }