From c3ce9e777480424b3a34de4e00ff9557b3d99197 Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Sat, 16 Nov 2024 11:25:30 -0600 Subject: [PATCH] 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 | 3 ++ cli/src/config-schema.json | 24 ++++++--- cli/src/config/merge_tools.toml | 3 ++ cli/src/merge_tools/external.rs | 19 ++++--- cli/src/merge_tools/mod.rs | 12 +++++ cli/tests/test_resolve_command.rs | 84 +++++++++++++++++++++++++++++++ 6 files changed, 131 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96a809a1d43..83458c486fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 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. +* New `merge-tools..conflict-marker-style` config option to override the + conflict marker style used for a specific merge tool. + ### Fixed bugs * `jj config unset ` no longer removes a table (such as `[ui]`.) diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 70080ccd516..b9989ebb644 100644 --- 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-diff3" + ], + "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-diff3" - ], - "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 index f560053a62f..a3eeac7ad2b 100644 --- a/cli/src/config/merge_tools.toml +++ b/cli/src/config/merge_tools.toml @@ -52,10 +52,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-diff3" # 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-diff3" diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 3bb1157f298..ed5915bfda8 100644 --- 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, } #[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 { + 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 { diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index a540cf0a50a..0afcb59411a 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -406,6 +406,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -433,6 +434,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -472,6 +474,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -495,6 +498,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -517,6 +521,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -545,6 +550,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -571,6 +577,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -591,6 +598,7 @@ mod tests { ], merge_args: [], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -643,6 +651,7 @@ mod tests { "$output", ], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -690,6 +699,7 @@ mod tests { "$output", ], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -718,6 +728,7 @@ mod tests { "$output", ], merge_tool_edits_conflict_markers: false, + conflict_marker_style: None, }, ) "###); @@ -749,6 +760,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 index 667ba485a6b..60620b1c223 100644 --- 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-diff3" + "#, + ], + ); + 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. }