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

tail: Refactor tail #3952

Closed
wants to merge 6 commits into from
Closed

tail: Refactor tail #3952

wants to merge 6 commits into from

Conversation

Joining7943
Copy link
Contributor

@Joining7943 Joining7943 commented Sep 18, 2022 •
edited
Loading

Follow up pr of #3905

@tertsdiepraam The last pr grew pretty big mostly because of the initial extraction of modules. I'm sure I can keep it smaller this time :)

@sylvestre
Copy link
Contributor

this isn't really refactor but just moving stuff around. What is the goal ?

@Joining7943
Copy link
Contributor Author

I've oversimplified the process when I called the initial part of #3905 "moving code around" and should have called it by name: An extraction of modules. Maybe, a problem is, that you won't see the other refactorings only in the diff because there was a lot more happening after this extraction. @tertsdiepraam came up with the idea of splitting the pr to simplify the review, but I couldn't stop right there because the program wasn't stable, yet. I needed to proceed until I found a point where on the one hand it was stabilized, what we've got so far and on the other hand have a proper base to move on. I've left an overview of introduced changes, additions etc. in the beforementioned pr, so you get an idea of what is going on. The overall goal of these refactorings (including this one) are:

  • Reduce complexity, improve readablity
  • Improve maintainablity
  • Change the internal structure to make it easier to understand and cheaper to modify
  • Do not change the observable behaviour, but maybe fix latent bugs on the way which we haven't observed, yet
  • Eliminate code duplication

I intend to fix the lack of deeper explanations of specific refactorings in the heading of this pr, within the documentation of the code and in the README.

@Joining7943 Joining7943 force-pushed the refactor-tail-continued branch 2 times, most recently from 2622da7 to a8b0a1a Compare October 27, 2022 15:59
@Joining7943 Joining7943 force-pushed the refactor-tail-continued branch 6 times, most recently from bed0ee2 to 867027a Compare November 4, 2022 12:54
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

GNU testsuite comparison:

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

@Joining7943 Joining7943 force-pushed the refactor-tail-continued branch 4 times, most recently from bc8845e to 0b07e57 Compare November 10, 2022 14:40
@sylvestre
Copy link
Contributor

I am sorry but your refactors are huge and very hard to review.
Could you please split it into small PRs?
thanks

@Joining7943
Copy link
Contributor Author

Sorry for that. Would it also be ok to squash related commits and add more explanation to them? Currently, the commits are pretty raw and I guess I could limit the commits to around 10, so the diffs between them would be smaller and easier to review.

@sylvestre
Copy link
Contributor

I would like smaller PR with changes that make sense to be grouped together

@github-actions
Copy link

GNU testsuite comparison:

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

@Joining7943
Copy link
Contributor Author

Sorry, it's not possible to split this pr. Please see the pr as a whole. However, to better the review situation, I'll squash the commits, add some explanation and document the tail module as far as possible. I'll also write more documention for tests I added and which made changes necessary. I think we can then review the commits like a pr, if you like to.

@sylvestre
Copy link
Contributor

yeah, this is far from ideal to do development this way ...
@jhscheer @tertsdiepraam wdyt?

@tertsdiepraam
Copy link
Member

Yeah I'm not a big fan of this either.

I think we can then review the commits like a pr, if you like to.

Sorry, but this is still a big ask. As reviewers we have to do a lot of context switching. Every time we review, we have to reread the PR, try to remember what's changed since last time, what our feedback was, etc.. With small PRs this is nicely chunked and we can merge them one at a time with quick iteration time. With a PR like this, we might want something changed and then have to check the entire PR again, which takes a long time. This is also why I merged the previous PR: it got too big and unwieldy for me to keep track of, just too many things going on at once, so I figured that starting with a clean slate and having smaller PRs in the future would be better.

I'll look through this PR now to see if I can identify a way to split this up and then you can open new PRs based on that. Does that sound good?

@tertsdiepraam
Copy link
Member

