Refactor tail by Joining7943 · Pull Request #3905 · uutils/coreutils

Joining7943 · 2022-09-05T19:36:17Z

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:

Extraction of smaller specialized modules from tail.rs. New modules are
- paths: Containing logic around paths and inputs
- args: Deal with argument parsing
- follow/files: Logic around the paths followed by a Watcher
- follow/watch: Contains the logic around following files and wraps a notify::Watcher
- text: Small module to contain static strings used in every module
Extraction of structs and methods in general
Rewrite and compact the tail::uu_tail function
Deal with stdin only when necessary, so when it is given on the commandline.
Remove mutables from Settings . Settings only store the initial values.
Do not use PathBufs to store strings like "-"
Simplify nested structures where possible

This PR is part of a larger refactoring, so there is still work that I address in a following PR.

tertsdiepraam · 2022-09-05T20:14:30Z

cc @jhscheer, who implemented #2695

tertsdiepraam

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!

tertsdiepraam · 2022-09-05T22:44:41Z

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!

Maybe it makes more sense to re-export uu_app from tail?

Cool. I haven't seen that.

jhscheer · 2022-09-06T12:04:13Z

Hi,
I'm with Terts, refactors are always welcome. However, there're a few things to watch out for.

I feel like the tail.rs file needs some refactoring to make it easier to implement new features.

What features are you thinking of?

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 there're two CI monitored tests that don't pass in this refactor.
A lot of tail.rs grew organically to pass various tests from GNU's test suite. I transferred most of GNU's tests to our test frame work, but passing the test frame work does not mean passing GNU's tests. And to complicate things even more, not all of GNU's test suite tests that uu_tail passes are monitored by the CI (i.e. they pass on a local Linux, but not in the CI container). I mentioned this in tail's README.
To summarise, please regularly run GNU's tests after bigger changes to see if the relevant tests still pass.

Right now, I've spent just a few hours with it.

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.

Joining7943 · 2022-09-06T16:23:06Z

What features are you thinking of?

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 todo()s and adding currently missing implementations for other platforms than linux, maybe also fixing some of the currently failing gnu tests.

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.

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.

Joining7943 · 2022-09-07T11:09:32Z

Is breaking the test_tail.rs file in smaller related parts, like test_tail/test_tail_follow.rs, test_tail/test_tail_args etc. an option? It would simplify spotting missing tests. Naming the files test_tail_... won't break the new_ucmd, util_name macros. It's possible without changes in the build script, but then there would be a test_tail.rs file and a folder test_tail in the test directory. The other approach would involve a small addition to the build script which checks if there is a directory "test_tail" (in case of tail) in the test directory and if so adds the path test_tail/test_tail.rs to the test_modules.rs file. If there is no such directory the file test_tail.rs within the root of the test directory is added to test_modules like before. See also my last commit 4253aa46e24374a139317299fb267ab41d63605c as a quick solution.

tertsdiepraam · 2022-09-07T11:23:20Z

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?

jhscheer · 2022-09-07T11:57:08Z

Is breaking the test_tail.rs file in smaller related parts, like test_tail/test_tail_follow.rs, test_tail/test_tail_args etc. an option? It would simplify spotting missing tests.

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.

Joining7943 · 2022-09-07T12:34:37Z

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 test_tail_follow to run all tests in the file test_tail/test_tail_follow.rs and the test names dont' have to necessarily include test_follow.

jhscheer · 2022-09-07T15:28:33Z

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 test_tail_follow to run all tests in the file test_tail/test_tail_follow.rs and the test names dont' have to necessarily include test_follow.

But that can already be done with modules, like what you did with mod pipe_tests.

Joining7943 · 2022-09-07T17:03:30Z

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.

tertsdiepraam · 2022-09-07T17:06:17Z

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.

tertsdiepraam

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.

tertsdiepraam · 2022-09-09T11:52:34Z

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.

tertsdiepraam · 2022-09-09T11:54:19Z

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.

tertsdiepraam · 2022-09-09T11:58:09Z

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?

tertsdiepraam · 2022-09-09T12:01:13Z

All of these Option/Result things are in the prelude so don't need to be imported right?

tertsdiepraam · 2022-09-09T12:05:12Z

This feels like it should be a free standing function

Joining7943 · 2022-09-09T12:50:34Z

@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.

tertsdiepraam · 2022-09-09T18:45:39Z

That sounds excellent! My comments were then a bit premature :)

Joining7943 · 2022-09-10T22:30:38Z

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.

tertsdiepraam · 2022-09-10T22:43:19Z

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.

Joining7943 · 2022-09-12T19:05:03Z

@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 tail - < dir. Like you suggested this'll be an intermediate pr. I'll fix the error, but this doesn't involve bigger changes. I'm also going to write a more exhaustive list of changes in the initial post of this pr.

