From a042e13453e54f763f6cdfbae53455dcd368714a Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Mon, 25 Nov 2024 13:55:20 -0600 Subject: [PATCH] merge_tools: add merge-tool-error-status-on-conflict Some Git merge drivers can partially resolve conflicts, leaving some conflict markers in the output file. In that case, they exit with a status between 1 and 127 to indicate that there was a conflict remaining in the file. We should support this convention as well, since it will allow many Git merge drivers to be used with Jujutsu. --- CHANGELOG.md | 4 ++ cli/src/config-schema.json | 5 +++ cli/src/merge_tools/external.rs | 32 ++++++++++++- cli/src/merge_tools/mod.rs | 12 +++++ cli/tests/test_resolve_command.rs | 74 +++++++++++++++++++++++++++++++ docs/config.md | 16 +++++-- 6 files changed, 138 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3892782529..29b890fe4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * New `merge-tools..conflict-marker-style` config option to override the conflict marker style used for a specific merge tool. +* New `merge-tools..merge-tool-error-status-on-conflict` config option to + allow a merge tool to exit with a status between 1 and 127 to indicate that + not all conflicts were resolved. + * `jj simplify-parents` now supports configuring the default revset when no `--source` or `--revisions` arguments are provided with the `revsets.simplify-parents` config. diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 3d1202463c..87c9449872 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -400,6 +400,11 @@ "type": "string" } }, + "merge-tool-error-status-on-conflict": { + "type": "boolean", + "description": "If true, an exit status between 1 and 127 indicates that the conflict was only partially resolved. See https://martinvonz.github.io/jj/latest/config/#editing-conflict-markers-with-a-tool-or-a-text-editor", + "default": false + }, "merge-tool-edits-conflict-markers": { "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", diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 8f7689b737..78db125662 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -58,6 +58,11 @@ pub struct ExternalMergeTool { /// `$left`, `$right`, `$base`, and `$output` are replaced with /// paths to the corresponding files. pub merge_args: Vec, + /// If false (default), any exit status from the merge tool other than 0 + /// will cause `jj resolve` to fail. If true, then any status between 1 and + /// 127 can be used to indicate that the conflict was not fully resolved, + /// and the output file should contain conflict markers. + pub merge_tool_error_status_on_conflict: bool, /// 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 @@ -93,6 +98,7 @@ impl Default for ExternalMergeTool { diff_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), merge_args: vec![], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, diff_invocation_mode: DiffToolMode::Dir, @@ -150,6 +156,11 @@ pub enum ExternalToolError { }, #[error("Tool exited with {exit_status} (run with --debug to see the exact invocation)")] ToolAborted { exit_status: ExitStatus }, + #[error( + "Tool exited with {exit_status}, but did not produce valid conflict markers (run with \ + --debug to see the exact invocation)" + )] + InvalidConflictMarkers { exit_status: ExitStatus }, #[error("I/O error")] Io(#[source] std::io::Error), } @@ -218,7 +229,13 @@ pub fn run_mergetool_external( tool_binary: editor.program.clone(), source: e, })?; - if !exit_status.success() { + + // Check whether the exit status implies that there should be conflict markers + let exit_status_implies_conflict = editor.merge_tool_error_status_on_conflict + && !exit_status.success() + && exit_status.code().is_some_and(|code| code <= 127); + + if !exit_status.success() && !exit_status_implies_conflict { return Err(ConflictResolveError::from(ExternalToolError::ToolAborted { exit_status, })); @@ -230,7 +247,7 @@ pub fn run_mergetool_external( return Err(ConflictResolveError::EmptyOrUnchanged); } - let new_file_ids = if editor.merge_tool_edits_conflict_markers { + let new_file_ids = if editor.merge_tool_edits_conflict_markers || exit_status_implies_conflict { conflicts::update_from_content( &file_merge, tree.store(), @@ -246,6 +263,17 @@ pub fn run_mergetool_external( .block_on()?; Merge::normal(new_file_id) }; + + // If the exit status indicated there should be conflict markers but there + // weren't any, it's likely that the tool generated incorrect conflict markers, + // so we need to inform the user. If we didn't treat this as an error, the user + // might think the conflict was resolved successfully. + if exit_status_implies_conflict && new_file_ids.is_resolved() { + return Err(ConflictResolveError::ExternalTool( + ExternalToolError::InvalidConflictMarkers { exit_status }, + )); + } + let new_tree_value = match new_file_ids.into_resolved() { Ok(new_file_id) => Merge::normal(TreeValue::File { id: new_file_id.unwrap(), diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index d7be4f55fa..2b279f2ed0 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -401,6 +401,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -429,6 +430,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -469,6 +471,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -493,6 +496,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -516,6 +520,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -545,6 +550,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -572,6 +578,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -593,6 +600,7 @@ mod tests { "$right", ], merge_args: [], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -646,6 +654,7 @@ mod tests { "$right", "$output", ], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -694,6 +703,7 @@ mod tests { "$right", "$output", ], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -723,6 +733,7 @@ mod tests { "$right", "$output", ], + merge_tool_error_status_on_conflict: false, merge_tool_edits_conflict_markers: false, conflict_marker_style: None, }, @@ -755,6 +766,7 @@ mod tests { "$right", "$output", ], + merge_tool_error_status_on_conflict: false, 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 7dd27a441f..58b4980483 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -432,6 +432,80 @@ fn test_resolution() { file 2-sided conflict "###); + // Check that merge tool can leave conflict markers by returning exit code 1 + // when using `merge-tool-error-status-on-conflict = true`. The Git "diff3" + // conflict markers should also be parsed correctly. + 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 editor5", + indoc! {" + write + <<<<<<< + some + ||||||| + fake + ======= + conflict + >>>>>>> + "}, + "fail", + ] + .join("\0"), + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "resolve", + "--config-toml", + "merge-tools.fake-editor.merge-tool-error-status-on-conflict = true", + ], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Resolving conflicts in: file + Working copy now at: vruxwmqv 6690cdf4 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 6690cdf4 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("editor5")).unwrap(), @""); + 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 9d7f6b335c..c43d3a5344 100644 --- a/docs/config.md +++ b/docs/config.md @@ -841,9 +841,19 @@ merge-tool-edits-conflict-markers = true # See below for an explanation ### Editing conflict markers with a tool or a text editor By default, the merge tool starts with an empty output file. If the tool puts -anything into the output file, and exits with the 0 exit code, -`jj` assumes that the conflict is fully resolved. This is appropriate for most -graphical merge tools. +anything into the output file and exits with the 0 exit code, +`jj` assumes that the conflict is fully resolved, while if the tool exits with +a non-zero exit code, `jj` assumes that the merge should be cancelled. +This is appropriate for most graphical merge tools. + +For merge tools which try to automatically resolve conflicts without user +input, this behavior may not be desired. These tools may require setting +the `merge-tools.TOOL.merge-tool-error-status-on-conflict = true` option. +With this option set, an exit code between 1 and 127 indicates that only some +of the conflicts were resolved, and the output file should contain conflict +markers. If a merge tool produces output using Git's "diff3" conflict style, +`jj` should be able to parse it correctly, so many Git merge drivers should be +usable with `jj` as well. Some tools (e.g. `vimdiff`) can present a multi-way diff but don't resolve conflict themselves. When using such tools, `jj`