merge: have Merge::adds()/removes() return iterator

The Merge type will be changed to store interleaved values internally.

cli/src/commands/chmod.rs

@@ -91,7 +91,6 @@ pub(crate) fn cmd_chmod(
         }
         let all_files = tree_value
             .adds()
-            .iter()
             .flatten()
             .all(|tree_value| matches!(tree_value, TreeValue::File { .. }));
         if !all_files {

cli/src/commands/resolve.rs

@@ -142,7 +142,7 @@ pub(crate) fn print_conflicted_paths(
 
     for ((_, conflict), formatted_path) in std::iter::zip(conflicts.iter(), formatted_paths) {
         let sides = conflict.num_sides();
-        let n_adds = conflict.adds().iter().flatten().count();
+        let n_adds = conflict.adds().flatten().count();
         let deletions = sides - n_adds;
 
         let mut seen_objects = BTreeMap::new(); // Sort for consistency and easier testing
@@ -159,7 +159,7 @@ pub(crate) fn print_conflicted_paths(
         // TODO: We might decide it's OK for `jj resolve` to ignore special files in the
         // `removes` of a conflict (see e.g. https://github.com/martinvonz/jj/pull/978). In
         // that case, `conflict.removes` should be removed below.
-        for term in itertools::chain(conflict.removes().iter(), conflict.adds().iter()).flatten() {
+        for term in itertools::chain(conflict.removes(), conflict.adds()).flatten() {
             seen_objects.insert(
                 match term {
                     TreeValue::File {

lib/src/conflicts.rs

@@ -161,7 +161,7 @@ pub fn materialize_merge_result(
                     }
 
                     //  Emit the remaining positive terms as snapshots.
-                    for slice in &hunk.adds()[add_index..] {
+                    for slice in hunk.adds().skip(add_index) {
                         output.write_all(CONFLICT_PLUS_LINE)?;
                         output.write_all(&slice.0)?;
                     }

lib/src/git_backend.rs

@@ -940,12 +940,10 @@ fn write_tree_conflict(
     let entries = itertools::chain(
         conflict
             .removes()
-            .iter()
             .enumerate()
             .map(|(i, tree_id)| (format!(".jjconflict-base-{i}"), tree_id)),
         conflict
             .adds()
-            .iter()
             .enumerate()
             .map(|(i, tree_id)| (format!(".jjconflict-side-{i}"), tree_id)),
     )

lib/src/merge.rs

@@ -161,13 +161,13 @@ impl<T> Merge<T> {
     }
 
     /// The removed values, also called negative terms.
-    pub fn removes(&self) -> &[T] {
-        &self.removes
+    pub fn removes(&self) -> impl ExactSizeIterator<Item = &T> {
+        self.removes.iter()
     }
 
     /// The added values, also called positive terms.
-    pub fn adds(&self) -> &[T] {
-        &self.adds
+    pub fn adds(&self) -> impl ExactSizeIterator<Item = &T> {
+        self.adds.iter()
     }
 
     /// Returns the zeroth added value, which is guaranteed to exist.
@@ -427,8 +427,8 @@ impl<T> Merge<Merge<T>> {
 
 impl<T: ContentHash> ContentHash for Merge<T> {
     fn hash(&self, state: &mut impl digest::Update) {
-        self.removes().hash(state);
-        self.adds().hash(state);
+        self.removes.hash(state);
+        self.adds.hash(state);
     }
 }
 
@@ -507,10 +507,10 @@ impl MergedTreeValue {
     /// Give a summary description of the conflict's "removes" and "adds"
     pub fn describe(&self, file: &mut dyn Write) -> std::io::Result<()> {
         file.write_all(b"Conflict:\n")?;
-        for term in self.removes().iter().flatten() {
+        for term in self.removes().flatten() {
             file.write_all(format!("  Removing {}\n", describe_conflict_term(term)).as_bytes())?;
         }
-        for term in self.adds().iter().flatten() {
+        for term in self.adds().flatten() {
             file.write_all(format!("  Adding {}\n", describe_conflict_term(term)).as_bytes())?;
         }
         Ok(())

lib/src/merged_tree.rs

@@ -132,10 +132,10 @@ impl MergedTree {
                 adds[i + 1].remove(path.clone());
             }
             // Now add the terms that were present in the conflict to the appropriate trees.
-            for (i, term) in conflict.removes().iter().enumerate() {
+            for (i, term) in conflict.removes().enumerate() {
                 removes[i].set_or_remove(path.clone(), term.clone());
             }
-            for (i, term) in conflict.adds().iter().enumerate() {
+            for (i, term) in conflict.adds().enumerate() {
                 adds[i].set_or_remove(path.clone(), term.clone());
             }
         }

lib/src/op_store.rs

@@ -128,11 +128,11 @@ impl RefTarget {
     }
 
     pub fn removed_ids(&self) -> impl Iterator<Item = &CommitId> {
-        self.merge.removes().iter().flatten()
+        self.merge.removes().flatten()
     }
 
     pub fn added_ids(&self) -> impl Iterator<Item = &CommitId> {
-        self.merge.adds().iter().flatten()
+        self.merge.adds().flatten()
     }
 
     pub fn as_merge(&self) -> &Merge<Option<CommitId>> {

lib/src/simple_op_store.rs

@@ -444,8 +444,8 @@ fn ref_target_to_proto(value: &RefTarget) -> Option<crate::protos::op_store::Ref
     };
     let merge = value.as_merge();
     let conflict_proto = crate::protos::op_store::RefConflict {
-        removes: merge.removes().iter().map(term_to_proto).collect(),
-        adds: merge.adds().iter().map(term_to_proto).collect(),
+        removes: merge.removes().map(term_to_proto).collect(),
+        adds: merge.adds().map(term_to_proto).collect(),
     };
     let proto = crate::protos::op_store::RefTarget {
         value: Some(crate::protos::op_store::ref_target::Value::Conflict(

lib/tests/test_merge_trees.rs

@@ -119,13 +119,13 @@ fn test_same_type() {
                 .read_conflict(&RepoPath::from_internal_string("_ab"), id)
                 .unwrap();
             assert_eq!(
-                conflict.adds(),
-                vec![
-                    side1_tree.value(&component).cloned(),
-                    side2_tree.value(&component).cloned(),
-                ]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side1_tree.value(&component), side2_tree.value(&component)]
+            );
+            assert_eq!(
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![None]
             );
-            assert_eq!(conflict.removes(), vec![None]);
         }
         _ => panic!("unexpected value"),
     };
@@ -136,12 +136,12 @@ fn test_same_type() {
                 .read_conflict(&RepoPath::from_internal_string("a_b"), id)
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
-                vec![side2_tree.value(&component).cloned(), None]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side2_tree.value(&component), None]
             );
         }
         _ => panic!("unexpected value"),
@@ -153,12 +153,12 @@ fn test_same_type() {
                 .read_conflict(&RepoPath::from_internal_string("ab_"), id)
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
-                vec![side1_tree.value(&component).cloned(), None]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side1_tree.value(&component), None]
             );
         }
         _ => panic!("unexpected value"),
@@ -170,15 +170,12 @@ fn test_same_type() {
                 .read_conflict(&RepoPath::from_internal_string("abc"), id)
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
-                vec![
-                    side1_tree.value(&component).cloned(),
-                    side2_tree.value(&component).cloned(),
-                ]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side1_tree.value(&component), side2_tree.value(&component)]
             );
         }
         _ => panic!("unexpected value"),
@@ -424,15 +421,12 @@ fn test_types() {
                 )
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
-                vec![
-                    side1_tree.value(&component).cloned(),
-                    side2_tree.value(&component).cloned(),
-                ]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side1_tree.value(&component), side2_tree.value(&component)]
             );
         }
         _ => panic!("unexpected value"),
@@ -444,15 +438,12 @@ fn test_types() {
                 .read_conflict(&RepoPath::from_internal_string("tree_normal_symlink"), id)
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
-                vec![
-                    side1_tree.value(&component).cloned(),
-                    side2_tree.value(&component).cloned(),
-                ]
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
+                vec![side1_tree.value(&component), side2_tree.value(&component)]
             );
         }
         _ => panic!("unexpected value"),
@@ -507,14 +498,14 @@ fn test_simplify_conflict() {
                 .read_conflict(&RepoPath::from_components(vec![component.clone()]), id)
                 .unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
                 vec![
-                    branch_tree.value(&component).cloned(),
-                    upstream2_tree.value(&component).cloned(),
+                    branch_tree.value(&component),
+                    upstream2_tree.value(&component),
                 ]
             );
         }
@@ -526,14 +517,14 @@ fn test_simplify_conflict() {
         TreeValue::Conflict(id) => {
             let conflict = store.read_conflict(&path, id).unwrap();
             assert_eq!(
-                conflict.removes(),
-                vec![base_tree.value(&component).cloned()]
+                conflict.removes().map(|v| v.as_ref()).collect_vec(),
+                vec![base_tree.value(&component)]
             );
             assert_eq!(
-                conflict.adds(),
+                conflict.adds().map(|v| v.as_ref()).collect_vec(),
                 vec![
-                    upstream2_tree.value(&component).cloned(),
-                    branch_tree.value(&component).cloned(),
+                    upstream2_tree.value(&component),
+                    branch_tree.value(&component)
                 ]
             );
         }