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 c037675cf9..43407df29d 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; @@ -249,3 +250,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("6feb53603f9f7324085d2d89dca19a6dac93fef6795cfd5d57090ff803d404ab1196b45d5b97faa641f6a78302ac0fbd149f5e5a880d1fd64d6520c31beab213") + 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 3a981880 (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("205bc702428a522e0b175938a51c51b59741c854a609ba63c89de76ffda6e5eff6fcc00725328b1a91f448401769773cefcff01fac3448190d2cea4e137d2166") + 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("2206ce3c108b1573df0841138c226bba1ab3cff900a5899ed31ac69162c7d6f30d37fb5ab43da60dba88047b8ab22d453887fff688f26dfcf04f2c99420a5563") + 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(