git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: "Carlos L. via GitGitGadget" <gitgitgadget@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Carlos López" <00xc@protonmail.com>
Subject: Re: [PATCH] grep: add --max-count command line option
Date: Sat, 14 May 2022 20:16:05 +0200	[thread overview]
Message-ID: <CAN0heSoCiNnrknyfE7RrsLKcGcGqDYo9k9ubzcEo1r+CxO1hVQ@mail.gmail.com> (raw)
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:
>
> 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.

> 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

  reply	other threads:[~2022-05-14 18:16 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-12 13:20 [PATCH] grep: add --max-count command line option Carlos L. via GitGitGadget
2022-05-14 18:16 ` Martin Ågren [this message]
2022-05-16  5:57 ` Junio C Hamano
2022-05-16  7:28   ` Paul Eggert
2022-05-16  8:38     ` Carlos L.
2022-05-16 15:36       ` Junio C Hamano
2022-05-17  5:53         ` Paul Eggert
2022-05-16 15:18     ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2022-06-20 15:49 Carlos L. via GitGitGadget
2022-06-20 15:57 ` Paul Eggert
2022-06-20 16:25   ` Carlos L.
2022-06-20 16:32     ` Paul Eggert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN0heSoCiNnrknyfE7RrsLKcGcGqDYo9k9ubzcEo1r+CxO1hVQ@mail.gmail.com \
    --to=martin.agren@gmail.com \
    --cc=00xc@protonmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).