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.

Sign up for GitHub

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

Jump to bottom

split -1 in should be accepted #5170

Closed
wants to merge 29 commits into from
Closed

split -1 in should be accepted #5170

wants to merge 29 commits into from

Conversation

zhitkoff
Copy link
Contributor

@zhitkoff zhitkoff commented Aug 19, 2023 •
edited
Loading

It looks like the GNU split -1 in behaves as split -l 1 in , i.e. a shorthand for -l or --lines
Having that, here is my take on it - using similar approach to what is done in fold

fixes #5033

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/split/filter is no longer failing!
GNU test failed: tests/split/additional-suffix. tests/split/additional-suffix is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/lines. tests/split/lines is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/numeric. tests/split/numeric is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/suffix-length. tests/split/suffix-length is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail-2/inotify-dir-recreate
GNU test error: tests/split/filter. tests/split/filter is passing on 'main'. Maybe you have to rebase?

@sylvestre
Copy link
Sponsor Contributor

a few comments:

  • could you please run rustfmt?
  • it seems that it regressed some tests
  • we will need a test covering this change
    thanks

@tertsdiepraam
Copy link
Member

In the issue I wrote that it should behave like fold (though we can use another method). Have you compared this method with what we do in fold?

@zhitkoff
Copy link
Contributor Author

In the issue I wrote that it should behave like fold (though we can use another method). Have you compared this method with what we do in fold?

I see ... So, basically - instead of trying to handle it all with clap::Arg , strip out the "obsolete" arguments like this one and handle it separately. Will do that.
However, one note on that - the implementation in fold allows both "obsolete" and "normal" arguments for width to be specified at the same time, but if so, only "normal" one will be used.
That is correct behavior for GNU fold , but it is not for GNU split - it fails when both used like so split -9 --lines=9 file. So, implementation would need to be a bit different for split

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/split/filter is no longer failing!
Skipping an intermittent issue tests/tail-2/inotify-dir-recreate
GNU test failed: tests/split/additional-suffix. tests/split/additional-suffix is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/lines. tests/split/lines is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/numeric. tests/split/numeric is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/suffix-length. tests/split/suffix-length is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/split/filter. tests/split/filter is passing on 'main'. Maybe you have to rebase?

@tertsdiepraam
Copy link
Member

So, basically - instead of trying to handle it all with clap::Arg , strip out the "obsolete" arguments like this one and handle it separately. Will do that.

Yes, that's the best approach we found so far, but if you find a better way do let us know!

That is correct behavior for GNU fold , but it is not for GNU split - it fails when both used like so split -9 --lines=9 file. So, implementation would need to be a bit different for split

split technically works like uniq, not like fold, but we simplified it a bit in uniq. It's very tricky to support all of these properly. I think that, for now, we have to find a balance between being compatible and having reasonably simple code. It just does not fit claps API very well.

@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail-2/inotify-dir-recreate

@zhitkoff
Copy link
Contributor Author

GNU testsuite comparison:

Skip an intermittent issue tests/tail-2/inotify-dir-recreate

This one actually shows as FAIL in gnu-full-result.json artifact from the last GnuTests action run for this PR.
Not sure what can be done to fix this ... considering this PR did not touch tail or any common code.
@tertsdiepraam @sylvestre could you please point me in the right direction?

@zhitkoff
Copy link
Contributor Author

a few comments:

  • could you please run rustfmt?
  • it seems that it regressed some tests
  • we will need a test covering this change
    thanks

should be all done, except some weird issue with GnuTests regression on tests/tail-2/inotify-dir-recreate ... see my other comment #5170 (comment)

