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: Reactivate ---presume-input-pipe option #3959

Merged

Conversation

Joining7943
Copy link
Contributor

During #3905, I missed to reactivate the ---presume-input-pipe option. This fix will reactivate it.

@Joining7943 Joining7943 marked this pull request as draft September 20, 2022 19:13
@tertsdiepraam
Copy link
Member

tertsdiepraam commented Sep 21, 2022

Okay, seems like I was a bit quick on the merge last time, we should at least have the same features with each PR. Any other missing functionality from that PR? And is there a reason to keep this as a draft or is it ready (when the conflicts are fixed)?

@Joining7943 Joining7943 force-pushed the tail-reactivate-presume-input-pipe branch from 9ffdb9a to c32d61d Compare September 21, 2022 14:42
@Joining7943
Copy link
Contributor Author

Joining7943 commented Sep 21, 2022

Okay, seems like I was a bit quick on the merge last time, we should at least have the same features with each PR. Any other missing functionality from that PR?

Sorry for that. No, ---presume-input-pipe is the only missing functionality. This was still a draft because I needed tests and a way to detect that we're not using the wrong function. Additionally, I was a little bit confused by some performance tests when I used ---presume-input-pipe, so I needed additional checks to verify we're doing the right thing.

@Joining7943 Joining7943 marked this pull request as ready for review September 21, 2022 14:50
@Joining7943
Copy link
Contributor Author

Joining7943 commented Sep 22, 2022

@tertsdiepraam Not meant badly, but I don't think this option is crucial. I guess gnu tail has this hidden, undocumented option because there is a perfomance difference between tail ---presume-input-pipe random_ascii_505MB and tail random_ascii_505MB.

❯ wc --lines --words --bytes --chars --max-line-length random_ascii_505MB            
  5000000  12650766 505000000 505000000       100 random_ascii_505MB

Maybe they used the option for testing purposes. However, we don't have this performance difference. Tailing this file with bounded_tail() or unbounded_tail() is comparable fast in our version of tail.

@tertsdiepraam
Copy link
Member

Oh I agree that it's not very useful, but if we want to claim full compatibility, then this option should be included and should probably do what it implies. The testing argument could also apply to us. However, there are indeed also some of those triple dash arguments we silently ignore because they don't apply.

@Joining7943
Copy link
Contributor Author

Sure. No problem. I didn't want to stress :) Just for completeness and maybe it helps: When I readded the functionality, I was looking at the code of tail in gnu's coreutils and I'm very sure, that it's all about not using file seeks. It's a little bit buried, but I think it can be stripped down to: in case of ---presume-input-pipe, they use pipe_lines(), pipe_bytes(), we use unbounded_tail().

@tertsdiepraam
Copy link
Member

That seems to match our previous investigations into this (see #2907 (comment)), so I think you're correct!

@Joining7943
Copy link
Contributor Author

@tertsdiepraam Are we ready here?

@tertsdiepraam tertsdiepraam merged commit 4b517a3 into uutils:main Sep 27, 2022
@tertsdiepraam
Copy link
Member

Yeah, all good!

@Joining7943 Joining7943 deleted the tail-reactivate-presume-input-pipe branch September 29, 2022 20:47
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