-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Refactor tail #3905
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
Refactor tail #3905
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactors are always welcome :) I can't look at this in depth now, but I have one small first comment. A quick scroll-through makes this look very promising!
build.rs
Outdated
.as_bytes(), | ||
) | ||
.unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes more sense to re-export uu_app
from tail
? I.e.
pub use args::uu_app;
I do like that you split the args into a separate file though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes more sense to re-export uu_app from tail?
Cool. I haven't seen that.
Hi,
What features are you thinking of?
Right now there're two CI monitored tests that don't pass in this refactor.
Please wait for the other two open tail PRs to get merged before you do more refactoring, or you'll have to deal with merge conflicts. |
I had no specific features in mind. As far as I can see, uu_tail basically already provides all features gnu's tail does. I also meant to simplify fixing the
Sure, no problem. After 2 of the gnu tests failed, which originally passed, I thought about writing more tests before continuing the refactorings. Also, I would like to continue your work, incorporating gnu's test suite concerning tail into our own test framework. I'll take care as much as possible that the refactorings don't break existent code and tests. |
Is breaking the |
I think we could do that, but I don't quite understand the advantage you're talking about. Could you explain it a bit more? |
How? If I'm only interested in a subset of tests I run, e.g.: cargo test test_stdin --features "tail" --no-default-features
cargo test test_retry --features "tail" --no-default-features etc. |
The main benefit in my opinion is an organizational one. There are smaller files, which can be filled with more tests which are already partly explained and categorized by the file name. Then names for the tests themselves can be shorter or more specific to the problem, what in turn makes it easier to spot missing tests, if any. This won't break running only subsets of tests. In addition, one can use for example the filter |
But that can already be done with modules, like what you did with |
Yes I did that and I didn't like it too much. I would have preferred moving the tests into an own file. However, I moved the tests into this mod to switch off the tests for windows in one go instead of annotating every single test. I don't see a reason keeping files big if there's no hard reason for it. Rust provides a nice way to split files into smaller units with ease and big files are just cumbersome to work with and most often unnecessary. If you think it's worth it, I'll move out the commit to an own pr and maybe fix the code duplication I introduce to the build file. |
My 2 cents: breaking up tests over files is unnecessary, because I usually just search for the test I need to look at, which is actually easier in one single file. For non-test code I agree that splitting is good, of course. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments, but looks good overall. Of course we can only proceed with this of all the tests mentioned by @jhscheer still pass.
src/uu/tail/src/watch.rs
Outdated
} | ||
} | ||
|
||
pub struct WatcherService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a name like WatcherService, I think youre implying that it does more than it actually does. Currently it's just a wrapper around the watcher and orphans. The abstraction makes sense but only if you take it further by making follow
and handle_event
methods of this struct, so you don't need to expose the fields.
src/uu/tail/src/watch.rs
Outdated
pub fn from(settings: &mut Settings) -> UResult<Self> { | ||
let mut watcher_service = if settings.follow.is_some() { | ||
let (watcher, rx) = start_watcher_thread(settings)?; | ||
Self::new(Some(watcher), Some(rx)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should always be handled together in 1 Option
so we ensure we never set only one of the two. This goes for his they are stored in the WatcherService
and how they are pleased around in functions like new
.
src/uu/tail/src/watch.rs
Outdated
watcher_service: WatcherService, | ||
) -> UResult<()> { | ||
let (mut watcher, rx, mut orphans) = ( | ||
watcher_service.watcher.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can get rid of the unwraps? I presume this is okay because the WatcherService has these properties if and only if we're following?
src/uu/tail/src/files.rs
Outdated
use crate::text; | ||
use core::option::Option; | ||
use core::option::Option::{None, Some}; | ||
use core::result::Result::{Err, Ok}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these Option
/Result
things are in the prelude so don't need to be imported right?
src/uu/tail/src/files.rs
Outdated
} | ||
|
||
/// Canonicalize `path` if it is not already an absolute path | ||
fn canonicalize_path(path: &Path) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be a free standing function
@tertsdiepraam Thanks for your comments and review. Current state is wip and I've waited to proceed with this pr until the two bigger prs around tail are merged before I make greater changes, especially to the logic. Right now, I just have to move things around in case of a merge conflict. I'm changing the code step by step working towards a final solution, executing the test suite after each step. At the moment, constructs may look useless, incomplete or illplaced. |
That sounds excellent! My comments were then a bit premature :) |
No problem :) There's still a lot to do and I'll keep pushing intermediate results, so if you like you can see where the journey goes. I'll welcome any suggestions. |
Nice! Just remember to not make this PR too big. The bigger it gets the harder it is to review and there's not really a downside to having multiple successive PRs. |
@tertsdiepraam I'm ready for review if you are. There's still a lot more that can be done and currently there's a last error on macos concerning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Some more small nits, but the big picture looks great I think. Have you checked the compared GNU tests locally as well with main
, to ensure nothing broke that's not in the CI?
Also did we break these recently or have they never passed? In both cases well done!
Warning: Congrats! The gnu test tests/tail-2/follow-name is no longer failing!
Warning: Congrats! The gnu test tests/tail-2/retry is no longer failing!
Warning: Congrats! The gnu test tests/tail-2/wait is no longer failing!
src/uu/tail/src/args.rs
Outdated
@@ -62,9 +130,10 @@ pub enum FollowMode { | |||
Name, | |||
} | |||
|
|||
// TODO: use getters instead of pub access to fields to make Settings not changeable from the outside ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. Usually this kind of thing is only really helpful when you want to expose some API to other crates, but when it's all relatively small like this, it's fine like it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
src/uu/tail/src/tail.rs
Outdated
let mut chunks = chunks::LinesChunkBuffer::new(*sep, *count); | ||
chunks.fill(reader)?; | ||
chunks.print(writer)?; | ||
} | ||
(FilterMode::Lines(count, sep), true) => { | ||
let mut num_skip = (*count).max(1) - 1; | ||
FilterMode::Lines(Signum::PlusZero, _) | FilterMode::Lines(Signum::Positive(1), _) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this cleanup of FilterMode
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised you could use or-patterns here:
FilterLines::Lines(SigNum::PlusZero | SigNum::Positive(1), _)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Looks a little bit more compact.
}; | ||
} | ||
|
||
/// Read new data from `path` and print it to stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you document the return value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. There's a lot of documenation missing in general. I can address this at the end of the next iteration.
match input.kind() { | ||
InputKind::Stdin => continue, | ||
InputKind::File(path) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match input.kind() { | |
InputKind::Stdin => continue, | |
InputKind::File(path) => { | |
if let InputKind::File(path) = input.kind() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think it adds some clearity keeping the match written out, here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An if clearly conveys that we're only using one arm in my opinion. I see what you mean though. If you think this is clearer then you can leave it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep it for now. It's just, that I'm not done with this part, and this little reminder helps me a little bit. I'll compact the match like you suggested in the next pr.
|
||
// main print loop | ||
for path in &paths { | ||
_read_some = watcher_service.files.tail_file(path, settings.verbose)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with all the unused values? Can we remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we can remove some or all of them, and this is certainly something we should address in the next pr.
src/uu/tail/src/tail.rs
Outdated
} | ||
} else if !(settings.stdin_is_pipe_or_fifo && settings.paths.len() == 1) { | ||
follow(files, &settings, watcher_rx, orphans)?; | ||
// TODO: can I do this check in watch::follow? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of TODO notes in here, which is fine, but because this is an open project, they need some context for everyone else who is reading the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that. I won't leave any TODOs when I'm finished, but this is just an intermediate pr, so I would like to keep them for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I found a solution, so we can both be happy. I removed all TODOs in tail, so they don't end up in the main branch. I'll revert that commit later. No big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you removed all TODOs, even those that were there before this refactor. This is very inconvenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't suggesting to remove them, I just wanted them to be a bit more verbose so other people might pick them up more easily if they come across them in the code :)
src/uu/tail/src/args.rs
Outdated
parse_size(size_string).map(|n| (n, starting_with)) | ||
} | ||
|
||
// TODO: delete ?? not used anymore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain this todo. Is it about the function? If it's really not used it should indeed be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangentially related (and should not be fixed in this PR), but it feels like the platform
module should handle this case correctly for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found that this function has no usages (anymore) within tail, so it's subject to deletion if I don't find any use for it.
fn from(matches: &ArgMatches) -> UResult<Self> { | ||
let zero_term = matches.contains_id(options::ZERO_TERM); | ||
let mode = if let Some(arg) = matches.value_of(options::BYTES) { | ||
match parse_num(arg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make parse_num
return UResult<Signum>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I was a little bit torn here between keeping the function general or optimizing it. But, I'm on your side and also tend to the latter, here.
|
||
/// Wrapper for HashMap::remove using Path::canonicalize | ||
pub fn remove(&mut self, k: &Path) -> PathData { | ||
self.map.remove(&Self::canonicalize_path(k)).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is unwrap
okay for these methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think it is and tend to return an Option or Result from these methods, but I need to dig deeper through the watch module to figure this out. These functions are heavily used. I can address this issue together with refactoring watch
in the next pr.
Thanks for this great review!
Good, you're pointing that out. I didn't knew this about the CI, so I haven't run the tests locally for the last 30 commits or so. I'll run them tomorrow together with the fix for the failing test on macos.
|
coreutils/src/uu/tail/README.md Lines 39 to 46 in 78a9f6e
There maybe others but I stopped keeping track since I made the mention in the README. |
073de92
to
951c51e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the nit-picks, this looks very good to me.
use std::collections::hash_map::Keys; | ||
use std::collections::HashMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use std::collections::hash_map::Keys; | |
use std::collections::HashMap; | |
use std::collections::{hash_map::Keys, HashMap}; |
use std::sync::mpsc; | ||
use std::sync::mpsc::{channel, Receiver}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use std::sync::mpsc; | |
use std::sync::mpsc::{channel, Receiver}; | |
use std::sync::mpsc::{self, channel, Receiver}; |
// * This file is part of the uutils coreutils package. | ||
// * | ||
// * For the full copyright and license information, please view the LICENSE | ||
// * file that was distributed with this source code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// * This file is part of the uutils coreutils package. | |
// * | |
// * For the full copyright and license information, please view the LICENSE | |
// * file that was distributed with this source code. |
match self.kind { | ||
InputKind::File(_) => false, | ||
InputKind::Stdin => true, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match self.kind { | |
InputKind::File(_) => false, | |
InputKind::Stdin => true, | |
} | |
matches!(self.kind, InputKind::Stdin) |
} | ||
InputKind::File(_) | InputKind::Stdin => { | ||
if cfg!(unix) { | ||
PathBuf::from(text::DEV_STDIN).canonicalize().ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathBuf::from(text::DEV_STDIN).canonicalize().ok() | |
// Workaround to handle redirects, e.g. `touch f && tail -f - < f` | |
PathBuf::from(text::DEV_STDIN).canonicalize().ok() |
} | ||
#[cfg(windows)] | ||
{ | ||
// use std::os::windows::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// use std::os::windows::prelude::*; | |
// TODO: `file_index` requires unstable library feature `windows_by_handle` | |
// use std::os::windows::prelude::*; |
fn uu_tail(mut settings: Settings) -> UResult<()> { | ||
let dash = PathBuf::from(text::DASH); | ||
|
||
fn uu_tail(settings: &Settings) -> UResult<()> { | ||
// Mimic GNU's tail for `tail -F` and exit immediately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Mimic GNU's tail for `tail -F` and exit immediately |
if (settings.paths.is_empty() || settings.paths.contains(&dash)) && settings.follow_name() { | ||
let mut input_service = InputService::from(settings); | ||
let mut watcher_service = WatcherService::from(settings); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Mimic GNU's tail for `tail -F` and exit immediately |
// dbg!("bounded"); | ||
// dbg!(&settings.mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// dbg!("bounded"); | |
// dbg!(&settings.mode); |
// dbg!("unbounded"); | ||
// dbg!(&settings.mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// dbg!("unbounded"); | |
// dbg!(&settings.mode); |
Like I said this is an intermediate pr. @tertsdiepraam If there's nothing essentially wrong I would like to go on. |
I think we should merge this once this round of (simple) comments have been fixed so that we don't lose track of them. And then we can move to the next PR. |
@tertsdiepraam I took care of every one of your comments, and then I proceeded. The other comments don't add any value, because I already addressed them on my new branch and are included in the follow up pr. |
Alright! Looking forward to the next one! |
Sorry what? I did a review when this PR was marked ready for review. Where is this new branch? How do you expect reviewers to keep track of issues they raise if the issues are not handled in the PR, but some where else? @tertsdiepraam I think that merging a PR with open issues is very unusual. |
I interpreted not adding value as already fixed in a larger change, so in a sense outdated. I see refactoring as an ongoing change anyway, so I think it's fine in this case, but it is indeed unusual. The diff is getting huge and I'd rather review smaller changes from this point onward. So, @Joining7943, please keep it small from now on :) |
`tests/tail`: Fix tests to reflect changes from the refactoring #3905
I feel like the tail.rs file needs some refactoring to make it easier to implement new features. The file grew big over time, so I started with moving functions and structs around and moved them into new modules without changing much of the logic. The tests are all still passing, but there's much more potential for refactorings. Right now, I've spent just a few hours with it. I could dive deeper into the issue, however, I would appreciate to hear some feedback first and maybe you've got some additional ideas, since you've worked on tail much longer than I did.
EDIT:
Here's a quick overview of changes to
tail
in this step:tail.rs
. New modules areWatcher
notify::Watcher
mutable
s fromSettings
. Settings only store the initial values.This PR is part of a larger refactoring, so there is still work that I address in a following PR.