Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diff: add a file-by-file variant for external diff tools #3982

Merged
merged 1 commit into from
Jul 4, 2024
+261 −24
Merged

diff: add a file-by-file variant for external diff tools #3982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

* `jj squash` now accepts a `--keep-emptied` option to keep the source commit.

* External diff tools can now be configured to invoke the tool on each file
individually instead of being passed a directory by setting
`merge-tools.$TOOL.diff-invocation-mode="file-by-file"` in config.toml.

fowles marked this conversation as resolved.
Show resolved Hide resolved
### Fixed bugs

* `jj git push` now ignores immutable commits when checking whether a
Expand Down
8 changes: 8 additions & 0 deletions cli/src/config-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,14 @@
"type": "string"
}
},
"diff-invocation-mode": {
"description": "Invoke the tool with directories or individual files",
"enum": [
"dir",
"file-by-file"
],
"default": "dir"
},
"edit-args": {
"type": "array",
"items": {
Expand Down
92 changes: 82 additions & 10 deletions cli/src/diff_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::cmp::max;
use std::collections::VecDeque;
use std::io;
use std::ops::Range;
use std::path::{Path, PathBuf};

use futures::{try_join, Stream, StreamExt};
use itertools::Itertools;
Expand All @@ -40,7 +41,10 @@ use unicode_width::UnicodeWidthStr as _;

use crate::config::CommandNameAndArgs;
use crate::formatter::Formatter;
use crate::merge_tools::{self, DiffGenerateError, ExternalMergeTool};
use crate::merge_tools::{
self, generate_diff, invoke_external_diff, new_utf8_temp_dir, DiffGenerateError, DiffToolMode,
ExternalMergeTool,
};
use crate::text_util;
use crate::ui::Ui;

Expand Down Expand Up @@ -273,15 +277,23 @@ impl<'a> DiffRenderer<'a> {
show_color_words_diff(repo, formatter, *context, tree_diff, path_converter)?;
}
DiffFormat::Tool(tool) => {
merge_tools::generate_diff(
ui,
formatter.raw(),
from_tree,
to_tree,
matcher,
tool,
)
.map_err(DiffRenderError::DiffGenerate)?;
match tool.diff_invocation_mode {
DiffToolMode::FileByFile => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
show_file_by_file_diff(
ui,
repo,
formatter,
tool,
tree_diff,
path_converter,
)
}
DiffToolMode::Dir => {
generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool)
.map_err(DiffRenderError::DiffGenerate)
}
}?;
}
}
}
Expand Down Expand Up @@ -648,6 +660,66 @@ pub fn show_color_words_diff(
Ok(())
}

pub fn show_file_by_file_diff(
fowles marked this conversation as resolved.
Show resolved Hide resolved
ui: &Ui,
repo: &dyn Repo,
formatter: &mut dyn Formatter,
tool: &ExternalMergeTool,
tree_diff: TreeDiffStream,
path_converter: &RepoPathUiConverter,
) -> Result<(), DiffRenderError> {
fn create_file(
path: &RepoPath,
wc_dir: &Path,
value: MaterializedTreeValue,
) -> Result<PathBuf, DiffRenderError> {
let fs_path = path.to_fs_path(wc_dir);
std::fs::create_dir_all(fs_path.parent().unwrap())?;
fowles marked this conversation as resolved.
Show resolved Hide resolved
let content = diff_content(path, value)?;
std::fs::write(&fs_path, content.contents)?;
Ok(fs_path)
}

let temp_dir = new_utf8_temp_dir("jj-diff-")?;
let left_wc_dir = temp_dir.path().join("left");
let right_wc_dir = temp_dir.path().join("right");
let mut diff_stream = materialized_diff_stream(repo.store(), tree_diff);
async {
while let Some((path, diff)) = diff_stream.next().await {
let ui_path = path_converter.format_file_path(&path);
let (left_value, right_value) = diff?;

match (&left_value, &right_value) {
(_, MaterializedTreeValue::AccessDenied(source))
| (MaterializedTreeValue::AccessDenied(source), _) => {
write!(
formatter.labeled("access-denied"),
"Access denied to {ui_path}:"
)?;
writeln!(formatter, " {source}")?;
continue;
}
_ => {}
}
let left_path = create_file(&path, &left_wc_dir, left_value)?;
let right_path = create_file(&path, &right_wc_dir, right_value)?;

fowles marked this conversation as resolved.
Show resolved Hide resolved
invoke_external_diff(
ui,
formatter.raw(),
tool,
&maplit::hashmap! {
"left" => left_path.to_str().expect("temp_dir should be valid utf-8"),
"right" => right_path.to_str().expect("temp_dir should be valid utf-8"),
},
)
.map_err(DiffRenderError::DiffGenerate)?;
fowles marked this conversation as resolved.
Show resolved Hide resolved
}
Ok::<(), DiffRenderError>(())
}
.block_on()
}

