[PATCH V2 0/5] Add PCRE support to git-grep

[PATCH V2 0/5] Add PCRE support to git-grep

[Michał Kiedrowicz]

This patch series attempts to add PCRE support to git-grep.

Changes from previous version:

* Now this series start from two patches which add the --line-number
  option to SYNOPSIS and bash completion; they can be treated
  separatedly from this series

* All patches have improved descriptions

* All patches have Signed-off-by

* libpcre specific code has been moved to a separate subsection to
  reduce number of #ifdefs; this addresses comments from Junio

* I noticed that even though I call pcre_study(), I didn't use its
  result in pcre_exec(); now this is fixed; this should improve
  git-grep --perl-regexp performance

* --perl-regexp has been added to bash-completion

* Added last patch which adds checks for libpcre to configure

* I repeated `make test` with and without pcre few more times and I
  cannot reproduce results from previous runs; possibly they were
  altered by current system load or cold cache. Now the difference
  is just few seconds. However, I'm still not convinced to link
  libgit.a with libpcre.

Michał Kiedrowicz (5):
  Documentation: Add --line-number to git-grep synopsis
  contrib/completion: --line-number to git grep
  grep: Put calls to fixmatch() and regmatch() into patmatch()
  git-grep: Learn PCRE
  configure: Check for libpcre

 Documentation/git-grep.txt             |    8 ++-
 Makefile                               |   16 +++++
 builtin/grep.c                         |    2 +
 config.mak.in                          |    1 +
 configure.ac                           |   26 ++++++++
 contrib/completion/git-completion.bash |    3 +-
 grep.c                                 |  100 +++++++++++++++++++++++++++++---
 grep.h                                 |    9 +++
 8 files changed, 154 insertions(+), 11 deletions(-)

-- 
1.7.3.4

[PATCH V2 1/5] Documentation: Add --line-number to git-grep synopsis

[Michał Kiedrowicz]

Commit 7d6cb10b ("grep: Add the option '--line-number'", 2011-03-28)
introduced the --line-number option and added its description to OPTIONS
section, but forgot to update SYNOPSIS.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 Documentation/git-grep.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d7523b3..4a58378 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]
 	   [-v | --invert-match] [-h|-H] [--full-name]
 	   [-E | --extended-regexp] [-G | --basic-regexp]
-	   [-F | --fixed-strings] [-n]
+	   [-F | --fixed-strings] [-n | --line-number]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
-- 
1.7.3.4

[PATCH V2 2/5] contrib/completion: --line-number to git grep

[Michał Kiedrowicz]

Commit 7d6cb10b ("grep: Add the option '--line-number'", 2011-03-28)
introduced the --line-number option, but did not add it to
bash-completion.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0df356d..4b2654d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1485,7 +1485,7 @@ _git_grep ()
 		__gitcomp "
 			--cached
 			--text --ignore-case --word-regexp --invert-match
-			--full-name
+			--full-name --line-number
 			--extended-regexp --basic-regexp --fixed-strings
 			--files-with-matches --name-only
 			--files-without-match
-- 
1.7.3.4

[PATCH V2 3/5] grep: Put calls to fixmatch() and regmatch() into patmatch()

[Michał Kiedrowicz]

Both match_one_pattern() and look_ahead() use fixmatch() and regmatch()
in the same way. They really want to match a pattern againt a string,
but now they need to know if the pattern is fixed or regexp.

