diff: simplify the parsing of short option clusters

Author: sylvestre

src/params.rs

@@ -79,35 +79,17 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> 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<Format>, 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<Format>,
-) -> Result<bool, String> {
+) -> 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"],

tests/by-util/test_diff.rs

@@ -119,76 +119,59 @@ fn differences_brief() -> Result<(), Box<dyn std::error::Error>> {
 }
 
 #[test]
-fn short_option_clusters_match_split_options() -> Result<(), Box<dyn std::error::Error>> {
+fn short_option_clusters() -> Result<(), Box<dyn std::error::Error>> {
     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<std::process::Output, Box<dyn std::error::Error>> {
-        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(())
 }