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

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)?
+    let updated_vbranches =
-        .0
+        super::get_status_by_branch(project_repository, Some(&integration_commit))?
-        .into_iter()
+            .0
-        .map(|(branch, _)| branch)
+            .into_iter()
-        .map(
+            .map(|(branch, _)| branch)
-            |mut branch: branch::Branch| -> Result<Option<branch::Branch>> {
+            .map(
-                let branch_tree = repo.find_tree(branch.tree)?;
+                |mut branch: branch::Branch| -> Result<Option<branch::Branch>> {
+                    let branch_tree = repo.find_tree(branch.tree)?;
 
-                let branch_head_commit = repo.find_commit(branch.head).context(format!(
+                    let branch_head_commit = repo.find_commit(branch.head).context(format!(
-                    "failed to find commit {} for branch {}",
+                        "failed to find commit {} for branch {}",
-                    branch.head, branch.id
+                        branch.head, branch.id
-                ))?;
+                    ))?;
-                let branch_head_tree = branch_head_commit.tree().context(format!(
+                    let branch_head_tree = branch_head_commit.tree().context(format!(
-                    "failed to find tree for commit {} for branch {}",
+                        "failed to find tree for commit {} for branch {}",
-                    branch.head, branch.id
+                        branch.head, branch.id
-                ))?;
+                    ))?;
 
-                let result_integrated_detected =
+                    let result_integrated_detected =
-                    |mut branch: branch::Branch| -> Result<Option<branch::Branch>> {
+                        |mut branch: branch::Branch| -> Result<Option<branch::Branch>> {
-                        // branch head tree is the same as the new target tree.
+                            // branch head tree is the same as the new target tree.
-                        // meaning we can safely use the new target commit as the branch head.
+                            // 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.
+                    let mut branch_head_merge_index = repo
-                        // disconnect it from the upstream
+                        .merge_trees(&old_target_tree, &branch_head_tree, &new_target_tree)
-                        branch.upstream = None;
+                        .context(format!(
-                        branch.upstream_head = None;
+                            "failed to merge head tree for branch {}",
+                            branch.id
+                        ))?;
 
-                        let non_commited_files = diff::trees(
+                    if branch_head_merge_index.has_conflicts() {
-                            &project_repository.git_repository,
+                        // branch commits conflict with new target, make sure the branch is
-                            &branch_head_tree,
+                        // unapplied. conflicts witll be dealt with when applying it back.
-                            &branch_tree,
+                        branch.applied = false;
-                        )?;
+                        vb_state.set_branch(branch.clone())?;
-                        if non_commited_files.is_empty() {
+                        return Ok(Some(branch));
-                            // if there are no commited files, then the branch is fully merged
+                    }
-                            // and we can delete it.
+
-                            vb_state.remove_branch(branch.id)?;
+                    // branch commits do not conflict with new target, so lets merge them
-                            project_repository.delete_branch_reference(&branch)?;
+                    let branch_head_merge_tree_oid = branch_head_merge_index
-                            Ok(None)
+                        .write_tree_to(repo)
-                        } else {
+                        .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<Option<branch::Branch>> {
+                            // 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() {
+                    if branch.upstream.is_some() && !ok_with_force_push {
-                    return result_integrated_detected(branch);
+                        return result_merge(branch);
-                }
+                    }
 
-                // try to merge branch head with new target
+                    // branch was not pushed to upstream yet. attempt a rebase,
-                let mut branch_tree_merge_index = repo
+                    let (_, committer) = project_repository.git_signatures(user)?;
-                    .merge_trees(&old_target_tree, &branch_tree, &new_target_tree)
+                    let mut rebase_options = git2::RebaseOptions::new();
-                    .context(format!("failed to merge trees for branch {}", branch.id))?;
+                    rebase_options.quiet(true);
-
+                    rebase_options.inmemory(true);
-                if branch_tree_merge_index.has_conflicts() {
+                    let mut rebase = repo
-                    // branch tree conflicts with new target, unapply branch for now. we'll handle it later, when user applies it back.
+                        .rebase(
-                    branch.applied = false;
+                            Some(branch.head),
-                    vb_state.set_branch(branch.clone())?;
+                            Some(new_target_commit.id()),
-                    return Ok(Some(branch));
+                            None,
-                }
+                            Some(&mut rebase_options),
-
-                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<Option<branch::Branch>> {
-                    // 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")?;
+                        .context("failed to rebase")?;
 
-                    branch.head = new_target_head;
+                    let mut rebase_success = true;
-                    branch.tree = branch_merge_index_tree_oid;
+                    // check to see if these commits have already been pushed
-                    vb_state.set_branch(branch.clone())?;
+                    let mut last_rebase_head = branch.head;
-                    Ok(Some(branch))
+                    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 {
+                        if let Ok(commit_id) = rebase.commit(None, &committer.clone().into(), None)
-                    return result_merge(branch);
+                        {
-                }
+                            last_rebase_head = commit_id.into();
-
+                        } else {
-                // branch was not pushed to upstream yet. attempt a rebase,
+                            rebase_success = false;
-                let (_, committer) = project_repository.git_signatures(user)?;
+                            break;
-                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) {
+                    if rebase_success {
-                        last_rebase_head = commit_id.into();
+                        // rebase worked out, rewrite the branch head
-                    } else {
+                        rebase.finish(None).context("failed to finish rebase")?;
-                        rebase_success = false;
+                        branch.head = last_rebase_head;
-                        break;
+                        branch.tree = branch_merge_index_tree_oid;
+                        vb_state.set_branch(branch.clone())?;
+                        return Ok(Some(branch));
                     }
-                }
 
-                if rebase_success {
+                    // rebase failed, do a merge commit
-                    // rebase worked out, rewrite the branch head
+                    rebase.abort().context("failed to abort rebase")?;
-                    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
+                    result_merge(branch)
-                rebase.abort().context("failed to abort rebase")?;
+                },
-
+            )
-                result_merge(branch)
+            .collect::<Result<Vec<_>>>()?
-            },
+            .into_iter()
-        )
+            .flatten()
-        .collect::<Result<Vec<_>>>()?
+            .collect::<Vec<_>>();
-        .into_iter()
-        .flatten()
-        .collect::<Vec<_>>();
 
     // ok, now all the problematic branches have been unapplied
     // now we calculate and checkout new tree for the working directory

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))?;

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()
 }

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);
+    }
 }