From 03ef18d2f047dc5a047fa46817e40cd16c4221b6 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 28 Aug 2024 07:41:53 +0200 Subject: [PATCH] Use read-only worktree diff The worktree diff is essentially a 'git status' with a given tree, and that can be expressed with a diff of the workdir, the index and a tree. This comes at the expense of not being able to control which file sizes are diffed. --- .../tests/extra/mod.rs | 10 ++- .../convert_to_real_branch.rs | 14 ++-- .../tests/virtual_branches/mod.rs | 5 +- crates/gitbutler-diff/src/diff.rs | 67 +++++++------------ crates/gitbutler-testsupport/src/suite.rs | 2 +- .../gitbutler-testsupport/src/test_project.rs | 11 +-- 6 files changed, 50 insertions(+), 59 deletions(-) diff --git a/crates/gitbutler-branch-actions/tests/extra/mod.rs b/crates/gitbutler-branch-actions/tests/extra/mod.rs index 04b8ce13d8..ed2350480a 100644 --- a/crates/gitbutler-branch-actions/tests/extra/mod.rs +++ b/crates/gitbutler-branch-actions/tests/extra/mod.rs @@ -759,7 +759,7 @@ fn commit_id_can_be_generated_or_specified() -> Result<()> { #[test] fn merge_vbranch_upstream_clean_rebase() -> Result<()> { let suite = Suite::default(); - let Case { ctx, project, .. } = &suite.new_case(); + let Case { ctx, project, .. } = &mut suite.new_case(); // create a commit and set the target let file_path = Path::new("test.txt"); @@ -829,8 +829,14 @@ fn merge_vbranch_upstream_clean_rebase() -> Result<()> { // create the branch let (branches, _) = list_virtual_branches(ctx, guard.write_permission())?; + assert_eq!(branches.len(), 1); let branch1 = &branches[0]; - assert_eq!(branch1.files.len(), 1); + assert_eq!( + branch1.files.len(), + 1 + 1, + "'test' (modified compared to index) and 'test2' (untracked).\ + This is actually correct when looking at the git repository" + ); assert_eq!(branch1.commits.len(), 1); // assert_eq!(branch1.upstream.as_ref().unwrap().commits.len(), 1); diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/convert_to_real_branch.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/convert_to_real_branch.rs index a552ba764d..5ee9c23937 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/convert_to_real_branch.rs +++ b/crates/gitbutler-branch-actions/tests/virtual_branches/convert_to_real_branch.rs @@ -61,15 +61,20 @@ fn conflicting() { let (branches, _) = controller.list_virtual_branches(project).unwrap(); assert_eq!(branches.len(), 1); - assert!(branches[0].base_current); - assert!(branches[0].active); + let branch = &branches[0]; + assert_eq!( + branch.name, "Virtual branch", + "the auto-created branch gets the default name" + ); + assert!(branch.base_current); + assert!(branch.active); assert_eq!( - branches[0].files[0].hunks[0].diff, + branch.files[0].hunks[0].diff, "@@ -1 +1 @@\n-first\n\\ No newline at end of file\n+conflict\n\\ No newline at end of file\n" ); let unapplied_branch = controller - .convert_to_real_branch(project, branches[0].id) + .convert_to_real_branch(project, branch.id) .unwrap(); Refname::from_str(&unapplied_branch).unwrap() @@ -100,7 +105,6 @@ fn conflicting() { assert_eq!(branches.len(), 1); let branch = &branches[0]; - // assert!(!branch.base_current); assert!(branch.conflicted); assert_eq!( branch.files[0].hunks[0].diff, diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs index b1b36b6b64..65b4797b66 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs +++ b/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs @@ -50,9 +50,8 @@ impl Test { /// Consume this instance and keep the temp directory that held the local repository, returning it. /// Best used inside a `dbg!(test.debug_local_repo())` #[allow(dead_code)] - pub fn debug_local_repo(mut self) -> PathBuf { - let repo = std::mem::take(&mut self.repository); - repo.debug_local_repo() + pub fn debug_local_repo(&mut self) -> Option { + self.repository.debug_local_repo() } } diff --git a/crates/gitbutler-diff/src/diff.rs b/crates/gitbutler-diff/src/diff.rs index 058db17a1b..ccd6227c43 100644 --- a/crates/gitbutler-diff/src/diff.rs +++ b/crates/gitbutler-diff/src/diff.rs @@ -1,9 +1,4 @@ -use std::{ - borrow::Cow, - collections::HashMap, - path::{Path, PathBuf}, - str, -}; +use std::{borrow::Cow, collections::HashMap, path::PathBuf, str}; use anyhow::{Context, Result}; use bstr::{BStr, BString, ByteSlice, ByteVec}; @@ -133,34 +128,6 @@ pub fn workdir(repo: &git2::Repository, commit_oid: &git2::Oid) -> Result i32 { - let file_size = std::fs::metadata(path).map(|m| m.len()).unwrap_or(0); - if file_size > 50_000_000 { - skipped_files.insert( - path.to_path_buf(), - FileDiff { - old_path: None, - new_path: None, - hunks: Vec::new(), - skipped: true, - binary: true, - old_size_bytes: 0, - new_size_bytes: 0, - }, - ); - 1 //skips the entry - } else { - 0 - } - }; - workdir_index.add_all(["."], git2::IndexAddOption::DEFAULT, Some(cb))?; - let workdir_tree_id = workdir_index.write_tree()?; - - let new_tree = repo.find_tree(workdir_tree_id)?; - let mut diff_opts = git2::DiffOptions::new(); diff_opts .recurse_untracked_dirs(true) @@ -170,14 +137,27 @@ pub fn workdir(repo: &git2::Repository, commit_oid: &git2::Oid) -> Result = index + .conflicts()? + .filter_map(Result::ok) + .filter_map(|c| { + c.our + .or(c.their) + .or(c.ancestor) + .and_then(|c| c.path.into_string().ok()) + }) + .collect(); + for conflict_path_to_resolve in paths_to_add { + index.add_path(conflict_path_to_resolve.as_ref())?; + } + let diff = repo.diff_tree_to_workdir_with_index(Some(&old_tree), Some(&mut diff_opts))?; + // TODO(ST): bring back support for skipped (large) files. + hunks_by_filepath(Some(repo), &diff) } pub fn trees( @@ -194,9 +174,10 @@ pub fn trees( .context_lines(3) .show_untracked_content(true); + // This is not a content-based diff, but also considers modification times apparently, + // maybe related to racy-git. This is why empty diffs have ot be filtered. let diff = repository.diff_tree_to_tree(Some(old_tree), Some(new_tree), Some(&mut diff_opts))?; - hunks_by_filepath(None, &diff) } diff --git a/crates/gitbutler-testsupport/src/suite.rs b/crates/gitbutler-testsupport/src/suite.rs index 2384c50507..4a434ccca0 100644 --- a/crates/gitbutler-testsupport/src/suite.rs +++ b/crates/gitbutler-testsupport/src/suite.rs @@ -92,7 +92,7 @@ pub struct Case { pub ctx: CommandContext, pub credentials: Helper, /// The directory containing the `ctx` - project_tmp: Option, + pub project_tmp: Option, } impl Drop for Case { diff --git a/crates/gitbutler-testsupport/src/test_project.rs b/crates/gitbutler-testsupport/src/test_project.rs index f4cfbf8ce4..b26b8311ab 100644 --- a/crates/gitbutler-testsupport/src/test_project.rs +++ b/crates/gitbutler-testsupport/src/test_project.rs @@ -19,8 +19,8 @@ pub struct TestProject { impl Drop for TestProject { fn drop(&mut self) { if std::env::var_os(VAR_NO_CLEANUP).is_some() { - let _ = self.local_tmp.take().unwrap().into_path(); - let _ = self.remote_tmp.take().unwrap().into_path(); + let _ = self.local_tmp.take().map(|tmp| tmp.into_path()); + let _ = self.remote_tmp.take().map(|tmp| tmp.into_path()); } } } @@ -83,10 +83,11 @@ impl Default for TestProject { } impl TestProject { - /// Consume this instance and keep the temp directory that held the local repository, returning it. + /// Take the tmp directory holding the local repository and make sure it won't be deleted, + /// returning a path to it. /// Best used inside a `dbg!(test_project.debug_local_repo())` - pub fn debug_local_repo(mut self) -> PathBuf { - self.local_tmp.take().unwrap().into_path() + pub fn debug_local_repo(&mut self) -> Option { + self.local_tmp.take().map(|tmp| tmp.into_path()) } pub fn path(&self) -> &std::path::Path { self.local_repository.workdir().unwrap()