{"thread":{"id":"27253","subject":"[PATCH V2 1/5] Documentation: Add --line-number to git-grep synopsis","startedAt":"2011-05-04T22:00:16Z","lastAt":"2011-05-05T16:55:14Z","messageCount":16,"participants":["Michał Kiedrowicz","Junio C Hamano","Bert Wesarg","Johannes Sixt","Michal Kiedrowicz","Alex Riesen"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"167055","messageId":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":null,"subject":"[PATCH V2 0/5] Add PCRE support to git-grep","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:16Z","receivedAt":"2011-05-04T22:00:16Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"This patch series attempts to add PCRE support to git-grep.\n\nChanges from previous version:\n\n* Now this series start from two patches which add the --line-number\n  option to SYNOPSIS and bash completion; they can be treated\n  separatedly from this series\n\n* All patches have improved descriptions\n\n* All patches have Signed-off-by\n\n* libpcre specific code has been moved to a separate subsection to\n  reduce number of #ifdefs; this addresses comments from Junio\n\n* I noticed that even though I call pcre_study(), I didn't use its\n  result in pcre_exec(); now this is fixed; this should improve\n  git-grep --perl-regexp performance\n\n* --perl-regexp has been added to bash-completion\n\n* Added last patch which adds checks for libpcre to configure\n\n* I repeated `make test` with and without pcre few more times and I\n  cannot reproduce results from previous runs; possibly they were\n  altered by current system load or cold cache. Now the difference\n  is just few seconds. However, I'm still not convinced to link\n  libgit.a with libpcre.\n\nMichał Kiedrowicz (5):\n  Documentation: Add --line-number to git-grep synopsis\n  contrib/completion: --line-number to git grep\n  grep: Put calls to fixmatch() and regmatch() into patmatch()\n  git-grep: Learn PCRE\n  configure: Check for libpcre\n\n Documentation/git-grep.txt             |    8 ++-\n Makefile                               |   16 +++++\n builtin/grep.c                         |    2 +\n config.mak.in                          |    1 +\n configure.ac                           |   26 ++++++++\n contrib/completion/git-completion.bash |    3 +-\n grep.c                                 |  100 +++++++++++++++++++++++++++++---\n grep.h                                 |    9 +++\n 8 files changed, 154 insertions(+), 11 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"167049","messageId":"1304546421-25439-2-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH V2 1/5] Documentation: Add --line-number to git-grep synopsis","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:17Z","receivedAt":"2011-05-04T22:00:17Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Commit 7d6cb10b (\"grep: Add the option '--line-number'\", 2011-03-28)\nintroduced the --line-number option and added its description to OPTIONS\nsection, but forgot to update SYNOPSIS.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n Documentation/git-grep.txt |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex d7523b3..4a58378 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]\n \t   [-v | --invert-match] [-h|-H] [--full-name]\n \t   [-E | --extended-regexp] [-G | --basic-regexp]\n-\t   [-F | --fixed-strings] [-n]\n+\t   [-F | --fixed-strings] [-n | --line-number]\n \t   [-l | --files-with-matches] [-L | --files-without-match]\n \t   [(-O | --open-files-in-pager) [<pager>]]\n \t   [-z | --null]\n-- \n1.7.3.4\n"},{"id":"167050","messageId":"1304546421-25439-3-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH V2 2/5] contrib/completion: --line-number to git grep","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:18Z","receivedAt":"2011-05-04T22:00:18Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Commit 7d6cb10b (\"grep: Add the option '--line-number'\", 2011-03-28)\nintroduced the --line-number option, but did not add it to\nbash-completion.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n contrib/completion/git-completion.bash |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 0df356d..4b2654d 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1485,7 +1485,7 @@ _git_grep ()\n \t\t__gitcomp \"\n \t\t\t--cached\n \t\t\t--text --ignore-case --word-regexp --invert-match\n-\t\t\t--full-name\n+\t\t\t--full-name --line-number\n \t\t\t--extended-regexp --basic-regexp --fixed-strings\n \t\t\t--files-with-matches --name-only\n \t\t\t--files-without-match\n-- \n1.7.3.4\n"},{"id":"167051","messageId":"1304546421-25439-4-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH V2 3/5] grep: Put calls to fixmatch() and regmatch() into patmatch()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:19Z","receivedAt":"2011-05-04T22:00:19Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Both match_one_pattern() and look_ahead() use fixmatch() and regmatch()\nin the same way. They really want to match a pattern againt a string,\nbut now they need to know if the pattern is fixed or regexp.\n\nThis change cleans this up by introducing patmatch() (from \"pattern\nmatch\") and also simplifies inserting other ways of matching a string.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n grep.c |   23 +++++++++++++++--------\n 1 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 63c4280..d67baf9 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -412,6 +412,19 @@ static int regmatch(const regex_t *preg, char *line, char *eol,\n \treturn regexec(preg, line, 1, match, eflags);\n }\n \n+static int patmatch(struct grep_pat *p, char *line, char *eol,\n+\t\t    regmatch_t *match, int eflags)\n+{\n+\tint hit;\n+\n+\tif (p->fixed)\n+\t\thit = !fixmatch(p, line, eol, match);\n+\telse\n+\t\thit = !regmatch(&p->regexp, line, eol, match, eflags);\n+\n+\treturn hit;\n+}\n+\n static int strip_timestamp(char *bol, char **eol_p)\n {\n \tchar *eol = *eol_p;\n@@ -461,10 +474,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t}\n \n  again:\n-\tif (p->fixed)\n-\t\thit = !fixmatch(p, bol, eol, pmatch);\n-\telse\n-\t\thit = !regmatch(&p->regexp, bol, eol, pmatch, eflags);\n+\thit = patmatch(p, bol, eol, pmatch, eflags);\n \n \tif (hit && p->word_regexp) {\n \t\tif ((pmatch[0].rm_so < 0) ||\n@@ -791,10 +801,7 @@ static int look_ahead(struct grep_opt *opt,\n \t\tint hit;\n \t\tregmatch_t m;\n \n-\t\tif (p->fixed)\n-\t\t\thit = !fixmatch(p, bol, bol + *left_p, &m);\n-\t\telse\n-\t\t\thit = !regmatch(&p->regexp, bol, bol + *left_p, &m, 0);\n+\t\thit = patmatch(p, bol, bol + *left_p, &m, 0);\n \t\tif (!hit || m.rm_so < 0 || m.rm_eo < 0)\n \t\t\tcontinue;\n \t\tif (earliest < 0 || m.rm_so < earliest)\n-- \n1.7.3.4\n"},{"id":"167052","messageId":"1304546421-25439-5-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:20Z","receivedAt":"2011-05-04T22:00:20Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"This patch teaches git-grep the --perl-regexp/-P options (naming\nborrowed from GNU grep) in order to allow specifying PCRE regexes on the\ncommand line.\n\nPCRE has a number of features which make them more handy to use than\nPOSIX regexes, like consistent escaping rules, extended character\nclasses, ungreedy matching etc.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n Documentation/git-grep.txt             |    6 +++\n Makefile                               |   16 +++++++\n builtin/grep.c                         |    2 +\n contrib/completion/git-completion.bash |    1 +\n grep.c                                 |   77 +++++++++++++++++++++++++++++++-\n grep.h                                 |    9 ++++\n 6 files changed, 110 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 4a58378..e150c77 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -12,6 +12,7 @@ SYNOPSIS\n 'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]\n \t   [-v | --invert-match] [-h|-H] [--full-name]\n \t   [-E | --extended-regexp] [-G | --basic-regexp]\n+\t   [-P | --perl-regexp]\n \t   [-F | --fixed-strings] [-n | --line-number]\n \t   [-l | --files-with-matches] [-L | --files-without-match]\n \t   [(-O | --open-files-in-pager) [<pager>]]\n@@ -97,6 +98,11 @@ OPTIONS\n \tUse POSIX extended/basic regexp for patterns.  Default\n \tis to use basic regexp.\n \n+-P::\n+--perl-regexp::\n+\tUse Perl-compatible regexp for patterns. Requires libpcre to be\n+\tcompiled in.\n+\n -F::\n --fixed-strings::\n \tUse fixed strings for patterns (don't interpret pattern\ndiff --git a/Makefile b/Makefile\nindex 3a1fe20..98841dc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -24,6 +24,12 @@ all::\n # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n # This also implies BLK_SHA1.\n #\n+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use\n+# Perl-compatible regexes.\n+#\n+# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in\n+# /foo/bar/include and /foo/bar/lib directories.\n+#\n # Define NO_CURL if you do not have libcurl installed.  git-http-pull and\n # git-http-push are not built, and you cannot use http:// and https://\n # transports.\n@@ -1251,6 +1257,16 @@ ifdef NO_LIBGEN_H\n \tCOMPAT_OBJS += compat/basename.o\n endif\n \n+ifdef NO_LIBPCRE\n+\tBASIC_CFLAGS += -DNO_LIBPCRE\n+else\n+\tifdef LIBPCREDIR\n+\t\tBASIC_CFLAGS += -I$(LIBPCREDIR)/include\n+\t\tEXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)\n+\tendif\n+\tEXTLIBS += -lpcre\n+endif\n+\n ifdef NO_CURL\n \tBASIC_CFLAGS += -DNO_CURL\n \tREMOTE_CURL_PRIMARY =\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 10a1f65..6831975 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -781,6 +781,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tREG_EXTENDED),\n \t\tOPT_BOOLEAN('F', \"fixed-strings\", &opt.fixed,\n \t\t\t\"interpret patterns as fixed strings\"),\n+\t\tOPT_BOOLEAN('P', \"perl-regexp\", &opt.pcre,\n+\t\t\t\t\"use Perl-compatible regular expressions\"),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOLEAN('n', \"line-number\", &opt.linenum, \"show line numbers\"),\n \t\tOPT_NEGBIT('h', NULL, &opt.pathname, \"don't show filenames\", 1),\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 4b2654d..95790a1 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1487,6 +1487,7 @@ _git_grep ()\n \t\t\t--text --ignore-case --word-regexp --invert-match\n \t\t\t--full-name --line-number\n \t\t\t--extended-regexp --basic-regexp --fixed-strings\n+\t\t\t--perl-regexp\n \t\t\t--files-with-matches --name-only\n \t\t\t--files-without-match\n \t\t\t--max-depth\ndiff --git a/grep.c b/grep.c\nindex d67baf9..561b791 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -3,6 +3,71 @@\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n \n+#ifdef NO_LIBPCRE\n+static void compile_pcre_regexp(struct grep_pat *p, struct grep_opt *opt)\n+{\n+\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n+}\n+\n+static int pcrematch(struct grep_pat *p, char *line, char *eol,\n+\t\tregmatch_t *match, int eflags)\n+{\n+\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n+}\n+\n+static void free_pcre_regexp(struct grep_pat *p)\n+{\n+\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n+}\n+\n+#else /* !NO_LIBPCRE */\n+static void compile_pcre_regexp(struct grep_pat *p, struct grep_opt *opt)\n+{\n+\tconst char *error;\n+\tint erroffset;\n+\tint options = 0;\n+\n+\tif (opt->ignore_case)\n+\t\toptions |= PCRE_CASELESS;\n+\n+\tp->pcre_regexp = pcre_compile(p->pattern, options, &error, &erroffset,\n+\t\t\tNULL);\n+\tif (!p->pcre_regexp)\n+\t\tdie(\"'%s': %s\", p->pattern, error);\n+\n+\tp->extra = pcre_study(p->pcre_regexp, 0, &error);\n+\tif (!p->extra && error)\n+\t\tdie(\"%s\", error);\n+}\n+\n+static int pcrematch(struct grep_pat *p, char *line, char *eol,\n+\t\tregmatch_t *match, int eflags)\n+{\n+\tint ovector[30], ret, flags = 0;\n+\n+\tif (eflags & REG_NOTBOL)\n+\t\tflags |= PCRE_NOTBOL;\n+\n+\tret = pcre_exec(p->pcre_regexp, p->extra, line, eol - line, 0, flags,\n+\t\t\tovector, ARRAY_SIZE(ovector));\n+\tif (ret < 0 && ret != PCRE_ERROR_NOMATCH)\n+\t\tdie(\"pcre_exec failed with error code %d\", ret);\n+\tif (ret > 0) {\n+\t\tret = 0;\n+\t\tmatch->rm_so = ovector[0];\n+\t\tmatch->rm_eo = ovector[1];\n+\t}\n+\n+\treturn ret;\n+}\n+\n+static void free_pcre_regexp(struct grep_pat *p)\n+{\n+\tpcre_free(p->pcre_regexp);\n+\tpcre_free(p->extra);\n+}\n+#endif /* !NO_LIBPCRE */\n+\n void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat)\n {\n \tstruct grep_pat *p = xcalloc(1, sizeof(*p));\n@@ -70,6 +135,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tif (p->fixed)\n \t\treturn;\n \n+\tif (opt->pcre) {\n+\t\tcompile_pcre_regexp(p, opt);\n+\t\treturn;\n+\t}\n+\n \terr = regcomp(&p->regexp, p->pattern, opt->regflags);\n \tif (err) {\n \t\tchar errbuf[1024];\n@@ -320,7 +390,10 @@ void free_grep_patterns(struct grep_opt *opt)\n \t\tcase GREP_PATTERN: /* atom */\n \t\tcase GREP_PATTERN_HEAD:\n \t\tcase GREP_PATTERN_BODY:\n-\t\t\tregfree(&p->regexp);\n+\t\t\tif (p->pcre_regexp)\n+\t\t\t\tfree_pcre_regexp(p);\n+\t\t\telse\n+\t\t\t\tregfree(&p->regexp);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tbreak;\n@@ -419,6 +492,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \n \tif (p->fixed)\n \t\thit = !fixmatch(p, line, eol, match);\n+\telse if (p->pcre_regexp)\n+\t\thit = !pcrematch(p, line, eol, match, eflags);\n \telse\n \t\thit = !regmatch(&p->regexp, line, eol, match, eflags);\n \ndiff --git a/grep.h b/grep.h\nindex 06621fe..68aa47a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -1,6 +1,12 @@\n #ifndef GREP_H\n #define GREP_H\n #include \"color.h\"\n+#ifndef NO_LIBPCRE\n+#include <pcre.h>\n+#else\n+typedef int pcre;\n+typedef int pcre_extra;\n+#endif /* NO_LIBPCRE */\n \n enum grep_pat_token {\n \tGREP_PATTERN,\n@@ -33,6 +39,8 @@ struct grep_pat {\n \tsize_t patternlen;\n \tenum grep_header_field field;\n \tregex_t regexp;\n+\tpcre *pcre_regexp;\n+\tpcre_extra *extra;\n \tunsigned fixed:1;\n \tunsigned ignore_case:1;\n \tunsigned word_regexp:1;\n@@ -83,6 +91,7 @@ struct grep_opt {\n #define GREP_BINARY_TEXT\t2\n \tint binary;\n \tint extended;\n+\tint pcre;\n \tint relative;\n \tint pathname;\n \tint null_following_name;\n-- \n1.7.3.4\n"},{"id":"167053","messageId":"1304546421-25439-6-git-send-email-michal.kiedrowicz@gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH V2 5/5] configure: Check for libpcre","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-04T22:00:21Z","receivedAt":"2011-05-04T22:00:21Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"This adds checks for libpcre based on checking for curl.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n config.mak.in |    1 +\n configure.ac  |   26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 0 deletions(-)\n\ndiff --git a/config.mak.in b/config.mak.in\nindex e378534..9bab021 100644\n--- a/config.mak.in\n+++ b/config.mak.in\n@@ -30,6 +30,7 @@ export srcdir VPATH\n ASCIIDOC7=@ASCIIDOC7@\n NEEDS_SSL_WITH_CRYPTO=@NEEDS_SSL_WITH_CRYPTO@\n NO_OPENSSL=@NO_OPENSSL@\n+NO_LIBPCRE=@NO_LIBPCRE@\n NO_CURL=@NO_CURL@\n NO_EXPAT=@NO_EXPAT@\n NO_LIBGEN_H=@NO_LIBGEN_H@\ndiff --git a/configure.ac b/configure.ac\nindex fafd815..4711369 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -220,6 +220,17 @@ AS_HELP_STRING([--with-openssl],[use OpenSSL library (default is YES)])\n AS_HELP_STRING([],              [ARG can be prefix for openssl library and headers]),\\\n GIT_PARSE_WITH(openssl))\n #\n+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use\n+# Perl-compatible regexes.\n+#\n+# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in\n+# /foo/bar/include and /foo/bar/lib directories.\n+#\n+AC_ARG_WITH(libpcre,\n+AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes (default is YES)])\n+AS_HELP_STRING([],           [ARG can be also prefix for libpcre library and headers]),\n+GIT_PARSE_WITH(libpcre))\n+#\n # Define NO_CURL if you do not have curl installed.  git-http-pull and\n # git-http-push are not built, and you cannot use http:// and https://\n # transports.\n@@ -435,6 +446,21 @@ AC_SUBST(NEEDS_SSL_WITH_CRYPTO)\n AC_SUBST(NO_OPENSSL)\n \n #\n+# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use\n+# Perl-compatible regexes.\n+#\n+\n+GIT_STASH_FLAGS($LIBPCREDIR)\n+\n+AC_CHECK_LIB([pcre], [pcre_version],\n+[NO_LIBPCRE=],\n+[NO_LIBPCRE=YesPlease])\n+\n+GIT_UNSTASH_FLAGS($LIBPCREDIR)\n+\n+AC_SUBST(NO_LIBPCRE)\n+\n+#\n # Define NO_CURL if you do not have libcurl installed.  git-http-pull and\n # git-http-push are not built, and you cannot use http:// and https://\n # transports.\n-- \n1.7.3.4\n"},{"id":"167059","messageId":"7vk4e6c5rh.fsf@alter.siamese.dyndns.org","threadId":"27253","inReplyTo":"1304546421-25439-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-05T01:09:38Z","receivedAt":"2011-05-05T01:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> diff --git a/Makefile b/Makefile\n> index 3a1fe20..98841dc 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -24,6 +24,12 @@ all::\n>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n>  # This also implies BLK_SHA1.\n>  #\n> +# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use\n> +# Perl-compatible regexes.\n\nAt least at first, I'd rather want to keep this an optional feature, i.e.\n\n+# Define WITH_LIBPCRE if you have and want to use libpcre.\n\n> @@ -33,6 +39,8 @@ struct grep_pat {\n>  \tsize_t patternlen;\n>  \tenum grep_header_field field;\n>  \tregex_t regexp;\n> +\tpcre *pcre_regexp;\n> +\tpcre_extra *extra;\n\nI don't think pcre will forever stay the _only_ thing that wants to hook\nan extra information to this structure.  That is why I included \"pcre_\" in\nthe field name in my earlier \"how about doing it this way\" suggestion.\n"},{"id":"167075","messageId":"BANLkTik6pcz696xJyoOju8a3ToResLF+3w@mail.gmail.com","threadId":"27253","inReplyTo":"7vk4e6c5rh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2011-05-05T05:47:43Z","receivedAt":"2011-05-05T05:47:43Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"2011/5/5 Junio C Hamano <gitster@pobox.com>:\n> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n>\n>> @@ -33,6 +39,8 @@ struct grep_pat {\n>>       size_t patternlen;\n>>       enum grep_header_field field;\n>>       regex_t regexp;\n>> +     pcre *pcre_regexp;\n>> +     pcre_extra *extra;\n>\n> I don't think pcre will forever stay the _only_ thing that wants to hook\n> an extra information to this structure.  That is why I included \"pcre_\" in\n> the field name in my earlier \"how about doing it this way\" suggestion.\n\nI would also suggest to share the space between regex_t and the\n(pcre*,pcre_extra*) tuple, like i did in my patch titled 'prepare for\nre-using the space...' from May 2. Sacrificing one bit to indicate\nthat this is a pcre compiled pattern should not hurt, because there\nare bits left.\n\nBert\n"},{"id":"167079","messageId":"4DC2418E.4070006@viscovery.net","threadId":"27253","inReplyTo":"1304546421-25439-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-05T06:19:58Z","receivedAt":"2011-05-05T06:19:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:\n> +# Define NO_LIBPCRE if you do not have libpcre installed.  git-grep cannot use\n> +# Perl-compatible regexes.\n\nFor what purpose are you adding Perl-regex when git-grep cannot use them?\n\n...\n\nOh! You mean to say \"..., but git-grep cannot use Perl-compatible regexes\n_in this case_\".\n\n;)\n\nThis repeats in patch 5/5.\n\n> +#ifdef NO_LIBPCRE\n> +static void compile_pcre_regexp(struct grep_pat *p, struct grep_opt *opt)\n> +{\n> +\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n\nThis is such a terminus technicus. Wouldn't it be much easier to read for\nJoe User if this were merely:\n\n\tdie(\"Perl-compatible regexes not supported\");\n\nAlso, wouldn't it be nicer to die already when the --perl-regexp option is\ndetected? Then you could make these functions dummies that behave as if\nnothing was matched.\n\n> +}\n> +\n> +static int pcrematch(struct grep_pat *p, char *line, char *eol,\n> +\t\tregmatch_t *match, int eflags)\n> +{\n> +\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n> +}\n> +\n> +static void free_pcre_regexp(struct grep_pat *p)\n> +{\n> +\tdie(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n> +}\n\n-- Hannes\n"},{"id":"167080","messageId":"4DC24371.7060906@viscovery.net","threadId":"27253","inReplyTo":"1304546421-25439-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-05T06:28:01Z","receivedAt":"2011-05-05T06:28:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:\n> +# Define NO_LIBPCRE if you do not have libpcre installed.\n\nThis makes libpcre a requirement with an opt-out. But I can imagine that\nthere are many platforms where libpcre is not installed. We would have to\nadd NO_LIBPRCE=1 to many platform configuration sections. But how can this\nsetting be countermanded if someone does install libpcre on such a platform?\n\nWouldn't it be better to make this an opt-in (USE_LIBPCRE)?\n\n-- Hannes\n"},{"id":"167087","messageId":"20110505094147.22e3d158@mkiedrowicz","threadId":"27253","inReplyTo":"4DC2418E.4070006@viscovery.net","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-05T07:41:47Z","receivedAt":"2011-05-05T07:41:47Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"On 05.05.2011 08:19:58 +0200 Johannes Sixt <j.sixt@viscovery.net> wrote:\n\n> Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:\n> > +# Define NO_LIBPCRE if you do not have libpcre installed.\n> > git-grep cannot use +# Perl-compatible regexes.\n> \n> For what purpose are you adding Perl-regex when git-grep cannot use\n> them?\n> \n> ...\n> \n> Oh! You mean to say \"..., but git-grep cannot use Perl-compatible\n> regexes _in this case_\".\n> \n> ;)\n> \n> This repeats in patch 5/5.\n\nI took this from NO_CURL and NO_EXPAT descriptions:\n\n\t Define NO_CURL if you do not have libcurl installed.\n\t git-http-pull and git-http-push are not built, and you\n\t cannot use http:// and https:// transports.\n\n\tDefine NO_EXPAT if you do not have expat installed.\n\tgit-http-push is not built, and you cannot push using\n\thttp:// and https:// transports.\n\nBut I can reword it :).\n\n> \n> > +#ifdef NO_LIBPCRE\n> > +static void compile_pcre_regexp(struct grep_pat *p, struct\n> > grep_opt *opt) +{\n> > +\tdie(\"cannot use Perl-compatible regexes when libpcre is\n> > not compiled in\");\n> \n> This is such a terminus technicus. Wouldn't it be much easier to read\n> for Joe User if this were merely:\n> \n> \tdie(\"Perl-compatible regexes not supported\");\n\nI can argue. My message says why they aren't available, while your\nproposition leaves Joe without a clue :). But I'm not that much attached\nto it, I can change it.\n\n> \n> Also, wouldn't it be nicer to die already when the --perl-regexp\n> option is detected? Then you could make these functions dummies that\n> behave as if nothing was matched.\n> \n\nI prefer to die() at lowest possible level in case someone wants to use\nthese functions in another way (e.g. from revision-walking code).\n\nBut I can abstract these calls to die_pcre_not_supported() to not\nrepeat die() message.\n\n> > +}\n> > +\n> > +static int pcrematch(struct grep_pat *p, char *line, char *eol,\n> > +\t\tregmatch_t *match, int eflags)\n> > +{\n> > +\tdie(\"cannot use Perl-compatible regexes when libpcre is\n> > not compiled in\"); +}\n> > +\n> > +static void free_pcre_regexp(struct grep_pat *p)\n> > +{\n> > +\tdie(\"cannot use Perl-compatible regexes when libpcre is\n> > not compiled in\"); +}\n> \n> -- Hannes\n\nThanks for comments :)\n"},{"id":"167088","messageId":"20110505094320.5d64234f@mkiedrowicz","threadId":"27253","inReplyTo":"4DC24371.7060906@viscovery.net","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-05T07:43:20Z","receivedAt":"2011-05-05T07:43:20Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"On 05.05.2011 08:28:01 +0200 Johannes Sixt <j.sixt@viscovery.net> wrote:\n\n> Am 5/5/2011 0:00, schrieb Michał Kiedrowicz:\n> > +# Define NO_LIBPCRE if you do not have libpcre installed.\n> \n> This makes libpcre a requirement with an opt-out. But I can imagine\n> that there are many platforms where libpcre is not installed. We\n> would have to add NO_LIBPRCE=1 to many platform configuration\n> sections. But how can this setting be countermanded if someone does\n> install libpcre on such a platform?\n> \n> Wouldn't it be better to make this an opt-in (USE_LIBPCRE)?\n> \n> -- Hannes\n\nYes, Junio already wrote that too. I'll change that.\n\nThanks!\n"},{"id":"167090","messageId":"BANLkTinXoDFyb9U0RJVb8eH269zj+XvBdQ@mail.gmail.com","threadId":"27253","inReplyTo":"1304546421-25439-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2011-05-05T07:43:50Z","receivedAt":"2011-05-05T07:43:50Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2011/5/5 Michał Kiedrowicz <michal.kiedrowicz@gmail.com>:\n> +#ifdef NO_LIBPCRE\n> +static void compile_pcre_regexp(struct grep_pat *p, struct grep_opt *opt)\n> +{\n> +       die(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n> +}\n\nLooks like these two functions below can be just left empty, because you will\nexit when calling compile_pcre_regexp in compile_regexp.\n\n> +static int pcrematch(struct grep_pat *p, char *line, char *eol,\n> +               regmatch_t *match, int eflags)\n> +{\n> +       die(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n> +}\n> +\n> +static void free_pcre_regexp(struct grep_pat *p)\n> +{\n> +       die(\"cannot use Perl-compatible regexes when libpcre is not compiled in\");\n> +}\n> +\n\nThese will be never called, because...\n\n> @@ -70,6 +135,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n>        if (p->fixed)\n>                return;\n>\n> +       if (opt->pcre) {\n> +               compile_pcre_regexp(p, opt);\n> +               return;\n> +       }\n\n... you die here, if PCRE not available.\n"},{"id":"167091","messageId":"4DC2567A.1060502@viscovery.net","threadId":"27253","inReplyTo":"20110505094147.22e3d158@mkiedrowicz","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-05T07:49:14Z","receivedAt":"2011-05-05T07:49:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/5/2011 9:41, schrieb Michal Kiedrowicz:\n> But I can abstract these calls to die_pcre_not_supported() to not\n> repeat die() message.\n\nGah! Don't over-engineer. The compiler will un-duplicate the message texts\nfor you if you carefully copy-and-paste them.\n\n-- Hannes\n"},{"id":"167093","messageId":"20110505103839.2a051583@mkiedrowicz","threadId":"27253","inReplyTo":"4DC2567A.1060502@viscovery.net","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2011-05-05T08:38:39Z","receivedAt":"2011-05-05T08:38:39Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"On 05.05.2011 09:49:14 +0200 Johannes Sixt <j.sixt@viscovery.net> wrote:\n\n> Am 5/5/2011 9:41, schrieb Michal Kiedrowicz:\n> > But I can abstract these calls to die_pcre_not_supported() to not\n> > repeat die() message.\n> \n> Gah! Don't over-engineer. The compiler will un-duplicate the message\n> texts for you if you carefully copy-and-paste them.\n> \n> -- Hannes\n\nThe whole point of using die_pcre_not_supported() is that it'll take\ncare of this \"if you carefully copy-and-paste them\" and remove\nredundancy in _source code_, not binary. But still these die()'s are\nminor nit (IMO).\n"},{"id":"167124","messageId":"7vei4d9jf1.fsf@alter.siamese.dyndns.org","threadId":"27253","inReplyTo":"BANLkTik6pcz696xJyoOju8a3ToResLF+3w@mail.gmail.com","subject":"Re: [PATCH V2 4/5] git-grep: Learn PCRE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-05T16:55:14Z","receivedAt":"2011-05-05T16:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg <bert.wesarg@googlemail.com> writes:\n\n> I would also suggest to share the space between regex_t and the\n> (pcre*,pcre_extra*) tuple, like i did in my patch titled 'prepare for\n> re-using the space...' from May 2. Sacrificing one bit to indicate\n> that this is a pcre compiled pattern should not hurt, because there\n> are bits left.\n\nIt might be an excessive over-engineering, though.\n\nUnlike \"struct object\" that need to stay in-core and grow proportionally\nto the size of the history being traversed, grep_pat corresponds to one\nitem from the pattern specified on the command line. If it makes the code\nharder to read because we end up constantly dereferencing union members\n(and no, \"#define pcre_regexp u.pcre_regexp\" is not a solution), the\nresulting 8-to-16 bytes saved per pattern may not be worth it.\n"}]}