From e625b5051c51ad5f0a58099d2f22230e1d2d5bfd Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 21 Mar 2024 17:12:25 -0700 Subject: [PATCH 1/3] conflicts.rs: Teach `jj` to parse conflict markers that are followed by a label The format is 7 characters of the separator followed by a space and arbitrary text, followed by a newline. Separator followed by a newline is also allowed. E.g.: <<<<<<< Random text %%%%%%% Random text line 2 -line 3 +left line 4 +++++++ Random text right %%%%%%% Random text line 2 +forward line 3 line 4 >>>>>>> Random text This commit only allows reading such conflicts. I considered allowing longer separators (`<<<<<<<<<<<<<< Random text`), but we wouldn't currently write them, so let's be strict for now. 7 characters if they are followed by a space and arbitrary text --- lib/src/conflicts.rs | 83 ++++++++++++++++++++++++------------- lib/tests/test_conflicts.rs | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 29 deletions(-) diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index e9627d7859..af2e105d66 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -19,6 +19,7 @@ use std::iter::zip; use futures::StreamExt; use itertools::Itertools; +use regex::bytes::Regex; use crate::backend::{BackendResult, CommitId, FileId, SymlinkId, TreeId, TreeValue}; use crate::diff::{find_line_ranges, Diff, DiffHunk}; @@ -33,6 +34,24 @@ const CONFLICT_END_LINE: &[u8] = b">>>>>>>\n"; const CONFLICT_DIFF_LINE: &[u8] = b"%%%%%%%\n"; const CONFLICT_MINUS_LINE: &[u8] = b"-------\n"; const CONFLICT_PLUS_LINE: &[u8] = b"+++++++\n"; +const CONFLICT_START_LINE_CHAR: u8 = CONFLICT_START_LINE[0]; +const CONFLICT_END_LINE_CHAR: u8 = CONFLICT_END_LINE[0]; +const CONFLICT_DIFF_LINE_CHAR: u8 = CONFLICT_DIFF_LINE[0]; +const CONFLICT_MINUS_LINE_CHAR: u8 = CONFLICT_MINUS_LINE[0]; +const CONFLICT_PLUS_LINE_CHAR: u8 = CONFLICT_PLUS_LINE[0]; + +/// A conflict marker is one of the separators, optionally followed by a space +/// and some text. +// TODO: All the `{7}` could be replaced with `{7,}` to allow longer +// separators. This could be useful to make it possible to allow conflict +// markers inside the text of the conflicts. +static CONFLICT_MARKER_REGEX: once_cell::sync::Lazy = once_cell::sync::Lazy::new(|| { + Regex::new( + r"(<{7}|>{7}|%{7}|\-{7}|\+{7})( .*)? +", + ) + .unwrap() +}); fn write_diff_hunks(hunks: &[DiffHunk], file: &mut dyn Write) -> std::io::Result<()> { for hunk in hunks { @@ -267,21 +286,25 @@ pub fn parse_conflict(input: &[u8], num_sides: usize) -> Option Merge { let mut removes = vec![]; let mut adds = vec![]; for line in input.split_inclusive(|b| *b == b'\n') { - match line { - CONFLICT_DIFF_LINE => { - state = State::Diff; - removes.push(ContentHunk(vec![])); - adds.push(ContentHunk(vec![])); - continue; - } - CONFLICT_MINUS_LINE => { - state = State::Minus; - removes.push(ContentHunk(vec![])); - continue; - } - CONFLICT_PLUS_LINE => { - state = State::Plus; - adds.push(ContentHunk(vec![])); - continue; + if CONFLICT_MARKER_REGEX.is_match_at(line, 0) { + match line[0] { + CONFLICT_DIFF_LINE_CHAR => { + state = State::Diff; + removes.push(ContentHunk(vec![])); + adds.push(ContentHunk(vec![])); + continue; + } + CONFLICT_MINUS_LINE_CHAR => { + state = State::Minus; + removes.push(ContentHunk(vec![])); + continue; + } + CONFLICT_PLUS_LINE_CHAR => { + state = State::Plus; + adds.push(ContentHunk(vec![])); + continue; + } + _ => {} } - _ => {} }; match state { State::Diff => { diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index ad8563c3d0..cb0618f8a4 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -239,6 +239,7 @@ fn test_materialize_conflict_multi_rebase_conflicts() { ); } +// TODO: With options #[test] fn test_materialize_parse_roundtrip() { let test_repo = TestRepo::init(); @@ -543,6 +544,42 @@ fn test_parse_conflict_simple() { ], ) "### + ); + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<<<<<< Text + %%%%%%%%%%% Different text + line 2 + -line 3 + +left + line 4 + +++++++++++ Yet <><>< more text + right + >>>>>>>>>>> More and more text + line 5 + "}, + 2 + ), + @r###" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "line 2\nleft\nline 4\n", + "line 2\nline 3\nline 4\n", + "right\n", + ], + ), + Resolved( + "line 5\n", + ), + ], + ) + "### ) } @@ -591,7 +628,50 @@ fn test_parse_conflict_multi_way() { ], ) "### + ); + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<< Random text + %%%%%%% Random text + line 2 + -line 3 + +left + line 4 + +++++++ Random text + right + %%%%%%% Random text + line 2 + +forward + line 3 + line 4 + >>>>>>> Random text + line 5 + "}, + 3 + ), + @r###" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "line 2\nleft\nline 4\n", + "line 2\nline 3\nline 4\n", + "right\n", + "line 2\nline 3\nline 4\n", + "line 2\nforward\nline 3\nline 4\n", + ], + ), + Resolved( + "line 5\n", + ), + ], ) + "### + ); } #[test] From e3d3fe77d1c7994f4c8594a54e8dfd17dcf3fc0b Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sat, 23 Mar 2024 15:39:58 -0700 Subject: [PATCH 2/3] test_resolve_command: use `diff --git` for readability --- cli/tests/test_resolve_command.rs | 216 ++++++++++++++++++------------ 1 file changed, 132 insertions(+), 84 deletions(-) diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index 02ea988837..b6de12a788 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -97,16 +97,21 @@ fn test_resolution() { insta::assert_snapshot!( std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###" "###); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in file: - 1 1: <<<<<<>>>>>> + diff --git a/file b/file + index 0000000000...88425ec521 100644 + --- a/file + +++ b/file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --base + -+a + -+++++++ + -b + ->>>>>>> + +resolution "###); insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]), @r###" @@ -132,16 +137,21 @@ fn test_resolution() { Parent commit : royxmykx db6a4daf b | b Added 0 files, modified 1 files, removed 0 files "###); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in file: - 1 1: <<<<<<>>>>>> + diff --git a/file b/file + index 0000000000...88425ec521 100644 + --- a/file + +++ b/file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --base + -+a + -+++++++ + -b + ->>>>>>> + +resolution "###); insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]), @r###" @@ -151,7 +161,7 @@ fn test_resolution() { // Check that the output file starts with conflict markers if // `merge-tool-edits-conflict-markers=true` test_env.jj_cmd_ok(&repo_path, &["undo"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @""); std::fs::write( &editor_script, @@ -176,22 +186,27 @@ fn test_resolution() { b >>>>>>> "###); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in file: - 1 1: <<<<<<>>>>>> + diff --git a/file b/file + index 0000000000...88425ec521 100644 + --- a/file + +++ b/file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --base + -+a + -+++++++ + -b + ->>>>>>> + +resolution "###); // Check that if merge tool leaves conflict markers in output file and // `merge-tool-edits-conflict-markers=true`, these markers are properly parsed. test_env.jj_cmd_ok(&repo_path, &["undo"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @""); std::fs::write( &editor_script, @@ -247,16 +262,22 @@ fn test_resolution() { >>>>>>> "###); // Note the "Modified" below - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Modified conflict in file: - 1 1: <<<<<<< - 2 2: %%%%%%% - 3 3: -basesome - 4 4: +afake - 5 5: +++++++ - 6 6: bconflict - 7 7: >>>>>>> + diff --git a/file b/file + --- a/file + +++ b/file + @@ -1,7 +1,7 @@ + <<<<<<< + %%%%%%% + --base + -+a + +-some + ++fake + +++++++ + -b + +conflict + >>>>>>> "###); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), @r###" @@ -267,7 +288,7 @@ fn test_resolution() { // `merge-tool-edits-conflict-markers=false` or is not specified, // `jj` considers the conflict resolved. test_env.jj_cmd_ok(&repo_path, &["undo"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @""); std::fs::write( &editor_script, @@ -300,16 +321,23 @@ fn test_resolution() { std::fs::read_to_string(test_env.env_root().join("editor3")).unwrap(), @r###" "###); // Note the "Resolved" below - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in file: - 1 1: <<<<<<< - 2 2: %%%%%%% - 3 3: -basesome - 4 4: +afake - 5 5: +++++++ - 6 6: bconflict - 7 7: >>>>>>> + diff --git a/file b/file + index 0000000000...0610716cc1 100644 + --- a/file + +++ b/file + @@ -1,7 +1,7 @@ + <<<<<<< + %%%%%%% + --base + -+a + +-some + ++fake + +++++++ + -b + +conflict + >>>>>>> "###); insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]), @r###" @@ -707,16 +735,21 @@ fn test_multiple_conflicts() { There are unresolved conflicts at these paths: this_file_has_a_very_long_name_to_test_padding 2-sided conflict "###); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in another_file: - 1 : <<<<<<< - 2 : %%%%%%% - 3 1: -secondresolution baseanother_file - 4 : +second a - 5 : +++++++ - 6 : second b - 7 : >>>>>>> + diff --git a/another_file b/another_file + index 0000000000...a9fcc7d486 100644 + --- a/another_file + +++ b/another_file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --second base + -+second a + -+++++++ + -second b + ->>>>>>> + +resolution another_file "###); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), @r###" @@ -733,7 +766,7 @@ fn test_multiple_conflicts() { // For the rest of the test, we call `jj resolve` several times in a row to // resolve each conflict in the order it chooses. test_env.jj_cmd_ok(&repo_path, &["undo"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @""); std::fs::write( &editor_script, @@ -741,16 +774,21 @@ fn test_multiple_conflicts() { ) .unwrap(); test_env.jj_cmd_ok(&repo_path, &["resolve"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in another_file: - 1 : <<<<<<< - 2 : %%%%%%% - 3 1: first resolution for auto-secondchosen basefile - 4 : +second a - 5 : +++++++ - 6 : second b - 7 : >>>>>>> + diff --git a/another_file b/another_file + index 0000000000...7903e1c1c7 100644 + --- a/another_file + +++ b/another_file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --second base + -+second a + -+++++++ + -second b + ->>>>>>> + +first resolution for auto-chosen file "###); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), @r###" @@ -763,24 +801,34 @@ fn test_multiple_conflicts() { .unwrap(); test_env.jj_cmd_ok(&repo_path, &["resolve"]); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r###" - Resolved conflict in another_file: - 1 : <<<<<<< - 2 : %%%%%%% - 3 1: first resolution for auto-secondchosen basefile - 4 : +second a - 5 : +++++++ - 6 : second b - 7 : >>>>>>> - Resolved conflict in this_file_has_a_very_long_name_to_test_padding: - 1 : <<<<<<< - 2 : %%%%%%% - 3 1: second resolution for auto-firstchosen basefile - 4 : +first a - 5 : +++++++ - 6 : first b - 7 : >>>>>>> + diff --git a/another_file b/another_file + index 0000000000...7903e1c1c7 100644 + --- a/another_file + +++ b/another_file + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --second base + -+second a + -+++++++ + -second b + ->>>>>>> + +first resolution for auto-chosen file + diff --git a/this_file_has_a_very_long_name_to_test_padding b/this_file_has_a_very_long_name_to_test_padding + index 0000000000...f8c72adf17 100644 + --- a/this_file_has_a_very_long_name_to_test_padding + +++ b/this_file_has_a_very_long_name_to_test_padding + @@ -1,7 +1,1 @@ + -<<<<<<< + -%%%%%%% + --first base + -+first a + -+++++++ + -first b + ->>>>>>> + +second resolution for auto-chosen file "###); insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]), From 07f5835d985c71e399baa0a28280a767683de1a2 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sat, 23 Mar 2024 15:16:28 -0700 Subject: [PATCH 3/3] conflicts.rs: label conflict number and sides next to conflict markers For example, ``` <<<<<<< Conflict 1 of 3 +++++++ Contents of side #1 left 3.1 left 3.2 left 3.3 %%%%%%% Changes from base to side #2 -line 3 +right 3.1 >>>>>>> ``` or ``` <<<<<<< Conflict 1 of 1 %%%%%%% Changes from base to side #1 -line 3 +right 3.1 +++++++ Contents of side #2 left 3.1 left 3.2 left 3.3 >>>>>>> ``` Currently, there is no way to disable these, this is TODO for a future PR. Other TODOs for future PRs: make these labels configurable. After that, we could support a `diff3/git`-like conflict format as well, in principle. Counting conflicts helps with knowing whether you fixed all the conflicts while you are in the editor. While labeling "side #1", etc, does not tell you the commit id or description as requested in #1176, I still think it's an improvement. Most importantly, I hope this will make `jj`'s conflict format less scary-looking for new users. I've used this for a bit, and I like it. Without the labels, I would see that the two conflicts have a different order of conflict markers, but I wouldn't be able to remember what that means. For longer diffs, it can be tricky for me to quickly tell that it's a diff as opposed to one of the sides. This also creates some hope of being able to navigate a conflict with more than 2 sides. Another not-so-secret goal for this is explained in https://github.com/martinvonz/jj/pull/3109#issuecomment-2014140627. The idea is a little weird, but I *think* it could be helpful, and I'd like to experiment with it. --- CHANGELOG.md | 4 + cli/tests/test_cat_command.rs | 6 +- cli/tests/test_chmod_command.rs | 30 +++---- cli/tests/test_diffedit_command.rs | 6 +- cli/tests/test_interdiff_command.rs | 6 +- cli/tests/test_obslog_command.rs | 12 +-- cli/tests/test_resolve_command.rs | 127 ++++++++++++++-------------- cli/tests/test_restore_command.rs | 40 ++++----- cli/tests/test_squash_command.rs | 16 ++-- lib/src/conflicts.rs | 50 +++++++++-- lib/tests/test_conflicts.rs | 78 ++++++++--------- 11 files changed, 209 insertions(+), 166 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 263390d10c..eb97c8edcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* Conflict markers now include an explanation of what each part of the conflict + represents. + + ### Fixed bugs ## [0.17.0] - 2024-05-01 diff --git a/cli/tests/test_cat_command.rs b/cli/tests/test_cat_command.rs index f15c6da557..0163bbc27d 100644 --- a/cli/tests/test_cat_command.rs +++ b/cli/tests/test_cat_command.rs @@ -89,11 +89,11 @@ fn test_cat() { test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "@", "-d", "@--"]); let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file1"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -b +a - +++++++ + +++++++ Contents of side #2 c >>>>>>> "###); diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index 4a6db40af3..0015200c29 100644 --- a/cli/tests/test_chmod_command.rs +++ b/cli/tests/test_chmod_command.rs @@ -71,11 +71,11 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +x - +++++++ + +++++++ Contents of side #2 n >>>>>>> "###); @@ -90,11 +90,11 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +x - +++++++ + +++++++ Contents of side #2 n >>>>>>> "###); @@ -107,11 +107,11 @@ fn test_chmod_regular_conflict() { let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +x - +++++++ + +++++++ Contents of side #2 n >>>>>>> "###); @@ -201,10 +201,10 @@ fn test_chmod_file_dir_deletion_conflicts() { let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 a - %%%%%%% + %%%%%%% Changes from base to side #2 -base >>>>>>> "###); @@ -234,10 +234,10 @@ fn test_chmod_file_dir_deletion_conflicts() { let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "-r=file_deletion", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 a - %%%%%%% + %%%%%%% Changes from base to side #2 -base >>>>>>> "###); diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 3b30a36307..e08ed7b607 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -395,11 +395,11 @@ fn test_diffedit_merge() { assert!(!repo_path.join("file1").exists()); let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file2"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -a +c - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); diff --git a/cli/tests/test_interdiff_command.rs b/cli/tests/test_interdiff_command.rs index 67257be912..b3f8da715e 100644 --- a/cli/tests/test_interdiff_command.rs +++ b/cli/tests/test_interdiff_command.rs @@ -153,11 +153,11 @@ fn test_interdiff_conflicting() { --- a/file +++ b/file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --foo -+abc - -+++++++ + -+++++++ Contents of side #2 -bar ->>>>>>> +def diff --git a/cli/tests/test_obslog_command.rs b/cli/tests/test_obslog_command.rs index dec0cda0ad..d328f2e6df 100644 --- a/cli/tests/test_obslog_command.rs +++ b/cli/tests/test_obslog_command.rs @@ -59,10 +59,10 @@ fn test_obslog_with_or_without_diff() { @ rlvkpnrz test.user@example.com 2001-02-03 08:05:10 66b42ad3 │ my description │ Resolved conflict in file1: - │ 1 1: <<<<<<>>>>>> @@ -111,10 +111,10 @@ fn test_obslog_with_or_without_diff() { --- a/file1 +++ b/file1 @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --foo - -+++++++ + -+++++++ Contents of side #2 -foo -bar ->>>>>>> diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index b6de12a788..d5f79f693c 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -69,14 +69,14 @@ fn test_resolution() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% - -base - +a - +++++++ - b - >>>>>>> - "###); + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base + +a + +++++++ Contents of side #2 + b + >>>>>>> + "###); let editor_script = test_env.set_up_fake_editor(); // Check that output file starts out empty and resolve the conflict @@ -104,11 +104,11 @@ fn test_resolution() { --- a/file +++ b/file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --base -+a - -+++++++ + -+++++++ Contents of side #2 -b ->>>>>>> +resolution @@ -144,11 +144,11 @@ fn test_resolution() { --- a/file +++ b/file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --base -+a - -+++++++ + -+++++++ Contents of side #2 -b ->>>>>>> +resolution @@ -178,11 +178,11 @@ fn test_resolution() { ); insta::assert_snapshot!( std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +a - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); @@ -193,11 +193,11 @@ fn test_resolution() { --- a/file +++ b/file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --base -+a - -+++++++ + -+++++++ Contents of side #2 -b ->>>>>>> +resolution @@ -253,11 +253,11 @@ fn test_resolution() { "###); insta::assert_snapshot!( std::fs::read_to_string(test_env.env_root().join("editor2")).unwrap(), @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +a - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); @@ -268,13 +268,13 @@ fn test_resolution() { --- a/file +++ b/file @@ -1,7 +1,7 @@ - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 --base -+a +-some ++fake - +++++++ + +++++++ Contents of side #2 -b +conflict >>>>>>> @@ -328,14 +328,17 @@ fn test_resolution() { --- a/file +++ b/file @@ -1,7 +1,7 @@ - <<<<<<< - %%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --base -+a + -+++++++ Contents of side #2 + -b + +<<<<<<< + +%%%%%%% +-some ++fake - +++++++ - -b + ++++++++ +conflict >>>>>>> "###); @@ -398,14 +401,14 @@ fn test_normal_conflict_input_files() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% - -base - +a - +++++++ - b - >>>>>>> - "###); + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base + +a + +++++++ Contents of side #2 + b + >>>>>>> + "###); check_resolve_produces_input_file(&mut test_env, &repo_path, "base", "base\n"); check_resolve_produces_input_file(&mut test_env, &repo_path, "left", "a\n"); @@ -439,10 +442,10 @@ fn test_baseless_conflict_input_files() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 +a - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); @@ -510,10 +513,10 @@ fn test_edit_delete_conflict_input_files() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 a - %%%%%%% + %%%%%%% Changes from base to side #2 -base >>>>>>> "###); @@ -682,22 +685,22 @@ fn test_multiple_conflicts() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("this_file_has_a_very_long_name_to_test_padding")).unwrap() , @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -first base +first a - +++++++ + +++++++ Contents of side #2 first b >>>>>>> "###); insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("another_file")).unwrap() , @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -second base +second a - +++++++ + +++++++ Contents of side #2 second b >>>>>>> "###); @@ -742,11 +745,11 @@ fn test_multiple_conflicts() { --- a/another_file +++ b/another_file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --second base -+second a - -+++++++ + -+++++++ Contents of side #2 -second b ->>>>>>> +resolution another_file @@ -781,11 +784,11 @@ fn test_multiple_conflicts() { --- a/another_file +++ b/another_file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --second base -+second a - -+++++++ + -+++++++ Contents of side #2 -second b ->>>>>>> +first resolution for auto-chosen file @@ -808,11 +811,11 @@ fn test_multiple_conflicts() { --- a/another_file +++ b/another_file @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --second base -+second a - -+++++++ + -+++++++ Contents of side #2 -second b ->>>>>>> +first resolution for auto-chosen file @@ -821,11 +824,11 @@ fn test_multiple_conflicts() { --- a/this_file_has_a_very_long_name_to_test_padding +++ b/this_file_has_a_very_long_name_to_test_padding @@ -1,7 +1,1 @@ - -<<<<<<< - -%%%%%%% + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 --first base -+first a - -+++++++ + -+++++++ Contents of side #2 -first b ->>>>>>> +second resolution for auto-chosen file diff --git a/cli/tests/test_restore_command.rs b/cli/tests/test_restore_command.rs index 6a352e3a52..726b5a5f8c 100644 --- a/cli/tests/test_restore_command.rs +++ b/cli/tests/test_restore_command.rs @@ -171,25 +171,25 @@ fn test_restore_conflicted_merge() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% - -base - +a - +++++++ - b - >>>>>>> - "###); + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base + +a + +++++++ Contents of side #2 + b + >>>>>>> + "###); // Overwrite the file... std::fs::write(repo_path.join("file"), "resolution").unwrap(); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), @r###" Resolved conflict in file: - 1 : <<<<<<< - 2 : %%%%%%% + 1 : <<<<<<< Conflict 1 of 1 + 2 : %%%%%%% Changes from base to side #1 3 : -base 4 : +a - 5 : +++++++ + 5 : +++++++ Contents of side #2 6 : b 7 : >>>>>>> 1: resolution @@ -210,11 +210,11 @@ fn test_restore_conflicted_merge() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +a - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); @@ -226,11 +226,11 @@ fn test_restore_conflicted_merge() { insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), @r###" Resolved conflict in file: - 1 : <<<<<<< - 2 : %%%%%%% + 1 : <<<<<<< Conflict 1 of 1 + 2 : %%%%%%% Changes from base to side #1 3 : -base 4 : +a - 5 : +++++++ + 5 : +++++++ Contents of side #2 6 : b 7 : >>>>>>> 1: resolution @@ -251,11 +251,11 @@ fn test_restore_conflicted_merge() { insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() , @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -base +a - +++++++ + +++++++ Contents of side #2 b >>>>>>> "###); diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index 871939c6dd..bb1c5ee7ee 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -649,14 +649,14 @@ fn test_squash_from_multiple() { // The changes from the sources have been applied let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base #1 to side #1 -a +d - %%%%%%% + %%%%%%% Changes from base #2 to side #2 -a +b - +++++++ + +++++++ Contents of side #3 c >>>>>>> "###); @@ -786,14 +786,14 @@ fn test_squash_from_multiple_partial() { // The selected changes from the sources have been applied let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file1"]); insta::assert_snapshot!(stdout, @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base #1 to side #1 -a +d - %%%%%%% + %%%%%%% Changes from base #2 to side #2 -a +b - +++++++ + +++++++ Contents of side #3 c >>>>>>> "###); diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index af2e105d66..a4fa0fc994 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -29,11 +29,11 @@ use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::repo_path::RepoPath; use crate::store::Store; -const CONFLICT_START_LINE: &[u8] = b"<<<<<<<\n"; -const CONFLICT_END_LINE: &[u8] = b">>>>>>>\n"; -const CONFLICT_DIFF_LINE: &[u8] = b"%%%%%%%\n"; -const CONFLICT_MINUS_LINE: &[u8] = b"-------\n"; -const CONFLICT_PLUS_LINE: &[u8] = b"+++++++\n"; +const CONFLICT_START_LINE: &[u8] = b"<<<<<<<"; +const CONFLICT_END_LINE: &[u8] = b">>>>>>>"; +const CONFLICT_DIFF_LINE: &[u8] = b"%%%%%%%"; +const CONFLICT_MINUS_LINE: &[u8] = b"-------"; +const CONFLICT_PLUS_LINE: &[u8] = b"+++++++"; const CONFLICT_START_LINE_CHAR: u8 = CONFLICT_START_LINE[0]; const CONFLICT_END_LINE_CHAR: u8 = CONFLICT_END_LINE[0]; const CONFLICT_DIFF_LINE_CHAR: u8 = CONFLICT_DIFF_LINE[0]; @@ -204,19 +204,37 @@ pub fn materialize_merge_result( output.write_all(&content.0)?; } MergeResult::Conflict(hunks) => { + let num_conflicts = hunks + .iter() + .filter(|hunk| hunk.as_resolved().is_none()) + .count(); + let mut conflict_index = 0; for hunk in hunks { if let Some(content) = hunk.as_resolved() { output.write_all(&content.0)?; } else { + conflict_index += 1; output.write_all(CONFLICT_START_LINE)?; + output.write_all( + format!(" Conflict {conflict_index} of {num_conflicts}\n").as_bytes(), + )?; let mut add_index = 0; - for left in hunk.removes() { + for (base_index, left) in hunk.removes().enumerate() { + // The vast majority of conflicts one actually tries to + // resolve manually have 1 base. + let base_str = if hunk.removes().len() == 1 { + "base".to_string() + } else { + format!("base #{}", base_index + 1) + }; + let right1 = if let Some(right1) = hunk.get_add(add_index) { right1 } else { // If we have no more positive terms, emit the remaining negative // terms as snapshots. output.write_all(CONFLICT_MINUS_LINE)?; + output.write_all(format!(" Contents of {base_str}\n").as_bytes())?; output.write_all(&left.0)?; continue; }; @@ -236,8 +254,18 @@ pub fn materialize_merge_result( // the current positive term as a snapshot and the next // positive term as a diff. output.write_all(CONFLICT_PLUS_LINE)?; + output.write_all( + format!(" Contents of side #{}\n", add_index + 1).as_bytes(), + )?; output.write_all(&right1.0)?; output.write_all(CONFLICT_DIFF_LINE)?; + output.write_all( + format!( + " Changes from {base_str} to side #{}\n", + add_index + 2 + ) + .as_bytes(), + )?; write_diff_hunks(&diff2, output)?; add_index += 2; continue; @@ -245,16 +273,24 @@ pub fn materialize_merge_result( } output.write_all(CONFLICT_DIFF_LINE)?; + output.write_all( + format!(" Changes from {base_str} to side #{}\n", add_index + 1) + .as_bytes(), + )?; write_diff_hunks(&diff1, output)?; add_index += 1; } // Emit the remaining positive terms as snapshots. - for slice in hunk.adds().skip(add_index) { + for (add_index, slice) in hunk.adds().enumerate().skip(add_index) { output.write_all(CONFLICT_PLUS_LINE)?; + output.write_all( + format!(" Contents of side #{}\n", add_index + 1).as_bytes(), + )?; output.write_all(&slice.0)?; } output.write_all(CONFLICT_END_LINE)?; + output.write_all(b"\n")?; } } } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index cb0618f8a4..86447fb38f 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -77,12 +77,12 @@ fn test_materialize_conflict_basic() { @r###" line 1 line 2 - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 left 3.1 left 3.2 left 3.3 - %%%%%%% + %%%%%%% Changes from base to side #2 -line 3 +right 3.1 >>>>>>> @@ -101,11 +101,11 @@ fn test_materialize_conflict_basic() { @r###" line 1 line 2 - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -line 3 +right 3.1 - +++++++ + +++++++ Contents of side #2 left 3.1 left 3.2 left 3.3 @@ -173,16 +173,16 @@ fn test_materialize_conflict_multi_rebase_conflicts() { &materialize_conflict_string(store, path, &conflict), @r###" line 1 - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 line 2 a.1 line 2 a.2 line 2 a.3 - %%%%%%% + %%%%%%% Changes from base #1 to side #2 -line 2 base +line 2 b.1 +line 2 b.2 - %%%%%%% + %%%%%%% Changes from base #2 to side #3 -line 2 base +line 2 c.1 >>>>>>> @@ -197,15 +197,15 @@ fn test_materialize_conflict_multi_rebase_conflicts() { &materialize_conflict_string(store, path, &conflict), @r###" line 1 - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base #1 to side #1 -line 2 base +line 2 c.1 - %%%%%%% + %%%%%%% Changes from base #2 to side #2 -line 2 base +line 2 b.1 +line 2 b.2 - +++++++ + +++++++ Contents of side #3 line 2 a.1 line 2 a.2 line 2 a.3 @@ -221,15 +221,15 @@ fn test_materialize_conflict_multi_rebase_conflicts() { &materialize_conflict_string(store, path, &conflict), @r###" line 1 - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base #1 to side #1 -line 2 base +line 2 c.1 - +++++++ + +++++++ Contents of side #2 line 2 a.1 line 2 a.2 line 2 a.3 - %%%%%%% + %%%%%%% Changes from base #2 to side #3 -line 2 base +line 2 b.1 +line 2 b.2 @@ -288,22 +288,22 @@ fn test_materialize_parse_roundtrip() { insta::assert_snapshot!( materialized, @r###" - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 2 + +++++++ Contents of side #1 line 1 left line 2 left - %%%%%%% + %%%%%%% Changes from base to side #2 -line 1 +line 1 right line 2 >>>>>>> line 3 - <<<<<<< - %%%%%%% + <<<<<<< Conflict 2 of 2 + %%%%%%% Changes from base to side #1 line 4 -line 5 +line 5 left - +++++++ + +++++++ Contents of side #2 line 4 right line 5 right >>>>>>> @@ -385,10 +385,10 @@ fn test_materialize_conflict_modify_delete() { insta::assert_snapshot!(&materialize_conflict_string(store, path, &conflict), @r###" line 1 line 2 - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 modified - %%%%%%% + %%%%%%% Changes from base to side #2 -line 3 >>>>>>> line 4 @@ -404,10 +404,10 @@ fn test_materialize_conflict_modify_delete() { insta::assert_snapshot!(&materialize_conflict_string(store, path, &conflict), @r###" line 1 line 2 - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 -line 3 - +++++++ + +++++++ Contents of side #2 modified >>>>>>> line 4 @@ -421,15 +421,15 @@ fn test_materialize_conflict_modify_delete() { vec![Some(modified_id.clone()), None], ); insta::assert_snapshot!(&materialize_conflict_string(store, path, &conflict), @r###" - <<<<<<< - %%%%%%% + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 line 1 line 2 -line 3 +modified line 4 line 5 - +++++++ + +++++++ Contents of side #2 >>>>>>> "### ); @@ -474,16 +474,16 @@ fn test_materialize_conflict_two_forward_diffs() { insta::assert_snapshot!( &materialize_conflict_string(store, path, &conflict), @r###" - <<<<<<< - +++++++ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 A - %%%%%%% + %%%%%%% Changes from base #1 to side #2 B - +++++++ + +++++++ Contents of side #3 D - %%%%%%% + %%%%%%% Changes from base #2 to side #4 C - ------- + ------- Contents of base #3 E >>>>>>> "###