merge: move some methods back to conflicts as free functions

I think I moved way too many functions onto `Merge<Option<TreeValue>>`
in 82883e648d. This effectively reverts almost all of that
commit. The `Merge<T>` type is simple container and it seems like it
should be at fairly low level in the dependency graph. By moving
functions off of it, we can get rid of the back-depdencies from the
`merge` module to the `conflict` module that I introduced when I moved
`Merge` to the `merge` module. I'm thinking the `conflict` module can
focus on materialized conflicts.

cli/src/commands/mod.rs

@@ -51,7 +51,7 @@ use jj_lib::settings::UserSettings;
 use jj_lib::tree::{merge_trees, Tree};
 use jj_lib::working_copy::SnapshotOptions;
 use jj_lib::workspace::Workspace;
-use jj_lib::{file_util, revset};
+use jj_lib::{conflicts, file_util, revset};
 use maplit::{hashmap, hashset};
 use tracing::instrument;
 
@@ -1449,9 +1449,7 @@ fn cmd_cat(ui: &mut Ui, command: &CommandHelper, args: &CatArgs) -> Result<(), C
         Some(TreeValue::Conflict(id)) => {
             let conflict = repo.store().read_conflict(&path, &id)?;
             let mut contents = vec![];
-            conflict
-                .materialize(repo.store(), &path, &mut contents)
-                .unwrap();
+            conflicts::materialize(&conflict, repo.store(), &path, &mut contents).unwrap();
             ui.request_pager();
             ui.stdout_formatter().write_all(&contents)?;
         }

cli/src/diff_util.rs

@@ -27,7 +27,7 @@ use jj_lib::repo::{ReadonlyRepo, Repo};
 use jj_lib::repo_path::RepoPath;
 use jj_lib::settings::{ConfigResultExt as _, UserSettings};
 use jj_lib::tree::{Tree, TreeDiffIterator};
-use jj_lib::{diff, files, rewrite, tree};
+use jj_lib::{conflicts, diff, files, rewrite, tree};
 use tracing::instrument;
 
 use crate::cli_util::{CommandError, WorkspaceCommandHelper};
@@ -345,9 +345,7 @@ fn diff_content(
         TreeValue::Conflict(id) => {
             let conflict = repo.store().read_conflict(path, id).unwrap();
             let mut content = vec![];
-            conflict
-                .materialize(repo.store(), path, &mut content)
-                .unwrap();
+            conflicts::materialize(&conflict, repo.store(), path, &mut content).unwrap();
             Ok(content)
         }
     }
@@ -496,9 +494,7 @@ fn git_diff_part(
             mode = "100644".to_string();
             hash = id.hex();
             let conflict = repo.store().read_conflict(path, id).unwrap();
-            conflict
-                .materialize(repo.store(), path, &mut content)
-                .unwrap();
+            conflicts::materialize(&conflict, repo.store(), path, &mut content).unwrap();
         }
     }
     let hash = hash[0..10].to_string();

cli/src/merge_tools.rs

@@ -22,7 +22,8 @@ use std::sync::Arc;
 use config::ConfigError;
 use itertools::Itertools;
 use jj_lib::backend::{TreeId, TreeValue};
-use jj_lib::conflicts::materialize_merge_result;
+use jj_lib::conflicts;
+use jj_lib::conflicts::{extract_as_single_hunk, materialize_merge_result};
 use jj_lib::gitignore::GitIgnoreFile;
 use jj_lib::matchers::{EverythingMatcher, Matcher};
 use jj_lib::repo_path::RepoPath;
@@ -256,7 +257,7 @@ pub fn run_mergetool(
             sides: file_merge.adds().len(),
         });
     };
