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).
jj-vcs · Nov 24, 2024 · 2a8c285 · 2a8c285
1 parent 10c90a5
commit 2a8c285
Show file tree

Hide file tree

Showing 9 changed files with 442 additions and 23 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 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)?;

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,
             },
         )
         "###);