From c4db79e789dcf99b36b80eb06a49c8e66a0776bb Mon Sep 17 00:00:00 2001 From: Caleb Owens Date: Tue, 9 Jul 2024 20:12:05 +0200 Subject: [PATCH 1/3] Remove nolonger valid tests --- .../tests/virtual_branches/create_commit.rs | 150 ------------------ 1 file changed, 150 deletions(-) diff --git a/crates/gitbutler-virtual/tests/virtual_branches/create_commit.rs b/crates/gitbutler-virtual/tests/virtual_branches/create_commit.rs index 155697a94..2ed40a032 100644 --- a/crates/gitbutler-virtual/tests/virtual_branches/create_commit.rs +++ b/crates/gitbutler-virtual/tests/virtual_branches/create_commit.rs @@ -58,106 +58,6 @@ async fn should_lock_updated_hunks() { } } -#[tokio::test] -async fn should_not_lock_disjointed_hunks() { - let Test { - project, - controller, - repository, - .. - } = &Test::default(); - - let mut lines: Vec<_> = (0_i32..24_i32).map(|i| format!("line {}", i)).collect(); - repository.write_file("file.txt", &lines); - repository.commit_all("my commit"); - repository.push(); - - controller - .set_base_branch(project, &"refs/remotes/origin/master".parse().unwrap()) - .await - .unwrap(); - - let branch_id = controller - .create_virtual_branch(project, &branch::BranchCreateRequest::default()) - .await - .unwrap(); - - { - // new hunk in the middle of the file - lines[12] = "commited stuff".to_string(); - repository.write_file("file.txt", &lines); - let branch = get_virtual_branch(controller, project, branch_id).await; - assert_eq!(branch.files.len(), 1); - assert_eq!(branch.files[0].path.display().to_string(), "file.txt"); - assert_eq!(branch.files[0].hunks.len(), 1); - assert!(!branch.files[0].hunks[0].locked); - } - - controller - .create_commit(project, branch_id, "test commit", None, false) - .await - .unwrap(); - controller - .push_virtual_branch(project, branch_id, false, None) - .await - .unwrap(); - - { - // hunk before the commited part is not locked - let mut changed_lines = lines.clone(); - changed_lines[8] = "updated line".to_string(); - repository.write_file("file.txt", &changed_lines); - let branch = get_virtual_branch(controller, project, branch_id).await; - assert_eq!(branch.files.len(), 1); - assert_eq!(branch.files[0].path.display().to_string(), "file.txt"); - assert_eq!(branch.files[0].hunks.len(), 1); - assert!(!branch.files[0].hunks[0].locked); - // cleanup - repository.write_file("file.txt", &lines); - } - { - // hunk after the commited part is not locked - let mut changed_lines = lines.clone(); - changed_lines[16] = "updated line".to_string(); - repository.write_file("file.txt", &changed_lines); - let branch = get_virtual_branch(controller, project, branch_id).await; - assert_eq!(branch.files.len(), 1); - assert_eq!(branch.files[0].path.display().to_string(), "file.txt"); - assert_eq!(branch.files[0].hunks.len(), 1); - assert!(!branch.files[0].hunks[0].locked); - // cleanup - repository.write_file("file.txt", &lines); - } - { - // hunk before the commited part but with overlapping context - let mut changed_lines = lines.clone(); - changed_lines[10] = "updated line".to_string(); - repository.write_file("file.txt", &changed_lines); - let branch = get_virtual_branch(controller, project, branch_id).await; - assert_eq!(branch.files.len(), 1); - assert_eq!(branch.files[0].path.display().to_string(), "file.txt"); - assert_eq!(branch.files[0].hunks.len(), 1); - // TODO: We lock this hunk, but can we afford not lock it? - assert!(branch.files[0].hunks[0].locked); - // cleanup - repository.write_file("file.txt", &lines); - } - { - // hunk after the commited part but with overlapping context - let mut changed_lines = lines.clone(); - changed_lines[14] = "updated line".to_string(); - repository.write_file("file.txt", &changed_lines); - let branch = get_virtual_branch(controller, project, branch_id).await; - assert_eq!(branch.files.len(), 1); - assert_eq!(branch.files[0].path.display().to_string(), "file.txt"); - assert_eq!(branch.files[0].hunks.len(), 1); - // TODO: We lock this hunk, but can we afford not lock it? - assert!(branch.files[0].hunks[0].locked); - // cleanup - repository.write_file("file.txt", &lines); - } -} - #[tokio::test] async fn should_reset_into_same_branch() { let Test { @@ -228,56 +128,6 @@ async fn should_reset_into_same_branch() { assert_eq!(files.len(), 1); } -#[tokio::test] -async fn should_double_lock() { - let Test { - project, - controller, - repository, - .. - } = &Test::default(); - - let mut lines = repository.gen_file("file.txt", 7); - repository.write_file("file.txt", &lines); - commit_and_push_initial(repository); - - controller - .set_base_branch(project, &"refs/remotes/origin/master".parse().unwrap()) - .await - .unwrap(); - - let branch_id = controller - .create_virtual_branch(project, &branch::BranchCreateRequest::default()) - .await - .unwrap(); - - lines[0] = "change 1".to_string(); - repository.write_file("file.txt", &lines); - - let commit_1 = controller - .create_commit(project, branch_id, "commit 1", None, false) - .await - .unwrap(); - - lines[6] = "change 2".to_string(); - repository.write_file("file.txt", &lines); - - let commit_2 = controller - .create_commit(project, branch_id, "commit 2", None, false) - .await - .unwrap(); - - lines[3] = "change3".to_string(); - repository.write_file("file.txt", &lines); - - let branch = get_virtual_branch(controller, project, branch_id).await; - let locks = &branch.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); -} - fn commit_and_push_initial(repository: &TestProject) { repository.commit_all("initial commit"); repository.push(); From 44aa0ba5dfab87c8a1677833b44768d36ab9d02c Mon Sep 17 00:00:00 2001 From: Caleb Owens Date: Tue, 9 Jul 2024 20:18:37 +0200 Subject: [PATCH 2/3] Remove old hunk locking algorythmimi --- .../gitbutler-virtual/src/branch_manager.rs | 1 - crates/gitbutler-virtual/src/virtual.rs | 129 ++---------------- 2 files changed, 9 insertions(+), 121 deletions(-) diff --git a/crates/gitbutler-virtual/src/branch_manager.rs b/crates/gitbutler-virtual/src/branch_manager.rs index 4d7d4c11d..75094266e 100644 --- a/crates/gitbutler-virtual/src/branch_manager.rs +++ b/crates/gitbutler-virtual/src/branch_manager.rs @@ -679,7 +679,6 @@ impl<'l> BranchManager<'l> { let (applied_statuses, _) = get_applied_status( self.project_repository, &integration_commit.id(), - &target_commit.id(), virtual_branches, ) .context("failed to get status by branch")?; diff --git a/crates/gitbutler-virtual/src/virtual.rs b/crates/gitbutler-virtual/src/virtual.rs index 3546cc0a3..f0ef3cae5 100644 --- a/crates/gitbutler-virtual/src/virtual.rs +++ b/crates/gitbutler-virtual/src/virtual.rs @@ -24,7 +24,6 @@ use std::{ use anyhow::{anyhow, bail, Context, Result}; use bstr::{BString, ByteSlice, ByteVec}; use diffy::{apply_bytes as diffy_apply, Line, Patch}; -use git2::ErrorCode; use git2_hooks::HookResult; use hex::ToHex; use serde::{Deserialize, Serialize}; @@ -206,7 +205,6 @@ 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() @@ -214,13 +212,9 @@ pub fn unapply_ownership( let integration_commit_id = get_workspace_head(&vb_state, project_repository)?; - let (applied_statuses, _) = get_applied_status( - project_repository, - &integration_commit_id, - &default_target.sha, - virtual_branches, - ) - .context("failed to get status by branch")?; + let (applied_statuses, _) = + get_applied_status(project_repository, &integration_commit_id, virtual_branches) + .context("failed to get status by branch")?; let hunks_to_unapply = applied_statuses .iter() @@ -1064,7 +1058,6 @@ 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, )?; @@ -1144,95 +1137,11 @@ fn new_compute_locks( Ok(locked_hunks) } -fn compute_merge_base( - project_repository: &ProjectRepository, - target_sha: &git2::Oid, - virtual_branches: &Vec, -) -> Result { - 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, -) -> Result>> { - let merge_base = compute_merge_base(project_repository, target_sha, virtual_branches)?; - let mut locked_hunk_map = HashMap::>::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).saturating_sub(1), - 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(); - if commit_id == *integration_commit || commit_id == *target_sha { - continue; - } - let hash = Hunk::hash_diff(&hunk.diff_lines); - let Some(branch_id) = commit_to_branch.get(&commit_id) else { - continue; - }; - - let hunk_lock = diff::HunkLock { - branch_id: *branch_id, - commit_id, - }; - locked_hunk_map - .entry(hash) - .and_modify(|locks| { - locks.push(hunk_lock); - }) - .or_insert(vec![hunk_lock]); - } - } - } - Ok(locked_hunk_map) -} - // Returns branches and their associated file changes, in addition to a list // of skipped files. pub(crate) fn get_applied_status( project_repository: &ProjectRepository, integration_commit: &git2::Oid, - target_sha: &git2::Oid, mut virtual_branches: Vec, ) -> Result<(AppliedStatuses, Vec)> { let base_file_diffs = diff::workdir(project_repository.repo(), &integration_commit.to_owned()) @@ -1262,17 +1171,7 @@ pub(crate) fn get_applied_status( .map(|branch| (branch.id, HashMap::new())) .collect(); - let locks = if project_repository.project().use_new_locking { - new_compute_locks(project_repository.repo(), &base_diffs, &virtual_branches)? - } else { - 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(); @@ -1538,7 +1437,7 @@ pub fn write_tree_onto_commit( files: impl IntoIterator, impl Borrow>)>, ) -> Result { // read the base sha into an index - let git_repository = project_repository.repo(); + let git_repository: &git2::Repository = project_repository.repo(); let head_commit = git_repository.find_commit(commit_oid)?; let base_tree = head_commit.tree()?; @@ -2248,12 +2147,8 @@ pub fn amend( let integration_commit_id = crate::integration::get_workspace_head(&vb_state, project_repository)?; - let (mut applied_statuses, _) = get_applied_status( - project_repository, - &integration_commit_id, - &default_target.sha, - virtual_branches, - )?; + let (mut applied_statuses, _) = + get_applied_status(project_repository, &integration_commit_id, virtual_branches)?; let (ref mut target_branch, target_status) = applied_statuses .iter_mut() @@ -2733,17 +2628,11 @@ pub 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 = crate::integration::get_workspace_head(&vb_state, project_repository)?; - let (mut applied_statuses, _) = get_applied_status( - project_repository, - &integration_commit_id, - &default_target.sha, - applied_branches, - )?; + let (mut applied_statuses, _) = + get_applied_status(project_repository, &integration_commit_id, applied_branches)?; let (ref mut source_branch, source_status) = applied_statuses .iter_mut() From c4393346332568d1f7633b4f3480c91bd02dd3b2 Mon Sep 17 00:00:00 2001 From: Caleb Owens Date: Tue, 9 Jul 2024 20:22:45 +0200 Subject: [PATCH 3/3] Remove old feature flag --- crates/gitbutler-project/src/project.rs | 7 ------- crates/gitbutler-project/src/storage.rs | 5 ----- 2 files changed, 12 deletions(-) diff --git a/crates/gitbutler-project/src/project.rs b/crates/gitbutler-project/src/project.rs index 745a9d579..52ad164d4 100644 --- a/crates/gitbutler-project/src/project.rs +++ b/crates/gitbutler-project/src/project.rs @@ -88,17 +88,10 @@ pub struct Project { pub omit_certificate_check: Option, // The number of changed lines that will trigger a snapshot pub snapshot_lines_threshold: Option, - - #[serde(default = "default_true")] - pub use_new_locking: bool, #[serde(default)] pub ignore_project_semaphore: bool, } -fn default_true() -> bool { - true -} - impl Project { pub fn is_sync_enabled(&self) -> bool { self.api.as_ref().map(|api| api.sync).unwrap_or_default() diff --git a/crates/gitbutler-project/src/storage.rs b/crates/gitbutler-project/src/storage.rs index fa4f67926..344ee2ff4 100644 --- a/crates/gitbutler-project/src/storage.rs +++ b/crates/gitbutler-project/src/storage.rs @@ -27,7 +27,6 @@ pub struct UpdateRequest { pub omit_certificate_check: Option, pub use_diff_context: Option, pub snapshot_lines_threshold: Option, - pub use_new_locking: Option, pub ignore_project_semaphore: Option, } @@ -123,10 +122,6 @@ impl Storage { project.snapshot_lines_threshold = Some(snapshot_lines_threshold); } - if let Some(use_new_locking) = update_request.use_new_locking { - project.use_new_locking = use_new_locking; - } - if let Some(ignore_project_semaphore) = update_request.ignore_project_semaphore { project.ignore_project_semaphore = ignore_project_semaphore; }