tertsdiepraam

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!

tertsdiepraam · 2022-09-12T21:37:09Z

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.

tertsdiepraam · 2022-09-12T21:41:19Z

I really like this cleanup of FilterMode!

I just realised you could use or-patterns here:

FilterLines::Lines(SigNum::PlusZero | SigNum::Positive(1), _)

Nice. Looks a little bit more compact.

tertsdiepraam · 2022-09-12T21:49:48Z

Could you document the return value here?

Sure. There's a lot of documenation missing in general. I can address this at the end of the next iteration.

tertsdiepraam · 2022-09-12T21:52:26Z

Suggested change

match input.kind() {

InputKind::Stdin => continue,

InputKind::File(path) => {

if let InputKind::File(path) = input.kind() {

Don't you think it adds some clearity keeping the match written out, here?

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.

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.

tertsdiepraam · 2022-09-12T21:54:41Z

What's going on with all the unused values? Can we remove them?

I'm pretty sure we can remove some or all of them, and this is certainly something we should address in the next pr.

tertsdiepraam · 2022-09-12T22:05:30Z

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.

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.

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.

It looks like you removed all TODOs, even those that were there before this refactor. This is very inconvenient.

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 :)

tertsdiepraam · 2022-09-12T22:08:34Z

Please explain this todo. Is it about the function? If it's really not used it should indeed be removed.

Tangentially related (and should not be fixed in this PR), but it feels like the platform module should handle this case correctly for Windows.

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.

tertsdiepraam · 2022-09-12T22:10:58Z

Can we make parse_num return UResult<Signum>?

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.

tertsdiepraam · 2022-09-12T22:14:46Z

Why is unwrap okay for these methods?

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.

Joining7943 · 2022-09-13T03:40:35Z

Thanks for this great review!

Have you checked the compared GNU tests locally as well with main, to ensure nothing broke that's not in the CI?

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.

Also did we break these recently or have they never passed? In both cases well done!
Thanks. Imho they pass since this refactoring and cleanup, but I haven't picked out these tests intentionally, so I can't say for sure.

jhscheer · 2022-09-13T15:02:15Z

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!

Imho they pass since this refactoring and cleanup, but I haven't picked out these tests intentionally, so I can't say for sure.

coreutils/src/uu/tail/README.md

Lines 39 to 46 in 78a9f6e

    
           There are 5 tests which are fixed but do not (always) pass the test suite if it's run inside the CI. 
        
           The reason for this is probably related to load/scheduling on the CI test VM. 
        
           The tests in question are: 
        
           - [x] `tail-2/F-vs-rename.sh` 
        
           - [x] `tail-2/follow-name.sh` 
        
           - [x] `tail-2/inotify-rotate.sh` 
        
           - [x] `tail-2/overlay-headers.sh` 
        
           - [x] `tail-2/retry.sh`

There maybe others but I stopped keeping track since I made the mention in the README.

…utils#3905 for details

jhscheer

Except for the nit-picks, this looks very good to me.

jhscheer · 2022-09-15T01:45:50Z

+use std::collections::hash_map::Keys;
+use std::collections::HashMap;


Suggested change

use std::collections::hash_map::Keys;

use std::collections::HashMap;

use std::collections::{hash_map::Keys, HashMap};

jhscheer · 2022-09-15T01:48:26Z

+use std::sync::mpsc;
+use std::sync::mpsc::{channel, Receiver};


Suggested change

use std::sync::mpsc;

use std::sync::mpsc::{channel, Receiver};

use std::sync::mpsc::{self, channel, Receiver};

jhscheer · 2022-09-15T01:49:21Z

+//  * 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.


Suggested change

// * 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.

jhscheer · 2022-09-15T01:50:17Z

+        match self.kind {
+            InputKind::File(_) => false,
+            InputKind::Stdin => true,
+        }


Suggested change

match self.kind {

InputKind::File(_) => false,

InputKind::Stdin => true,

}

matches!(self.kind, InputKind::Stdin)

jhscheer · 2022-09-15T01:52:55Z

+            }
+            InputKind::File(_) | InputKind::Stdin => {
+                if cfg!(unix) {
+                    PathBuf::from(text::DEV_STDIN).canonicalize().ok()


Suggested change

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()

jhscheer · 2022-09-15T01:57:30Z

+        }
+        #[cfg(windows)]
+        {
+            // use std::os::windows::prelude::*;


Suggested change

// use std::os::windows::prelude::*;

// TODO: `file_index` requires unstable library feature `windows_by_handle`

// use std::os::windows::prelude::*;

jhscheer · 2022-09-15T02:00:32Z

-    let dash = PathBuf::from(text::DASH);
-
+fn uu_tail(settings: &Settings) -> UResult<()> {
    // Mimic GNU's tail for `tail -F` and exit immediately


Suggested change

// Mimic GNU's tail for `tail -F` and exit immediately

jhscheer · 2022-09-15T02:01:11Z

-    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);
+


Suggested change

// Mimic GNU's tail for `tail -F` and exit immediately

jhscheer · 2022-09-15T02:01:56Z

+    // dbg!("bounded");
+    // dbg!(&settings.mode);


Suggested change

// dbg!("bounded");

// dbg!(&settings.mode);

jhscheer · 2022-09-15T02:02:19Z

+    // dbg!("unbounded");
+    // dbg!(&settings.mode);


Suggested change

// dbg!("unbounded");

// dbg!(&settings.mode);

Joining7943 · 2022-09-15T10:11:23Z

Like I said this is an intermediate pr. @tertsdiepraam If there's nothing essentially wrong I would like to go on.

tertsdiepraam · 2022-09-15T15:51:15Z

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.

Joining7943 · 2022-09-17T15:24:27Z

@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.

tertsdiepraam · 2022-09-17T18:40:07Z

Alright! Looking forward to the next one!

jhscheer · 2022-09-17T20:44:50Z

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.

Sorry what?

I did a review when this PR was marked ready for review.
Why do my comments not add any value? What does that even mean?

Where is this new branch?
Why did I spend time to address issues if they were already fixed some where?

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.

tertsdiepraam · 2022-09-17T20:57:25Z

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 :)

…3905

`tests/tail`: Fix tests to reflect changes from the refactoring #3905

tertsdiepraam reviewed Sep 5, 2022

View reviewed changes

tertsdiepraam reviewed Sep 9, 2022

View reviewed changes

tertsdiepraam reviewed Sep 12, 2022

View reviewed changes

tail: large refactoring and cleanup of the tail code base. See also u…

951c51e

…utils#3905 for details

Joining7943 force-pushed the refactor-tail branch from 073de92 to 951c51e Compare September 13, 2022 21:01

Joining7943 marked this pull request as ready for review September 14, 2022 06:39

jhscheer reviewed Sep 15, 2022

View reviewed changes

tertsdiepraam merged commit 8c83988 into uutils:main Sep 17, 2022

This was referenced Sep 18, 2022

tail: Refactor tail #3952

Closed

Fix random errors for piped input on macos #3953

Merged

tail: Reactivate ---presume-input-pipe option #3959

Merged

tests/tail: Fix tests to reflect changes from the refactoring #3905 #3965

Merged

Joining7943 deleted the refactor-tail branch September 29, 2022 20:49

sylvestre pushed a commit to Joining7943/uutil-coreutils that referenced this pull request Oct 5, 2022

tests/tail: Fix tests to reflect changes from the refactoring uutils#…

1725151

…3905

sylvestre added a commit that referenced this pull request Oct 6, 2022

Merge pull request #3965 from Joining7943/fix-windows-pipe

97f3dfa

`tests/tail`: Fix tests to reflect changes from the refactoring #3905

-                match input.kind() {
-                    InputKind::Stdin => continue,
-                    InputKind::File(path) => {
+                if let InputKind::File(path) = input.kind() {

		use std::collections::hash_map::Keys;
		use std::collections::HashMap;

	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};

	use std::sync::mpsc;
	use std::sync::mpsc::{channel, Receiver};
	use std::sync::mpsc::{self, channel, Receiver};

	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()

	// use std::os::windows::prelude::*;
	// TODO: `file_index` requires unstable library feature `windows_by_handle`
	// use std::os::windows::prelude::*;

Uh oh!

Conversation

Joining7943 commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Sep 5, 2022

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhscheer commented Sep 6, 2022

Uh oh!

Joining7943 commented Sep 6, 2022

Uh oh!

Joining7943 commented Sep 7, 2022

Uh oh!

tertsdiepraam commented Sep 7, 2022

Uh oh!

jhscheer commented Sep 7, 2022

Uh oh!

Joining7943 commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhscheer commented Sep 7, 2022

Uh oh!

Joining7943 commented Sep 7, 2022

Uh oh!

tertsdiepraam commented Sep 7, 2022

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joining7943 commented Sep 9, 2022

Uh oh!

tertsdiepraam commented Sep 9, 2022

Uh oh!

Joining7943 commented Sep 10, 2022

Uh oh!

tertsdiepraam commented Sep 10, 2022

Uh oh!

Joining7943 commented Sep 12, 2022

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Joining7943 commented Sep 5, 2022 •

edited

Loading

Joining7943 commented Sep 7, 2022 •

edited

Loading

tertsdiepraam Sep 13, 2022 •

edited

Loading