Repository navigation
diff: accept clustered short options such as -qst and -ut - #304
SichenLiang wants to merge 2 commits into
Conversation
Merging this PR will improve performance by 3.15%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ |
cmd_cmp_gnu_equal[25000] |
23 µs | 22.3 µs | +3.22% |
| ⚡ |
cmd_cmp_gnu_equal[10000] |
23 µs | 22.3 µs | +3.21% |
| ⚡ |
cmd_diff_gnu_equal[10000] |
23.1 µs | 22.3 µs | +3.21% |
| ⚡ |
cmd_diff_gnu_equal[1000] |
23.1 µs | 22.3 µs | +3.21% |
| ⚡ |
cmd_diff_gnu_equal[25000] |
23.1 µs | 22.4 µs | +3.2% |
| ⚡ |
cmd_cmp_gnu_equal[100] |
23.6 µs | 22.9 µs | +3.13% |
| ⚡ |
cmd_diff_gnu_equal[100] |
24.4 µs | 23.7 µs | +3.03% |
| ⚡ |
cmd_diff_release_equal[1000] |
24.4 µs | 23.6 µs | +3.03% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing SichenLiang:short-option-clusters (385935e) with main (2d605f6)
| .and_then(|arg| arg.strip_prefix('-')) | ||
| .filter(|arg| !arg.starts_with('-')) | ||
| { | ||
| if short.len() == 1 { |
There was a problem hiding this comment.
a single letter is just a cluster of one, the len() == 1 branch could go away
| } | ||
| } else if short.len() > 1 { | ||
| // Leave mixed words whole for legacy numeric options and their diagnostics. | ||
| let all_recognized = short.bytes().all(|option| { |
There was a problem hiding this comment.
this dry run on a throwaway Params is convoluted
could you please just check short.bytes().all(|b| b"ceqstuy".contains(&b))?
| ) | ||
| }); | ||
| if all_recognized { | ||
| let mut clustered_params = params.clone(); |
There was a problem hiding this comment.
why clone? on error we return anyway, so applying directly to params/format should be enough, no?
| } | ||
| } | ||
| #[test] | ||
| fn control_partly_recognized_short_option_clusters() { |
There was a problem hiding this comment.
odd name, maybe partly_recognized_short_option_clusters?
| let mut right = NamedTempFile::new()?; | ||
| right.write_all(b"a b\n")?; | ||
|
|
||
| let equivalent = |cluster: &[&str], |
There was a problem hiding this comment.
this test is hard to read with the nested closures
a few direct cases would be enough here, the unit tests already cover the matrix
Handle the seven short options that take no argument through the same path in their separate and clustered forms. Keeping the existing fallback when a word contains an unsupported option, and share the output-style conflict checks between both forms. Add parser and command-output tests for clustered options.
385935e to
d50d5ad
Compare
|
Thanks for taking care of these! |
GNU diff allows short options that take no argument to be combined, so
diff -qst a bis the same asdiff -q -s -t a b. uutils treated the combined form as one unknown option.The existing argument-free options (
-c,-e,-q,-s,-t,-uand-y) now use the same path whether they are separate or clustered. Output-style conflicts are checked in the same place, while a word containing an unsupported option keeps the old handling.Fixes #298