Hi Carlos,
Welcome to the mailing list. :-)
On Thu, 12 May 2022 at 21:13, Carlos L. via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: =?UTF-8?q?Carlos=20L=C3=B3pez?= <00xc@xxxxxxxxxxxxxx>
>
> 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.
> 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>".
> +-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
