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

Forbid octal numbers for width parameter #3613

Merged
merged 3 commits into from
Jun 13, 2022
Merged

Conversation

Ganneff
Copy link
Contributor

@Ganneff Ganneff commented Jun 10, 2022

This will fix issue #3609

@sylvestre
Copy link
Sponsor Contributor

Nice to see you here ;)

@sylvestre
Copy link
Sponsor Contributor

Could you please add a test? probably tests/by-util/test_ls.rs

@tertsdiepraam
Copy link
Member

Maybe it's also good to add a small comment that GNU supports octal, but uutils doesn't (yet).

@Ganneff
Copy link
Contributor Author

Ganneff commented Jun 11, 2022

Maybe it's also good to add a small comment that GNU supports octal, but uutils doesn't (yet).

I've gone the way I did in code due to the way the ticket was written.

We can alternatively go and, for numbers with a leading 0, use https://doc.rust-lang.org/std/primitive.u16.html#method.from_str_radix with a base of 8 for parsing and support octal numbers?

Not that I can imagine a good reason for wanting to use octal numbers for width, but meh. Which way preferred, error out or support?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Jun 11, 2022

For now, I was just simply suggesting a comment in the code, just so that somebody who reads the code can understand why we don't allow leading zeros, but supporting it would also be good, I think. Ultimately, I guess we do want to support it for compatibility.

@Ganneff
Copy link
Contributor Author

Ganneff commented Jun 12, 2022

For now, I was just simply suggesting a comment in the code, just so that somebody who reads the code can understand why we don't allow leading zeros, but supporting it would also be good, I think. Ultimately, I guess we do want to support it for compatibility.

If the goal is full compatibility, then lets go that way.

} else {
u
Some(x) => {
if x.starts_with('0') && x.len() > 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra check for the len is not technically needed, from_str_radix with a 0 gets us correct results.
But using it we will only use from_str_radix on numbers that truly look octal (0 and one more following) and thje rest, like now, goes through the normal parse function.

@sylvestre sylvestre merged commit 76d6a4a into uutils:main Jun 13, 2022
@Ganneff Ganneff deleted the lswidthoctal branch June 13, 2022 19:58
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.

None yet

3 participants