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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 19, 2018, 17:54 UTC
Message-ID: <20180619175422.GA48866@syl.attlocal.net>
In-Reply-To: <20180619174846.GA27820@sigill.intra.peff.net>

On Tue, Jun 19, 2018 at 01:48:47PM -0400, Jeff King wrote:

Show 7 quoted lines

> On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
> > 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.

Agreed. I would prefer _not_ to apply the patch that I sent to René, since I think it adds more complexity than its worth (precisely because the short-circuiting logic is known, though certainly the problem gets worse as the expression tree grows).

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

Agreed again, but it's worth noting that --color is the default. To play devil's advocate, the use-case that I imagine will be most common is with "git jump," so perhaps that doesn't matter as much.

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.

Piggy-backing on what I said to René, agreed again.

> -Peff

Thanks, Taylor