Here are the parts I think should at the very least be split off into separate PRs:

  1. Splitting up the code over more files
  2. error.rs (also look more closely into UResult/UError, which should make the whole IoContextError thing redundant if I'm not mistaken)
  3. The refactored checking of the warnings with the new atty dependency and VerrificationResult and all that.
  4. parse_duration

@Joining7943
Copy link
Contributor Author

I really try to understand your concerns regarding the review and I am honestly sorry, that there are some. However, maybe I can clear them up? Please try to understand me too, that it is by far harder, close to impossible, to change the implementation and its history than changing the review to be based on the commit history instead of standalone prs.

This is actually a draft and there's also a lot of documentation missing, which I think would clarify a lot in advance. Some changes only make sense in the whole context. We could also talk things through on the base of the topics you suggested or else have in mind. If there's something wrong on my side I only have to change that once based on the current state and don't have to transport it through a pr chain. Please let me try to fix the commits and documentation first and then let's see? Sooner or later, I need to do this anyways.

However, as a peace offering, I could try to unhook the parsing of the duration and the additional warning adding the atty dependency. These changes should't have much implications on the rest of the code. I'm not sure what you mean with 1. splitting up the code over more files?

Concerning number 2, I need the IoErrorcontext in two places. That's when printing the correct error message and when figuring out the different behavior on different platforms based on the context where the error appeared. Compliance with gnu's tail error handling and when/how they add a file/stdin for monitoring with follow makes this necessary and this enum simplifies coordinating that. The IoContextError is mainly just transporting this context which is a little bit specific to tail so I can't use UioError for that, at least as far as I've seen, but maybe you have an idea?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Nov 12, 2022 •
edited
Loading

Alright, I'm gonna try to clear your questions up.

I think that your idea of cleaning up the commits and separate PRs are already quite close. The PRs I'm suggesting don't have to be independent, but we can merge one first and then you can put up a new one that builds on the previous one. So if you have those commits ready, all you have to do is make a PR with just the first one and we'll continue from there.

This is actually a draft and there's also a lot of documentation missing, which I think would clarify a lot in advance

This might help a bit but is not the main issue, I think. The main issue is too many things happening at once, regardless of whether they're well-documented.

However, as a peace offering, I could try to unhook the parsing of the duration and the additional warning adding the atty dependency.

This would be already a great improvement!

I'm not sure what you mean with 1. splitting up the code over more files?

Nevermind, I thought this PR created new files, but that's not the case I see. Sorry!

Concerning number 2, I need the IoErrorcontext in two places.

Printing the message should be all UIoError, which I think you are indeed (partially) delegating too. For the second usecase, I see what you mean, but I can't evaluate the solution very well in the current state of the PR. The current implementation uses a lot of show_error! and set_exit_code which is less than ideal, so you're right that it needs some refactoring. But, I'm not sure that we need the whole handler and the context. I'd love to discuss this further in a separate PR :)

@Joining7943
Copy link
Contributor Author

Sorry, but I have to ask, before I'm going through this. Do you know, that you can select (multiple) commits in the Github review panel and start a review on them?

@tertsdiepraam
Copy link
Member

I do, but the functionality is far from perfect in my opinion

@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/rm/rm2. tests/rm/rm2 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?

@sylvestre
Copy link
Contributor

@Joining7943 Are you still interested by this ?

sylvestre
sylvestre requested changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

needs rebasing

@uutils uutils deleted a comment from github-actions bot Mar 27, 2023
@Joining7943
Copy link
Contributor Author

needs rebasing

No problem. I didn't have much time lately to work on this pr but I haven't forgotten it.

@Joining7943 Joining7943 changed the title tail: Refactor tail continued tail: Refactor tail Apr 2, 2023
@Joining7943
tests/tail: Add, fix tests. Note some tests are not passing yet
9ec15f1
@Joining7943
tail: Refactor Input::from parameter to use AsRef<OsStr>. Settings::i…
b7588be 
…nputs now uses Vec<Input>
@Joining7943
tail: Refactor chunks.rs and small parts of tail.rs. Replace unwraps …
3c9171f 
…with error propagation ...

Return io::Result from methods in chunks.rs to be able to replace unwraps with error propagation and
return the bytes read from the source so far. Store how many bytes are read in LinesChunkBuffer.

Fixes: tail does not output anything when there's no newline in the source and the `--follow` option
is given. Flush the buffered writer after a write explicitly.

Simplify the BytesChunkBuffer::fill and LinesChunkBuffer::fill methods and make use of some new
helper methods in these structs.

