From e8a04657fb3e3097fa4857a53303882a2323165e Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:29:29 +0000 Subject: [PATCH] fix(help): two the flag column got wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both against the column that just landed, and both found by review. **A declared name is not a short form.** A flag may carry a name the forms do not imply — `jobs: -j --parallel` — and the split treated that whole prefix as the thing to put a comma after. The joined string is already wider than the column being padded to, so the padding did nothing and the space vanished: jobs: -j,--parallel Only a bare `-x` goes in the short column now; anything else keeps the shape it had. **A description of only spaces is no description.** `usage-argv` filters a blank one wherever it reads one; the template asked only whether the string was *there*, so `help=" "` bought a column of padding and a line of trailing spaces on one side and nothing on the other — two renderings of one spec, which is exactly what the gate exists to prevent and could not see, because mise's spec has no such flag. Normalised where the docs model is built, so every renderer downstream gets the same answer. Found by Cursor Bugbot and greptile. Co-Authored-By: Claude Opus 5 --- argv/src/help.rs | 12 ++++++++- conformance/tests/flag_column.rs | 44 ++++++++++++++++++++++++++++++++ lib/src/docs/cli/mod.rs | 32 +++++++++++++++++++++++ lib/src/docs/models.rs | 25 +++++++++++++++--- 4 files changed, 109 insertions(+), 4 deletions(-) diff --git a/argv/src/help.rs b/argv/src/help.rs index 97a64eaf1..3559926a4 100644 --- a/argv/src/help.rs +++ b/argv/src/help.rs @@ -469,7 +469,17 @@ fn column_usage(meta: &FlagMeta<'_>) -> String { return rest; }; let (before, after) = rest.split_at(at); - let short = match before.trim() { + let short = before.trim(); + // Only a bare short form belongs in the short column. A flag may carry a declared name the + // forms do not imply — `jobs: -j --parallel` — and that prefix is not something to line up + // with a comma after: it rendered `jobs: -j,--parallel`, losing the space entirely, because + // the glued string is already wider than the column. + let bare_short = short.is_empty() + || (short.starts_with('-') && !short.starts_with("--") && short.chars().count() == 2); + if !bare_short { + return rest; + } + let short = match short { "" => String::new(), s => format!("{s},"), }; diff --git a/conformance/tests/flag_column.rs b/conformance/tests/flag_column.rs index bb952ecd5..036f4ec8a 100644 --- a/conformance/tests/flag_column.rs +++ b/conformance/tests/flag_column.rs @@ -131,3 +131,47 @@ fn the_fields_are_bound() { assert!(ex.github_release && ex.dry_run && ex.describe); assert_eq!(ex.output.as_deref(), Some("o")); } + +/// A flag whose declared name the forms do not imply, and one whose help says nothing +#[derive(Cli)] +#[usage(bin = "odd")] +struct Odd { + /// How many at once + #[usage(name = "jobs", long = "parallel", short = 'j')] + parallel: Option, + /// A description made only of spaces is no description + #[usage(long, help = " ")] + blank: bool, +} + +#[test] +fn a_declared_name_is_not_mistaken_for_a_short_form() { + // `jobs: -j --parallel` — the prefix is the flag's *name*, not something to line a comma up + // after. Gluing one on lost the space entirely and rendered `jobs: -j,--parallel`, because + // the joined string is already wider than the column it was being padded to. + let page = usage_argv::help::render(Odd::spec(), Odd::spec().root.cmd, false).expect("a page"); + assert!(page.contains("jobs: -j --parallel"), "{page}"); + assert!(!page.contains("-j,--parallel"), "{page}"); +} + +#[test] +fn a_description_of_only_spaces_is_no_description() { + // Filtered wherever a description is read, so it does not buy a column of padding and a + // line of trailing spaces. usage-lib normalises it in the docs model for the same reason — + // one blank spec, two renderings, was a parity break waiting to be found. + let page = usage_argv::help::render(Odd::spec(), Odd::spec().root.cmd, false).expect("a page"); + let line = page + .lines() + .find(|l| l.contains("--blank")) + .unwrap_or_else(|| panic!("{page}")); + assert_eq!(line, line.trim_end(), "trailing space on {line:?}"); +} + +#[test] +fn the_odd_fields_are_bound() { + use std::ffi::OsStr; + let argv = ["-j", "4", "--blank"].map(OsStr::new); + let odd = Odd::parse_from(&argv).expect("should parse"); + assert_eq!(odd.parallel.as_deref(), Some("4")); + assert!(odd.blank); +} diff --git a/lib/src/docs/cli/mod.rs b/lib/src/docs/cli/mod.rs index 5d6fbd1d9..05e4417de 100644 --- a/lib/src/docs/cli/mod.rs +++ b/lib/src/docs/cli/mod.rs @@ -84,6 +84,38 @@ mod tests { use super::*; use insta::assert_snapshot; + #[test] + fn a_description_of_only_spaces_is_no_description() { + // `usage-argv` filters a blank description wherever it reads one, and this template + // asked only whether the string was there — so `help=" "` bought a column of padding + // and a line of trailing spaces here and nothing there. Two renderings of one spec. + // + // Asserted on the trailing whitespace rather than by comparing the two renderers, so + // the test says what is wrong with the line rather than only that they disagree. + let spec = crate::spec! { r#" +bin "ex" +flag "--blank" help=" " +flag "--plain" help="plain" + "# } + .unwrap(); + + for long in [false, true] { + let page = super::render_help(&spec, &spec.cmd, long); + // In the flags section, not the usage line — `Usage: ex [--blank] [--plain]` + // also contains the name and has no padding to get wrong. + let listing = page.split_once("\nFlags:").expect("a flags section").1; + let line = listing + .lines() + .find(|l| l.contains("--blank")) + .unwrap_or_else(|| panic!("long={long}: {page}")); + assert_eq!( + line, + line.trim_end(), + "long={long}: trailing space on {line:?}" + ); + } + } + #[test] fn test_render_help_omits_hidden_entries() { let spec = crate::spec! { r#" diff --git a/lib/src/docs/models.rs b/lib/src/docs/models.rs index 20e6e6836..5770f7dcc 100644 --- a/lib/src/docs/models.rs +++ b/lib/src/docs/models.rs @@ -521,6 +521,16 @@ impl From<&crate::SpecCommand> for SpecCommand { } } +/// Help text, with whitespace-only treated as none. +/// +/// `usage-argv` filters a blank description out everywhere it reads one, so a spec written with +/// `help=" "` produced a padded column and a line of trailing spaces here and nothing there — +/// two renderings of the same metadata. Normalised once, where the model is built, so every +/// renderer downstream sees the same answer. +fn said(help: &Option) -> Option { + help.as_ref().filter(|h| !h.trim().is_empty()).cloned() +} + /// The width of the short column: `-x, `, or the blank that stands in for it. /// /// Fixed, because a short form is one character. clap's, measured. @@ -560,7 +570,16 @@ fn column_usage(flag: &crate::SpecFlag) -> String { return rest; }; let (before, after) = rest.split_at(at); - let short = match before.trim() { + let short = before.trim(); + // Only a bare short form belongs in the short column — see the twin in `usage-argv`. A + // declared name the forms do not imply (`jobs: -j --parallel`) is not one, and gluing a + // comma to it lost the space before the long form. + let bare_short = short.is_empty() + || (short.starts_with('-') && !short.starts_with("--") && short.chars().count() == 2); + if !bare_short { + return rest; + } + let short = match short { "" => String::new(), s => format!("{s},"), }; @@ -574,7 +593,7 @@ impl From<&crate::SpecFlag> for SpecFlag { effect: flag.effect, usage: flag.usage.clone(), display_usage: column_usage(flag), - help: flag.help.clone(), + help: said(&flag.help), help_long: flag.help_long.clone(), help_md: flag.help_md.clone(), help_first_line: flag.help_first_line.clone(), @@ -618,7 +637,7 @@ impl From<&crate::SpecArg> for SpecArg { Self { name: arg.name.clone(), usage: arg.usage.clone(), - help: arg.help.clone(), + help: said(&arg.help), help_long: arg.help_long.clone(), help_md: arg.help_md.clone(), help_first_line: arg.help_first_line.clone(),