On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote: > 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 ;) ). > 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