-    let content = file_merge.extract_as_single_hunk(tree.store(), repo_path);
+    let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path);
 
     let editor = get_merge_tool_from_settings(ui, settings)?;
     let initial_output_content: Vec<u8> = if editor.merge_tool_edits_conflict_markers {
@@ -324,7 +325,8 @@ pub fn run_mergetool(
 
     let mut new_tree_value: Option<TreeValue> = None;
     if editor.merge_tool_edits_conflict_markers {
-        if let Some(new_conflict) = conflict.update_from_content(
+        if let Some(new_conflict) = conflicts::update_from_content(
+            &conflict,
             tree.store(),
             repo_path,
             output_file_contents.as_slice(),

lib/src/conflicts.rs

@@ -18,10 +18,13 @@ use std::io::Write;
 
 use itertools::Itertools;
 
+use crate::backend::{BackendResult, FileId, TreeValue};
 use crate::diff::{find_line_ranges, Diff, DiffHunk};
 use crate::files;
 use crate::files::{ContentHunk, MergeResult};
 use crate::merge::Merge;
+use crate::repo_path::RepoPath;
+use crate::store::Store;
 
 const CONFLICT_START_LINE: &[u8] = b"<<<<<<<\n";
 const CONFLICT_END_LINE: &[u8] = b">>>>>>>\n";
@@ -53,6 +56,47 @@ fn write_diff_hunks(hunks: &[DiffHunk], file: &mut dyn Write) -> std::io::Result
     Ok(())
 }
 
+fn get_file_contents(store: &Store, path: &RepoPath, term: &Option<FileId>) -> ContentHunk {
+    match term {
+        Some(id) => {
+            let mut content = vec![];
+            store
+                .read_file(path, id)
+                .unwrap()
+                .read_to_end(&mut content)
+                .unwrap();
+            ContentHunk(content)
+        }
+        // If the conflict had removed the file on one side, we pretend that the file
+        // was empty there.
+        None => ContentHunk(vec![]),
+    }
+}
+
+pub fn extract_as_single_hunk(
+    merge: &Merge<Option<FileId>>,
+    store: &Store,
+    path: &RepoPath,
+) -> Merge<ContentHunk> {
+    merge.map(|term| get_file_contents(store, path, term))
+}
+
+pub fn materialize(
+    conflict: &Merge<Option<TreeValue>>,
+    store: &Store,
+    path: &RepoPath,
+    output: &mut dyn Write,
+) -> std::io::Result<()> {
+    if let Some(file_merge) = conflict.to_file_merge() {
+        let content = extract_as_single_hunk(&file_merge, store, path);
+        materialize_merge_result(&content, output)
+    } else {
+        // Unless all terms are regular files, we can't do much better than to try to
+        // describe the merge.
+        conflict.describe(output)
+    }
+}
+
 pub fn materialize_merge_result(
     single_hunk: &Merge<ContentHunk>,
     output: &mut dyn Write,
@@ -249,3 +293,102 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge<ContentHunk> {
 
     Merge::new(removes, adds)
 }
+
+/// Returns `None` if there are no conflict markers in `content`.
+pub fn update_from_content(
+    conflict: &Merge<Option<TreeValue>>,
+    store: &Store,
+    path: &RepoPath,
+    content: &[u8],
+) -> BackendResult<Option<Merge<Option<TreeValue>>>> {
+    // TODO: Check that the conflict only involves files and convert it to a
+    // `Merge<Option<FileId>>` so we can remove the wildcard pattern in the loops
+    // further down.
+
+    // First check if the new content is unchanged compared to the old content. If
+    // it is, we don't need parse the content or write any new objects to the
+    // store. This is also a way of making sure that unchanged tree/file
+    // conflicts (for example) are not converted to regular files in the working
+    // copy.
+    let mut old_content = Vec::with_capacity(content.len());
+    materialize(conflict, store, path, &mut old_content).unwrap();
+    if content == old_content {
+        return Ok(Some(conflict.clone()));
+    }
+
+    let mut removed_content = vec![vec![]; conflict.removes().len()];
+    let mut added_content = vec![vec![]; conflict.adds().len()];
+    let Some(hunks) = parse_conflict(content, conflict.removes().len(), conflict.adds().len())
+    else {
+        // Either there are no self markers of they don't have the expected arity
+        return Ok(None);
+    };
+    for hunk in hunks {
+        if let Some(slice) = hunk.as_resolved() {
+            for buf in &mut removed_content {
+                buf.extend_from_slice(&slice.0);
+            }
+            for buf in &mut added_content {
+                buf.extend_from_slice(&slice.0);
+            }
+        } else {
+            let (removes, adds) = hunk.take();
+            for (i, buf) in removes.into_iter().enumerate() {
+                removed_content[i].extend(buf.0);
+            }
+            for (i, buf) in adds.into_iter().enumerate() {
+                added_content[i].extend(buf.0);
+            }
+        }
+    }
+    // Now write the new files contents we found by parsing the file
+    // with conflict markers. Update the Merge object with the new
+    // FileIds.
+    let mut new_removes = vec![];
+    for (i, buf) in removed_content.iter().enumerate() {
+        match &conflict.removes()[i] {
+            Some(TreeValue::File { id: _, executable }) => {
+                let file_id = store.write_file(path, &mut buf.as_slice())?;
+                let new_value = TreeValue::File {
+                    id: file_id,
+                    executable: *executable,
+                };
+                new_removes.push(Some(new_value));
+            }
+            None if buf.is_empty() => {
+                // The missing side of a conflict is still represented by
+                // the empty string we materialized it as
+                new_removes.push(None);
+            }
+            _ => {
+                // The user edited a non-file side. This should never happen. We consider the
+                // conflict resolved for now.
+                return Ok(None);
+            }
+        }
+    }
+    let mut new_adds = vec![];
+    for (i, buf) in added_content.iter().enumerate() {
+        match &conflict.adds()[i] {
+            Some(TreeValue::File { id: _, executable }) => {
+                let file_id = store.write_file(path, &mut buf.as_slice())?;
+                let new_value = TreeValue::File {
+                    id: file_id,
+                    executable: *executable,
+                };
+                new_adds.push(Some(new_value));
+            }
+            None if buf.is_empty() => {
+                // The missing side of a conflict is still represented by
+                // the empty string we materialized it as => nothing to do
+                new_adds.push(None);
+            }
+            _ => {
+                // The user edited a non-file side. This should never happen. We consider the
+                // conflict resolved for now.
+                return Ok(None);
+            }
+        }
+    }
+    Ok(Some(Merge::new(new_removes, new_adds)))
+}

lib/src/merge.rs

@@ -22,13 +22,12 @@ use std::sync::Arc;
 
 use itertools::Itertools;
 
-use crate::backend::{BackendError, BackendResult, FileId, ObjectId, TreeId, TreeValue};
+use crate::backend;
+use crate::backend::{BackendError, FileId, ObjectId, TreeId, TreeValue};
 use crate::content_hash::ContentHash;
-use crate::files::ContentHunk;
 use crate::repo_path::RepoPath;
 use crate::store::Store;
 use crate::tree::Tree;
-use crate::{backend, conflicts};
 
 /// Attempt to resolve trivial conflicts between the inputs. There must be
 /// exactly one more adds than removes.
@@ -336,22 +335,6 @@ impl Merge<Option<TreeValue>> {
         backend::Conflict { removes, adds }
     }
 
-    pub fn materialize(
-        &self,
-        store: &Store,
-        path: &RepoPath,
-        output: &mut dyn Write,
-    ) -> std::io::Result<()> {
-        if let Some(file_merge) = self.to_file_merge() {
-            let content = file_merge.extract_as_single_hunk(store, path);
-            conflicts::materialize_merge_result(&content, output)
-        } else {
-            // Unless all terms are regular files, we can't do much better than to try to
-            // describe the merge.
-            self.describe(output)
-        }
-    }
-
     pub fn to_file_merge(&self) -> Option<Merge<Option<FileId>>> {
         self.maybe_map(|term| match term {
             None => Some(None),
@@ -374,112 +357,6 @@ impl Merge<Option<TreeValue>> {
         }
         Ok(())
     }
-
-    /// Returns `None` if there are no conflict markers in `content`.
-    pub fn update_from_content(
-        &self,
-        store: &Store,
-        path: &RepoPath,
-        content: &[u8],
-    ) -> BackendResult<Option<Merge<Option<TreeValue>>>> {
-        // TODO: Check that the conflict only involves files and convert it to a
-        // `Merge<Option<FileId>>` so we can remove the wildcard pattern in the loops
-        // further down.
-
-        // First check if the new content is unchanged compared to the old content. If
-        // it is, we don't need parse the content or write any new objects to the
-        // store. This is also a way of making sure that unchanged tree/file
-        // conflicts (for example) are not converted to regular files in the working
-        // copy.
-        let mut old_content = Vec::with_capacity(content.len());
-        self.materialize(store, path, &mut old_content).unwrap();
-        if content == old_content {
-            return Ok(Some(self.clone()));
-        }
-
-        let mut removed_content = vec![vec![]; self.removes().len()];
-        let mut added_content = vec![vec![]; self.adds().len()];
-        let Some(hunks) =
-            conflicts::parse_conflict(content, self.removes().len(), self.adds().len())
-        else {
-            // Either there are no self markers of they don't have the expected arity
-            return Ok(None);
-        };
-        for hunk in hunks {
-            if let Some(slice) = hunk.as_resolved() {
-                for buf in &mut removed_content {
-                    buf.extend_from_slice(&slice.0);
-                }
-                for buf in &mut added_content {
-                    buf.extend_from_slice(&slice.0);
-                }
-            } else {
-                let (removes, adds) = hunk.take();
-                for (i, buf) in removes.into_iter().enumerate() {
-                    removed_content[i].extend(buf.0);
-                }
-                for (i, buf) in adds.into_iter().enumerate() {
-                    added_content[i].extend(buf.0);
-                }
-            }
-        }
-        // Now write the new files contents we found by parsing the file
-        // with conflict markers. Update the Merge object with the new
-        // FileIds.
-        let mut new_removes = vec![];
-        for (i, buf) in removed_content.iter().enumerate() {
-            match &self.removes()[i] {
-                Some(TreeValue::File { id: _, executable }) => {
-                    let file_id = store.write_file(path, &mut buf.as_slice())?;
-                    let new_value = TreeValue::File {
-                        id: file_id,
-                        executable: *executable,
-                    };
-                    new_removes.push(Some(new_value));
-                }
-                None if buf.is_empty() => {
-                    // The missing side of a conflict is still represented by
-                    // the empty string we materialized it as
-                    new_removes.push(None);
-                }
-                _ => {
-                    // The user edited a non-file side. This should never happen. We consider the
-                    // conflict resolved for now.
-                    return Ok(None);
-                }
-            }
-        }
-        let mut new_adds = vec![];
-        for (i, buf) in added_content.iter().enumerate() {
-            match &self.adds()[i] {
-                Some(TreeValue::File { id: _, executable }) => {
-                    let file_id = store.write_file(path, &mut buf.as_slice())?;
-                    let new_value = TreeValue::File {
-                        id: file_id,
-                        executable: *executable,
-                    };
-                    new_adds.push(Some(new_value));
-                }
-                None if buf.is_empty() => {
-                    // The missing side of a conflict is still represented by
-                    // the empty string we materialized it as => nothing to do
-                    new_adds.push(None);
-                }
-                _ => {
-                    // The user edited a non-file side. This should never happen. We consider the
-                    // conflict resolved for now.
-                    return Ok(None);
-                }
-            }
-        }
-        Ok(Some(Merge::new(new_removes, new_adds)))
-    }
-}
-
-impl Merge<Option<FileId>> {
-    pub fn extract_as_single_hunk(&self, store: &Store, path: &RepoPath) -> Merge<ContentHunk> {
-        self.map(|term| get_file_contents(store, path, term))
-    }
 }
 
 impl<T> Merge<Option<T>>
@@ -549,23 +426,6 @@ fn describe_conflict_term(value: &TreeValue) -> String {
     }
 }
 
-fn get_file_contents(store: &Store, path: &RepoPath, term: &Option<FileId>) -> ContentHunk {
-    match term {
-        Some(id) => {
-            let mut content = vec![];
-            store
-                .read_file(path, id)
-                .unwrap()
-                .read_to_end(&mut content)
-                .unwrap();
-            ContentHunk(content)
-        }
-        // If the conflict had removed the file on one side, we pretend that the file
-        // was empty there.
-        None => ContentHunk(vec![]),
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

lib/src/working_copy.rs

@@ -42,6 +42,7 @@ use tracing::{instrument, trace_span};
 use crate::backend::{
     BackendError, ConflictId, FileId, MillisSinceEpoch, ObjectId, SymlinkId, TreeId, TreeValue,
 };
+use crate::conflicts;
 #[cfg(feature = "watchman")]
 use crate::fsmonitor::watchman;
 use crate::fsmonitor::FsmonitorKind;
@@ -967,9 +968,9 @@ impl TreeState {
         let mut content = vec![];
         file.read_to_end(&mut content).unwrap();
         let conflict = self.store.read_conflict(repo_path, &conflict_id)?;
-        if let Some(new_conflict) = conflict
-            .update_from_content(self.store.as_ref(), repo_path, &content)
-            .unwrap()
+        if let Some(new_conflict) =
+            conflicts::update_from_content(&conflict, self.store.as_ref(), repo_path, &content)
+                .unwrap()
         {
             if new_conflict != conflict {
                 let new_conflict_id = self.store.write_conflict(repo_path, &new_conflict)?;
@@ -1109,8 +1110,7 @@ impl TreeState {
                 err,
             })?;
         let mut conflict_data = vec![];
-        conflict
-            .materialize(self.store.as_ref(), path, &mut conflict_data)
+        conflicts::materialize(&conflict, self.store.as_ref(), path, &mut conflict_data)
             .expect("Failed to materialize conflict to in-memory buffer");
         file.write_all(&conflict_data)
             .map_err(|err| CheckoutError::IoError {

lib/tests/test_conflicts.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use jj_lib::backend::{FileId, TreeValue};
-use jj_lib::conflicts::parse_conflict;
+use jj_lib::conflicts::{materialize, parse_conflict, update_from_content};
 use jj_lib::merge::Merge;
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
@@ -289,7 +289,7 @@ line 5 right
         vec![Some(file_value(&left_id)), Some(file_value(&right_id))],
     );
     let mut result: Vec<u8> = vec![];
-    conflict.materialize(store, &path, &mut result).unwrap();
+    materialize(&conflict, store, &path, &mut result).unwrap();
     insta::assert_snapshot!(
         String::from_utf8(result.clone()).unwrap(),
         @r###"
@@ -662,28 +662,23 @@ fn test_update_conflict_from_content() {
     // If the content is unchanged compared to the materialized value, we get the
     // old conflict id back.
     let mut materialized = vec![];
-    conflict
-        .materialize(store, &path, &mut materialized)
-        .unwrap();
-    let result = conflict
-        .update_from_content(store, &path, &materialized)
-        .unwrap();
+    materialize(&conflict, store, &path, &mut materialized).unwrap();
+    let result = update_from_content(&conflict, store, &path, &materialized).unwrap();
     assert_eq!(result, Some(conflict.clone()));
 
     // If the conflict is resolved, we get None back to indicate that.
-    let result = conflict
-        .update_from_content(store, &path, b"resolved 1\nline 2\nresolved 3\n")
-        .unwrap();
+    let result =
+        update_from_content(&conflict, store, &path, b"resolved 1\nline 2\nresolved 3\n").unwrap();
     assert_eq!(result, None);
 
     // If the conflict is partially resolved, we get a new conflict back.
-    let result = conflict
-        .update_from_content(
-            store,
-            &path,
-            b"resolved 1\nline 2\n<<<<<<<\n%%%%%%%\n-line 3\n+left 3\n+++++++\nright 3\n>>>>>>>\n",
-        )
-        .unwrap();
+    let result = update_from_content(
+        &conflict,
+        store,
+        &path,
+        b"resolved 1\nline 2\n<<<<<<<\n%%%%%%%\n-line 3\n+left 3\n+++++++\nright 3\n>>>>>>>\n",
+    )
+    .unwrap();
     let new_conflict = result.unwrap();
     assert_ne!(new_conflict, conflict);
     // Calculate expected new FileIds
@@ -718,22 +713,16 @@ fn test_update_conflict_from_content_modify_delete() {
     // If the content is unchanged compared to the materialized value, we get the
     // old conflict id back.
     let mut materialized = vec![];
-    conflict
-        .materialize(store, &path, &mut materialized)
-        .unwrap();
-    let result = conflict
-        .update_from_content(store, &path, &materialized)
-        .unwrap();
+    materialize(&conflict, store, &path, &mut materialized).unwrap();
+    let result = update_from_content(&conflict, store, &path, &materialized).unwrap();
     assert_eq!(result, Some(conflict.clone()));
 
     // If the conflict is resolved, we get None back to indicate that.
-    let result = conflict
-        .update_from_content(store, &path, b"resolved\n")
-        .unwrap();
+    let result = update_from_content(&conflict, store, &path, b"resolved\n").unwrap();
     assert_eq!(result, None);
 
     // If the conflict is modified, we get a new conflict back.
-    let result = conflict.update_from_content(
+    let result = update_from_content(&conflict,
         store,
         &path,
         b"<<<<<<<\n%%%%%%%\n line 1\n-line 2 before\n+line 2 modified after\n line 3\n+++++++\n>>>>>>>\n",
@@ -760,6 +749,6 @@ fn materialize_conflict_string(
     conflict: &Merge<Option<TreeValue>>,
 ) -> String {
     let mut result: Vec<u8> = vec![];
-    conflict.materialize(store, path, &mut result).unwrap();
+    materialize(conflict, store, path, &mut result).unwrap();
     String::from_utf8(result).unwrap()
 }