Correctly process overlapping unblamed hunks

Previously, `process_changes` assumed that `UnblamedHunk`s would never
overlap. This constraint has been lifted.

As a consequence, the high-level assumption that two different lines
from a *Blamed File* can never map to the same line in a *Source File*
has been lifted as well. There is not really a way to enforce this
constraint in the blame algorithm alone as it heavily depends on the
algorithm used for diffing.

Author: cruessler

Cargo.lock

@@ -31,3 +31,4 @@ gix-fs = { path = "../gix-fs" }
 gix-index = { path = "../gix-index" }
 gix-odb = { path = "../gix-odb" }
 gix-testtools = { path = "../tests/tools" }
+pretty_assertions = "1.4.0"

gix-blame/Cargo.toml

@@ -303,33 +303,6 @@ pub fn file(
             unblamed_hunk.remove_blame(suspect);
             true
         });
-
-        // This block asserts that line ranges for each suspect never overlap. If they did overlap
-        // this would mean that the same line in a *Source File* would map to more than one line in
-        // the *Blamed File* and this is not possible.
-        #[cfg(debug_assertions)]
-        {
-            let ranges = hunks_to_blame.iter().fold(
-                std::collections::BTreeMap::<ObjectId, Vec<Range<u32>>>::new(),
-                |mut acc, hunk| {
-                    for (suspect, range) in hunk.suspects.clone() {
-                        acc.entry(suspect).or_default().push(range);
-                    }
-
-                    acc
-                },
-            );
-
-            for (_, mut ranges) in ranges {
-                ranges.sort_by(|a, b| a.start.cmp(&b.start));
-
-                for window in ranges.windows(2) {
-                    if let [a, b] = window {
-                        assert!(a.end <= b.start, "#{hunks_to_blame:#?}");
-                    }
-                }
-            }
-        }
     }
 
     debug_assert_eq!(

gix-blame/src/file/function.rs

@@ -320,36 +320,43 @@ fn process_change(
 
 /// Consume `hunks_to_blame` and `changes` to pair up matches ranges (also overlapping) with each other.
 /// Once a match is found, it's pushed onto `out`.
+///
+/// `process_changes` assumes that ranges coming from the same *Source File* can and do
+/// occasionally overlap. If it were a desirable property of the blame algorithm as a whole to
+/// never have two different lines from a *Blamed File* mapped to the same line in a *Source File*,
+/// this property would need to be enforced at a higher level than `process_changes` if which case
+/// the nested loops could potentially be flattened into one.
 fn process_changes(
     hunks_to_blame: Vec<UnblamedHunk>,
     changes: Vec<Change>,
     suspect: ObjectId,
     parent: ObjectId,
 ) -> Vec<UnblamedHunk> {
-    let mut hunks_iter = hunks_to_blame.into_iter();
-    let mut changes_iter = changes.into_iter();
+    let mut new_hunks_to_blame = Vec::new();
 
-    let mut hunk = hunks_iter.next();
-    let mut change = changes_iter.next();
+    for hunk in hunks_to_blame {
+        let mut hunk = Some(hunk);
 
-    let mut new_hunks_to_blame = Vec::new();
-    let mut offset_in_destination = Offset::Added(0);
-
-    loop {
-        (hunk, change) = process_change(
-            &mut new_hunks_to_blame,
-            &mut offset_in_destination,
-            suspect,
-            parent,
-            hunk,
-            change,
-        );
-
-        hunk = hunk.or_else(|| hunks_iter.next());
-        change = change.or_else(|| changes_iter.next());
-
-        if hunk.is_none() && change.is_none() {
-            break;
+        let mut offset_in_destination = Offset::Added(0);
+
+        let mut changes_iter = changes.iter();
+        let mut change: Option<Change> = changes_iter.next().map(|change| change.clone());
+
+        loop {
+            (hunk, change) = process_change(
+                &mut new_hunks_to_blame,
+                &mut offset_in_destination,
+                suspect,
+                parent,
+                hunk,
+                change,
+            );
+
+            change = change.or_else(|| changes_iter.next().map(|change| change.clone()));
+
+            if hunk.is_none() {
+                break;
+            }
         }
     }
     new_hunks_to_blame

gix-blame/src/file/mod.rs

@@ -959,6 +959,8 @@ mod process_change {
 }
 
 mod process_changes {
+    use pretty_assertions::assert_eq;
+
     use crate::file::{
         process_changes,
         tests::{new_unblamed_hunk, one_sha, zero_sha},
@@ -1337,4 +1339,38 @@ mod process_changes {
             ]
         );
     }
+
+    #[test]
+    fn subsequent_hunks_overlapping_end_of_addition() {
+        let suspect = zero_sha();
+        let parent = one_sha();
+        let hunks_to_blame = vec![
+            new_unblamed_hunk(13..16, suspect, Offset::Added(0)),
+            new_unblamed_hunk(10..17, suspect, Offset::Added(0)),
+        ];
+        let changes = vec![Change::AddedOrReplaced(10..14, 0)];
+        let new_hunks_to_blame = process_changes(hunks_to_blame, changes, suspect, parent);
+
+        assert_eq!(
+            new_hunks_to_blame,
+            [
+                UnblamedHunk {
+                    range_in_blamed_file: 13..14,
+                    suspects: [(suspect, 13..14)].into()
+                },
+                UnblamedHunk {
+                    range_in_blamed_file: 14..16,
+                    suspects: [(parent, 10..12)].into(),
+                },
+                UnblamedHunk {
+                    range_in_blamed_file: 10..14,
+                    suspects: [(suspect, 10..14)].into(),
+                },
+                UnblamedHunk {
+                    range_in_blamed_file: 14..17,
+                    suspects: [(parent, 10..13)].into(),
+                },
+            ]
+        );
+    }
 }

gix-blame/src/file/tests.rs

@@ -353,7 +353,10 @@ pub(crate) enum Either<T, U> {
 }
 
 /// A single change between two blobs, or an unchanged region.
-#[derive(Debug, PartialEq)]
+///
+/// Line numbers refer to the file that is referred to as `after` or `NewOrDestination`, depending
+/// on the context.
+#[derive(Clone, Debug, PartialEq)]
 pub enum Change {
     /// A range of tokens that wasn't changed.
     Unchanged(Range<u32>),