martinvonz/jj
1
1
Fork 0
mirror of https://github.com/martinvonz/jj.git synced 2024-11-10 05:44:29 +03:00
Code Issues Packages Projects Releases Wiki Activity

cli: add option to generate textual diff by external command

Browse Source 
This is basic implementation. There's no config knob to enable the external
diff command by default. It reuses the merge-tools table because that's how
external diff/merge commands are currently configured. We might want to
reorganize them in #1285.

If you run "jj diff --tool meld", GUI diff will open and jj will wait for
meld to quit. This also applies to "jj log -p". The "diff --tool gui" behavior
is somewhat useful, but "log -p --tool gui" wouldn't. We might want some flag
to mark the tool output can't be streamed.

Another thing to consider is tools that can't generate directory diffs. Git
executes ext-diff tool per file, but we don't. Difftastic can compare
directories, and doing that should be more efficient since diffs can be
computed in parallel (at the expense of unsorted output.)

Closes #1886
This commit is contained in:
Yuya Nishihara 2023-08-02 12:12:11 +09:00
parent 65b06e92ed
commit 0b9d23c3ad
9 changed files with 276 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
CHANGELOG.md
View File

@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
from a merge revision's parents. This undoes the changes that `jj diff -r`
would show.
* `jj diff`/`log` now supports `--tool <name>` option to generate diffs by
external program. For configuration, see [the documentation](docs/config.md).
[#1886](https://github.com/martinvonz/jj/issues/1886)
### Fixed bugs

15
docs/config.md
View File

@ -141,6 +141,21 @@ ui.default-command = "log"
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.
```toml
[merge-tools.<name>]
# program = "<name>" # Defaults to the name of the tool if not specified
diff-args = ["--color=always", "$left", "$right"]
```
- `$left` and `$right` are replaced with the paths to the left and right
directories to diff respectively.
### Default revisions to log
You can configure the revisions `jj log` without `-r` should show.

8
src/cli_util.rs
View File

@ -72,7 +72,7 @@ use crate::config::{
new_config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs,
};
use crate::formatter::{FormatRecorder, Formatter, PlainTextFormatter};
use crate::merge_tools::{ConflictResolveError, DiffEditError};
use crate::merge_tools::{ConflictResolveError, DiffEditError, DiffGenerateError};
use crate::template_parser::{TemplateAliasesMap, TemplateParseError};
use crate::templater::Template;
use crate::ui::{ColorChoice, Ui};
@ -239,6 +239,12 @@ impl From<DiffEditError> for CommandError {
}
}
impl From<DiffGenerateError> for CommandError {
fn from(err: DiffGenerateError) -> Self {
user_error(format!("Failed to generate diff: {err}"))
}
}
impl From<ConflictResolveError> for CommandError {
fn from(err: ConflictResolveError) -> Self {
user_error(format!("Failed to use external tool to resolve: {err}"))

6
src/config-schema.json
View File

@ -242,6 +242,12 @@
"program": {
"type": "string"
},
"diff-args": {
"type": "array",
"items": {
"type": "string"
}
},
"edit-args": {
"type": "array",
"items": {

37
src/diff_util.rs
View File

@ -32,10 +32,11 @@ use tracing::instrument;
use crate::cli_util::{CommandError, WorkspaceCommandHelper};
use crate::formatter::Formatter;
use crate::merge_tools::{self, MergeTool};
#[derive(clap::Args, Clone, Debug)]
#[command(group(clap::ArgGroup::new("short-format").args(&["summary", "types"])))]
#[command(group(clap::ArgGroup::new("long-format").args(&["git", "color_words"])))]
#[command(group(clap::ArgGroup::new("long-format").args(&["git", "color_words", "tool"])))]
pub struct DiffFormatArgs {
/// For each path, show only whether it was modified, added, or removed
#[arg(long, short)]
@ -55,14 +56,18 @@ pub struct DiffFormatArgs {
/// Show a word-level diff with changes indicated only by color
#[arg(long)]
pub color_words: bool,
/// Generate diff by external command
#[arg(long)]
pub tool: Option<String>,
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum DiffFormat {
Summary,
Types,
Git,
ColorWords,
Tool(Box<MergeTool>),
}
/// Returns a list of requested diff formats, which will never be empty.
@ -70,7 +75,7 @@ pub fn diff_formats_for(
settings: &UserSettings,
args: &DiffFormatArgs,
) -> Result<Vec<DiffFormat>, config::ConfigError> {
let formats = diff_formats_from_args(args);
let formats = diff_formats_from_args(settings, args)?;
if formats.is_empty() {
Ok(vec![default_diff_format(settings)?])
} else {
@ -85,7 +90,7 @@ pub fn diff_formats_for_log(
args: &DiffFormatArgs,
patch: bool,
) -> Result<Vec<DiffFormat>, config::ConfigError> {
let mut formats = diff_formats_from_args(args);
let mut formats = diff_formats_from_args(settings, args)?;
// --patch implies default if no format other than --summary is specified
if patch && matches!(formats.as_slice(), [] | [DiffFormat::Summary]) {
formats.push(default_diff_format(settings)?);
@ -94,8 +99,11 @@ pub fn diff_formats_for_log(
Ok(formats)
}
fn diff_formats_from_args(args: &DiffFormatArgs) -> Vec<DiffFormat> {
[
fn diff_formats_from_args(
settings: &UserSettings,
args: &DiffFormatArgs,
) -> Result<Vec<DiffFormat>, config::ConfigError> {
let mut formats = [
(args.summary, DiffFormat::Summary),
(args.types, DiffFormat::Types),
(args.git, DiffFormat::Git),
@ -103,7 +111,13 @@ fn diff_formats_from_args(args: &DiffFormatArgs) -> Vec<DiffFormat> {
]
.into_iter()
.filter_map(|(arg, format)| arg.then_some(format))
.collect()
.collect_vec();
if let Some(name) = &args.tool {
let tool = merge_tools::get_tool_config(settings, name)?
.unwrap_or_else(|| MergeTool::with_program(name));
formats.push(DiffFormat::Tool(Box::new(tool)));
}
Ok(formats)
}
fn default_diff_format(settings: &UserSettings) -> Result<DiffFormat, config::ConfigError> {
@ -120,6 +134,7 @@ 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}"
))),
@ -135,20 +150,26 @@ pub fn show_diff(
formats: &[DiffFormat],
) -> Result<(), CommandError> {
for format in formats {
let tree_diff = from_tree.diff(to_tree, matcher);
match format {
DiffFormat::Summary => {
let tree_diff = from_tree.diff(to_tree, matcher);
show_diff_summary(formatter, workspace_command, tree_diff)?;
}
DiffFormat::Types => {
let tree_diff = from_tree.diff(to_tree, matcher);
show_types(formatter, workspace_command, tree_diff)?;
}
DiffFormat::Git => {
let tree_diff = from_tree.diff(to_tree, matcher);
show_git_diff(formatter, workspace_command, tree_diff)?;
}
DiffFormat::ColorWords => {
let tree_diff = from_tree.diff(to_tree, matcher);
show_color_words_diff(formatter, workspace_command, tree_diff)?;
}
DiffFormat::Tool(tool) => {
merge_tools::generate_diff(formatter.raw(), from_tree, to_tree, matcher, tool)?;
}
}
}
Ok(())

106
src/merge_tools.rs
View File

@ -16,7 +16,7 @@ use std::collections::HashMap;
use std::fs::File;
use std::io::{self, Write};
use std::path::{Path, PathBuf};
use std::process::Command;
use std::process::{Command, Stdio};
use std::sync::Arc;
use config::ConfigError;
@ -94,6 +94,14 @@ pub enum DiffEditError {
Config(#[from] config::ConfigError),
}
#[derive(Debug, Error)]
pub enum DiffGenerateError {
#[error(transparent)]
ExternalTool(#[from] ExternalToolError),
#[error(transparent)]
DiffCheckoutError(#[from] DiffCheckoutError),
}
#[derive(Debug, Error)]
pub enum ConflictResolveError {
#[error(transparent)]
@ -420,13 +428,48 @@ pub fn edit_diff(
Ok(right_tree_state.current_tree_id().clone())
}
/// Generates textual diff by the specified `tool`, and writes into `writer`.
pub fn generate_diff(
writer: &mut dyn Write,
left_tree: &Tree,
right_tree: &Tree,
matcher: &dyn Matcher,
tool: &MergeTool,
) -> Result<(), DiffGenerateError> {
let store = left_tree.store();
let diff_wc = check_out_trees(store, left_tree, right_tree, matcher)?;
// TODO: Add support for tools without directory diff functionality?
// TODO: Somehow propagate --color to the external command?
let patterns = diff_wc.to_command_variables();
let mut cmd = Command::new(&tool.program);
cmd.args(interpolate_variables(&tool.diff_args, &patterns));
tracing::info!(?cmd, "Invoking the external diff generator:");
let mut child = cmd
.stdin(Stdio::null())
.stdout(Stdio::piped())
.spawn()
.map_err(|source| ExternalToolError::FailedToExecute {
tool_binary: tool.program.clone(),
source,
})?;
io::copy(&mut child.stdout.take().unwrap(), writer).map_err(ExternalToolError::Io)?;
// Non-zero exit code isn't an error. For example, the traditional diff command
// 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:");
Ok(())
}
/// Merge/diff tool loaded from the settings.
#[derive(Clone, Debug, serde::Deserialize)]
#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)]
#[serde(default, rename_all = "kebab-case")]
struct MergeTool {
pub struct MergeTool {
/// Program to execute. Must be defined; defaults to the tool name
/// if not specified in the config.
pub program: String,
/// Arguments to pass to the program when generating diffs.
/// `$left` and `$right` are replaced with the corresponding directories.
pub diff_args: Vec<String>,
/// Arguments to pass to the program when editing diffs.
/// `$left` and `$right` are replaced with the corresponding directories.
pub edit_args: Vec<String>,
@ -450,6 +493,7 @@ impl Default for MergeTool {
fn default() -> Self {
MergeTool {
program: String::new(),
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_edits_conflict_markers: false,
@ -458,6 +502,13 @@ impl Default for MergeTool {
}
impl MergeTool {
pub fn with_program(program: impl Into<String>) -> Self {
MergeTool {
program: program.into(),
..Default::default()
}
}
pub fn with_edit_args(command_args: &CommandNameAndArgs) -> Self {
let (name, args) = command_args.split_name_and_args();
let mut tool = MergeTool {
@ -484,7 +535,10 @@ impl MergeTool {
}
/// Loads merge tool options from `[merge-tools.<name>]`.
fn get_tool_config(settings: &UserSettings, name: &str) -> Result<Option<MergeTool>, ConfigError> {
pub fn get_tool_config(
settings: &UserSettings,
name: &str,
) -> Result<Option<MergeTool>, ConfigError> {
const TABLE_KEY: &str = "merge-tools";
let tools_table = settings.config().get_table(TABLE_KEY)?;
if let Some(v) = tools_table.get(name) {
@ -583,6 +637,10 @@ mod tests {
insta::assert_debug_snapshot!(get("").unwrap(), @r###"
MergeTool {
program: "meld",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -603,6 +661,10 @@ mod tests {
insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r###"
MergeTool {
program: "my-diff",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -617,6 +679,10 @@ mod tests {
get(r#"ui.diff-editor = "my-diff -l $left -r $right""#).unwrap(), @r###"
MergeTool {
program: "my-diff",
diff_args: [
"$left",
"$right",
],
edit_args: [
"-l",
"$left",
@ -633,6 +699,10 @@ mod tests {
get(r#"ui.diff-editor = ["my-diff", "--diff", "$left", "$right"]"#).unwrap(), @r###"
MergeTool {
program: "my-diff",
diff_args: [
"$left",
"$right",
],
edit_args: [
"--diff",
"$left",
@ -653,6 +723,10 @@ mod tests {
).unwrap(), @r###"
MergeTool {
program: "foo bar",
diff_args: [
"$left",
"$right",
],
edit_args: [
"--edit",
"args",
@ -674,6 +748,10 @@ mod tests {
).unwrap(), @r###"
MergeTool {
program: "MyDiff",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -687,6 +765,10 @@ mod tests {
insta::assert_debug_snapshot!(get(r#"ui.diff-editor = ["meld"]"#).unwrap(), @r###"
MergeTool {
program: "meld",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -713,6 +795,10 @@ mod tests {
insta::assert_debug_snapshot!(get("").unwrap(), @r###"
MergeTool {
program: "meld",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -741,6 +827,10 @@ mod tests {
get(r#"ui.merge-editor = "my-merge $left $base $right $output""#).unwrap(), @r###"
MergeTool {
program: "my-merge",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -762,6 +852,10 @@ mod tests {
).unwrap(), @r###"
MergeTool {
program: "my-merge",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
@ -786,6 +880,10 @@ mod tests {
).unwrap(), @r###"
MergeTool {
program: "foo bar",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",

13
testing/fake-diff-editor.rs
View File

@ -80,6 +80,19 @@ fn main() {
exit(1)
}
}
["print", message] => {
println!("{message}");
}
["print-files-before"] => {
for base_name in files_recursively(&args.before).iter().sorted() {
println!("{base_name}");
}
}
["print-files-after"] => {
for base_name in files_recursively(&args.after).iter().sorted() {
println!("{base_name}");
}
}
["rm", file] => {
std::fs::remove_file(args.after.join(file)).unwrap();
}

7
tests/common/mod.rs
View File

@ -255,7 +255,12 @@ impl TestEnvironment {
// 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"\\");
self.add_config(&format!(r#"ui.diff-editor = "{escaped_diff_editor_path}""#));
self.add_config(&format!(
r###"
ui.diff-editor = "fake-diff-editor"
merge-tools.fake-diff-editor.program = "{escaped_diff_editor_path}"
"###
));
let edit_script = self.env_root().join("diff_edit_script");
std::fs::write(&edit_script, "").unwrap();
self.add_env_var("DIFF_EDIT_SCRIPT", edit_script.to_str().unwrap());

95
tests/test_diff_command.rs
View File

@ -574,3 +574,98 @@ fn test_diff_skipped_context() {
10 10: j
"###);
}
#[test]
fn test_diff_external_tool() {
let mut test_env = TestEnvironment::default();
test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
std::fs::write(repo_path.join("file1"), "foo\n").unwrap();
std::fs::write(repo_path.join("file2"), "foo\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::remove_file(repo_path.join("file1")).unwrap();
std::fs::write(repo_path.join("file2"), "foo\nbar\n").unwrap();
std::fs::write(repo_path.join("file3"), "foo\n").unwrap();
let edit_script = test_env.set_up_fake_diff_editor();
std::fs::write(
&edit_script,
"print-files-before\0print --\0print-files-after",
)
.unwrap();
// diff without file patterns
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["diff", "--tool=fake-diff-editor"]), @r###"
file1
file2
--
file2
file3
"###);
// diff with file patterns
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["diff", "--tool=fake-diff-editor", "file1"]), @r###"
file1
--
"###);
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["log", "-p", "--tool=fake-diff-editor"]), @r###"
@ rlvkpnrz test.user@example.com 2001-02-03 04:05:09.000 +07:00 0cba70c7
│ (no description set)
│ file1
│ file2
│ --
│ file2
│ file3
◉ qpvuntsm test.user@example.com 2001-02-03 04:05:08.000 +07:00 39b5a56f
│ (no description set)
│ --
│ file1
│ file2
◉ zzzzzzzz 1970-01-01 00:00:00.000 +00:00 00000000
(empty) (no description set)
--
"###);
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor"]), @r###"
Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6
Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp
Author: Test User <test.user@example.com> (2001-02-03 04:05:08.000 +07:00)
Committer: Test User <test.user@example.com> (2001-02-03 04:05:09.000 +07:00)
(no description set)
file1
file2
--
file2
file3
"###);
// Output of external diff tool shouldn't be escaped
std::fs::write(&edit_script, "print \x1b[1;31mred").unwrap();
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["diff", "--color=always", "--tool=fake-diff-editor"]),
@r###"
red
"###);
// Non-zero exit code isn't an error
std::fs::write(&edit_script, "print diff\0fail").unwrap();
insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor"]), @r###"
Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6
Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp
Author: Test User <test.user@example.com> (2001-02-03 04:05:08.000 +07:00)
Committer: Test User <test.user@example.com> (2001-02-03 04:05:09.000 +07:00)
(no description set)
diff
"###);
}