Skip to content

Commit

Permalink
merge_tools: enable :builtin as default diff/merge editor
Browse files Browse the repository at this point in the history
  • Loading branch information
@arxanas
arxanas committed Sep 21, 2023
1 parent 1e94f92 commit 409356f
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 86 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
`jj log -T 'description ++ "\0"' --no-graph` to output descriptions only, but
be able to tell where the boundaries are

* jj now bundles a TUI tool to use as the default diff and merge editors. (The
previous default was `meld`.)

### Fixed bugs

## [0.9.0] - 2023-09-06
Expand Down
20 changes: 15 additions & 5 deletions cli/src/diff_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use unicode_width::UnicodeWidthStr as _;

use crate::cli_util::{CommandError, WorkspaceCommandHelper};
use crate::formatter::Formatter;
use crate::merge_tools::{self, ExternalMergeTool};
use crate::merge_tools::{self, ExternalMergeTool, MergeTool};
use crate::text_util;
use crate::ui::Ui;

Expand Down Expand Up @@ -124,8 +124,13 @@ fn diff_formats_from_args(
.collect_vec();
if let Some(name) = &args.tool {
let tool = merge_tools::get_tool_config(settings, name)?
.unwrap_or_else(|| ExternalMergeTool::with_program(name));
formats.push(DiffFormat::Tool(Box::new(tool)));
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_program(name)));
match tool {
MergeTool::Builtin => {}
MergeTool::External(tool) => {
formats.push(DiffFormat::Tool(Box::new(tool)));
}
}
}
Ok(formats)
}
Expand All @@ -135,8 +140,13 @@ fn default_diff_format(settings: &UserSettings) -> Result<DiffFormat, config::Co
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(|| ExternalMergeTool::with_diff_args(&args));
return Ok(DiffFormat::Tool(Box::new(tool)));
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_diff_args(&args)));
match tool {
MergeTool::Builtin => {}
MergeTool::External(tool) => {
return Ok(DiffFormat::Tool(Box::new(tool)));
}
}
}
let name = if let Some(name) = config.get_string("ui.diff.format").optional()? {
name
Expand Down
85 changes: 23 additions & 62 deletions cli/src/merge_tools/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ pub use self::external::{generate_diff, ExternalMergeTool};
use crate::config::CommandNameAndArgs;
use crate::ui::Ui;

const BUILTIN_EDITOR_NAME: &str = ":builtin";

#[derive(Debug, Error)]
pub enum DiffEditError {
#[error(transparent)]
Expand Down Expand Up @@ -166,7 +168,7 @@ fn editor_args_from_settings(
if let Some(args) = settings.config().get(key).optional()? {
Ok(args)
} else {
let default_editor = "meld";
let default_editor = BUILTIN_EDITOR_NAME;
writeln!(
ui.hint(),
"Using default editor '{default_editor}'; you can change this by setting {key}"
Expand All @@ -180,7 +182,11 @@ fn editor_args_from_settings(
pub fn get_tool_config(
settings: &UserSettings,
name: &str,
) -> Result<Option<ExternalMergeTool>, ConfigError> {
) -> Result<Option<MergeTool>, ConfigError> {
if name == BUILTIN_EDITOR_NAME {
return Ok(Some(MergeTool::Builtin));
}

const TABLE_KEY: &str = "merge-tools";
let tools_table = settings.config().get_table(TABLE_KEY)?;
if let Some(v) = tools_table.get(name) {
Expand All @@ -193,7 +199,7 @@ pub fn get_tool_config(
if result.program.is_empty() {
result.program.clone_from(&name.to_string());
};
Ok(Some(result))
Ok(Some(MergeTool::External(result)))
} else {
Ok(None)
}
Expand All @@ -204,7 +210,7 @@ pub fn get_tool_config(
pub fn get_tool_config_from_args(
settings: &UserSettings,
args: &CommandNameAndArgs,
) -> Result<Option<ExternalMergeTool>, ConfigError> {
) -> Result<Option<MergeTool>, ConfigError> {
match args {
CommandNameAndArgs::String(name) => get_tool_config(settings, name),
CommandNameAndArgs::Vec(_) | CommandNameAndArgs::Structured { .. } => Ok(None),
Expand All @@ -217,23 +223,24 @@ fn get_diff_editor_from_settings(
) -> Result<MergeTool, ExternalToolError> {
let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?;
let editor = get_tool_config_from_args(settings, &args)?
.unwrap_or_else(|| ExternalMergeTool::with_edit_args(&args));
Ok(MergeTool::External(editor))
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args)));
Ok(editor)
}

fn get_merge_tool_from_settings(
ui: &Ui,
settings: &UserSettings,
) -> Result<MergeTool, ExternalToolError> {
let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?;
let editor = get_tool_config_from_args(settings, &args)?
.unwrap_or_else(|| ExternalMergeTool::with_merge_args(&args));
if editor.merge_args.is_empty() {
Err(ExternalToolError::MergeArgsNotConfigured {
tool_name: args.to_string(),
})
} else {
Ok(MergeTool::External(editor))
let mergetool = get_tool_config_from_args(settings, &args)?
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args)));
match mergetool {
MergeTool::External(mergetool) if mergetool.merge_args.is_empty() => {
Err(ExternalToolError::MergeArgsNotConfigured {
tool_name: args.to_string(),
})
}
mergetool => Ok(mergetool),
}
}

Expand All @@ -260,30 +267,7 @@ mod tests {
};

// Default
insta::assert_debug_snapshot!(get("").unwrap(), @r###"
External(
ExternalMergeTool {
program: "meld",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
],
merge_args: [
"$left",
"$base",
"$right",
"-o",
"$output",
"--auto-merge",
],
merge_tool_edits_conflict_markers: false,
},
)
"###);
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###"
Expand Down Expand Up @@ -432,30 +416,7 @@ mod tests {
};

// Default
insta::assert_debug_snapshot!(get("").unwrap(), @r###"
External(
ExternalMergeTool {
program: "meld",
diff_args: [
"$left",
"$right",
],
edit_args: [
"$left",
"$right",
],
merge_args: [
"$left",
"$base",
"$right",
"-o",
"$output",
"--auto-merge",
],
merge_tool_edits_conflict_markers: false,
},
)
"###);
insta::assert_debug_snapshot!(get("").unwrap(), @"Builtin");

// Just program name
insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###"
Expand Down
22 changes: 3 additions & 19 deletions docs/config.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,9 @@ Obviously, you would only set one line, don't copy them all in!

## Editing diffs

The `ui.diff-editor` setting affects the tool used for editing diffs (e.g.
`jj split`, `jj amend -i`). The default is `meld`.
The `ui.diff-editor` setting affects the tool used for editing diffs (e.g. `jj
split`, `jj amend -i`). The default is the special value `:builtin`, which
launches a TUI tool to edit the diff in your terminal.

`jj` makes the following substitutions:

Expand Down Expand Up @@ -388,23 +389,6 @@ the directory where the diff editor will be expected to put the result of the
user's edits. Initially, the contents of `$output` will be the same as the
contents of `$right`.

### Setting up `scm-diff-editor`

`scm-diff-editor` is a terminal-based diff editor that is part of
the [git-branchless](https://github.com/arxanas/git-branchless) suite of tools.
It's a good alternative to Meld, especially if you don't have a graphical
environment (e.g. when using SSH). To install it:

```shell
cargo install --git https://github.com/arxanas/git-branchless scm-record --features scm-diff-editor
```

Then config it as follows:

```toml
ui.diff-editor = ["scm-diff-editor", "--dir-diff", "$left", "$right"]
```

### `JJ-INSTRUCTIONS`

When editing a diff, jj will include a synthetic file called `JJ-INSTRUCTIONS`
Expand Down

0 comments on commit 409356f

Please sign in to comment.