merge: micro-optimize Merge, clean up constructors

cli/src/merge_tools/builtin.rs

@@ -722,13 +722,11 @@ mod tests {
                 }
             }
         }
-        let merge = Merge::new(
-            vec![to_file_id(base_tree.path_value(&path))],
-            vec![
-                to_file_id(left_tree.path_value(&path)),
-                to_file_id(right_tree.path_value(&path)),
-            ],
-        );
+        let merge = Merge::from_vec(vec![
+            to_file_id(left_tree.path_value(&path)),
+            to_file_id(base_tree.path_value(&path)),
+            to_file_id(right_tree.path_value(&path)),
+        ]);
         let content = extract_as_single_hunk(&merge, store, &path).block_on();
         let slices = content.map(|ContentHunk(buf)| buf.as_slice());
         let merge_result = files::merge(&slices);

lib/src/conflicts.rs

@@ -285,7 +285,7 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge<ContentHunk> {
         }
     }
 
-    Merge::new(removes, adds)
+    Merge::from_removes_adds(removes, adds)
 }
 
 /// Parses conflict markers in `content` and returns an updated version of

lib/src/files.rs

@@ -190,15 +190,13 @@ pub fn merge(slices: &Merge<&[u8]>) -> MergeResult {
                         merge_hunks.push(Merge::resolved(resolved_hunk));
                         resolved_hunk = ContentHunk(vec![]);
                     }
-                    merge_hunks.push(Merge::new(
+                    merge_hunks.push(Merge::from_removes_adds(
                         parts[..num_diffs]
                             .iter()
-                            .map(|part| ContentHunk(part.to_vec()))
-                            .collect_vec(),
+                            .map(|part| ContentHunk(part.to_vec())),
                         parts[num_diffs..]
                             .iter()
-                            .map(|part| ContentHunk(part.to_vec()))
-                            .collect_vec(),
+                            .map(|part| ContentHunk(part.to_vec())),
                     ));
                 }
             }
@@ -224,7 +222,7 @@ mod tests {
     }
 
     fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult {
-        super::merge(&Merge::new(removes.to_vec(), adds.to_vec()))
+        super::merge(&Merge::from_removes_adds(removes.to_vec(), adds.to_vec()))
     }
 
     #[test]
@@ -269,7 +267,7 @@ mod tests {
         // One side modified, two sides added
         assert_eq!(
             merge(&[b"a", b""], &[b"b", b"b", b"b"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a"), hunk(b"")],
                 vec![hunk(b"b"), hunk(b"b"), hunk(b"b")]
             )])
@@ -282,7 +280,7 @@ mod tests {
         // One side modified, two sides removed
         assert_eq!(
             merge(&[b"a\n", b"a\n"], &[b"b\n", b"", b""]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a\n"), hunk(b"a\n")],
                 vec![hunk(b"b\n"), hunk(b""), hunk(b"")]
             )])
@@ -295,23 +293,23 @@ mod tests {
         // One side removed, one side modified
         assert_eq!(
             merge(&[b"a\n"], &[b"", b"b\n"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a\n")],
                 vec![hunk(b""), hunk(b"b\n")]
             )])
         );
         // One side modified, one side removed
         assert_eq!(
             merge(&[b"a\n"], &[b"b\n", b""]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a\n")],
                 vec![hunk(b"b\n"), hunk(b"")]
             )])
         );
         // Two sides modified in different ways
         assert_eq!(
             merge(&[b"a"], &[b"b", b"c"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a")],
                 vec![hunk(b"b"), hunk(b"c")]
             )])
