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

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2022, 15:36 UTC
Message-ID: <xmqq1qwt5w2e.fsf@gitster.g>
In-Reply-To: <MHNbacVw7D6ZU3OJvgIqqRMu70HlgYIYQPduUEUnzWCqkGUsUGRLopGGWj-CbyjNilDcUfLB6elfSRgDOaob9cPpjeAf-I6xuMArQZ0y3io=@protonmail.com>

"Carlos L." <00xc@protonmail.com> writes:

Show 9 quoted lines

>> 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.
>
> This seems sound. Is there any documentation on how to write tests for git?

t/README and Documentation/MyFirstContribution would be two good places to start.

Show 10 quoted lines

>> 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.
>
> I would favor treating it as an error. As mentioned above, using 0
> to describe "unlimited matches" (e.g. the default) is my
> preference, but I am willing to concede if someone can think of a
> good use for `-m 0`.

With Devil's advocate hat on.

"GNU grep has been doing so for the past 20 years and existing users of the command expects '-m 0' to behave that way" is a good enough reason, especially if '-m 0' is not the only possible way to say "unlimited".

> Also, from the implementation side (although
> not as important) it looks better: if we allow negative values, we
> need to distinguish between -1 (unlimited) and -4 (display error
> to user, probably)

If we are going to document "you can pass a negative value to explicitly say 'unlimited', which is a useful way to countermand another `-m <num>` that appear earlier on the command line", then -1 and -4 would equally be 'unlimited' and there is no need to distinguish anything.

Devil's advocate hat off.

I personally do not mind if "-m <non-positive>" means "unlimited", as long as that is clearly documented and tested, but for long time "GNU grep" users "-m 0" might appear surprising (not necessarily because they would find that the "-m 0" that immediately fails is useful, but because the behaviour is deliberately made different).

Thanks.