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

Add the m option (from grep). #93

Closed
wants to merge 1 commit into from
Closed

Conversation

ovhpa
Copy link

@ovhpa ovhpa commented Sep 11, 2023

I wanted to stop agrep after N matches, so I add a -m option as close as possible to that of the GNU grep.

I'm sharing this in the hope that it will be useful to someone else.


Note: When using -m combined with the -B option, agrep will look for the N-best candidates.

@trushworth
Copy link
Collaborator

This looks reasonable, but I want to test it offline first and it will be a week or two before I can do that. Thanks for this (in advance)!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This entire hunk is plagiarized from the GNU grep manual and cannot be used. Not just because of the blatant copyright violation, but also because it does not accurately describe the added functionality.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it was included by accident I think, sorry for that m(_ _)m

I had decided to not include any change to the manual page, but it must have slip through somehow.
Also, I don't get why there are so many changes...

Sorry again, as I said below, you can safely ignore this PR.

@@ -652,6 +666,10 @@ Copyright (c) 2001-2009 Ville Laurikari <vl@iki.fi>.\n"));
if (show_help)
tre_agrep_usage(0);

/* Fail without reading if count number is 0 */
if (max_count == 0)
return EXIT_FAILURE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use EXIT_SUCCESS or EXIT_FAILURE elsewhere, so we shouldn't use them here.

Copy link
Author

Choose a reason for hiding this comment

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

Then would return 1; be more appropriate?

/* exit after max_count match(es) */
if((max_count > 0)&&(count > (max_count-1))) {
if(count_matches) break;/* to get the result */
else exit(0);/* to leave the stream "as is" */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exiting here produces the wrong result when multiple inputs are provided and at least one of them has max_count or more matching lines. The correct thing to do is break in all cases. Moreover, we're reading from a file descriptor, not a stream, and since we read a variable amount of data (but at least 10 kB) at a time, there is no guarantee that the input will be left in any particular state when we reach the match limit.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I added the ugly else exit(0); as a quick fix for my use case.

There were some trouble either when piping or using multiple files, but I can't recall what problems exactly, sorry m(_ _)m

dag-erling added a commit to dag-erling/tre that referenced this pull request Sep 4, 2024
This behaves similarly to the equivalent option in GNU and BSD grep, i.e.
stops processing each input as soon as the specified number of matches is
reached.

Fixes laurikari#93.
@ovhpa
Copy link
Author

ovhpa commented Sep 4, 2024

Bonjour @dag-erling,

Thank you very much for your review.
I added that option quickly for a use case.
But to be honest, it was a single-use and quite a long time ago so I forgot about the details, I am very sorry for that m(_ _)m

Anyway, your PR is surely more thoughtful so you can ignore this one!

Have a nice day,

@dag-erling dag-erling closed this Sep 4, 2024
@dag-erling
Copy link
Collaborator

merged #110 instead

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.

3 participants