Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ls: Limit value of --width to maximum value if overflowing #5074

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

Skryptonyte
Copy link
Contributor

This change will clamp --width value to u16::MAX if the number passed is too large. The present behaviour results in invalid line width error. This lets gnu/tests/ls/w-option.sh progress a bit further till the last test.

The actual correct behaviour would have been to set the limit to u64::MAX as per the GNU test but all the logic related to formatting seems to be heavily reliant on u16 values, including one crate terminal_size and that would probably take a while for me to sift through. In all practical cases, this should be indistinguishable unless you are printing on a billboard maybe.

@@ -800,12 +800,24 @@ impl Config {
// Read number as octal
match u16::from_str_radix(x, 8) {
Ok(v) => v,
Err(_) => return Err(LsError::InvalidLineWidth(x.into()).into()),
Err(err) => {
Copy link
Contributor

@sylvestre sylvestre Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about moving this into a function like:


fn parse_width(x: &str, is_octal: bool) -> Result<u16, LsError> {
    let parse_result = if x.starts_with('0') && x.len() > 1 {
        u16::from_str_radix(x, 8)
    } else {
        x.parse::<u16>()
    };

    match parse_result {
        Ok(v) => Ok(v),
        Err(err) => match err {
            ParseIntError::Overflow => {
                eprintln!("The width value provided is too large. It has been set to the maximum possible value.");
                Ok(u16::MAX)
            },
            _ => Err(LsError::InvalidLineWidth(x.into()).into()),
        }
    }
}

Copy link
Contributor Author

@Skryptonyte Skryptonyte Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought what I did looked pretty ugly. This should look much cleaner now.

@sylvestre
Copy link
Contributor

indeed, fails a bit later now:


2023-07-12T18:46:36.0409766Z FAIL: tests/ls/w-option
2023-07-12T18:46:36.0409857Z =======================
2023-07-12T18:46:36.0409864Z 
2023-07-12T18:46:36.0410127Z ls: invalid line width: '-1'
2023-07-12T18:46:36.0410299Z ls: invalid line width: '08'
2023-07-12T18:46:36.0410384Z a
2023-07-12T18:46:36.0410466Z b
2023-07-12T18:46:36.0410615Z �[1;32ma�[0m  b  exp  out
2023-07-12T18:46:36.0410813Z --- exp	2023-07-12 18:36:23.974039937 +0000
2023-07-12T18:46:36.0410981Z +++ out	2023-07-12 18:36:23.978039942 +0000
2023-07-12T18:46:36.0411113Z @@ -1 +1,2 @@
2023-07-12T18:46:36.0411225Z -a  b
2023-07-12T18:46:36.0411309Z +a
2023-07-12T18:46:36.0411391Z +b
2023-07-12T18:46:36.0411590Z FAIL tests/ls/w-option.sh (exit status: 1)

@sylvestre sylvestre merged commit bd4ecb6 into uutils:main Jul 12, 2023
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants