From fd24497be5174834d439d6bb3b36b42707438f3c Mon Sep 17 00:00:00 2001 From: Logic769 Date: Fri, 30 Oct 2026 04:22:22 +0800 Subject: [PATCH 1/4] ls: validate --block-size before -h/--si overrides it --- src/uu/ls/src/config.rs | 24 ++++++++++++++++-------- src/uu/ls/src/ls.rs | 38 +++++++++++++++++++++++++++++++++----- tests/by-util/test_ls.rs | 15 +++++++++++++++ 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index 29f4d9415af..e2ce1d1f96a 100644 --- a/src/uu/ls/src/config.rs +++ b/src/uu/ls/src/config.rs @@ -736,14 +736,15 @@ impl Config { }; let (file_size_block_size, block_size) = if let Some(opt_block_size) = opt_block_size { - // --block-size command-line argument: parse it, error on invalid - // If --block-size=si or --block-size=human-readable, skip numeric parsing - if opt_si { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) - } else if opt_hr { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) + // Validate a numeric --block-size even when a later -h/--si changes + // the display format: GNU ls always rejects an invalid block size + // argument (e.g. --block-size=0), regardless of option order. + let numeric_block_size = if opt_block_size == options::size::SI + || opt_block_size == options::size::HUMAN_READABLE + { + None } else { - let size = parse_size_non_zero_u64(opt_block_size).map_err(|error| { + Some(parse_size_non_zero_u64(opt_block_size).map_err(|error| { let ls_error = LsError::BlockSizeParseError(opt_block_size.clone()); let message = ls_error.to_string(); error.size_value_error( @@ -757,8 +758,15 @@ impl Config { &message, ls_error, ) - })?; + })?) + }; + if opt_si { + (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) + } else if opt_hr { + (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) + } else { // --block-size overrides -k + let size = numeric_block_size.unwrap(); (size, size) } } else if !opt_si && !opt_hr { diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c7e6fd48804..0042489cccf 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -37,6 +37,7 @@ use uucore::{ fs::FileInformation, fsext::metadata_get_time, os_str_as_bytes_lossy, + parser::parse_size::parse_size_non_zero_u64, parser::shortcut_value_parser::ShortcutValueParser, show, translate, version_cmp::version_cmp, @@ -113,15 +114,42 @@ impl UError for LsError { } } +/// Reject an invalid numeric value for `--block-size` even when a later +/// `-h`/`--si`/another `--block-size` makes clap drop the value from the +/// matches. GNU ls validates the argument as it is parsed, independent of +/// option order (e.g. `ls --block-size=0 --si` is an error). +fn validate_block_size_arguments(args: &[OsString]) -> UResult<()> { + let mut iter = args.iter().skip(1); + while let Some(arg) = iter.next() { + let Some(arg) = arg.to_str() else { + continue; + }; + let value = if let Some(value) = arg.strip_prefix("--block-size=") { + Some(value) + } else if arg == "--block-size" { + iter.next().and_then(|value| value.to_str()) + } else { + None + }; + if let Some(value) = value + && value != options::size::SI + && value != options::size::HUMAN_READABLE + && parse_size_non_zero_u64(value).is_err() + { + return Err(LsError::BlockSizeParseError(value.to_string()).into()); + } + } + Ok(()) +} + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { // The arguments are kept for the caret in SIZE diagnostics, which echoes // the command line. - let (matches, diag_args) = uucore::clap_localization::handle_clap_result_with_diagnostics( - uu_app(), - args.collect(), - 2, - )?; + let args: Vec = args.collect(); + validate_block_size_arguments(&args)?; + let (matches, diag_args) = + uucore::clap_localization::handle_clap_result_with_diagnostics(uu_app(), args, 2)?; uucore::i18n::collator::init_locale_collation(); diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 636873d199b..bf439b11ba5 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -6640,6 +6640,21 @@ fn test_ls_invalid_block_size() { .fails_with_code(2) .no_stdout() .stderr_is("ls: invalid --block-size argument '0'\n"); + + // A later -h/--si must not skip validation of an invalid --block-size. + new_ucmd!() + .arg("--block-size=0") + .arg("--si") + .fails_with_code(2) + .no_stdout() + .stderr_is("ls: invalid --block-size argument '0'\n"); + + new_ucmd!() + .arg("--block-size=invalid") + .arg("--human-readable") + .fails_with_code(2) + .no_stdout() + .stderr_is("ls: invalid --block-size argument 'invalid'\n"); } #[cfg(all(unix, feature = "dd"))] From 0baead82ec675226d8a6eba024a2488fede0b62b Mon Sep 17 00:00:00 2001 From: Logic769 Date: Fri, 30 Oct 2026 19:25:21 +0800 Subject: [PATCH 2/4] ls: keep the caret when --block-size is rejected under -h or --si --- src/uu/ls/src/config.rs | 24 +++++++------------- src/uu/ls/src/ls.rs | 50 ++++++++++++++++++++++++++++++----------- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index e2ce1d1f96a..29f4d9415af 100644 --- a/src/uu/ls/src/config.rs +++ b/src/uu/ls/src/config.rs @@ -736,15 +736,14 @@ impl Config { }; let (file_size_block_size, block_size) = if let Some(opt_block_size) = opt_block_size { - // Validate a numeric --block-size even when a later -h/--si changes - // the display format: GNU ls always rejects an invalid block size - // argument (e.g. --block-size=0), regardless of option order. - let numeric_block_size = if opt_block_size == options::size::SI - || opt_block_size == options::size::HUMAN_READABLE - { - None + // --block-size command-line argument: parse it, error on invalid + // If --block-size=si or --block-size=human-readable, skip numeric parsing + if opt_si { + (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) + } else if opt_hr { + (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) } else { - Some(parse_size_non_zero_u64(opt_block_size).map_err(|error| { + let size = parse_size_non_zero_u64(opt_block_size).map_err(|error| { let ls_error = LsError::BlockSizeParseError(opt_block_size.clone()); let message = ls_error.to_string(); error.size_value_error( @@ -758,15 +757,8 @@ impl Config { &message, ls_error, ) - })?) - }; - if opt_si { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) - } else if opt_hr { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) - } else { + })?; // --block-size overrides -k - let size = numeric_block_size.unwrap(); (size, size) } } else if !opt_si && !opt_hr { diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 0042489cccf..5e8720132a3 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -31,6 +31,7 @@ use std::{ #[cfg(unix)] use uucore::libc::{S_IXGRP, S_IXOTH, S_IXUSR}; use uucore::{ + diagnostics::OptionValue, display::Quotable, error::{UError, UResult, set_exit_code, strip_errno}, format_usage, @@ -114,32 +115,51 @@ impl UError for LsError { } } -/// Reject an invalid numeric value for `--block-size` even when a later -/// `-h`/`--si`/another `--block-size` makes clap drop the value from the -/// matches. GNU ls validates the argument as it is parsed, independent of -/// option order (e.g. `ls --block-size=0 --si` is an error). -fn validate_block_size_arguments(args: &[OsString]) -> UResult<()> { +/// The numeric value of `--block-size` when it does not parse. +/// +/// `--block-size` belongs to the size mode group, so a later `-h`/`--si` makes clap +/// drop the value from the matches even though GNU ls still rejects it (e.g. +/// `ls --block-size=0 --si` is an error). The raw arguments are scanned for the value, +/// and the diagnostic is raised once clap has accepted the command line, so that the +/// caret can point at the value as it was typed. +fn invalid_block_size_argument(args: &[OsString]) -> Option { let mut iter = args.iter().skip(1); while let Some(arg) = iter.next() { let Some(arg) = arg.to_str() else { continue; }; let value = if let Some(value) = arg.strip_prefix("--block-size=") { - Some(value) + value } else if arg == "--block-size" { - iter.next().and_then(|value| value.to_str()) + match iter.next().and_then(|value| value.to_str()) { + Some(value) => value, + None => continue, + } } else { - None + continue; }; - if let Some(value) = value - && value != options::size::SI + if value != options::size::SI && value != options::size::HUMAN_READABLE && parse_size_non_zero_u64(value).is_err() { - return Err(LsError::BlockSizeParseError(value.to_string()).into()); + return Some(value.to_string()); } } - Ok(()) + None +} + +/// The report for an invalid `--block-size` value, with the caret under the value. +fn block_size_error(value: &str, diag_args: Option<&[OsString]>) -> Box { + let ls_error = LsError::BlockSizeParseError(value.to_string()); + let message = ls_error.to_string(); + let parse_error = parse_size_non_zero_u64(value).expect_err("the value was rejected above"); + parse_error.size_value_error( + diag_args, + &OptionValue::with_names(value, None, Some(options::size::BLOCK_SIZE)), + 0, + &message, + ls_error, + ) } #[uucore::main] @@ -147,10 +167,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // The arguments are kept for the caret in SIZE diagnostics, which echoes // the command line. let args: Vec = args.collect(); - validate_block_size_arguments(&args)?; + let invalid_block_size = invalid_block_size_argument(&args); let (matches, diag_args) = uucore::clap_localization::handle_clap_result_with_diagnostics(uu_app(), args, 2)?; + if let Some(value) = invalid_block_size { + return Err(block_size_error(&value, diag_args.as_deref())); + } + uucore::i18n::collator::init_locale_collation(); let config = Config::from(&matches, diag_args.as_deref())?; From 9d2bcef34224220e349d4a430337a352a38f49c2 Mon Sep 17 00:00:00 2001 From: Logic769 Date: Tue, 3 Nov 2026 07:44:55 +0800 Subject: [PATCH 3/4] ls: collect --block-size values with clap Validate every --block-size occurrence in config.rs and pick the last size mode option instead of scanning argv by hand. --- src/uu/ls/src/config.rs | 99 ++++++++++++++++++++++++++-------------- src/uu/ls/src/ls.rs | 68 +++------------------------ tests/by-util/test_ls.rs | 9 ++++ 3 files changed, 79 insertions(+), 97 deletions(-) diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index 29f4d9415af..e09ea71fff3 100644 --- a/src/uu/ls/src/config.rs +++ b/src/uu/ls/src/config.rs @@ -20,7 +20,7 @@ use term_grid::SPACES_IN_TAB; use uucore::{ diagnostics::OptionValue, display::Quotable, - error::UResult, + error::{UError, UResult}, format::human::SizeFormat, fsext::MetadataTimeField, line_ending::LineEnding, @@ -622,6 +622,20 @@ fn parse_width(width_match: Option<&String>) -> Result { Ok(ret) } +/// The report for an invalid `--block-size` value, with the caret under the value. +fn block_size_error(value: &str, diag_args: Option<&[OsString]>) -> Box { + let ls_error = LsError::BlockSizeParseError(value.to_string()); + let message = ls_error.to_string(); + let parse_error = parse_size_non_zero_u64(value).expect_err("the value was rejected above"); + parse_error.size_value_error( + diag_args, + &OptionValue::with_names(value, None, Some(options::size::BLOCK_SIZE)), + 0, + &message, + ls_error, + ) +} + /// Parses the tab size value from the command line fn parse_tab_size(size_str: &str) -> Result { size_str @@ -720,53 +734,68 @@ impl Config { let mut needs_color = extract_color(options); let hyperlink = extract_hyperlink(options); - let opt_block_size = options.get_one::(options::size::BLOCK_SIZE); - let opt_si = opt_block_size.is_some_and(|x| x == options::size::SI) - || options.get_flag(options::size::SI); - let opt_hr = opt_block_size.is_some_and(|x| x == options::size::HUMAN_READABLE) - || options.get_flag(options::size::HUMAN_READABLE); - let opt_kb = options.get_flag(options::size::KIBIBYTES); + // Every --block-size value must be valid, even when a later -h/--si + // overrides it (GNU ls rejects all of them). The last of the size mode + // options on the command line decides the display format. + let block_sizes: Vec<(&String, usize)> = options + .get_many::(options::size::BLOCK_SIZE) + .map(|values| values.collect::>()) + .unwrap_or_default() + .into_iter() + .zip( + options + .indices_of(options::size::BLOCK_SIZE) + .into_iter() + .flatten(), + ) + .collect(); + + for (value, _) in &block_sizes { + if value.as_str() != options::size::SI + && value.as_str() != options::size::HUMAN_READABLE + && parse_size_non_zero_u64(value).is_err() + { + return Err(block_size_error(value, diag_args)); + } + } - let size_format = if opt_si { + let mut size_mode: Option<(usize, &str)> = None; + for (value, index) in &block_sizes { + if size_mode.is_none_or(|(latest, _)| *index > latest) { + size_mode = Some((*index, value.as_str())); + } + } + for arg in [options::size::SI, options::size::HUMAN_READABLE] { + if options.get_flag(arg) + && let Some(index) = options.indices_of(arg).and_then(|indices| indices.max()) + && size_mode.is_none_or(|(latest, _)| index > latest) + { + size_mode = Some((index, arg)); + } + } + let size_mode = size_mode.map(|(_, mode)| mode); + + let size_format = if size_mode == Some(options::size::SI) { SizeFormat::Decimal - } else if opt_hr { + } else if size_mode == Some(options::size::HUMAN_READABLE) { SizeFormat::Binary } else { SizeFormat::Bytes }; - let (file_size_block_size, block_size) = if let Some(opt_block_size) = opt_block_size { - // --block-size command-line argument: parse it, error on invalid - // If --block-size=si or --block-size=human-readable, skip numeric parsing - if opt_si { + let (file_size_block_size, block_size) = if let Some(value) = size_mode { + if value == options::size::SI { (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) - } else if opt_hr { + } else if value == options::size::HUMAN_READABLE { (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) } else { - let size = parse_size_non_zero_u64(opt_block_size).map_err(|error| { - let ls_error = LsError::BlockSizeParseError(opt_block_size.clone()); - let message = ls_error.to_string(); - error.size_value_error( - diag_args, - &OptionValue::with_names( - opt_block_size, - None, - Some(options::size::BLOCK_SIZE), - ), - 0, - &message, - ls_error, - ) - })?; - // --block-size overrides -k + // --block-size also overrides -k. + let size = + parse_size_non_zero_u64(value).expect("invalid values were rejected above"); (size, size) } - } else if !opt_si && !opt_hr { - resolve_block_sizes_from_env(opt_kb) - } else if opt_si { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, 1000) } else { - (DEFAULT_FILE_SIZE_BLOCK_SIZE, DEFAULT_BLOCK_SIZE) + resolve_block_sizes_from_env(options.get_flag(options::size::KIBIBYTES)) }; let long = { diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 5e8720132a3..e1f5277ea1c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -31,14 +31,12 @@ use std::{ #[cfg(unix)] use uucore::libc::{S_IXGRP, S_IXOTH, S_IXUSR}; use uucore::{ - diagnostics::OptionValue, display::Quotable, error::{UError, UResult, set_exit_code, strip_errno}, format_usage, fs::FileInformation, fsext::metadata_get_time, os_str_as_bytes_lossy, - parser::parse_size::parse_size_non_zero_u64, parser::shortcut_value_parser::ShortcutValueParser, show, translate, version_cmp::version_cmp, @@ -115,65 +113,13 @@ impl UError for LsError { } } -/// The numeric value of `--block-size` when it does not parse. -/// -/// `--block-size` belongs to the size mode group, so a later `-h`/`--si` makes clap -/// drop the value from the matches even though GNU ls still rejects it (e.g. -/// `ls --block-size=0 --si` is an error). The raw arguments are scanned for the value, -/// and the diagnostic is raised once clap has accepted the command line, so that the -/// caret can point at the value as it was typed. -fn invalid_block_size_argument(args: &[OsString]) -> Option { - let mut iter = args.iter().skip(1); - while let Some(arg) = iter.next() { - let Some(arg) = arg.to_str() else { - continue; - }; - let value = if let Some(value) = arg.strip_prefix("--block-size=") { - value - } else if arg == "--block-size" { - match iter.next().and_then(|value| value.to_str()) { - Some(value) => value, - None => continue, - } - } else { - continue; - }; - if value != options::size::SI - && value != options::size::HUMAN_READABLE - && parse_size_non_zero_u64(value).is_err() - { - return Some(value.to_string()); - } - } - None -} - -/// The report for an invalid `--block-size` value, with the caret under the value. -fn block_size_error(value: &str, diag_args: Option<&[OsString]>) -> Box { - let ls_error = LsError::BlockSizeParseError(value.to_string()); - let message = ls_error.to_string(); - let parse_error = parse_size_non_zero_u64(value).expect_err("the value was rejected above"); - parse_error.size_value_error( - diag_args, - &OptionValue::with_names(value, None, Some(options::size::BLOCK_SIZE)), - 0, - &message, - ls_error, - ) -} - #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - // The arguments are kept for the caret in SIZE diagnostics, which echoes - // the command line. - let args: Vec = args.collect(); - let invalid_block_size = invalid_block_size_argument(&args); - let (matches, diag_args) = - uucore::clap_localization::handle_clap_result_with_diagnostics(uu_app(), args, 2)?; - - if let Some(value) = invalid_block_size { - return Err(block_size_error(&value, diag_args.as_deref())); - } + let (matches, diag_args) = uucore::clap_localization::handle_clap_result_with_diagnostics( + uu_app(), + args.collect(), + 2, + )?; uucore::i18n::collator::init_locale_collation(); @@ -660,7 +606,6 @@ pub fn uu_app() -> Command { .short('h') .long(options::size::HUMAN_READABLE) .help(translate!("ls-help-human-readable")) - .overrides_with_all([options::size::BLOCK_SIZE, options::size::SI]) .action(ArgAction::SetTrue), ) .arg( @@ -674,7 +619,6 @@ pub fn uu_app() -> Command { Arg::new(options::size::SI) .long(options::size::SI) .help(translate!("ls-help-si")) - .overrides_with_all([options::size::BLOCK_SIZE, options::size::HUMAN_READABLE]) .action(ArgAction::SetTrue), ) .arg( @@ -682,7 +626,7 @@ pub fn uu_app() -> Command { .long(options::size::BLOCK_SIZE) .value_name("BLOCK_SIZE") .help(translate!("ls-help-block-size")) - .overrides_with_all([options::size::SI, options::size::HUMAN_READABLE]), + .action(ArgAction::Append), ) .arg( Arg::new(options::INODE) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index bf439b11ba5..4f547dd2908 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -6655,6 +6655,15 @@ fn test_ls_invalid_block_size() { .fails_with_code(2) .no_stdout() .stderr_is("ls: invalid --block-size argument 'invalid'\n"); + + // --block-size and its value can be two separate arguments. + new_ucmd!() + .arg("--block-size") + .arg("0") + .arg("-h") + .fails_with_code(2) + .no_stdout() + .stderr_is("ls: invalid --block-size argument '0'\n"); } #[cfg(all(unix, feature = "dd"))] From f7b9531854cbd413034f0d9f9a12de865d365cae Mon Sep 17 00:00:00 2001 From: Logic769 Date: Tue, 3 Nov 2026 21:32:13 +0800 Subject: [PATCH 4/4] ls: fix clippy redundant closures --- src/uu/ls/src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index e09ea71fff3..d273cc42bc9 100644 --- a/src/uu/ls/src/config.rs +++ b/src/uu/ls/src/config.rs @@ -739,7 +739,7 @@ impl Config { // options on the command line decides the display format. let block_sizes: Vec<(&String, usize)> = options .get_many::(options::size::BLOCK_SIZE) - .map(|values| values.collect::>()) + .map(Iterator::collect::>) .unwrap_or_default() .into_iter() .zip( @@ -767,7 +767,7 @@ impl Config { } for arg in [options::size::SI, options::size::HUMAN_READABLE] { if options.get_flag(arg) - && let Some(index) = options.indices_of(arg).and_then(|indices| indices.max()) + && let Some(index) = options.indices_of(arg).and_then(Iterator::max) && size_mode.is_none_or(|(latest, _)| index > latest) { size_mode = Some((index, arg));