Add more unit tests for structs in chunks.rs
@Joining7943
tail/args: Fix parsing follow mode has wrong mode when -F together wi…
d2b8241 
…th --retry
@Joining7943
tail: Big refactor of error handling in all affected modules ...
dc78261 
tail.rs:
* Change and simplify the control flow to try to open the input or file first and then react
  appropriately on errors. Note this also fixes some misbehavior on macos when a fifo pointed to a
  directory and other minor related issues.
* Differentiate between read and write errors to exit on write errors. Read errors in contrast are
  not fatal and are handled like the other `TailError` types in the `TailErrorHandler` in the newly
  created error.rs module.
* Return `Result<u64, TailError>` from unbounded_tail and bounded_tail. The Ok result (bytes read
  and printed) is currently not used but can be used later in the follow module.
* Fix printing of Fifos multiple times. For example `tail - - < file` printed the content of the
  file twice). Fifos should behave like other standard input in such cases.

follow/files.rs: Add "untailable" files separately in an own method to the set of watched files.

paths.rs:
* Remove the resolve method and replace the workaround with the more reliable open method which also
  differentiates between a fifo and a pipe in case of stdin. It uses an improved version of
  `FileExtTail::is_seekable` to be able to do so.
* Cleanup the redundant `stdin_is_bad_fd` function
@Joining7943
tail: Cleanup code, fix clippy warnings and add documentation
a4de92b
@tertsdiepraam
Copy link
Member

The last pr grew pretty big mostly because of the initial extraction of modules. I'm sure I can keep it smaller this time :)

Reminder :)

@Joining7943
Copy link
Contributor Author

Reminder :)

I'm trying, but I also want it stable and the current state is not the end of the road. Please don't misunderstand the last commits and pushes as completely new stuff. I mostly simplified the existing code, also, to get it ready for review. I squashed the commits down to 6 which can be reviewed separately. We can group 9ec15f1 and b7588be, 3c9171f, d2b8241, dc78261 and a4de92b and discuss them in separate prs if you like to. If you wnat to group them differently, just tell.

@tertsdiepraam
Copy link
Member

Please don't misunderstand the last commits and pushes as completely new stuff.

Alright!

discuss them in separate prs if you like to

I think that makes sense for some of them like the error and the changes to the tests. I'll leave the precise separation up to you.

@Joining7943
Copy link
Contributor Author

Ok, then let's talk about the tests first. I'll disable the failing tests and then reactivate them in the fitting commit.

@Joining7943
Copy link
Contributor Author

@tertsdiepraam I moved the tests into #4727

@Joining7943 Joining7943 mentioned this pull request Apr 12, 2023
Closed
@Joining7943
Copy link
Contributor Author

@tertsdiepraam To avoid further misunderstandings, here's the current roadmap for this pr. I'm going to pull every commit before dc78261 (which are: b7588be, 3c9171f, d2b8241) from this pr into a separate pr. Changes to the tests from 9ec15f1 will be integrated into the respective pr. I already created #4734 which is based on d2b8241. The main error handling refactor (and fix) is dc78261, which you wanted separated as far as I understood. I have to keep this commit as one of the last because it depends on 3c9171f.

@sylvestre
Copy link
Contributor

@Joining7943 should we close this? thanks

@Joining7943
Copy link
Contributor Author

Why are you asking me this all the time? I'm waiting for #4756 being merged so I can go on with the rest of it from here

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Jun 23, 2023 •
edited
Loading

We just lose track of what's going on across several issues and PRs sometimes. I felt the conversation on the other PR has gone stale and I figured you might not want to work on this anymore given your comment on the other PR. In any case, there's a lot of good stuff in here, such as all the documentation from before. Maybe we can select some commits independent from #4756 and wrap it up? In particular, I would propose to merge these commits:

@sylvestre
Copy link
Contributor

@Joining7943 sorry, i was lost with the PRs and the fact it is conflicting :)

@tertsdiepraam tertsdiepraam mentioned this pull request Aug 19, 2023
Closed
@tertsdiepraam
Copy link
Member

I'm closing this in favor of #5173, which is based on this PR, but rebased and cleaned up. The commits there still have the author credits of the original author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@Joining7943 @sylvestre @tertsdiepraam