Skip to content

Commit

Permalink
cli: load default diff command from "ui.diff.tool" config
Browse files Browse the repository at this point in the history
This could be a "ui.diff.format" variant, but we would need to solve namespace
problem by prefixing "tool:<name>" for example. Then, inlining command arguments
would become uglier.

#1886
  • Loading branch information
yuja committed Aug 4, 2023
1 parent 962cd25 commit b352061
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 10 deletions.
15 changes: 12 additions & 3 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,18 @@ ui.diff.format = "git"

### Generating diffs by external command

If `diff --tool <name>` argument is given, the external diff command will be
called instead of the internal diff function. The command arguments can be
specified as follows.
If `ui.diff.tool` is set, the specified diff command will be called instead of
the internal diff function.

```toml
# Use Difftastic by default
ui.diff.tool = ["difft", "--color=always", "$left", "$right"]
# Use tool named "<name>" (see below)
ui.diff.tool = "<name>"
```

The external diff tool can also be enabled by `diff --tool <name>` argument.
For the tool named `<name>`, command arguments can be configured as follows.

```toml
[merge-tools.<name>]
Expand Down
4 changes: 4 additions & 0 deletions src/config-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@
"summary"
],
"default": "color-words"
},
"tool": {
"type": "string",
"description": "External tool for generating diffs"
}
}
},
Expand Down
7 changes: 6 additions & 1 deletion src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ fn diff_formats_from_args(

fn default_diff_format(settings: &UserSettings) -> Result<DiffFormat, config::ConfigError> {
let config = settings.config();
if let Some(args) = config.get("ui.diff.tool").optional()? {
// External "tool" overrides the internal "format" option.
let tool = merge_tools::get_tool_config_from_args(settings, &args)?
.unwrap_or_else(|| MergeTool::with_diff_args(&args));
return Ok(DiffFormat::Tool(Box::new(tool)));
}
let name = if let Some(name) = config.get_string("ui.diff.format").optional()? {
name
} else if let Some(name) = config.get_string("diff.format").optional()? {
Expand All @@ -134,7 +140,6 @@ fn default_diff_format(settings: &UserSettings) -> Result<DiffFormat, config::Co
"types" => Ok(DiffFormat::Types),
"git" => Ok(DiffFormat::Git),
"color-words" => Ok(DiffFormat::ColorWords),
// TODO: add configurable default for DiffFormat::Tool?
_ => Err(config::ConfigError::Message(format!(
"invalid diff format: {name}"
))),
Expand Down
6 changes: 5 additions & 1 deletion src/merge_tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,10 @@ impl MergeTool {
}
}

pub fn with_diff_args(command_args: &CommandNameAndArgs) -> Self {
Self::with_args_inner(command_args, |tool| &mut tool.diff_args)
}

pub fn with_edit_args(command_args: &CommandNameAndArgs) -> Self {
Self::with_args_inner(command_args, |tool| &mut tool.edit_args)
}
Expand Down Expand Up @@ -558,7 +562,7 @@ pub fn get_tool_config(

/// Loads merge tool options from `[merge-tools.<name>]` if `args` is of
/// unstructured string type.
fn get_tool_config_from_args(
pub fn get_tool_config_from_args(
settings: &UserSettings,
args: &CommandNameAndArgs,
) -> Result<Option<MergeTool>, ConfigError> {
Expand Down
14 changes: 9 additions & 5 deletions tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,11 +250,7 @@ impl TestEnvironment {
/// Sets up the fake diff-editor to read an edit script from the returned
/// path
pub fn set_up_fake_diff_editor(&mut self) -> PathBuf {
let diff_editor_path = assert_cmd::cargo::cargo_bin("fake-diff-editor");
assert!(diff_editor_path.is_file());
// Simplified TOML escaping, hoping that there are no '"' or control characters
// in it
let escaped_diff_editor_path = diff_editor_path.to_str().unwrap().replace('\\', r"\\");
let escaped_diff_editor_path = escaped_fake_diff_editor_path();
self.add_config(&format!(
r###"
ui.diff-editor = "fake-diff-editor"
Expand Down Expand Up @@ -301,3 +297,11 @@ pub fn get_stdout_string(assert: &assert_cmd::assert::Assert) -> String {
pub fn get_stderr_string(assert: &assert_cmd::assert::Assert) -> String {
String::from_utf8(assert.get_output().stderr.clone()).unwrap()
}

pub fn escaped_fake_diff_editor_path() -> String {
let diff_editor_path = assert_cmd::cargo::cargo_bin("fake-diff-editor");
assert!(diff_editor_path.is_file());
// Simplified TOML escaping, hoping that there are no '"' or control characters
// in it
diff_editor_path.to_str().unwrap().replace('\\', r"\\")
}
21 changes: 21 additions & 0 deletions tests/test_diff_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,27 @@ fn test_diff_external_tool() {
file3
"###);

// Enabled by default, looks up the merge-tools table
let config = "--config-toml=ui.diff.tool='fake-diff-editor'";
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", config]), @r###"
file1
file2
--
file2
file3
"###);

// Inlined command arguments
let command = common::escaped_fake_diff_editor_path();
let config = format!(r#"--config-toml=ui.diff.tool=["{command}", "$right", "$left"]"#);
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", &config]), @r###"
file2
file3
--
file1
file2
"###);

// Output of external diff tool shouldn't be escaped
std::fs::write(&edit_script, "print \x1b[1;31mred").unwrap();
insta::assert_snapshot!(
Expand Down

0 comments on commit b352061

Please sign in to comment.