From 308b0b7d7e49d7346048e56d2a8479f65062458b Mon Sep 17 00:00:00 2001
From: Ilya Grigoriev <ilyagr@users.noreply.github.com>
Date: Tue, 25 Jun 2024 22:20:18 -0700
Subject: [PATCH] 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 FromIterator for Merge. I could split it into a
separate commit, but then it'd have no uses.
---
 CHANGELOG.md                |  6 ++++++
 lib/src/conflicts.rs        | 16 ++++++++++++++++
 lib/src/merge.rs            |  7 +++++++
 lib/tests/test_conflicts.rs | 24 ++++++++++++++++++++----
 4 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 24c05d90c5..07b4d1219b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -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
diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs
index 9d6ff0212f..31feee233f 100644
--- a/lib/src/conflicts.rs
+++ b/lib/src/conflicts.rs
@@ -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 {
@@ -218,6 +220,19 @@ pub fn materialize_merge_result(
     single_hunk: Merge<ContentHunk>,
     output: &mut dyn Write,
 ) -> std::io::Result<()> {
+    // The following code assumes that 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_iter()
+        .map(|ContentHunk(mut side)| {
+            if !side.is_empty() && !side.ends_with(b"\n") {
+                side.push(b'\n');
+            };
+            ContentHunk(side)
+        })
+        .collect();
+
     let slices = single_hunk.map(|content| content.0.as_slice());
     let merge_result = files::merge(&slices);
     match merge_result {
@@ -239,6 +254,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..9657dc9372 100644
--- a/lib/src/merge.rs
+++ b/lib/src/merge.rs
@@ -415,6 +415,13 @@ impl<T> FromIterator<T> for MergeBuilder<T> {
     }
 }
 
+impl<T> FromIterator<T> for Merge<T> {
+    fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
+        // TODO(ilyagr): Would using Merge::from_vec be better?
+        MergeBuilder::from_iter(iter).build()
+    }
+}
+
 impl<T> Extend<T> for MergeBuilder<T> {
     fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
         self.values.extend(iter)
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]