merge_tools: allow setting conflict marker style per-tool

I left the "merge-tool-edits-conflict-markers" option unchanged,
since removing it would likely break some existing configurations. It
also seems like it could be useful to have a merge tool use the default
conflict markers instead of requiring the conflict marker style to
always be set for the merge tool (e.g. if a merge tool allows the user
to manually edit the conflicts).

CHANGELOG.md

@@ -71,6 +71,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   "diff3" conflict style, meaning it is more likely to work with external tools,
   but it doesn't support conflicts with more than 2 sides.
 
+* New `merge-tools.<TOOL>.conflict-marker-style` config option to override the
+  conflict marker style used for a specific merge tool.
+
 * `jj simplify-parents` now supports configuring the default revset when no
    `--source` or `--revisions` arguments are provided with the
    `revsets.simplify-parents` config.

cli/src/config-schema.json

@@ -35,6 +35,18 @@
         "ui": {
             "type": "object",
             "description": "UI settings",
+            "definitions": {
+                "conflict-marker-style": {
+                    "type": "string",
+                    "description": "Conflict marker style to use when materializing conflicts in the working copy",
+                    "enum": [
+                        "diff",
+                        "snapshot",
+                        "git"
+                    ],
+                    "default": "diff"
+                }
+            },
             "properties": {
                 "allow-init-native": {
                     "type": "boolean",
@@ -160,14 +172,7 @@
                     "description": "Tool to use for resolving three-way merges. Behavior for a given tool name can be configured in merge-tools.TOOL tables"
                 },
                 "conflict-marker-style": {
-                    "type": "string",
-                    "description": "Conflict marker style to use when materializing conflicts in the working copy",
-                    "enum": [
-                        "diff",
-                        "snapshot",
-                        "git"
-                    ],
-                    "default": "diff"
+                    "$ref": "#/properties/ui/definitions/conflict-marker-style"
                 }
             }
         },
@@ -399,6 +404,9 @@
                         "type": "boolean",
                         "description": "Whether to populate the output file with conflict markers before starting the merge tool. See https://martinvonz.github.io/jj/latest/config/#editing-conflict-markers-with-a-tool-or-a-text-editor",
                         "default": false
+                    },
+                    "conflict-marker-style": {
+                        "$ref": "#/properties/ui/definitions/conflict-marker-style"
                     }
                 }
             }

cli/src/config/merge_tools.toml

@@ -39,6 +39,7 @@ program = "vim"
 merge-args = ["-f", "-d", "$output", "-M", "$left", "$base", "$right",
               "-c", "wincmd J", "-c", "set modifiable", "-c", "set write"]
 merge-tool-edits-conflict-markers = true
+conflict-marker-style = "snapshot"
 # Using vimdiff as a diff editor is not recommended. For instructions on configuring
 # the DirDiff Vim plugin for a better experience, see
 # https://gist.github.com/ilyagr/5d6339fb7dac5e7ab06fe1561ec62d45
@@ -52,10 +53,13 @@ merge-args = ["--wait", "--merge", "$left", "$right", "$base", "$output"]
 # markers. Unfortunately, it does not seem to be able to output conflict markers when
 # the user only resolves some of the conflicts.
 merge-tool-edits-conflict-markers = true
+# VS Code prefers Git-style conflict markers
+conflict-marker-style = "git"
 
 # free/libre distribution of vscode, functionally more or less the same
 [merge-tools.vscodium]
 program = "codium"
 merge-args = ["--wait", "--merge", "$left", "$right", "$base", "$output"]
 merge-tool-edits-conflict-markers = true
+conflict-marker-style = "git"
 

cli/src/merge_tools/external.rs

