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

From: Martin Ågren <martin.agren@gmail.com>
Date: May 14, 2022, 18:16 UTC
Message-ID: <CAN0heSoCiNnrknyfE7RrsLKcGcGqDYo9k9ubzcEo1r+CxO1hVQ@mail.gmail.com>
In-Reply-To: <pull.1264.git.git.1652361610103.gitgitgadget@gmail.com>

Hi Carlos,

Welcome to the mailing list. :-)

On Thu, 12 May 2022 at 21:13, Carlos L. via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 9 quoted lines

>
> From: =?UTF-8?q?Carlos=20L=C3=B3pez?= <00xc@protonmail.com>
>
> 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)).

Makes sense to me.

Show 9 quoted lines

> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 3d393fbac1b..02b36046475 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -23,6 +23,7 @@ SYNOPSIS
>            [--break] [--heading] [-p | --show-function]
>            [-A <post-context>] [-B <pre-context>] [-C <context>]
>            [-W | --function-context]
> +          [-m | --max-count <num>]

I think this should be

    [(-m | --max-count) <num>]

since the short form "-m" also wants to take "<num>".

Show 5 quoted lines

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

Please use `backticks` with `-v` and `--invert-match` so that they are set in monospace.

Regarding the special value 0, it's a bit unclear what "has no effect" means. In particular, it can have an effect in the sense that when it is used like

  git grep -m 1 -m 0 foo

it undoes the `-m 1`.

But also, that's not how my grep(1) behaves: with `-m 0`, it limits the number of matches to zero. I don't know how useful that is (can that zero-case be optimized by exiting with 1 before even trying to find the needle!?), or if maybe different variants of grep handle this differently? If all grep implementations handle 0 by actually only emitting zero hits, I think it would be wise for us to handle 0 the same way.

As for overriding an earlier `-m <foo>`, which could be useful, it seems to me like `--no-max-count` would make sense.

All in all, I would suggest the following documentation:

    -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. Use `--no-max-count` to countermand an
           earlier `--max-count` option on the command line.

... and of course the matching implementation. :-) Maybe you could achieve that by using -1 to signal that there's no max-count in play? How does that sound to you?

Even if we want to handle the zero just like you do, I think this patch needs a few tests. We should make sure to test the 0-case (whatever we end up wanting it to behave like), and probably the "suppress an earlier -m by giving --no-max-count" case. It also seems wise to set up some test scenario where there are several files involved so that we can see that we don't just print the first m matches *globally*, but that the counter is really handled *per file*.

I think this `-m` flag would be a nice addition. I know that I've been missing something like it a few times. As you wrote in your commit message, `| head -3` can work for some use-cases, but definitely not for others. This `-m` is a lot more granular than `-l` which can be seen as a crude `-m 1`. Thanks for posting this patch! I hope you find my comments useful.

Martin