stty: Improve option handling and align behavior with GNU coreutils #9255
Conversation
|
GNU testsuite comparison: |
CodSpeed Performance ReportMerging #9255 will not alter performanceComparing Summary
Footnotes
|
|
GNU testsuite comparison: |
src/uu/stty/src/stty.rs
Outdated
|
|
||
| // Obtain the original termios and overwrite specific fields on top of it to preserve | ||
| // platform-dependent fields. | ||
| let mut termios = tcgetattr(std::io::stdout().as_fd()) |
There was a problem hiding this comment.
This is is assuming that we're opening stdout, but no, it should be the device: tcgetattr(opts.file.as_fd()).
So, I think that this function should be instead moved at later point after all the args have been parsed, and it should take a &Termios, not returning it once it has been opened in the same way it's done currently
| // combination setting | ||
| } else if let Some(combo) = string_to_combo(arg) { | ||
| valid_args.append(&mut combo_to_flags(combo)); | ||
| } else if arg.starts_with('-') { |
There was a problem hiding this comment.
Mhlhl, this implies that doing stty foo bar would pass, until we fail parsing... While to me we should just fail as GNU does in that case with stty: invalid argument 'foo'
There was a problem hiding this comment.
Right. Let's do that.
|
GNU testsuite comparison: |
src/uu/stty/src/stty.rs
Outdated
| } | ||
|
|
||
| // Fetch termios for the target device once and reuse it downstream | ||
| let base_termios = tcgetattr(opts.file.as_fd())?; |
There was a problem hiding this comment.
IMHO this can be still just an Option set to None here, and you set it when needed, so before parsing, or - if unset - before applying the arguments.
src/uu/stty/src/stty.rs
Outdated
|
|
||
| /// GNU stty -g compatibility: restore Termios from the colon-separated hexadecimal representation | ||
| /// produced by print_in_save_format. | ||
| fn parse_save_format(s: &str, base: &Termios) -> Result<Termios, Box<dyn UError>> { |
There was a problem hiding this comment.
Can you just clone before passing and avoid returning it?
|
GNU testsuite comparison: |
|
I think my latests CR's have a bit of conflicts with this and was wondering if you're still working on it |
|
GNU testsuite comparison: |
| if i >= max_cc { | ||
| break; | ||
| } | ||
| let val = u32::from_str_radix(hex, 16).map_err(|_| { |
There was a problem hiding this comment.
Is this another case of rust-lang/rust-clippy#16213 ?
There was a problem hiding this comment.
Thanks! That’s the same from_str_radix pitfall (leading +). We can add a hex-only check if we want to be strict
|
GNU testsuite comparison: |
- Add GNU-compatible parsing of colon-separated hex -g output - Restore termios from saved state before applying new settings - Preserve platform-dependent fields while validating input robustly
- Update missing/invalid argument helpers to return Box<dyn UError> for consistent error typing - Propagate errors with map_err(Into::into) at call sites to match new helper signatures - Simplify string handling (String::as_str, &**s) to reduce verbosity and clarify intent
…rmance - Delay the initial tcgetattr call until termios is actually needed, using a closure that caches the result. - Modify parse_save_format to mutate the passed termios in-place instead of returning a clone, reducing unnecessary copies and improving efficiency.
Add a new test case `invalid_free_word_fails_immediately` to verify that `stty` appropriately handles and rejects invalid arguments like 'foo' and 'bar', ensuring the command fails with an error message "invalid argument 'foo'". This improves test coverage for argument validation.
- Updated error handling comment to describe GNU stty behavior more clearly for English readers. - Translated comment on termios cloning to explain preservation of platform-specific fields.
|
GNU testsuite comparison: |
Summary:
Details:
Rationale:
Scope / Impact:
Testing:
#9056