From 7106f6fd4924e00d901201df6d1f639a6a2776ee Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Fri, 31 May 2024 06:58:34 +0800 Subject: [PATCH] conflicts: handle parsing of simplified conflicts --- lib/src/conflicts.rs | 69 +++++++++++------ lib/tests/test_conflicts.rs | 146 ++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 22 deletions(-) diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 86a9a557e..6bc9bcee7 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -451,24 +451,40 @@ pub async fn update_from_content( path: &RepoPath, content: &[u8], ) -> BackendResult>> { + let simplified_file_ids = file_ids.clone().simplify(); + let simplified_file_ids = &simplified_file_ids; + // 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()); - let merge_hunk = extract_as_single_hunk(file_ids, store, path).await; + let merge_hunk = extract_as_single_hunk(simplified_file_ids, store, path).await; materialize_merge_result(&merge_hunk, &mut old_content).unwrap(); if content == old_content { return Ok(file_ids.clone()); } - let Some(hunks) = parse_conflict(content, file_ids.num_sides()) else { - // Either there are no self markers of they don't have the expected arity + // Parse conflicts from the new content using the arity of the simplified + // conflicts initially. If unsuccessful, attempt to parse conflicts from with + // the arity of the unsimplified conflicts since such a conflict may be + // present in the working copy if written by an earlier version of jj. + let (used_file_ids, hunks) = 'hunks: { + if let Some(hunks) = parse_conflict(content, simplified_file_ids.num_sides()) { + break 'hunks (simplified_file_ids, hunks); + }; + if simplified_file_ids.num_sides() != file_ids.num_sides() { + if let Some(hunks) = parse_conflict(content, file_ids.num_sides()) { + break 'hunks (file_ids, hunks); + }; + }; + // Either there are no markers or they don't have the expected arity let file_id = store.write_file(path, &mut &content[..])?; return Ok(Merge::normal(file_id)); }; - let mut contents = file_ids.map(|_| vec![]); + + let mut contents = used_file_ids.map(|_| vec![]); for hunk in hunks { if let Some(slice) = hunk.as_resolved() { for content in contents.iter_mut() { @@ -483,7 +499,7 @@ pub async fn update_from_content( // If the user edited the empty placeholder for an absent side, we consider the // conflict resolved. - if zip(contents.iter(), file_ids.iter()) + if zip(contents.iter(), used_file_ids.iter()) .any(|(content, file_id)| file_id.is_none() && !content.is_empty()) { let file_id = store.write_file(path, &mut &content[..])?; @@ -491,22 +507,31 @@ pub async fn update_from_content( } // Now write the new files contents we found by parsing the file with conflict - // markers. Update the Merge object with the new FileIds. - let builder: BackendResult>> = - zip(contents.iter(), file_ids.iter()) - .map(|(content, file_id)| { - match file_id { - Some(_) => { - let file_id = store.write_file(path, &mut content.as_slice())?; - Ok(Some(file_id)) - } - None => { - // The missing side of a conflict is still represented by - // the empty string we materialized it as - Ok(None) - } + // markers. + let new_file_ids: Vec> = zip(contents.iter(), used_file_ids.iter()) + .map(|(content, file_id)| -> BackendResult> { + match file_id { + Some(_) => { + let file_id = store.write_file(path, &mut content.as_slice())?; + Ok(Some(file_id)) } - }) - .collect(); - Ok(builder?.build()) + None => { + // The missing side of a conflict is still represented by + // the empty string we materialized it as + Ok(None) + } + } + }) + .try_collect()?; + + // If the conflict was simplified, expand the conflict to the original + // number of sides. + let new_file_ids = if new_file_ids.len() != file_ids.iter().len() { + file_ids + .clone() + .update_from_simplified(Merge::from_vec(new_file_ids)) + } else { + Merge::from_vec(new_file_ids) + }; + Ok(new_file_ids) } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index deda8fe53..78924f34c 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -839,6 +839,152 @@ fn test_update_conflict_from_content_modify_delete() { ); } +#[test] +fn test_update_conflict_from_content_simplified_conflict() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let path = RepoPath::from_internal_string("dir/file"); + let base_file_id = testutils::write_file(store, path, "line 1\nline 2\nline 3\n"); + 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"); + // Conflict: left - base + base - base + right + let conflict = Merge::from_removes_adds( + vec![Some(base_file_id.clone()), Some(base_file_id.clone())], + vec![ + Some(left_file_id.clone()), + Some(base_file_id.clone()), + Some(right_file_id.clone()), + ], + ); + let simplified_conflict = conflict.clone().simplify(); + + // If the content is unchanged compared to the materialized value, we get the + // old conflict id back. Both the simplified and unsimplified materialized + // conflicts should return the old conflict id. + let materialized = materialize_conflict_string(store, path, &conflict); + let materialized_simplified = materialize_conflict_string(store, path, &simplified_conflict); + let parse = |content| { + update_from_content(&conflict, store, path, content) + .block_on() + .unwrap() + }; + insta::assert_snapshot!( + materialized, + @r###" + <<<<<<< Conflict 1 of 2 + +++++++ Contents of side #1 + left 1 + %%%%%%% Changes from base #1 to side #2 + line 1 + %%%%%%% Changes from base #2 to side #3 + -line 1 + +right 1 + >>>>>>> Conflict 1 of 2 ends + line 2 + <<<<<<< Conflict 2 of 2 + +++++++ Contents of side #1 + left 3 + %%%%%%% Changes from base #1 to side #2 + line 3 + %%%%%%% Changes from base #2 to side #3 + -line 3 + +right 3 + >>>>>>> Conflict 2 of 2 ends + "### + ); + insta::assert_snapshot!( + materialized_simplified, + @r###" + <<<<<<< Conflict 1 of 2 + %%%%%%% Changes from base to side #1 + -line 1 + +left 1 + +++++++ Contents of side #2 + right 1 + >>>>>>> Conflict 1 of 2 ends + line 2 + <<<<<<< Conflict 2 of 2 + %%%%%%% Changes from base to side #1 + -line 3 + +left 3 + +++++++ Contents of side #2 + right 3 + >>>>>>> Conflict 2 of 2 ends + "### + ); + assert_eq!(parse(materialized.as_bytes()), conflict); + assert_eq!(parse(materialized_simplified.as_bytes()), conflict); + + // If the conflict is resolved, we get a normal merge back to indicate that. + let expected_file_id = testutils::write_file(store, path, "resolved 1\nline 2\nresolved 3\n"); + assert_eq!( + parse(b"resolved 1\nline 2\nresolved 3\n"), + Merge::normal(expected_file_id) + ); + + // If the conflict is partially resolved, we get a new conflict back. + // This should work with both the simplified and unsimplified conflict. + let new_conflict = parse(indoc! {b" + resolved 1 + line 2 + <<<<<<< Conflict 2 of 2 + +++++++ Contents of side #1 + edited left 3 + %%%%%%% Changes from base #1 to side #2 + edited line 3 + %%%%%%% Changes from base #2 to side #3 + -edited line 3 + +edited right 3 + >>>>>>> Conflict 2 of 2 ends + "}); + let new_simplified_conflict = parse(indoc! {b" + resolved 1 + line 2 + <<<<<<< Conflict 2 of 2 + %%%%%%% Changes from base to side #1 + -edited line 3 + +edited left 3 + +++++++ Contents of side #2 + edited right 3 + >>>>>>> Conflict 2 of 2 ends + "}); + assert_ne!(new_conflict, conflict); + assert_ne!(new_simplified_conflict, conflict); + // Calculate expected new FileIds + let new_base_file_id = + testutils::write_file(store, path, "resolved 1\nline 2\nedited line 3\n"); + let new_left_file_id = + testutils::write_file(store, path, "resolved 1\nline 2\nedited left 3\n"); + let new_right_file_id = + testutils::write_file(store, path, "resolved 1\nline 2\nedited right 3\n"); + assert_eq!( + new_conflict, + Merge::from_removes_adds( + vec![ + Some(new_base_file_id.clone()), + Some(new_base_file_id.clone()) + ], + vec![ + Some(new_left_file_id.clone()), + Some(new_base_file_id.clone()), + Some(new_right_file_id.clone()) + ] + ) + ); + assert_eq!( + new_simplified_conflict, + Merge::from_removes_adds( + vec![Some(base_file_id.clone()), Some(new_base_file_id.clone())], + vec![ + Some(new_left_file_id.clone()), + Some(base_file_id.clone()), + Some(new_right_file_id.clone()) + ] + ) + ); +} + fn materialize_conflict_string( store: &Store, path: &RepoPath,