@@ -61,13 +61,15 @@ pub struct ExternalMergeTool {
     /// If false (default), the `$output` file starts out empty and is accepted
     /// as a full conflict resolution as-is by `jj` after the merge tool is
     /// done with it. If true, the `$output` file starts out with the
-    /// contents of the conflict, with JJ's conflict markers. After the
-    /// merge tool is done, any remaining conflict markers in the
-    /// file parsed and taken to mean that the conflict was only partially
+    /// contents of the conflict, with the configured conflict markers. After
+    /// the merge tool is done, any remaining conflict markers in the
+    /// file are parsed and taken to mean that the conflict was only partially
     /// resolved.
-    // TODO: Instead of a boolean, this could denote the flavor of conflict markers to put in
-    // the file (`jj` or `diff3` for example).
     pub merge_tool_edits_conflict_markers: bool,
+    /// If provided, overrides the normal conflict marker style setting. This is
+    /// useful if a tool parses conflict markers, and so it requires a specific
+    /// format, or if a certain format is more readable than another.
+    pub conflict_marker_style: Option<ConflictMarkerStyle>,
 }
 
 #[derive(serde::Deserialize, Copy, Clone, Debug, Eq, PartialEq)]
@@ -92,6 +94,7 @@ impl Default for ExternalMergeTool {
             edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(),
             merge_args: vec![],
             merge_tool_edits_conflict_markers: false,
+            conflict_marker_style: None,
             diff_invocation_mode: DiffToolMode::Dir,
         }
     }
