Re: [PATCH] grep: add --max-count command line option

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2022, 05:57 UTC
Message-ID: <xmqqilq658b3.fsf@gitster.g>
In-Reply-To: <pull.1264.git.git.1652361610103.gitgitgadget@gmail.com>

"Carlos L. via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: =?UTF-8?q?Carlos=20L=C3=B3pez?= <00xc@protonmail.com>

Offtopic, but I wonder why this line is encoded like so? The "Signed-off-by:" line is not, and it is safely transmitted, so it feels like we do not need to encode the in-body header that is added only for e-mail but not in the original commit...

Show 16 quoted lines

> This patch adds a command line option analogous to that of GNU
> grep(1)'s -m / --max-count, which users might already be used to.
> This makes it possible to limit the amount of matches shown in the
> output while keeping the functionality of other options such as -C
> (show code context) or -p (show containing function), which would be
> difficult to do with a shell pipeline (e.g. head(1)).
>
> Signed-off-by: Carlos López <00xc@protonmail.com>
> ---
> ...
> +-m <num>::
> +--max-count <num>::
> +	Limit the amount of matches per file. When using the -v or
> +	--invert-match option, the search stops after the specified
> +	number of non-matches. Setting this option to 0 has no effect.
> +

Good thing that this is defined as "per-file" limit. If it were a global limit, the interaction between this one and "--threads=<num>" would have been interesting. Perhaps add a test to make sure the feature continues to work with "--threads=2" (I am assuming that you have already tested this implementation works with the option).

Martin already commented on the wording "no effect"; I agree it is a poor choice of words from the point of view of "overriding with 0".

It indeed is curious why GNU grep chose to immediately exit with 1 when "-m 0" was given, but that was decision made more than 20 years ago (http://gnu.ist.utl.pt/software/grep/changes.html and look for "2000-03-17"). Between "being consistent even with a seemingly useless design choice made by somebody else" and "choose to be different in a corner case where nobody should care and allow us to be more useful", I am slightly in favor in this particular case.

What "git grep -m -1" should do? IIRC, OPT_INTEGER is for signed integer but the new .max_count member, as well as the existing "count" that is compared with it, are of "unsigned" type. Either erroring out or treating it as unlimited is probably fine, but whatever we do, we should document and have a test for it.

Thanks.