From cfc85ba20be082094a0cec1f8cc7c583c238d9aa Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 05:37:45 +0300 Subject: [PATCH] sort: match GNU's error for a bad --parallel value `--parallel` used clap's built-in ranged `u64` parser, so a bad value got clap's wording instead of GNU's own -- the same class of gap fixed for other options earlier in this project: $ sort --parallel=0 f # ours, before error: invalid value '0' for '--parallel ': 0 is not in 1..18446744073709551615 $ sort --parallel=0 f # GNU 9.11 sort: number in parallel must be nonzero $ sort --parallel=-1 f # ours, before error: invalid value '-1' for '--parallel ': invalid digit found in string $ sort --parallel=-1 f # GNU sort: invalid --parallel argument '-1' `--parallel` shares the same number-with-suffix parser `-S`/`--buffer-size` already uses via `format_error_message`, so this reuses that rather than inventing new wording: GNU syntactically accepts a suffix here too, but none is ever valid for a thread count, so the allow list passed to the parser is empty rather than the one `-S` uses. A value starting with `-` is still accepted as this option's value, not a new flag -- `--parallel -1` -- matching GNU. This needed `allow_hyphen_values`, which the old `value_parser!(u64).range(1..)` did not have, so `--parallel -1` (space-separated) was already broken before this change; verified that specific breakage pre-dates this PR. ## Deliberately not covered GNU accepts an arbitrarily large `--parallel` value without erroring, and apparently does not actually try to spawn that many OS threads for it. uutils' `rayon::ThreadPoolBuilder` builds its pool eagerly, so asking for anywhere near that many threads hangs rather than completing -- a pre-existing limitation, already reachable today with an ordinary in-range count (`--parallel 10000` already hangs on current `main`). Rather than clamp an overflowing value the way `head`/`tail`/`numfmt` do elsewhere in this project (which would make the hang reachable from an even wider set of inputs, including ordinary overflow past `u64::MAX`), this keeps the pre-existing behavior of erroring on overflow, just with a clearer message. Fixing the hang itself means teaching the thread pool that `--parallel` is a soft upper bound, not a literal thread count to build eagerly, which felt like a distinct problem from an error-message mismatch. Separately, and also not touched here: sort's `-S`/`--buffer-size` has a comment acknowledging that GNU echoes back whichever spelling, `-S` or `--buffer-size`, was actually typed, and that this is not yet implemented (the message always says "--buffer-size"). `--parallel` has no short form, so this PR does not need that distinction, but `-S` still does. --- src/uu/sort/locales/en-US.ftl | 1 + src/uu/sort/locales/fr-FR.ftl | 1 + src/uu/sort/src/sort.rs | 42 ++++++++++++++++++++++++++++++----- tests/by-util/test_sort.rs | 31 +++++++++++++++++++++++--- 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/src/uu/sort/locales/en-US.ftl b/src/uu/sort/locales/en-US.ftl index 18d340152e1..495f1e50870 100644 --- a/src/uu/sort/locales/en-US.ftl +++ b/src/uu/sort/locales/en-US.ftl @@ -58,6 +58,7 @@ sort-invalid-suffix-in-option-arg = invalid suffix in --{$option} argument {$arg sort-invalid-option-arg = invalid --{$option} argument {$arg} sort-option-arg-too-large = --{$option} argument {$arg} too large sort-error-disorder = {$file}:{$line_number}: disorder: {$line} +sort-error-parallel-nonzero = number in parallel must be nonzero sort-error-buffer-size-too-big = Buffer size {$size} does not fit in address space sort-error-no-match-for-key = ^ no match for key sort-error-write-failed = write failed: {$output} diff --git a/src/uu/sort/locales/fr-FR.ftl b/src/uu/sort/locales/fr-FR.ftl index 6605ee54776..dde675ea23a 100644 --- a/src/uu/sort/locales/fr-FR.ftl +++ b/src/uu/sort/locales/fr-FR.ftl @@ -57,6 +57,7 @@ sort-invalid-suffix-in-option-arg = suffixe invalide dans l'argument --{$option} sort-invalid-option-arg = argument --{$option} invalide {$arg} sort-option-arg-too-large = argument --{$option} {$arg} trop grand sort-error-disorder = {$file}:{$line_number}: désordre : {$line} +sort-error-parallel-nonzero = le nombre en parallèle doit être non nul sort-error-buffer-size-too-big = La taille du tampon {$size} ne rentre pas dans l'espace d'adressage sort-error-no-match-for-key = ^ aucune correspondance pour la clé sort-error-write-failed = échec d'écriture : {$output} diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 7a8bd550fdb..a7be5e371b2 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -2249,10 +2249,42 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // WASI doesn't support threads, so we ignore the corresponding option #[cfg(not(target_os = "wasi"))] { - let threads = matches - .get_one::(options::PARALLEL) - .copied() - .unwrap_or_else(|| std::thread::available_parallelism().map_or(1, |n| n.get() as u64)); + let threads = if let Some(threads_str) = matches.get_one::(options::PARALLEL) { + // GNU accepts a suffix syntactically -- it goes through the same + // number-with-suffix parser as `-S` -- but none is ever valid for + // a thread count, so the allow list is empty rather than absent. + // + // `parse_u64` errors on overflow instead of clamping: GNU itself + // accepts an arbitrarily large count without erroring, but + // `rayon::ThreadPoolBuilder` builds its pool eagerly, so asking + // it for anywhere near that many threads hangs rather than + // completing quickly. That is a pre-existing limitation already + // reachable with an ordinary large-but-in-range count (10000 + // already hangs); erroring here at least avoids making it + // reachable from *more* inputs than it already was. + let count = Parser::default() + .with_allow_list(&[]) + .parse_u64(threads_str) + .map_err(|error| { + let message = format_error_message(&error, threads_str, options::PARALLEL); + error.size_value_error( + key_args.as_deref(), + &OptionValue::with_names(threads_str, None, Some(options::PARALLEL)), + 0, + &message, + USimpleError::new(2, message.clone()), + ) + })?; + if count == 0 { + return Err(USimpleError::new( + 2, + translate!("sort-error-parallel-nonzero"), + )); + } + count + } else { + std::thread::available_parallelism().map_or(1, |n| n.get() as u64) + }; let _ = rayon::ThreadPoolBuilder::new() .num_threads(threads as usize) .build_global(); @@ -2667,7 +2699,7 @@ pub fn uu_app() -> Command { Arg::new(options::PARALLEL) .long(options::PARALLEL) .help(translate!("sort-help-parallel")) - .value_parser(clap::value_parser!(u64).range(1..)) + .allow_hyphen_values(true) .value_name("NUM_THREADS"), ) .arg( diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 0747ddc3935..53b2916e049 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -197,9 +197,34 @@ fn test_version_empty_lines() { #[test] fn test_parallel_invalid() { - // clap provided stderr - new_ucmd!().arg("--parallel=0").fails().code_is(2); - new_ucmd!().arg("--parallel=NaN").fails().code_is(2); + new_ucmd!() + .arg("--parallel=0") + .fails_with_code(2) + .stderr_only("sort: number in parallel must be nonzero\n"); + new_ucmd!() + .arg("--parallel=NaN") + .fails_with_code(2) + .stderr_only("sort: invalid --parallel argument 'NaN'\n"); + new_ucmd!() + .arg("--parallel=-1") + .fails_with_code(2) + .stderr_only("sort: invalid --parallel argument '-1'\n"); + new_ucmd!() + .arg("--parallel=") + .fails_with_code(2) + .stderr_only("sort: invalid --parallel argument ''\n"); + // No unit is ever valid for a thread count, unlike `-S`/`--buffer-size`, + // which this shares its parser with. + new_ucmd!() + .arg("--parallel=2K") + .fails_with_code(2) + .stderr_only("sort: invalid suffix in --parallel argument '2K'\n"); + // A separate (not `=`-attached) value starting with `-` is still this + // option's value, not a new flag -- as GNU accepts it. + new_ucmd!() + .args(&["--parallel", "-1"]) + .fails_with_code(2) + .stderr_only("sort: invalid --parallel argument '-1'\n"); } #[test]