This change cleans this up by introducing patmatch() (from "pattern
match") and also simplifies inserting other ways of matching a string.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 grep.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/grep.c b/grep.c
index 63c4280..d67baf9 100644
--- a/grep.c
+++ b/grep.c
@@ -412,6 +412,19 @@ static int regmatch(const regex_t *preg, char *line, char *eol,
 	return regexec(preg, line, 1, match, eflags);
 }
 
+static int patmatch(struct grep_pat *p, char *line, char *eol,
+		    regmatch_t *match, int eflags)
+{
+	int hit;
+
+	if (p->fixed)
+		hit = !fixmatch(p, line, eol, match);
+	else
+		hit = !regmatch(&p->regexp, line, eol, match, eflags);
+
+	return hit;
+}
+
 static int strip_timestamp(char *bol, char **eol_p)
 {
 	char *eol = *eol_p;
@@ -461,10 +474,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 	}
 
  again:
-	if (p->fixed)
-		hit = !fixmatch(p, bol, eol, pmatch);
-	else
-		hit = !regmatch(&p->regexp, bol, eol, pmatch, eflags);
+	hit = patmatch(p, bol, eol, pmatch, eflags);
 
 	if (hit && p->word_regexp) {
 		if ((pmatch[0].rm_so < 0) ||
@@ -791,10 +801,7 @@ static int look_ahead(struct grep_opt *opt,
 		int hit;
 		regmatch_t m;
 
-		if (p->fixed)
-			hit = !fixmatch(p, bol, bol + *left_p, &m);
-		else
-			hit = !regmatch(&p->regexp, bol, bol + *left_p, &m, 0);
+		hit = patmatch(p, bol, bol + *left_p, &m, 0);
 		if (!hit || m.rm_so < 0 || m.rm_eo < 0)
 			continue;
 		if (earliest < 0 || m.rm_so < earliest)
-- 
1.7.3.4

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

[Michał Kiedrowicz]

This patch teaches git-grep the --perl-regexp/-P options (naming
borrowed from GNU grep) in order to allow specifying PCRE regexes on the
command line.

PCRE has a number of features which make them more handy to use than
POSIX regexes, like consistent escaping rules, extended character
classes, ungreedy matching etc.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 Documentation/git-grep.txt             |    6 +++
 Makefile                               |   16 +++++++
 builtin/grep.c                         |    2 +
 contrib/completion/git-completion.bash |    1 +
 grep.c                                 |   77 +++++++++++++++++++++++++++++++-
 grep.h                                 |    9 ++++
 6 files changed, 110 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a58378..e150c77 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]
 	   [-v | --invert-match] [-h|-H] [--full-name]
 	   [-E | --extended-regexp] [-G | --basic-regexp]
+	   [-P | --perl-regexp]
 	   [-F | --fixed-strings] [-n | --line-number]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
@@ -97,6 +98,11 @@ OPTIONS
 	Use POSIX extended/basic regexp for patterns.  Default
 	is to use basic regexp.
 
+-P::
+--perl-regexp::
+	Use Perl-compatible regexp for patterns. Requires libpcre to be
+	compiled in.
+
 -F::
 --fixed-strings::
 	Use fixed strings for patterns (don't interpret pattern
diff --git a/Makefile b/Makefile
index 3a1fe20..98841dc 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,12 @@ all::
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 # This also implies BLK_SHA1.
 #
+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use
+# Perl-compatible regexes.
+#
+# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in
+# /foo/bar/include and /foo/bar/lib directories.
+#
 # 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.
@@ -1251,6 +1257,16 @@ ifdef NO_LIBGEN_H
 	COMPAT_OBJS += compat/basename.o
 endif
 
+ifdef NO_LIBPCRE
+	BASIC_CFLAGS += -DNO_LIBPCRE
+else
+	ifdef LIBPCREDIR
+		BASIC_CFLAGS += -I$(LIBPCREDIR)/include
+		EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
+	endif
+	EXTLIBS += -lpcre
+endif
+
 ifdef NO_CURL
 	BASIC_CFLAGS += -DNO_CURL
 	REMOTE_CURL_PRIMARY =
diff --git a/builtin/grep.c b/builtin/grep.c
index 10a1f65..6831975 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -781,6 +781,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			REG_EXTENDED),
 		OPT_BOOLEAN('F', "fixed-strings", &opt.fixed,
 			"interpret patterns as fixed strings"),
+		OPT_BOOLEAN('P', "perl-regexp", &opt.pcre,
+				"use Perl-compatible regular expressions"),
 		OPT_GROUP(""),
 		OPT_BOOLEAN('n', "line-number", &opt.linenum, "show line numbers"),
 		OPT_NEGBIT('h', NULL, &opt.pathname, "don't show filenames", 1),
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4b2654d..95790a1 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1487,6 +1487,7 @@ _git_grep ()
 			--text --ignore-case --word-regexp --invert-match
 			--full-name --line-number
 			--extended-regexp --basic-regexp --fixed-strings
+			--perl-regexp
 			--files-with-matches --name-only
 			--files-without-match
 			--max-depth
diff --git a/grep.c b/grep.c
index d67baf9..561b791 100644
--- a/grep.c
+++ b/grep.c
@@ -3,6 +3,71 @@
 #include "userdiff.h"
 #include "xdiff-interface.h"
 
+#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");
+}
+
+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");
+}
+
+#else /* !NO_LIBPCRE */
+static void compile_pcre_regexp(struct grep_pat *p, struct grep_opt *opt)
+{
+	const char *error;
+	int erroffset;
+	int options = 0;
+
+	if (opt->ignore_case)
+		options |= PCRE_CASELESS;
+
+	p->pcre_regexp = pcre_compile(p->pattern, options, &error, &erroffset,
+			NULL);
+	if (!p->pcre_regexp)
+		die("'%s': %s", p->pattern, error);
+
+	p->extra = pcre_study(p->pcre_regexp, 0, &error);
+	if (!p->extra && error)
+		die("%s", error);
+}
+
+static int pcrematch(struct grep_pat *p, char *line, char *eol,
+		regmatch_t *match, int eflags)
+{
+	int ovector[30], ret, flags = 0;
+
+	if (eflags & REG_NOTBOL)
+		flags |= PCRE_NOTBOL;
+
+	ret = pcre_exec(p->pcre_regexp, p->extra, line, eol - line, 0, flags,
+			ovector, ARRAY_SIZE(ovector));
+	if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
+		die("pcre_exec failed with error code %d", ret);
+	if (ret > 0) {
+		ret = 0;
+		match->rm_so = ovector[0];
+		match->rm_eo = ovector[1];
+	}
+
+	return ret;
+}
+
+static void free_pcre_regexp(struct grep_pat *p)
+{
+	pcre_free(p->pcre_regexp);
+	pcre_free(p->extra);
+}
+#endif /* !NO_LIBPCRE */
+
 void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat)
 {
 	struct grep_pat *p = xcalloc(1, sizeof(*p));
@@ -70,6 +135,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	if (p->fixed)
 		return;
 
+	if (opt->pcre) {
+		compile_pcre_regexp(p, opt);
+		return;
+	}
+
 	err = regcomp(&p->regexp, p->pattern, opt->regflags);
 	if (err) {
 		char errbuf[1024];
@@ -320,7 +390,10 @@ void free_grep_patterns(struct grep_opt *opt)
 		case GREP_PATTERN: /* atom */
 		case GREP_PATTERN_HEAD:
 		case GREP_PATTERN_BODY:
-			regfree(&p->regexp);
+			if (p->pcre_regexp)
+				free_pcre_regexp(p);
+			else
+				regfree(&p->regexp);
 			break;
 		default:
 			break;
@@ -419,6 +492,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,
 
 	if (p->fixed)
 		hit = !fixmatch(p, line, eol, match);
+	else if (p->pcre_regexp)
+		hit = !pcrematch(p, line, eol, match, eflags);
 	else
 		hit = !regmatch(&p->regexp, line, eol, match, eflags);
 
diff --git a/grep.h b/grep.h
index 06621fe..68aa47a 100644
--- a/grep.h
+++ b/grep.h
@@ -1,6 +1,12 @@
 #ifndef GREP_H
 #define GREP_H
 #include "color.h"
+#ifndef NO_LIBPCRE
+#include <pcre.h>
+#else
+typedef int pcre;
+typedef int pcre_extra;
+#endif /* NO_LIBPCRE */
 
 enum grep_pat_token {
 	GREP_PATTERN,
@@ -33,6 +39,8 @@ struct grep_pat {
 	size_t patternlen;
 	enum grep_header_field field;
 	regex_t regexp;
+	pcre *pcre_regexp;
+	pcre_extra *extra;
 	unsigned fixed:1;
 	unsigned ignore_case:1;
 	unsigned word_regexp:1;
@@ -83,6 +91,7 @@ struct grep_opt {
 #define GREP_BINARY_TEXT	2
 	int binary;
 	int extended;
+	int pcre;
 	int relative;
 	int pathname;
 	int null_following_name;
-- 
1.7.3.4

[PATCH V2 5/5] configure: Check for libpcre

[Michał Kiedrowicz]

This adds checks for libpcre based on checking for curl.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 config.mak.in |    1 +
 configure.ac  |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index e378534..9bab021 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -30,6 +30,7 @@ export srcdir VPATH
 ASCIIDOC7=@ASCIIDOC7@
 NEEDS_SSL_WITH_CRYPTO=@NEEDS_SSL_WITH_CRYPTO@
 NO_OPENSSL=@NO_OPENSSL@
+NO_LIBPCRE=@NO_LIBPCRE@
 NO_CURL=@NO_CURL@
 NO_EXPAT=@NO_EXPAT@
 NO_LIBGEN_H=@NO_LIBGEN_H@
diff --git a/configure.ac b/configure.ac
index fafd815..4711369 100644
--- a/configure.ac
+++ b/configure.ac
@@ -220,6 +220,17 @@ AS_HELP_STRING([--with-openssl],[use OpenSSL library (default is YES)])
 AS_HELP_STRING([],              [ARG can be prefix for openssl library and headers]),\
 GIT_PARSE_WITH(openssl))
 #
+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use
+# Perl-compatible regexes.
+#
+# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in
+# /foo/bar/include and /foo/bar/lib directories.
+#
+AC_ARG_WITH(libpcre,
+AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes (default is YES)])
+AS_HELP_STRING([],           [ARG can be also prefix for libpcre library and headers]),
+GIT_PARSE_WITH(libpcre))
+#
 # Define NO_CURL if you do not have curl installed.  git-http-pull and
 # git-http-push are not built, and you cannot use http:// and https://
 # transports.
@@ -435,6 +446,21 @@ AC_SUBST(NEEDS_SSL_WITH_CRYPTO)
 AC_SUBST(NO_OPENSSL)
 
 #
+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use
+# Perl-compatible regexes.
+#
+
+GIT_STASH_FLAGS($LIBPCREDIR)
+
+AC_CHECK_LIB([pcre], [pcre_version],
+[NO_LIBPCRE=],
+[NO_LIBPCRE=YesPlease])
+
+GIT_UNSTASH_FLAGS($LIBPCREDIR)
+
+AC_SUBST(NO_LIBPCRE)
+
+#
 # 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.
-- 
1.7.3.4

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

[Junio C Hamano]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> diff --git a/Makefile b/Makefile
> index 3a1fe20..98841dc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -24,6 +24,12 @@ all::
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
>  # This also implies BLK_SHA1.
>  #
> +# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use
> +# Perl-compatible regexes.

At least at first, I'd rather want to keep this an optional feature, i.e.

+# Define WITH_LIBPCRE if you have and want to use libpcre.

> @@ -33,6 +39,8 @@ struct grep_pat {
>  	size_t patternlen;
>  	enum grep_header_field field;
>  	regex_t regexp;
> +	pcre *pcre_regexp;
> +	pcre_extra *extra;

I don't think pcre will forever stay the _only_ thing that wants to hook
an extra information to this structure.  That is why I included "pcre_" in
the field name in my earlier "how about doing it this way" suggestion.

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

[Bert Wesarg]

2011/5/5 Junio C Hamano <gitster@pobox.com>:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
>> @@ -33,6 +39,8 @@ struct grep_pat {
>>       size_t patternlen;
>>       enum grep_header_field field;
>>       regex_t regexp;
>> +     pcre *pcre_regexp;
>> +     pcre_extra *extra;
>
> I don't think pcre will forever stay the _only_ thing that wants to hook
> an extra information to this structure.  That is why I included "pcre_" in
> the field name in my earlier "how about doing it this way" suggestion.

I would also suggest to share the space between regex_t and the
(pcre*,pcre_extra*) tuple, like i did in my patch titled 'prepare for
re-using the space...' from May 2. Sacrificing one bit to indicate
that this is a pcre compiled pattern should not hurt, because there
are bits left.

Bert

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

[Johannes Sixt]

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.

> +#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");

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.

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

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

[Johannes Sixt]

Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:
> +# Define NO_LIBPCRE if you do not have libpcre installed.

This makes libpcre a requirement with an opt-out. But I can imagine that
there are many platforms where libpcre is not installed. We would have to
add NO_LIBPRCE=1 to many platform configuration sections. But how can this
setting be countermanded if someone does install libpcre on such a platform?

Wouldn't it be better to make this an opt-in (USE_LIBPCRE)?

-- Hannes

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

[Michal Kiedrowicz]

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

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

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

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

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

[Michal Kiedrowicz]

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

> Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:
> > +# Define NO_LIBPCRE if you do not have libpcre installed.
> 
> This makes libpcre a requirement with an opt-out. But I can imagine
> that there are many platforms where libpcre is not installed. We
> would have to add NO_LIBPRCE=1 to many platform configuration
> sections. But how can this setting be countermanded if someone does
> install libpcre on such a platform?
> 
> Wouldn't it be better to make this an opt-in (USE_LIBPCRE)?
> 
> -- Hannes

Yes, Junio already wrote that too. I'll change that.

Thanks!

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

[Alex Riesen]

2011/5/5 Michał Kiedrowicz <michal.kiedrowicz@gmail.com>:
> +#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");
> +}

Looks like these two functions below can be just left empty, because you will
exit when calling compile_pcre_regexp in compile_regexp.

> +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");
> +}
> +

These will be never called, because...

> @@ -70,6 +135,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>        if (p->fixed)
>                return;
>
> +       if (opt->pcre) {
> +               compile_pcre_regexp(p, opt);
> +               return;
> +       }

... you die here, if PCRE not available.

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

[Johannes Sixt]

Am 5/5/2011 9:41, schrieb Michal Kiedrowicz:
> But I can abstract these calls to die_pcre_not_supported() to not
> repeat die() message.

Gah! Don't over-engineer. The compiler will un-duplicate the message texts
for you if you carefully copy-and-paste them.

-- Hannes

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

[Michal Kiedrowicz]

On 05.05.2011 09:49:14 +0200 Johannes Sixt <j.sixt@viscovery.net> wrote:

> Am 5/5/2011 9:41, schrieb Michal Kiedrowicz:
> > But I can abstract these calls to die_pcre_not_supported() to not
> > repeat die() message.
> 
> Gah! Don't over-engineer. The compiler will un-duplicate the message
> texts for you if you carefully copy-and-paste them.
> 
> -- Hannes

The whole point of using die_pcre_not_supported() is that it'll take
care of this "if you carefully copy-and-paste them" and remove
redundancy in _source code_, not binary. But still these die()'s are
minor nit (IMO).

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> I would also suggest to share the space between regex_t and the
> (pcre*,pcre_extra*) tuple, like i did in my patch titled 'prepare for
> re-using the space...' from May 2. Sacrificing one bit to indicate
> that this is a pcre compiled pattern should not hurt, because there
> are bits left.

It might be an excessive over-engineering, though.

Unlike "struct object" that need to stay in-core and grow proportionally
to the size of the history being traversed, grep_pat corresponds to one
item from the pattern specified on the command line. If it makes the code
harder to read because we end up constantly dereferencing union members
(and no, "#define pcre_regexp u.pcre_regexp" is not a solution), the
resulting 8-to-16 bytes saved per pattern may not be worth it.