Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conflicts: insert trailing newline when materializing conflicts #3977

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cli/src/merge_tools/external.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ pub fn run_mergetool_external(
) -> Result<MergedTreeId, ConflictResolveError> {
let initial_output_content: Vec<u8> = 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 {
Expand Down
19 changes: 16 additions & 3 deletions lib/src/conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ static CONFLICT_MARKER_REGEX: once_cell::sync::Lazy<Regex> = 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 {
Expand Down Expand Up @@ -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
Expand All @@ -215,9 +217,19 @@ async fn materialize_tree_value_no_access_denied(
}

pub fn materialize_merge_result(
single_hunk: &Merge<ContentHunk>,
single_hunk: Merge<ContentHunk>,
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<ContentHunk> = 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 {
Expand All @@ -239,6 +251,7 @@ pub fn materialize_merge_result(
output.write_all(
format!(" Conflict {conflict_index} of {num_conflicts}\n").as_bytes(),
)?;

ilyagr marked this conversation as resolved.
Show resolved Hide resolved
let mut add_index = 0;
for (base_index, left) in hunk.removes().enumerate() {
// The vast majority of conflicts one actually tries to
Expand Down Expand Up @@ -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());
}
Expand Down
6 changes: 6 additions & 0 deletions lib/src/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,12 @@ impl<T> Merge<T> {
self.values.iter_mut()
}

/// Creates a new merge by applying `f` to each remove and add.
pub fn into_map<U>(self, f: impl FnMut(T) -> U) -> Merge<U> {
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<U> {
let values = self.values.iter().map(f).collect();
Expand Down
48 changes: 47 additions & 1 deletion lib/tests/test_conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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()
}