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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jun 21, 2018 at 07:53:02AM -0400, Jeff King wrote:
> On Wed, Jun 20, 2018 at 03:05:30PM -0500, Taylor Blau wrote:
>
> > Hi,
> >
> > Here is a re-roll of my series to add --column to 'git-grep(1)'. Since
> > last time, not much has changed other than the following:
> >
> >   - Fix a typo where 'col', 'icol' were spelled as 'match', 'imatch'
> >     [1].
> >
> >   - Disable short-circuiting OR when --column is given [2].
>
> If we're going to do this, should we be short-circuiting AND, too?
>
> Handling just OR makes this work:
>
>   $ ./git grep --column -e scalable --or -e fast -- README.md
>   README.md:7:Git - fast, scalable, distributed revision control system
>   README.md:10:Git is a fast, scalable, distributed revision control system with an
>
> but not this:
>
>   $ ./git grep --column -v --not -e scalable --and --not -e fast -- README.md
>   README.md:13:Git - fast, scalable, distributed revision control system
>   README.md:16:Git is a fast, scalable, distributed revision control system with an
>
> I wasn't sure where we landed in the discussion on "how much crazy stuff
> to support". But AFAIK, the code in this iteration handles every crazy
> case already except this one. If we're going to care about OR, maybe we
> should just cover all cases.

Good catch. As I understand it, we need to short-circuit AND because an
--invert or a --not above it in the expression tree would cause it to be
turned into an OR by de Morgan's Law, and therefore be susceptible to
the same reasoning that caused me to remove short-circuiting on OR.

> > @@ -1429,7 +1447,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
> >  	 */
> >  	if (opt->columnnum && cno) {
> >  		char buf[32];
> > -		xsnprintf(buf, sizeof(buf), "%d", cno);
> > +		xsnprintf(buf, sizeof(buf), "%zu", cno);
>
> Unfortunately %z isn't portable. You have to use:
>
>   xsnprintf(buf, sizeof(buf), "%"PRIuMAX, (uintmax_t)cno);

I see. Per your next note, I've changed this to "%"PRIuMAX (and the
appropriate cast into uintmax_t), and will send another patch out in the
future changing it _back_ to "%zu" and figure out how folks feel about
it.

I published these changes on my fork at [1], and will let this thread
sit overnight again to see if anyone has any more feedback. Thank you!

> -Peff
Thanks,
Taylor

[1]: https://github.com/ttaylorr/git/compare/tb/grep-colno