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();