From b99285a9095d29c910a14a3765e6d033c50db941 Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Wed, 27 Nov 2024 14:15:49 -0600 Subject: [PATCH] local_working_copy: store materialized conflict marker length Storing the conflict marker length in the working copy makes conflict parsing more consistent, and it allows us to parse valid conflict hunks even if the user left some invalid conflict markers in the file while resolving the conflicts. --- cli/src/commands/debug/local_working_copy.rs | 3 +- cli/src/merge_tools/external.rs | 20 ++- cli/tests/test_working_copy.rs | 180 +++++++++++++++++++ lib/src/conflicts.rs | 26 ++- lib/src/local_working_copy.rs | 106 +++++++++-- lib/src/protos/working_copy.proto | 6 + lib/src/protos/working_copy.rs | 9 + lib/tests/test_conflicts.rs | 105 ++++++++--- 8 files changed, 412 insertions(+), 43 deletions(-) diff --git a/cli/src/commands/debug/local_working_copy.rs b/cli/src/commands/debug/local_working_copy.rs index 98408799b2..eee9a8de51 100644 --- a/cli/src/commands/debug/local_working_copy.rs +++ b/cli/src/commands/debug/local_working_copy.rs @@ -40,10 +40,11 @@ pub fn cmd_debug_local_working_copy( for (file, state) in wc.file_states()? { writeln!( ui.stdout(), - "{:?} {:13?} {:10?} {:?}", + "{:?} {:13?} {:10?} {:?} {:?}", state.file_type, state.size, state.mtime.0, + state.materialized_conflict_data, file )?; } diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index ff4db30619..5f388976c9 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -12,8 +12,10 @@ use jj_lib::backend::FileId; use jj_lib::backend::MergedTreeId; use jj_lib::backend::TreeValue; use jj_lib::conflicts; -use jj_lib::conflicts::materialize_merge_result_to_bytes; +use jj_lib::conflicts::choose_materialized_conflict_marker_len; +use jj_lib::conflicts::materialize_merge_result_to_bytes_with_marker_len; use jj_lib::conflicts::ConflictMarkerStyle; +use jj_lib::conflicts::MIN_CONFLICT_MARKER_LEN; use jj_lib::gitignore::GitIgnoreFile; use jj_lib::matchers::Matcher; use jj_lib::merge::Merge; @@ -181,8 +183,21 @@ pub fn run_mergetool_external( .conflict_marker_style .unwrap_or(default_conflict_marker_style); + // If the merge tool doesn't get conflict markers pre-populated in the output + // file, we should default to accepting MIN_CONFLICT_MARKER_LEN since the + // merge tool is unlikely to know about our rules for conflict marker length. + // In the future, we may want to add a "$markerLength" variable. + let conflict_marker_len = if editor.merge_tool_edits_conflict_markers { + choose_materialized_conflict_marker_len(&content) + } else { + MIN_CONFLICT_MARKER_LEN + }; let initial_output_content = if editor.merge_tool_edits_conflict_markers { - materialize_merge_result_to_bytes(&content, conflict_marker_style) + materialize_merge_result_to_bytes_with_marker_len( + &content, + conflict_marker_style, + conflict_marker_len, + ) } else { BString::default() }; @@ -257,6 +272,7 @@ pub fn run_mergetool_external( repo_path, output_file_contents.as_slice(), conflict_marker_style, + conflict_marker_len, ) .block_on()? } else { diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs index 16d54149e6..c135cbd26f 100644 --- a/cli/tests/test_working_copy.rs +++ b/cli/tests/test_working_copy.rs @@ -13,6 +13,7 @@ // limitations under the License. use indoc::indoc; +use regex::Regex; use crate::common::TestEnvironment; @@ -253,3 +254,182 @@ fn test_snapshot_invalid_ignore_pattern() { 2: invalid utf-8 sequence of 1 bytes from index 0 "##); } + +#[test] +fn test_conflict_marker_length_stored_in_working_copy() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + // Create a conflict in the working copy with long markers on one side + let conflict_file = repo_path.join("file"); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + line 2 + line 3 + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "base"]); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + line 2 - left + line 3 - left + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "side-a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "description(base)", "-m", "side-b"]); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + ======= fake marker + line 2 - right + ======= fake marker + line 3 + "}, + ) + .unwrap(); + test_env.jj_cmd_ok( + &repo_path, + &["new", "description(side-a)", "description(side-b)"], + ); + + // File should be materialized with long conflict markers + insta::assert_snapshot!(std::fs::read_to_string(&conflict_file).unwrap(), @r##" + line 1 + <<<<<<<<<<< Conflict 1 of 1 + %%%%%%%%%%% Changes from base to side #1 + -line 2 + -line 3 + +line 2 - left + +line 3 - left + +++++++++++ Contents of side #2 + ======= fake marker + line 2 - right + ======= fake marker + line 3 + >>>>>>>>>>> Conflict 1 of 1 ends + "##); + + // The timestamps in the `jj debug local-working-copy` output change, so we want + // to remove them before asserting the snapshot + let timestamp_regex = Regex::new(r"\b\d{10,}\b").unwrap(); + // On Windows, executable is always `()`, but on Unix-like systems, it's `true` + // or `false`, so we want to remove it from the output as well + let executable_regex = Regex::new("executable: [^ ]+").unwrap(); + + let redact_output = |output: &str| { + let output = timestamp_regex.replace_all(output, ""); + let output = executable_regex.replace_all(&output, ""); + output.into_owned() + }; + + // Working copy should contain conflict marker length + let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]); + insta::assert_snapshot!(redact_output(&stdout), @r#" + Current operation: OperationId("da133d2605b63b84c53b512007b32bd5822e4821d7f8ca69b03a0bbd702cd61fad7857e430e911011aaecf3bf6942e81a95180792c7e0056af18bc956ee834a4") + Current tree: Merge(Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("f56b8223da0dab22b03b8323ced4946329aeb4e0")])) + Normal { } 249 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" + "#); + + // Update the conflict with more fake markers, and it should still parse + // correctly (the markers should be ignored) + std::fs::write( + &conflict_file, + indoc! {" + line 1 + <<<<<<<<<<< Conflict 1 of 1 + %%%%%%%%%%% Changes from base to side #1 + -line 2 + -line 3 + +line 2 - left + +line 3 - left + +++++++++++ Contents of side #2 + <<<<<<< fake marker + ||||||| fake marker + line 2 - right + ======= fake marker + line 3 + >>>>>>> fake marker + >>>>>>>>>>> Conflict 1 of 1 ends + "}, + ) + .unwrap(); + + // The file should still be conflicted, and the new content should be saved + let stdout = test_env.jj_cmd_success(&repo_path, &["st"]); + insta::assert_snapshot!(stdout, @r#" + Working copy changes: + M file + There are unresolved conflicts at these paths: + file 2-sided conflict + Working copy : mzvwutvl b7dadc87 (conflict) (no description set) + Parent commit: rlvkpnrz ce613b49 side-a + Parent commit: zsuskuln 7b2b03ab side-b + "#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r##" + diff --git a/file b/file + --- a/file + +++ b/file + @@ -6,8 +6,10 @@ + +line 2 - left + +line 3 - left + +++++++++++ Contents of side #2 + -======= fake marker + +<<<<<<< fake marker + +||||||| fake marker + line 2 - right + ======= fake marker + line 3 + +>>>>>>> fake marker + >>>>>>>>>>> Conflict 1 of 1 ends + "##); + + // Working copy should still contain conflict marker length + let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]); + insta::assert_snapshot!(redact_output(&stdout), @r#" + Current operation: OperationId("65b1b6a0da226e45694fda78d85efa5397176204b166f107b10c8ac0dcecfcfa9346b59317d8b572711666a3e5f168bcb561c278095a83363885911e246b2230") + Current tree: Merge(Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("3329c18c95f7b7a55c278c2259e9c4ce711fae59")])) + Normal { } 289 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" + "#); + + // Resolve the conflict + std::fs::write( + &conflict_file, + indoc! {" + line 1 + <<<<<<< fake marker + ||||||| fake marker + line 2 - left + line 2 - right + ======= fake marker + line 3 - left + >>>>>>> fake marker + "}, + ) + .unwrap(); + + let stdout = test_env.jj_cmd_success(&repo_path, &["st"]); + insta::assert_snapshot!(stdout, @r#" + Working copy changes: + M file + Working copy : mzvwutvl 1aefd866 (no description set) + Parent commit: rlvkpnrz ce613b49 side-a + Parent commit: zsuskuln 7b2b03ab side-b + "#); + + // When the file is resolved, the conflict marker length is removed from the + // working copy + let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]); + insta::assert_snapshot!(redact_output(&stdout), @r#" + Current operation: OperationId("6dc38b23e076d05a7c80327559e6de48d2fbc0811b06e9319bdbbff392bc991385e1ecbc378613101ba862e07dad1e6703247c5239a5a672a4761411815fe9fa") + Current tree: Merge(Resolved(TreeId("6120567b3cb2472d549753ed3e4b84183d52a650"))) + Normal { } 130 None "file" + "#); +} diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 0438f802ac..1a081c43fe 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -344,7 +344,7 @@ pub fn materialize_merge_result>( } } -fn materialize_merge_result_with_marker_len>( +pub fn materialize_merge_result_with_marker_len>( single_hunk: &Merge, conflict_marker_style: ConflictMarkerStyle, conflict_marker_len: usize, @@ -381,6 +381,28 @@ pub fn materialize_merge_result_to_bytes>( } } +pub fn materialize_merge_result_to_bytes_with_marker_len>( + single_hunk: &Merge, + conflict_marker_style: ConflictMarkerStyle, + conflict_marker_len: usize, +) -> BString { + let merge_result = files::merge(single_hunk); + match merge_result { + MergeResult::Resolved(content) => content, + MergeResult::Conflict(hunks) => { + let mut output = Vec::new(); + materialize_conflict_hunks( + &hunks, + conflict_marker_style, + conflict_marker_len, + &mut output, + ) + .expect("writing to an in-memory buffer should never fail"); + output.into() + } + } +} + fn materialize_conflict_hunks( hunks: &[Merge], conflict_marker_style: ConflictMarkerStyle, @@ -824,6 +846,7 @@ pub async fn update_from_content( path: &RepoPath, content: &[u8], conflict_marker_style: ConflictMarkerStyle, + conflict_marker_len: usize, ) -> BackendResult>> { let simplified_file_ids = file_ids.clone().simplify(); let simplified_file_ids = &simplified_file_ids; @@ -835,7 +858,6 @@ pub async fn update_from_content( // copy. let mut old_content = Vec::with_capacity(content.len()); let merge_hunk = extract_as_single_hunk(simplified_file_ids, store, path).await?; - let conflict_marker_len = choose_materialized_conflict_marker_len(&merge_hunk); materialize_merge_result_with_marker_len( &merge_hunk, conflict_marker_style, diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index f07af6d0ee..5fa8251abd 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -66,10 +66,12 @@ use crate::backend::TreeId; use crate::backend::TreeValue; use crate::commit::Commit; use crate::conflicts; -use crate::conflicts::materialize_merge_result_to_bytes; +use crate::conflicts::choose_materialized_conflict_marker_len; +use crate::conflicts::materialize_merge_result_to_bytes_with_marker_len; use crate::conflicts::materialize_tree_value; use crate::conflicts::ConflictMarkerStyle; use crate::conflicts::MaterializedTreeValue; +use crate::conflicts::MIN_CONFLICT_MARKER_LEN; use crate::file_util::check_symlink_support; use crate::file_util::try_symlink; #[cfg(feature = "watchman")] @@ -125,11 +127,17 @@ pub enum FileType { GitSubmodule, } +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub struct MaterializedConflictData { + pub conflict_marker_len: u32, +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct FileState { pub file_type: FileType, pub mtime: MillisSinceEpoch, pub size: u64, + pub materialized_conflict_data: Option, /* TODO: What else do we need here? Git stores a lot of fields. * TODO: Could possibly handle case-insensitive file systems keeping an * Option with the actual path here. */ @@ -147,10 +155,16 @@ impl FileState { file_type: FileType::Normal { executable }, mtime: MillisSinceEpoch(0), size: 0, + materialized_conflict_data: None, } } - fn for_file(executable: bool, size: u64, metadata: &Metadata) -> Self { + fn for_file( + executable: bool, + size: u64, + metadata: &Metadata, + materialized_conflict_data: Option, + ) -> Self { #[cfg(windows)] let executable = { // Windows doesn't support executable bit. @@ -160,6 +174,7 @@ impl FileState { file_type: FileType::Normal { executable }, mtime: mtime_from_metadata(metadata), size, + materialized_conflict_data, } } @@ -171,6 +186,7 @@ impl FileState { file_type: FileType::Symlink, mtime: mtime_from_metadata(metadata), size: metadata.len(), + materialized_conflict_data: None, } } @@ -179,6 +195,7 @@ impl FileState { file_type: FileType::GitSubmodule, mtime: MillisSinceEpoch(0), size: 0, + materialized_conflict_data: None, } } } @@ -418,6 +435,11 @@ fn file_state_from_proto(proto: &crate::protos::working_copy::FileState) -> File file_type, mtime: MillisSinceEpoch(proto.mtime_millis_since_epoch), size: proto.size, + materialized_conflict_data: proto.materialized_conflict_data.as_ref().map(|data| { + MaterializedConflictData { + conflict_marker_len: data.conflict_marker_len, + } + }), } } @@ -436,6 +458,11 @@ fn file_state_to_proto(file_state: &FileState) -> crate::protos::working_copy::F proto.file_type = file_type as i32; proto.mtime_millis_since_epoch = file_state.mtime.0; proto.size = file_state.size; + proto.materialized_conflict_data = file_state.materialized_conflict_data.map(|data| { + crate::protos::working_copy::MaterializedConflictData { + conflict_marker_len: data.conflict_marker_len, + } + }); proto } @@ -651,7 +678,10 @@ fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { ) } -fn file_state(metadata: &Metadata) -> Option { +fn file_state( + metadata: &Metadata, + materialized_conflict_data: Option, +) -> Option { let metadata_file_type = metadata.file_type(); let file_type = if metadata_file_type.is_dir() { None @@ -676,6 +706,7 @@ fn file_state(metadata: &Metadata) -> Option { file_type, mtime, size, + materialized_conflict_data, } }) } @@ -1250,8 +1281,12 @@ impl FileSnapshotter<'_> { max_size: self.max_new_file_size, }; self.untracked_paths_tx.send((path, reason)).ok(); - Ok(None) - } else if let Some(new_file_state) = file_state(&metadata) { + return Ok(None); + } + let materialized_conflict_data = maybe_current_file_state + .as_ref() + .and_then(|state| state.materialized_conflict_data); + if let Some(new_file_state) = file_state(&metadata, materialized_conflict_data) { self.process_present_file( path, &entry.path(), @@ -1286,7 +1321,9 @@ impl FileSnapshotter<'_> { }); } }; - if let Some(new_file_state) = metadata.as_ref().and_then(file_state) { + if let Some(new_file_state) = metadata.as_ref().and_then(|metadata| { + file_state(metadata, current_file_state.materialized_conflict_data) + }) { self.process_present_file( tracked_path.to_owned(), &disk_path, @@ -1305,13 +1342,13 @@ impl FileSnapshotter<'_> { path: RepoPathBuf, disk_path: &Path, maybe_current_file_state: Option<&FileState>, - new_file_state: FileState, + mut new_file_state: FileState, ) -> Result<(), SnapshotError> { let update = self.get_updated_tree_value( &path, disk_path, maybe_current_file_state, - &new_file_state, + &mut new_file_state, )?; if let Some(tree_value) = update { self.tree_entries_tx.send((path.clone(), tree_value)).ok(); @@ -1360,7 +1397,7 @@ impl FileSnapshotter<'_> { repo_path: &RepoPath, disk_path: &Path, maybe_current_file_state: Option<&FileState>, - new_file_state: &FileState, + new_file_state: &mut FileState, ) -> Result, SnapshotError> { let clean = match maybe_current_file_state { None => { @@ -1391,7 +1428,13 @@ impl FileSnapshotter<'_> { }; let new_tree_values = match new_file_type { FileType::Normal { executable } => self - .write_path_to_store(repo_path, disk_path, ¤t_tree_values, executable) + .write_path_to_store( + repo_path, + disk_path, + ¤t_tree_values, + new_file_state, + executable, + ) .block_on()?, FileType::Symlink => { let id = self @@ -1418,6 +1461,7 @@ impl FileSnapshotter<'_> { repo_path: &RepoPath, disk_path: &Path, current_tree_values: &MergedTreeValue, + new_file_state: &mut FileState, executable: FileExecutableFlag, ) -> Result { if let Some(current_tree_value) = current_tree_values.as_resolved() { @@ -1449,6 +1493,11 @@ impl FileSnapshotter<'_> { repo_path, &content, self.conflict_marker_style, + new_file_state + .materialized_conflict_data + .map_or(MIN_CONFLICT_MARKER_LEN, |data| { + data.conflict_marker_len as usize + }), ) .block_on()?; match new_file_ids.into_resolved() { @@ -1463,6 +1512,8 @@ impl FileSnapshotter<'_> { false } }; + // Clear materialized conflict data if the file was resolved + new_file_state.materialized_conflict_data = None; Ok(Merge::normal(TreeValue::File { id: file_id.unwrap(), executable, @@ -1552,7 +1603,7 @@ impl TreeState { let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size, &metadata)) + Ok(FileState::for_file(executable, size, &metadata, None)) } fn write_symlink(&self, disk_path: &Path, target: String) -> Result { @@ -1576,6 +1627,7 @@ impl TreeState { disk_path: &Path, conflict_data: Vec, executable: bool, + materialized_conflict_data: Option, ) -> Result { let mut file = OpenOptions::new() .write(true) @@ -1595,7 +1647,12 @@ impl TreeState { let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size, &metadata)) + Ok(FileState::for_file( + executable, + size, + &metadata, + materialized_conflict_data, + )) } #[cfg_attr(windows, allow(unused_variables))] @@ -1786,16 +1843,29 @@ impl TreeState { contents, executable, } => { - let data = - materialize_merge_result_to_bytes(&contents, conflict_marker_style).into(); - self.write_conflict(&disk_path, data, executable)? + let conflict_marker_len = choose_materialized_conflict_marker_len(&contents); + let data = materialize_merge_result_to_bytes_with_marker_len( + &contents, + conflict_marker_style, + conflict_marker_len, + ) + .into(); + let materialized_conflict_data = MaterializedConflictData { + conflict_marker_len: conflict_marker_len.try_into().unwrap_or(u32::MAX), + }; + self.write_conflict( + &disk_path, + data, + executable, + Some(materialized_conflict_data), + )? } MaterializedTreeValue::OtherConflict { id } => { // Unless all terms are regular files, we can't do much // better than trying to describe the merge. let data = id.describe().into_bytes(); let executable = false; - self.write_conflict(&disk_path, data, executable)? + self.write_conflict(&disk_path, data, executable, None)? } }; changed_file_states.push((path, file_state)); @@ -1851,6 +1921,7 @@ impl TreeState { file_type, mtime: MillisSinceEpoch(0), size: 0, + materialized_conflict_data: None, }; changed_file_states.push((path, file_state)); } @@ -2310,6 +2381,7 @@ mod tests { }, mtime: MillisSinceEpoch(0), size, + materialized_conflict_data: None, }; let new_static_entry = |path: &'static str, size| (repo_path(path), new_state(size)); let new_owned_entry = |path: &str, size| (repo_path(path).to_owned(), new_state(size)); @@ -2358,6 +2430,7 @@ mod tests { }, mtime: MillisSinceEpoch(0), size, + materialized_conflict_data: None, }; let new_proto_entry = |path: &str, size| { file_state_entry_to_proto(repo_path(path).to_owned(), &new_state(size)) @@ -2422,6 +2495,7 @@ mod tests { }, mtime: MillisSinceEpoch(0), size, + materialized_conflict_data: None, }; let new_proto_entry = |path: &str, size| { file_state_entry_to_proto(repo_path(path).to_owned(), &new_state(size)) diff --git a/lib/src/protos/working_copy.proto b/lib/src/protos/working_copy.proto index 634c25da2e..7ed5c21a9e 100644 --- a/lib/src/protos/working_copy.proto +++ b/lib/src/protos/working_copy.proto @@ -24,12 +24,18 @@ enum FileType { GitSubmodule = 4; } +message MaterializedConflictData { + // TODO: maybe we should store num_sides here as well + uint32 conflict_marker_len = 1; +} + message FileState { int64 mtime_millis_since_epoch = 1; uint64 size = 2; FileType file_type = 3; // Set only if file_type is Conflict bytes conflict_id = 4 [deprecated = true]; + MaterializedConflictData materialized_conflict_data = 5; } message FileStateEntry { diff --git a/lib/src/protos/working_copy.rs b/lib/src/protos/working_copy.rs index 90c1bfe676..50621c7164 100644 --- a/lib/src/protos/working_copy.rs +++ b/lib/src/protos/working_copy.rs @@ -1,6 +1,13 @@ // This file is @generated by prost-build. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] +pub struct MaterializedConflictData { + /// TODO: maybe we should store num_sides here as well + #[prost(uint32, tag = "1")] + pub conflict_marker_len: u32, +} +#[allow(clippy::derive_partial_eq_without_eq)] +#[derive(Clone, PartialEq, ::prost::Message)] pub struct FileState { #[prost(int64, tag = "1")] pub mtime_millis_since_epoch: i64, @@ -12,6 +19,8 @@ pub struct FileState { #[deprecated] #[prost(bytes = "vec", tag = "4")] pub conflict_id: ::prost::alloc::vec::Vec, + #[prost(message, optional, tag = "5")] + pub materialized_conflict_data: ::core::option::Option, } #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index d62f8e1eac..cd2ace3e6d 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -15,6 +15,7 @@ use indoc::indoc; use itertools::Itertools; use jj_lib::backend::FileId; +use jj_lib::conflicts::choose_materialized_conflict_marker_len; use jj_lib::conflicts::extract_as_single_hunk; use jj_lib::conflicts::materialize_merge_result_to_bytes; use jj_lib::conflicts::parse_conflict; @@ -590,10 +591,16 @@ fn test_materialize_parse_roundtrip_different_markers() { for materialize_style in all_styles { let materialized = materialize_conflict_string(store, path, &conflict, materialize_style); for parse_style in all_styles { - let parsed = - update_from_content(&conflict, store, path, materialized.as_bytes(), parse_style) - .block_on() - .unwrap(); + let parsed = update_from_content( + &conflict, + store, + path, + materialized.as_bytes(), + parse_style, + MIN_CONFLICT_MARKER_LEN, + ) + .block_on() + .unwrap(); assert_eq!( parsed, conflict, @@ -1630,9 +1637,16 @@ fn test_update_conflict_from_content() { let materialized = materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Diff); let parse = |content| { - update_from_content(&conflict, store, path, content, ConflictMarkerStyle::Diff) - .block_on() - .unwrap() + update_from_content( + &conflict, + store, + path, + content, + ConflictMarkerStyle::Diff, + MIN_CONFLICT_MARKER_LEN, + ) + .block_on() + .unwrap() }; assert_eq!(parse(materialized.as_bytes()), conflict); @@ -1680,9 +1694,16 @@ fn test_update_conflict_from_content_modify_delete() { let materialized = materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Diff); let parse = |content| { - update_from_content(&conflict, store, path, content, ConflictMarkerStyle::Diff) - .block_on() - .unwrap() + update_from_content( + &conflict, + store, + path, + content, + ConflictMarkerStyle::Diff, + MIN_CONFLICT_MARKER_LEN, + ) + .block_on() + .unwrap() }; assert_eq!(parse(materialized.as_bytes()), conflict); @@ -1736,9 +1757,16 @@ fn test_update_conflict_from_content_simplified_conflict() { let materialized_simplified = materialize_conflict_string(store, path, &simplified_conflict, ConflictMarkerStyle::Diff); let parse = |content| { - update_from_content(&conflict, store, path, content, ConflictMarkerStyle::Diff) - .block_on() - .unwrap() + update_from_content( + &conflict, + store, + path, + content, + ConflictMarkerStyle::Diff, + MIN_CONFLICT_MARKER_LEN, + ) + .block_on() + .unwrap() }; insta::assert_snapshot!( materialized, @@ -1896,6 +1924,12 @@ fn test_update_conflict_from_content_with_long_markers() { ); // The conflict should be materialized using long conflict markers + let materialized_marker_len = choose_materialized_conflict_marker_len( + &extract_as_single_hunk(&conflict, store, path) + .block_on() + .unwrap(), + ); + assert!(materialized_marker_len > MIN_CONFLICT_MARKER_LEN); let materialized = materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Snapshot); insta::assert_snapshot!(materialized, @r##" @@ -1923,9 +1957,16 @@ fn test_update_conflict_from_content_with_long_markers() { // to avoid the two versions of the file being obviously identical, so that we // can test the actual parsing logic. let parse = |conflict, content| { - update_from_content(conflict, store, path, content, ConflictMarkerStyle::Diff) - .block_on() - .unwrap() + update_from_content( + conflict, + store, + path, + content, + ConflictMarkerStyle::Diff, + materialized_marker_len, + ) + .block_on() + .unwrap() }; assert_eq!(parse(&conflict, materialized.as_bytes()), conflict); @@ -1999,6 +2040,11 @@ fn test_update_conflict_from_content_with_long_markers() { new_conflict ); + // If we add back the second conflict, it should still be parsed correctly + // (the fake conflict markers shouldn't be interpreted as conflict markers + // still, since they aren't the longest ones in the file). + assert_eq!(parse(&new_conflict, materialized.as_bytes()), conflict); + // If the new conflict is materialized again, it should have shorter // conflict markers now insta::assert_snapshot!( @@ -2062,6 +2108,14 @@ fn test_update_from_content_malformed_conflict() { vec![Some(left_file_id.clone()), Some(right_file_id.clone())], ); + // The conflict should be materialized with normal markers + let materialized_marker_len = choose_materialized_conflict_marker_len( + &extract_as_single_hunk(&conflict, store, path) + .block_on() + .unwrap(), + ); + assert!(materialized_marker_len == MIN_CONFLICT_MARKER_LEN); + let materialized = materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Diff); insta::assert_snapshot!(materialized, @r##" @@ -2086,9 +2140,16 @@ fn test_update_from_content_malformed_conflict() { ); let parse = |conflict, content| { - update_from_content(conflict, store, path, content, ConflictMarkerStyle::Diff) - .block_on() - .unwrap() + update_from_content( + conflict, + store, + path, + content, + ConflictMarkerStyle::Diff, + materialized_marker_len, + ) + .block_on() + .unwrap() }; assert_eq!(parse(&conflict, materialized.as_bytes()), conflict); @@ -2164,10 +2225,10 @@ fn test_update_from_content_malformed_conflict() { line 5 "##); - // Snapshotting again will cause the conflict to appear resolved + // Even though the file now contains markers of length 7, the materialized + // markers of length 7 are still parsed let second_snapshot = parse(&new_conflict, new_conflict_contents.as_bytes()); - assert_ne!(second_snapshot, new_conflict); - assert!(second_snapshot.is_resolved()); + assert_eq!(second_snapshot, new_conflict); } fn materialize_conflict_string(