From 5c53c5e3d955c4f993503bfdd4dc82b436017d22 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 15 Aug 2024 06:42:41 -0400 Subject: [PATCH] Support passing arguments to git diff and diff (#1697) Adds --diff-args with short form -@. --- Cargo.lock | 197 +++++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + src/cli.rs | 19 +++- src/config.rs | 2 + src/options/set.rs | 1 + src/subcommands/diff.rs | 202 ++++++++++++++++++++++++++++++---------- src/utils/bat/less.rs | 43 +++++---- src/utils/git.rs | 31 ++++++ src/utils/mod.rs | 1 + 9 files changed, 426 insertions(+), 71 deletions(-) create mode 100644 src/utils/git.rs diff --git a/Cargo.lock b/Cargo.lock index 238a02dfb..c82c656d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -483,6 +483,101 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "futures" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "645c6916888f6cb6350d2550b80fb63e734897a8498abe35cfb732b6487804b0" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-channel" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eac8f7d7865dcb88bd4373ab671c8cf4508703796caa2b1985a9ca867b3fcb78" +dependencies = [ + "futures-core", + "futures-sink", +] + +[[package]] +name = "futures-core" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dfc6580bb841c5a68e9ef15c77ccc837b40a7504914d52e47b8b0e9bbda25a1d" + +[[package]] +name = "futures-executor" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a576fc72ae164fca6b9db127eaa9a9dda0d61316034f33a0a0d4eda41f02b01d" +dependencies = [ + "futures-core", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-io" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a44623e20b9681a318efdd71c299b6b222ed6f231972bfe2f224ebad6311f0c1" + +[[package]] +name = "futures-macro" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "87750cf4b7a4c0625b1529e4c543c2182106e4dedc60a2a6455e00d212c489ac" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "futures-sink" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9fb8e00e87438d937621c1c6269e53f536c14d3fbd6a042bb24879e57d474fb5" + +[[package]] +name = "futures-task" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "38d84fa142264698cdce1a9f9172cf383a0c82de1bddcf3092901442c4097004" + +[[package]] +name = "futures-timer" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" + +[[package]] +name = "futures-util" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d6401deb83407ab3da39eba7e33987a73c3df0c82b4bb5813ee871c19c41d48" +dependencies = [ + "futures-channel", + "futures-core", + "futures-io", + "futures-macro", + "futures-sink", + "futures-task", + "memchr", + "pin-project-lite", + "pin-utils", + "slab", +] + [[package]] name = "getrandom" version = "0.2.11" @@ -521,6 +616,7 @@ dependencies = [ "palette", "pathdiff", "regex", + "rstest", "serde", "serde_json", "shell-words", @@ -944,6 +1040,18 @@ dependencies = [ "siphasher", ] +[[package]] +name = "pin-project-lite" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bda66fc9667c18cb2758a2ac84d1167245054bcf85d5d1aaa6923f45801bdd02" + +[[package]] +name = "pin-utils" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" + [[package]] name = "pkg-config" version = "0.3.28" @@ -969,6 +1077,15 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391" +[[package]] +name = "proc-macro-crate" +version = "3.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d37c51ca738a55da99dc0c4a34860fd675453b8b36209178c2249bb13651284" +dependencies = [ + "toml_edit", +] + [[package]] name = "proc-macro2" version = "1.0.75" @@ -1060,6 +1177,12 @@ version = "0.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c08c74e62047bb2de4ff487b251e4a92e24f48745648451635cec7d591162d9f" +[[package]] +name = "relative-path" +version = "1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" + [[package]] name = "rgb" version = "0.8.37" @@ -1069,6 +1192,45 @@ dependencies = [ "bytemuck", ] +[[package]] +name = "rstest" +version = "0.21.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9afd55a67069d6e434a95161415f5beeada95a01c7b815508a82dcb0e1593682" +dependencies = [ + "futures", + "futures-timer", + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.21.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4165dfae59a39dd41d8dec720d3cbfbc71f69744efb480a3920f5d4e0cc6798d" +dependencies = [ + "cfg-if", + "glob", + "proc-macro-crate", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn", + "unicode-ident", +] + +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.38.28" @@ -1165,6 +1327,15 @@ version = "0.3.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "38b58827f4464d87d377d175e90bf58eb00fd8716ff0a62f80356b5e61555d0d" +[[package]] +name = "slab" +version = "0.4.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f92a496fb766b417c996b9c5e57daf2f7ad3b0bebe1ccfca4856390e3d3bb67" +dependencies = [ + "autocfg", +] + [[package]] name = "smol_str" version = "0.1.24" @@ -1342,6 +1513,23 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" +[[package]] +name = "toml_datetime" +version = "0.6.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4badfd56924ae69bcc9039335b2e017639ce3f9b001c393c1b2d1ef846ce2cbf" + +[[package]] +name = "toml_edit" +version = "0.21.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8534fd7f78b5405e860340ad6575217ce99f38d4d5c8f2442cb5ecb50090e1" +dependencies = [ + "indexmap", + "toml_datetime", + "winnow", +] + [[package]] name = "unicode-bidi" version = "0.3.14" @@ -1721,6 +1909,15 @@ version = "0.52.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dff9641d1cd4be8d1a070daf9e3773c5f67e78b4d9d42263020c057706765c04" +[[package]] +name = "winnow" +version = "0.5.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f593a95398737aeed53e489c785df13f3618e41dbcd6718c6addbf1395aa6876" +dependencies = [ + "memchr", +] + [[package]] name = "xdg" version = "2.5.2" diff --git a/Cargo.toml b/Cargo.toml index 7e0682ec7..211364bbc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,6 +63,7 @@ unexpected_cfgs = { level = "warn", check-cfg = ['cfg(tarpaulin_include)'] } [dev-dependencies] insta = { version = "1.*", features = ["colors", "filters"] } +rstest = "0.21.0" [profile.test] opt-level = 2 diff --git a/src/cli.rs b/src/cli.rs index 79617ab63..fa635a756 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -166,6 +166,21 @@ pub struct Opt { #[arg(long = "detect-dark-light", value_enum, default_value_t = DetectDarkLight::default())] pub detect_dark_light: DetectDarkLight, + #[arg( + long = "diff-args", + short = '@', + default_value = "", + value_name = "STRING" + )] + /// Extra arguments to pass to `git diff` when using delta to diff two files. + /// + /// E.g. `delta --diff-args=-U999 file_1 file_2` is equivalent to + /// `git diff --no-index --color -U999 file_1 file_2 | delta`. + /// + /// If you use process substitution (`delta <(command_1) <(command_2)`) and your git version + /// doesn't support it, then delta will fall back to `diff` instead of `git diff`. + pub diff_args: String, + #[arg(long = "diff-highlight")] /// Emulate diff-highlight. /// @@ -960,12 +975,12 @@ pub struct Opt { /// Deprecated: use --true-color. pub _24_bit_color: Option, - /// First file to be compared when delta is being used in diff mode + /// First file to be compared when delta is being used to diff two files. /// /// `delta file1 file2` is equivalent to `diff -u file1 file2 | delta`. pub minus_file: Option, - /// Second file to be compared when delta is being used in diff mode. + /// Second file to be compared when delta is being used to diff two files. pub plus_file: Option, #[arg(skip)] diff --git a/src/config.rs b/src/config.rs index c2fbccb73..8d42574f2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -53,6 +53,7 @@ pub struct Config { pub cwd_relative_to_repo_root: Option, pub decorations_width: cli::Width, pub default_language: String, + pub diff_args: String, pub diff_stat_align_width: usize, pub error_exit_code: i32, pub file_added_label: String, @@ -298,6 +299,7 @@ impl From for Config { cwd_relative_to_repo_root, decorations_width: opt.computed.decorations_width, default_language: opt.default_language, + diff_args: opt.diff_args, diff_stat_align_width: opt.diff_stat_align_width, error_exit_code: 2, // Use 2 for error because diff uses 0 and 1 for non-error. file_added_label, diff --git a/src/options/set.rs b/src/options/set.rs index 2fae345bf..f02cd4ed7 100644 --- a/src/options/set.rs +++ b/src/options/set.rs @@ -140,6 +140,7 @@ pub fn set_options( commit_regex, commit_style, default_language, + diff_args, diff_stat_align_width, file_added_label, file_copied_label, diff --git a/src/subcommands/diff.rs b/src/subcommands/diff.rs index ddaf44d88..215d45e38 100644 --- a/src/subcommands/diff.rs +++ b/src/subcommands/diff.rs @@ -1,4 +1,4 @@ -use std::io::{ErrorKind, Write}; +use std::io::{BufRead, ErrorKind, Write}; use std::path::{Path, PathBuf}; use std::process; @@ -6,8 +6,16 @@ use bytelines::ByteLinesReader; use crate::config::{self, delta_unreachable}; use crate::delta; +use crate::utils::git::retrieve_git_version; -/// Run `git diff` on the files provided on the command line and display the output. +#[derive(Debug, PartialEq)] +enum Differ { + GitDiff, + Diff, +} + +/// Run `git diff` on the files provided on the command line and display the output. Fall back to +/// `diff` if the supplied "files" use process substitution. pub fn diff( minus_file: &Path, plus_file: &Path, @@ -16,20 +24,57 @@ pub fn diff( ) -> i32 { use std::io::BufReader; - // When called as `delta <(echo foo) <(echo bar)`, then git as of version 2.34 just prints the - // diff of the filenames which were created by the process substitution and does not read their - // content, so fall back to plain `diff` which simply opens the given input as files. - // This fallback ignores git settings, but is better than nothing. + let mut diff_args = match shell_words::split(config.diff_args.trim()) { + Ok(words) => words, + Err(err) => { + eprintln!("Failed to parse diff args: {}: {err}", config.diff_args); + return config.error_exit_code; + } + }; + // Permit e.g. -@U1 + if diff_args + .first() + .map(|arg| !arg.is_empty() && !arg.starts_with('-')) + .unwrap_or(false) + { + diff_args[0] = format!("-{}", diff_args[0]) + } + let via_process_substitution = |f: &Path| f.starts_with("/proc/self/fd/") || f.starts_with("/dev/fd/"); - let diff_cmd = if via_process_substitution(minus_file) || via_process_substitution(plus_file) { - ["diff", "-u", "--"].as_slice() - } else { - ["git", "diff", "--no-index", "--color", "--"].as_slice() + // https://stackoverflow.com/questions/22706714/why-does-git-diff-not-work-with-process-substitution + // git <2.42 does not support process substitution + let (differ, mut diff_cmd) = match retrieve_git_version() { + Some(version) + if version >= (2, 42) + || !(via_process_substitution(minus_file) + || via_process_substitution(plus_file)) => + { + ( + Differ::GitDiff, + vec!["git", "diff", "--no-index", "--color"], + ) + } + _ => ( + Differ::Diff, + if diff_args_set_unified_context(&diff_args) { + vec!["diff"] + } else { + vec!["diff", "-U3"] + }, + ), }; - let diff_bin = diff_cmd[0]; + diff_cmd.extend( + diff_args + .iter() + .filter(|s| !s.is_empty()) + .map(String::as_str), + ); + diff_cmd.push("--"); + + let (diff_bin, diff_cmd) = diff_cmd.split_first().unwrap(); let diff_path = match grep_cli::resolve_binary(PathBuf::from(diff_bin)) { Ok(path) => path, Err(err) => { @@ -38,10 +83,11 @@ pub fn diff( } }; - let diff_process = process::Command::new(diff_path) - .args(&diff_cmd[1..]) + let diff_process = process::Command::new(&diff_path) + .args(diff_cmd) .args([minus_file, plus_file]) .stdout(process::Stdio::piped()) + .stderr(process::Stdio::piped()) .spawn(); if let Err(err) = diff_process { @@ -64,10 +110,10 @@ pub fn diff( } }; - // Return the exit code from the diff process, so that the exit code - // contract of `delta file_A file_B` is the same as that of `diff file_A - // file_B` (i.e. 0 if same, 1 if different, 2 if error). - diff_process + // Return the exit code from the diff process, so that the exit code contract of `delta file1 + // file2` is the same as that of `diff file1 file2` (i.e. 0 if same, 1 if different, >= 2 if + // error). + let code = diff_process .wait() .unwrap_or_else(|_| { delta_unreachable(&format!("'{diff_bin}' process not running.")); @@ -76,43 +122,100 @@ pub fn diff( .unwrap_or_else(|| { eprintln!("'{diff_bin}' process terminated without exit status."); config.error_exit_code - }) + }); + if code >= 2 { + for line in BufReader::new(diff_process.stderr.unwrap()).lines() { + eprintln!("{}", line.unwrap_or("".into())); + if code == 129 && differ == Differ::GitDiff { + // `git diff` unknown option: print first line (which is an error message) but not + // the remainder (which is the entire --help text). + break; + } + } + eprintln!( + "'{diff_bin}' process failed with exit status {code}. Command was: {}", + format_args!( + "{} {} {} {}", + diff_path.display(), + shell_words::join(diff_cmd), + minus_file.display(), + plus_file.display() + ) + ); + config.error_exit_code + } else { + code + } +} + +/// Do the user-supplied `diff` args set the unified context? +fn diff_args_set_unified_context(args: I) -> bool +where + I: IntoIterator, + S: AsRef, +{ + // This function is applied to `diff` args; not `git diff`. + for arg in args { + let arg = arg.as_ref(); + if arg == "-u" || arg == "-U" { + // diff allows a space after -U (git diff does not) + return true; + } + if (arg.starts_with("-U") || arg.starts_with("-u")) + && arg.split_at(2).1.parse::().is_ok() + { + return true; + } + } + false } #[cfg(test)] mod main_tests { - use std::io::{Cursor, Read, Seek}; + use std::io::Cursor; use std::path::PathBuf; - use super::diff; + use super::{diff, diff_args_set_unified_context}; use crate::tests::integration_test_utils; - #[test] - #[ignore] // https://github.com/dandavison/delta/pull/546 - fn test_diff_same_empty_file() { - _do_diff_test("/dev/null", "/dev/null", false); - } - - #[test] - #[cfg_attr(target_os = "windows", ignore)] - fn test_diff_same_non_empty_file() { - _do_diff_test("/etc/passwd", "/etc/passwd", false); - } - - #[test] - #[cfg_attr(target_os = "windows", ignore)] - fn test_diff_empty_vs_non_empty_file() { - _do_diff_test("/dev/null", "/etc/passwd", true); + use rstest::rstest; + + #[rstest] + #[case(&["-u"], true)] + #[case(&["-u7"], true)] + #[case(&["-u77"], true)] + #[case(&["-ux"], false)] + #[case(&["-U"], true)] + #[case(&["-U7"], true)] + #[case(&["-U77"], true)] + #[case(&["-Ux"], false)] + fn test_unified_diff_arg_is_detected_in_diff_args( + #[case] diff_args: &[&str], + #[case] expected: bool, + ) { + assert_eq!(diff_args_set_unified_context(diff_args), expected) } - #[test] - #[cfg_attr(target_os = "windows", ignore)] - fn test_diff_two_non_empty_files() { - _do_diff_test("/etc/group", "/etc/passwd", true); + enum ExpectDiff { + Yes, + No, } - fn _do_diff_test(file_a: &str, file_b: &str, expect_diff: bool) { - let config = integration_test_utils::make_config_from_args(&[]); + #[cfg(not(target_os = "windows"))] + #[rstest] + // #[case("/dev/null", "/dev/null", ExpectDiff::No)] https://github.com/dandavison/delta/pull/546#issuecomment-835852373 + #[case("/etc/group", "/etc/passwd", ExpectDiff::Yes)] + #[case("/dev/null", "/etc/passwd", ExpectDiff::Yes)] + #[case("/etc/passwd", "/etc/passwd", ExpectDiff::No)] + fn test_diff_real_files( + #[case] file_a: &str, + #[case] file_b: &str, + #[case] expect_diff: ExpectDiff, + #[values(vec![], vec!["-@''"], vec!["-@-u"], vec!["-@-U99"], vec!["-@-U0"])] args: Vec< + &str, + >, + ) { + let config = integration_test_utils::make_config_from_args(&args); let mut writer = Cursor::new(vec![]); let exit_code = diff( &PathBuf::from(file_a), @@ -120,13 +223,12 @@ mod main_tests { &config, &mut writer, ); - assert_eq!(exit_code, if expect_diff { 1 } else { 0 }); - } - - fn _read_to_string(cursor: &mut Cursor>) -> String { - let mut s = String::new(); - cursor.rewind().unwrap(); - cursor.read_to_string(&mut s).unwrap(); - s + assert_eq!( + exit_code, + match expect_diff { + ExpectDiff::Yes => 1, + ExpectDiff::No => 0, + } + ); } } diff --git a/src/utils/bat/less.rs b/src/utils/bat/less.rs index 1ca9f76fb..4de350403 100644 --- a/src/utils/bat/less.rs +++ b/src/utils/bat/less.rs @@ -19,9 +19,13 @@ fn parse_less_version(output: &[u8]) -> Option { } } -#[test] -fn test_parse_less_version_487() { - let output = b"less 487 (GNU regular expressions) +#[cfg(test)] +mod tests { + use super::parse_less_version; + + #[test] + fn test_parse_less_version_487() { + let output = b"less 487 (GNU regular expressions) Copyright (C) 1984-2016 Mark Nudelman less comes with NO WARRANTY, to the extent permitted by law. @@ -29,12 +33,12 @@ For information about the terms of redistribution, see the file named README in the less distribution. Homepage: http://www.greenwoodsoftware.com/less"; - assert_eq!(Some(487), parse_less_version(output)); -} + assert_eq!(Some(487), parse_less_version(output)); + } -#[test] -fn test_parse_less_version_529() { - let output = b"less 529 (Spencer V8 regular expressions) + #[test] + fn test_parse_less_version_529() { + let output = b"less 529 (Spencer V8 regular expressions) Copyright (C) 1984-2017 Mark Nudelman less comes with NO WARRANTY, to the extent permitted by law. @@ -42,12 +46,12 @@ For information about the terms of redistribution, see the file named README in the less distribution. Homepage: http://www.greenwoodsoftware.com/less"; - assert_eq!(Some(529), parse_less_version(output)); -} + assert_eq!(Some(529), parse_less_version(output)); + } -#[test] -fn test_parse_less_version_551() { - let output = b"less 551 (PCRE regular expressions) + #[test] + fn test_parse_less_version_551() { + let output = b"less 551 (PCRE regular expressions) Copyright (C) 1984-2019 Mark Nudelman less comes with NO WARRANTY, to the extent permitted by law. @@ -55,12 +59,13 @@ For information about the terms of redistribution, see the file named README in the less distribution. Home page: http://www.greenwoodsoftware.com/less"; - assert_eq!(Some(551), parse_less_version(output)); -} + assert_eq!(Some(551), parse_less_version(output)); + } -#[test] -fn test_parse_less_version_wrong_program() { - let output = b"more from util-linux 2.34"; + #[test] + fn test_parse_less_version_wrong_program() { + let output = b"more from util-linux 2.34"; - assert_eq!(None, parse_less_version(output)); + assert_eq!(None, parse_less_version(output)); + } } diff --git a/src/utils/git.rs b/src/utils/git.rs new file mode 100644 index 000000000..fa01e05bb --- /dev/null +++ b/src/utils/git.rs @@ -0,0 +1,31 @@ +use std::process::Command; + +pub fn retrieve_git_version() -> Option<(usize, usize)> { + if let Ok(git_path) = grep_cli::resolve_binary("git") { + let cmd = Command::new(git_path).arg("--version").output().ok()?; + parse_git_version(&cmd.stdout) + } else { + None + } +} + +fn parse_git_version(output: &[u8]) -> Option<(usize, usize)> { + let mut parts = output.strip_prefix(b"git version ")?.split(|&b| b == b'.'); + let major = std::str::from_utf8(parts.next()?).ok()?.parse().ok()?; + let minor = std::str::from_utf8(parts.next()?).ok()?.parse().ok()?; + Some((major, minor)) +} + +#[cfg(test)] +mod tests { + use super::parse_git_version; + use rstest::rstest; + + #[rstest] + #[case(b"git version 2.46.0", Some((2, 46)))] + #[case(b"git version 2.39.3 (Apple Git-146)", Some((2, 39)))] + #[case(b"", None)] + fn test_parse_git_version(#[case] input: &[u8], #[case] expected: Option<(usize, usize)>) { + assert_eq!(parse_git_version(input), expected); + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 2d952fe32..b460e5af3 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,5 +1,6 @@ #[cfg(not(tarpaulin_include))] pub mod bat; +pub mod git; pub mod helpwrap; pub mod path; pub mod process;