Merge PR #2935: Fix/cli option value errors

contributors/people.toml: union of both new entries

apps/pdfcraft-cli/src/main.rs

@@ -478,17 +478,31 @@ fn edit(args: &[String]) -> Result<(), CliError> {
             "--password" => password = Some(value),
             "--rotate" => {
                 let (pages, deg) = value.split_once(':').ok_or("--rotate PAGES:DEGREES")?;
-                edits.push(Edit::RotatePages { pages: page_list(pages)?, degrees: deg.parse().map_err(|_| "bad degrees")? });
+                let degrees: i64 = deg.parse().map_err(|_| format!("bad --rotate degrees: expected a whole number of degrees, got {deg:?}"))?;
+                // The same rule `rotate_pages` enforces, checked here so a bad angle is named
+                // with its option before any file is opened.
+                if degrees % 90 != 0 {
+                    return Err(format!("bad --rotate degrees: expected a multiple of 90 (such as 90, -90 or 180), got {degrees}").into());
+                }
+                edits.push(Edit::RotatePages { pages: page_list(pages)?, degrees });
             }
             "--delete" => edits.push(Edit::DeletePages { pages: page_list(value)? }),
             "--move" => {
                 let (pages, to) = value.split_once(':').ok_or("--move PAGES:TO")?;
-                let to: usize = to.parse().map_err(|_| "bad target")?;
-                edits.push(Edit::MovePages { pages: page_list(pages)?, to: to.checked_sub(1).ok_or("--move target must be at least 1")? });
+                let target: usize = to
+                    .parse()
+                    .ok()
+                    .filter(|target| *target > 0)
+                    .ok_or_else(|| format!("bad --move target: expected a page number of at least 1, got {to:?}"))?;
+                edits.push(Edit::MovePages { pages: page_list(pages)?, to: target - 1 });
             }
             "--insert-blank" => {
-                let at: usize = value.parse().map_err(|_| "bad position")?;
-                edits.push(Edit::InsertBlankPage { at: at.checked_sub(1).ok_or("--insert-blank must be at least 1")?, width: 612.0, height: 792.0 });
+                let position: usize = value
+                    .parse()
+                    .ok()
+                    .filter(|position| *position > 0)
+                    .ok_or_else(|| format!("bad --insert-blank position: expected a page position of at least 1, got {value:?}"))?;
+                edits.push(Edit::InsertBlankPage { at: position - 1, width: 612.0, height: 792.0 });
             }
             "--title" => edits.push(Edit::SetInfo { key: "Title".into(), value: value.into() }),
             "--author" => edits.push(Edit::SetInfo { key: "Author".into(), value: value.into() }),
@@ -570,7 +584,16 @@ fn split(args: &[String]) -> Result<(), CliError> {
     use pdfcraft_engine::SplitBy;
     let path = *positional(args).first().ok_or("split: missing file")?;
     let by = match (flag(args, "--every"), flag(args, "--before")) {
-        (Some(n), None) => SplitBy::PageCount(n.parse().map_err(|_| "bad --every")?),
+        (Some(n), None) => {
+            // A count the splitter cannot hold is refused here, before the file is opened,
+            // rather than silently clamped to 1 further down.
+            let count = n
+                .parse::<usize>()
+                .ok()
+                .filter(|count| *count > 0)
+                .ok_or_else(|| format!("bad --every: expected a positive whole number of pages, got {n:?}"))?;
+            SplitBy::PageCount(count)
+        }
         (None, Some(list)) => SplitBy::Before(page_list(list)?),
         _ => return Err("split: give either --every N or --before PAGES".into()),
     };
@@ -590,12 +613,25 @@ fn split(args: &[String]) -> Result<(), CliError> {
     Ok(())
 }
 
+/// A `--dpi` the renderer can be asked for: a positive, finite number. `0`, a negative value
+/// or `NaN` parse as an `f32` but reach the renderer as a scale it refuses with its own
+/// internal message (and in `check`, passed on as text, they would turn every file into a
+/// child that printed nothing), so the CLI names the option and the value instead.
+fn parse_dpi(value: &str) -> Result<f32, String> {
+    value
+        .parse::<f32>()
+        .ok()
+        .filter(|dpi| dpi.is_finite() && *dpi > 0.0)
+        .ok_or_else(|| format!("bad --dpi: expected a positive number of dots per inch, got {value:?}"))
+}
+
 fn render(args: &[String]) -> Result<(), CliError> {
     validate_options("render", args, RENDER_OPTIONS)?;
     let path = *positional(args).first().ok_or("render: missing file")?;
-    let page: usize = flag(args, "--page").unwrap_or("1").parse().map_err(|_| "bad --page")?;
-    let page_index = page.checked_sub(1).ok_or("--page must be at least 1")?;
-    let dpi: f32 = flag(args, "--dpi").unwrap_or("96").parse().map_err(|_| "bad --dpi")?;
+    let page: usize =
+        flag(args, "--page").unwrap_or("1").parse().ok().filter(|page| *page > 0).ok_or("bad --page: expected a positive page number")?;
+    let page_index = page - 1;
+    let dpi = parse_dpi(flag(args, "--dpi").unwrap_or("96"))?;
     let out = flag(args, "--out").ok_or("render: missing --out (.png, .jpg, .tif or .pam)")?;
     // The file is what its name says (#248): a `.png` used to get a netpbm PAM stream.
     let format = match Path::new(out).extension().and_then(|e| e.to_str()).map(str::to_ascii_lowercase).as_deref() {
@@ -647,7 +683,7 @@ fn render(args: &[String]) -> Result<(), CliError> {
 fn check_one(args: &[String]) -> Result<(), CliError> {
     validate_options("check-one", args, CHECK_ONE_OPTIONS)?;
     let path = *positional(args).first().ok_or("check-one: missing file")?;
-    let dpi: f32 = flag(args, "--dpi").unwrap_or("36").parse().map_err(|_| "bad --dpi")?;
+    let dpi = parse_dpi(flag(args, "--dpi").unwrap_or("36"))?;
     let start = Instant::now();
     let bytes = read(path)?;
     let (status, pages, failed, detail, warnings) = match inspect(bytes.clone(), None) {
@@ -739,12 +775,23 @@ fn collect(paths: &[&str]) -> Vec<PathBuf> {
 
 fn check(args: &[String]) -> Result<(), CliError> {
     validate_options("check", args, CHECK_OPTIONS)?;
+    // Every option is checked before the inputs are scanned, so a bad value is one message,
+    // not one problem per file: a zero timeout used to make every file time out, and a bad
+    // dpi used to reach each child, which printed nothing and was reported as a crash.
+    let timeout_value = flag(args, "--timeout").unwrap_or("20");
+    let timeout = Duration::from_secs(
+        timeout_value
+            .parse::<u64>()
+            .ok()
+            .filter(|secs| *secs > 0)
+            .ok_or_else(|| format!("bad --timeout: expected a positive whole number of seconds, got {timeout_value:?}"))?,
+    );
+    let dpi = flag(args, "--dpi").unwrap_or("36").to_string();
+    parse_dpi(&dpi)?;
     let files = collect(&positional(args));
     if files.is_empty() {
         return Err("check: no PDF files found".into());
     }
-    let timeout = Duration::from_secs(flag(args, "--timeout").unwrap_or("20").parse().map_err(|_| "bad --timeout")?);
-    let dpi = flag(args, "--dpi").unwrap_or("36").to_string();
     let exe = std::env::current_exe().map_err(|e| e.to_string())?;
     let mut results = Vec::new();
     let mut counts: std::collections::BTreeMap<String, usize> = Default::default();

apps/pdfcraft-cli/tests/check.rs

@@ -1,4 +1,5 @@
 //! The robustness harness visits each directory once, including symlink aliases and cycles.
+//! Its `--timeout` and `--dpi` options are checked before anything is scanned.
 
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
@@ -109,3 +110,22 @@ fn check_follows_directory_and_file_symlinks() {
     assert_eq!(results[0]["status"], "ok");
     assert_eq!(results[0]["file"], file_alias.to_string_lossy().as_ref());
 }
+
+/// `--timeout` and `--dpi` are checked before any input is scanned: a zero timeout used to
+/// make every file time out at once, and a dpi that is not a positive finite number used to
+/// reach each child, which printed nothing and was reported as a crash per file.
+#[test]
+fn a_bad_timeout_or_dpi_is_refused_before_scanning() {
+    let out = Command::new(BIN).args(["check", "no-such-input", "--timeout", "0"]).output().unwrap();
+    assert_eq!(out.status.code(), Some(1));
+    assert_eq!(out.stderr, b"pdfcraft-cli: bad --timeout: expected a positive whole number of seconds, got \"0\"\n");
+    for value in ["abc", "0", "-3", "NaN"] {
+        let out = Command::new(BIN).args(["check", "no-such-input", "--dpi", value]).output().unwrap();
+        assert_eq!(out.status.code(), Some(1), "--dpi {value}");
+        assert_eq!(
+            out.stderr,
+            format!("pdfcraft-cli: bad --dpi: expected a positive number of dots per inch, got {value:?}\n").as_bytes(),
+            "--dpi {value}"
+        );
+    }
+}

apps/pdfcraft-cli/tests/edit.rs

@@ -141,9 +141,9 @@ mod one_based_arguments {
             for destination in ["output.pam", "new.pam", "input.pdf"] {
                 std::fs::write(dir.0.join("output.pam"), b"keep destination").unwrap();
                 for (command, option, value, diagnostic) in [
-                    ("render", "--page", "0", "--page must be at least 1"),
-                    ("edit", "--move", "1:0", "--move target must be at least 1"),
-                    ("edit", "--insert-blank", "0", "--insert-blank must be at least 1"),
+                    ("render", "--page", "0", "bad --page: expected a positive page number"),
+                    ("edit", "--move", "1:0", "bad --move target: expected a page number of at least 1, got \"0\""),
+                    ("edit", "--insert-blank", "0", "bad --insert-blank position: expected a page position of at least 1, got \"0\""),
                 ] {
                     let out = Command::new(BIN).current_dir(&dir.0).args([command, input, "--out", destination, option, value]).output().unwrap();
                     assert_eq!(out.status.code(), Some(1), "{command} {input} {option} {value}");
@@ -194,6 +194,33 @@ mod one_based_arguments {
     }
 }
 
+/// A value the option cannot hold names the option and the value, and fails before the input
+/// file is opened: a missing input still gets the option's own message.
+#[test]
+fn a_value_an_option_cannot_hold_is_named_before_the_file_is_opened() {
+    let dir = tmp("values", "");
+    for (option, value, diagnostic) in [
+        ("--rotate", "1:abc", "bad --rotate degrees: expected a whole number of degrees, got \"abc\""),
+        ("--rotate", "1:45", "bad --rotate degrees: expected a multiple of 90 (such as 90, -90 or 180), got 45"),
+        ("--rotate", "1:-45", "bad --rotate degrees: expected a multiple of 90 (such as 90, -90 or 180), got -45"),
+        ("--move", "1:abc", "bad --move target: expected a page number of at least 1, got \"abc\""),
+        ("--move", "1:0", "bad --move target: expected a page number of at least 1, got \"0\""),
+        ("--insert-blank", "abc", "bad --insert-blank position: expected a page position of at least 1, got \"abc\""),
+        ("--insert-blank", "0", "bad --insert-blank position: expected a page position of at least 1, got \"0\""),
+    ] {
+        let out = Command::new(BIN).current_dir(&dir).args(["edit", "missing.pdf", "--out", "out.pdf", option, value]).output().unwrap();
+        assert_eq!(out.status.code(), Some(1), "{option} {value}");
+        assert_eq!(out.stderr, format!("pdfcraft-cli: {diagnostic}\n").as_bytes(), "{option} {value}");
+        assert!(out.stdout.is_empty(), "{option} {value}");
+        assert!(!dir.join("out.pdf").exists(), "{option} {value}: no output may be written");
+    }
+    // A negative multiple of 90 is still a rotation the engine accepts: the refusal above is
+    // the multiple-of-90 rule, not a sign problem (the missing input is the next error).
+    let out = Command::new(BIN).current_dir(&dir).args(["edit", "missing.pdf", "--out", "out.pdf", "--rotate", "1:-90"]).output().unwrap();
+    assert_eq!(out.status.code(), Some(1));
+    assert!(!String::from_utf8_lossy(&out.stderr).contains("--rotate degrees"), "{}", String::from_utf8_lossy(&out.stderr));
+}
+
 /// A destination past the end must fail, not silently become an append (#1330).
 #[test]
 fn a_destination_past_the_end_fails_and_writes_nothing() {

apps/pdfcraft-cli/tests/render.rs

@@ -88,3 +88,27 @@ fn an_unknown_extension_fails_and_writes_nothing() {
     assert!(stderr.contains(".png, .jpg, .tif or .pam"), "{stderr}");
     assert!(!out.exists());
 }
+
+/// `--page` and `--dpi` are checked before the file is opened. A page that is not a positive
+/// whole number names the same expectation `text` names; a dpi that parses as an `f32` but
+/// cannot be rendered (`0`, a negative value, `NaN`) names the option and the value, instead
+/// of reaching the renderer as a scale it refuses with its own internal message.
+#[test]
+fn a_page_or_dpi_the_renderer_cannot_use_is_refused_before_reading_the_file() {
+    for (option, value, diagnostic) in [
+        ("--page", "abc", "bad --page: expected a positive page number"),
+        ("--page", "0", "bad --page: expected a positive page number"),
+        ("--dpi", "abc", "bad --dpi: expected a positive number of dots per inch, got \"abc\""),
+        ("--dpi", "0", "bad --dpi: expected a positive number of dots per inch, got \"0\""),
+        ("--dpi", "-3", "bad --dpi: expected a positive number of dots per inch, got \"-3\""),
+        ("--dpi", "NaN", "bad --dpi: expected a positive number of dots per inch, got \"NaN\""),
+        ("--dpi", "inf", "bad --dpi: expected a positive number of dots per inch, got \"inf\""),
+    ] {
+        let out = tmp("refuse", "out.png");
+        // `missing.pdf` is never read: the option's own message is the error.
+        let r = Command::new(BIN).args(["render", "missing.pdf", "--out", out.to_str().unwrap(), option, value]).output().unwrap();
+        assert_eq!(r.status.code(), Some(1), "{option} {value}");
+        assert_eq!(r.stderr, format!("pdfcraft-cli: {diagnostic}\n").as_bytes(), "{option} {value}");
+        assert!(!out.exists(), "{option} {value}: no output may be written");
+    }
+}

apps/pdfcraft-cli/tests/split.rs

@@ -1,4 +1,5 @@
 //! `split --out-dir DIR` creates DIR when it does not exist yet (#249), and says so when it can't.
+//! `--every` must be a positive whole number of pages, checked before the file is opened.
 
 use std::path::PathBuf;
 use std::process::{Command, Output};
@@ -74,3 +75,22 @@ fn an_out_dir_that_cannot_be_created_is_reported_as_such() {
     assert_eq!(std::fs::read(&not_a_dir).unwrap(), b"in the way");
     let _ = std::fs::remove_dir_all(&dir);
 }
+
+/// `--every` must be a positive whole number of pages. `0` parses, but the engine used to
+/// clamp it to 1 silently; refused here, before the file is opened, it cannot surprise anyone.
+#[test]
+fn a_non_positive_or_unparseable_every_is_refused_before_the_file_is_opened() {
+    let dir = workdir("every");
+    for value in ["0", "abc"] {
+        let out_dir = dir.join("parts");
+        let out = Command::new(BIN).args(["split", "missing.pdf", "--every", value, "--out-dir", out_dir.to_str().unwrap()]).output().unwrap();
+        assert_eq!(out.status.code(), Some(1), "--every {value}");
+        assert_eq!(
+            out.stderr,
+            format!("pdfcraft-cli: bad --every: expected a positive whole number of pages, got {value:?}\n").as_bytes(),
+            "--every {value}"
+        );
+        assert!(!out_dir.exists(), "--every {value}: nothing may be written");
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+}

contributors/people.toml

@@ -153,3 +153,6 @@ sync_github_name = true
 real_name = "Eric Kaden"
 display_name = "Eric Kaden"
 sync_github_name = false
+
+[people.Bode512]
+sync_github_name = true