conflicts: minimize diffs in materialized conflicts

One advantage of our conflict marker style (compared to the usual
3-way markers) is that they provide the user with the diff between the
base and one side so the user doesn't have to do that in their head
(which is how I use 3-way markers anyway). However, since we currently
always use the "first" side for the diff, that diff can be larger than
if we had picked the other side, which makes the marker style worse
than the usual 3-way markers. This has bothered me many times and it's
about time we fix it.

lib/src/conflicts.rs

@@ -12,7 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::cmp::min;
 use std::io::{Cursor, Write};
 
 use itertools::Itertools;
@@ -102,9 +101,8 @@ fn get_file_contents(store: &Store, path: &RepoPath, part: &ConflictPart) -> Vec
     }
 }
 
-fn write_diff_hunks(left: &[u8], right: &[u8], file: &mut dyn Write) -> std::io::Result<()> {
-    let diff = Diff::for_tokenizer(&[left, right], &find_line_ranges);
-    for hunk in diff.hunks() {
+fn write_diff_hunks(hunks: &[DiffHunk], file: &mut dyn Write) -> std::io::Result<()> {
+    for hunk in hunks {
         match hunk {
             DiffHunk::Matching(content) => {
                 for line in content.split_inclusive(|b| *b == b'\n') {
@@ -164,24 +162,39 @@ pub fn materialize_conflict(
                     MergeHunk::Resolved(content) => {
                         output.write_all(&content)?;
                     }
-                    MergeHunk::Conflict { removes, adds } => {
-                        let num_diffs = min(removes.len(), adds.len());
-
-                        // TODO: Pair up a remove with an add in a way that minimizes the size of
-                        // the diff
+                    MergeHunk::Conflict {
+                        mut removes,
+                        mut adds,
+                    } => {
                         output.write_all(CONFLICT_START_LINE)?;
-                        for i in 0..num_diffs {
+                        while !removes.is_empty() && !adds.is_empty() {
+                            let left = &removes[0];
+                            let mut diffs = vec![];
+                            for right in &adds {
+                                diffs.push(
+                                    Diff::for_tokenizer(&[left, right], &find_line_ranges)
+                                        .hunks()
+                                        .collect_vec(),
+                                );
+                            }
+                            let min_diff_index = diffs
+                                .iter()
+                                .position_min_by_key(|diff| diff_size(*diff))
+                                .unwrap();
                             output.write_all(CONFLICT_MINUS_LINE)?;
                             output.write_all(CONFLICT_PLUS_LINE)?;
-                            write_diff_hunks(&removes[i], &adds[i], output)?;
+                            write_diff_hunks(&diffs[min_diff_index], output)?;
+                            removes.remove(0);
+                            adds.remove(min_diff_index);
                         }
-                        for slice in removes.iter().skip(num_diffs) {
+
+                        for slice in removes {
                             output.write_all(CONFLICT_MINUS_LINE)?;
-                            output.write_all(slice)?;
+                            output.write_all(&slice)?;
                         }
-                        for slice in adds.iter().skip(num_diffs) {
+                        for slice in adds {
                             output.write_all(CONFLICT_PLUS_LINE)?;
-                            output.write_all(slice)?;
+                            output.write_all(&slice)?;
                         }
                         output.write_all(CONFLICT_END_LINE)?;
                     }
@@ -192,6 +205,16 @@ pub fn materialize_conflict(
     Ok(())
 }
 
+fn diff_size(hunks: &[DiffHunk]) -> usize {
+    hunks
+        .iter()
+        .map(|hunk| match hunk {
+            DiffHunk::Matching(_) => 0,
+            DiffHunk::Different(slices) => slices.iter().map(|slice| slice.len()).sum(),
+        })
+        .sum()
+}
+
 pub fn conflict_to_materialized_value(
     store: &Store,
     path: &RepoPath,

lib/tests/test_conflicts.rs

@@ -41,7 +41,9 @@ line 5
         &path,
         "line 1
 line 2
-left
+left 3.1
+left 3.2
+left 3.3
 line 4
 line 5
 ",
@@ -51,13 +53,13 @@ line 5
         &path,
         "line 1
 line 2
-right
+right 3.1
 line 4
 line 5
 ",
     );
 
-    let conflict = Conflict {
+    let mut conflict = Conflict {
         removes: vec![ConflictPart {
             value: TreeValue::Normal {
                 id: base_id,
@@ -88,9 +90,32 @@ line 5
     -------
     +++++++
     -line 3
-    +left
+    +right 3.1
     +++++++
-    right
+    left 3.1
+    left 3.2
+    left 3.3
+    >>>>>>>
+    line 4
+    line 5
+    "###
+    );
+    // Test with the larger diff first. We still want the small diff.
+    conflict.adds.reverse();
+    insta::assert_snapshot!(
+        &materialize_conflict_string(store, &path, &conflict),
+        @r###"
+    line 1
+    line 2
+    <<<<<<<
+    -------
+    +++++++
+    -line 3
+    +right 3.1
+    +++++++
+    left 3.1
+    left 3.2
+    left 3.3
     >>>>>>>
     line 4
     line 5
@@ -163,8 +188,8 @@ line 5
     -------
     +++++++
     -line 3
-    +left
     +++++++
+    left
     >>>>>>>
     line 4
     line 5