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

shuf: include all echo args, not just the last #5978

Merged
merged 3 commits into from
Feb 17, 2024

Conversation

BenWiederhake
Copy link
Collaborator

@BenWiederhake BenWiederhake commented Feb 15, 2024

This PR fixes all shuf bugs around repeated arguments:

  • GNU shuf considers all arguments to --echo. uutils shuf currently only considers the very last --echo, and ignores all the others.
  • GNU shuf accepts multiple head-counts (-n), effectively only considering the lowest. uutils shuf currently only considers the very last --head-count, and ignores all the others.
  • GNU shuf rejects repeated input ranges and output files with a clear error message. uutils shuf currently only considers the very last element each, silently ignoring all the others.

This is clearly buggy, since command-line arguments should be ignored only in exceptional cases. This appears to be a regression after #3329.

In this PR I'm fixing these and adopt the behavior of GNU, which is in my eyes the only sane reaction to those scenarios. Of course, this PR also adds tests for that.

I'm entirely new to uutils, so please do point out any resources that might help me. (I've already read {DEVELOPMENT,CONTRIBUTING,CODE_OF_CONDUCT}.md.)

Multiple echos example
$ shuf -e a b -e c
c
b
a
$ shuf -e a b -e c
a
b
c

The following demonstrations are run with this diff applied, to see what's going on:

diff --git a/src/uu/shuf/src/shuf.rs b/src/uu/shuf/src/shuf.rs
--- a/src/uu/shuf/src/shuf.rs
+++ b/src/uu/shuf/src/shuf.rs
@@ -48,9 +48,10 @@ mod options {
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = uu_app().try_get_matches_from(args)?;
+    println!("**>{:?}<**", matches);
 
     let mode = if let Some(args) = matches.get_many::<String>(options::ECHO) {
-        Mode::Echo(args.map(String::from).collect())
+        Mode::Echo(args.map(String::from).inspect(|x| println!(">{x}<")).collect())
     } else if let Some(range) = matches.get_one::<String>(options::INPUT_RANGE) {
         match parse_range(range) {
             Ok(m) => Mode::InputRange(m),

Behavior before this PR:

$ cargo run -- shuf -e a b -e c
**>ArgMatches { valid_args: ["echo", "input-range", "head-count", "output", "random-source", "repeat", "zero-terminated", "file", "help", "version"], valid_subcommands: [], args: FlatMap { keys: ["echo", "repeat", "zero-terminated"], values: [MatchedArg { source: Some(CommandLine), indices: [5], type_id: Some(alloc::string::String), vals: [[AnyValue { inner: alloc::string::String }]], raw_vals: [["c"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [6], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [7], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }] }, subcommand: None }<**
>c<
c

Note that shuf isn't even aware that a and b were passed in.

Behavior after this PR:

$ cargo run -- shuf -e a b -e c
**>ArgMatches { valid_args: ["echo", "input-range", "head-count", "output", "random-source", "repeat", "zero-terminated", "file", "help", "version"], valid_subcommands: [], args: FlatMap { keys: ["echo", "repeat", "zero-terminated"], values: [MatchedArg { source: Some(CommandLine), indices: [2, 3, 5], type_id: Some(alloc::string::String), vals: [[AnyValue { inner: alloc::string::String }, AnyValue { inner: alloc::string::String }], [AnyValue { inner: alloc::string::String }]], raw_vals: [["a", "b"], ["c"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [6], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [7], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }] }, subcommand: None }<**
>a<
>b<
>c<
b
c
a

@BenWiederhake
Copy link
Collaborator Author

BenWiederhake commented Feb 15, 2024

CI failed, and I see a similar ICE panic when I run pre-commit (specifically cargo +nightly clippy --workspace --all-targets --all-features --). I don't really know what to do, and hope that the problem will somehow go away soon.

Anyway, stable clippy (cargo clippy --workspace --all-targets --all-features --) is clean, so I expect no problems.

EDIT: Also, I cannot reproduce the CI test failures of test_od::test_f16 on Android and test_split::test_round_robin_limited_file_descriptors on Ubuntu-musl. These should be unrelated, as I didn't touch that code. Please advise?

@tertsdiepraam
Copy link
Member

You can ignore both failures, they're known issues :)

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Nice work! I just think there's one case that's not handled correctly by this.

src/uu/shuf/src/shuf.rs Show resolved Hide resolved
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Made the tests test_head_count_multi_small_then_big, test_head_count_multi_big_then_small, and test_shuf_multiple_outputs sharper. Thanks @cakebaker!
  • No longer introduce a bug regarding multiple -r and -z arguments, and add tests for it. Thanks @tertsdiepraam!
  • Rebased on current main.

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@cakebaker cakebaker merged commit a12d7c2 into uutils:main Feb 17, 2024
53 of 59 checks passed
@cakebaker
Copy link
Contributor

Good job, thanks!

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.

3 participants