From 2bc8444546516d35ff0536b1c108a4f0f8cf2c96 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Mon, 22 Jul 2024 08:50:24 -0400 Subject: [PATCH] Set unified context iff needed --- Cargo.lock | 197 ++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + src/subcommands/diff.rs | 119 +++++++++++++++++++----- 3 files changed, 296 insertions(+), 21 deletions(-) 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/subcommands/diff.rs b/src/subcommands/diff.rs index 07d37228d..8b2a32cc4 100644 --- a/src/subcommands/diff.rs +++ b/src/subcommands/diff.rs @@ -7,7 +7,8 @@ use bytelines::ByteLinesReader; use crate::config::{self, delta_unreachable}; use crate::delta; -/// Run `git diff` on the files provided on the command line and display the output. +/// 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, @@ -23,15 +24,34 @@ pub fn diff( 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) { - format!("diff -U3 {} --", config.diff_args) - } else { - format!("git diff --no-index --color {} --", config.diff_args) + let diff_args = match shell_words::split(&config.diff_args) { + Ok(words) => words, + Err(err) => { + eprintln!("Failed to parse diff args: {}: {err}", config.diff_args); + return config.error_exit_code; + } }; - let mut diff_args = diff_cmd.split_whitespace(); - let diff_bin = diff_args.next().unwrap(); - + // https://stackoverflow.com/questions/22706714/why-does-git-diff-not-work-with-process-substitution + // TODO: git >= 2.42 supports process substitution + let use_git_diff = + !via_process_substitution(minus_file) && !via_process_substitution(plus_file); + let mut diff_cmd = if use_git_diff { + vec!["git", "diff", "--no-index", "--color"] + } else if diff_args_set_unified_context(&diff_args) { + vec!["diff"] + } else { + vec!["diff", "-U3"] + }; + 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) => { @@ -40,8 +60,8 @@ pub fn diff( } }; - let diff_process = process::Command::new(diff_path) - .args(diff_args) + let diff_process = process::Command::new(&diff_path) + .args(diff_cmd) .args([minus_file, plus_file]) .stdout(process::Stdio::piped()) .spawn(); @@ -80,21 +100,70 @@ pub fn diff( config.error_exit_code }); if code >= 2 { - eprintln!("'{diff_bin}' process failed with exit status {code}. Command was {diff_cmd}"); + eprintln!( + "'{diff_bin}' process failed with exit status {code}. Command was: {}", + format_args!( + "{} {} {} {}", + diff_path.display(), + diff_cmd.join(" "), + 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::path::PathBuf; - use super::diff; + use super::{diff, diff_args_set_unified_context}; use crate::tests::integration_test_utils; + 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] #[ignore] // https://github.com/dandavison/delta/pull/546 fn test_diff_same_empty_file() { @@ -120,15 +189,23 @@ mod main_tests { } fn _do_diff_test(file_a: &str, file_b: &str, expect_diff: bool) { - let config = integration_test_utils::make_config_from_args(&[]); - let mut writer = Cursor::new(vec![]); - let exit_code = diff( - &PathBuf::from(file_a), - &PathBuf::from(file_b), - &config, - &mut writer, - ); - assert_eq!(exit_code, if expect_diff { 1 } else { 0 }); + for args in [ + vec![], + vec!["-@''"], + vec!["-@-u"], + vec!["-@-U99"], + vec!["-@-U0"], + ] { + let config = integration_test_utils::make_config_from_args(&args); + let mut writer = Cursor::new(vec![]); + let exit_code = diff( + &PathBuf::from(file_a), + &PathBuf::from(file_b), + &config, + &mut writer, + ); + assert_eq!(exit_code, if expect_diff { 1 } else { 0 }); + } } fn _read_to_string(cursor: &mut Cursor>) -> String {