feat!: rewrite-tracker now uses blob diff-platform for correct and optimized diffing.

Byron · Byron · commit feca5d0c1f56 · 2023-12-02T19:56:22.000+01:00
Correctness is improved as all necessary transformation are now performed.
Performance is improved by avoiding duplicate work by caching transformed
diffable data for later reuse.
diff --git a/gix-diff/src/blob/platform.rs b/gix-diff/src/blob/platform.rs
@@ -351,7 +351,7 @@ impl Platform {
         mode: gix_object::tree::EntryKind,
         rela_path: &BStr,
         kind: ResourceKind,
-        objects: &impl gix_object::FindObjectOrHeader, // TODO: make this `dyn` once https://github.com/rust-lang/rust/issues/65991 is stable
+        objects: &impl gix_object::FindObjectOrHeader, // TODO: make this `dyn` once https://github.com/rust-lang/rust/issues/65991 is stable, then also make tracker.rs `objects` dyn
     ) -> Result<(), set_resource::Error> {
         let res = self.set_resource_inner(id, mode, rela_path, kind, objects);
         if res.is_err() {
diff --git a/gix-diff/src/rewrites/mod.rs b/gix-diff/src/rewrites/mod.rs
@@ -10,14 +10,8 @@ pub struct Tracker<T> {
     items: Vec<tracker::Item<T>>,
     /// A place to store all paths in to reduce amount of allocations.
     path_backing: Vec<u8>,
-    /// A buffer for use when fetching objects for similarity tests.
-    buf1: Vec<u8>,
-    /// Another buffer for use when fetching objects for similarity tests.
-    buf2: Vec<u8>,
     /// How to track copies and/or rewrites.
     rewrites: Rewrites,
-    /// The diff algorithm to use when checking for similarity.
-    diff_algo: crate::blob::Algorithm,
 }
 
 /// Determine in which set of files to search for copies.
diff --git a/gix-diff/src/rewrites/tracker.rs b/gix-diff/src/rewrites/tracker.rs
@@ -8,11 +8,11 @@ use std::ops::Range;
 
 use gix_object::tree::{EntryKind, EntryMode};
 
-use crate::blob::DiffLineStats;
+use crate::blob::platform::prepare_diff::Operation;
+use crate::blob::{DiffLineStats, ResourceKind};
 use crate::rewrites::{CopySource, Outcome};
 use crate::{rewrites::Tracker, Rewrites};
 use bstr::BStr;
-use gix_object::FindExt;
 
 /// The kind of a change.
 #[derive(Debug, Copy, Clone, Ord, PartialOrd, PartialEq, Eq)]
@@ -123,21 +123,21 @@ pub mod emit {
         FindExistingBlob(#[from] gix_object::find::existing_object::Error),
         #[error("Could not obtain exhaustive item set to use as possible sources for copy detection")]
         GetItemsForExhaustiveCopyDetection(#[source] Box<dyn std::error::Error + Send + Sync>),
+        #[error(transparent)]
+        SetResource(#[from] crate::blob::platform::set_resource::Error),
+        #[error(transparent)]
+        PrepareDiff(#[from] crate::blob::platform::prepare_diff::Error),
     }
 }
 
 /// Lifecycle
 impl<T: Change> Tracker<T> {
-    /// Create a new instance with `rewrites` configuration, and the `diff_algo` to use when performing
-    /// similarity checking.
-    pub fn new(rewrites: Rewrites, diff_algo: crate::blob::Algorithm) -> Self {
+    /// Create a new instance with `rewrites` configuration.
+    pub fn new(rewrites: Rewrites) -> Self {
         Tracker {
             items: vec![],
             path_backing: vec![],
-            buf1: Vec::new(),
-            buf2: Vec::new(),
             rewrites,
-            diff_algo,
         }
     }
 }
@@ -177,25 +177,31 @@ impl<T: Change> Tracker<T> {
     ///
     /// `objects` is used to access blob data for similarity checks if required and is taken directly from the object database.
     /// Worktree filters and text conversions will be applied afterwards automatically. Note that object-caching *should not*
-    /// be enabled as caching is implemented internally, after all, the blob that's actually diffed is going through conversion steps.
+    /// be enabled as caching is implemented by `diff_cache`, after all, the blob that's actually diffed is going
+    /// through conversion steps.
     ///
-    /// Use `worktree_filter` to obtain working-tree versions of files present on disk before diffing to see if rewrites happened,
-    /// with text-conversions being applied afterwards.
+    /// `diff_cache` is a way to retain a cache of resources that are prepared for rapid diffing, and it also controls
+    /// the diff-algorithm (provided no user-algorithm is set).
+    /// Note that we control a few options of `diff_cache` to assure it will ignore external commands.
+    /// Note that we do not control how the `diff_cache` converts resources, it's left to the caller to decide
+    /// if it should look at what's stored in `git`, or in the working tree, along with all diff-specific conversions.
     ///
     /// `push_source_tree(push_fn: push(change, location))` is a function that is called when the entire tree of the source
     /// should be added as modifications by calling `push` repeatedly to use for perfect copy tracking. Note that `push`
     /// will panic if `change` is not a modification, and it's valid to not call `push` at all.
     pub fn emit<PushSourceTreeFn, E>(
         &mut self,
         mut cb: impl FnMut(visit::Destination<'_, T>, Option<visit::Source<'_>>) -> crate::tree::visit::Action,
-        objects: &dyn gix_object::Find,
-        _worktree_filter: &mut gix_filter::Pipeline,
+        diff_cache: &mut crate::blob::Platform,
+        objects: &impl gix_object::FindObjectOrHeader,
         mut push_source_tree: PushSourceTreeFn,
     ) -> Result<Outcome, emit::Error>
     where
         PushSourceTreeFn: FnMut(&mut dyn FnMut(T, &BStr)) -> Result<(), E>,
         E: std::error::Error + Send + Sync + 'static,
     {
+        diff_cache.options.skip_internal_diff_if_external_is_configured = false;
+
         fn by_id_and_location<T: Change>(a: &Item<T>, b: &Item<T>) -> std::cmp::Ordering {
             a.change
                 .id()
@@ -213,11 +219,19 @@ impl<T: Change> Tracker<T> {
             &mut cb,
             self.rewrites.percentage,
             &mut out,
+            diff_cache,
             objects,
         )?;
 
         if let Some(copies) = self.rewrites.copies {
-            self.match_pairs_of_kind(visit::SourceKind::Copy, &mut cb, copies.percentage, &mut out, objects)?;
+            self.match_pairs_of_kind(
+                visit::SourceKind::Copy,
+                &mut cb,
+                copies.percentage,
+                &mut out,
+                diff_cache,
+                objects,
+            )?;
 
             match copies.source {
                 CopySource::FromSetOfModifiedFiles => {}
@@ -233,7 +247,14 @@ impl<T: Change> Tracker<T> {
                     .map_err(|err| emit::Error::GetItemsForExhaustiveCopyDetection(Box::new(err)))?;
                     self.items.sort_by(by_id_and_location);
 
-                    self.match_pairs_of_kind(visit::SourceKind::Copy, &mut cb, copies.percentage, &mut out, objects)?;
+                    self.match_pairs_of_kind(
+                        visit::SourceKind::Copy,
+                        &mut cb,
+                        copies.percentage,
+                        &mut out,
+                        diff_cache,
+                        objects,
+                    )?;
                 }
             }
         }
@@ -263,11 +284,14 @@ impl<T: Change> Tracker<T> {
         cb: &mut impl FnMut(visit::Destination<'_, T>, Option<visit::Source<'_>>) -> crate::tree::visit::Action,
         percentage: Option<f32>,
         out: &mut Outcome,
-        objects: &dyn gix_object::Find,
+        diff_cache: &mut crate::blob::Platform,
+        objects: &impl gix_object::FindObjectOrHeader,
     ) -> Result<(), emit::Error> {
         // we try to cheaply reduce the set of possibilities first, before possibly looking more exhaustively.
         let needs_second_pass = !needs_exact_match(percentage);
-        if self.match_pairs(cb, None /* by identity */, kind, out, objects)? == crate::tree::visit::Action::Cancel {
+        if self.match_pairs(cb, None /* by identity */, kind, out, diff_cache, objects)?
+            == crate::tree::visit::Action::Cancel
+        {
             return Ok(());
         }
         if needs_second_pass {
@@ -292,7 +316,7 @@ impl<T: Change> Tracker<T> {
                 }
             };
             if !is_limited {
-                self.match_pairs(cb, percentage, kind, out, objects)?;
+                self.match_pairs(cb, percentage, kind, out, diff_cache, objects)?;
             }
         }
         Ok(())
@@ -304,9 +328,9 @@ impl<T: Change> Tracker<T> {
         percentage: Option<f32>,
         kind: visit::SourceKind,
         stats: &mut Outcome,
-        objects: &dyn gix_object::Find,
+        diff_cache: &mut crate::blob::Platform,
+        objects: &impl gix_object::FindObjectOrHeader,
     ) -> Result<crate::tree::visit::Action, emit::Error> {
-        // TODO(perf): reuse object data and interner state and interned tokens, make these available to `find_match()`
         let mut dest_ofs = 0;
         while let Some((mut dest_idx, dest)) = self.items[dest_ofs..].iter().enumerate().find_map(|(idx, item)| {
             (!item.emitted && matches!(item.change.kind(), ChangeKind::Addition)).then_some((idx, item))
@@ -317,12 +341,12 @@ impl<T: Change> Tracker<T> {
                 &self.items,
                 dest,
                 dest_idx,
-                percentage.map(|p| (p, self.diff_algo)),
+                percentage,
                 kind,
                 stats,
                 objects,
-                &mut self.buf1,
-                &mut self.buf2,
+                diff_cache,
+                &self.path_backing,
             )?
             .map(|(src_idx, src, diff)| {
                 let (id, entry_mode) = src.change.id_and_entry_mode();
@@ -409,15 +433,15 @@ fn find_match<'a, T: Change>(
     items: &'a [Item<T>],
     item: &Item<T>,
     item_idx: usize,
-    percentage: Option<(f32, crate::blob::Algorithm)>,
+    percentage: Option<f32>,
     kind: visit::SourceKind,
     stats: &mut Outcome,
-    objects: &dyn gix_object::Find,
-    buf1: &mut Vec<u8>,
-    buf2: &mut Vec<u8>,
+    objects: &impl gix_object::FindObjectOrHeader,
+    diff_cache: &mut crate::blob::Platform,
+    path_backing: &[u8],
 ) -> Result<Option<SourceTuple<'a, T>>, emit::Error> {
     let (item_id, item_mode) = item.change.id_and_entry_mode();
-    if needs_exact_match(percentage.map(|t| t.0)) || item_mode.is_link() {
+    if needs_exact_match(percentage) || item_mode.is_link() {
         let first_idx = items.partition_point(|a| a.change.id() < item_id);
         let range = match items.get(first_idx..).map(|items| {
             let end = items
@@ -440,55 +464,76 @@ fn find_match<'a, T: Change>(
             return Ok(Some(src));
         }
     } else {
-        let mut new = None;
-        let (percentage, algo) = percentage.expect("it's set to something below 1.0 and we assured this");
+        let mut has_new = false;
+        let percentage = percentage.expect("it's set to something below 1.0 and we assured this");
         debug_assert_eq!(
             item.change.entry_mode().kind(),
             EntryKind::Blob,
             "symlinks are matched exactly, and trees aren't used here"
         );
+
         for (can_idx, src) in items
             .iter()
             .enumerate()
             .filter(|(src_idx, src)| *src_idx != item_idx && src.is_source_for_destination_of(kind, item_mode))
         {
-            let new = match &new {
-                Some(new) => new,
-                None => {
-                    new = objects.find_blob(item_id, buf1)?.into();
-                    new.as_ref().expect("just set")
-                }
-            };
-            let old = objects.find_blob(src.change.id(), buf2)?;
-            // TODO: make sure we get attribute handling/worktree conversion and binary skips and filters right here.
-            let tokens = crate::blob::intern::InternedInput::new(
-                crate::blob::sources::byte_lines_with_terminator(old.data),
-                crate::blob::sources::byte_lines_with_terminator(new.data),
-            );
-            let counts = crate::blob::diff(
-                algo,
-                &tokens,
-                crate::blob::sink::Counter::new(diff::Statistics {
-                    removed_bytes: 0,
-                    input: &tokens,
-                }),
-            );
-            let similarity = (old.data.len() - counts.wrapped) as f32 / old.data.len().max(new.data.len()) as f32;
+            if !has_new {
+                diff_cache.set_resource(
+                    item_id.to_owned(),
+                    item_mode.kind(),
+                    item.location(path_backing),
+                    ResourceKind::NewOrDestination,
+                    objects,
+                )?;
+                has_new = true;
+            }
+            let (src_id, src_mode) = src.change.id_and_entry_mode();
+            diff_cache.set_resource(
+                src_id.to_owned(),
+                src_mode.kind(),
+                src.location(path_backing),
+                ResourceKind::OldOrSource,
+                objects,
+            )?;
+            let prep = diff_cache.prepare_diff()?;
             stats.num_similarity_checks += 1;
-            if similarity >= percentage {
-                return Ok(Some((
-                    can_idx,
-                    src,
-                    DiffLineStats {
-                        removals: counts.removals,
-                        insertions: counts.insertions,
-                        before: tokens.before.len().try_into().expect("interner handles only u32"),
-                        after: tokens.after.len().try_into().expect("interner handles only u32"),
-                        similarity,
+            match prep.operation {
+                Operation::InternalDiff { algorithm } => {
+                    let tokens =
+                        crate::blob::intern::InternedInput::new(prep.old.intern_source(), prep.new.intern_source());
+                    let counts = crate::blob::diff(
+                        algorithm,
+                        &tokens,
+                        crate::blob::sink::Counter::new(diff::Statistics {
+                            removed_bytes: 0,
+                            input: &tokens,
+                        }),
+                    );
+                    let old_data_len = prep.old.data.as_slice().unwrap_or_default().len();
+                    let new_data_len = prep.new.data.as_slice().unwrap_or_default().len();
+                    let similarity = (old_data_len - counts.wrapped) as f32 / old_data_len.max(new_data_len) as f32;
+                    if similarity >= percentage {
+                        return Ok(Some((
+                            can_idx,
+                            src,
+                            DiffLineStats {
+                                removals: counts.removals,
+                                insertions: counts.insertions,
+                                before: tokens.before.len().try_into().expect("interner handles only u32"),
+                                after: tokens.after.len().try_into().expect("interner handles only u32"),
+                                similarity,
+                            }
+                            .into(),
+                        )));
                     }
-                    .into(),
-                )));
-            }
+                }
+                Operation::ExternalCommand { .. } => {
+                    unreachable!("we have disabled this possibility with an option")
+                }
+                Operation::SourceOrDestinationIsBinary => {
+                    // TODO: figure out if git does more here
+                }
+            };
         }
     }
     Ok(None)
diff --git a/gix-diff/tests/rewrites/tracker.rs b/gix-diff/tests/rewrites/tracker.rs
@@ -564,23 +564,23 @@ mod util {
     pub fn assert_emit_with_objects(
         tracker: &mut rewrites::Tracker<Change>,
         cb: impl FnMut(Destination<'_, Change>, Option<Source<'_>>) -> Action,
-        objects: impl gix_object::Find,
+        objects: impl gix_object::FindObjectOrHeader,
     ) -> rewrites::Outcome {
         assert_emit_with_objects_and_sources(tracker, cb, objects, None)
     }
 
     pub fn assert_emit_with_objects_and_sources<'a>(
         tracker: &mut rewrites::Tracker<Change>,
         cb: impl FnMut(Destination<'_, Change>, Option<Source<'_>>) -> Action,
-        objects: impl gix_object::Find,
+        objects: impl gix_object::FindObjectOrHeader,
         sources: impl IntoIterator<Item = (Change, &'a str)>,
     ) -> rewrites::Outcome {
         let mut sources: Vec<_> = sources.into_iter().collect();
         tracker
             .emit(
                 cb,
+                &mut new_platform_no_worktree(),
                 &objects,
-                &mut gix_filter::Pipeline::new(Default::default(), Default::default()),
                 |cb| -> Result<(), std::io::Error> {
                     let sources = std::mem::take(&mut sources);
                     if sources.is_empty() {
@@ -596,6 +596,34 @@ mod util {
     }
 
     pub fn new_tracker(rewrites: Rewrites) -> rewrites::Tracker<Change> {
-        rewrites::Tracker::new(rewrites, gix_diff::blob::Algorithm::Myers)
+        rewrites::Tracker::new(rewrites)
+    }
+
+    fn new_platform_no_worktree() -> gix_diff::blob::Platform {
+        let root = gix_testtools::scripted_fixture_read_only_standalone("make_blob_repo.sh").expect("valid fixture");
+        let attributes = gix_worktree::Stack::new(
+            root,
+            gix_worktree::stack::State::AttributesStack(gix_worktree::stack::state::Attributes::new(
+                Default::default(),
+                None,
+                gix_worktree::stack::state::attributes::Source::IdMapping,
+                Default::default(),
+            )),
+            gix_worktree::glob::pattern::Case::Sensitive,
+            Vec::new(),
+            Vec::new(),
+        );
+        let filter = gix_diff::blob::Pipeline::new(
+            Default::default(),
+            gix_filter::Pipeline::default(),
+            Vec::new(),
+            Default::default(),
+        );
+        gix_diff::blob::Platform::new(
+            Default::default(),
+            filter,
+            gix_diff::blob::pipeline::Mode::ToGit,
+            attributes,
+        )
     }
 }