cakebaker and others added 15 commits August 22, 2023 16:17
@cakebaker @zhitkoff
nl: implement TryFrom<&str> for NumberingStyle
1f3fba6
@tertsdiepraam @zhitkoff
cp: make more types public and add more documentation
9dfc256
@tertsdiepraam @zhitkoff
cp: refactor attribute parsing
3bc22e8 
Clean up the parsing of the attributes to
preserve. There are several improvements here: Preserve now uses  `max`
from Ord, the `max` method is now called `union` and does not mutate,
the parse loop is extracted into a new function `parse_iter`, the `all`
and `none` methods are replaced with const values. Finally
all fields of Attributes are now public.
@renovate @zhitkoff
chore(deps): update rust crate tempfile to 3.8.0
89b557f
@cakebaker @zhitkoff
nl: handle line number overflow
ff29832
@tertsdiepraam @zhitkoff
docs: add platform support page in the docs
38f8290
@tertsdiepraam @zhitkoff
docs: remove README.target.md in favor of platform support page in th…
24f7830 
…e docs
@simon04 @tertsdiepraam @zhitkoff
Extract uucore::line_ending::LineEnding (uutils#5120)
9097b50 
* Extract uucore::line_ending::LineEnding

Aims to provide consistent newline/zero terminator handling.

* Apply suggestions from code review

Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>

* cargo fmt

* Use uucore::line_ending::LineEnding

* Remove uucore::line_ending::LineEnding::Space

* Rename LineEnding::from_zero_flag

* Replace LineEnding::None with Option<LineEnding>

* cargo clippy

* assert_eq

* cargo clippy

* cargo clippy

* uucore/line_ending: add more documentation

---------

Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>
@tertsdiepraam @zhitkoff
website: fix path_prefix
17f83f4
@cakebaker @zhitkoff
docs: add missing "not"
9228b1a
@tommady @zhitkoff
date: fix panic when input will cause overflow (uutils#5160)
ce97565 
* fix issue 5149

* fix clippy style issue

* fix spell issue

* address comment

* address comments

* fix cspell
@tertsdiepraam @zhitkoff
Cargo.toml: add feat_require_unix_hostid to feat_os_unix
1254b7c
@sylvestre @zhitkoff
fuzz the test command
9ed04ae
@sylvestre @zhitkoff
Add license headers on all files
6004bd2
@cakebaker @zhitkoff
chown: remove some duplication in tests
5f38c29
sylvestre and others added 9 commits August 22, 2023 16:17
@sylvestre @cakebaker @zhitkoff
Remove the author copyright notices (uutils#5184)
892ad4b 
* Remove the author copyright notices

Rational:
* not maintained
* does not reflect reality
* don't provide any value (the info can be found in the git log)
* we don't have rules to update them
  (ex: should you update it after one line, two lines, etc)

Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
@tertsdiepraam @zhitkoff
docs: add conda installer
7818c5a
@sylvestre @zhitkoff
Fix some clippy warnings
87fcb6c
@sylvestre @zhitkoff
Don't warn too_many_lines for generated map
590426a
@cakebaker @zhitkoff
uucore: add missing "!"
800ce57
@cakebaker @zhitkoff
stdbuf: fix get_preload_env doesn't need a mutable reference
f9f1538
@cakebaker @zhitkoff
id: remove mutable reference when calling id_print
3286aa9
@tertsdiepraam @zhitkoff
README: update links to docs
f0e69cf
@cakebaker @zhitkoff
factor: simplify loop in test
b2cd1eb
@zhitkoff
Copy link
Contributor Author

zhitkoff commented Aug 22, 2023 •
edited
Loading

rebased to main, looks like GnuTests are passing now

@zhitkoff zhitkoff mentioned this pull request Aug 25, 2023
Closed
@zhitkoff
Copy link
Contributor Author

as noted in issue #5033:
This is a bit more complicated than supporting split -1 in use case. The -1 obsolete option could also be found inside combined short options, like following (all below are valid and work with GNU split)
split -1dxe file which means split -l 1 -d -x -e file
split -ex23d file which means split -e -x -l 23 -d file
etc
will need to refactor, so I will close this pull request for now and resubmit a new one with code that would handle this broader use case.

@zhitkoff zhitkoff closed this Aug 25, 2023
@zhitkoff zhitkoff deleted the zhitkoff/issue5033 branch August 25, 2023 20:07
@zhitkoff zhitkoff mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

split -1 in should be accepted
6 participants
@zhitkoff @sylvestre @tertsdiepraam @cakebaker @simon04 @tommady