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/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..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 { @@ -191,7 +193,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,9 +217,19 @@ 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<()> { + // 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 @@ -456,7 +469,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/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 6876508945..d24b760fd7 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -338,6 +338,52 @@ 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 + "### + ); + // 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() + ),@r###" + Some( + [ + Conflicted( + [ + "", + "base\n", + "right\n", + ], + ), + ], + ) + "###); +} + #[test] fn test_materialize_conflict_modify_delete() { let test_repo = TestRepo::init(); @@ -994,6 +1040,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() }