From f70e6987b5e7b5d68aeb39d8bd1f21f0d07cbc31 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 17 Feb 2023 22:29:30 -0800 Subject: [PATCH] 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 b84be06c0822, 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 | 3 ++ lib/src/conflicts.rs | 67 +++++++++++++++++++++-------------- lib/tests/test_conflicts.rs | 25 +++++++------ tests/test_resolve_command.rs | 4 +-- 4 files changed, 60 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25ee5ee56..08308232b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 3ae7d4ba9..739c3d684 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -192,38 +192,53 @@ pub fn materialize_merge_result( MergeHunk::Resolved(content) => { output.write_all(&content)?; } - MergeHunk::Conflict(ConflictHunk { - mut removes, - mut adds, - }) => { + MergeHunk::Conflict(ConflictHunk { removes, adds }) => { output.write_all(CONFLICT_START_LINE)?; - while !removes.is_empty() && !adds.is_empty() { - let left = &removes[0]; - let mut diffs = vec![]; - for right in &adds { - diffs.push( - Diff::for_tokenizer(&[left, right], &find_line_ranges) - .hunks() - .collect_vec(), - ); + let mut add_index = 0; + for left in removes { + if add_index == adds.len() { + // If we have no more positive terms, emit the remaining negative + // terms as snapshots. + output.write_all(CONFLICT_MINUS_LINE)?; + output.write_all(&left)?; + continue; } - let min_diff_index = diffs - .iter() - .position_min_by_key(|diff| diff_size(diff)) - .unwrap(); + let diff1 = + Diff::for_tokenizer(&[&left, &adds[add_index]], &find_line_ranges) + .hunks() + .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)?; - removes.remove(0); - adds.remove(min_diff_index); + write_diff_hunks(&diff1, output)?; + add_index += 1; } - for slice in removes { - output.write_all(CONFLICT_MINUS_LINE)?; - output.write_all(&slice)?; - } - for slice in adds { + // Emit the remaining positive terms as snapshots. + for slice in &adds[add_index..] { output.write_all(CONFLICT_PLUS_LINE)?; - output.write_all(&slice)?; + output.write_all(slice)?; } output.write_all(CONFLICT_END_LINE)?; } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index d0f42acd9..1d4cb924c 100644 --- a/lib/tests/test_conflicts.rs +++ b/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 diff --git a/tests/test_resolve_command.rs b/tests/test_resolve_command.rs index 7586c368e..90c5c0f66 100644 --- a/tests/test_resolve_command.rs +++ b/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 >>>>>>> "###);