diff --git a/cli/src/merge_tools.rs b/cli/src/merge_tools.rs index ba4d24938..f35649efd 100644 --- a/cli/src/merge_tools.rs +++ b/cli/src/merge_tools.rs @@ -325,12 +325,13 @@ pub fn run_mergetool( let mut new_tree_value: Option = None; if editor.merge_tool_edits_conflict_markers { - if let Some(new_conflict) = conflicts::update_from_content( - &conflict, + if let Some(new_file_ids) = conflicts::update_from_content( + &file_merge, tree.store(), repo_path, output_file_contents.as_slice(), )? { + let new_conflict = conflict.with_new_file_ids(&new_file_ids); let new_conflict_id = tree.store().write_conflict(repo_path, &new_conflict)?; new_tree_value = Some(TreeValue::Conflict(new_conflict_id)); } diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 95fc1a83c..a095a61f3 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -296,29 +296,26 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge { /// Returns `None` if there are no conflict markers in `content`. pub fn update_from_content( - conflict: &Merge>, + file_ids: &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. - +) -> BackendResult>>> { // 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(); + let merge_hunk = extract_as_single_hunk(file_ids, store, path); + materialize_merge_result(&merge_hunk, &mut old_content).unwrap(); if content == old_content { - return Ok(Some(conflict.clone())); + return Ok(Some(file_ids.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()) + let mut removed_content = vec![vec![]; file_ids.removes().len()]; + let mut added_content = vec![vec![]; file_ids.adds().len()]; + let Some(hunks) = parse_conflict(content, file_ids.removes().len(), file_ids.adds().len()) else { // Either there are no self markers of they don't have the expected arity return Ok(None); @@ -346,14 +343,10 @@ pub fn update_from_content( // FileIds. let mut new_removes = vec![]; for (i, buf) in removed_content.iter().enumerate() { - match &conflict.removes()[i] { - Some(TreeValue::File { id: _, executable }) => { + match &file_ids.removes()[i] { + Some(_) => { 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)); + new_removes.push(Some(file_id)); } None if buf.is_empty() => { // The missing side of a conflict is still represented by @@ -361,22 +354,18 @@ pub fn update_from_content( new_removes.push(None); } _ => { - // The user edited a non-file side. This should never happen. We consider the - // conflict resolved for now. + // The user edited the empty placeholder for an absent side. We consider the + // conflict resolved. 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 }) => { + match &file_ids.adds()[i] { + Some(_) => { 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)); + new_adds.push(Some(file_id)); } None if buf.is_empty() => { // The missing side of a conflict is still represented by @@ -384,8 +373,8 @@ pub fn update_from_content( new_adds.push(None); } _ => { - // The user edited a non-file side. This should never happen. We consider the - // conflict resolved for now. + // The user edited the empty placeholder for an absent side. We consider the + // conflict resolved. return Ok(None); } } diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 586cffd7e..607c38ab4 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -18,6 +18,7 @@ use std::borrow::Borrow; use std::collections::HashMap; use std::hash::Hash; use std::io::Write; +use std::iter::zip; use std::sync::Arc; use itertools::Itertools; @@ -346,6 +347,34 @@ impl Merge> { }) } + /// Creates a new merge with the file ids from the given merge. In other + /// words, only the executable bits from `self` will be preserved. + pub fn with_new_file_ids(&self, file_ids: &Merge>) -> Self { + assert_eq!(self.removes.len(), file_ids.removes.len()); + fn updated_terms( + old_tree_values: &[Option], + file_ids: &[Option], + ) -> Vec> { + let mut new_tree_values = vec![]; + for (tree_value, file_id) in zip(old_tree_values, file_ids) { + if let Some(TreeValue::File { id: _, executable }) = tree_value { + new_tree_values.push(Some(TreeValue::File { + id: file_id.as_ref().unwrap().clone(), + executable: *executable, + })); + } else { + assert!(tree_value.is_none()); + assert!(file_id.is_none()); + new_tree_values.push(None); + } + } + new_tree_values + } + let removes = updated_terms(&self.removes, &file_ids.removes); + let adds = updated_terms(&self.adds, &file_ids.adds); + Merge { removes, adds } + } + /// 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")?; diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index d7e0bead2..306501cf3 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -968,19 +968,28 @@ 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) = - 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)?; - Ok(TreeValue::Conflict(new_conflict_id)) + if let Some(old_file_ids) = conflict.to_file_merge() { + if let Some(new_file_ids) = conflicts::update_from_content( + &old_file_ids, + self.store.as_ref(), + repo_path, + &content, + ) + .unwrap() + { + if new_file_ids != old_file_ids { + let new_conflict = conflict.with_new_file_ids(&new_file_ids); + let new_conflict_id = self.store.write_conflict(repo_path, &new_conflict)?; + Ok(TreeValue::Conflict(new_conflict_id)) + } else { + Ok(TreeValue::Conflict(conflict_id)) + } } else { - Ok(TreeValue::Conflict(conflict_id)) + let id = self.store.write_file(repo_path, &mut content.as_slice())?; + Ok(TreeValue::File { id, executable }) } } else { - let id = self.store.write_file(repo_path, &mut content.as_slice())?; - Ok(TreeValue::File { id, executable }) + Ok(TreeValue::Conflict(conflict_id)) } } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 6e09c709b..8043263f0 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -12,21 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -use jj_lib::backend::{FileId, TreeValue}; -use jj_lib::conflicts::{materialize, parse_conflict, update_from_content}; +use jj_lib::backend::FileId; +use jj_lib::conflicts::{ + extract_as_single_hunk, materialize_merge_result, parse_conflict, update_from_content, +}; use jj_lib::merge::Merge; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; use jj_lib::store::Store; use testutils::TestRepo; -fn file_value(file_id: &FileId) -> TreeValue { - TreeValue::File { - id: file_id.clone(), - executable: false, - } -} - #[test] fn test_materialize_conflict_basic() { let test_repo = TestRepo::init(false); @@ -69,8 +64,8 @@ line 5 // The left side should come first. The diff should be use the smaller (right) // side, and the left side should be a snapshot. let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![Some(file_value(&left_id)), Some(file_value(&right_id))], + vec![Some(base_id.clone())], + vec![Some(left_id.clone()), Some(right_id.clone())], ); insta::assert_snapshot!( &materialize_conflict_string(store, &path, &conflict), @@ -93,8 +88,8 @@ line 5 // Swap the positive terms in the conflict. The diff should still use the right // side, but now the right side should come first. let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![Some(file_value(&right_id)), Some(file_value(&left_id))], + vec![Some(base_id.clone())], + vec![Some(right_id.clone()), Some(left_id.clone())], ); insta::assert_snapshot!( &materialize_conflict_string(store, &path, &conflict), @@ -162,12 +157,8 @@ line 3 // The order of (a, b, c) should be preserved. For all cases, the "a" side // should be a snapshot. let conflict = Merge::new( - vec![Some(file_value(&base_id)), Some(file_value(&base_id))], - vec![ - Some(file_value(&a_id)), - Some(file_value(&b_id)), - Some(file_value(&c_id)), - ], + vec![Some(base_id.clone()), Some(base_id.clone())], + vec![Some(a_id.clone()), Some(b_id.clone()), Some(c_id.clone())], ); insta::assert_snapshot!( &materialize_conflict_string(store, &path, &conflict), @@ -190,12 +181,8 @@ line 3 "### ); let conflict = Merge::new( - vec![Some(file_value(&base_id)), Some(file_value(&base_id))], - vec![ - Some(file_value(&c_id)), - Some(file_value(&b_id)), - Some(file_value(&a_id)), - ], + vec![Some(base_id.clone()), Some(base_id.clone())], + vec![Some(c_id.clone()), Some(b_id.clone()), Some(a_id.clone())], ); insta::assert_snapshot!( &materialize_conflict_string(store, &path, &conflict), @@ -218,12 +205,8 @@ line 3 "### ); let conflict = Merge::new( - vec![Some(file_value(&base_id)), Some(file_value(&base_id))], - vec![ - Some(file_value(&c_id)), - Some(file_value(&a_id)), - Some(file_value(&b_id)), - ], + vec![Some(base_id.clone()), Some(base_id.clone())], + vec![Some(c_id.clone()), Some(a_id.clone()), Some(b_id.clone())], ); insta::assert_snapshot!( &materialize_conflict_string(store, &path, &conflict), @@ -285,8 +268,8 @@ line 5 right ); let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![Some(file_value(&left_id)), Some(file_value(&right_id))], + vec![Some(base_id.clone())], + vec![Some(left_id.clone()), Some(right_id.clone())], ); let materialized = materialize_conflict_string(store, &path, &conflict); insta::assert_snapshot!( @@ -387,11 +370,8 @@ line 5 // left modifies a line, right deletes the same line. let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![ - Some(file_value(&modified_id)), - Some(file_value(&deleted_id)), - ], + vec![Some(base_id.clone())], + vec![Some(modified_id.clone()), Some(deleted_id.clone())], ); insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###" line 1 @@ -409,11 +389,8 @@ line 5 // right modifies a line, left deletes the same line. let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![ - Some(file_value(&deleted_id)), - Some(file_value(&modified_id)), - ], + vec![Some(base_id.clone())], + vec![Some(deleted_id.clone()), Some(modified_id.clone())], ); insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###" line 1 @@ -431,8 +408,8 @@ line 5 // modify/delete conflict at the file level let conflict = Merge::new( - vec![Some(file_value(&base_id))], - vec![Some(file_value(&modified_id)), None], + vec![Some(base_id.clone())], + vec![Some(modified_id.clone()), None], ); insta::assert_snapshot!(&materialize_conflict_string(store, &path, &conflict), @r###" <<<<<<< @@ -651,11 +628,8 @@ fn test_update_conflict_from_content() { let left_file_id = testutils::write_file(store, &path, "left 1\nline 2\nleft 3\n"); let right_file_id = testutils::write_file(store, &path, "right 1\nline 2\nright 3\n"); let conflict = Merge::new( - vec![Some(file_value(&base_file_id))], - vec![ - Some(file_value(&left_file_id)), - Some(file_value(&right_file_id)), - ], + vec![Some(base_file_id.clone())], + vec![Some(left_file_id.clone()), Some(right_file_id.clone())], ); // If the content is unchanged compared to the materialized value, we get the @@ -686,10 +660,10 @@ fn test_update_conflict_from_content() { assert_eq!( new_conflict, Merge::new( - vec![Some(file_value(&new_base_file_id))], + vec![Some(new_base_file_id.clone())], vec![ - Some(file_value(&new_left_file_id)), - Some(file_value(&new_right_file_id)) + Some(new_left_file_id.clone()), + Some(new_right_file_id.clone()) ] ) ); @@ -703,10 +677,7 @@ fn test_update_conflict_from_content_modify_delete() { let path = RepoPath::from_internal_string("dir/file"); let before_file_id = testutils::write_file(store, &path, "line 1\nline 2 before\nline 3\n"); let after_file_id = testutils::write_file(store, &path, "line 1\nline 2 after\nline 3\n"); - let conflict = Merge::new( - vec![Some(file_value(&before_file_id))], - vec![Some(file_value(&after_file_id)), None], - ); + let conflict = Merge::new(vec![Some(before_file_id)], vec![Some(after_file_id), None]); // If the content is unchanged compared to the materialized value, we get the // old conflict id back. @@ -734,8 +705,8 @@ fn test_update_conflict_from_content_modify_delete() { assert_eq!( new_conflict, Merge::new( - vec![Some(file_value(&new_base_file_id))], - vec![Some(file_value(&new_left_file_id)), None] + vec![Some(new_base_file_id.clone())], + vec![Some(new_left_file_id.clone()), None] ) ); } @@ -743,9 +714,10 @@ fn test_update_conflict_from_content_modify_delete() { fn materialize_conflict_string( store: &Store, path: &RepoPath, - conflict: &Merge>, + conflict: &Merge>, ) -> String { let mut result: Vec = vec![]; - materialize(conflict, store, path, &mut result).unwrap(); + let contents = extract_as_single_hunk(conflict, store, path); + materialize_merge_result(&contents, &mut result).unwrap(); String::from_utf8(result).unwrap() }