struct GitDiffPart {
mode: String,
hash: String,
Expand Down
29 changes: 25 additions & 4 deletions cli/src/merge_tools/external.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ 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<String>,
/// Whether to execute the tool with a pair of directories or individual
/// files.
pub diff_invocation_mode: DiffToolMode,
/// Arguments to pass to the program when editing diffs.
/// `$left` and `$right` are replaced with the corresponding directories.
pub edit_args: Vec<String>,
Expand All @@ -50,6 +53,15 @@ pub struct ExternalMergeTool {
pub merge_tool_edits_conflict_markers: bool,
}

#[derive(serde::Deserialize, Copy, Clone, Debug, Eq, PartialEq)]
#[serde(rename_all = "kebab-case")]
pub enum DiffToolMode {
/// Invoke the diff tool on a temp directory of the modified files.
Dir,
/// Invoke the diff tool on each of the modified files individually.
FileByFile,
}

impl Default for ExternalMergeTool {
fn default() -> Self {
Self {
Expand All @@ -63,6 +75,7 @@ impl Default for ExternalMergeTool {
edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(),
merge_args: vec![],
merge_tool_edits_conflict_markers: false,
diff_invocation_mode: DiffToolMode::Dir,
}
}
}
Expand Down Expand Up @@ -257,7 +270,7 @@ pub fn edit_diff_external(
diffedit_wc.snapshot_results(base_ignores)
}

/// Generates textual diff by the specified `tool`, and writes into `writer`.
/// Generates textual diff by the specified `tool` and writes into `writer`.
pub fn generate_diff(
ui: &Ui,
writer: &mut dyn Write,
Expand All @@ -272,11 +285,19 @@ pub fn generate_diff(
.map_err(ExternalToolError::SetUpDir)?;
set_readonly_recursively(diff_wc.right_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
// TODO: Add support for tools without directory diff functionality?
invoke_external_diff(ui, writer, tool, &diff_wc.to_command_variables())
}

/// Invokes the specified `tool` directing its output into `writer`.
pub fn invoke_external_diff(
ui: &Ui,
writer: &mut dyn Write,
tool: &ExternalMergeTool,
patterns: &HashMap<&str, &str>,
) -> Result<(), DiffGenerateError> {
// 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));
cmd.args(interpolate_variables(&tool.diff_args, patterns));
tracing::info!(?cmd, "Invoking the external diff generator:");
let mut child = cmd
.stdin(Stdio::null())
Expand Down
15 changes: 14 additions & 1 deletion cli/src/merge_tools/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ use pollster::FutureExt;
use thiserror::Error;

use self::builtin::{edit_diff_builtin, edit_merge_builtin, BuiltinToolError};
pub(crate) use self::diff_working_copies::new_utf8_temp_dir;
use self::diff_working_copies::DiffCheckoutError;
use self::external::{edit_diff_external, ExternalToolError};
pub use self::external::{generate_diff, ExternalMergeTool};
pub use self::external::{generate_diff, invoke_external_diff, DiffToolMode, ExternalMergeTool};
use crate::config::CommandNameAndArgs;
use crate::ui::Ui;

Expand Down Expand Up @@ -351,6 +352,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand All @@ -375,6 +377,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"--edit",
"args",
Expand Down Expand Up @@ -410,6 +413,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand All @@ -430,6 +434,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"-l",
"$left",
Expand All @@ -452,6 +457,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"--diff",
"$left",
Expand All @@ -478,6 +484,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"--edit",
"args",
Expand Down Expand Up @@ -505,6 +512,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand All @@ -524,6 +532,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand Down Expand Up @@ -569,6 +578,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand Down Expand Up @@ -614,6 +624,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand Down Expand Up @@ -641,6 +652,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand Down Expand Up @@ -671,6 +683,7 @@ mod tests {
"$left",
"$right",
],
diff_invocation_mode: Dir,
edit_args: [
"$left",
"$right",
Expand Down
22 changes: 13 additions & 9 deletions cli/testing/fake-diff-editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,21 @@ struct Args {
_ignore: Vec<String>,
}

fn files_recursively(dir: &Path) -> HashSet<String> {
fn files_recursively(p: &Path) -> HashSet<String> {
let mut files = HashSet::new();
for dir_entry in std::fs::read_dir(dir).unwrap() {
let dir_entry = dir_entry.unwrap();
let base_name = dir_entry.file_name().to_str().unwrap().to_string();
if dir_entry.path().is_dir() {
for sub_path in files_recursively(&dir_entry.path()) {
files.insert(format!("{base_name}/{sub_path}"));
if !p.is_dir() {
files.insert(p.file_name().unwrap().to_str().unwrap().to_string());
} else {
for dir_entry in std::fs::read_dir(p).unwrap() {
let dir_entry = dir_entry.unwrap();
let base_name = dir_entry.file_name().to_str().unwrap().to_string();
if !dir_entry.path().is_dir() {
files.insert(base_name);
} else {
for sub_path in files_recursively(&dir_entry.path()) {
files.insert(format!("{base_name}/{sub_path}"));
}
}
} else {
files.insert(base_name);
}
}
files
Expand Down
Loading
Loading