From 6e4ed59448376fd6ebb53b0fe9d50439ade50f77 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 +++++++++++++++++++++++++++++++ docs/config.md | 3 ++ 7 files changed, 134 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2b040bc359..4da7af92c23 100644 --- 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..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 436f86cf265..82b4a839da2 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" + ], + "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 index f560053a62f..14eb38db931 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" # 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 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..7dd27a441f2 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" + "#, + ], + ); + 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 index 60ee143596d..c4b05a0d162 100644 --- 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