Re: BUG: git grep behave oddly with alternatives

From: Jeff King <peff@peff.net>
Date: Jan 6, 2023, 09:09 UTC
Message-ID: <Y7flVcALZQgz0VPl@coredump.intra.peff.net>
In-Reply-To: <e5165840-331c-e9b6-b45f-62abab860d79@web.de>

On Wed, Jan 04, 2023 at 05:36:21PM +0100, René Scharfe wrote:

Show 19 quoted lines

> > I didn't test, but just from looking at the patch I'd expect this to
> > affect other parts of Git besides git-grep. E.g., "git log --grep".
> > Which raises two questions:
> >
> >  - would a more generalized name be better? USE_REG_ENHANCED or
> >    something? That might be _too_ general, but see below.
> >
> >  - should this cover other cases? Grepping for "regcomp", would people
> >    want this to behave consistently for "git config --get-regexp", or
> >    diff funcnames, and so on?
> >
> > If so, then I could envision a USE_REG_ENHANCED which just wraps the
> > system regcomp and adds the REG_ENHANCED flag when REG_EXTENDED is not
> > set?
> 
> Good point.  I don't know what people want, though.  re_format(7) on
> macOS/BSD and regex(7) on Linux call basic REs "obsolete" and extended
> REs "modern", so they seem to push people away from the old kind,
> enhanced or not.

Oh, good point. I was just grepping for regcomp(), but of course any case which is already passing REG_EXTENDED would not be affected anyway. And most places are already using that. E.g., the config code always does so, and it looks like pickaxe "-G" does so.

For diffs, we have diff.*.xfuncname, which uses EREs. We do still support regular "funcname" for backwards compatibility, but we only document the extended version. Ironically, that option was introduced because BREs did not portably support things like alternation, even with the "enhanced" syntax. ;) See 45d9414fa5 (diff.*.xfuncname which uses "extended" regex's for hunk header selection, 2008-09-18).

So I think we are embracing the "everyone should use EREs" mentality already. The only spots I see that use BREs are:

  - grep.c, which handles "git grep" and "git log --grep"

  - line-range.c, presumably for "-L" function matching

  - deprecated non-ERE funcname patterns

Your patch is handling the first, which is by the far most important. I would be OK leaving the others as-is, but I also wouldn't mind a patch that works at the regcomp() level to make things automatically consistent.

-Peff