merge_tools: add merge-tool-conflict-exit-codes option

Some Git merge drivers can partially resolve conflicts, leaving some conflict markers in the output file. In that case, they exit with a code between 1 and 127 to indicate that there was a conflict remaining in the file (since Git uses a shell to run the merge drivers, and shells often use exit codes above 127 for special meanings such as exiting due to a signal): https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver We should support this convention as well, since it will allow many Git merge drivers to be used with Jujutsu, but we don't run our merge tools through a shell, so there is no reason to treat exit codes 1 through 127 specially. Instead, we let the user specify which exact exit codes should indicate conflicts. This is also better for cross-platform support, since Windows may use different exit codes than Linux for instance.
martinvonz · Dec 3, 2024 · cc19790 · cc19790
1 parent 94c6d6e
commit cc19790
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -75,6 +75,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * New `merge-tools.<TOOL>.conflict-marker-style` config option to override the
   conflict marker style used for a specific merge tool.
 
+* New `merge-tools.<TOOL>.merge-tool-conflict-exit-codes` config option to
+  allow a merge tool to exit with a non-zero code 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
@@ -400,6 +400,14 @@
                             "type": "string"
                         }
                     },
+                    "merge-tool-conflict-exit-codes": {
+                        "type": "array",
+                        "items": {
+                            "type": "number"
+                        },
+                        "description": "Array of exit codes to indicate 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": []
+                    },
                     "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
@@ -58,6 +58,14 @@ pub struct ExternalMergeTool {
     /// `$left`, `$right`, `$base`, and `$output` are replaced with
     /// paths to the corresponding files.
     pub merge_args: Vec<String>,
+    /// By default, if a merge tool exits with a non-zero exit code, then the
+    /// merge will be cancelled. Some merge tools allow leaving some conflicts
+    /// unresolved, in which case they will be left as conflict markers in the
+    /// output file. In that case, the merge tool may exit with a non-zero exit
+    /// code to indicate that not all conflicts were resolved. Adding an exit
+    /// code to this array will tell `jj` to interpret that exit code as
+    /// indicating that the `$output` file should contain conflict markers.
+    pub merge_tool_conflict_exit_codes: Vec<i32>,
     /// 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 +101,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_conflict_exit_codes: vec![],
             merge_tool_edits_conflict_markers: false,
             conflict_marker_style: None,
             diff_invocation_mode: DiffToolMode::Dir,
@@ -150,6 +159,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 +232,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 = exit_status
+        .code()
+        .is_some_and(|code| editor.merge_tool_conflict_exit_codes.contains(&code));
+
+    if !exit_status.success() && !exit_status_implies_conflict {
         return Err(ConflictResolveError::from(ExternalToolError::ToolAborted {
             exit_status,
         }));
@@ -230,7 +250,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 +266,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 invalid 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
@@ -405,6 +405,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -433,6 +434,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -473,6 +475,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -497,6 +500,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -520,6 +524,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -549,6 +554,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -576,6 +582,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -597,6 +604,7 @@ mod tests {
                     "$right",
                 ],
                 merge_args: [],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -650,6 +658,7 @@ mod tests {
                     "$right",
                     "$output",
                 ],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -698,6 +707,7 @@ mod tests {
                     "$right",
                     "$output",
                 ],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -727,6 +737,7 @@ mod tests {
                     "$right",
                     "$output",
                 ],
+                merge_tool_conflict_exit_codes: [],
                 merge_tool_edits_conflict_markers: false,
                 conflict_marker_style: None,
             },
@@ -759,6 +770,7 @@ mod tests {
                     "$right",
                     "$output",
                 ],
+                merge_tool_conflict_exit_codes: [],
                 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
@@ -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-conflict-exit-codes = [1]`. 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-conflict-exit-codes = [1]",
+        ],
+    );
+    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
@@ -841,9 +841,20 @@ 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. For instance, some automatic merge tools use
+an exit code of 1 to indicate that some conflicts were unable to be resolved and
+that the output file should contain conflict markers. In that case, you could
+set the config option `merge-tools.TOOL.merge-tool-conflict-exit-codes = [1]` to
+tell `jj` to expect conflict markers in the output file if the exit code is 1.
+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`