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

From: Carlos L. <00xc@protonmail.com>
Date: Jun 22, 2022, 06:41 UTC
Message-ID: <XWF-gaPGsSObqE_fNBKYQ-577E-oM5mMtmnDeGE-oCRmjjTSnByfmgGwesPecoAAVN9B2Gk1mA-Hu-KVyGyPiwzUxgC4Yc1HUetOvYoC2YU=@protonmail.com>
In-Reply-To: <xmqq5ykuhthp.fsf@gitster.g>

Hi,

Just a couple of questions.

On Tuesday, June 21st, 2022 at 18:27, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> "Carlos L. via GitGitGadget" gitgitgadget@gmail.com writes:
>
> > 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)).
> >
> > Signed-off-by: Carlos López 00xc@protonmail.com
> > ---
> > ...
> > Documentation/git-grep.txt | 8 ++++++++
> > builtin/grep.c | 9 +++++++++
> > grep.c | 2 ++
> > grep.h | 2 ++
> > 4 files changed, 21 insertions(+)
>
>
> Tests?

Right. Is it OK if I include my test(s) in t/t7810-grep.sh, or should it be a different/new file?

Show 15 quoted lines

> > diff --git a/grep.c b/grep.c
> > index 82eb7da1022..b32ab75cb6b 100644
> > --- a/grep.c
> > +++ b/grep.c
> > @@ -1686,6 +1686,8 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
> > bol = eol + 1;
> > if (!left)
> > break;
> > + if (opt->max_count != -1 && count == opt->max_count)
> > + break;
>
>
> I would have written it "if (0 <= opt->max_count && ...)". What
>
> happens when a trickster asks you to do "git grep -m -2"?

Fair enough. Since it's already optimized out above, is there any reason we need to include zero (<=)?

Show 6 quoted lines

> I guess what I am getting at is if we are better off saying that
> negative means unlimited, instead of special casing -1 like this. I
> didn't think it through so it may be perfectly possible that what
> you wrote makes more sense than "anything negative is unlimited".
>
> I dunno.

I think you're right, I'll adjust my patch.

Best, Carlos