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

On 05.05.2011 08:19:58 +0200 Johannes Sixt [off-list ref] wrote:

Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:

quoted

+# 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 :).

quoted

+#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.

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.

quoted

+}
+
+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 :)