tail: Refactor tail #3952
tail: Refactor tail #3952
Conversation
|
this isn't really refactor but just moving stuff around. What is the goal ? |
|
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:
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. |
sylvestre
force-pushed
the
refactor-tail-continued
branch
from
7e7f82c to
8e4264a
Compare
Joining7943
force-pushed
the
refactor-tail-continued
branch
2 times, most recently
from
2622da7 to
a8b0a1a
Compare
Joining7943
force-pushed
the
refactor-tail-continued
branch
6 times, most recently
from
bed0ee2 to
867027a
Compare
|
GNU testsuite comparison: |
Joining7943
force-pushed
the
refactor-tail-continued
branch
4 times, most recently
from
bc8845e to
0b07e57
Compare
|
I am sorry but your refactors are huge and very hard to review. |
Joining7943
force-pushed
the
refactor-tail-continued
branch
from
8ff014d to
1b0c4f7
Compare
|
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. |
|
I would like smaller PR with changes that make sense to be grouped together |
Joining7943
force-pushed
the
refactor-tail-continued
branch
from
1b0c4f7 to
c3b817c
Compare
|
GNU testsuite comparison: |
|
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. |
|
yeah, this is far from ideal to do development this way ... |
|
Yeah I'm not a big fan of this either.
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? |
|
Here are the parts I think should at the very least be split off into separate PRs:
|
|
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 Concerning number 2, I need the |
|
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 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.
This would be already a great improvement!
Nevermind, I thought this PR created new files, but that's not the case I see. Sorry!
Printing the message should be all |
|
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? |
|
I do, but the functionality is far from perfect in my opinion |
Joining7943
force-pushed
the
refactor-tail-continued
branch
from
85b1e84 to
2a06ced
Compare
|
GNU testsuite comparison: |
|
@Joining7943 Are you still interested by this ? |
No problem. I didn't have much time lately to work on this pr but I haven't forgotten it. |
Joining7943
force-pushed
the
refactor-tail-continued
branch
from
2a06ced to
13c566b
Compare
Joining7943
force-pushed
the
refactor-tail-continued
branch
from
d5c0774 to
9ef9d74
Compare
…nputs now uses Vec<Input>
…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
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
force-pushed
the
refactor-tail-continued
branch
from
291660c to
a4de92b
Compare
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. |
Alright!
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. |
|
Ok, then let's talk about the tests first. I'll disable the failing tests and then reactivate them in the fitting commit. |
|
@tertsdiepraam I moved the tests into #4727 |
|
@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. |
|
@Joining7943 should we close this? thanks |
|
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 |
|
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: |
|
@Joining7943 sorry, i was lost with the PRs and the fact it is conflicting :) |
|
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. |
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 :)