-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
stty: add combination settings #8256
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
Conversation
015dc4e to
76e19fe
Compare
|
GNU testsuite comparison: |
76e19fe to
e9cf338
Compare
src/uu/stty/src/stty.rs
Outdated
| let mut flags: Vec<ArgOptions> = Vec::new(); | ||
| match combo { | ||
| "lcase" | "LCASE" => { | ||
| if let Some(flag) = string_to_flag("xcase") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are never supposed to fail right? Maybe we should unwrap them? It's better to know early if it doesn't work. I forget how this worked, but maybe we can use the ArgOptions directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be done in a more functional style with less repetition:
let flags = ["xcase", ...];
let flags = flags
.iter()
.map(|f| {
ArgOptions::Flags(string_to_flag(f).unwrap())
})
.collect();(or don't unwrap and use filter_map)
This also makes it easier to see what these combo flags map to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree that the current way I did this is repetitive. I like the functional approach, I'll change it towards that. Unwrapping should be fine, assuming that every flag in every combination is available on all necessary platforms. Maybe some flags in combinations aren't available on something like macOS? I'll need to investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there are flags that exist in combinations and aren't present on macOS. olcuc for example. I'm thinking its better to not unwrap and use filter_map because the alternative is having cfg platform directives in both flags.rs and stty.rs for the same flags.
|
GNU testsuite comparison: |
e9cf338 to
47ca624
Compare
|
GNU testsuite comparison: |
|
@tertsdiepraam @cakebaker anything else you'd like me to change? |
47ca624 to
b474cc6
Compare
|
GNU testsuite comparison: |
| "-lcase" | "-LCASE" => { | ||
| flags = vec!["-xcase", "-iuclc", "-olcuc"]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be the same flags as for lcase/LCASE? From the help:
[-]lcase same as xcase iuclc olcuc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with GNU stty, '-lcase'/'-LCASE' yields '-xcase -iuclc -olcuc'
src/uu/stty/src/stty.rs
Outdated
| flags = vec!["-parenb", "cs8"]; | ||
| } | ||
| "litout" => { | ||
| flags = vec!["-parenb", "-istrip", "-opost", "-cs8"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cs8 is not negated according to the help:
litout same as -parenb -istrip -opost cs8
| flags = vec!["-parenb", "-istrip", "-opost", "-cs8"]; | |
| flags = vec!["-parenb", "-istrip", "-opost", "cs8"]; |
src/uu/stty/src/stty.rs
Outdated
| "cread", "-ignbrk", "brkint", "-inlcr", "-igncr", "icrnl", "icanon", "iexten", | ||
| "echo", "echoe", "echok", "-echonl", "-noflsh", "-ixoff", "-iutf8", "-iuclc", | ||
| "-xcase", "-ixany", "imaxbel", "-olcuc", "-ocrnl", "opost", "-ofill", "onlcr", | ||
| "-onocr", "-onlret", "n10", "cr0", "tab0", "bs0", "vt0", "ff0", "isig", "-tostop", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's nl0, not n10:
| "-onocr", "-onlret", "n10", "cr0", "tab0", "bs0", "vt0", "ff0", "isig", "-tostop", | |
| "-onocr", "-onlret", "nl0", "cr0", "tab0", "bs0", "vt0", "ff0", "isig", "-tostop", |
src/uu/stty/src/stty.rs
Outdated
| "echo", "echoe", "echok", "-echonl", "-noflsh", "-ixoff", "-iutf8", "-iuclc", | ||
| "-xcase", "-ixany", "imaxbel", "-olcuc", "-ocrnl", "opost", "-ofill", "onlcr", | ||
| "-onocr", "-onlret", "n10", "cr0", "tab0", "bs0", "vt0", "ff0", "isig", "-tostop", | ||
| "-ofdel", "-echoprt", "echoctl", "echoke", "-extproc", "-flush0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's -flusho instead of -flush0:
| "-ofdel", "-echoprt", "echoctl", "echoke", "-extproc", "-flush0", | |
| "-ofdel", "-echoprt", "echoctl", "echoke", "-extproc", "-flusho", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks!
b474cc6 to
a2711f0
Compare
|
GNU testsuite comparison: |
|
Thanks! |
fixes for #3860. i added the function
combo_to_flags()which takes a combination name as a string and decomposes the combination into the individual settings. this function returns a vec of all the flags that correspond to the combination, which are then appended to thevalid_argsvec that is used during arg parsing. one of the combinations is not fully complete yet, as it requires functionality in #3859, which i will tackle next.