Skip to content

Commit

Permalink
merge_tools: add merge-tool-error-status-on-conflict
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@scott2000
scott2000 committed Nov 26, 2024
1 parent 7f57866 commit a042e13
Show file tree
Hide file tree
Showing 6 changed files with 138 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,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-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.
Expand Down
5 changes: 5 additions & 0 deletions cli/src/config-schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
32 changes: 30 additions & 2 deletions cli/src/merge_tools/external.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ pub struct ExternalMergeTool {
/// `$left`, `$right`, `$base`, and `$output` are replaced with
/// paths to the corresponding files.
pub merge_args: Vec<String>,
/// 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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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,
}));
Expand All @@ -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(),
Expand All @@ -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(),
Expand Down
12 changes: 12 additions & 0 deletions cli/src/merge_tools/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down
74 changes: 74 additions & 0 deletions cli/tests/test_resolve_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand Down
16 changes: 13 additions & 3 deletions docs/config.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down

0 comments on commit a042e13

Please sign in to comment.