Add test reproducing hunk locking problem [do not merge]

crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs

@@ -81,6 +81,7 @@ impl BranchManager<'_> {
         let (applied_statuses, _, _) = get_applied_status(
             self.project_repository,
             &integration_commit.id(),
+            &target_commit.id(),
             virtual_branches,
             None,
         )

crates/gitbutler-branch-actions/src/virtual.rs

@@ -1,3 +1,4 @@
+use git2::ErrorCode;
 use gitbutler_branch::{dedup, BranchUpdateRequest, VirtualBranchesHandle};
 use gitbutler_branch::{dedup_fmt, Branch, BranchCreateRequest, BranchId};
 use gitbutler_branch::{reconcile_claims, BranchOwnershipClaims};
@@ -21,6 +22,7 @@ use std::{
     path::{Path, PathBuf},
     time, vec,
 };
+use tokio::time::Instant;
 
 use anyhow::{anyhow, bail, Context, Result};
 use bstr::{BString, ByteSlice, ByteVec};
@@ -230,6 +232,7 @@ pub fn unapply_ownership(
     project_repository.assure_resolved()?;
 
     let vb_state = project_repository.project().virtual_branches();
+    let default_target = &vb_state.get_default_target()?;
 
     let virtual_branches = vb_state
         .list_branches_in_workspace()
@@ -240,6 +243,7 @@ pub fn unapply_ownership(
     let (applied_statuses, _, _) = get_applied_status(
         project_repository,
         &integration_commit_id,
+        &default_target.sha,
         virtual_branches,
         None,
     )
@@ -1094,13 +1098,101 @@ pub fn get_status_by_branch(
         project_repository,
         // TODO: Keep this optional or update lots of tests?
         integration_commit.unwrap_or(&default_target.sha),
+        &default_target.sha,
         virtual_branches,
         perm,
     )?;
 
     Ok((applied_status, skipped_files, locks))
 }
 
+fn compute_merge_base(
+    project_repository: &ProjectRepository,
+    target_sha: &git2::Oid,
+    virtual_branches: &Vec<Branch>,
+) -> Result<git2::Oid> {
+    let repo = project_repository.repo();
+    let mut merge_base = *target_sha;
+    for branch in virtual_branches {
+        if let Some(last) = project_repository
+            .l(branch.head, LogUntil::Commit(*target_sha))?
+            .last()
+            .map(|id| repo.find_commit(id.to_owned()))
+        {
+            if let Ok(parent) = last?.parent(0) {
+                merge_base = repo.merge_base(parent.id(), merge_base)?;
+            }
+        }
+    }
+    Ok(merge_base)
+}
+
+fn compute_locks(
+    project_repository: &ProjectRepository,
+    integration_commit: &git2::Oid,
+    target_sha: &git2::Oid,
+    base_diffs: &BranchStatus,
+    virtual_branches: &Vec<Branch>,
+) -> Result<HashMap<HunkHash, Vec<HunkLock>>> {
+    let merge_base = compute_merge_base(project_repository, target_sha, virtual_branches)?;
+    let mut locked_hunk_map = HashMap::<HunkHash, Vec<HunkLock>>::new();
+
+    let mut commit_to_branch = HashMap::new();
+    for branch in virtual_branches {
+        for commit in project_repository.log(branch.head, LogUntil::Commit(*target_sha))? {
+            commit_to_branch.insert(commit.id(), branch.id);
+        }
+    }
+
+    for (path, hunks) in base_diffs.clone().into_iter() {
+        for hunk in hunks {
+            let blame = match project_repository.repo().blame(
+                &path,
+                hunk.old_start,
+                hunk.old_start + hunk.old_lines,
+                merge_base,
+                *integration_commit,
+            ) {
+                Ok(blame) => blame,
+                Err(error) => {
+                    if error.code() == ErrorCode::NotFound {
+                        continue;
+                    } else {
+                        return Err(error.into());
+                    }
+                }
+            };
+
+            for blame_hunk in blame.iter() {
+                let commit_id = blame_hunk.orig_commit_id();
+                dbg!(commit_id);
+                if commit_id == *integration_commit || commit_id == *target_sha {
+                    continue;
+                }
+                let hash = Hunk::hash_diff(&hunk.diff_lines);
+                dbg!(hash);
+                let Some(branch_id) = commit_to_branch.get(&commit_id) else {
+                    continue;
+                };
+                dbg!(branch_id);
+
+                let hunk_lock = HunkLock {
+                    branch_id: *branch_id,
+                    commit_id,
+                };
+                dbg!(&hunk_lock);
+                locked_hunk_map
+                    .entry(hash)
+                    .and_modify(|locks| {
+                        locks.push(hunk_lock);
+                    })
+                    .or_insert(vec![hunk_lock]);
+            }
+        }
+    }
+    Ok(locked_hunk_map)
+}
+
 fn new_compute_locks(
     repository: &git2::Repository,
     unstaged_hunks_by_path: &HashMap<PathBuf, Vec<gitbutler_diff::GitHunk>>,
@@ -1196,6 +1288,7 @@ fn new_compute_locks(
 pub(crate) fn get_applied_status(
     project_repository: &ProjectRepository,
     integration_commit: &git2::Oid,
+    target_sha: &git2::Oid,
     mut virtual_branches: Vec<Branch>,
     perm: Option<&mut WorktreeWritePermission>,
 ) -> Result<(
@@ -1235,7 +1328,14 @@ pub(crate) fn get_applied_status(
         .map(|branch| (branch.id, HashMap::new()))
         .collect();
 
-    let locks = new_compute_locks(project_repository.repo(), &base_diffs, &virtual_branches)?;
+    let locks = compute_locks(
+        project_repository,
+        integration_commit,
+        target_sha,
+        &base_diffs,
+        &virtual_branches,
+    )?;
+    // let locks = new_compute_locks(project_repository.repo(), &base_diffs, &virtual_branches)?;
 
     for branch in &mut virtual_branches {
         let old_claims = branch.ownership.claims.clone();
@@ -1627,20 +1727,12 @@ pub(crate) fn write_tree_onto_tree(
                         "failed to apply\n{}\nonto:\n{}",
                         all_diffs.as_bstr(),
                         blob_contents.as_bstr()
-                    ));
-
-                    match blob_contents {
-                        Ok(blob_contents) => {
-                            // create a blob
-                            let new_blob_oid = git_repository.blob(blob_contents.as_bytes())?;
-                            // upsert into the builder
-                            builder.upsert(rel_path, new_blob_oid, filemode);
-                        }
-                        Err(_) => {
-                            // If the patch failed to apply, do nothing, this is handled elsewhere
-                            continue;
-                        }
-                    }
+                    ))?;
+
+                    // create a blob
+                    let new_blob_oid = git_repository.blob(&blob_contents)?;
+                    // upsert into the builder
+                    builder.upsert(rel_path, new_blob_oid, filemode);
                 }
             } else if is_submodule {
                 let mut blob_contents = BString::default();
@@ -2238,6 +2330,7 @@ pub(crate) fn amend(
     let (mut applied_statuses, _, _) = get_applied_status(
         project_repository,
         &integration_commit_id,
+        &default_target.sha,
         virtual_branches,
         None,
     )?;
@@ -2720,11 +2813,13 @@ pub(crate) fn move_commit(
         bail!("branch {target_branch_id} is not among applied branches")
     }
 
+    let default_target = &vb_state.get_default_target()?;
     let integration_commit_id = get_workspace_head(&vb_state, project_repository)?;
 
     let (mut applied_statuses, _, _) = get_applied_status(
         project_repository,
         &integration_commit_id,
+        &default_target.sha,
         applied_branches,
         None,
     )?;
@@ -2933,6 +3028,8 @@ fn update_conflict_markers(
 
 #[cfg(test)]
 mod tests {
+    use git2::ErrorCode;
+
     use super::*;
     #[test]
     fn joined_test() {

crates/gitbutler-branch-actions/tests/virtual_branches/selected_for_changes.rs

@@ -445,3 +445,104 @@ async fn new_locked_hunk_without_modifying_existing() {
     assert_eq!(branches[0].files.len(), 1);
     assert_eq!(branches[1].files.len(), 1);
 }
+
+// This is a temporary test to demonstrate a bug in the hunk locking mechanism.
+//
+// The idea is you introduce change A, then you shift it down in a a different
+// (default) branch with commit B, so that new line numbers no longer
+// intersect. If you then change A again you will have a failure to apply
+// the hunk onto head of branch with commit B.
+//
+// The error contains the following descriptive error message.
+//
+// called `Result::unwrap()` on an `Err` value: failed to apply
+// @@ -10,4 +10,4 @@ line 3
+//  line 4
+//  line 5
+//  line 6
+// -modification 1 to line 7
+// \ No newline at end of file
+// +modification 2 to original line 9
+// \ No newline at end of file
+
+// onto:
+// inserted line 0
+// inserted line 1
+// inserted line 2
+// inserted line 3
+// inserted line 4
+// line 0
+// line 1
+// line 2
+// line 3
+// line 4
+// line 5
+// line 6
+// line 7
+#[tokio::test]
+async fn hunk_locking_confused_by_line_number_shift() {
+    let Test {
+        repository,
+        project,
+        controller,
+        ..
+    } = &Test::default();
+
+    let mut lines = repository.gen_file("file.txt", 9);
+    repository.commit_all("first commit");
+    repository.push();
+
+    controller
+        .set_base_branch(project, &"refs/remotes/origin/master".parse().unwrap())
+        .await
+        .unwrap();
+
+    lines[8] = "modification 1 to line 8".to_string();
+    repository.write_file("file.txt", &lines);
+
+    let (branches, _) = controller.list_virtual_branches(project).await.unwrap();
+    assert_eq!(branches[0].files.len(), 1);
+
+    controller
+        .create_commit(project, branches[0].id, "second commit", None, false)
+        .await
+        .expect("failed to create commit");
+
+    let (branches, _) = controller.list_virtual_branches(project).await.unwrap();
+    assert_eq!(branches[0].files.len(), 0);
+    assert_eq!(branches[0].commits.len(), 1);
+
+    controller
+        .create_virtual_branch(
+            project,
+            &BranchCreateRequest {
+                selected_for_changes: Some(true),
+                ..Default::default()
+            },
+        )
+        .await
+        .unwrap();
+
+    let new_lines: Vec<_> = (0_i32..5_i32)
+        .map(|i| format!("inserted line {}", i))
+        .collect();
+    lines.splice(0..0, new_lines);
+    repository.write_file("file.txt", &lines);
+
+    // Has to be called before `create_commit`
+    let (branches, _) = controller.list_virtual_branches(project).await.unwrap();
+
+    controller
+        .create_commit(project, branches[1].id, "second commit", None, false)
+        .await
+        .expect("failed to create commit");
+
+    assert_eq!(branches[0].commits.len(), 1);
+    assert_eq!(branches[0].commits.len(), 1);
+
+    lines[13] = "modification 2 to original line 8".to_string();
+    repository.write_file("file.txt", &lines);
+
+    // This line makes the test panic.
+    let (branches, _) = controller.list_virtual_branches(project).await.unwrap();
+}

crates/gitbutler-repo/src/repository_ext.rs

@@ -224,8 +224,7 @@ impl RepositoryExt for Repository {
         opts.min_line(min_line as usize)
             .max_line(max_line as usize)
             .newest_commit(newest_commit)
-            .oldest_commit(oldest_commit)
-            .first_parent(true);
+            .oldest_commit(oldest_commit);
         self.blame_file(path, Some(&mut opts))
     }