Am 19.06.2018 um 19:48 schrieb Jeff King: > 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. > 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. >> 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). >> 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é
