conflicts: preserve order of adds in materialized conflict

We write conflict to the working copy by materializing them as
conflict markers in a file. When the file has been modified (or just
the mtime has changed), we parse the markers to reconstruct the
conflict. For example, let's say we see this conflict marker:

```
<<<<<<<
+++++++
b
%%%%%%%
-a
+c
>>>>>>>
```

Then we will create a hunk with ["a"] as removed and ["b", "c"] as
added.

Now, since commit b84be06c08, when we materialize conflicts, we
minimize the diff part of the marker (the `%%%%%%%` part). The problem
is that that minimization may result in a different order of the
positive conflict terms. That's particularly bad because we do the
minimization per hunk, so we can end up reconstructing an input that
never existed.

This commit fixes the bug by only considering the next add and the one
after that, and emitting either only the first with `%%%%%%%`, or both
of them, with the first one in `++++++++` and the second one in
`%%%%%%%`.

Note that the recent fix to add context to modify/delete conflicts
means that when we parse modified such conflicts, we'll always
consider them resolved, since the expected adds/removes we pass will
not match what's actually in the file. That doesn't seem so bad, and
it's not obvious what the fix should be, so I'll leave that for later.

CHANGELOG.md

@@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Modify/delete conflicts now include context lines
   [#1244](https://github.com/martinvonz/jj/issues/1244).
 
+* Fixed a bug that could get partially resolved conflicts to be interpreted
+  incorrectly.
+
 ## [0.7.0] - 2023-02-16
 
 ### Breaking changes

lib/src/conflicts.rs

@@ -192,38 +192,53 @@ pub fn materialize_merge_result(
                     MergeHunk::Resolved(content) => {
                         output.write_all(&content)?;
                     }
-                    MergeHunk::Conflict(ConflictHunk {
+                    MergeHunk::Conflict(ConflictHunk { removes, adds }) => {
-                        mut removes,
-                        mut adds,
-                    }) => {
                         output.write_all(CONFLICT_START_LINE)?;
-                        while !removes.is_empty() && !adds.is_empty() {
+                        let mut add_index = 0;
-                            let left = &removes[0];
+                        for left in removes {
-                            let mut diffs = vec![];
+                            if add_index == adds.len() {
-                            for right in &adds {
+                                // If we have no more positive terms, emit the remaining negative
-                                diffs.push(
+                                // terms as snapshots.
-                                    Diff::for_tokenizer(&[left, right], &find_line_ranges)
+                                output.write_all(CONFLICT_MINUS_LINE)?;
-                                        .hunks()
+                                output.write_all(&left)?;
-                                        .collect_vec(),
+                                continue;
-                                );
                             }
-                            let min_diff_index = diffs
+                            let diff1 =
-                                .iter()
+                                Diff::for_tokenizer(&[&left, &adds[add_index]], &find_line_ranges)
-                                .position_min_by_key(|diff| diff_size(diff))
+                                    .hunks()
-                                .unwrap();
+                                    .collect_vec();
+                            // Check if the diff against the next positive term is better. Since
+                            // we want to preserve the order of the terms, we don't match against
+                            // any later positive terms.
+                            if add_index < adds.len() {
+                                let diff2 = Diff::for_tokenizer(
+                                    &[&left, &adds[add_index + 1]],
+                                    &find_line_ranges,
+                                )
+                                .hunks()
+                                .collect_vec();
+                                if diff_size(&diff2) < diff_size(&diff1) {
+                                    // If the next positive term is a better match, emit
+                                    // the current positive term as a snapshot and the next
+                                    // positive term as a diff.
+                                    output.write_all(CONFLICT_PLUS_LINE)?;
+                                    output.write_all(&adds[add_index])?;
+                                    output.write_all(CONFLICT_DIFF_LINE)?;
+                                    write_diff_hunks(&diff2, output)?;
+                                    add_index += 2;
+                                    continue;
+                                }
+                            }
+
                             output.write_all(CONFLICT_DIFF_LINE)?;
-                            write_diff_hunks(&diffs[min_diff_index], output)?;
+                            write_diff_hunks(&diff1, output)?;
-                            removes.remove(0);
+                            add_index += 1;
-                            adds.remove(min_diff_index);
                         }
 
-                        for slice in removes {
+                        //  Emit the remaining positive terms as snapshots.
-                            output.write_all(CONFLICT_MINUS_LINE)?;
+                        for slice in &adds[add_index..] {
-                            output.write_all(&slice)?;
-                        }
-                        for slice in adds {
                             output.write_all(CONFLICT_PLUS_LINE)?;
-                            output.write_all(&slice)?;
+                            output.write_all(slice)?;
                         }
                         output.write_all(CONFLICT_END_LINE)?;
                     }

lib/tests/test_conflicts.rs

@@ -67,6 +67,8 @@ line 5
 ",
     );
 
+    // The left side should come first. The diff should be use the smaller (right)
+    // side, and the left side should be a snapshot.
     let mut conflict = Conflict {
         removes: vec![file_conflict_term(&base_id)],
         adds: vec![file_conflict_term(&left_id), file_conflict_term(&right_id)],
@@ -77,19 +79,20 @@ line 5
     line 1
     line 2
     <<<<<<<
-    %%%%%%%
-    -line 3
-    +right 3.1
     +++++++
     left 3.1
     left 3.2
     left 3.3
+    %%%%%%%
+    -line 3
+    +right 3.1
     >>>>>>>
     line 4
     line 5
     "###
     );
-    // Test with the larger diff first. We still want the small diff.
+    // Swap the positive terms in the conflict. The diff should still use the right
+    // side, but now the right side should come first.
     conflict.adds.reverse();
     insta::assert_snapshot!(
         &materialize_conflict_string(store, &path, &conflict),
@@ -158,13 +161,13 @@ line 5 right
         String::from_utf8(result.clone()).unwrap(),
         @r###"
     <<<<<<<
+    +++++++
+    line 1 left
+    line 2 left
     %%%%%%%
     -line 1
     +line 1 right
      line 2
-    +++++++
-    line 1 left
-    line 2 left
     >>>>>>>
     line 3
     <<<<<<<
@@ -179,7 +182,7 @@ line 5 right
     "###
     );
 
-    // TODO: The first add should always be from the left side
+    // The first add should always be from the left side
     insta::assert_debug_snapshot!(
         parse_conflict(&result, conflict.removes.len(), conflict.adds.len()),
         @r###"
@@ -190,8 +193,8 @@ line 5 right
                     "line 1\nline 2\n",
                 ],
                 adds: [
-                    "line 1 right\nline 2\n",
                     "line 1 left\nline 2 left\n",
+                    "line 1 right\nline 2\n",
                 ],
             },
             Resolved(
@@ -259,10 +262,10 @@ line 5
     line 1
     line 2
     <<<<<<<
-    %%%%%%%
-    -line 3
     +++++++
     modified
+    %%%%%%%
+    -line 3
     >>>>>>>
     line 4
     line 5

tests/test_resolve_command.rs

@@ -425,10 +425,10 @@ fn test_edit_delete_conflict_input_files() {
     std::fs::read_to_string(repo_path.join("file")).unwrap()
         , @r###"
     <<<<<<<
-    %%%%%%%
-    -base
     +++++++
     a
+    %%%%%%%
+    -base
     >>>>>>>
     "###);