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

tail: Refactor handling of warnings and early exits #4135

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

Joining7943
Copy link
Contributor

@Joining7943 Joining7943 commented Nov 13, 2022

Refactor

  • printing of warnings because of problematic configuration by the user on the command line
  • handling of early exits.

This pr also adds the missing warning because of --follow being ineffective under some special circumstances.

The warnings and early exits are organized in dedidacted methods within the Settings struct providing:

  • Clearer, less complex control flow and overall code structure
  • args::Settings::from has no side effects anymore and the methods sole purpose is parsing and storing the initial configuration provided by the user
  • It's simpler now to produce the warnings in the same order as gnu's tail does
  • Early exits are organized in a more prominent place in the main module tail
  • Fixes an issue with a warning which was printed multiple times instead of only once at the start of tail

This pr also adds more tests which originate from the main refactoring pr #3952. A lot of these tests (besides the ones testing the warning functionality) show some broken functionality of tail or a bug and are therefore (partly) disabled_until_fixed, here. However, many of them were fixed on the way of #3952 and are enabled there.

There's also some additional code cleanup:

  • Remove InputService and move some of its functionality into Settings
  • Remove unneeded stdin_is_pipe_of_fifo function

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/timeout is no longer failing!

1 similar comment
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/timeout is no longer failing!

src/uu/tail/src/args.rs Show resolved Hide resolved
src/uu/tail/src/args.rs Outdated Show resolved Hide resolved
src/uu/tail/src/follow/watch.rs Show resolved Hide resolved
src/uu/tail/src/platform/unix.rs Outdated Show resolved Hide resolved
src/uu/tail/src/paths.rs Show resolved Hide resolved
src/uu/tail/src/follow/watch.rs Show resolved Hide resolved
src/uu/tail/src/args.rs Outdated Show resolved Hide resolved
@Joining7943
Copy link
Contributor Author

Sorry, that the tests for the warnings aren't integrated right now. But, I've ran the tests against the last 3 commits locally, so these changes are most probably working. We'll see this hopefully soon also in in the ci.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/timeout is no longer failing!

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@Joining7943
Copy link
Contributor Author

Joining7943 commented Dec 4, 2022

I've added all additional tests written during the refactoring of tail. These include the tests for the warnings. A lot of these additional tests (besides the warnings tests) are currently (partly) disabled and only show the bugs and wrong behavior if enabled. I could fix many of them on the way of #3952 and they are therefore enabled there.

Joining7943 added a commit to Joining7943/uutil-coreutils that referenced this pull request Dec 7, 2022
Joining7943 added a commit to Joining7943/uutil-coreutils that referenced this pull request Dec 7, 2022
@github-actions
Copy link

github-actions bot commented Dec 7, 2022

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@Joining7943 Joining7943 force-pushed the refactor-tail-check-warnings branch 2 times, most recently from 6156597 to 8958e82 Compare December 7, 2022 16:11
@Joining7943 Joining7943 changed the title Refactor tail check warnings tail: Refactor handling of warnings and early exits Dec 7, 2022
@Joining7943
Copy link
Contributor Author

@tertsdiepraam ping :) i think this pr would be ready for a final review.

src/uu/tail/src/args.rs Outdated Show resolved Hide resolved
tests/by-util/test_tail.rs Show resolved Hide resolved
@Joining7943 Joining7943 force-pushed the refactor-tail-check-warnings branch 2 times, most recently from fcd92a9 to b4bc6ad Compare December 10, 2022 15:42
@Joining7943
Copy link
Contributor Author

Are we ready, here?

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.

Code looks good, I just have some questions about the TODO's

src/uu/tail/src/paths.rs Show resolved Hide resolved
src/uu/tail/src/paths.rs Outdated Show resolved Hide resolved
…InputService ...

Other changes summary:
* Cleanup imports
* Remove stdin_is_pipe_of_fifo function
* Rename WatcherService to Observer
…from tip of refactoring branch

Other changes summmary:
* Use module level imports instead of fully qualified imports where appropriate
* Fix intermittent failing test_positive_zero_bytes because of broken pipe
* Cleanup old disabled test for warning of following indefinitely
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm1 is no longer failing!
Congrats! The gnu test tests/rm/rm2 is no longer failing!

@tertsdiepraam tertsdiepraam merged commit e4bed1c into uutils:main Dec 14, 2022
@tertsdiepraam
Copy link
Member

Very nice!

@Joining7943 Joining7943 deleted the refactor-tail-check-warnings branch January 2, 2023 09:20
Joining7943 added a commit to Joining7943/uutil-coreutils that referenced this pull request Jan 7, 2023
Joining7943 added a commit to Joining7943/uutil-coreutils that referenced this pull request Feb 18, 2023
Joining7943 added a commit to Joining7943/uutil-coreutils that referenced this pull request Apr 2, 2023
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.

2 participants