Re: [PATCH V2 4/5] git-grep: Learn PCRE

From: Michal Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: May 5, 2011, 07:41 UTC
Message-ID: <20110505094147.22e3d158@mkiedrowicz>
In-Reply-To: <4DC2418E.4070006@viscovery.net>

On 05.05.2011 08:19:58 +0200 Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 15 quoted lines

> Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:
> > +# Define NO_LIBPCRE if you do not have libpcre installed.
> > git-grep cannot use +# Perl-compatible regexes.
> 
> For what purpose are you adding Perl-regex when git-grep cannot use
> them?
> 
> ...
> 
> Oh! You mean to say "..., but git-grep cannot use Perl-compatible
> regexes _in this case_".
> 
> ;)
> 
> This repeats in patch 5/5.

I took this from NO_CURL and NO_EXPAT descriptions:

	 Define NO_CURL if you do not have libcurl installed.
	 git-http-pull and git-http-push are not built, and you
	 cannot use http:// and https:// transports.

	Define NO_EXPAT if you do not have expat installed.
	git-http-push is not built, and you cannot push using
	http:// and https:// transports.

But I can reword it :).

Show 11 quoted lines

> 
> > +#ifdef NO_LIBPCRE
> > +static void compile_pcre_regexp(struct grep_pat *p, struct
> > grep_opt *opt) +{
> > +	die("cannot use Perl-compatible regexes when libpcre is
> > not compiled in");
> 
> This is such a terminus technicus. Wouldn't it be much easier to read
> for Joe User if this were merely:
> 
> 	die("Perl-compatible regexes not supported");

I can argue. My message says why they aren't available, while your proposition leaves Joe without a clue :). But I'm not that much attached to it, I can change it.

Show 5 quoted lines

> 
> Also, wouldn't it be nicer to die already when the --perl-regexp
> option is detected? Then you could make these functions dummies that
> behave as if nothing was matched.
>

I prefer to die() at lowest possible level in case someone wants to use these functions in another way (e.g. from revision-walking code).

But I can abstract these calls to die_pcre_not_supported() to not repeat die() message.

Show 14 quoted lines

> > +}
> > +
> > +static int pcrematch(struct grep_pat *p, char *line, char *eol,
> > +		regmatch_t *match, int eflags)
> > +{
> > +	die("cannot use Perl-compatible regexes when libpcre is
> > not compiled in"); +}
> > +
> > +static void free_pcre_regexp(struct grep_pat *p)
> > +{
> > +	die("cannot use Perl-compatible regexes when libpcre is
> > not compiled in"); +}
> 
> -- Hannes

Thanks for comments :)