Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

From: René Scharfe <l.s.r@web.de>
Date: Jun 19, 2018, 18:50 UTC
Message-ID: <6ab94453-84ef-e269-c320-e102b02d6f3b@web.de>
In-Reply-To: <20180619174846.GA27820@sigill.intra.peff.net>

Am 19.06.2018 um 19:48 schrieb Jeff King:

Show 20 quoted lines

> On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
> 
>>> The key thing about this iteration is that it doesn't regress
>>> performance, because we always short-circuit where we used to. The other
>>> obvious route is to stop short-circuiting only when "--column" is in
>>> effect, which would have the same property (at the expense of a little
>>> extra code in match_expr_eval()).
>>
>> The performance impact of the exhaustive search for --color scales with
>> the number of shown lines, while it would scale with the total number of
>> lines for --column.  Coloring the results of highly selective patterns
>> is relatively cheap, short-circuiting them still helps significantly.
> 
> I thought that at first, too, but I think we'd still scale with the
> number of shown lines. We're talking about short-circuiting OR, so by
> definition we stop the short-circuit because we matched the first half
> of the OR.
> 
> If you stop short-circuiting AND, then yes, you incur a penalty for
> every line. But I don't think --column would need to do that.

Good point. So disabling that optimization for --column (or in even in general) shouldn't be a dramatic loss.

Show 11 quoted lines

> Although there are interesting cases around inversion. For example:
> 
>    git grep --not \( --not -e a --and --not -e b \)
> 
> is equivalent to:
> 
>    git grep -e a --or -e b
> 
> Do people care if we actually hunt down the exact column where we
> _didn't_ match "b" in the first case?  The two are equivalent, but I
> have to wonder if somebody writing the first one really cares.

I'd rather not guess the intentions of someone using such convoluted expressions. ;-)

Negation causes the whole non-matching line to match, with --column reporting 1 or nothing in such a case, right? Or I think doing the same when the operator is applied a second time is explainable.

Show 6 quoted lines

>> Disabling that optimization for --column wouldn't be a regression since
>> it's a new option..  Picking a random result (based on the order of
>> evaluation) seems sloppy and is probably going to surprise users.
> 
> I don't see it as a random result; short-circuiting logic is well
> understood and we follow the user's ordering.

When ORing multiple expressions I don't pay attention to their order as that operator is commutative. Having results depend on that order would at least surprise me.

> I think the place where it's _most_ ugly is "--column --color", where we
> may color the short-circuited value in the second pass.

The double negative example you gave above has that discrepancy as well, but I think in that case it just highlights the different intentions (--color: highlight search terms, --column: show leftmost match).

Show 6 quoted lines

>> We could add an optimizer pass to reduce the number of regular
>> expressions in certain cases if that is really too slow.  E.g. this:
> 
> Yes, we actually discussed this kind of transformation. I think it's way
> out of scope for this patch series, though. If we do anything more, I
> think it should be to disable short-circuiting when --column is in use.

Yep.

René