From b53b92aa541030b84bc7434706b0d455c72a53d4 Mon Sep 17 00:00:00 2001 From: Sichen Date: Tue, 13 Oct 2026 14:45:34 +0000 Subject: [PATCH 1/2] diff: parse clusters of supported short options Handle the seven short options that take no argument through the same path in their separate and clustered forms. Keeping the existing fallback when a word contains an unsupported option, and share the output-style conflict checks between both forms. Add parser and command-output tests for clustered options. --- src/params.rs | 172 ++++++++++++++++++++++++++++++------- tests/by-util/test_diff.rs | 74 ++++++++++++++++ 2 files changed, 217 insertions(+), 29 deletions(-) diff --git a/src/params.rs b/src/params.rs index 239ebca4..686e47bd 100644 --- a/src/params.rs +++ b/src/params.rs @@ -79,37 +79,58 @@ pub fn parse_params>(mut opts: Peekable) -> Resu } continue; } - if param == "-s" || param == "--report-identical-files" { + if let Some(short) = param + .to_str() + .and_then(|arg| arg.strip_prefix('-')) + .filter(|arg| !arg.starts_with('-')) + { + if short.len() == 1 { + if apply_short_option(short.as_bytes()[0], &mut params, &mut format)? { + continue; + } + } else if short.len() > 1 { + // Leave mixed words whole for legacy numeric options and their diagnostics. + let all_recognized = short.bytes().all(|option| { + let mut isolated_format = None; + matches!( + apply_short_option(option, &mut Params::default(), &mut isolated_format), + Ok(true) + ) + }); + if all_recognized { + let mut clustered_params = params.clone(); + let mut clustered_format = format; + for option in short.bytes() { + apply_short_option(option, &mut clustered_params, &mut clustered_format)?; + } + params = clustered_params; + format = clustered_format; + continue; + } + } + } + if param == "--report-identical-files" { params.report_identical_files = true; continue; } - if param == "-q" || param == "--brief" { + if param == "--brief" { params.brief = true; continue; } - if param == "-t" || param == "--expand-tabs" { + if param == "--expand-tabs" { params.expand_tabs = true; continue; } if param == "--normal" { - if format.is_some() && format != Some(Format::Normal) { - return Err("Conflicting output style options".to_string()); - } - format = Some(Format::Normal); + set_format(&mut format, Format::Normal)?; continue; } - if param == "-e" || param == "--ed" { - if format.is_some() && format != Some(Format::Ed) { - return Err("Conflicting output style options".to_string()); - } - format = Some(Format::Ed); + if param == "--ed" { + set_format(&mut format, Format::Ed)?; continue; } - if param == "-y" || param == "--side-by-side" { - if format.is_some() && format != Some(Format::SideBySide) { - return Err("Conflicting output style option".to_string()); - } - format = Some(Format::SideBySide); + if param == "--side-by-side" { + set_format(&mut format, Format::SideBySide)?; continue; } if width_re.is_match(param.to_string_lossy().as_ref()) { @@ -156,14 +177,13 @@ pub fn parse_params>(mut opts: Peekable) -> Resu continue; } - match match_context_diff_params(¶m, next_param, format) { + match match_context_diff_params(¶m, next_param, &mut format) { Ok(DiffStyleMatch { is_match, context_count, next_param_consumed, }) => { if is_match { - format = Some(Format::Context); if context_count.is_some() { context = context_count; } @@ -175,14 +195,13 @@ pub fn parse_params>(mut opts: Peekable) -> Resu } Err(error) => return Err(error), } - match match_unified_diff_params(¶m, next_param, format) { + match match_unified_diff_params(¶m, next_param, &mut format) { Ok(DiffStyleMatch { is_match, context_count, next_param_consumed, }) => { if is_match { - format = Some(Format::Unified); if context_count.is_some() { context = context_count; } @@ -249,6 +268,38 @@ pub fn parse_params>(mut opts: Peekable) -> Resu Ok(params) } +fn set_format(format: &mut Option, requested: Format) -> Result<(), String> { + if format.is_some() && *format != Some(requested) { + // Preserve the singular side-by-side diagnostic for compatibility. + let message = if requested == Format::SideBySide { + "Conflicting output style option" + } else { + "Conflicting output style options" + }; + return Err(message.to_string()); + } + *format = Some(requested); + Ok(()) +} + +fn apply_short_option( + option: u8, + params: &mut Params, + format: &mut Option, +) -> Result { + match option { + b'c' => set_format(format, Format::Context)?, + b'e' => set_format(format, Format::Ed)?, + b'q' => params.brief = true, + b's' => params.report_identical_files = true, + b't' => params.expand_tabs = true, + b'u' => set_format(format, Format::Unified)?, + b'y' => set_format(format, Format::SideBySide)?, + _ => return Ok(false), + } + Ok(true) +} + struct DiffStyleMatch { is_match: bool, context_count: Option, @@ -258,7 +309,7 @@ struct DiffStyleMatch { fn match_context_diff_params( param: &OsString, next_param: Option<&OsString>, - format: Option, + format: &mut Option, ) -> Result { const CONTEXT_RE: &str = r"^(-[cC](?\d*)|--context(=(?\d*))?|-(?\d+)c)$"; let regex = Regex::new(CONTEXT_RE).unwrap(); @@ -266,9 +317,7 @@ fn match_context_diff_params( let mut context_count = None; let mut next_param_consumed = false; if is_match { - if format.is_some() && format != Some(Format::Context) { - return Err("Conflicting output style options".to_string()); - } + set_format(format, Format::Context)?; let captures = regex.captures(param.to_str().unwrap()).unwrap(); let num = captures .name("num1") @@ -302,7 +351,7 @@ fn match_context_diff_params( fn match_unified_diff_params( param: &OsString, next_param: Option<&OsString>, - format: Option, + format: &mut Option, ) -> Result { const UNIFIED_RE: &str = r"^(-[uU](?\d*)|--unified(=(?\d*))?|-(?\d+)u)$"; let regex = Regex::new(UNIFIED_RE).unwrap(); @@ -310,9 +359,7 @@ fn match_unified_diff_params( let mut context_count = None; let mut next_param_consumed = false; if is_match { - if format.is_some() && format != Some(Format::Unified) { - return Err("Conflicting output style options".to_string()); - } + set_format(format, Format::Unified)?; let captures = regex.captures(param.to_str().unwrap()).unwrap(); let num = captures .name("num1") @@ -719,6 +766,64 @@ mod tests { } } #[test] + fn short_option_clusters() { + for (cluster, format, brief, report_identical_files, expand_tabs) in [ + ("-cc", Format::Context, false, false, false), + ("-ee", Format::Ed, false, false, false), + ("-qq", Format::Normal, true, false, false), + ("-ss", Format::Normal, false, true, false), + ("-tt", Format::Normal, false, false, true), + ("-uu", Format::Unified, false, false, false), + ("-yy", Format::SideBySide, false, false, false), + ("-cqt", Format::Context, true, false, true), + ("-eqs", Format::Ed, true, true, false), + ("-qst", Format::Normal, true, true, true), + ("-uqt", Format::Unified, true, false, true), + ("-yqs", Format::SideBySide, true, true, false), + ] { + assert_eq!( + Ok(Params { + executable: os("diff"), + from: os("foo"), + to: os("bar"), + format, + brief, + report_identical_files, + expand_tabs, + ..Default::default() + }), + parse_params( + [os("diff"), os(cluster), os("foo"), os("bar")] + .iter() + .cloned() + .peekable() + ), + "{cluster}" + ); + } + } + #[test] + fn control_partly_recognized_short_option_clusters() { + for options in [ + vec!["-qg"], + vec!["-u", "-cX"], + vec!["-u", "-qcX"], + vec!["-c", "-uX"], + ] { + let unknown = options.last().unwrap().to_string(); + let args = ["diff"] + .into_iter() + .chain(options) + .chain(["foo", "bar"]) + .map(os) + .collect::>(); + assert_eq!( + Err(format!("unrecognized option '{unknown}'")), + parse_params(args.into_iter().peekable()) + ); + } + } + #[test] fn tabsize() { assert_eq!( Ok(Params { @@ -965,5 +1070,14 @@ mod tests { ) .is_err()); } + assert_eq!( + Err("Conflicting output style options".to_string()), + parse_params( + [os("diff"), os("-c"), os("-ut"), os("foo"), os("bar")] + .iter() + .cloned() + .peekable() + ) + ); } } diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index fbdd4940..84459bb0 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -118,6 +118,80 @@ fn differences_brief() -> Result<(), Box> { Ok(()) } +#[test] +fn short_option_clusters_match_split_options() -> Result<(), Box> { + let mut left = NamedTempFile::new()?; + left.write_all(b"a\tb\n")?; + let mut right = NamedTempFile::new()?; + right.write_all(b"a b\n")?; + + let equivalent = |cluster: &[&str], + split: &[&str], + other: &std::path::Path| + -> Result> { + let run = |options: &[&str]| { + cargo_bin_cmd!("diffutils") + .arg("diff") + .args(options) + .arg(left.path()) + .arg(other) + .output() + }; + let clustered = run(cluster)?; + let separate = run(split)?; + assert_eq!( + clustered.status.code(), + separate.status.code(), + "{cluster:?}" + ); + assert_eq!(clustered.stdout, separate.stdout, "{cluster:?}"); + assert_eq!(clustered.stderr, separate.stderr, "{cluster:?}"); + Ok(clustered) + }; + + for (cluster, split, expected_code) in [ + (&["-cc"][..], &["-c", "-c"][..], Some(1)), + (&["-ee"], &["-e", "-e"], Some(1)), + (&["-qq"], &["-q", "-q"], Some(1)), + (&["-ss"], &["-s", "-s"], Some(1)), + (&["-tt"], &["-t", "-t"], Some(1)), + (&["-uu"], &["-u", "-u"], Some(1)), + (&["-cqt"], &["-c", "-q", "-t"], Some(1)), + (&["-eqs"], &["-e", "-q", "-s"], Some(1)), + (&["-qst"], &["-q", "-s", "-t"], Some(1)), + (&["-uqt"], &["-u", "-q", "-t"], Some(1)), + (&["-cu"], &["-c", "-u"], Some(2)), + (&["-uc"], &["-u", "-c"], Some(2)), + (&["-eu"], &["-e", "-u"], Some(2)), + (&["-c", "-ut"], &["-c", "-u", "-t"], Some(2)), + (&["-ut", "-c"], &["-u", "-t", "-c"], Some(2)), + ] { + let clustered = equivalent(cluster, split, right.path())?; + assert_eq!(clustered.status.code(), expected_code, "{cluster:?}"); + } + + let mut same = NamedTempFile::new()?; + same.write_all(b"a\tb\n")?; + let identical = format!( + "Files {} and {} are identical\n", + left.path().to_string_lossy(), + same.path().to_string_lossy() + ); + for (cluster, split) in [ + (&["-ss"][..], &["-s", "-s"][..]), + (&["-qs"], &["-q", "-s"]), + (&["-ys"], &["-y", "-s"]), + ] { + let clustered = equivalent(cluster, split, same.path())?; + assert_eq!(clustered.status.code(), Some(0), "{cluster:?}"); + assert!( + clustered.stdout.ends_with(identical.as_bytes()), + "{cluster:?}" + ); + } + Ok(()) +} + #[test] fn missing_newline() -> Result<(), Box> { let mut file1 = NamedTempFile::new()?; From d50d5adb255f9a17ef104c6fa2a0750175b6426d Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 24 Oct 2026 14:53:18 +0200 Subject: [PATCH 2/2] diff: simplify the parsing of short option clusters --- src/params.rs | 42 +++++--------- tests/by-util/test_diff.rs | 111 ++++++++++++++++--------------------- 2 files changed, 61 insertions(+), 92 deletions(-) diff --git a/src/params.rs b/src/params.rs index 686e47bd..9325a1f1 100644 --- a/src/params.rs +++ b/src/params.rs @@ -79,35 +79,17 @@ pub fn parse_params>(mut opts: Peekable) -> Resu } continue; } + // A word of short options taking no argument, such as `-qst`. Other + // words are left whole for legacy numeric options and their diagnostics. if let Some(short) = param .to_str() .and_then(|arg| arg.strip_prefix('-')) - .filter(|arg| !arg.starts_with('-')) + .filter(|short| !short.is_empty() && short.bytes().all(|b| SHORT_OPTIONS.contains(&b))) { - if short.len() == 1 { - if apply_short_option(short.as_bytes()[0], &mut params, &mut format)? { - continue; - } - } else if short.len() > 1 { - // Leave mixed words whole for legacy numeric options and their diagnostics. - let all_recognized = short.bytes().all(|option| { - let mut isolated_format = None; - matches!( - apply_short_option(option, &mut Params::default(), &mut isolated_format), - Ok(true) - ) - }); - if all_recognized { - let mut clustered_params = params.clone(); - let mut clustered_format = format; - for option in short.bytes() { - apply_short_option(option, &mut clustered_params, &mut clustered_format)?; - } - params = clustered_params; - format = clustered_format; - continue; - } + for option in short.bytes() { + apply_short_option(option, &mut params, &mut format)?; } + continue; } if param == "--report-identical-files" { params.report_identical_files = true; @@ -282,11 +264,14 @@ fn set_format(format: &mut Option, requested: Format) -> Result<(), Stri Ok(()) } +/// The short options that take no argument, and can be grouped. +const SHORT_OPTIONS: &[u8] = b"ceqstuy"; + fn apply_short_option( option: u8, params: &mut Params, format: &mut Option, -) -> Result { +) -> Result<(), String> { match option { b'c' => set_format(format, Format::Context)?, b'e' => set_format(format, Format::Ed)?, @@ -295,9 +280,10 @@ fn apply_short_option( b't' => params.expand_tabs = true, b'u' => set_format(format, Format::Unified)?, b'y' => set_format(format, Format::SideBySide)?, - _ => return Ok(false), + // Callers only pass bytes of SHORT_OPTIONS. + _ => unreachable!("not a short option: {}", char::from(option)), } - Ok(true) + Ok(()) } struct DiffStyleMatch { @@ -803,7 +789,7 @@ mod tests { } } #[test] - fn control_partly_recognized_short_option_clusters() { + fn partly_recognized_short_option_clusters() { for options in [ vec!["-qg"], vec!["-u", "-cX"], diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index 84459bb0..70f997d4 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -119,76 +119,59 @@ fn differences_brief() -> Result<(), Box> { } #[test] -fn short_option_clusters_match_split_options() -> Result<(), Box> { +fn short_option_clusters() -> Result<(), Box> { let mut left = NamedTempFile::new()?; - left.write_all(b"a\tb\n")?; + left.write_all(b"one\ttwo\n")?; let mut right = NamedTempFile::new()?; - right.write_all(b"a b\n")?; - - let equivalent = |cluster: &[&str], - split: &[&str], - other: &std::path::Path| - -> Result> { - let run = |options: &[&str]| { - cargo_bin_cmd!("diffutils") - .arg("diff") - .args(options) - .arg(left.path()) - .arg(other) - .output() - }; - let clustered = run(cluster)?; - let separate = run(split)?; - assert_eq!( - clustered.status.code(), - separate.status.code(), - "{cluster:?}" - ); - assert_eq!(clustered.stdout, separate.stdout, "{cluster:?}"); - assert_eq!(clustered.stderr, separate.stderr, "{cluster:?}"); - Ok(clustered) - }; - - for (cluster, split, expected_code) in [ - (&["-cc"][..], &["-c", "-c"][..], Some(1)), - (&["-ee"], &["-e", "-e"], Some(1)), - (&["-qq"], &["-q", "-q"], Some(1)), - (&["-ss"], &["-s", "-s"], Some(1)), - (&["-tt"], &["-t", "-t"], Some(1)), - (&["-uu"], &["-u", "-u"], Some(1)), - (&["-cqt"], &["-c", "-q", "-t"], Some(1)), - (&["-eqs"], &["-e", "-q", "-s"], Some(1)), - (&["-qst"], &["-q", "-s", "-t"], Some(1)), - (&["-uqt"], &["-u", "-q", "-t"], Some(1)), - (&["-cu"], &["-c", "-u"], Some(2)), - (&["-uc"], &["-u", "-c"], Some(2)), - (&["-eu"], &["-e", "-u"], Some(2)), - (&["-c", "-ut"], &["-c", "-u", "-t"], Some(2)), - (&["-ut", "-c"], &["-u", "-t", "-c"], Some(2)), - ] { - let clustered = equivalent(cluster, split, right.path())?; - assert_eq!(clustered.status.code(), expected_code, "{cluster:?}"); - } - + right.write_all(b"one two\n")?; let mut same = NamedTempFile::new()?; - same.write_all(b"a\tb\n")?; - let identical = format!( + same.write_all(b"one\ttwo\n")?; + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff") + .arg("-qs") + .arg(left.path()) + .arg(right.path()); + cmd.assert().code(predicate::eq(1)).stdout(format!( + "Files {} and {} differ\n", + left.path().to_string_lossy(), + right.path().to_string_lossy() + )); + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff").arg("-sq").arg(left.path()).arg(same.path()); + cmd.assert().code(predicate::eq(0)).stdout(format!( "Files {} and {} are identical\n", left.path().to_string_lossy(), same.path().to_string_lossy() - ); - for (cluster, split) in [ - (&["-ss"][..], &["-s", "-s"][..]), - (&["-qs"], &["-q", "-s"]), - (&["-ys"], &["-y", "-s"]), - ] { - let clustered = equivalent(cluster, split, same.path())?; - assert_eq!(clustered.status.code(), Some(0), "{cluster:?}"); - assert!( - clustered.stdout.ends_with(identical.as_bytes()), - "{cluster:?}" - ); - } + )); + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff") + .arg("-te") + .arg(right.path()) + .arg(left.path()); + cmd.assert() + .code(predicate::eq(1)) + .stdout("1c\none two\n.\n"); + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff") + .arg("-ue") + .arg(left.path()) + .arg(right.path()); + cmd.assert() + .code(predicate::eq(2)) + .stderr(predicate::str::contains("Conflicting output style options")); + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff") + .arg("-qz") + .arg(left.path()) + .arg(right.path()); + cmd.assert() + .code(predicate::eq(2)) + .stderr(predicate::str::contains("unrecognized option '-qz'")); Ok(()) }