fix: Adjust blob-merge baseline to also test the reverse of each operation

Byron · Byron · commit 399fea93c1a3 · 2024-10-20T17:41:19.000+02:00
This also fixes an issue with blob merge computations.
diff --git a/gix-merge/src/blob/builtin_driver/text/function.rs b/gix-merge/src/blob/builtin_driver/text/function.rs
@@ -5,6 +5,7 @@ use crate::blob::builtin_driver::text::utils::{
 };
 use crate::blob::builtin_driver::text::{Conflict, ConflictStyle, Labels, Options};
 use crate::blob::Resolution;
+use std::ops::Range;
 
 /// Merge `current` and `other` with `ancestor` as base according to `opts`.
 ///
@@ -36,7 +37,7 @@ pub fn merge<'a>(
     input.update_before(tokens(ancestor));
     input.update_after(tokens(current));
 
-    let current_hunks = imara_diff::diff(
+    let hunks = imara_diff::diff(
         opts.diff_algorithm,
         input,
         CollectHunks {
@@ -53,7 +54,7 @@ pub fn merge<'a>(
         input,
         CollectHunks {
             side: Side::Other,
-            hunks: current_hunks,
+            hunks,
         },
     );
 
@@ -62,33 +63,28 @@ pub fn merge<'a>(
     let mut intersecting = Vec::new();
     let mut ancestor_integrated_until = 0;
     let mut resolution = Resolution::Complete;
-    let mut filled_hunks = Vec::with_capacity(2);
-    while let Some(hunk) = hunks.next() {
-        if take_intersecting(&hunk, &mut hunks, &mut intersecting) {
-            fill_ancestor(&hunk.before, &mut intersecting);
-
-            let filled_hunks_side = hunk.side;
-            filled_hunks.clear();
-            filled_hunks.push(hunk);
-            fill_ancestor(
-                &intersecting
-                    .first()
-                    .zip(intersecting.last())
-                    .map(|(f, l)| f.before.start..l.before.end)
-                    .expect("at least one entry"),
-                &mut filled_hunks,
-            );
+    let mut current_hunks = Vec::with_capacity(2);
+    while take_intersecting(&mut hunks, &mut current_hunks, &mut intersecting).is_some() {
+        if !intersecting.is_empty() {
+            let filled_hunks_side = current_hunks.first().expect("at least one hunk").side;
+            {
+                let filled_hunks_range = before_range_from_hunks(&current_hunks);
+                let intersecting_range = before_range_from_hunks(&intersecting);
+                let extended_range = filled_hunks_range.start..intersecting_range.end.max(filled_hunks_range.end);
+                fill_ancestor(&extended_range, &mut current_hunks);
+                fill_ancestor(&extended_range, &mut intersecting);
+            }
             match opts.conflict {
                 Conflict::Keep { style, marker_size } => {
                     let (hunks_front_and_back, num_hunks_front) = match style {
                         ConflictStyle::Merge | ConflictStyle::ZealousDiff3 => {
-                            zealously_contract_hunks(&mut filled_hunks, &mut intersecting, input, &current_tokens)
+                            zealously_contract_hunks(&mut current_hunks, &mut intersecting, input, &current_tokens)
                         }
                         ConflictStyle::Diff3 => (Vec::new(), 0),
                     };
                     let (our_hunks, their_hunks) = match filled_hunks_side {
-                        Side::Current => (&filled_hunks, &intersecting),
-                        Side::Other => (&intersecting, &filled_hunks),
+                        Side::Current => (&current_hunks, &intersecting),
+                        Side::Other => (&intersecting, &current_hunks),
                         Side::Ancestor => {
                             unreachable!("initial hunks are never ancestors")
                         }
@@ -173,8 +169,8 @@ pub fn merge<'a>(
                 }
                 Conflict::ResolveWithOurs | Conflict::ResolveWithTheirs => {
                     let (our_hunks, their_hunks) = match filled_hunks_side {
-                        Side::Current => (&filled_hunks, &intersecting),
-                        Side::Other => (&intersecting, &filled_hunks),
+                        Side::Current => (&current_hunks, &intersecting),
+                        Side::Other => (&intersecting, &current_hunks),
                         Side::Ancestor => {
                             unreachable!("initial hunks are never ancestors")
                         }
@@ -194,11 +190,11 @@ pub fn merge<'a>(
                 }
                 Conflict::ResolveWithUnion => {
                     let (hunks_front_and_back, num_hunks_front) =
-                        zealously_contract_hunks(&mut filled_hunks, &mut intersecting, input, &current_tokens);
+                        zealously_contract_hunks(&mut current_hunks, &mut intersecting, input, &current_tokens);
 
                     let (our_hunks, their_hunks) = match filled_hunks_side {
-                        Side::Current => (&filled_hunks, &intersecting),
-                        Side::Other => (&intersecting, &filled_hunks),
+                        Side::Current => (&current_hunks, &intersecting),
+                        Side::Other => (&intersecting, &current_hunks),
                         Side::Ancestor => {
                             unreachable!("initial hunks are never ancestors")
                         }
@@ -228,6 +224,7 @@ pub fn merge<'a>(
                 }
             }
         } else {
+            let hunk = current_hunks.pop().expect("always pushed during intersection check");
             write_ancestor(input, ancestor_integrated_until, hunk.before.start as usize, out);
             ancestor_integrated_until = hunk.before.end;
             write_hunks(std::slice::from_ref(&hunk), input, &current_tokens, out);
@@ -237,3 +234,11 @@ pub fn merge<'a>(
 
     resolution
 }
+
+fn before_range_from_hunks(hunks: &[Hunk]) -> Range<u32> {
+    hunks
+        .first()
+        .zip(hunks.last())
+        .map(|(f, l)| f.before.start..l.before.end)
+        .expect("at least one entry")
+}
diff --git a/gix-merge/src/blob/builtin_driver/text/utils.rs b/gix-merge/src/blob/builtin_driver/text/utils.rs
@@ -414,21 +414,46 @@ fn write_tokens(
 }
 
 /// Find all hunks in `iter` which aren't from the same side as `hunk` and intersect with it.
-/// Return `true` if `out` is non-empty after the operation, indicating overlapping hunks were found.
-pub fn take_intersecting(hunk: &Hunk, iter: &mut Peekable<impl Iterator<Item = Hunk>>, out: &mut Vec<Hunk>) -> bool {
-    out.clear();
-    while iter
-        .peek()
-        .filter(|b_hunk| {
-            b_hunk.side != hunk.side
-                && (hunk.before.contains(&b_hunk.before.start)
-                    || (hunk.before.is_empty() && hunk.before.start == b_hunk.before.start))
-        })
-        .is_some()
-    {
-        out.extend(iter.next());
+/// Also put `hunk` into `input` so it's the first item, and possibly put more hunks of the side of `hunk` so
+/// `iter` doesn't have any overlapping hunks left.
+/// Return `true` if `intersecting` is non-empty after the operation, indicating overlapping hunks were found.
+pub fn take_intersecting(
+    iter: &mut Peekable<impl Iterator<Item = Hunk>>,
+    input: &mut Vec<Hunk>,
+    intersecting: &mut Vec<Hunk>,
+) -> Option<()> {
+    input.clear();
+    input.push(iter.next()?);
+    intersecting.clear();
+
+    fn left_overlaps_right(left: &Hunk, right: &Hunk) -> bool {
+        left.side != right.side
+            && (right.before.contains(&left.before.start)
+                || (right.before.is_empty() && right.before.start == left.before.start))
+    }
+
+    loop {
+        let hunk = input.last().expect("just pushed");
+        while iter.peek().filter(|b_hunk| left_overlaps_right(b_hunk, hunk)).is_some() {
+            intersecting.extend(iter.next());
+        }
+        // The hunks that overlap might themselves overlap with a following hunk of the other side.
+        // If so, split it so it doesn't overlap anymore.
+        let mut found_more_intersections = false;
+        while intersecting
+            .last_mut()
+            .zip(iter.peek_mut())
+            .filter(|(last_intersecting, candidate)| left_overlaps_right(candidate, last_intersecting))
+            .is_some()
+        {
+            input.extend(iter.next());
+            found_more_intersections = true;
+        }
+        if !found_more_intersections {
+            break;
+        }
     }
-    !out.is_empty()
+    Some(())
 }
 
 pub fn tokens(input: &[u8]) -> imara_diff::sources::ByteLines<'_, true> {
diff --git a/gix-merge/tests/fixtures/generated-archives/text-baseline.tar b/gix-merge/tests/fixtures/generated-archives/text-baseline.tar
diff --git a/gix-merge/tests/fixtures/text-baseline.sh b/gix-merge/tests/fixtures/text-baseline.sh
@@ -12,8 +12,11 @@ function baseline() {
 
   shift 4
   git merge-file --stdout "$@" "$ours" "$base" "$theirs" > "$output" || true
-
   echo "$ours" "$base" "$theirs" "$output" "$@" >> baseline.cases
+
+  local output="${output}-reversed"
+  git merge-file --stdout "$@" "$theirs" "$base" "$ours" > "${output}" || true
+  echo "$theirs" "$base" "$ours" "${output}" "$@" >> baseline-reversed.cases
 }
 
 mkdir simple
diff --git a/gix-merge/tests/merge/blob/builtin_driver.rs b/gix-merge/tests/merge/blob/builtin_driver.rs
@@ -71,60 +71,100 @@ mod text {
         "complex/spurious-c-conflicts/zdiff3-histogram.merged",
     ];
 
+    /// Should be a copy of `DIVERGING` once the reverse operation truly works like before
+    const DIVERGING_REVERSED: &[&str] = &[
+        // expected cases
+        // TODO: double-check them, maybe some could actually work, they seem to have similar failure modes as above
+        "zdiff3-middlecommon/merge.merged-reversed",
+        "zdiff3-middlecommon/merge-union.merged-reversed",
+        "zdiff3-interesting/merge.merged-reversed",
+        "zdiff3-interesting/merge-theirs.merged-reversed",
+        "zdiff3-interesting/diff3.merged-reversed",
+        "zdiff3-interesting/diff3-histogram.merged-reversed",
+        "zdiff3-interesting/zdiff3.merged-reversed",
+        "zdiff3-interesting/zdiff3-histogram.merged-reversed",
+        "zdiff3-interesting/merge-union.merged-reversed",
+        "zdiff3-evil/merge.merged-reversed",
+        "zdiff3-evil/merge-union.merged-reversed",
+        "complex/missing-LF-at-EOF/merge.merged-reversed",
+        "complex/missing-LF-at-EOF/diff3.merged-reversed",
+        "complex/missing-LF-at-EOF/diff3-histogram.merged-reversed",
+        "complex/missing-LF-at-EOF/zdiff3.merged-reversed",
+        "complex/missing-LF-at-EOF/zdiff3-histogram.merged-reversed",
+        "complex/missing-LF-at-EOF/merge-ours.merged-reversed",
+        "complex/missing-LF-at-EOF/merge-theirs.merged-reversed",
+        "complex/missing-LF-at-EOF/merge-union.merged-reversed",
+        "complex/auto-simplification/merge.merged-reversed",
+        "complex/auto-simplification/merge-union.merged-reversed",
+        "complex/marker-newline-handling-lf2/zdiff3.merged-reversed",
+        "complex/marker-newline-handling-lf2/zdiff3-histogram.merged-reversed",
+        "complex/spurious-c-conflicts/merge.merged-reversed",
+        "complex/spurious-c-conflicts/merge-union.merged-reversed",
+        "complex/spurious-c-conflicts/diff3-histogram.merged-reversed",
+        "complex/spurious-c-conflicts/zdiff3-histogram.merged-reversed",
+    ];
+
     // TODO: fix all of these eventually
-    fn is_case_diverging(case: &baseline::Expectation) -> bool {
-        DIVERGING.iter().any(|name| case.name == *name)
+    fn is_case_diverging(case: &baseline::Expectation, diverging: &[&str]) -> bool {
+        diverging.iter().any(|name| case.name == *name)
     }
 
     #[test]
     fn run_baseline() -> crate::Result {
         let root = gix_testtools::scripted_fixture_read_only("text-baseline.sh")?;
-        let cases = std::fs::read_to_string(root.join("baseline.cases"))?;
-        let mut out = Vec::new();
-        let mut num_diverging = 0;
-        let mut num_cases = 0;
-        for case in baseline::Expectations::new(&root, &cases) {
-            num_cases += 1;
-            let mut input = imara_diff::intern::InternedInput::default();
-            let actual = gix_merge::blob::builtin_driver::text(
-                &mut out,
-                &mut input,
-                case.labels(),
-                &case.ours,
-                &case.base,
-                &case.theirs,
-                case.options,
-            );
-            if is_case_diverging(&case) {
-                num_diverging += 1;
-            } else {
-                let expected_resolution = if case.expected.contains_str("<<<<<<<") {
-                    Resolution::Conflict
-                } else {
-                    Resolution::Complete
-                };
-                assert_eq!(out.as_bstr(), case.expected);
-                assert_str_eq!(
-                    out.as_bstr().to_str_lossy(),
-                    case.expected.to_str_lossy(),
-                    "{}: output mismatch\n{}",
-                    case.name,
-                    out.as_bstr()
+        for (baseline, diverging, expected_percentage) in [
+            ("baseline-reversed.cases", DIVERGING_REVERSED, 11),
+            ("baseline.cases", DIVERGING, 11),
+        ] {
+            let cases = std::fs::read_to_string(root.join(baseline))?;
+            let mut out = Vec::new();
+            let mut num_diverging = 0;
+            let mut num_cases = 0;
+            for case in baseline::Expectations::new(&root, &cases)
+            // .filter(|case| case.name == "partial-match/merge-union.merged-reversed")
+            {
+                num_cases += 1;
+                let mut input = imara_diff::intern::InternedInput::default();
+                let actual = gix_merge::blob::builtin_driver::text(
+                    &mut out,
+                    &mut input,
+                    case.labels(),
+                    &case.ours,
+                    &case.base,
+                    &case.theirs,
+                    case.options,
                 );
-                assert_eq!(actual, expected_resolution, "{}: resolution mismatch", case.name,);
+                if is_case_diverging(&case, diverging) {
+                    num_diverging += 1;
+                } else {
+                    let expected_resolution = if case.expected.contains_str("<<<<<<<") {
+                        Resolution::Conflict
+                    } else {
+                        Resolution::Complete
+                    };
+                    assert_str_eq!(
+                        out.as_bstr().to_str_lossy(),
+                        case.expected.to_str_lossy(),
+                        "{}: output mismatch\n{}",
+                        case.name,
+                        out.as_bstr()
+                    );
+                    assert_eq!(out.as_bstr(), case.expected);
+                    assert_eq!(actual, expected_resolution, "{}: resolution mismatch", case.name,);
+                }
             }
-        }
 
-        assert_eq!(
-            num_diverging,
-            DIVERGING.len(),
-            "Number of expected diverging cases must match the actual one - probably the implementation improved"
-        );
-        assert_eq!(
-            ((num_diverging as f32 / num_cases as f32) * 100.0) as usize,
-            11,
-            "Just to show the percentage of skipped tests - this should get better"
-        );
+            assert_eq!(
+                num_diverging,
+                diverging.len(),
+                "Number of expected diverging cases must match the actual one - probably the implementation improved"
+            );
+            assert_eq!(
+                ((num_diverging as f32 / num_cases as f32) * 100.0) as usize,
+                expected_percentage,
+                "Just to show the percentage of skipped tests - this should get better"
+            );
+        }
         Ok(())
     }
 
