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).
martinvonz · Nov 23, 2024 · 2279d2f · 2279d2f
1 parent 81251c9
commit 2279d2f
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 21 deletions.
diff --git a/CHANGELOG.md b/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.
+
 ### Fixed bugs
 
 * `jj config unset <TABLE-NAME>` no longer removes a table (such as `[ui]`.)

diff --git a/cli/src/config-schema.json b/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"
                     }
                 }
             }

diff --git a/cli/src/config/merge_tools.toml b/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"
 
diff --git a/cli/src/merge_tools/external.rs b/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 merge tool parses conflict markers, and so it requires a
+    /// specific format.
+    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();

diff --git a/cli/src/merge_tools/mod.rs b/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,
             },
         )
         "###);

diff --git a/cli/tests/test_resolve_command.rs b/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.
 }

diff --git a/docs/config.md b/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