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

From: Jeff King <peff@peff.net>
Date: Jun 19, 2018, 16:35 UTC
Message-ID: <20180619163506.GB22034@sigill.intra.peff.net>
In-Reply-To: <cover.1529365072.git.me@ttaylorr.com>

On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:

Show 9 quoted lines

> Attached is a ``fresh start'' of my series to teach 'git grep --column'.
> Since the last time I sent this, much has changed, notably the semantics
> for deciding which column is the first when given (1) extended
> expressions and (2) --invert.
> 
> Both (1) and (2) are described in-depth in patch 2/7, but I am happy to
> answer more questions should they arise here. Peff and I worked on this
> together off-list, and we are both happy with the semantics, and believe
> that it covers most reasonable cases.

So with the exception of some minor type-quibbling in patch 4, this all looks good to me. Since we discussed this quite a bit off-list, you can take that review either with a giant grain of salt (reviewing something I helped to generate in the first place) or a ringing endorsement (I thought about it a lot more than I would have for a normal review ;) ).

Show 20 quoted lines

> The notable case that it does _not_ cover is matching the following
> line:
> 
>   a ... b
> 
> with the following expression
> 
>   git grep --column -e b --or -e a
> 
> This will produce the column for 'b' rather than the column for 'a',
> since we short-circuit an --or when the left child finds a match, in
> this case 'b'. So, we break the semantics for this case, at the benefit
> of not having to do twice the work.
> 
> In the future, I'd like to revisit this, since any performance gains
> that we _do_ make in this area are moot when we rescan all lines in
> show_line() with --color. A path forward, I imagine, would look like a
> list of regmatch_t's, or a set of locations in the expression tree, such
> that we could either enumerate the list or walk the tree in order to
> colorize the line. But, I think for now that is #leftoverbits.

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()).

-Peff