From 1ec906b11425c09d40346ecdfdb669882f659b6c Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sun, 14 May 2023 06:38:55 -0700
Subject: [PATCH] files: make `merge()` require one more adds than removes

All call paths already check before calling the function that the
condition is true. One caller - `tree::try_resolve_file_conflict()` -
checks it itself. The other caller -
`conflicts::materialize_merge_result()` - doesn't, but its callers
have checked it via `extract_file_conflict_as_single_hunk()`.

The deleted comment about empty strings seems to be obsolete since
e48ace56d1e9. The caller pads the inputs with empty strings since that
commit.

I think we should ideally change this function's signature to make it
impossible to call it with bad inputs, and I hope to get back to that
soon.
---
 lib/src/files.rs | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/lib/src/files.rs b/lib/src/files.rs
index 724d0a969..7f7fcffcf 100644
--- a/lib/src/files.rs
+++ b/lib/src/files.rs
@@ -207,11 +207,8 @@ struct SyncRegion {
     right: Range<usize>,
 }
 
-// TODO: Should we require `add.len() == removes.len() + 1`? If that condition
-// is false, it effectively means that we should pretend that there are empty
-// strings in `removes` or `adds` to make it true. Maybe we should have to
-// caller make it explicitly that way.
 pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult {
+    assert_eq!(adds.len(), removes.len() + 1);
     let num_removes = removes.len();
     // TODO: Using the first remove as base (first in the inputs) is how it's
     // usually done for 3-way conflicts. Are there better heuristics when there are
@@ -225,9 +222,7 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult {
     for diff_hunk in diff.hunks() {
         match diff_hunk {
             DiffHunk::Matching(content) => {
-                if adds.len() > removes.len() {
-                    resolved_hunk.extend(content);
-                }
+                resolved_hunk.extend(content);
             }
             DiffHunk::Different(parts) => {
                 let mut removed_parts = parts[..num_removes].to_vec();
@@ -338,28 +333,28 @@ mod tests {
         );
         // All sides added same content
         assert_eq!(
-            merge(&[], &[b"a\n", b"a\n", b"a\n"]),
+            merge(&[b"", b""], &[b"a\n", b"a\n", b"a\n"]),
             MergeResult::Resolved(b"a\n".to_vec())
         );
         // One side modified, two sides added
         assert_eq!(
-            merge(&[b"a"], &[b"b", b"b", b"b"]),
+            merge(&[b"a", b""], &[b"b", b"b", b"b"]),
             MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
-                removes: vec![b"a".to_vec()],
+                removes: vec![b"a".to_vec(), b"".to_vec()],
                 adds: vec![b"b".to_vec(), b"b".to_vec(), b"b".to_vec()]
             })])
         );
         // All sides removed same content
         assert_eq!(
-            merge(&[b"a\n", b"a\n", b"a\n"], &[]),
+            merge(&[b"a\n", b"a\n", b"a\n"], &[b"", b"", b"", b""]),
             MergeResult::Resolved(b"".to_vec())
         );
         // One side modified, two sides removed
         assert_eq!(
-            merge(&[b"a\n", b"a\n", b"a\n"], &[b""]),
+            merge(&[b"a\n", b"a\n"], &[b"b\n", b"", b""]),
             MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
-                removes: vec![b"a\n".to_vec(), b"a\n".to_vec(), b"a\n".to_vec()],
-                adds: vec![b"".to_vec()]
+                removes: vec![b"a\n".to_vec(), b"a\n".to_vec()],
+                adds: vec![b"b\n".to_vec(), b"".to_vec(), b"".to_vec()]
             })])
         );
         // Three sides made the same change
@@ -367,14 +362,6 @@ mod tests {
             merge(&[b"a", b"a"], &[b"b", b"b", b"b"]),
             MergeResult::Resolved(b"b".to_vec())
         );
-        // One side unchanged, one side added
-        assert_eq!(
-            merge(&[b"a\n"], &[b"a\nb\n"]),
-            MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
-                removes: vec![b"".to_vec()],
-                adds: vec![b"b\n".to_vec()]
-            })])
-        );
         // Two sides left one line unchanged, and added conflicting additional lines
         assert_eq!(
             merge(&[b"a\n"], &[b"a\nb\n", b"a\nc\n"]),