feat: support rename tracking across changed directories

Author: Byron

gix-diff/src/rewrites/mod.rs

@@ -1,4 +1,6 @@
+use crate::tree::visit::ChangeId;
 use crate::Rewrites;
+use std::collections::BTreeSet;
 
 /// Types related to the rename tracker for renames, rewrites and copies.
 pub mod tracker;
@@ -12,6 +14,8 @@ pub struct Tracker<T> {
     path_backing: Vec<u8>,
     /// How to track copies and/or rewrites.
     rewrites: Rewrites,
+    /// Previously emitted relation ids of rewrite pairs, with `(deleted source, added destination)`.
+    child_renames: BTreeSet<(ChangeId, ChangeId)>,
 }
 
 /// Determine in which set of files to search for copies.

gix-diff/src/rewrites/tracker.rs

@@ -10,6 +10,7 @@ use std::ops::Range;
 use bstr::{BStr, ByteSlice};
 use gix_object::tree::{EntryKind, EntryMode};
 
+use crate::rewrites::tracker::visit::SourceKind;
 use crate::tree::visit::{Action, ChangeId, Relation};
 use crate::{
     blob::{platform::prepare_diff::Operation, DiffLineStats, ResourceKind},
@@ -155,6 +156,7 @@ impl<T: Change> Tracker<T> {
             items: vec![],
             path_backing: vec![],
             rewrites,
+            child_renames: Default::default(),
         }
     }
 }
@@ -244,8 +246,9 @@ impl<T: Change> Tracker<T> {
         };
         self.items.sort_by(by_id_and_location);
 