@@ -329,7 +327,7 @@ mod tests {
         // One side unchanged, two other sides make the different change
         assert_eq!(
             merge(&[b"a", b"a"], &[b"b", b"a", b"c"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a"), hunk(b"a")],
                 vec![hunk(b"b"), hunk(b"a"), hunk(b"c")]
             )])
@@ -344,15 +342,15 @@ mod tests {
         // Merge of an unresolved conflict and another branch.
         assert_eq!(
             merge(&[b"a", b"b"], &[b"c", b"d", b"e"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a"), hunk(b"b")],
                 vec![hunk(b"c"), hunk(b"d"), hunk(b"e")]
             )])
         );
         // Two sides made the same change, third side made a different change
         assert_eq!(
             merge(&[b"a", b"b"], &[b"c", b"c", b"c"]),
-            MergeResult::Conflict(vec![Merge::new(
+            MergeResult::Conflict(vec![Merge::from_removes_adds(
                 vec![hunk(b"a"), hunk(b"b")],
                 vec![hunk(b"c"), hunk(b"c"), hunk(b"c")]
             )])
@@ -366,7 +364,7 @@ mod tests {
             merge(&[b"a\n"], &[b"a\nb\n", b"a\nc\n"]),
             MergeResult::Conflict(vec![
                 Merge::resolved(hunk(b"a\n")),
-                Merge::new(vec![hunk(b"")], vec![hunk(b"b\n"), hunk(b"c\n")])
+                Merge::from_removes_adds(vec![hunk(b"")], vec![hunk(b"b\n"), hunk(b"c\n")])
             ])
         );
         // Two sides changed different lines: no conflict
@@ -379,7 +377,7 @@ mod tests {
             merge(&[b"a\nb\nc\n"], &[b"a\nb1\nc\n", b"a\nb2\nc\n"]),
             MergeResult::Conflict(vec![
                 Merge::resolved(hunk(b"a\n")),
-                Merge::new(vec![hunk(b"b\n")], vec![hunk(b"b1\n"), hunk(b"b2\n")]),
+                Merge::from_removes_adds(vec![hunk(b"b\n")], vec![hunk(b"b1\n"), hunk(b"b2\n")]),
                 Merge::resolved(hunk(b"c\n"))
             ])
         );

lib/src/git_backend.rs

@@ -1376,7 +1376,7 @@ mod tests {
             TreeId::from_bytes(tree_builder.write().unwrap().as_bytes())
         };
 
-        let root_tree = Merge::new(
+        let root_tree = Merge::from_removes_adds(
             vec![create_tree(0), create_tree(1)],
             vec![create_tree(2), create_tree(3), create_tree(4)],
         );

lib/src/merge.rs

@@ -21,10 +21,11 @@ use std::fmt::{Debug, Formatter};
 use std::hash::Hash;
 use std::io::Write;
 use std::iter::zip;
+use std::slice;
 use std::sync::Arc;
-use std::{slice, vec};
 
 use itertools::Itertools;
+use smallvec::{smallvec_inline, SmallVec};
 
 use crate::backend;
 use crate::backend::{BackendError, FileId, ObjectId, TreeId, TreeValue};
@@ -115,7 +116,7 @@ where
 #[derive(PartialEq, Eq, Hash, Clone)]
 pub struct Merge<T> {
     /// Alternates between positive and negative terms, starting with positive.
-    values: Vec<T>,
+    values: SmallVec<[T; 1]>,
 }
 
 impl<T: Debug> Debug for Merge<T> {
@@ -131,19 +132,37 @@ impl<T: Debug> Debug for Merge<T> {
 }
 
 impl<T> Merge<T> {
+    /// Creates a `Merge` from the given values, in which positive and negative
+    /// terms alternate.
+    pub fn from_vec(values: impl Into<SmallVec<[T; 1]>>) -> Self {
+        let values = values.into();
+        assert!(
+            values.len() & 1 != 0,
+            "must have one more adds than removes"
+        );
+        Merge { values }
+    }
+
     /// Creates a new merge object from the given removes and adds.
-    pub fn new(removes: Vec<T>, adds: Vec<T>) -> Self {
-        // TODO: removes and adds can be just IntoIterator.
-        // TODO: maybe add constructor that takes single values vec, and rename this
-        assert_eq!(adds.len(), removes.len() + 1);
-        let values = itertools::interleave(adds, removes).collect();
+    pub fn from_removes_adds(
+        removes: impl IntoIterator<Item = T>,
+        adds: impl IntoIterator<Item = T>,
+    ) -> Self {
+        let removes = removes.into_iter();
+        let mut adds = adds.into_iter();
+        let mut values = SmallVec::with_capacity(removes.size_hint().0 * 2 + 1);
+        values.push(adds.next().expect("must have at least one add"));
+        for diff in removes.zip_longest(adds) {
+            let (remove, add) = diff.both().expect("must have one more adds than removes");
+            values.extend([remove, add]);
+        }
         Merge { values }
     }
 
     /// Creates a `Merge` with a single resolved value.
     pub fn resolved(value: T) -> Self {
         Merge {
-            values: vec![value],
+            values: smallvec_inline![value],
         }
     }
 
@@ -153,16 +172,14 @@ impl<T> Merge<T> {
         removes: impl IntoIterator<Item = T>,
         adds: impl IntoIterator<Item = T>,
     ) -> Merge<Option<T>> {
-        // TODO: no need to build intermediate removes/adds vecs
-        let mut removes = removes.into_iter().map(Some).collect_vec();
-        let mut adds = adds.into_iter().map(Some).collect_vec();
-        while removes.len() + 1 < adds.len() {
-            removes.push(None);
+        let removes = removes.into_iter();
+        let mut adds = adds.into_iter().fuse();
+        let mut values = smallvec_inline![adds.next()];
+        for diff in removes.zip_longest(adds) {
+            let (remove, add) = diff.map_any(Some, Some).or_default();
+            values.extend([remove, add]);
         }
-        while adds.len() < removes.len() + 1 {
-            adds.push(None);
-        }
-        Merge::new(removes, adds)
+        Merge { values }
     }
 
     /// The removed values, also called negative terms.
@@ -321,7 +338,7 @@ impl<T> Merge<T> {
 /// the checking until after `from_iter()` (to `MergeBuilder::build()`).
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct MergeBuilder<T> {
-    values: Vec<T>,
+    values: SmallVec<[T; 1]>,
 }
 
 impl<T> Default for MergeBuilder<T> {
@@ -336,16 +353,13 @@ impl<T> MergeBuilder<T> {
     /// Requires that exactly one more "adds" than "removes" have been added to
     /// this builder.
     pub fn build(self) -> Merge<T> {
-        assert!(self.values.len() & 1 != 0);
-        Merge {
-            values: self.values,
-        }
+        Merge::from_vec(self.values)
     }
 }
 
 impl<T> IntoIterator for Merge<T> {
     type Item = T;
-    type IntoIter = vec::IntoIter<T>;
+    type IntoIter = smallvec::IntoIter<[T; 1]>;
 
     fn into_iter(self) -> Self::IntoIter {
         self.values.into_iter()
@@ -599,7 +613,7 @@ mod tests {
     use super::*;
 
     fn c<T: Clone>(removes: &[T], adds: &[T]) -> Merge<T> {
-        Merge::new(removes.to_vec(), adds.to_vec())
+        Merge::from_removes_adds(removes.to_vec(), adds.to_vec())
     }
 
     #[test]
@@ -674,39 +688,48 @@ mod tests {
             assert_eq!(Merge::from_legacy_form(legacy_form.0, legacy_form.1), merge);
         }
         // Non-conflict
-        test_equivalent((vec![], vec![0]), Merge::new(vec![], vec![Some(0)]));
+        test_equivalent(
+            (vec![], vec![0]),
+            Merge::from_removes_adds(vec![], vec![Some(0)]),
+        );
         // Regular 3-way conflict
         test_equivalent(
             (vec![0], vec![1, 2]),
-            Merge::new(vec![Some(0)], vec![Some(1), Some(2)]),
+            Merge::from_removes_adds(vec![Some(0)], vec![Some(1), Some(2)]),
         );
         // Modify/delete conflict
         test_equivalent(
             (vec![0], vec![1]),
-            Merge::new(vec![Some(0)], vec![Some(1), None]),
+            Merge::from_removes_adds(vec![Some(0)], vec![Some(1), None]),
         );
         // Add/add conflict
         test_equivalent(
             (vec![], vec![0, 1]),
-            Merge::new(vec![None], vec![Some(0), Some(1)]),
+            Merge::from_removes_adds(vec![None], vec![Some(0), Some(1)]),
         );
         // 5-way conflict
         test_equivalent(
             (vec![0, 1], vec![2, 3, 4]),
-            Merge::new(vec![Some(0), Some(1)], vec![Some(2), Some(3), Some(4)]),
+            Merge::from_removes_adds(vec![Some(0), Some(1)], vec![Some(2), Some(3), Some(4)]),
         );
         // 5-way delete/delete conflict
         test_equivalent(
             (vec![0, 1], vec![]),
-            Merge::new(vec![Some(0), Some(1)], vec![None, None, None]),
+            Merge::from_removes_adds(vec![Some(0), Some(1)], vec![None, None, None]),
         );
     }
 
     #[test]
     fn test_as_resolved() {
-        assert_eq!(Merge::new(vec![], vec![0]).as_resolved(), Some(&0));
+        assert_eq!(
+            Merge::from_removes_adds(vec![], vec![0]).as_resolved(),
+            Some(&0)
+        );
         // Even a trivially resolvable merge is not resolved
-        assert_eq!(Merge::new(vec![0], vec![0, 1]).as_resolved(), None);
+        assert_eq!(
+            Merge::from_removes_adds(vec![0], vec![0, 1]).as_resolved(),
+            None
+        );
     }
 
     #[test]
@@ -781,7 +804,7 @@ mod tests {
     #[test]
     fn test_merge_invariants() {
         fn check_invariants(removes: &[u32], adds: &[u32]) {
-            let merge = Merge::new(removes.to_vec(), adds.to_vec());
+            let merge = Merge::from_removes_adds(removes.to_vec(), adds.to_vec());
             // `simplify()` is idempotent
             assert_eq!(
                 merge.clone().simplify().simplify(),