From b1b2c62c3eb449ff3cca4b865f56dc59ed1f5f78 Mon Sep 17 00:00:00 2001 From: Bryce Berger Date: Fri, 3 Jan 2025 14:12:35 -0500 Subject: [PATCH] diff: add `merge-tools.*.diff-expected-exit-codes` Certain tools (`diff`, `delta`) exit with code 1 to indicate there was a difference. This allows selectively suppressing the "Tool exited with ... status" warning from jj when generating a diff. example: ```toml [merge.tools.delta] diff-expected-exit-codes = [0, 1] ``` --- CHANGELOG.md | 3 ++ cli/src/config-schema.json | 8 ++++ cli/src/merge_tools/external.rs | 8 +++- cli/src/merge_tools/mod.rs | 84 +++++++++++++++++++++++---------- cli/tests/test_diff_command.rs | 30 ++++++++++++ docs/config.md | 9 ++++ 6 files changed, 117 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e0fc4ab7..6fa5522af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj status` now shows untracked files when they reside directly under a tracked directory. +* New `merge-tools..diff-expected-exit-codes` config option to suppress + warnings from tools exiting with non-zero exit codes. + ### Fixed bugs * Fixed diff selection by external tools with `jj split`/`commit -i FILESETS`. diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 8a02e7116..edaf1e8a2 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -380,6 +380,14 @@ "type": "string" } }, + "diff-expected-exit-codes": { + "type": "array", + "items": { + "type": "integer" + }, + "description": "Array of exit codes that do not indicate tool failure, i.e. [0, 1] for unix diff.", + "default": [0] + }, "diff-invocation-mode": { "description": "Invoke the tool with directories or individual files", "enum": [ diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 2b50fccc8..17457c378 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -50,6 +50,8 @@ pub struct ExternalMergeTool { /// Arguments to pass to the program when generating diffs. /// `$left` and `$right` are replaced with the corresponding directories. pub diff_args: Vec, + /// Exit codes to be treated as success when generating diffs. + pub diff_expected_exit_codes: Vec, /// Whether to execute the tool with a pair of directories or individual /// files. pub diff_invocation_mode: DiffToolMode, @@ -101,6 +103,7 @@ impl Default for ExternalMergeTool { // `edit-args = false`, or `edit-args = []`, or `edit = { disabled = // true }` to go with `edit = { args = [...] }`. diff_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), + diff_expected_exit_codes: vec![0], edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), merge_args: vec![], merge_conflict_exit_codes: vec![], @@ -411,7 +414,10 @@ pub fn invoke_external_diff( // will exit with 1 if inputs are different. let exit_status = child.wait().map_err(ExternalToolError::Io)?; tracing::info!(?cmd, ?exit_status, "The external diff generator exited:"); - if !exit_status.success() { + let exit_ok = exit_status + .code() + .is_some_and(|status| tool.diff_expected_exit_codes.contains(&status)); + if !exit_ok { writeln!( ui.warning_default(), "Tool exited with {exit_status} (run with --debug to see the exact invocation)", diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 0b5517287..49531bb2a 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -393,7 +393,7 @@ mod tests { insta::assert_debug_snapshot!(get(":builtin", "").unwrap(), @"Builtin"); // Just program name, edit_args are filled by default - insta::assert_debug_snapshot!(get("my diff", "").unwrap(), @r###" + insta::assert_debug_snapshot!(get("my diff", "").unwrap(), @r#" External( ExternalMergeTool { program: "my diff", @@ -401,6 +401,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -412,7 +415,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // Pick from merge-tools insta::assert_debug_snapshot!(get( @@ -420,7 +423,7 @@ mod tests { [merge-tools."foo bar"] edit-args = ["--edit", "args", "$left", "$right"] "#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "foo bar", @@ -428,6 +431,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "--edit", @@ -441,7 +447,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); } #[test] @@ -463,7 +469,7 @@ mod tests { insta::assert_debug_snapshot!(get("").unwrap(), @"Builtin"); // Just program name, edit_args are filled by default - insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r###" + insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r#" External( ExternalMergeTool { program: "my-diff", @@ -471,6 +477,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -482,11 +491,11 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // String args (with interpolation variables) insta::assert_debug_snapshot!( - get(r#"ui.diff-editor = "my-diff -l $left -r $right""#).unwrap(), @r###" + get(r#"ui.diff-editor = "my-diff -l $left -r $right""#).unwrap(), @r#" External( ExternalMergeTool { program: "my-diff", @@ -494,6 +503,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "-l", @@ -507,11 +519,11 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // List args (with interpolation variables) insta::assert_debug_snapshot!( - get(r#"ui.diff-editor = ["my-diff", "--diff", "$left", "$right"]"#).unwrap(), @r###" + get(r#"ui.diff-editor = ["my-diff", "--diff", "$left", "$right"]"#).unwrap(), @r#" External( ExternalMergeTool { program: "my-diff", @@ -519,6 +531,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "--diff", @@ -531,7 +546,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // Pick from merge-tools insta::assert_debug_snapshot!(get( @@ -540,7 +555,7 @@ mod tests { [merge-tools."foo bar"] edit-args = ["--edit", "args", "$left", "$right"] "#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "foo bar", @@ -548,6 +563,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "--edit", @@ -561,7 +579,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // Pick from merge-tools, but no edit-args specified insta::assert_debug_snapshot!(get( @@ -570,7 +588,7 @@ mod tests { [merge-tools.my-diff] program = "MyDiff" "#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "MyDiff", @@ -578,6 +596,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -589,10 +610,10 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // List args should never be a merge-tools key, edit_args are filled by default - insta::assert_debug_snapshot!(get(r#"ui.diff-editor = ["meld"]"#).unwrap(), @r###" + insta::assert_debug_snapshot!(get(r#"ui.diff-editor = ["meld"]"#).unwrap(), @r#" External( ExternalMergeTool { program: "meld", @@ -600,6 +621,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -611,7 +635,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // Invalid type assert!(get(r#"ui.diff-editor.k = 0"#).is_err()); @@ -641,7 +665,7 @@ mod tests { [merge-tools."foo bar"] merge-args = ["$base", "$left", "$right", "$output"] "#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "foo bar", @@ -649,6 +673,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -665,7 +692,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); } #[test] @@ -690,7 +717,7 @@ mod tests { // String args insta::assert_debug_snapshot!( - get(r#"ui.merge-editor = "my-merge $left $base $right $output""#).unwrap(), @r###" + get(r#"ui.merge-editor = "my-merge $left $base $right $output""#).unwrap(), @r#" External( ExternalMergeTool { program: "my-merge", @@ -698,6 +725,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -714,13 +744,13 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // List args insta::assert_debug_snapshot!( get( r#"ui.merge-editor = ["my-merge", "$left", "$base", "$right", "$output"]"#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "my-merge", @@ -728,6 +758,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -744,7 +777,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // Pick from merge-tools insta::assert_debug_snapshot!(get( @@ -753,7 +786,7 @@ mod tests { [merge-tools."foo bar"] merge-args = ["$base", "$left", "$right", "$output"] "#, - ).unwrap(), @r###" + ).unwrap(), @r#" External( ExternalMergeTool { program: "foo bar", @@ -761,6 +794,9 @@ mod tests { "$left", "$right", ], + diff_expected_exit_codes: [ + 0, + ], diff_invocation_mode: Dir, edit_args: [ "$left", @@ -777,7 +813,7 @@ mod tests { conflict_marker_style: None, }, ) - "###); + "#); // List args should never be a merge-tools key insta::assert_debug_snapshot!( diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 0c07eeaeb..c92007573 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -2042,6 +2042,36 @@ fn test_diff_external_tool() { std::fs::write(repo_path.join("file3"), "foo\n").unwrap(); let edit_script = test_env.set_up_fake_diff_editor(); + + // nonzero exit codes should print a warning + std::fs::write(&edit_script, "fail").unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["diff", "--config=ui.diff.tool=fake-diff-editor"], + ); + let mut insta_settings = insta::Settings::clone_current(); + insta_settings.add_filter("exit (status|code)", ""); + insta_settings.bind(|| { + insta::assert_snapshot!(stdout, @r""); + insta::assert_snapshot!(stderr, @r#" + Warning: Tool exited with : 1 (run with --debug to see the exact invocation) + "#); + }); + + // nonzero exit codes should not print a warning if it's an expected exit code + std::fs::write(&edit_script, "fail").unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "diff", + "--tool", + "fake-diff-editor", + "--config=merge-tools.fake-diff-editor.diff-expected-exit-codes=[1]", + ], + ); + insta::assert_snapshot!(stdout, @r""); + insta::assert_snapshot!(stderr, @r""); + std::fs::write( &edit_script, "print-files-before\0print --\0print-files-after", diff --git a/docs/config.md b/docs/config.md index 22828d5f5..00e6c2052 100644 --- a/docs/config.md +++ b/docs/config.md @@ -286,6 +286,15 @@ diff.tool = "vimdiff" diff-invocation-mode = "file-by-file" ``` +By default `jj` will display a warning when the command exits with a non-success +error code. The `diff-expected-exit-codes` config can suppress this warning +message for specific exit codes: + +```toml +[merge-tools.delta] +diff-expected-exit-codes = [0, 1] +``` + ### Conflict marker style You can configure which style of conflict markers to use when materializing