-        // Rewrites by directory can be pruned out quickly, quickly pruning candidates
-        // for the following per-item steps.
+        // Rewrites by directory (without local changes) can be pruned out quickly,
+        // by finding only parents, their counterpart, and then all children can be matched by
+        // relationship ID.
         self.match_pairs_of_kind(
             visit::SourceKind::Rename,
             &mut cb,
@@ -266,6 +269,8 @@ impl<T: Change> Tracker<T> {
             None,
         )?;
 
+        self.match_renamed_directories(&mut cb)?;
+
         if let Some(copies) = self.rewrites.copies {
             self.match_pairs_of_kind(
                 visit::SourceKind::Copy,
@@ -387,6 +392,7 @@ impl<T: Change> Tracker<T> {
         }) {
             dest_idx += dest_ofs;
             dest_ofs = dest_idx + 1;
+            self.items[dest_idx].location(&self.path_backing);
             let src = find_match(
                 &self.items,
                 dest,
@@ -434,18 +440,25 @@ impl<T: Change> Tracker<T> {
                 return Ok(Action::Cancel);
             }
 
-            if let Some((Relation::Parent(src), Relation::Parent(dst))) = relations {
-                let res = self.emit_child_renames_matching_identity(cb, kind, src, dst)?;
-                if res == Action::Cancel {
-                    return Ok(Action::Cancel);
+            match relations {
+                Some((Relation::Parent(src), Relation::Parent(dst))) => {
+                    let res = self.emit_child_renames_matching_identity(cb, kind, src, dst)?;
+                    if res == Action::Cancel {
+                        return Ok(Action::Cancel);
+                    }
                 }
+                Some((Relation::ChildOfParent(src), Relation::ChildOfParent(dst))) => {
+                    self.child_renames.insert((src, dst));
+                }
+                _ => {}
             }
         }
         Ok(Action::Continue)
     }
 
     /// Emit the children of `src_parent_id` and `dst_parent_id` as pairs of exact matches, which are assumed
     /// as `src` and `dst` were an exact match (so all children have to match exactly).
+    /// Note that we intentionally do not record them as their parents will be emitted, too.
     fn emit_child_renames_matching_identity(
         &mut self,
         cb: &mut impl FnMut(visit::Destination<'_, T>, Option<visit::Source<'_, T>>) -> Action,
@@ -504,6 +517,56 @@ impl<T: Change> Tracker<T> {
         }
         Ok(Action::Continue)
     }
+
+    /// Find directories with relation id that haven't been emitted yet and store them for lookup.
+    /// Then use the previously stored emitted renames with relation id to learn which directories they 'link'
+    /// and emit them, too.
+    /// Note that this works whenever top-level directories are renamed because they are always added and deleted,
+    /// and we only match those. Thus, one rewrite inside the directory is enough.
+    fn match_renamed_directories(
+        &mut self,
+        cb: &mut impl FnMut(visit::Destination<'_, T>, Option<visit::Source<'_, T>>) -> Action,
+    ) -> Result<(), emit::Error> {
+        fn unemitted_directory_matching_relation_id<T: Change>(items: &[Item<T>], child_id: ChangeId) -> Option<usize> {
+            items.iter().position(|i| {
+                !i.emitted && matches!(i.change.relation(), Some(Relation::Parent(pid)) if pid == child_id)
+            })
+        }
+        for (deleted_child_id, added_child_id) in &self.child_renames {
+            let Some(src_idx) = unemitted_directory_matching_relation_id(&self.items, *deleted_child_id) else {
+                continue;
+            };
+            let Some(dst_idx) = unemitted_directory_matching_relation_id(&self.items, *added_child_id) else {
+                // This could go wrong in case there are mismatches, so be defensive here.
+                // But generally, we'd expect the destination item to exist.
+                continue;
+            };
+
+            let (src_item, dst_item) = (&self.items[src_idx], &self.items[dst_idx]);
+            let entry_mode = src_item.change.entry_mode();
+            let location = src_item.location(&self.path_backing);
+            let src = visit::Source {
+                entry_mode,
+                id: src_item.change.id().to_owned(),
+                kind: SourceKind::Rename,
+                location,
+                change: &src_item.change,
+                diff: None,
+            };
+            let location = dst_item.location(&self.path_backing);
+            let change = dst_item.change.clone();
+            let dst = visit::Destination { change, location };
+            let res = cb(dst, Some(src));
+
+            self.items[src_idx].emitted = true;
+            self.items[dst_idx].emitted = true;
+
+            if res == Action::Cancel {
+                return Ok(());
+            }
+        }
+        Ok(())
+    }
 }
 
 fn filename(path: &BStr) -> &BStr {
@@ -572,8 +635,8 @@ fn find_match<'a, T: Change>(
     let (item_id, item_mode) = item.change.id_and_entry_mode();
     if needs_exact_match(percentage) || item_mode.is_link() {
         let first_idx = items.partition_point(|a| a.change.id() < item_id);
-        let range = items.get(first_idx..).map(|items| {
-            let end = items
+        let range = items.get(first_idx..).map(|slice| {
+            let end = slice
                 .iter()
                 .position(|a| a.change.id() != item_id)
                 .map_or(items.len(), |idx| first_idx + idx);

gix-diff/tests/diff/rewrites/tracker.rs

@@ -588,6 +588,7 @@ fn directory_renames_by_id_can_fail_gracefully() -> crate::Result {
             (Change::addition(), "b", "firt\nsecond\n"),
         ],
     );
+
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
         &mut track,
@@ -600,10 +601,17 @@ fn directory_renames_by_id_can_fail_gracefully() -> crate::Result {
                     assert_eq!(src.location, expected_src);
                     assert_eq!(dst.location, expected_dst);
                 }
-                3..=6 => {
+                3 => {
+                    assert_eq!(src.unwrap().location, "d");
+                    assert_eq!(
+                        dst.location, "d-renamed",
+                        "it can now track modified and renamed directories"
+                    );
+                }
+                4 => {
                     assert_eq!(src, None);
-                    let expected_dst = ["d", "d-renamed", "d/subdir/d"][calls - 3];
-                    assert_eq!(dst.location, expected_dst);
+                    assert_eq!(dst.change.kind, ChangeKind::Deletion);
+                    assert_eq!(dst.location, "d/subdir/d");
                 }
                 _ => unreachable!("Should have expected emission call {calls}"),
             }
@@ -620,7 +628,7 @@ fn directory_renames_by_id_can_fail_gracefully() -> crate::Result {
             ..Default::default()
         }
     );
-    assert_eq!(calls, 6, "Should not have too few calls");
+    assert_eq!(calls, 5, "Should not have too few calls");
     Ok(())
 }
 

gix-diff/tests/diff/tree_with_rewrites.rs

@@ -1904,6 +1904,137 @@ rename to gix-sec/tests/sec.rs
     Ok(())
 }
 
+#[test]
+fn realistic_renames_3_without_identity() -> crate::Result {
+    let (changes, out) = collect_changes_opts(
+        "r4-base",
+        "r4-dir-rename-non-identity",
+        Options {
+            location: Some(Location::Path),
+            rewrites: Some(Rewrites {
+                copies: None,
+                percentage: None,
+                limit: 0,
+            }),
+        },
+    )?;
+
+    // Look how nicely it captures and associates this directory rename.
+    insta::assert_debug_snapshot!(changes.into_iter()
+                                     .filter(|c| !c.entry_mode().is_tree() ||
+                                                  c.relation().map_or(false, |r| matches!(r, Relation::Parent(_)))
+                                     ).collect::<Vec<_>>(), @r#"
+    [
+        Rewrite {
+            source_location: "src/plumbing/options.rs",
+            source_entry_mode: EntryMode(
+                33188,
+            ),
+            source_relation: Some(
+                ChildOfParent(
+                    2,
+                ),
+            ),
+            source_id: Sha1(00750edc07d6415dcc07ae0351e9397b0222b7ba),
+            diff: None,
+            entry_mode: EntryMode(
+                33188,
+            ),
+            id: Sha1(00750edc07d6415dcc07ae0351e9397b0222b7ba),
+            location: "src/plumbing-renamed/options/mod.rs",
+            relation: Some(
+                ChildOfParent(
+                    1,
+                ),
+            ),
+            copy: false,
+        },
+        Rewrite {
+            source_location: "src/plumbing/mod.rs",
+            source_entry_mode: EntryMode(
+                33188,
+            ),
+            source_relation: Some(
+                ChildOfParent(
+                    2,
+                ),
+            ),
+            source_id: Sha1(0cfbf08886fca9a91cb753ec8734c84fcbe52c9f),
+            diff: None,
+            entry_mode: EntryMode(
+                33188,
+            ),
+            id: Sha1(0cfbf08886fca9a91cb753ec8734c84fcbe52c9f),
+            location: "src/plumbing-renamed/mod.rs",
+            relation: Some(
+                ChildOfParent(
+                    1,
+                ),
+            ),
+            copy: false,
+        },
+        Rewrite {
+            source_location: "src/plumbing/main.rs",
+            source_entry_mode: EntryMode(
+                33188,
+            ),
+            source_relation: Some(
+                ChildOfParent(
+                    2,
+                ),
+            ),
+            source_id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d),
+            diff: None,
+            entry_mode: EntryMode(
+                33188,
+            ),
+            id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d),
+            location: "src/plumbing-renamed/main.rs",
+            relation: Some(
+                ChildOfParent(
+                    1,
+                ),
+            ),
+            copy: false,
+        },
+        Rewrite {
+            source_location: "src/plumbing",
+            source_entry_mode: EntryMode(
+                16384,
+            ),
+            source_relation: Some(
+                Parent(
+                    2,
+                ),
+            ),
+            source_id: Sha1(b9d41dcdbd92fcab2fb6594d04f2ad99b3472621),
+            diff: None,
+            entry_mode: EntryMode(
+                16384,
+            ),
+            id: Sha1(202702465d7bb291153629dc2e8b353afe9cbdae),
+            location: "src/plumbing-renamed",
+            relation: Some(
+                Parent(
+                    1,
+                ),
+            ),
+            copy: false,
+        },
+    ]
+    "#);
+
+    let out = out.expect("tracking enabled");
+    assert_eq!(
+        out.num_similarity_checks, 0,
+        "similarity checks disabled, and not necessary"
+    );
+    assert_eq!(out.num_similarity_checks_skipped_for_rename_tracking_due_to_limit, 0);
+    assert_eq!(out.num_similarity_checks_skipped_for_copy_tracking_due_to_limit, 0);
+
+    Ok(())
+}
+
 mod util {
     use gix_diff::rewrites;
     use gix_object::{FindExt, TreeRefIter};

gix-diff/tests/fixtures/generated-archives/make_diff_for_rewrites_repo.tar

@@ -794,4 +794,16 @@ git -c diff.renames=1 show HEAD~2 > baseline-2.with-renames
 git -c diff.renames=0 show HEAD~4 > baseline.no-renames
 git -c diff.renames=1 show HEAD~4 > baseline.with-renames
 
+echo 1 >src/plumbing/main.rs
+echo 2 >src/plumbing/mod.rs
+echo 3 >src/plumbing/options.rs
+git add src/plumbing && git commit -m "r4-base"
+store_tree "r4-base"
+
+mkdir src/plumbing/options
+git mv src/plumbing/options.rs src/plumbing/options/mod.rs
+git mv src/plumbing src/plumbing-renamed
+git commit -m "r4-dir-rename-non-identity"
+store_tree "r4-dir-rename-non-identity"
+
 mv ../*.tree .