From 26f5d6150c12af0f25a1e701b3ed8be2527e072a Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Sun, 17 Nov 2024 08:00:04 -0600 Subject: [PATCH] conflicts: add "git" conflict marker style Adds a new "git" conflict marker style option. This option matches Git's "diff3" conflict style, allowing these conflicts to be parsed by some external tools that don't support JJ-style conflicts. If a conflict has more than 2 sides, then it falls back to the similar "snapshot" conflict marker style. The conflict parsing code now supports parsing Git-style conflict markers in addition to the normal JJ-style conflict markers, regardless of the conflict marker style setting. This has the benefit of allowing the user to switch the conflict marker style while they already have conflicts checked out, and their old conflicts will still be parsed correctly. Example of "git" conflict markers: ``` <<<<<<< Side #1 (Conflict 1 of 1) fn example(word: String) { println!("word is {word}"); ||||||| Base fn example(w: String) { println!("word is {w}"); ======= fn example(w: &str) { println!("word is {w}"); >>>>>>> Side #2 (Conflict 1 of 1 ends) } ``` --- CHANGELOG.md | 4 +- cli/src/config-schema.json | 3 +- cli/tests/test_working_copy.rs | 102 +++++++++++ lib/src/conflicts.rs | 122 ++++++++++++- lib/tests/test_conflicts.rs | 310 ++++++++++++++++++++++++++++++++- 5 files changed, 534 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae3f0f1c1d..c2b040bc35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,7 +67,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). materialized in the working copy. The default option ("diff") renders conflicts as a snapshot with a list of diffs to apply to the snapshot. The new "snapshot" option renders conflicts as a series of snapshots, showing - each side and base of the conflict. + each side and base of the conflict. The new "git" option replicates Git's + "diff3" conflict style, meaning it is more likely to work with external tools, + but it doesn't support conflicts with more than 2 sides. ### Fixed bugs diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 0468e95c35..436f86cf26 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -164,7 +164,8 @@ "description": "Conflict marker style to use when materializing conflicts in the working copy", "enum": [ "diff", - "snapshot" + "snapshot", + "git" ], "default": "diff" } diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs index d826638858..3efb8a5073 100644 --- a/cli/tests/test_working_copy.rs +++ b/cli/tests/test_working_copy.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use indoc::indoc; + use crate::common::TestEnvironment; #[test] @@ -57,3 +59,103 @@ fn test_snapshot_large_file() { let stdout = test_env.jj_cmd_success(&repo_path, &["file", "list"]); insta::assert_snapshot!(stdout, @""); } + +#[test] +fn test_materialize_and_snapshot_different_conflict_markers() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + // Configure to use Git-style conflict markers + test_env.add_config(r#"ui.conflict-marker-style = "git""#); + + // Create a conflict in the working copy + let conflict_file = repo_path.join("file"); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + line 2 + line 3 + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "base"]); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + line 2 - a + line 3 + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "side-a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "description(base)", "-m", "side-b"]); + std::fs::write( + &conflict_file, + indoc! {" + line 1 + line 2 - b + line 3 - b + "}, + ) + .unwrap(); + test_env.jj_cmd_ok( + &repo_path, + &["new", "description(side-a)", "description(side-b)"], + ); + + // File should have Git-style conflict markers + insta::assert_snapshot!(std::fs::read_to_string(&conflict_file).unwrap(), @r##" + line 1 + <<<<<<< Side #1 (Conflict 1 of 1) + line 2 - a + line 3 + ||||||| Base + line 2 + line 3 + ======= + line 2 - b + line 3 - b + >>>>>>> Side #2 (Conflict 1 of 1 ends) + "##); + + // Configure to use JJ-style "snapshot" conflict markers + test_env.add_config(r#"ui.conflict-marker-style = "snapshot""#); + + // Update the conflict, still using Git-style conflict markers + std::fs::write( + &conflict_file, + indoc! {" + line 1 + <<<<<<< + line 2 - a + line 3 - a + ||||||| + line 2 + line 3 + ======= + line 2 - b + line 3 - b + >>>>>>> + "}, + ) + .unwrap(); + + // Git-style markers should be parsed, then rendered with new config + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r##" + diff --git a/file b/file + --- a/file + +++ b/file + @@ -2,7 +2,7 @@ + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 + line 2 - a + -line 3 + +line 3 - a + ------- Contents of base + line 2 + line 3 + "##); +} diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 705478a7bd..32997363c1 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -56,11 +56,15 @@ const CONFLICT_END_LINE: &str = ">>>>>>>"; const CONFLICT_DIFF_LINE: &str = "%%%%%%%"; const CONFLICT_MINUS_LINE: &str = "-------"; const CONFLICT_PLUS_LINE: &str = "+++++++"; +const CONFLICT_GIT_ANCESTOR_LINE: &str = "|||||||"; +const CONFLICT_GIT_SEPARATOR_LINE: &str = "======="; const CONFLICT_START_LINE_CHAR: u8 = CONFLICT_START_LINE.as_bytes()[0]; const CONFLICT_END_LINE_CHAR: u8 = CONFLICT_END_LINE.as_bytes()[0]; const CONFLICT_DIFF_LINE_CHAR: u8 = CONFLICT_DIFF_LINE.as_bytes()[0]; const CONFLICT_MINUS_LINE_CHAR: u8 = CONFLICT_MINUS_LINE.as_bytes()[0]; const CONFLICT_PLUS_LINE_CHAR: u8 = CONFLICT_PLUS_LINE.as_bytes()[0]; +const CONFLICT_GIT_ANCESTOR_LINE_CHAR: u8 = CONFLICT_GIT_ANCESTOR_LINE.as_bytes()[0]; +const CONFLICT_GIT_SEPARATOR_LINE_CHAR: u8 = CONFLICT_GIT_SEPARATOR_LINE.as_bytes()[0]; /// A conflict marker is one of the separators, optionally followed by a space /// and some text. @@ -68,7 +72,7 @@ const CONFLICT_PLUS_LINE_CHAR: u8 = CONFLICT_PLUS_LINE.as_bytes()[0]; // 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(|| { - RegexBuilder::new(r"^(<{7}|>{7}|%{7}|\-{7}|\+{7})( .*)?$") + RegexBuilder::new(r"^(<{7}|>{7}|%{7}|\-{7}|\+{7}|\|{7}|={7})( .*)?$") .multi_line(true) .build() .unwrap() @@ -242,6 +246,8 @@ pub enum ConflictMarkerStyle { Diff, /// Style which shows a snapshot for each base and side. Snapshot, + /// Style which replicates Git's "diff3" style to support external tools. + Git, } pub fn materialize_merge_result>( @@ -291,12 +297,44 @@ fn materialize_conflict_hunks( conflict_index += 1; let conflict_info = format!("Conflict {conflict_index} of {num_conflicts}"); - materialize_jj_style_conflict(hunk, &conflict_info, conflict_marker_style, output)?; + match (conflict_marker_style, hunk.as_slice()) { + // 2-sided conflicts can use Git-style conflict markers + (ConflictMarkerStyle::Git, [left, base, right]) => { + materialize_git_style_conflict(left, base, right, &conflict_info, output)?; + } + _ => { + materialize_jj_style_conflict( + hunk, + &conflict_info, + conflict_marker_style, + output, + )?; + } + } } } Ok(()) } +fn materialize_git_style_conflict( + left: &[u8], + base: &[u8], + right: &[u8], + conflict_info: &str, + output: &mut dyn Write, +) -> io::Result<()> { + writeln!(output, "{CONFLICT_START_LINE} Side #1 ({conflict_info})")?; + output.write_all(left)?; + writeln!(output, "{CONFLICT_GIT_ANCESTOR_LINE} Base")?; + output.write_all(base)?; + // VS Code doesn't seem to support any trailing text on the separator line + writeln!(output, "{CONFLICT_GIT_SEPARATOR_LINE}")?; + output.write_all(right)?; + writeln!(output, "{CONFLICT_END_LINE} Side #2 ({conflict_info} ends)")?; + + Ok(()) +} + fn materialize_jj_style_conflict( hunk: &Merge, conflict_info: &str, @@ -474,7 +512,33 @@ pub fn parse_conflict(input: &[u8], num_sides: usize) -> Option Merge { + // If the hunk starts with a conflict marker, find its first character + let initial_conflict_marker_char = input + .lines_with_terminator() + .next() + .filter(|line| is_conflict_marker_line(line)) + .map(|line| line[0]); + + match initial_conflict_marker_char { + // JJ-style conflicts must start with one of these 3 conflict marker lines + Some(CONFLICT_DIFF_LINE_CHAR | CONFLICT_MINUS_LINE_CHAR | CONFLICT_PLUS_LINE_CHAR) => { + parse_jj_style_conflict_hunk(input) + } + // Git-style conflicts either must not start with a conflict marker line, or must start with + // the "|||||||" conflict marker line (if the first side was empty) + None | Some(CONFLICT_GIT_ANCESTOR_LINE_CHAR) => parse_git_style_conflict_hunk(input), + // No other conflict markers are allowed at the start of a hunk + Some(_) => Merge::resolved(BString::new(vec![])), + } +} + +fn parse_jj_style_conflict_hunk(input: &[u8]) -> Merge { enum State { Diff, Minus, @@ -484,7 +548,7 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge { let mut state = State::Unknown; let mut removes = vec![]; let mut adds = vec![]; - for line in input.split_inclusive(|b| *b == b'\n') { + for line in input.lines_with_terminator() { if is_conflict_marker_line(line) { match line[0] { CONFLICT_DIFF_LINE_CHAR => { @@ -505,7 +569,7 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge { } _ => {} } - }; + } match state { State::Diff => { if let Some(rest) = line.strip_prefix(b"-") { @@ -547,6 +611,56 @@ fn parse_conflict_hunk(input: &[u8]) -> Merge { } } +fn parse_git_style_conflict_hunk(input: &[u8]) -> Merge { + #[derive(PartialEq, Eq)] + enum State { + Left, + Base, + Right, + } + let mut state = State::Left; + let mut left = BString::new(vec![]); + let mut base = BString::new(vec![]); + let mut right = BString::new(vec![]); + for line in input.lines_with_terminator() { + if is_conflict_marker_line(line) { + match line[0] { + CONFLICT_GIT_ANCESTOR_LINE_CHAR => { + if state == State::Left { + state = State::Base; + continue; + } else { + // Base must come after left + return Merge::resolved(BString::new(vec![])); + } + } + CONFLICT_GIT_SEPARATOR_LINE_CHAR => { + if state == State::Base { + state = State::Right; + continue; + } else { + // Right must come after base + return Merge::resolved(BString::new(vec![])); + } + } + _ => {} + } + } + match state { + State::Left => left.extend_from_slice(line), + State::Base => base.extend_from_slice(line), + State::Right => right.extend_from_slice(line), + } + } + + if state == State::Right { + Merge::from_vec(vec![left, base, right]) + } else { + // Doesn't look like a valid conflict + Merge::resolved(BString::new(vec![])) + } +} + /// Check whether a line is a conflict marker. Removes trailing whitespace /// before checking against regex to ensure it parses CRLF endings correctly. fn is_conflict_marker_line(line: &[u8]) -> bool { diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index e3e19b3f03..e3c4a00f6f 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -140,6 +140,29 @@ fn test_materialize_conflict_basic() { line 5 "## ); + // Test materializing "git" conflict markers + let conflict = Merge::from_removes_adds( + vec![Some(base_id.clone())], + vec![Some(left_id.clone()), Some(right_id.clone())], + ); + insta::assert_snapshot!( + &materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Git), + @r##" + line 1 + line 2 + <<<<<<< Side #1 (Conflict 1 of 1) + left 3.1 + left 3.2 + left 3.3 + ||||||| Base + line 3 + ======= + right 3.1 + >>>>>>> Side #2 (Conflict 1 of 1 ends) + line 4 + line 5 + "## + ); } #[test] @@ -255,6 +278,34 @@ fn test_materialize_conflict_three_sides() { line 5 "## ); + // Test materializing "git" conflict markers (falls back to "snapshot" since + // "git" conflict markers don't support more than 2 sides) + insta::assert_snapshot!( + &materialize_conflict_string(store, path, &conflict, ConflictMarkerStyle::Git), + @r##" + line 1 + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 + line 2 a.1 + line 3 a.2 + line 4 base + ------- Contents of base #1 + line 2 base + line 3 base + line 4 base + +++++++ Contents of side #2 + line 2 b.1 + line 3 base + line 4 b.2 + ------- Contents of base #2 + line 2 base + +++++++ Contents of side #3 + line 2 base + line 3 c.2 + >>>>>>> Conflict 1 of 1 ends + line 5 + "## + ); } #[test] @@ -525,7 +576,11 @@ fn test_materialize_parse_roundtrip_different_markers() { vec![Some(a_id.clone()), Some(b_id.clone())], ); - let all_styles = [ConflictMarkerStyle::Diff, ConflictMarkerStyle::Snapshot]; + let all_styles = [ + ConflictMarkerStyle::Diff, + ConflictMarkerStyle::Snapshot, + ConflictMarkerStyle::Git, + ]; // For every pair of conflict marker styles, materialize the conflict using the // first style and parse it using the second. It should return the same result @@ -898,6 +953,80 @@ fn test_parse_conflict_simple() { ) "# ); + // Test "git" style + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<< Side #1 + line 3.1 + line 3.2 + ||||||| Base + line 3 + line 4 + ======= Side #2 + line 3 + line 4.1 + >>>>>>> End + line 5 + "}, + 2 + ), + @r#" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "line 3.1\nline 3.2\n", + "line 3\nline 4\n", + "line 3\nline 4.1\n", + ], + ), + Resolved( + "line 5\n", + ), + ], + ) + "# + ); + // Test "git" style with empty side 1 + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<< Side #1 + ||||||| Base + line 3 + line 4 + ======= Side #2 + line 3.1 + line 4.1 + >>>>>>> End + line 5 + "}, + 2 + ), + @r#" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "", + "line 3\nline 4\n", + "line 3.1\nline 4.1\n", + ], + ), + Resolved( + "line 5\n", + ), + ], + ) + "# + ); // The conflict markers are too long and shouldn't parse (though we may // decide to change this in the future) insta::assert_debug_snapshot!( @@ -1257,6 +1386,185 @@ fn test_parse_conflict_snapshot_missing_header() { ); } +#[test] +fn test_parse_conflict_wrong_git_style() { + // The "|||||||" section is missing + assert_eq!( + parse_conflict( + indoc! {b" + line 1 + <<<<<<< + left + ======= + right + >>>>>>> + line 5 + "}, + 2 + ), + None + ); +} + +#[test] +fn test_parse_conflict_git_reordered_headers() { + // The "=======" header must come after the "|||||||" header + assert_eq!( + parse_conflict( + indoc! {b" + line 1 + <<<<<<< + left + ======= + right + ||||||| + base + >>>>>>> + line 5 + "}, + 2 + ), + None + ); +} + +#[test] +fn test_parse_conflict_git_too_many_sides() { + // Git-style conflicts only allow 2 sides + assert_eq!( + parse_conflict( + indoc! {b" + line 1 + <<<<<<< + a + ||||||| + b + ======= + c + ||||||| + d + ======= + e + >>>>>>> + line 5 + "}, + 3 + ), + None + ); +} + +#[test] +fn test_parse_conflict_mixed_header_styles() { + // "|||||||" can't be used in place of "-------" + assert_eq!( + parse_conflict( + indoc! {b" + line 1 + <<<<<<< + +++++++ + left + ||||||| + base + +++++++ + right + >>>>>>> + line 5 + "}, + 2 + ), + None + ); + // "+++++++" can't be used in place of "=======" + assert_eq!( + parse_conflict( + indoc! {b" + line 1 + <<<<<<< + left + ||||||| + base + +++++++ + right + >>>>>>> + line 5 + "}, + 2 + ), + None + ); + // Test Git-style markers are ignored inside of JJ-style conflict + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<< Conflict 1 of 1 + +++++++ Contents of side #1 + ======= ignored + ------- Contents of base + ||||||| ignored + +++++++ Contents of side #2 + >>>>>>> Conflict 1 of 1 ends + line 5 + "}, + 2 + ), + @r#" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "======= ignored\n", + "||||||| ignored\n", + "", + ], + ), + Resolved( + "line 5\n", + ), + ], + ) + "# + ); + // Test JJ-style markers are ignored inside of Git-style conflict + insta::assert_debug_snapshot!( + parse_conflict(indoc! {b" + line 1 + <<<<<<< Side #1 (Conflict 1 of 1) + ||||||| Base + ------- ignored + %%%%%%% ignored + ======= + +++++++ ignored + >>>>>>> Side #2 (Conflict 1 of 1 ends) + line 5 + "}, + 2 + ), + @r#" + Some( + [ + Resolved( + "line 1\n", + ), + Conflicted( + [ + "", + "------- ignored\n%%%%%%% ignored\n", + "+++++++ ignored\n", + ], + ), + Resolved( + "line 5\n", + ), + ], + ) + "# + ); +} + #[test] fn test_update_conflict_from_content() { let test_repo = TestRepo::init();