shuf: include all echo args, not just the last by BenWiederhake · Pull Request #5978 · uutils/coreutils

BenWiederhake · 2024-02-15T20:30:25Z

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 · 2024-02-15T23:08:04Z

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 · 2024-02-15T23:34:40Z

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

tests/by-util/test_shuf.rs

tertsdiepraam

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

src/uu/shuf/src/shuf.rs

BenWiederhake · 2024-02-16T20:17:30Z

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.

github-actions · 2024-02-16T20:45:26Z

GNU testsuite comparison:

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

cakebaker · 2024-02-17T15:23:04Z

Good job, thanks!

BenWiederhake mentioned this pull request Feb 15, 2024

shuf: Treat empty file as zero elements instead of one emptystring #5979

Merged

cakebaker reviewed Feb 16, 2024

View reviewed changes

tests/by-util/test_shuf.rs Outdated Show resolved Hide resolved

cakebaker reviewed Feb 16, 2024

View reviewed changes

tests/by-util/test_shuf.rs Outdated Show resolved Hide resolved

tertsdiepraam reviewed Feb 16, 2024

View reviewed changes

src/uu/shuf/src/shuf.rs Show resolved Hide resolved

BenWiederhake added 3 commits February 16, 2024 20:49

shuf: include all echo args, not just the last

07e8f4c

shuf: obey all headcount args, not just the last

69f23c2

shuf: refuse multiple input ranges and multiple output files

b091911

BenWiederhake force-pushed the dev-shuf-multi branch from 639702e to b091911 Compare February 16, 2024 20:15

cakebaker merged commit a12d7c2 into uutils:main Feb 17, 2024

BenWiederhake deleted the dev-shuf-multi branch February 17, 2024 15:41

This was referenced Feb 17, 2024

shuf: Fix OOM crash for huge number ranges #5980

Merged

Allow repeated flags much more often #5998

Open

chenrui333 mentioned this pull request Mar 24, 2024

uutils-coreutils 0.0.25 Homebrew/homebrew-core#167053

Merged

moonfruit mentioned this pull request Mar 28, 2024

uutils-selected 0.0.25 moonfruit/homebrew-tap#92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

shuf: include all echo args, not just the last#5978
cakebaker merged 3 commits intouutils:mainfrom
BenWiederhake:dev-shuf-multi

BenWiederhake commented Feb 15, 2024 •

edited

Loading

Uh oh!

BenWiederhake commented Feb 15, 2024 •

edited

Loading

Uh oh!

tertsdiepraam commented Feb 15, 2024

Uh oh!

Uh oh!

Uh oh!

tertsdiepraam left a comment

Uh oh!

Uh oh!

BenWiederhake commented Feb 16, 2024

Uh oh!

github-actions bot commented Feb 16, 2024

Uh oh!

cakebaker commented Feb 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Uh oh!

Conversation

BenWiederhake commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenWiederhake commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Feb 15, 2024

Uh oh!

Uh oh!

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BenWiederhake commented Feb 16, 2024

Uh oh!

github-actions bot commented Feb 16, 2024

Uh oh!

cakebaker commented Feb 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

BenWiederhake commented Feb 15, 2024 •

edited

Loading

BenWiederhake commented Feb 15, 2024 •

edited

Loading