bug(ls): when --tabsize is a negative number ls successeds instead of rejecting it by Devel08 · Pull Request #13278 · uutils/coreutils

Devel08 · 2026-07-05T11:45:33Z

this PR resolves #12835
Whenever the value from --tabsize can't be parsed, it'll now throw an error, which message is identical to GNU's one. error code is 2, like in GNU, hexadecimal values are accepted

for more information, see https://pre-commit.ci

sylvestre · 2026-07-05T11:55:49Z

some jobs are still failing

Devel08 · 2026-07-05T11:56:10Z

@sylvestre yeah, that because of clippy, im fixing them rn

codspeed-hq · 2026-07-05T12:13:38Z

Merging this PR will not alter performance

✅ 331 untouched benchmarks
⏩ 46 skipped benchmarks¹

_{Comparing Devel08:fix-issue-12835 (0b41ec4) with main (62f307c)}

46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Devel08 · 2026-07-05T12:38:41Z

@sylvestre ready for review :)

github-actions · 2026-07-05T12:47:10Z

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/retry (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)

sylvestre · 2026-07-05T12:56:46Z

+                    } else {
+                        return Err(Box::new(LsError::InvalidTabSize(size_str.clone())));
+                    }
+                }


it should be moved in a function
and it should look more like (ie without the Ok())

fn parse_tab_size(size_str: &str) -> Result<usize, Box<LsError>> { size_str .parse::<usize>() .ok() .or_else(|| { size_str .strip_prefix("0x") .or_else(|| size_str.strip_prefix("0X")) .and_then(|hex| usize::from_str_radix(hex, 16).ok()) }) .ok_or_else(|| Box::new(LsError::InvalidTabSize(size_str.to_string()))) }

Alright, I'm going to use that function, thanks!

for more information, see https://pre-commit.ci

sylvestre · 2026-07-05T13:42:17Z

 }

 impl Config {
+    fn parse_tab_size(size_str: &str) -> Result<usize, Box<LsError>> {


why in impl Config { ?

oops, i'll move it under parse_width, sorry

sylvestre · 2026-07-05T13:45:13Z

+            match Self::parse_tab_size(size_str) {
+                Ok(n) => Some(n),
+                Err(e) => return Err(e),
+            }
        } else {
-            options
-                .get_one::<String>(options::format::TAB_SIZE)
-                .and_then(|size| size.parse::<usize>().ok())
-                .or_else(|| std::env::var("TABSIZE").ok().and_then(|s| s.parse().ok()))
-        }
-        .unwrap_or(SPACES_IN_TAB);
+            None
+        };


something like ?

Suggested change

match Self::parse_tab_size(size_str) {

Ok(n) => Some(n),

Err(e) => return Err(e),

}

} else {

options

.get_one::<String>(options::format::TAB_SIZE)

.and_then(|size| size.parse::<usize>().ok())

.or_else(|| std::env::var("TABSIZE").ok().and_then(|s| s.parse().ok()))

}

.unwrap_or(SPACES_IN_TAB);

None

};

Some(Self::parse_tab_size(size_str)?

};

We can't directly propagate LsError, can we?

sylvestre · 2026-07-05T13:45:52Z


    scene.ucmd().args(&["-T", "3"]).succeeds();
    scene.ucmd().args(&["--tabsize", "0"]).succeeds();
+    scene.ucmd().arg("--tabsize=-3").fails();


verify the error message
and also add a string and a decimal for --tabsize=

* tests/ls/ls-misc.pl (tabsize-negative, tabsize-non-numeric): New tests verifying that a negative or non-numeric --tabsize argument is rejected with "invalid tab size" and exit status 2. uutils/coreutils#13278

sylvestre · 2026-07-05T14:20:36Z

        let tab_size = if needs_color {
            Some(0)
+        } else if let Some(size_str) = options.get_one::<String>(options::format::TAB_SIZE) {
+            match parse_tab_size(size_str) {


this needs to be improve too

we can use match to check if its none or not, but I think that's the most readable way, should I do it with match?

} else if let Some(size_str) = options.get_one::<String>(options::format::TAB_SIZE) { Some(parse_tab_size(size_str)?) } else {

is the same

yeah, but that can't be done, as parse_tab_size returns an LsError and it cannot be propagated to from()

* tests/ls/ls-misc.pl: Add tests, generated by a loop over invalid values, verifying that a negative (-9), non-numeric (zz), or fractional (0.5) --tabsize argument is rejected with "invalid tab size" and exit status 2. uutils/coreutils#13278

sylvestre · 2026-07-05T16:15:01Z

#13283 as follow up

oech3 · 2026-07-05T16:35:23Z

Why do we have our own parser at here instead of clap's validation?

Devel08 and others added 7 commits July 5, 2026 13:59

ls: throw error when tab size option can't be parsed

5f14789

ls: add invalid tab size error

b795dad

add invalid tabsize msg to locale

73efa36

add negative tabsize test

db4bc66

allow hexadecimal numbers for tabsize

b4dd8d7

[pre-commit.ci] auto fixes from pre-commit.com hooks

fbf2e32

for more information, see https://pre-commit.ci

fmt

c6ff362

Devel08 added 2 commits July 5, 2026 14:59

clippy

4c62b1e

fmt

f87f796

sylvestre reviewed Jul 5, 2026

View reviewed changes

Devel08 and others added 2 commits July 5, 2026 16:40

parse the tab size in seperate function

af10173

[pre-commit.ci] auto fixes from pre-commit.com hooks

406bde5

for more information, see https://pre-commit.ci

sylvestre reviewed Jul 5, 2026

View reviewed changes

Devel08 added 4 commits July 5, 2026 16:48

move parse tab size func outside config

ed085bf

identation

e1ba921

add floating point and string tests in test_tabsize_option

8d60ce9

Update test_ls.rs

0b41ec4

sylvestre mentioned this pull request Jul 5, 2026

tests: ls: check that an invalid --tabsize is diagnosed coreutils/coreutils#311

Open

sylvestre reviewed Jul 5, 2026

View reviewed changes

sylvestre merged commit a1763bf into uutils:main Jul 5, 2026
177 of 178 checks passed

Uh oh!

Uh oh!

Conversation

Devel08 commented Jul 5, 2026

Uh oh!

sylvestre commented Jul 5, 2026

Uh oh!

Devel08 commented Jul 5, 2026

Uh oh!

codspeed-hq Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Devel08 commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sylvestre commented Jul 5, 2026

Uh oh!

oech3 commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq Bot commented Jul 5, 2026 •

edited

Loading

github-actions Bot commented Jul 5, 2026 •

edited

Loading

sylvestre Jul 5, 2026 •

edited

Loading