@@ -158,8 +161,12 @@ pub fn run_mergetool_external(
     repo_path: &RepoPath,
     conflict: MergedTreeValue,
     tree: &MergedTree,
-    conflict_marker_style: ConflictMarkerStyle,
+    default_conflict_marker_style: ConflictMarkerStyle,
 ) -> Result<MergedTreeId, ConflictResolveError> {
+    let conflict_marker_style = editor
+        .conflict_marker_style
+        .unwrap_or(default_conflict_marker_style);
+
     let initial_output_content = if editor.merge_tool_edits_conflict_markers {
         materialize_merge_result_to_bytes(&content, conflict_marker_style)
     } else {
@@ -259,8 +266,15 @@ pub fn edit_diff_external(
     matcher: &dyn Matcher,
     instructions: Option<&str>,
     base_ignores: Arc<GitIgnoreFile>,
-    options: &CheckoutOptions,
+    default_conflict_marker_style: ConflictMarkerStyle,
 ) -> Result<MergedTreeId, DiffEditError> {
+    let conflict_marker_style = editor
+        .conflict_marker_style
+        .unwrap_or(default_conflict_marker_style);
+    let options = CheckoutOptions {
+        conflict_marker_style,
+    };
+
     let got_output_field = find_all_variables(&editor.edit_args).contains(&"output");
     let store = left_tree.store();
     let diffedit_wc = DiffEditWorkingCopies::check_out(
@@ -270,7 +284,7 @@ pub fn edit_diff_external(
         matcher,
         got_output_field.then_some(DiffSide::Right),
         instructions,
-        options,
+        &options,
     )?;
 
     let patterns = diffedit_wc.working_copies.to_command_variables();
@@ -300,12 +314,15 @@ pub fn generate_diff(
     right_tree: &MergedTree,
     matcher: &dyn Matcher,
     tool: &ExternalMergeTool,
-    conflict_marker_style: ConflictMarkerStyle,
+    default_conflict_marker_style: ConflictMarkerStyle,
 ) -> Result<(), DiffGenerateError> {
-    let store = left_tree.store();
+    let conflict_marker_style = tool
+        .conflict_marker_style
+        .unwrap_or(default_conflict_marker_style);
     let options = CheckoutOptions {
         conflict_marker_style,
     };
+    let store = left_tree.store();
     let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, None, &options)?;
     set_readonly_recursively(diff_wc.left_working_copy_path())
         .map_err(ExternalToolError::SetUpDir)?;

cli/src/merge_tools/mod.rs

@@ -30,7 +30,6 @@ use jj_lib::repo_path::RepoPath;
 use jj_lib::repo_path::RepoPathBuf;
 use jj_lib::settings::ConfigResultExt as _;
 use jj_lib::settings::UserSettings;
-use jj_lib::working_copy::CheckoutOptions;
 use jj_lib::working_copy::SnapshotError;
 use pollster::FutureExt;
 use thiserror::Error;
@@ -239,9 +238,6 @@ impl DiffEditor {
         matcher: &dyn Matcher,
         format_instructions: impl FnOnce() -> String,
     ) -> Result<MergedTreeId, DiffEditError> {
-        let checkout_options = CheckoutOptions {
-            conflict_marker_style: self.conflict_marker_style,
-        };
         match &self.tool {
             MergeTool::Builtin => {
                 Ok(
@@ -258,7 +254,7 @@ impl DiffEditor {
                     matcher,
                     instructions.as_deref(),
                     self.base_ignores.clone(),
-                    &checkout_options,
+                    self.conflict_marker_style,
                 )
             }
         }
@@ -406,6 +402,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -433,6 +430,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -472,6 +470,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -495,6 +494,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -517,6 +517,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -545,6 +546,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -571,6 +573,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -591,6 +594,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -643,6 +647,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -690,6 +695,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -718,6 +724,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);
@@ -749,6 +756,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                conflict_marker_style: None,
             },
         )
         "###);

cli/tests/test_diff_command.rs

@@ -2303,6 +2303,113 @@ fn test_diff_external_tool_symlink() {
     );
 }
 
+#[test]
+fn test_diff_external_tool_conflict_marker_style() {
+    let mut 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");
+    let file_path = repo_path.join("file");
+
+    // Create a conflict
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2
+        line 3
+        line 4
+        line 5
+    "},
+    )
+    .unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "base"]);
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.1
+        line 2.2
+        line 3
+        line 4.1
+        line 5
+    "},
+    )
+    .unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "side-a"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "description(base)", "-m", "side-b"]);
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.3
+        line 3
+        line 4.2
+        line 4.3
+        line 5
+    "},
+    )
+    .unwrap();
+
+    // Resolve one of the conflicts in the working copy
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &["new", "description(side-a)", "description(side-b)"],
+    );
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.1
+        line 2.2
+        line 2.3
+        line 3
+        <<<<<<<
+        %%%%%%%
+        -line 4
+        +line 4.1
+        +++++++
+        line 4.2
+        line 4.3
+        >>>>>>>
+        line 5
+    "},
+    )
+    .unwrap();
+
+    // Set up diff editor to use "snapshot" conflict markers
+    let edit_script = test_env.set_up_fake_diff_editor();
+    test_env.add_config(r#"merge-tools.fake-diff-editor.conflict-marker-style = "snapshot""#);
+
+    // We want to see whether the diff is using the correct conflict markers
+    std::fs::write(
+        &edit_script,
+        ["files-before file", "files-after file", "dump file file"].join("\0"),
+    )
+    .unwrap();
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diff", "--tool", "fake-diff-editor"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @"");
+    // Conflicts should render using "snapshot" format
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("file")).unwrap(), @r##"
+    line 1
+    line 2.1
+    line 2.2
+    line 2.3
+    line 3
+    <<<<<<< Conflict 1 of 1
+    +++++++ Contents of side #1
+    line 4.1
+    ------- Contents of base
+    line 4
+    +++++++ Contents of side #2
+    line 4.2
+    line 4.3
+    >>>>>>> Conflict 1 of 1 ends
+    line 5
+    "##);
+}
+
 #[test]
 fn test_diff_stat() {
     let test_env = TestEnvironment::default();

cli/tests/test_diffedit_command.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::escaped_fake_diff_editor_path;
 use crate::common::TestEnvironment;
 
@@ -235,6 +237,189 @@ fn test_diffedit_new_file() {
     "###);
 }
 
+#[test]
+fn test_diffedit_external_tool_conflict_marker_style() {
+    let mut 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");
+    let file_path = repo_path.join("file");
+
+    // Create a conflict
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2
+        line 3
+        line 4
+        line 5
+    "},
+    )
+    .unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "base"]);
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.1
+        line 2.2
+        line 3
+        line 4.1
+        line 5
+    "},
+    )
+    .unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "side-a"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "description(base)", "-m", "side-b"]);
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.3
+        line 3
+        line 4.2
+        line 4.3
+        line 5
+    "},
+    )
+    .unwrap();
+
+    // Resolve one of the conflicts in the working copy
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &["new", "description(side-a)", "description(side-b)"],
+    );
+    std::fs::write(
+        &file_path,
+        indoc! {"
+        line 1
+        line 2.1
+        line 2.2
+        line 2.3
+        line 3
+        <<<<<<<
+        %%%%%%%
+        -line 4
+        +line 4.1
+        +++++++
+        line 4.2
+        line 4.3
+        >>>>>>>
+        line 5
+    "},
+    )
+    .unwrap();
+
+    // Set up diff editor to use "snapshot" conflict markers
+    let edit_script = test_env.set_up_fake_diff_editor();
+    test_env.add_config(r#"merge-tools.fake-diff-editor.conflict-marker-style = "snapshot""#);
+
+    // We want to see whether the diff editor is using the correct conflict markers,
+    // and reset it to make sure that it parses the conflict markers as well
+    std::fs::write(
+        &edit_script,
+        [
+            "files-before file",
+            "files-after JJ-INSTRUCTIONS file",
+            "dump file after-file",
+            "reset file",
+            "dump file before-file",
+        ]
+        .join("\0"),
+    )
+    .unwrap();
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r#"
+    Created mzvwutvl 7b92839f (conflict) (empty) (no description set)
+    Working copy now at: mzvwutvl 7b92839f (conflict) (empty) (no description set)
+    Parent commit      : rlvkpnrz 3765cc27 side-a
+    Parent commit      : zsuskuln 8b3de837 side-b
+    Added 0 files, modified 1 files, removed 0 files
+    There are unresolved conflicts at these paths:
+    file    2-sided conflict
+    Existing conflicts were resolved or abandoned from these commits:
+      mzvwutvl hidden fae32b29 (conflict) (no description set)
+    "#);
+    // Conflicts should render using "snapshot" format in diff editor
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("before-file")).unwrap(), @r##"
+    line 1
+    <<<<<<< Conflict 1 of 2
+    +++++++ Contents of side #1
+    line 2.1
+    line 2.2
+    ------- Contents of base
+    line 2
+    +++++++ Contents of side #2
+    line 2.3
+    >>>>>>> Conflict 1 of 2 ends
+    line 3
+    <<<<<<< Conflict 2 of 2
+    +++++++ Contents of side #1
+    line 4.1
+    ------- Contents of base
+    line 4
+    +++++++ Contents of side #2
+    line 4.2
+    line 4.3
+    >>>>>>> Conflict 2 of 2 ends
+    line 5
+    "##);
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("after-file")).unwrap(), @r##"
+    line 1
+    line 2.1
+    line 2.2
+    line 2.3
+    line 3
+    <<<<<<< Conflict 1 of 1
+    +++++++ Contents of side #1
+    line 4.1
+    ------- Contents of base
+    line 4
+    +++++++ Contents of side #2
+    line 4.2
+    line 4.3
+    >>>>>>> Conflict 1 of 1 ends
+    line 5
+    "##);
+    // Conflicts should be materialized using "diff" format in working copy
+    insta::assert_snapshot!(
+        std::fs::read_to_string(&file_path).unwrap(), @r##"
+    line 1
+    <<<<<<< Conflict 1 of 2
+    +++++++ Contents of side #1
+    line 2.1
+    line 2.2
+    %%%%%%% Changes from base to side #2
+    -line 2
+    +line 2.3
+    >>>>>>> Conflict 1 of 2 ends
+    line 3
+    <<<<<<< Conflict 2 of 2
+    %%%%%%% Changes from base to side #1
+    -line 4
+    +line 4.1
+    +++++++ Contents of side #2
+    line 4.2
+    line 4.3
+    >>>>>>> Conflict 2 of 2 ends
+    line 5
+    "##);
+
+    // File should be conflicted with no changes
+    let stdout = test_env.jj_cmd_success(&repo_path, &["st"]);
+    insta::assert_snapshot!(stdout, @r#"
+    The working copy is clean
+    There are unresolved conflicts at these paths:
+    file    2-sided conflict
+    Working copy : mzvwutvl 7b92839f (conflict) (empty) (no description set)
+    Parent commit: rlvkpnrz 3765cc27 side-a
+    Parent commit: zsuskuln 8b3de837 side-b
+    "#);
+}
+
 #[test]
 fn test_diffedit_3pane() {
     let mut test_env = TestEnvironment::default();

cli/tests/test_resolve_command.rs

@@ -348,6 +348,90 @@ fn test_resolution() {
     Error: No conflicts found at this revision
     "###);
 
+    // Check that merge tool can override conflict marker style setting, and that
+    // the merge tool can output Git-style conflict markers
+    test_env.jj_cmd_ok(&repo_path, &["undo"]);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), 
+        @"");
+    std::fs::write(
+        &editor_script,
+        [
+            "dump editor4",
+            indoc! {"
+                write
+                <<<<<<<
+                some
+                |||||||
+                fake
+                =======
+                conflict
+                >>>>>>>
+            "},
+        ]
+        .join("\0"),
+    )
+    .unwrap();
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "resolve",
+            "--config-toml",
+            r#"
+            [merge-tools.fake-editor]
+            merge-tool-edits-conflict-markers = true
+            conflict-marker-style = "git"
+            "#,
+        ],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r#"
+    Resolving conflicts in: file
+    Working copy now at: vruxwmqv 6701dfd3 conflict | (conflict) conflict
+    Parent commit      : zsuskuln aa493daf a | a
+    Parent commit      : royxmykx db6a4daf b | b
+    Added 0 files, modified 1 files, removed 0 files
+    There are unresolved conflicts at these paths:
+    file    2-sided conflict
+    New conflicts appeared in these commits:
+      vruxwmqv 6701dfd3 conflict | (conflict) conflict
+    To resolve the conflicts, start by updating to it:
+      jj new vruxwmqv
+    Then use `jj resolve`, or edit the conflict markers in the file directly.
+    Once the conflicts are resolved, you may want to inspect the result with `jj diff`.
+    Then run `jj squash` to move the resolution into the conflicted commit.
+    "#);
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("editor4")).unwrap(), @r##"
+    <<<<<<< Side #1 (Conflict 1 of 1)
+    a
+    ||||||| Base
+    base
+    =======
+    b
+    >>>>>>> Side #2 (Conflict 1 of 1 ends)
+    "##);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), 
+    @r##"
+    diff --git a/file b/file
+    --- a/file
+    +++ b/file
+    @@ -1,7 +1,7 @@
+     <<<<<<< Conflict 1 of 1
+     %%%%%%% Changes from base to side #1
+    --base
+    -+a
+    +-fake
+    ++some
+     +++++++ Contents of side #2
+    -b
+    +conflict
+     >>>>>>> Conflict 1 of 1 ends
+    "##);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), 
+    @r###"
+    file    2-sided conflict
+    "###);
+
     // TODO: Check that running `jj new` and then `jj resolve -r conflict` works
     // correctly.
 }

docs/config.md

@@ -856,6 +856,9 @@ With this option set, if the output file still contains conflict markers after
 the conflict is done, `jj` assumes that the conflict was only partially resolved
 and parses the conflict markers to get the new state of the conflict. The
 conflict is considered fully resolved when there are no conflict markers left.
+The conflict marker style can also be customized per tool using the
+`merge-tools.TOOL.conflict-marker-style` option, which takes the same values as
+[`ui.conflict-marker-style`](#conflict-marker-style).
 
 ## Code formatting and other file content transformations