Skip to content

Commit

Permalink
conflicts: insert trailing newline when materializing conflicts
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ilyagr committed Jun 28, 2024
1 parent a578e1c commit dba45bb
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 4 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
to-be-pushed commit has conflicts, or has no description / committer / author
set. [#3029](https://github.com/martinvonz/jj/issues/3029)

* 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.18.0] - 2024-06-05

### Breaking changes
Expand Down
13 changes: 13 additions & 0 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 @@ -218,6 +220,16 @@ pub fn materialize_merge_result(
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(),
)?;

let mut add_index = 0;
for (base_index, left) in hunk.removes().enumerate() {
// The vast majority of conflicts one actually tries to
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
24 changes: 20 additions & 4 deletions lib/tests/test_conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit dba45bb

Please sign in to comment.