From a995c666350bd4ed77cf8325c9488dbaa4c69637 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 9 Aug 2023 23:46:25 -0700 Subject: [PATCH] merge: move some methods back to `conflicts` as free functions I think I moved way too many functions onto `Merge>` in 82883e648da4. This effectively reverts almost all of that commit. The `Merge` 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 | 6 +- cli/src/diff_util.rs | 10 +-- cli/src/merge_tools.rs | 8 +- lib/src/conflicts.rs | 143 +++++++++++++++++++++++++++++++++++ lib/src/merge.rs | 144 +----------------------------------- lib/src/working_copy.rs | 10 +-- lib/tests/test_conflicts.rs | 47 +++++------- 7 files changed, 178 insertions(+), 190 deletions(-) diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index fada8a3f1..aae2b1ce1 100644 --- a/cli/src/commands/mod.rs +++ b/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)?; } diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index d78e79bae..c67f164e8 100644 --- a/cli/src/diff_util.rs +++ b/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(); diff --git a/cli/src/merge_tools.rs b/cli/src/merge_tools.rs index 42d67f7e0..ba4d24938 100644 --- a/cli/src/merge_tools.rs +++ b/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 = if editor.merge_tool_edits_conflict_markers { @@ -324,7 +325,8 @@ pub fn run_mergetool( let mut new_tree_value: Option = 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(), diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 6d1257ed4..95fc1a83c 100644 --- a/lib/src/conflicts.rs +++ b/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) -> 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>, + store: &Store, + path: &RepoPath, +) -> Merge { + merge.map(|term| get_file_contents(store, path, term)) +} + +pub fn materialize( + conflict: &Merge>, + 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, output: &mut dyn Write, @@ -249,3 +293,102 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge { Merge::new(removes, adds) } + +/// Returns `None` if there are no conflict markers in `content`. +pub fn update_from_content( + conflict: &Merge>, + store: &Store, + path: &RepoPath, + content: &[u8], +) -> BackendResult>>> { + // TODO: Check that the conflict only involves files and convert it to a + // `Merge>` 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))) +} diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 7721b075d..586cffd7e 100644 --- a/lib/src/merge.rs +++ b/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> { 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>> { self.maybe_map(|term| match term { None => Some(None), @@ -374,112 +357,6 @@ impl Merge> { } Ok(()) } - - /// Returns `None` if there are no conflict markers in `content`. - pub fn update_from_content( - &self, - store: &Store, - path: &RepoPath, - content: &[u8], - ) -> BackendResult>>> { - // TODO: Check that the conflict only involves files and convert it to a - // `Merge>` 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> { - pub fn extract_as_single_hunk(&self, store: &Store, path: &RepoPath) -> Merge { - self.map(|term| get_file_contents(store, path, term)) - } } impl Merge> @@ -549,23 +426,6 @@ fn describe_conflict_term(value: &TreeValue) -> String { } } -fn get_file_contents(store: &Store, path: &RepoPath, term: &Option) -> 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::*; diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 29676d52e..d7e0bead2 100644 --- a/lib/src/working_copy.rs +++ b/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 { diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 08d7cc233..6e1ef9a09 100644 --- a/lib/tests/test_conflicts.rs +++ b/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 = 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>, ) -> String { let mut result: Vec = vec![]; - conflict.materialize(store, path, &mut result).unwrap(); + materialize(conflict, store, path, &mut result).unwrap(); String::from_utf8(result).unwrap() }