From dc036123368017ad0c76ca4999adecd08c6a437e Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 25 Jun 2024 21:46:52 -0700 Subject: [PATCH 1/3] conflicts: demo failure to materialize if conflicts don't end in a newline #3968 --- lib/tests/test_conflicts.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 6876508945..83a419c361 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -338,6 +338,36 @@ fn test_materialize_parse_roundtrip() { "###); } +#[test] +fn test_materialize_conflict_no_newlines_at_eof() { + let test_repo = TestRepo::init(); + let store = test_repo.repo.store(); + + let path = RepoPath::from_internal_string("file"); + let base_id = testutils::write_file(store, path, "base"); + let left_empty_id = testutils::write_file(store, path, ""); + let right_id = testutils::write_file(store, path, "right"); + + let conflict = Merge::from_removes_adds( + vec![Some(base_id.clone())], + vec![Some(left_empty_id.clone()), Some(right_id.clone())], + ); + let materialized = &materialize_conflict_string(store, path, &conflict); + insta::assert_snapshot!(materialized, + @r###" + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base+++++++ Contents of side #2 + right>>>>>>> Conflict 1 of 1 ends + "### + ); + // BUG(#3968): These conflict markers cannot be parsed + insta::assert_debug_snapshot!(parse_conflict( + materialized.as_bytes(), + conflict.num_sides() + ),@"None"); +} + #[test] fn test_materialize_conflict_modify_delete() { let test_repo = TestRepo::init(); From d5b1eecd57200d792352a95ee39e1cd7d62f6ef5 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 26 Jun 2024 15:24:13 -0700 Subject: [PATCH 2/3] conflicts: Have materialize_merge_result take the object instead of a reference We are about to do some pre-processing of that value, and we will do more in the near future. --- cli/src/merge_tools/external.rs | 2 +- lib/src/conflicts.rs | 6 +++--- lib/tests/test_conflicts.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 3025f376db..9ac546c352 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -144,7 +144,7 @@ pub fn run_mergetool_external( ) -> Result { let initial_output_content: Vec = if editor.merge_tool_edits_conflict_markers { let mut materialized_conflict = vec![]; - materialize_merge_result(&content, &mut materialized_conflict) + materialize_merge_result(content.clone(), &mut materialized_conflict) .expect("Writing to an in-memory buffer should never fail"); materialized_conflict } else { diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index c90aa27714..2713436c42 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -191,7 +191,7 @@ async fn materialize_tree_value_no_access_denied( if let Some(file_merge) = conflict.to_file_merge() { let file_merge = file_merge.simplify(); let content = extract_as_single_hunk(&file_merge, store, path).await?; - materialize_merge_result(&content, &mut contents) + materialize_merge_result(content, &mut contents) .expect("Failed to materialize conflict to in-memory buffer"); } else { // Unless all terms are regular files, we can't do much better than to try to @@ -215,7 +215,7 @@ async fn materialize_tree_value_no_access_denied( } pub fn materialize_merge_result( - single_hunk: &Merge, + single_hunk: Merge, output: &mut dyn Write, ) -> std::io::Result<()> { let slices = single_hunk.map(|content| content.0.as_slice()); @@ -456,7 +456,7 @@ 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?; - materialize_merge_result(&merge_hunk, &mut old_content).unwrap(); + materialize_merge_result(merge_hunk, &mut old_content).unwrap(); if content == old_content { return Ok(file_ids.clone()); } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 83a419c361..4d7ee7d790 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -1024,6 +1024,6 @@ fn materialize_conflict_string( let contents = extract_as_single_hunk(conflict, store, path) .block_on() .unwrap(); - materialize_merge_result(&contents, &mut result).unwrap(); + materialize_merge_result(contents, &mut result).unwrap(); String::from_utf8(result).unwrap() } From a6f8d2a3af63be8c5c5d9689b174f9adfee2d3e1 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 25 Jun 2024 22:20:18 -0700 Subject: [PATCH 3/3] conflicts: insert trailing newline when materializing conflicts This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to #3968, a better fix is TODO; see #3975. This also implements `Merge::into_map` to avoid distracting iterations and MergeBuilders. I could split it into a separate commit, but then it'd have no uses. --- CHANGELOG.md | 6 ++++++ lib/src/conflicts.rs | 13 +++++++++++++ lib/src/merge.rs | 6 ++++++ lib/tests/test_conflicts.rs | 24 ++++++++++++++++++++---- 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ace2489aa8..68e36f6745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed bugs +* Conflicts involving non-empty files that do not end in a newline no longer + look broken when materialized. + [#3968](https://github.com/martinvonz/jj/issues/3968). This is a partial fix, + see also [#3975](https://github.com/martinvonz/jj/issues/3968) which is not + yet fixed. + ## [0.19.0] - 2024-07-03 ### Breaking changes diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 2713436c42..f9e14b9760 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -53,6 +53,8 @@ static CONFLICT_MARKER_REGEX: once_cell::sync::Lazy = once_cell::sync::La .unwrap() }); +/// This function assumes that each hunk consists of 0 or more lines, each +/// ending with a newline. fn write_diff_hunks(hunks: &[DiffHunk], file: &mut dyn Write) -> std::io::Result<()> { for hunk in hunks { match hunk { @@ -218,6 +220,16 @@ pub fn materialize_merge_result( single_hunk: Merge, output: &mut dyn Write, ) -> std::io::Result<()> { + // The following code assumes that each version of the file contains 0 or + // more lines, each ending with `\n`. For now, we force this to be true. + // TODO(#3795): A better solution for this. + let single_hunk: Merge = single_hunk.into_map(|ContentHunk(mut side)| { + if !side.is_empty() && !side.ends_with(b"\n") { + side.push(b'\n'); + }; + ContentHunk(side) + }); + let slices = single_hunk.map(|content| content.0.as_slice()); let merge_result = files::merge(&slices); match merge_result { @@ -239,6 +251,7 @@ pub fn materialize_merge_result( output.write_all( format!(" Conflict {conflict_index} of {num_conflicts}\n").as_bytes(), )?; + let mut add_index = 0; for (base_index, left) in hunk.removes().enumerate() { // The vast majority of conflicts one actually tries to diff --git a/lib/src/merge.rs b/lib/src/merge.rs index e16786ee36..0eef5a45ea 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -345,6 +345,12 @@ impl Merge { self.values.iter_mut() } + /// Creates a new merge by applying `f` to each remove and add. + pub fn into_map(self, f: impl FnMut(T) -> U) -> Merge { + let values = self.values.into_iter().map(f).collect(); + Merge { values } + } + /// Creates a new merge by applying `f` to each remove and add. pub fn map<'a, U>(&'a self, f: impl FnMut(&'a T) -> U) -> Merge { let values = self.values.iter().map(f).collect(); diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 4d7ee7d790..d24b760fd7 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -357,15 +357,31 @@ fn test_materialize_conflict_no_newlines_at_eof() { @r###" <<<<<<< Conflict 1 of 1 %%%%%%% Changes from base to side #1 - -base+++++++ Contents of side #2 - right>>>>>>> Conflict 1 of 1 ends + -base + +++++++ Contents of side #2 + right + >>>>>>> Conflict 1 of 1 ends "### ); - // BUG(#3968): These conflict markers cannot be parsed + // These conflict markers can be parsed (#3968) + // TODO(#3975): BUG: The result of the parsing has newlines where original files + // didn't. insta::assert_debug_snapshot!(parse_conflict( materialized.as_bytes(), conflict.num_sides() - ),@"None"); + ),@r###" + Some( + [ + Conflicted( + [ + "", + "base\n", + "right\n", + ], + ), + ], + ) + "###); } #[test]