{"thread":{"id":"45636","subject":"[PATCH 00/12] PCREv2 & more","startedAt":"2017-04-08T13:25:48Z","lastAt":"2017-04-16T22:26:09Z","messageCount":43,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"316398","messageId":"20170408132506.5415-1-avarab@gmail.com","threadId":"45636","inReplyTo":null,"subject":"[PATCH 00/12] PCREv2 & more","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:54Z","receivedAt":"2017-04-08T13:25:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This adds PCRE v2 support, but as I was adding that I kept noticing\nother related problems to fix. It's all bundled up into the same\nseries because much of it conflicts because it modifies the same test\nor other code. Notes on each patch below.\n\nÆvar Arnfjörð Bjarmason (12):\n grep: add ability to disable threading with --threads=0 or\n grep.threads=0\n\nThis really has nothing to do with the rest except I'm using it to\ntest non-multithreaded & threaded PCRE more easily.\n\n grep: remove redundant regflags assignment under PCRE\n Makefile & configure: reword outdated comment about PCRE\n\nJust some trivial cleanups.\n\n grep: add a test for backreferences in PCRE patterns\n log: add exhaustive tests for pattern style options & config\n\nYay, more tests!\n\n log: add -P as a synonym for --perl-regexp\n\nWe've had --perl-regexp for years, but not -P like grep, add it.\n\n grep & rev-list doc: stop promising libpcre for --perl-regexp\n grep: make grep.patternType=[pcre|pcre1] a synonym for \"perl\"\n test-lib: rename the LIBPCRE prerequisite to PCRE\n grep: change the internal PCRE macro names to be PCRE1\n grep: change the internal PCRE code & header names to be PCRE1\n grep: add support for PCRE v2\n\nThese combined add the support for PCRE 2. It's split up for ease of\nreadability. The last one's still a bit big, and I could e.g. split up\nall the Makefile/autoconf stuff into a different patch (which wouldn't\ndo anything without the code), but I thought on balance doing it this\nway made the most sense.\n\n Documentation/config.txt | 7 ++\n Documentation/git-grep.txt | 8 +-\n Documentation/rev-list-options.txt | 6 +-\n Makefile | 28 +++++-\n builtin/grep.c | 26 +++++-\n configure.ac | 61 ++++++++++--\n grep.c | 184 ++++++++++++++++++++++++++++++-------\n grep.h | 26 ++++--\n revision.c | 2 +-\n t/README | 16 +++-\n t/t4202-log.sh | 76 ++++++++++++++-\n t/t7810-grep.sh | 79 +++++++++++++---\n t/t7812-grep-icase-non-ascii.sh | 4 +-\n t/t7813-grep-icase-iso.sh | 11 ++-\n t/test-lib.sh | 4 +-\n 15 files changed, 456 insertions(+), 82 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"316399","messageId":"20170408132506.5415-2-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:55Z","receivedAt":"2017-04-08T13:25:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add the ability to entirely disable threading by having grep.threads=0\nin the config or --threads=0 on the command-line.\n\nThis was made configurable in commit 89f09dd34e (\"grep: add\n--threads= option and grep.threads configuration\",\n2015-12-15). Before that change there was no way to disable threaded\ngrep other than to recompile Git.\n\nIt's very useful for testing & debugging to be able to entirely\ndisable threading without recompiling with NO_PTHREADS=YesPlease, so\nsupport setting the value to 0 to disable threading.\n\nThere was no reason this wasn't the case already other than an\nimplementation detail in how OPT_INTEGER() works. When it's used\nthere's no way to tell the difference between an unset value & the\ndefault value. Use OPT_CALLBACK() instead using the same pattern as in\ncommit b16a991c1b (\"cherry-pick: detect bogus arguments to\n--mainline\", 2017-03-15).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-grep.txt | 4 ++--\n builtin/grep.c | 26 ++++++++++++++++++++++----\n t/t7810-grep.sh | 10 ++++++++++\n 3 files changed, 34 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 71f32f3508..7b52e3fbc4 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -56,8 +56,8 @@ grep.extendedRegexp::\n \tother than 'default'.\n \n grep.threads::\n-\tNumber of grep worker threads to use. If unset (or set to 0),\n-\t8 threads are used by default (for now).\n+\tNumber of grep worker threads to use. If unset, 8 threads are\n+\tused by default (for now). Set to 0 to disable threading.\n \n grep.fullName::\n \tIf set to true, enable `--full-name` option by default.\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 65070c52fc..9478ab5dff 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -35,7 +35,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \t\t\t\t const struct grep_source *gs);\n \n #define GREP_NUM_THREADS_DEFAULT 8\n-static int num_threads;\n+static int num_threads = -1;\n \n #ifndef NO_PTHREADS\n static pthread_t *threads;\n@@ -897,6 +897,24 @@ static int context_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int thread_callback(const struct option *opt,\n+\t\t\t const char *arg, int unset)\n+{\n+\tint *threads = (int*)opt->value;\n+\tchar *end;\n+\n+\tif (unset) {\n+\t\t*threads = GREP_NUM_THREADS_DEFAULT;\n+\t\treturn 0;\n+\t}\n+\n+\t*threads = strtol(arg, &end, 10);\n+\tif (*end || *threads < 0)\n+\t\treturn opterror(opt, \"invalid number of threads specified\", 0);\n+\n+\treturn 0;\n+}\n+\n static int file_callback(const struct option *opt, const char *arg, int unset)\n {\n \tstruct grep_opt *grep_opt = opt->value;\n@@ -1049,8 +1067,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"show context lines before matches\")),\n \t\tOPT_INTEGER('A', \"after-context\", &opt.post_context,\n \t\t\tN_(\"show context lines after matches\")),\n-\t\tOPT_INTEGER(0, \"threads\", &num_threads,\n-\t\t\tN_(\"use worker threads\")),\n+\t\tOPT_CALLBACK(0, \"threads\", &num_threads, N_(\"n\"),\n+\t\t\tN_(\"use worker threads\"), thread_callback),\n \t\tOPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n \t\t\tcontext_callback),\n \t\tOPT_BOOL('p', \"show-function\", &opt.funcname,\n@@ -1222,7 +1240,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n #ifndef NO_PTHREADS\n \tif (list.nr || cached || show_in_pager)\n \t\tnum_threads = 0;\n-\telse if (num_threads == 0)\n+\telse if (num_threads == -1)\n \t\tnum_threads = GREP_NUM_THREADS_DEFAULT;\n \telse if (num_threads < 0)\n \t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex cee42097b0..53c2ca05c4 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1505,4 +1505,14 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep with thread options' '\n+\tgit -c grep.threads=4 grep st.*dio &&\n+\tgit grep --threads=4 st.*dio &&\n+\tgit -c grep.threads=4 grep --threads=6 st.*dio &&\n+\ttest_must_fail git -c grep.threads=-1 grep st.*dio &&\n+\ttest_must_fail git -c grep.threads=-1 grep --threads=-1 st.*dio &&\n+\ttest_must_fail git -c grep.threads=-1 grep --threads=1 st.*dio &&\n+\ttest_must_fail git -c grep.threads=1 grep --threads=-1 st.*dio\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"316400","messageId":"20170408132506.5415-3-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 02/12] grep: remove redundant regflags assignment under PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:56Z","receivedAt":"2017-04-08T13:25:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove a redundant assignment to the \"regflags\" variable. This\nvariable is only used for POSIX regular expression matching, not when\nthe PCRE library is used.\n\nThis redundant assignment was added as a result of copy/paste\nprogramming in commit 84befcd0a4 (\"grep: add a grep.patternType\nconfiguration setting\", 2012-08-03). That commit modified already\nworking code in commit cca2c172e0 (\"git-grep: do not die upon -F/-P\nwhen grep.extendedRegexp is set.\", 2011-05-09) which didn't assign to\nregflags when under PCRE.\n\nRevert back to that behavior, more to reduce \"wait this is used under\nPCRE how?\" confusion when reading the code, than to to save ourselves\ntrivial CPU cycles by removing one assignment.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 56ef0ecbff..8564fe726d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -197,7 +197,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \tcase GREP_PATTERN_TYPE_PCRE:\n \t\topt->fixed = 0;\n \t\topt->pcre = 1;\n-\t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \t}\n }\n-- \n2.11.0\n\n"},{"id":"316401","messageId":"20170408132506.5415-4-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 03/12] Makefile & configure: reword outdated comment about PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:57Z","receivedAt":"2017-04-08T13:26:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Reword an outdated comment which suggests that only git-grep can use\nPCRE.\n\nThis comment was added back when PCRE support was initially added in\ncommit 63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09), and was true\nat the time.\n\nIt hasn't been telling the full truth since git-log learned to use\nPCRE with --grep in commit 727b6fc3ed (\"log --grep: accept\n--basic-regexp and --perl-regexp\", 2012-10-03), and more importantly\nis likely to get more inaccurate over time as more use is made of PCRE\nin other areas.\n\nReword it to be more future-proof, and to more clearly explain that\nthis enables user-initiated runtime behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 6 ++++--\n configure.ac | 12 ++++++++----\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9b36068ac5..23945d87cf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -24,8 +24,10 @@ all::\n # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n # This also implies BLK_SHA1.\n #\n-# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n-# able to use Perl-compatible regular expressions.\n+# Define USE_LIBPCRE if you have and want to use libpcre. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\n #\n # Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in\n # /foo/bar/include and /foo/bar/lib directories.\ndiff --git a/configure.ac b/configure.ac\nindex 128165529f..d09a204a7e 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -250,8 +250,10 @@ 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 USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n-# able to use Perl-compatible regular expressions.\n+# Define USE_LIBPCRE if you have and want to use libpcre. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\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@@ -499,8 +501,10 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO])\n GIT_CONF_SUBST([NO_OPENSSL])\n \n #\n-# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n-# able to use Perl-compatible regular expressions.\n+# Define USE_LIBPCRE if you have and want to use libpcre. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\n #\n \n if test -n \"$USE_LIBPCRE\"; then\n-- \n2.11.0\n\n"},{"id":"316402","messageId":"20170408132506.5415-5-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 04/12] grep: add a test for backreferences in PCRE patterns","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:58Z","receivedAt":"2017-04-08T13:26:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a test for backreferences such as (.)\\1 in PCRE patterns. This\ntest ensures that the PCRE_NO_AUTO_CAPTURE option isn't turned\non. Before this change turning it on would break these sort of\npatterns, but wouldn't break any tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7810-grep.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 53c2ca05c4..83b0ee53be 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1102,6 +1102,13 @@ test_expect_success LIBPCRE 'grep -P -w pattern' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success PCRE 'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE flag is not set)' '\n+\tgit grep -P -h \"(?P.)(?P=one)\" hello_world >actual &&\n+\ttest_cmp hello_world actual &&\n+\tgit grep -P -h \"(.)\\1\" hello_world >actual &&\n+\ttest_cmp hello_world actual\n+'\n+\n test_expect_success 'grep -G invalidpattern properly dies ' '\n \ttest_must_fail git grep -G \"a[\"\n '\n-- \n2.11.0\n\n"},{"id":"316403","messageId":"20170408132506.5415-6-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 05/12] log: add exhaustive tests for pattern style options & config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:24:59Z","receivedAt":"2017-04-08T13:26:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add exhaustive tests for how the different grep.patternType options &\nthe corresponding command-line options affect git-log.\n\nBefore this change it was possible to patch revision.c so that the\n--basic-regexp option was synonymous with --extended-regexp, and\n--perl-regexp wasn't recognized at all, and still have 100% of the\ntest suite pass.\n\nThis was because the first test being modified here, added in commit\n34a4ae55b2 (\"log --grep: use the same helper to set -E/-F options as\n\"git grep\"\", 2012-10-03), didn't actually check whether we'd enabled\nextended regular expressions as distinct from re-toggling non-fixed\nstring support.\n\nFix that by changing the pattern to a pattern that'll only match if\n--extended-regexp option is provided, but won't match under the\ndefault --basic-regexp option.\n\nOther potential regressions were possible since there were no tests\nfor the rest of the combinations of grep.patternType configuration\ntoggles & corresponding git-log command-line options. Add exhaustive\ntests for those.\n\nThe patterns being passed to fixed/basic/extended/PCRE are carefully\ncrafted to return the wrong thing if the grep engine were to pick any\nother matching method than the one it's told to use.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4202-log.sh | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 48b55bfd27..00d0585253 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -261,7 +261,13 @@ test_expect_success 'log --grep -i' '\n \n test_expect_success 'log -F -E --grep= uses ere' '\n \techo second >expect &&\n-\tgit log -1 --pretty=\"tformat:%s\" -F -E --grep=s.c.nd >actual &&\n+\tgit log -1 --pretty=\"tformat:%s\" -F -E --grep=\"(s).c.nd\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success LIBPCRE 'log -F -E --perl-regexp --grep= uses PCRE' '\n+\techo second >expect &&\n+\tgit log -1 --pretty=\"tformat:%s\" -F -E --perl-regexp --grep=\"s(?=ec)econd\" >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -279,6 +285,65 @@ test_expect_success 'log with grep.patternType configuration and command line' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log with various grep.patternType configurations & command-lines' '\n+\tgit init pattern-type &&\n+\t(\n+\t\tcd pattern-type &&\n+\t\ttest_commit 1 file A &&\n+\t\ttest_commit \"(1|2)\" file B &&\n+\n+\t\techo \"(1|2)\" >expect.fixed &&\n+\t\tcp expect.fixed expect.basic &&\n+\t\tcp expect.fixed expect.extended &&\n+\t\tcp expect.fixed expect.perl &&\n+\n+\t\tgit -c grep.patternType=fixed log --pretty=tformat:%s \\\n+\t\t\t--grep=\"(1|2)\" >actual.fixed &&\n+\t\tgit -c grep.patternType=basic log --pretty=tformat:%s \\\n+\t\t\t--grep=\"(.|.)\" >actual.basic &&\n+\t\tgit -c grep.patternType=extended log --pretty=tformat:%s \\\n+\t\t\t--grep=\"\\|2\" >actual.extended &&\n+\t\tif test_have_prereq LIBPCRE\n+\t\tthen\n+\t\t\tgit -c grep.patternType=perl log --pretty=tformat:%s \\\n+\t\t\t\t--grep=\"\\((?=1)\" >actual.perl\n+\t\tfi &&\n+\t\ttest_cmp expect.fixed actual.fixed &&\n+\t\ttest_cmp expect.basic actual.basic &&\n+\t\ttest_cmp expect.extended actual.extended &&\n+\t\tif test_have_prereq LIBPCRE\n+\t\tthen\n+\t\t\ttest_cmp expect.perl actual.perl\n+\t\tfi &&\n+\n+\t\tgit log --pretty=tformat:%s -F \\\n+\t\t\t--grep=\"(1|2)\" >actual.fixed.short-arg &&\n+\t\tgit log --pretty=tformat:%s -E \\\n+\t\t\t--grep=\"\\|2\" >actual.extended.short-arg &&\n+\t\ttest_cmp expect.fixed actual.fixed.short-arg &&\n+\t\ttest_cmp expect.extended actual.extended.short-arg &&\n+\n+\t\tgit log --pretty=tformat:%s --fixed-strings \\\n+\t\t\t--grep=\"(1|2)\" >actual.fixed.long-arg &&\n+\t\tgit log --pretty=tformat:%s --basic-regexp \\\n+\t\t\t--grep=\"(.|.)\" >actual.basic.long-arg &&\n+\t\tgit log --pretty=tformat:%s --extended-regexp \\\n+\t\t\t--grep=\"\\|2\" >actual.extended.long-arg &&\n+\t\tif test_have_prereq LIBPCRE\n+\t\tthen\n+\t\t\tgit log --pretty=tformat:%s --perl-regexp \\\n+\t\t\t\t--grep=\"\\((?=1)\" >actual.perl.long-arg\n+\t\tfi &&\n+\t\ttest_cmp expect.fixed actual.fixed.long-arg &&\n+\t\ttest_cmp expect.basic actual.basic.long-arg &&\n+\t\ttest_cmp expect.extended actual.extended.long-arg &&\n+\t\tif test_have_prereq LIBPCRE\n+\t\tthen\n+\t\t\ttest_cmp expect.perl actual.perl.long-arg\n+\t\tfi\n+\t)\n+'\n+\n cat > expect <.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/rev-list-options.txt | 1 +\n revision.c | 2 +-\n t/t4202-log.sh | 9 +++++++++\n 3 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a02f7324c0..5b7fa49a7f 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -91,6 +91,7 @@ endif::git-rev-list[]\n \tConsider the limiting patterns to be fixed strings (don't interpret\n \tpattern as a regular expression).\n \n+-P::\n --perl-regexp::\n \tConsider the limiting patterns to be Perl-compatible regular expressions.\n \tRequires libpcre to be compiled in.\ndiff --git a/revision.c b/revision.c\nindex 7ff61ff5f7..03a3a012de 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1995,7 +1995,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n-\t} else if (!strcmp(arg, \"--perl-regexp\")) {\n+\t} else if (!strcmp(arg, \"--perl-regexp\") || !strcmp(arg, \"-P\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;\n \t} else if (!strcmp(arg, \"--all-match\")) {\n \t\trevs->grep_filter.all_match = 1;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 00d0585253..6f7c74e027 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -320,8 +320,17 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\t\t--grep=\"(1|2)\" >actual.fixed.short-arg &&\n \t\tgit log --pretty=tformat:%s -E \\\n \t\t\t--grep=\"\\|2\" >actual.extended.short-arg &&\n+\t\tif test_have_prereq PCRE\n+\t\tthen\n+\t\t\tgit log --pretty=tformat:%s -P \\\n+\t\t\t\t--grep=\"\\((?=1)\" >actual.perl.short-arg\n+\t\tfi &&\n \t\ttest_cmp expect.fixed actual.fixed.short-arg &&\n \t\ttest_cmp expect.extended actual.extended.short-arg &&\n+\t\tif test_have_prereq PCRE\n+\t\tthen\n+\t\t\ttest_cmp expect.perl actual.perl.short-arg\n+\t\tfi &&\n \n \t\tgit log --pretty=tformat:%s --fixed-strings \\\n \t\t\t--grep=\"(1|2)\" >actual.fixed.long-arg &&\n-- \n2.11.0\n\n"},{"id":"316405","messageId":"20170408132506.5415-9-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 08/12] grep: make grep.patternType=[pcre|pcre1] a synonym for \"perl\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:02Z","receivedAt":"2017-04-08T13:26:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the pattern types \"pcre\" & \"pcre1\" synonyms for long-standing\n\"perl\" grep.patternType.\n\nThis change is part of a longer patch series to add pcre2 support to\nGit. It's nice to be able to performance test PCRE v1 v.s. v2 without\nhaving to recompile git, and doing that via grep.patternType makes\nsense.\n\nHowever, just adding \"pcre2\" when we only have \"perl\" would be\nconfusing, so start by adding a \"pcre\" & \"pcre1\" synonym.\n\nIn the future \"perl\" and \"pcre\" might be changed to default to \"pcre2\"\ninstead of \"pcre1\", and depending on how Git is compiled the more\nspecific \"pcre1\" or \"pcre2\" pattern types might produce an error.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 9 +++++++++\n grep.c | 4 +++-\n t/t7810-grep.sh | 10 ++++++++++\n 3 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d51..5ef12d0694 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1624,6 +1624,15 @@ grep.patternType::\n \t'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,\n \t`--fixed-strings`, or `--perl-regexp` option accordingly, while the\n \tvalue 'default' will return to the default matching behavior.\n++\n+The 'pcre' and 'pcre1' values are synonyms for 'perl'. The other\n+values starting with 'pcre' are reserved for future use, e.g. if we'd\n+like to use 'pcre2' for the PCRE v2 library.\n++\n+In the future 'perl' and 'pcre' might become synonyms for some other\n+implementation or PCRE version, such as 'pcre2', while the more\n+specific 'pcre1' & 'pcre2' might throw errors depending on whether git\n+is compiled to include those libraries.\n \n grep.extendedRegexp::\n \tIf set to true, enable `--extended-regexp` option by default. This\ndiff --git a/grep.c b/grep.c\nindex 8564fe726d..1575f8f9ed 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -60,7 +60,9 @@ static int parse_pattern_type_arg(const char *opt, const char *arg)\n \t\treturn GREP_PATTERN_TYPE_ERE;\n \telse if (!strcmp(arg, \"fixed\"))\n \t\treturn GREP_PATTERN_TYPE_FIXED;\n-\telse if (!strcmp(arg, \"perl\"))\n+\telse if (!strcmp(arg, \"perl\") ||\n+\t\t !strcmp(arg, \"pcre\") ||\n+\t\t !strcmp(arg, \"pcre1\"))\n \t\treturn GREP_PATTERN_TYPE_PCRE;\n \tdie(\"bad %s argument: %s\", opt, arg);\n }\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 83b0ee53be..b50f1dff43 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1522,4 +1522,14 @@ test_expect_success 'grep with thread options' '\n \ttest_must_fail git -c grep.threads=1 grep --threads=-1 st.*dio\n '\n \n+test_expect_success LIBPCRE \"grep with grep.patternType synonyms perl/pcre/pcre1\" '\n+\techo \"#include \" >expected &&\n+\tgit -c grep.patternType=perl grep -h --no-line-number \"st(?=dio)\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit -c grep.patternType=pcre grep -h --no-line-number \"st(?=dio)\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit -c grep.patternType=pcre1 grep -h --no-line-number \"st(?=dio)\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"316406","messageId":"20170408132506.5415-8-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 07/12] grep & rev-list doc: stop promising libpcre for --perl-regexp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:01Z","receivedAt":"2017-04-08T13:26:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop promising in our grep & rev-list options documentation that we're\nalways going to be using libpcre when given the --perl-regexp option.\n\nInstead talk about using \"Perl-compatible regular expressions\" and\nusing \"the PCRE library\". Saying \"libpcre\" strongly suggests that we\nmight be talking about libpcre.so, which is always going to be v1.\n\nNot only do does the documentation now align more clearly with future\nplans, but it should be more easily readable to the layman.\n\nThis is for preparation for libpcre2 support, which in the future may\nbe powering the --perl-regexp option by default, depending on how Git\nis compiled.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-grep.txt | 4 ++--\n Documentation/rev-list-options.txt | 5 +++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 7b52e3fbc4..1c01154dbe 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -161,8 +161,8 @@ OPTIONS\n \n -P::\n --perl-regexp::\n-\tUse Perl-compatible regexp for patterns. Requires libpcre to be\n-\tcompiled in.\n+\tUse Perl-compatible regular expressions for patterns. Uses the\n+\tPCRE library, which Git needs to be compiled against.\n \n -F::\n --fixed-strings::\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5b7fa49a7f..565cde636e 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -93,8 +93,9 @@ endif::git-rev-list[]\n \n -P::\n --perl-regexp::\n-\tConsider the limiting patterns to be Perl-compatible regular expressions.\n-\tRequires libpcre to be compiled in.\n+\tConsider the limiting patterns to be Perl-compatible regular\n+\texpressions. Uses the PCRE library, which Git needs to be\n+\tcompiled against.\n \n --remove-empty::\n \tStop when a given path disappears from the tree.\n-- \n2.11.0\n\n"},{"id":"316407","messageId":"20170408132506.5415-10-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 09/12] test-lib: rename the LIBPCRE prerequisite to PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:03Z","receivedAt":"2017-04-08T13:26:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rename the LIBPCRE prerequisite to PCRE. This is for preparation for\nlibpcre2 support, where having just \"LIBPCRE\" would be confusing as it\nimplies v1 of the library.\n\nNone of these tests are incompatible between versions 1 & 2 of\nlibpcre, it's less confusing to give them a more general name to make\nit clear that they work on both library versions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 4 ++--\n t/t4202-log.sh | 10 +++++-----\n t/t7810-grep.sh | 30 +++++++++++++++---------------\n t/t7812-grep-icase-non-ascii.sh | 4 ++--\n t/t7813-grep-icase-iso.sh | 2 +-\n t/test-lib.sh | 2 +-\n 6 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex ab386c3681..a90cb62583 100644\n--- a/t/README\n+++ b/t/README\n@@ -803,9 +803,9 @@ use these, and \"test_set_prereq\" for how to define your own.\n Test is not run by root user, and an attempt to write to an\n unwritable file is expected to fail correctly.\n \n- - LIBPCRE\n+ - PCRE\n \n- Git was compiled with USE_LIBPCRE=YesPlease. Wrap any tests\n+ Git was compiled with support for PCRE. Wrap any tests\n that use git-grep --perl-regexp or git-grep -P in these.\n \n - CASE_INSENSITIVE_FS\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 6f7c74e027..936666bb8a 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -265,7 +265,7 @@ test_expect_success 'log -F -E --grep= uses ere' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success LIBPCRE 'log -F -E --perl-regexp --grep= uses PCRE' '\n+test_expect_success PCRE 'log -F -E --perl-regexp --grep= uses PCRE' '\n \techo second >expect &&\n \tgit log -1 --pretty=\"tformat:%s\" -F -E --perl-regexp --grep=\"s(?=ec)econd\" >actual &&\n \ttest_cmp expect actual\n@@ -303,7 +303,7 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\t\t--grep=\"(.|.)\" >actual.basic &&\n \t\tgit -c grep.patternType=extended log --pretty=tformat:%s \\\n \t\t\t--grep=\"\\|2\" >actual.extended &&\n-\t\tif test_have_prereq LIBPCRE\n+\t\tif test_have_prereq PCRE\n \t\tthen\n \t\t\tgit -c grep.patternType=perl log --pretty=tformat:%s \\\n \t\t\t\t--grep=\"\\((?=1)\" >actual.perl\n@@ -311,7 +311,7 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\ttest_cmp expect.fixed actual.fixed &&\n \t\ttest_cmp expect.basic actual.basic &&\n \t\ttest_cmp expect.extended actual.extended &&\n-\t\tif test_have_prereq LIBPCRE\n+\t\tif test_have_prereq PCRE\n \t\tthen\n \t\t\ttest_cmp expect.perl actual.perl\n \t\tfi &&\n@@ -338,7 +338,7 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\t\t--grep=\"(.|.)\" >actual.basic.long-arg &&\n \t\tgit log --pretty=tformat:%s --extended-regexp \\\n \t\t\t--grep=\"\\|2\" >actual.extended.long-arg &&\n-\t\tif test_have_prereq LIBPCRE\n+\t\tif test_have_prereq PCRE\n \t\tthen\n \t\t\tgit log --pretty=tformat:%s --perl-regexp \\\n \t\t\t\t--grep=\"\\((?=1)\" >actual.perl.long-arg\n@@ -346,7 +346,7 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\ttest_cmp expect.fixed actual.fixed.long-arg &&\n \t\ttest_cmp expect.basic actual.basic.long-arg &&\n \t\ttest_cmp expect.extended actual.extended.long-arg &&\n-\t\tif test_have_prereq LIBPCRE\n+\t\tif test_have_prereq PCRE\n \t\tthen\n \t\t\ttest_cmp expect.perl actual.perl.long-arg\n \t\tfi\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex b50f1dff43..46f528183d 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -275,7 +275,7 @@ do\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success LIBPCRE \"grep $L with grep.patterntype=perl\" '\n+\ttest_expect_success PCRE \"grep $L with grep.patterntype=perl\" '\n \t\techo \"${HC}ab:a+b*c\" >expected &&\n \t\tgit -c grep.patterntype=perl grep \"a\\x{2b}b\\x{2a}c\" $H ab >actual &&\n \t\ttest_cmp expected actual\n@@ -1053,12 +1053,12 @@ hello.c:int main(int argc, const char **argv)\n hello.c:\tprintf(\"Hello world.\\n\");\n EOF\n \n-test_expect_success LIBPCRE 'grep --perl-regexp pattern' '\n+test_expect_success PCRE 'grep --perl-regexp pattern' '\n \tgit grep --perl-regexp \"\\p{Ps}.*?\\p{Pe}\" hello.c >actual &&\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P pattern' '\n+test_expect_success PCRE 'grep -P pattern' '\n \tgit grep -P \"\\p{Ps}.*?\\p{Pe}\" hello.c >actual &&\n \ttest_cmp expected actual\n '\n@@ -1070,13 +1070,13 @@ test_expect_success 'grep pattern with grep.extendedRegexp=true' '\n \ttest_cmp empty actual\n '\n \n-test_expect_success LIBPCRE 'grep -P pattern with grep.extendedRegexp=true' '\n+test_expect_success PCRE 'grep -P pattern with grep.extendedRegexp=true' '\n \tgit -c grep.extendedregexp=true \\\n \t\tgrep -P \"\\p{Ps}.*?\\p{Pe}\" hello.c >actual &&\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P -v pattern' '\n+test_expect_success PCRE 'grep -P -v pattern' '\n \t{\n \t\techo \"ab:a+b*c\"\n \t\techo \"ab:a+bc\"\n@@ -1085,7 +1085,7 @@ test_expect_success LIBPCRE 'grep -P -v pattern' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P -i pattern' '\n+test_expect_success PCRE 'grep -P -i pattern' '\n \tcat >expected <<-EOF &&\n \thello.c:\tprintf(\"Hello world.\\n\");\n \tEOF\n@@ -1093,7 +1093,7 @@ test_expect_success LIBPCRE 'grep -P -i pattern' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P -w pattern' '\n+test_expect_success PCRE 'grep -P -w pattern' '\n \t{\n \t\techo \"hello_world:Hello world\"\n \t\techo \"hello_world:HeLLo world\"\n@@ -1125,11 +1125,11 @@ test_expect_success 'grep invalidpattern properly dies with grep.patternType=ext\n \ttest_must_fail git -c grep.patterntype=extended grep \"a[\"\n '\n \n-test_expect_success LIBPCRE 'grep -P invalidpattern properly dies ' '\n+test_expect_success PCRE 'grep -P invalidpattern properly dies ' '\n \ttest_must_fail git grep -P \"a[\"\n '\n \n-test_expect_success LIBPCRE 'grep invalidpattern properly dies with grep.patternType=perl' '\n+test_expect_success PCRE 'grep invalidpattern properly dies with grep.patternType=perl' '\n \ttest_must_fail git -c grep.patterntype=perl grep \"a[\"\n '\n \n@@ -1198,13 +1198,13 @@ test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =perl, =e\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -G -F -E -P pattern' '\n+test_expect_success PCRE 'grep -G -F -E -P pattern' '\n \techo \"d0:0\" >expected &&\n \tgit grep -G -F -E -P \"[\\d]\" d0 >actual &&\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep pattern with grep.patternType=fixed, =basic, =extended, =perl' '\n+test_expect_success PCRE 'grep pattern with grep.patternType=fixed, =basic, =extended, =perl' '\n \techo \"d0:0\" >expected &&\n \tgit \\\n \t\t-c grep.patterntype=fixed \\\n@@ -1215,7 +1215,7 @@ test_expect_success LIBPCRE 'grep pattern with grep.patternType=fixed, =basic, =\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P pattern with grep.patternType=fixed' '\n+test_expect_success PCRE 'grep -P pattern with grep.patternType=fixed' '\n \techo \"ab:a+b*c\" >expected &&\n \tgit \\\n \t\t-c grep.patterntype=fixed \\\n@@ -1350,12 +1350,12 @@ space: line with leading space2\n space: line with leading space3\n EOF\n \n-test_expect_success LIBPCRE 'grep -E \"^ \"' '\n+test_expect_success PCRE 'grep -E \"^ \"' '\n \tgit grep -E \"^ \" space >actual &&\n \ttest_cmp expected actual\n '\n \n-test_expect_success LIBPCRE 'grep -P \"^ \"' '\n+test_expect_success PCRE 'grep -P \"^ \"' '\n \tgit grep -P \"^ \" space >actual &&\n \ttest_cmp expected actual\n '\n@@ -1522,7 +1522,7 @@ test_expect_success 'grep with thread options' '\n \ttest_must_fail git -c grep.threads=1 grep --threads=-1 st.*dio\n '\n \n-test_expect_success LIBPCRE \"grep with grep.patternType synonyms perl/pcre/pcre1\" '\n+test_expect_success PCRE \"grep with grep.patternType synonyms perl/pcre/pcre1\" '\n \techo \"#include \" >expected &&\n \tgit -c grep.patternType=perl grep -h --no-line-number \"st(?=dio)\" >actual &&\n \ttest_cmp expected actual &&\ndiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\nindex 169fd8d706..04a61cb8e0 100755\n--- a/t/t7812-grep-icase-non-ascii.sh\n+++ b/t/t7812-grep-icase-non-ascii.sh\n@@ -20,13 +20,13 @@ test_expect_success REGEX_LOCALE 'grep literal string, no -F' '\n \tgit grep -i \"TILRAUN: HALLÓ HEIMUR!\"\n '\n \n-test_expect_success GETTEXT_LOCALE,LIBPCRE 'grep pcre utf-8 icase' '\n+test_expect_success GETTEXT_LOCALE,PCRE 'grep pcre utf-8 icase' '\n \tgit grep --perl-regexp \"TILRAUN: H.lló Heimur!\" &&\n \tgit grep --perl-regexp -i \"TILRAUN: H.lló Heimur!\" &&\n \tgit grep --perl-regexp -i \"TILRAUN: H.LLÓ HEIMUR!\"\n '\n \n-test_expect_success GETTEXT_LOCALE,LIBPCRE 'grep pcre utf-8 string with \"+\"' '\n+test_expect_success GETTEXT_LOCALE,PCRE 'grep pcre utf-8 string with \"+\"' '\n \ttest_write_lines \"TILRAUN: Hallóó Heimur!\" >file2 &&\n \tgit add file2 &&\n \tgit grep -l --perl-regexp \"TILRAUN: H.lló+ Heimur!\" >actual &&\ndiff --git a/t/t7813-grep-icase-iso.sh b/t/t7813-grep-icase-iso.sh\nindex efef7fb81f..701e08a8e5 100755\n--- a/t/t7813-grep-icase-iso.sh\n+++ b/t/t7813-grep-icase-iso.sh\n@@ -11,7 +11,7 @@ test_expect_success GETTEXT_ISO_LOCALE 'setup' '\n \texport LC_ALL\n '\n \n-test_expect_success GETTEXT_ISO_LOCALE,LIBPCRE 'grep pcre string' '\n+test_expect_success GETTEXT_ISO_LOCALE,PCRE 'grep pcre string' '\n \tgit grep --perl-regexp -i \"TILRAUN: H.ll� Heimur!\" &&\n \tgit grep --perl-regexp -i \"TILRAUN: H.LL� HEIMUR!\"\n '\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 13b5696822..6abf1d1918 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1010,7 +1010,7 @@ esac\n ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1\n test -z \"$NO_PERL\" && test_set_prereq PERL\n test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n-test -n \"$USE_LIBPCRE\" && test_set_prereq LIBPCRE\n+test -n \"$USE_LIBPCRE\" && test_set_prereq PCRE\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-- \n2.11.0\n\n"},{"id":"316408","messageId":"20170408132506.5415-11-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 10/12] grep: change the internal PCRE macro names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:04Z","receivedAt":"2017-04-08T13:26:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the internal USE_LIBPCRE define, & build options flag to use a\nnaming convention ending in PCRE1, without changing the long-standing\nUSE_LIBPCRE Makefile flag which enables this code.\n\nThis is for preparation for libpcre2 support where having things like\nUSE_LIBPCRE and USE_LIBPCRE2 in any more places than we absolutely\nneed to for backwards compatibility with old Makefile arguments would\nbe confusing.\n\nIn some ways it would be better to change everything that now uses\nUSE_LIBPCRE to use USE_LIBPCRE1, and to make specifying\nUSE_LIBPCRE (or --with-pcre) an error. This would impose a one-time\nburden on packagers of git to s/USE_LIBPCRE/USE_LIBPCRE1/ in their\nbuild scripts.\n\nHowever I'd like to leave the door open to making\nUSE_LIBPCRE=YesPlease eventually mean USE_LIBPCRE2=YesPlease,\ni.e. once PCRE v2 is ubiquitous enough that it makes sense to make it\nthe default.\n\nThis code and the USE_LIBPCRE Makefile argument was added in commit\n63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09). At the time there was\nno indication that the PCRE project would release an entirely new &\nincompatible API around 3 years later.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 4 ++--\n grep.c | 6 +++---\n grep.h | 2 +-\n t/test-lib.sh | 2 +-\n 4 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 23945d87cf..c8a26087e3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1084,7 +1084,7 @@ ifdef NO_LIBGEN_H\n endif\n \n ifdef USE_LIBPCRE\n-\tBASIC_CFLAGS += -DUSE_LIBPCRE\n+\tBASIC_CFLAGS += -DUSE_LIBPCRE1\n \tifdef LIBPCREDIR\n \t\tBASIC_CFLAGS += -I$(LIBPCREDIR)/include\n \t\tEXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)\n@@ -2235,7 +2235,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo TAR=\\''$(subst ','\\'',$(subst ','\\'',$(TAR)))'\\' >>$@+\n \t@echo NO_CURL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_CURL)))'\\' >>$@+\n \t@echo NO_EXPAT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_EXPAT)))'\\' >>$@+\n-\t@echo USE_LIBPCRE=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE)))'\\' >>$@+\n+\t@echo USE_LIBPCRE1=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE)))'\\' >>$@+\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\ndiff --git a/grep.c b/grep.c\nindex 1575f8f9ed..99b9e9447f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -325,7 +325,7 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p,\n \tdie(\"%s'%s': %s\", where, p->pattern, error);\n }\n \n-#ifdef USE_LIBPCRE\n+#ifdef USE_LIBPCRE1\n static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tconst char *error;\n@@ -377,7 +377,7 @@ static void free_pcre_regexp(struct grep_pat *p)\n \tpcre_free(p->pcre_extra_info);\n \tpcre_free((void *)p->pcre_tables);\n }\n-#else /* !USE_LIBPCRE */\n+#else /* !USE_LIBPCRE1 */\n static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\n@@ -392,7 +392,7 @@ static int pcrematch(struct grep_pat *p, const char *line, const char *eol,\n static void free_pcre_regexp(struct grep_pat *p)\n {\n }\n-#endif /* !USE_LIBPCRE */\n+#endif /* !USE_LIBPCRE1 */\n \n static int is_fixed(const char *s, size_t len)\n {\ndiff --git a/grep.h b/grep.h\nindex 267534ca24..073b0e4c92 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -1,7 +1,7 @@\n #ifndef GREP_H\n #define GREP_H\n #include \"color.h\"\n-#ifdef USE_LIBPCRE\n+#ifdef USE_LIBPCRE1\n #include \n #else\n typedef int pcre;\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 6abf1d1918..e5cfbcc36b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1010,7 +1010,7 @@ esac\n ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1\n test -z \"$NO_PERL\" && test_set_prereq PERL\n test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n-test -n \"$USE_LIBPCRE\" && test_set_prereq PCRE\n+test -n \"$USE_LIBPCRE1\" && test_set_prereq PCRE\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-- \n2.11.0\n\n"},{"id":"316409","messageId":"20170408132506.5415-13-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 12/12] grep: add support for PCRE v2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:06Z","receivedAt":"2017-04-08T13:26:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add support for v2 of the PCRE API. This is a new major version of\nPCRE that came out in early 2015[1]. The regular expression syntax is\nthe same, but while similar-ish requires a different codepath to\nsupport it.\n\nGit can now be compiled with any combination of\nUSE_LIBPCRE=[YesPlease|] & USE_LIBPCRE2=[YesPlease|]. If both are\nprovided the version of the PCRE library can be selected at runtime\nwith grep.PatternType, but the default (for now) is v1. This table\nshows what the various combinations do depending on what libraries Git\nis compiled against:\n\n |------------------+-----+-----+----------|\n | grep.PatternType | v1 | v2 | v1 && v2 |\n |------------------+-----+-----+----------|\n | perl | v1 | v2 | v1 |\n | pcre | v1 | v2 | v1 |\n | pcre1 | v1 | ERR | v1 |\n | pcre2 | ERR | v2 | v2 |\n |------------------+-----+-----+----------|\n\nWhen Git is only compiled with v2 grep.PatternType=perl, --perl-regexp\n& -P will use v2. All tests pass with this new PCRE version. When Git\nis compiled with both v1 & v2 most of the tests will only test v1, but\nthere are some v2-specific tests that will be run.\n\nOriginally I started work on this series because my ad-hoc patch to\nsupport v2 (\"Very promising results with libpcre2\",\n)\nshowed very promising performance results. E.g. grepping a rare string\non the linux.git tree for 50 iterations gives[2]:\n\n s/iter basic extended pcre1 fixed pcre2\n basic 2.31 -- -5% -6% -36% -46%\n extended 2.20 5% -- -1% -32% -44%\n pcre1 2.17 6% 1% -- -32% -43%\n fixed 1.49 56% 48% 46% -- -17%\n pcre2 1.24 87% 77% 75% 20% --\n\nI.e. PCRE v2 is around 20% faster than the previously fastest codepath\nto grep for a fixed string, while grepping for a similar regex. A\nsimilar test for 'log --grep' is less promising[3]:\n\n s/iter extended basic pcre2 pcre1 fixed\n extended 8.36 -- -0% -1% -6% -11%\n basic 8.35 0% -- -1% -5% -11%\n pcre2 8.26 1% 1% -- -4% -10%\n pcre1 7.90 6% 6% 5% -- -6%\n fixed 7.44 12% 12% 11% 6% --\n\nHere PCRE v2 is around 5% slower than v1. I don't know why that\nis. Perhaps PCRE v2 is worse at matching shorter strings, commit\nmessages tend to be shorter than files.\n\nI eventually found out though that the main difference in v1 and v2\nperformance is because v2 is using the JIT feature, but we never got\naround to doing that for v1. The two benchmarks above run without v2\nJIT are for \"grep\" & \"log\", respectively:\n\n s/iter basic extended pcre1 pcre2 fixed\n basic 2.23 -- -0% -1% -15% -31%\n extended 2.22 0% -- -1% -15% -31%\n pcre1 2.21 1% 1% -- -15% -31%\n pcre2 1.89 18% 18% 17% -- -19%\n fixed 1.53 46% 45% 44% 23% --\n\n s/iter basic pcre2 extended pcre1 fixed\n basic 10.9 -- -21% -24% -28% -33%\n pcre2 8.63 26% -- -4% -9% -16%\n extended 8.30 31% 4% -- -5% -12%\n pcre1 7.88 38% 10% 5% -- -8%\n fixed 7.28 50% 19% 14% 8% --\n\nHere v2 still outperforms v1 on the \"grep\" test, but by 15% instead of\n43%, v1 is still faster on \"log\", but by 9% instead of 4%.\n\nWhen both v1 and v2 use JIT v1 will ever so slightly beat v2 by 1% on\nthe \"grep\" test:\n\n s/iter basic extended fixed pcre2 pcre1\n basic 2.22 -- -0% -33% -44% -45%\n extended 2.21 0% -- -33% -44% -45%\n fixed 1.49 49% 49% -- -17% -18%\n pcre2 1.24 80% 79% 20% -- -1%\n pcre1 1.22 82% 81% 22% 1% --\n\nBut v1 is 10% faster than v2 for \"log\" when both have JIT, as opposed\nto 4% faster when v2 has JIT and v1 doesn't:\n\n s/iter basic extended pcre2 pcre1 fixed\nbasic 9.43 -- -11% -12% -21% -22%\nextended 8.39 12% -- -2% -11% -12%\npcre2 8.26 14% 2% -- -10% -11%\npcre1 7.45 27% 13% 11% -- -1%\nfixed 7.37 28% 14% 12% 1% --\n\nThe v1 JIT support uses a patch that isn't part of this series\nyet. Supporting it is slightly tricker than for v2 because we need to\nsupport PCRE v1 versions that don't have JIT support, the \"SIMPLE USE\nOF JIT\" section in pcrejit(3) documents how to do that.\n\nThe PCRE performance improvements suggest that there are numerous\nother follow-up projects on grep.c which could yield better\nperformance:\n\n - Disable the \"fast\" path for fixed patterns in compile_regexp() when\n PCRE is being used, at least in some cases. It was added in commit\n 9eceddeec6 (\"Use kwset in grep\", 2011-08-21) as an optimization,\n but clearly results in taking a slower path with PCRE in some\n cases.\n\n - Similarly -F on a pattern that !is_fixed() (e.g. -F '[f]oo') might\n be better of escaping the regex characters & feeding them to PCRE\n than using kwset.\n\n - Other things could be farmed out to the regex engine rather than\n being handled by our own custom code. E.g. the support for -w in\n match_one_pattern() added in commit 7839a25eab (\"builtin-grep:\n support -w (--word-regexp).\", 2006-05-02) can instead by done by\n munging the PATTERN to \\bPATTERN\\b.\n\n This would make it easy to support e.g. -x by similarly prefixing\n the line with \"^(?:\" & suffixing it with \")$\". See how pcre2grep.c\n handles -w, -x & -F by simply munging the pattern.\n\n - The look-ahead support added in commit a26345b608 (\"grep: optimize\n built-in grep by skipping lines that do not hit\", 2010-01-10) might\n need tweaking to either match in smaller or bigger\n batches. Possibly with some heuristic that tries to detect if the\n pattern has been matching a lot (\".\" v.s. \"rare.*string\") so far.\n\nA brief note on thread safety: As noted in pcre2api(3) & pcre2jit(3)\nthe compiled pattern can be shared between threads, but not some of\nthe JIT context, however the grep threading support does all pattern &\nJIT compilation in separate threads, so this code doesn't need to\nconcern itself with thread safety.\n\nSee commit 63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) for the\ninitial addition of PCRE v1. This change follows some of the same\npatterns it did (and which were discussed on list at the time),\ne.g. mocking up types with typedef instead of ifdef-ing them out when\nUSE_LIBPCRE2 isn't defined. This adds some trivial memory use to the\nprogram, but makes the code look nicer.\n\n1. https://lists.exim.org/lurker/message/20150105.162835.0666407a.en.html\n\n2. PF=~/g/git/ perl -MBenchmark=cmpthese -wE 'cmpthese(50, { fixed => sub { system \"$ENV{PF}git grep -F avarasu >/dev/null\" }, basic => sub { system \"$ENV{PF}git grep -G avara?su >/dev/null\" }, extended => sub { system \"$ENV{PF}git grep -E avara?su >/dev/null\" }, pcre1 => sub { system \"$ENV{PF}git -c grep.patternType=pcre1 grep avara?su >/dev/null\" }, pcre2 => sub { system \"$ENV{PF}git -c grep.patternType=pcre2 grep avara?su >/dev/null\" } })'\n\n3. PF=~/g/git/ perl -MBenchmark=cmpthese -wE 'cmpthese(10, { fixed => sub { system \"$ENV{PF}git log -F --grep=avarasu >/dev/null\" }, basic => sub { system \"$ENV{PF}git log --basic-regexp --grep=avara?su >/dev/null\" }, extended => sub { system \"$ENV{PF}git log --extended-regexp --grep=avara?su >/dev/null\" }, pcre1 => sub { system \"$ENV{PF}git -c grep.patternType=pcre1 log --grep=avara?su >/dev/null\" }, pcre2 => sub { system \"$ENV{PF}git -c grep.patternType=pcre2 log --grep=avara?su >/dev/null\" } })'\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 14 +++---\n Makefile | 18 +++++++\n builtin/grep.c | 4 +-\n configure.ac | 49 +++++++++++++++++-\n grep.c | 125 +++++++++++++++++++++++++++++++++++++++++++++-\n grep.h | 16 +++++-\n revision.c | 2 +-\n t/README | 12 +++++\n t/t7810-grep.sh | 30 +++++++++--\n t/t7813-grep-icase-iso.sh | 11 ++--\n t/test-lib.sh | 4 +-\n 11 files changed, 261 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5ef12d0694..a5fc482495 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1625,14 +1625,12 @@ grep.patternType::\n \t`--fixed-strings`, or `--perl-regexp` option accordingly, while the\n \tvalue 'default' will return to the default matching behavior.\n +\n-The 'pcre' and 'pcre1' values are synonyms for 'perl'. The other\n-values starting with 'pcre' are reserved for future use, e.g. if we'd\n-like to use 'pcre2' for the PCRE v2 library.\n-+\n-In the future 'perl' and 'pcre' might become synonyms for some other\n-implementation or PCRE version, such as 'pcre2', while the more\n-specific 'pcre1' & 'pcre2' might throw errors depending on whether git\n-is compiled to include those libraries.\n+The 'perl' and 'pcre' values are synonyms. Depending on which PCRE\n+library Git was compiled with either or both of 'pcre1' and 'pcre2'\n+might also be available.\n++\n+If both are available Git currently defaults to 'pcre1', but this\n+might change in future versions.\n \n grep.extendedRegexp::\n \tIf set to true, enable `--extended-regexp` option by default. This\ndiff --git a/Makefile b/Makefile\nindex c8a26087e3..bf8db4bae9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -32,6 +32,14 @@ all::\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 USE_LIBPCRE if you have and want to use libpcre2. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\n+#\n+# Define LIBPCRE2DIR=/foo/bar if your libpcre2 header and library\n+# files are in /foo/bar/include and /foo/bar/lib directories.\n+#\n # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.\n #\n # Define NO_CURL if you do not have libcurl installed. git-http-fetch and\n@@ -1092,6 +1100,15 @@ ifdef USE_LIBPCRE\n \tEXTLIBS += -lpcre\n endif\n \n+ifdef USE_LIBPCRE2\n+\tBASIC_CFLAGS += -DUSE_LIBPCRE2\n+\tifdef LIBPCRE2DIR\n+\t\tBASIC_CFLAGS += -I$(LIBPCREDIR)/include\n+\t\tEXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)\n+\tendif\n+\tEXTLIBS += -lpcre2-8\n+endif\n+\n ifdef HAVE_ALLOCA_H\n \tBASIC_CFLAGS += -DHAVE_ALLOCA_H\n endif\n@@ -2236,6 +2253,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_CURL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_CURL)))'\\' >>$@+\n \t@echo NO_EXPAT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_EXPAT)))'\\' >>$@+\n \t@echo USE_LIBPCRE1=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE)))'\\' >>$@+\n+\t@echo USE_LIBPCRE2=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE2)))'\\' >>$@+\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex dffb9743b8..9478ab5dff 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\targv_array_push(&submodule_options, \"-F\");\n \t\tbreak;\n-\tcase GREP_PATTERN_TYPE_PCRE1:\n+\tcase GREP_PATTERN_TYPE_PCRE:\n \t\targv_array_push(&submodule_options, \"-P\");\n \t\tbreak;\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n@@ -1036,7 +1036,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t GREP_PATTERN_TYPE_FIXED),\n \t\tOPT_SET_INT('P', \"perl-regexp\", &pattern_type_arg,\n \t\t\t N_(\"use Perl-compatible regular expressions\"),\n-\t\t\t GREP_PATTERN_TYPE_PCRE1),\n+\t\t\t GREP_PATTERN_TYPE_PCRE),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOL('n', \"line-number\", &opt.linenum, N_(\"show line numbers\")),\n \t\tOPT_NEGBIT('h', NULL, &opt.pathname, N_(\"don't show filenames\"), 1),\ndiff --git a/configure.ac b/configure.ac\nindex d09a204a7e..6c99c4c285 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -259,8 +259,8 @@ GIT_PARSE_WITH([openssl]))\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 NO)])\n-AS_HELP_STRING([], [ARG can be also prefix for libpcre library and headers]),\n+AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes via libpcre1 (default is NO)])\n+AS_HELP_STRING([], [ARG can be also prefix for libpcre1 library and headers]),\n if test \"$withval\" = \"no\"; then\n \tUSE_LIBPCRE=\n elif test \"$withval\" = \"yes\"; then\n@@ -273,6 +273,30 @@ AS_HELP_STRING([], [ARG can be also prefix for libpcre library and hea\n dnl it yet.\n \tGIT_CONF_SUBST([LIBPCREDIR])\n fi)\n+\n+# Define USE_LIBPCRE2 if you have and want to use libpcre2. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\n+#\n+# Define LIBPCR2EDIR=/foo/bar if your libpcre2 header and library\n+# files are in /foo/bar/include and /foo/bar/lib directories.\n+#\n+AC_ARG_WITH(libpcre2,\n+AS_HELP_STRING([--with-libpcre2],[support Perl-compatible regexes via libpcre2 (default is NO)])\n+AS_HELP_STRING([], [ARG can be also prefix for libpcre library and headers]),\n+ if test \"$withval\" = \"no\"; then\n+\tUSE_LIBPCRE2=\n+ elif test \"$withval\" = \"yes\"; then\n+\tUSE_LIBPCRE2=YesPlease\n+ else\n+\tUSE_LIBPCRE2=YesPlease\n+\tLIBPCRE2DIR=$withval\n+\tAC_MSG_NOTICE([Setting LIBPCRE2DIR to $LIBPCREDIR])\n+ dnl USE_LIBPCRE2 can still be modified below, so don't substitute\n+ dnl it yet.\n+\tGIT_CONF_SUBST([LIBPCRE2DIR])\n+ fi)\n #\n # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.\n AC_FUNC_ALLOCA\n@@ -522,6 +546,27 @@ GIT_CONF_SUBST([USE_LIBPCRE])\n fi\n \n #\n+# Define USE_LIBPCRE2 if you have and want to use libpcre. Various\n+# commands such as like log, grep offer runtime options to use\n+# Perl-compatible regular expressions instead of standard or extended\n+# POSIX regular expressions.\n+#\n+\n+if test -n \"$USE_LIBPCRE2\"; then\n+\n+GIT_STASH_FLAGS($LIBPCRE2DIR)\n+\n+AC_CHECK_LIB([pcre2-8], [pcre2_config_8],\n+[USE_LIBPCRE2=YesPlease],\n+[USE_LIBPCRE2=])\n+\n+GIT_UNSTASH_FLAGS($LIBPCRE2DIR)\n+\n+GIT_CONF_SUBST([USE_LIBPCRE2])\n+\n+fi\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.\ndiff --git a/grep.c b/grep.c\nindex ac7d6f9bbf..1797807f9c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -61,9 +61,12 @@ static int parse_pattern_type_arg(const char *opt, const char *arg)\n \telse if (!strcmp(arg, \"fixed\"))\n \t\treturn GREP_PATTERN_TYPE_FIXED;\n \telse if (!strcmp(arg, \"perl\") ||\n-\t\t !strcmp(arg, \"pcre\") ||\n-\t\t !strcmp(arg, \"pcre1\"))\n+\t\t !strcmp(arg, \"pcre\"))\n+\t\treturn GREP_PATTERN_TYPE_PCRE;\n+\telse if (!strcmp(arg, \"pcre1\"))\n \t\treturn GREP_PATTERN_TYPE_PCRE1;\n+\telse if (!strcmp(arg, \"pcre2\"))\n+\t\treturn GREP_PATTERN_TYPE_PCRE2;\n \tdie(\"bad %s argument: %s\", opt, arg);\n }\n \n@@ -181,24 +184,48 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \tcase GREP_PATTERN_TYPE_BRE:\n \t\topt->fixed = 0;\n \t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 0;\n \t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n \t\topt->fixed = 0;\n \t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 0;\n \t\topt->regflags |= REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\topt->fixed = 1;\n \t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 0;\n \t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \n+\tcase GREP_PATTERN_TYPE_PCRE:\n+\t\topt->fixed = 0;\n+\t\t/* We default to pcre1 in the prescience of both\n+\t\t * versions. This may change in future versions.\n+\t\t */\n+#ifdef USE_LIBPCRE1\n+\t\topt->pcre1 = 1;\n+\t\topt->pcre2 = 0;\n+#elif USE_LIBPCRE2\n+\t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 1;\n+#endif\n+\t\tbreak;\n+\n \tcase GREP_PATTERN_TYPE_PCRE1:\n \t\topt->fixed = 0;\n \t\topt->pcre1 = 1;\n+\t\topt->pcre2 = 0;\n+\t\tbreak;\n+\n+\tcase GREP_PATTERN_TYPE_PCRE2:\n+\t\topt->fixed = 0;\n+\t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 1;\n \t\tbreak;\n \t}\n }\n@@ -394,6 +421,93 @@ static void free_pcre1_regexp(struct grep_pat *p)\n }\n #endif /* !USE_LIBPCRE1 */\n \n+#ifdef USE_LIBPCRE2\n+static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n+{\n+\tint error;\n+\tPCRE2_UCHAR errbuf[256];\n+\tPCRE2_SIZE erroffset;\n+\tint options = PCRE2_MULTILINE;\n+\tconst uint8_t *character_tables = NULL;\n+\tuint32_t canjit;\n+\tint jitret;\n+\n+\tp->pcre2_ccontext = NULL;\n+\n+\tif (opt->ignore_case) {\n+\t\tif (has_non_ascii(p->pattern)) {\n+\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tp->pcre2_ccontext = pcre2_compile_context_create(NULL);\n+\t\t\tpcre2_set_character_tables(p->pcre2_ccontext, character_tables);\n+\t\t}\n+\t\toptions |= PCRE2_CASELESS;\n+\t}\n+\tif (is_utf8_locale() && has_non_ascii(p->pattern))\n+\t\toptions |= PCRE2_UTF;\n+\n+\tp->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n+\t\t\t\t\t p->patternlen, options, &error, &erroffset,\n+\t\t\t\t\t p->pcre2_ccontext);\n+\tif (!p->pcre2_pattern) {\n+\t\tpcre2_get_error_message(error, errbuf, sizeof(errbuf));\n+\t\tcompile_regexp_failed(p, (const char *)&errbuf);\n+\t}\n+\n+\tpcre2_config(PCRE2_CONFIG_JIT, &canjit);\n+\tif (canjit == 1) {\n+\t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n+\t\tif (jitret)\n+\t\t\tdie(\"BUG: Couldn't JIT the PCRE pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t}\n+}\n+\n+static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n+\t\tregmatch_t *match, int eflags)\n+{\n+\tint ret, flags = 0;\n+\tPCRE2_SIZE *ovector;\n+\n+\tif (eflags & REG_NOTBOL)\n+\t\tflags |= PCRE2_NOTBOL;\n+\n+\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\tret = pcre2_match(p->pcre2_pattern, (unsigned char *)line, eol - line,\n+\t\t\t 0, flags, p->pcre2_match_data, NULL);\n+\tif (ret < 0 && ret != PCRE2_ERROR_NOMATCH)\n+\t\tdie(\"pcre_exec failed with error code %d\", ret);\n+\tif (ret > 0) {\n+\t\tovector = pcre2_get_ovector_pointer(p->pcre2_match_data);\n+\t\tret = 0;\n+\t\tmatch->rm_so = (int)ovector[0];\n+\t\tmatch->rm_eo = (int)ovector[1];\n+\t}\n+\n+\treturn ret;\n+}\n+\n+static void free_pcre2_pattern(struct grep_pat *p)\n+{\n+\tpcre2_code_free(p->pcre2_pattern);\n+\tpcre2_match_data_free(p->pcre2_match_data);\n+\tpcre2_compile_context_free(p->pcre2_ccontext);\n+}\n+#else /* !USE_LIBPCRE2 */\n+static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n+{\n+\tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE2\");\n+}\n+\n+static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n+\t\tregmatch_t *match, int eflags)\n+{\n+\treturn 1;\n+}\n+\n+static void free_pcre2_pattern(struct grep_pat *p)\n+{\n+}\n+#endif /* !USE_LIBPCRE2 */\n+\n static int is_fixed(const char *s, size_t len)\n {\n \tsize_t i;\n@@ -481,6 +595,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t\treturn;\n \t}\n \n+\tif (opt->pcre2) {\n+\t\tcompile_pcre2_pattern(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@@ -915,6 +1034,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \t\thit = !fixmatch(p, line, eol, match);\n \telse if (p->pcre1_regexp)\n \t\thit = !pcre1match(p, line, eol, match, eflags);\n+\telse if (p->pcre2_pattern)\n+\t\thit = !pcre2match(p, line, eol, match, eflags);\n \telse\n \t\thit = !regexec_buf(&p->regexp, line, eol - line, 1, match,\n \t\t\t\t eflags);\ndiff --git a/grep.h b/grep.h\nindex fa2ab9485f..5c7f4dc799 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -7,6 +7,14 @@\n typedef int pcre;\n typedef int pcre_extra;\n #endif\n+#ifdef USE_LIBPCRE2\n+#define PCRE2_CODE_UNIT_WIDTH 8\n+#include \n+#else\n+typedef int pcre2_code;\n+typedef int pcre2_match_data;\n+typedef int pcre2_compile_context;\n+#endif\n #include \"kwset.h\"\n #include \"thread-utils.h\"\n #include \"userdiff.h\"\n@@ -49,6 +57,9 @@ struct grep_pat {\n \tpcre *pcre1_regexp;\n \tpcre_extra *pcre1_extra_info;\n \tconst unsigned char *pcre1_tables;\n+\tpcre2_code *pcre2_pattern;\n+\tpcre2_match_data *pcre2_match_data;\n+\tpcre2_compile_context *pcre2_ccontext;\n \tkwset_t kws;\n \tunsigned fixed:1;\n \tunsigned ignore_case:1;\n@@ -68,7 +79,9 @@ enum grep_pattern_type {\n \tGREP_PATTERN_TYPE_BRE,\n \tGREP_PATTERN_TYPE_ERE,\n \tGREP_PATTERN_TYPE_FIXED,\n-\tGREP_PATTERN_TYPE_PCRE1\n+\tGREP_PATTERN_TYPE_PCRE,\n+\tGREP_PATTERN_TYPE_PCRE1,\n+\tGREP_PATTERN_TYPE_PCRE2\n };\n \n struct grep_expr {\n@@ -112,6 +125,7 @@ struct grep_opt {\n \tint extended;\n \tint use_reflog_filter;\n \tint pcre1;\n+\tint pcre2;\n \tint relative;\n \tint pathname;\n \tint null_following_name;\ndiff --git a/revision.c b/revision.c\nindex 7a10a8570a..03a3a012de 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1996,7 +1996,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n \t} else if (!strcmp(arg, \"--perl-regexp\") || !strcmp(arg, \"-P\")) {\n-\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE1;\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;\n \t} else if (!strcmp(arg, \"--all-match\")) {\n \t\trevs->grep_filter.all_match = 1;\n \t} else if (!strcmp(arg, \"--invert-grep\")) {\ndiff --git a/t/README b/t/README\nindex a90cb62583..547b06e700 100644\n--- a/t/README\n+++ b/t/README\n@@ -808,6 +808,18 @@ use these, and \"test_set_prereq\" for how to define your own.\n Git was compiled with support for PCRE. Wrap any tests\n that use git-grep --perl-regexp or git-grep -P in these.\n \n+ - LIBPCRE1\n+\n+ Git was compiled with PCRE v1 support via\n+ USE_LIBPCRE=YesPlease. Wrap any PCRE using tests that for some\n+ reason need v1 of the PCRE library instead of v2 in these.\n+\n+ - LIBPCRE2\n+\n+ Git was compiled with PCRE v2 support via\n+ USE_LIBPCRE2=YesPlease. Wrap any PCRE using tests that for some\n+ reason need v2 of the PCRE library instead of v1 in these.\n+\n - CASE_INSENSITIVE_FS\n \n Test is run on a case insensitive file system.\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 46f528183d..6e9e38cec9 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1063,6 +1063,16 @@ test_expect_success PCRE 'grep -P pattern' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success LIBPCRE1 'grep libpcre1 pattern' '\n+\tgit -c grep.patternType=pcre1 grep \"\\p{Ps}.*?\\p{Pe}\" hello.c >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success LIBPCRE2 'grep libpcre2 pattern' '\n+\tgit -c grep.patternType=pcre2 grep \"\\p{Ps}.*?\\p{Pe}\" hello.c >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'grep pattern with grep.extendedRegexp=true' '\n \t>empty &&\n \ttest_must_fail git -c grep.extendedregexp=true \\\n@@ -1522,14 +1532,28 @@ test_expect_success 'grep with thread options' '\n \ttest_must_fail git -c grep.threads=1 grep --threads=-1 st.*dio\n '\n \n-test_expect_success PCRE \"grep with grep.patternType synonyms perl/pcre/pcre1\" '\n+test_expect_success PCRE \"grep with grep.patternType synonyms perl/pcre\" '\n \techo \"#include \" >expected &&\n \tgit -c grep.patternType=perl grep -h --no-line-number \"st(?=dio)\" >actual &&\n \ttest_cmp expected actual &&\n \tgit -c grep.patternType=pcre grep -h --no-line-number \"st(?=dio)\" >actual &&\n-\ttest_cmp expected actual &&\n-\tgit -c grep.patternType=pcre1 grep -h --no-line-number \"st(?=dio)\" >actual &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success LIBPCRE1 \"grep with grep.patternType=pcre1\" '\n+\techo \"#include \" >expected &&\n+\tgit -c grep.patternType=pcre1 grep -h --no-line-number \"st(?=dio)\" >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_must_fail git -c grep.patternType=pcre1 grep \"foo(?+bar)\" 2>error &&\n+\ttest_i18ngrep -q \"digit expected after\" error\n+'\n+\n+test_expect_success LIBPCRE2 \"grep with grep.patternType=pcre2\" '\n+\techo \"#include \" >expected &&\n+\tgit -c grep.patternType=pcre2 grep -h --no-line-number \"st(?=dio)\" >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_must_fail git -c grep.patternType=pcre2 grep \"foo(?+bar)\" 2>error &&\n+\ttest_i18ngrep -q \"digit expected after\" error\n+'\n+\n test_done\ndiff --git a/t/t7813-grep-icase-iso.sh b/t/t7813-grep-icase-iso.sh\nindex 701e08a8e5..e16570690c 100755\n--- a/t/t7813-grep-icase-iso.sh\n+++ b/t/t7813-grep-icase-iso.sh\n@@ -11,9 +11,12 @@ test_expect_success GETTEXT_ISO_LOCALE 'setup' '\n \texport LC_ALL\n '\n \n-test_expect_success GETTEXT_ISO_LOCALE,PCRE 'grep pcre string' '\n-\tgit grep --perl-regexp -i \"TILRAUN: H.ll� Heimur!\" &&\n-\tgit grep --perl-regexp -i \"TILRAUN: H.LL� HEIMUR!\"\n-'\n+for pcrev in 1 2\n+do\n+\ttest_expect_success GETTEXT_ISO_LOCALE,LIBPCRE$pcrev \"grep -i with i18n string using libpcre$pcrev\" \"\n+\t\tgit -c grep.patternType=pcre$pcrev grep -i \\\"TILRAUN: H.ll� Heimur!\\\" &&\n+\t\tgit -c grep.patternType=pcre$pcrev grep -i \\\"TILRAUN: H.LL� HEIMUR!\\\"\n+\t\"\n+done\n \n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e5cfbcc36b..6f873de3e7 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1010,7 +1010,9 @@ esac\n ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1\n test -z \"$NO_PERL\" && test_set_prereq PERL\n test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n-test -n \"$USE_LIBPCRE1\" && test_set_prereq PCRE\n+test -n \"$USE_LIBPCRE1$USE_LIBPCRE2\" && test_set_prereq PCRE\n+test -n \"$USE_LIBPCRE1\" && test_set_prereq LIBPCRE1\n+test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-- \n2.11.0\n\n"},{"id":"316410","messageId":"20170408132506.5415-12-avarab@gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"[PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T13:25:05Z","receivedAt":"2017-04-08T13:26:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the internal PCRE variable & function names to have a \"1\"\nsuffix. This is for preparation for libpcre2 support, where having\nnon-versioned names would be confusing.\n\nThe earlier \"grep: change the internal PCRE macro names to be PCRE1\"\nchange elaborates on the motivations behind this commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 4 ++--\n grep.c | 56 ++++++++++++++++++++++++++++----------------------------\n grep.h | 10 +++++-----\n revision.c | 2 +-\n 4 files changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9478ab5dff..dffb9743b8 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\targv_array_push(&submodule_options, \"-F\");\n \t\tbreak;\n-\tcase GREP_PATTERN_TYPE_PCRE:\n+\tcase GREP_PATTERN_TYPE_PCRE1:\n \t\targv_array_push(&submodule_options, \"-P\");\n \t\tbreak;\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n@@ -1036,7 +1036,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t GREP_PATTERN_TYPE_FIXED),\n \t\tOPT_SET_INT('P', \"perl-regexp\", &pattern_type_arg,\n \t\t\t N_(\"use Perl-compatible regular expressions\"),\n-\t\t\t GREP_PATTERN_TYPE_PCRE),\n+\t\t\t GREP_PATTERN_TYPE_PCRE1),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOL('n', \"line-number\", &opt.linenum, N_(\"show line numbers\")),\n \t\tOPT_NEGBIT('h', NULL, &opt.pathname, N_(\"don't show filenames\"), 1),\ndiff --git a/grep.c b/grep.c\nindex 99b9e9447f..ac7d6f9bbf 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -63,7 +63,7 @@ static int parse_pattern_type_arg(const char *opt, const char *arg)\n \telse if (!strcmp(arg, \"perl\") ||\n \t\t !strcmp(arg, \"pcre\") ||\n \t\t !strcmp(arg, \"pcre1\"))\n-\t\treturn GREP_PATTERN_TYPE_PCRE;\n+\t\treturn GREP_PATTERN_TYPE_PCRE1;\n \tdie(\"bad %s argument: %s\", opt, arg);\n }\n \n@@ -180,25 +180,25 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \n \tcase GREP_PATTERN_TYPE_BRE:\n \t\topt->fixed = 0;\n-\t\topt->pcre = 0;\n+\t\topt->pcre1 = 0;\n \t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n \t\topt->fixed = 0;\n-\t\topt->pcre = 0;\n+\t\topt->pcre1 = 0;\n \t\topt->regflags |= REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\topt->fixed = 1;\n-\t\topt->pcre = 0;\n+\t\topt->pcre1 = 0;\n \t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \n-\tcase GREP_PATTERN_TYPE_PCRE:\n+\tcase GREP_PATTERN_TYPE_PCRE1:\n \t\topt->fixed = 0;\n-\t\topt->pcre = 1;\n+\t\topt->pcre1 = 1;\n \t\tbreak;\n \t}\n }\n@@ -326,7 +326,7 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p,\n }\n \n #ifdef USE_LIBPCRE1\n-static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n+static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tconst char *error;\n \tint erroffset;\n@@ -334,23 +334,23 @@ static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern))\n-\t\t\tp->pcre_tables = pcre_maketables();\n+\t\t\tp->pcre1_tables = pcre_maketables();\n \t\toptions |= PCRE_CASELESS;\n \t}\n \tif (is_utf8_locale() && has_non_ascii(p->pattern))\n \t\toptions |= PCRE_UTF8;\n \n-\tp->pcre_regexp = pcre_compile(p->pattern, options, &error, &erroffset,\n-\t\t\t\t p->pcre_tables);\n-\tif (!p->pcre_regexp)\n+\tp->pcre1_regexp = pcre_compile(p->pattern, options, &error, &erroffset,\n+\t\t\t\t p->pcre1_tables);\n+\tif (!p->pcre1_regexp)\n \t\tcompile_regexp_failed(p, error);\n \n-\tp->pcre_extra_info = pcre_study(p->pcre_regexp, 0, &error);\n-\tif (!p->pcre_extra_info && error)\n+\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, 0, &error);\n+\tif (!p->pcre1_extra_info && error)\n \t\tdie(\"%s\", error);\n }\n \n-static int pcrematch(struct grep_pat *p, const char *line, const char *eol,\n+static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n \tint ovector[30], ret, flags = 0;\n@@ -358,7 +358,7 @@ static int pcrematch(struct grep_pat *p, const char *line, const char *eol,\n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\n \n-\tret = pcre_exec(p->pcre_regexp, p->pcre_extra_info, line, eol - line,\n+\tret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,\n \t\t\t0, flags, ovector, ARRAY_SIZE(ovector));\n \tif (ret < 0 && ret != PCRE_ERROR_NOMATCH)\n \t\tdie(\"pcre_exec failed with error code %d\", ret);\n@@ -371,25 +371,25 @@ static int pcrematch(struct grep_pat *p, const char *line, const char *eol,\n \treturn ret;\n }\n \n-static void free_pcre_regexp(struct grep_pat *p)\n+static void free_pcre1_regexp(struct grep_pat *p)\n {\n-\tpcre_free(p->pcre_regexp);\n-\tpcre_free(p->pcre_extra_info);\n-\tpcre_free((void *)p->pcre_tables);\n+\tpcre_free(p->pcre1_regexp);\n+\tpcre_free(p->pcre1_extra_info);\n+\tpcre_free((void *)p->pcre1_tables);\n }\n #else /* !USE_LIBPCRE1 */\n-static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n+static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\n }\n \n-static int pcrematch(struct grep_pat *p, const char *line, const char *eol,\n+static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n \treturn 1;\n }\n \n-static void free_pcre_regexp(struct grep_pat *p)\n+static void free_pcre1_regexp(struct grep_pat *p)\n {\n }\n #endif /* !USE_LIBPCRE1 */\n@@ -476,8 +476,8 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t\treturn;\n \t}\n \n-\tif (opt->pcre) {\n-\t\tcompile_pcre_regexp(p, opt);\n+\tif (opt->pcre1) {\n+\t\tcompile_pcre1_regexp(p, opt);\n \t\treturn;\n \t}\n \n@@ -833,8 +833,8 @@ void free_grep_patterns(struct grep_opt *opt)\n \t\tcase GREP_PATTERN_BODY:\n \t\t\tif (p->kws)\n \t\t\t\tkwsfree(p->kws);\n-\t\t\telse if (p->pcre_regexp)\n-\t\t\t\tfree_pcre_regexp(p);\n+\t\t\telse if (p->pcre1_regexp)\n+\t\t\t\tfree_pcre1_regexp(p);\n \t\t\telse\n \t\t\t\tregfree(&p->regexp);\n \t\t\tfree(p->pattern);\n@@ -913,8 +913,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 if (p->pcre1_regexp)\n+\t\thit = !pcre1match(p, line, eol, match, eflags);\n \telse\n \t\thit = !regexec_buf(&p->regexp, line, eol - line, 1, match,\n \t\t\t\t eflags);\ndiff --git a/grep.h b/grep.h\nindex 073b0e4c92..fa2ab9485f 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -46,9 +46,9 @@ struct grep_pat {\n \tsize_t patternlen;\n \tenum grep_header_field field;\n \tregex_t regexp;\n-\tpcre *pcre_regexp;\n-\tpcre_extra *pcre_extra_info;\n-\tconst unsigned char *pcre_tables;\n+\tpcre *pcre1_regexp;\n+\tpcre_extra *pcre1_extra_info;\n+\tconst unsigned char *pcre1_tables;\n \tkwset_t kws;\n \tunsigned fixed:1;\n \tunsigned ignore_case:1;\n@@ -68,7 +68,7 @@ enum grep_pattern_type {\n \tGREP_PATTERN_TYPE_BRE,\n \tGREP_PATTERN_TYPE_ERE,\n \tGREP_PATTERN_TYPE_FIXED,\n-\tGREP_PATTERN_TYPE_PCRE\n+\tGREP_PATTERN_TYPE_PCRE1\n };\n \n struct grep_expr {\n@@ -111,7 +111,7 @@ struct grep_opt {\n \tint allow_textconv;\n \tint extended;\n \tint use_reflog_filter;\n-\tint pcre;\n+\tint pcre1;\n \tint relative;\n \tint pathname;\n \tint null_following_name;\ndiff --git a/revision.c b/revision.c\nindex 03a3a012de..7a10a8570a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1996,7 +1996,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n \t} else if (!strcmp(arg, \"--perl-regexp\") || !strcmp(arg, \"-P\")) {\n-\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE1;\n \t} else if (!strcmp(arg, \"--all-match\")) {\n \t\trevs->grep_filter.all_match = 1;\n \t} else if (!strcmp(arg, \"--invert-grep\")) {\n-- \n2.11.0\n\n"},{"id":"316455","messageId":"CAPc5daV2mFs90AaKv9HnhPfwZ4_dyt5cFvb8ZJJQmXjto+0AbQ@mail.gmail.com","threadId":"45636","inReplyTo":"20170408132506.5415-7-avarab@gmail.com","subject":"Re: [PATCH 06/12] log: add -P as a synonym for --perl-regexp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-10T02:39:02Z","receivedAt":"2017-04-10T02:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sat, Apr 8, 2017 at 10:25 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Add a short -P option as a synonym for the longer --perl-regexp, for\n> consistency with the options the corresponding grep invocations\n> accept.\n>\n> This was intentionally omitted in commit 727b6fc3ed (\"log --grep:\n> accept --basic-regexp and --perl-regexp\", 2012-10-03) for unspecified\n> future use.\n>\n> Since nothing has come along in over 4 1/2 years that's wanted to use\n> it, it's more valuable to make it consistent with \"grep\" than to keep\n> it open for future use, and to avoid the confusion of -P meaning\n> different things for grep & log, as is the case with the -G option.\n\nI initially had a strong reaction to the above \"4 1/2 years entitles us to\ndo anything that might inconvenience future developers\" reasoning, but\nas long as we already have -E for extended regexp usable, even if we\nwill never be able to use -G for basic regexp, I am OK with giving -P for\npcre to be consistent with \"git grep\". I'd be even supportive if others\nagree with this change.\n\nThanks.\n"},{"id":"316563","messageId":"20170411100656.5bptxdaptc4zznan@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-2-avarab@gmail.com","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:06:57Z","receivedAt":"2017-04-11T10:07:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:24:55PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Add the ability to entirely disable threading by having grep.threads=0\n> in the config or --threads=0 on the command-line.\n\nIn pack-objects and index-pack, --threads=0 means \"auto-detect\". It\nseems like we should try to keep this consistent.\n\nWouldn't --threads=1 be a better way to disable threading? Pack-objects\nis smart enough to just use the non-threaded code path entirely in that\ncase (rather than wasting time spawning a single worker thread). Grep\nshould probably do the same.\n\n> +static int thread_callback(const struct option *opt,\n> +\t\t\t const char *arg, int unset)\n> +{\n> +\tint *threads = (int*)opt->value;\n> +\tchar *end;\n> +\n> +\tif (unset) {\n> +\t\t*threads = GREP_NUM_THREADS_DEFAULT;\n> +\t\treturn 0;\n> +\t}\n\nThis means that \"--no-threads\" will use 8 threads. Which _kind of_ makes\nsense in that it cancels any previous \"--threads\", but I wonder if it\nshould be the same as \"--threads=1\".\n\nThis isn't really a change in behavior from the existing code, though\n(OPT_INTEGER will set it to 0 in that case, too, and we'd later pick up\nthe default value).\n\n-Peff\n"},{"id":"316565","messageId":"20170411101010.fl437hkvidkkx54w@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-3-avarab@gmail.com","subject":"Re: [PATCH 02/12] grep: remove redundant regflags assignment under PCRE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:10:10Z","receivedAt":"2017-04-11T10:11:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:24:56PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Remove a redundant assignment to the \"regflags\" variable. This\n> variable is only used for POSIX regular expression matching, not when\n> the PCRE library is used.\n> \n> This redundant assignment was added as a result of copy/paste\n> programming in commit 84befcd0a4 (\"grep: add a grep.patternType\n> configuration setting\", 2012-08-03). That commit modified already\n> working code in commit cca2c172e0 (\"git-grep: do not die upon -F/-P\n> when grep.extendedRegexp is set.\", 2011-05-09) which didn't assign to\n> regflags when under PCRE.\n> \n> Revert back to that behavior, more to reduce \"wait this is used under\n> PCRE how?\" confusion when reading the code, than to to save ourselves\n> trivial CPU cycles by removing one assignment.\n\nRight, this makes sense. I wondered if there might be an obscure case\nwhere we set the flag, but I couldn't find one.\n\n-Peff\n"},{"id":"316567","messageId":"20170411101412.jjzdscnirh5yerbn@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-4-avarab@gmail.com","subject":"Re: [PATCH 03/12] Makefile & configure: reword outdated comment about PCRE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:14:13Z","receivedAt":"2017-04-11T10:14:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:24:57PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Reword an outdated comment which suggests that only git-grep can use\n> PCRE.\n\nMakes sense.\n\n> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n> -# able to use Perl-compatible regular expressions.\n> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n> +# commands such as like log, grep offer runtime options to use\n> +# Perl-compatible regular expressions instead of standard or extended\n> +# POSIX regular expressions.\n\ns/such as like log, grep/such as log and grep/ ?\n\n> diff --git a/configure.ac b/configure.ac\n> [...]\n> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n> -# able to use Perl-compatible regular expressions.\n> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n> +# commands such as like log, grep offer runtime options to use\n> +# Perl-compatible regular expressions instead of standard or extended\n> +# POSIX regular expressions.\n\nDitto.\n\n> @@ -499,8 +501,10 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO])\n> GIT_CONF_SUBST([NO_OPENSSL])\n> \n> #\n> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n> -# able to use Perl-compatible regular expressions.\n> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n> +# commands such as like log, grep offer runtime options to use\n> +# Perl-compatible regular expressions instead of standard or extended\n> +# POSIX regular expressions.\n\nAnd again. It's weird that the text appears twice in configure.ac.\nApparently you can use --with-libpcre or USE_LIBPCRE in the environment?\nConfigure is weird.\n\n-Peff\n"},{"id":"316569","messageId":"20170411102333.itfca2aofu5l23bc@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-6-avarab@gmail.com","subject":"Re: [PATCH 05/12] log: add exhaustive tests for pattern style options & config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:23:33Z","receivedAt":"2017-04-11T10:24:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:24:59PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Add exhaustive tests for how the different grep.patternType options &\n> the corresponding command-line options affect git-log.\n> \n> Before this change it was possible to patch revision.c so that the\n> --basic-regexp option was synonymous with --extended-regexp, and\n> --perl-regexp wasn't recognized at all, and still have 100% of the\n> test suite pass.\n\nI thought we _did_ have good coerage here, but I think it is only for\ngrep (via t7810). It makes sense to cover this for \"log\", too.\n\n> The patterns being passed to fixed/basic/extended/PCRE are carefully\n> crafted to return the wrong thing if the grep engine were to pick any\n> other matching method than the one it's told to use.\n\nThis can be tricky since POSIX allows implementations to add arbitrary\nextensions for otherwise invalid syntax.\n\nSee my recent 7675c7bd0 (t7810: avoid assumption about invalid regex\nsyntax, 2017-01-11). In particular:\n\n> +\t\tif test_have_prereq LIBPCRE\n> +\t\tthen\n> +\t\t\tgit -c grep.patternType=perl log --pretty=tformat:%s \\\n> +\t\t\t\t--grep=\"\\((?=1)\" >actual.perl\n> +\t\tfi &&\n\nI'd have to double-check POSIX, but I suspect that it may allow (?=1) to\nwork in an ERE (since it's otherwise bogus to have \"?\" without a prior\nelement to match).\n\n-Peff\n"},{"id":"316570","messageId":"20170411102613.g2twgjkcrshxdszd@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-7-avarab@gmail.com","subject":"Re: [PATCH 06/12] log: add -P as a synonym for --perl-regexp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:26:13Z","receivedAt":"2017-04-11T10:28:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:00PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Add a short -P option as a synonym for the longer --perl-regexp, for\n> consistency with the options the corresponding grep invocations\n> accept.\n> \n> This was intentionally omitted in commit 727b6fc3ed (\"log --grep:\n> accept --basic-regexp and --perl-regexp\", 2012-10-03) for unspecified\n> future use.\n> \n> Since nothing has come along in over 4 1/2 years that's wanted to use\n> it, it's more valuable to make it consistent with \"grep\" than to keep\n> it open for future use, and to avoid the confusion of -P meaning\n> different things for grep & log, as is the case with the -G option.\n\nLike Junio, the \"well, nothing else has come along\" triggered my\ninitial reflexes. But I think the consistency argument you make here is\na good one. The \"-P\" option is tainted whether we do this or not.\n\n-Peff\n"},{"id":"316571","messageId":"20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-9-avarab@gmail.com","subject":"Re: [PATCH 08/12] grep: make grep.patternType=[pcre|pcre1] a synonym for \"perl\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:30:18Z","receivedAt":"2017-04-11T10:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:02PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Make the pattern types \"pcre\" & \"pcre1\" synonyms for long-standing\n> \"perl\" grep.patternType.\n> \n> This change is part of a longer patch series to add pcre2 support to\n> Git. It's nice to be able to performance test PCRE v1 v.s. v2 without\n> having to recompile git, and doing that via grep.patternType makes\n> sense.\n> \n> However, just adding \"pcre2\" when we only have \"perl\" would be\n> confusing, so start by adding a \"pcre\" & \"pcre1\" synonym.\n> \n> In the future \"perl\" and \"pcre\" might be changed to default to \"pcre2\"\n> instead of \"pcre1\", and depending on how Git is compiled the more\n> specific \"pcre1\" or \"pcre2\" pattern types might produce an error.\n\nI think this makes sense.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874d51..5ef12d0694 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1624,6 +1624,15 @@ grep.patternType::\n> \t'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,\n> \t`--fixed-strings`, or `--perl-regexp` option accordingly, while the\n> \tvalue 'default' will return to the default matching behavior.\n> ++\n> +The 'pcre' and 'pcre1' values are synonyms for 'perl'. The other\n> +values starting with 'pcre' are reserved for future use, e.g. if we'd\n> +like to use 'pcre2' for the PCRE v2 library.\n\nDo you want to define this the other way around (that perl and pcre are\nsynonyms for pcre1)?\n\nI know that hey are being added now as synonyms, but from the\nperspective of the user (and this is the user-facing documentation), the\nend-game we want is \"you can pick pcre1 or pcre2, and the others are\nfloating synonyms that may change\".\n\nI think it would be OK to continue to refer to \"perl\" elsewhere. Even\nthough it's a synonym, it's the one we'd expect people to use.\n\n-Peff\n"},{"id":"316572","messageId":"20170411103105.256mhk5hwwrdswzh@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-10-avarab@gmail.com","subject":"Re: [PATCH 09/12] test-lib: rename the LIBPCRE prerequisite to PCRE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:31:05Z","receivedAt":"2017-04-11T10:34:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:03PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Rename the LIBPCRE prerequisite to PCRE. This is for preparation for\n> libpcre2 support, where having just \"LIBPCRE\" would be confusing as it\n> implies v1 of the library.\n> \n> None of these tests are incompatible between versions 1 & 2 of\n> libpcre, it's less confusing to give them a more general name to make\n> it clear that they work on both library versions.\n\nOK. I don't think LIBPCRE is all that confusing, but it doesn't hurt to\nbe on the careful side.\n\n-Peff\n"},{"id":"316573","messageId":"CACBZZX5jGGUToK83e2iugpbSBDpATi7J0BWg9=UY=YC4ZixT+Q@mail.gmail.com","threadId":"45636","inReplyTo":"20170411102333.itfca2aofu5l23bc@sigill.intra.peff.net","subject":"Re: [PATCH 05/12] log: add exhaustive tests for pattern style options & config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T10:32:57Z","receivedAt":"2017-04-11T10:34:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:23 PM, Jeff King wrote:\n> On Sat, Apr 08, 2017 at 01:24:59PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Add exhaustive tests for how the different grep.patternType options &\n>> the corresponding command-line options affect git-log.\n>>\n>> Before this change it was possible to patch revision.c so that the\n>> --basic-regexp option was synonymous with --extended-regexp, and\n>> --perl-regexp wasn't recognized at all, and still have 100% of the\n>> test suite pass.\n>\n> I thought we _did_ have good coerage here, but I think it is only for\n> grep (via t7810). It makes sense to cover this for \"log\", too.\n>\n>> The patterns being passed to fixed/basic/extended/PCRE are carefully\n>> crafted to return the wrong thing if the grep engine were to pick any\n>> other matching method than the one it's told to use.\n>\n> This can be tricky since POSIX allows implementations to add arbitrary\n> extensions for otherwise invalid syntax.\n\nFor POSIX basic v.s. extended I'm relying on (|) not being\nmetacharacters in basic but metachars needing quoting in extended. I\nvery much doubt any regex implementation doesn't conform to that, but\nmaybe some crazy implementation does...\n\n> See my recent 7675c7bd0 (t7810: avoid assumption about invalid regex\n> syntax, 2017-01-11). In particular:\n>\n>> + if test_have_prereq LIBPCRE\n>> + then\n>> + git -c grep.patternType=perl log --pretty=tformat:%s \\\n>> + --grep=\"\\((?=1)\" >actual.perl\n>> + fi &&\n>\n> I'd have to double-check POSIX, but I suspect that it may allow (?=1) to\n> work in an ERE (since it's otherwise bogus to have \"?\" without a prior\n> element to match).\n\nDistinguishing PCRE from the rest is much easier, I'll add some more\nobscure PCRE feature to that which definitely doesn't exist in any\nPOSIX rx library, e.g. (*COMMIT) or something.\n"},{"id":"316574","messageId":"20170411103523.tlub4v65er32ikix@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-11-avarab@gmail.com","subject":"Re: [PATCH 10/12] grep: change the internal PCRE macro names to be PCRE1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:35:24Z","receivedAt":"2017-04-11T10:35:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:04PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the internal USE_LIBPCRE define, & build options flag to use a\n> naming convention ending in PCRE1, without changing the long-standing\n> USE_LIBPCRE Makefile flag which enables this code.\n> \n> This is for preparation for libpcre2 support where having things like\n> USE_LIBPCRE and USE_LIBPCRE2 in any more places than we absolutely\n> need to for backwards compatibility with old Makefile arguments would\n> be confusing.\n> \n> In some ways it would be better to change everything that now uses\n> USE_LIBPCRE to use USE_LIBPCRE1, and to make specifying\n> USE_LIBPCRE (or --with-pcre) an error. This would impose a one-time\n> burden on packagers of git to s/USE_LIBPCRE/USE_LIBPCRE1/ in their\n> build scripts.\n> \n> However I'd like to leave the door open to making\n> USE_LIBPCRE=YesPlease eventually mean USE_LIBPCRE2=YesPlease,\n> i.e. once PCRE v2 is ubiquitous enough that it makes sense to make it\n> the default.\n\nYeah, I think it's nice to keep the build-time knobs compatible. In the\nlong run I assume we'll want to add a USE_LIBPCRE1 flag and USE_LIBPCRE\njust becomes a synonym for it (in fact, we could do that in this\ncommit).\n\nI suspect we won't ever want to make it a synonym for USE_LIBPCRE2.\nUnlike the run-time synonyms, where the expressions themselves are\nbackwards compatible, builders need to care which one they're using.\n\n-Peff\n"},{"id":"316575","messageId":"20170411103746.bth3rof753gbjtjf@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-12-avarab@gmail.com","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:37:46Z","receivedAt":"2017-04-11T10:38:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:05PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9478ab5dff..dffb9743b8 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n> \tcase GREP_PATTERN_TYPE_FIXED:\n> \t\targv_array_push(&submodule_options, \"-F\");\n> \t\tbreak;\n> -\tcase GREP_PATTERN_TYPE_PCRE:\n> +\tcase GREP_PATTERN_TYPE_PCRE1:\n> \t\targv_array_push(&submodule_options, \"-P\");\n> \t\tbreak;\n\nHmm. This isn't a problem yet, but wouldn't this need to pass some\npcre1-specific option instead of just \"-P\"?\n\n-Peff\n"},{"id":"316576","messageId":"CACBZZX5V96Vv5NAYpkQSBWpn-4rgHOatAnL51o4-4DADAF+pEg@mail.gmail.com","threadId":"45636","inReplyTo":"20170411103746.bth3rof753gbjtjf@sigill.intra.peff.net","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T10:45:55Z","receivedAt":"2017-04-11T10:46:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:37 PM, Jeff King wrote:\n> On Sat, Apr 08, 2017 at 01:25:05PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index 9478ab5dff..dffb9743b8 100644\n>> --- a/builtin/grep.c\n>> +++ b/builtin/grep.c\n>> @@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n>> case GREP_PATTERN_TYPE_FIXED:\n>> argv_array_push(&submodule_options, \"-F\");\n>> break;\n>> - case GREP_PATTERN_TYPE_PCRE:\n>> + case GREP_PATTERN_TYPE_PCRE1:\n>> argv_array_push(&submodule_options, \"-P\");\n>> break;\n>\n> Hmm. This isn't a problem yet, but wouldn't this need to pass some\n> pcre1-specific option instead of just \"-P\"?\n\nYes, this is a bug. I'll need to add a git_options along with\nsubmodule_options and pass -c grep.patternType=....\n"},{"id":"316577","messageId":"20170411104345.apqr37jn53p6xwdf@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-13-avarab@gmail.com","subject":"Re: [PATCH 12/12] grep: add support for PCRE v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:43:45Z","receivedAt":"2017-04-11T10:47:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:25:06PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -32,6 +32,14 @@ all::\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 USE_LIBPCRE if you have and want to use libpcre2. Various\n> +# commands such as like log, grep offer runtime options to use\n> +# Perl-compatible regular expressions instead of standard or extended\n> +# POSIX regular expressions.\n\nUSE_LIBPCRE2 here, right?\n\nI'll admit I skimmed a bit on reading this patch, as I don't know the\npcre2 API at all. I didn't see anything that looked obviously wrong,\nthough.\n\n-Peff\n"},{"id":"316578","messageId":"20170411104739.xzhxggpufvwgr3fu@sigill.intra.peff.net","threadId":"45636","inReplyTo":"20170408132506.5415-1-avarab@gmail.com","subject":"Re: [PATCH 00/12] PCREv2 & more","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:47:39Z","receivedAt":"2017-04-11T10:47:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 01:24:54PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> This adds PCRE v2 support, but as I was adding that I kept noticing\n> other related problems to fix. It's all bundled up into the same\n> series because much of it conflicts because it modifies the same test\n> or other code. Notes on each patch below.\n\nOverall, the series looks OK to me.\n\nI'm not sure if it is worth all the complexity to carry pcre1/pcre2 as\nrun-time options. That does make it easier to do back-to-back\ncomparisons, but it makes the code a lot more complicated. In particular\nI'm worried about subtle cases where we pcre1 turns into pcre2 (or vice\nversa) by use of the aliases. That shouldn't matter to a user for\ncorrectness, but it would throw off the benchmarking.\n\nIf we literally just added USE_LIBPCRE2 and built against one or the\nother, then all the complexity would be limited to a few #ifdefs. The\nbig drawback AFAICT is that anybody doing timing tests would have to\nrecompile in between.\n\nSo I dunno. I had hoped libpcre2 would be a hands-down win on timing,\nbut it sounds like there's a little back-and-forth depending on the\ncontext. Is there a ticking clock on pcre1 going away? I suspect it will\nbe around for many years to come, but if they'll continue optimizing\npcre2 but not pcre1, then it may make sense to hitch our wagon to the\none that upstream is actually working on.\n\n-Peff\n"},{"id":"316579","messageId":"20170411104828.skkujde3qrvn4jrt@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX5V96Vv5NAYpkQSBWpn-4rgHOatAnL51o4-4DADAF+pEg@mail.gmail.com","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:48:28Z","receivedAt":"2017-04-11T10:49:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 12:45:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Apr 11, 2017 at 12:37 PM, Jeff King wrote:\n> > On Sat, Apr 08, 2017 at 01:25:05PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> diff --git a/builtin/grep.c b/builtin/grep.c\n> >> index 9478ab5dff..dffb9743b8 100644\n> >> --- a/builtin/grep.c\n> >> +++ b/builtin/grep.c\n> >> @@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n> >> case GREP_PATTERN_TYPE_FIXED:\n> >> argv_array_push(&submodule_options, \"-F\");\n> >> break;\n> >> - case GREP_PATTERN_TYPE_PCRE:\n> >> + case GREP_PATTERN_TYPE_PCRE1:\n> >> argv_array_push(&submodule_options, \"-P\");\n> >> break;\n> >\n> > Hmm. This isn't a problem yet, but wouldn't this need to pass some\n> > pcre1-specific option instead of just \"-P\"?\n> \n> Yes, this is a bug. I'll need to add a git_options along with\n> submodule_options and pass -c grep.patternType=....\n\nMaybe that's an indication we should have --pcre1-regexp and\n--pcre2-regexp, so we don't have to resort to config tweaking.\n\n-Peff\n"},{"id":"316580","messageId":"20170411105130.2ezvvm4kl5wdav3x@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX5jGGUToK83e2iugpbSBDpATi7J0BWg9=UY=YC4ZixT+Q@mail.gmail.com","subject":"Re: [PATCH 05/12] log: add exhaustive tests for pattern style options & config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:51:31Z","receivedAt":"2017-04-11T10:53:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 12:32:57PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > This can be tricky since POSIX allows implementations to add arbitrary\n> > extensions for otherwise invalid syntax.\n> \n> For POSIX basic v.s. extended I'm relying on (|) not being\n> metacharacters in basic but metachars needing quoting in extended. I\n> very much doubt any regex implementation doesn't conform to that, but\n> maybe some crazy implementation does...\n\nYeah, I think BRE vs ERE (vs fixed) is fine, because POSIX specifies the\ndifferences. It's really PCRE, because ERE implementations have a\nsurprising amount of flexibility according to the standard (basically\nanything invalid is listed as \"undefined\" and implementations are\nallowed to do what they please).\n\n> >> + if test_have_prereq LIBPCRE\n> >> + then\n> >> + git -c grep.patternType=perl log --pretty=tformat:%s \\\n> >> + --grep=\"\\((?=1)\" >actual.perl\n> >> + fi &&\n> >\n> > I'd have to double-check POSIX, but I suspect that it may allow (?=1) to\n> > work in an ERE (since it's otherwise bogus to have \"?\" without a prior\n> > element to match).\n> \n> Distinguishing PCRE from the rest is much easier, I'll add some more\n> obscure PCRE feature to that which definitely doesn't exist in any\n> POSIX rx library, e.g. (*COMMIT) or something.\n\nI think (*) would be \"undefined\" by POSIX, and thus allowed for\nextensions (though in practice it's probably fine). The \"[\\d]\" trick was\nthe simplest one I could find that is reliably defined to be different\nbetween ERE and PCRE.\n\n-Peff\n"},{"id":"316581","messageId":"CACBZZX7gmmTm6qU-6sviRQ3znwkOXG6999C6SDD_oJe6pqdTWw@mail.gmail.com","threadId":"45636","inReplyTo":"20170411103523.tlub4v65er32ikix@sigill.intra.peff.net","subject":"Re: [PATCH 10/12] grep: change the internal PCRE macro names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T10:51:09Z","receivedAt":"2017-04-11T10:53:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:35 PM, Jeff King wrote:\n> On Sat, Apr 08, 2017 at 01:25:04PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the internal USE_LIBPCRE define, & build options flag to use a\n>> naming convention ending in PCRE1, without changing the long-standing\n>> USE_LIBPCRE Makefile flag which enables this code.\n>>\n>> This is for preparation for libpcre2 support where having things like\n>> USE_LIBPCRE and USE_LIBPCRE2 in any more places than we absolutely\n>> need to for backwards compatibility with old Makefile arguments would\n>> be confusing.\n>>\n>> In some ways it would be better to change everything that now uses\n>> USE_LIBPCRE to use USE_LIBPCRE1, and to make specifying\n>> USE_LIBPCRE (or --with-pcre) an error. This would impose a one-time\n>> burden on packagers of git to s/USE_LIBPCRE/USE_LIBPCRE1/ in their\n>> build scripts.\n>>\n>> However I'd like to leave the door open to making\n>> USE_LIBPCRE=YesPlease eventually mean USE_LIBPCRE2=YesPlease,\n>> i.e. once PCRE v2 is ubiquitous enough that it makes sense to make it\n>> the default.\n>\n> Yeah, I think it's nice to keep the build-time knobs compatible. In the\n> long run I assume we'll want to add a USE_LIBPCRE1 flag and USE_LIBPCRE\n> just becomes a synonym for it (in fact, we could do that in this\n> commit).\n\nI could just add that. Hints for how to do that without entirely\ncopy/pasting the \"ifdef USE_LIBPCRE\" in the Makefile welcome. The\nMakefile syntax doesn't support some form of \"ifdef X || ifdef Y\"\nAFAICT, so it looks like I'll need to copy those lines...\n\n> I suspect we won't ever want to make it a synonym for USE_LIBPCRE2.\n> Unlike the run-time synonyms, where the expressions themselves are\n> backwards compatible, builders need to care which one they're using.\n\nMaybe, that's a problem for another day, at some point we might want\nto turn PCRE on by default, and then maybe v2 will be prolific enough\nto make that the default...\n"},{"id":"316582","messageId":"20170411105312.5h6gc35lhxvz4ro2@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX7gmmTm6qU-6sviRQ3znwkOXG6999C6SDD_oJe6pqdTWw@mail.gmail.com","subject":"Re: [PATCH 10/12] grep: change the internal PCRE macro names to be PCRE1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T10:53:13Z","receivedAt":"2017-04-11T10:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 12:51:09PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Yeah, I think it's nice to keep the build-time knobs compatible. In the\n> > long run I assume we'll want to add a USE_LIBPCRE1 flag and USE_LIBPCRE\n> > just becomes a synonym for it (in fact, we could do that in this\n> > commit).\n> \n> I could just add that. Hints for how to do that without entirely\n> copy/pasting the \"ifdef USE_LIBPCRE\" in the Makefile welcome. The\n> Makefile syntax doesn't support some form of \"ifdef X || ifdef Y\"\n> AFAICT, so it looks like I'll need to copy those lines...\n\nMaybe:\n\n USE_LIBPCRE1 ?= $(USE_LIBPCRE)\n ifdef USE_LIBPCRE1\n ...\n endif\n\n-Peff\n"},{"id":"316584","messageId":"CACBZZX6A8tcXy-hCcQqqDsGNDtXWmZMdTY=RxeAOn8QFYDypyg@mail.gmail.com","threadId":"45636","inReplyTo":"20170411104828.skkujde3qrvn4jrt@sigill.intra.peff.net","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T11:02:56Z","receivedAt":"2017-04-11T11:03:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:48 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 12:45:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Tue, Apr 11, 2017 at 12:37 PM, Jeff King wrote:\n>> > On Sat, Apr 08, 2017 at 01:25:05PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>> >\n>> >> diff --git a/builtin/grep.c b/builtin/grep.c\n>> >> index 9478ab5dff..dffb9743b8 100644\n>> >> --- a/builtin/grep.c\n>> >> +++ b/builtin/grep.c\n>> >> @@ -490,7 +490,7 @@ static void compile_submodule_options(const struct grep_opt *opt,\n>> >> case GREP_PATTERN_TYPE_FIXED:\n>> >> argv_array_push(&submodule_options, \"-F\");\n>> >> break;\n>> >> - case GREP_PATTERN_TYPE_PCRE:\n>> >> + case GREP_PATTERN_TYPE_PCRE1:\n>> >> argv_array_push(&submodule_options, \"-P\");\n>> >> break;\n>> >\n>> > Hmm. This isn't a problem yet, but wouldn't this need to pass some\n>> > pcre1-specific option instead of just \"-P\"?\n>>\n>> Yes, this is a bug. I'll need to add a git_options along with\n>> submodule_options and pass -c grep.patternType=....\n>\n> Maybe that's an indication we should have --pcre1-regexp and\n> --pcre2-regexp, so we don't have to resort to config tweaking.\n\nI'd rather not. To reply to both your\n<20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net> & this, one\nthing I was trying to do in this series (and I don't think I went far\nenough in \"grep & rev-list doc: stop promising libpcre for\n--perl-regexp\") was to stop promising some specific version of PCRE.\n\nI.e. I think we should have the likes of core.patternType=perl & -P\nfor the user, but whether we implement that with pcre/pcre2, or even\nlibperl itself, or any of the other PCRE reimplementations around\n(there's one for java, one for C#, cpython has one I think...), or\nmaybe even re2 should be an implementation detail.\n\nI.e. as far as the user is concerned they just want perl-y regexes,\nbut they most likely don't care about the 1% featureset of those\nregexes where the various implementations of \"perl-y regex\" actually\ndiffer, because those cases tend to be really obscure syntax.\n\nHaving core.patternType=pcre2 is a neccesary evil for our own\nimplementation & testing, e.g. for the submodule grep it would be\nreally bizarro if some edge case where the implementations differ\ncauses us to produce different grep results, since one side would use\npcre2 & the other pcre1.\n\nBut I don't think we should take it to the level of having documented\n--pcreN-regexp options.\n"},{"id":"316592","messageId":"20170411125741.7b7hlmuucid37b3r@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX6A8tcXy-hCcQqqDsGNDtXWmZMdTY=RxeAOn8QFYDypyg@mail.gmail.com","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T12:57:41Z","receivedAt":"2017-04-11T12:57:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 01:02:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> Yes, this is a bug. I'll need to add a git_options along with\n> >> submodule_options and pass -c grep.patternType=....\n> >\n> > Maybe that's an indication we should have --pcre1-regexp and\n> > --pcre2-regexp, so we don't have to resort to config tweaking.\n> \n> I'd rather not. To reply to both your\n> <20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net> & this, one\n> thing I was trying to do in this series (and I don't think I went far\n> enough in \"grep & rev-list doc: stop promising libpcre for\n> --perl-regexp\") was to stop promising some specific version of PCRE.\n\nWe don't necessarily have to document them. This is just in the general\nrule of \"if there's config, there should be command-line to override\nit\". Because without that, you get this exact situation where you have\nto bolt on \"-c\" options to another part of the command line, which gets\nawkward.\n\nI'm also not sure it would be strictly correct, if the sub-program runs\nother sub-programs. Providing \"-c\" affects all child processes, whereas\ncommand-line options are propagated manually. So imagine you have a\nprocess tree like:\n\n grep\n \\-grep\n \\-textconv\n\nI.e., grep recurses to a submodule which then has to kick off a textconv\nfilter for one of the files. If you use \"-c\" to pass options to the\nsecond grep, then those options will continue to have an effect inside\nthe textconv filter. Which _probably_ doesn't run git commands that\nwould care, but technically it could do anything.\n\n> I.e. as far as the user is concerned they just want perl-y regexes,\n> but they most likely don't care about the 1% featureset of those\n> regexes where the various implementations of \"perl-y regex\" actually\n> differ, because those cases tend to be really obscure syntax.\n\nYeah, that's what led me to the \"why are we even worrying about run-time\nswitching\" direction. I'd think a build-time switch would be enough for\npeople to test, and it makes all of this type of complexity go away.\n\n-Peff\n"},{"id":"316603","messageId":"20170411165127.GC36152@google.com","threadId":"45636","inReplyTo":"20170411125741.7b7hlmuucid37b3r@sigill.intra.peff.net","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-11T16:51:27Z","receivedAt":"2017-04-11T16:51:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 01:02:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > >> Yes, this is a bug. I'll need to add a git_options along with\n> > >> submodule_options and pass -c grep.patternType=....\n> > >\n> > > Maybe that's an indication we should have --pcre1-regexp and\n> > > --pcre2-regexp, so we don't have to resort to config tweaking.\n> > \n> > I'd rather not. To reply to both your\n> > <20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net> & this, one\n> > thing I was trying to do in this series (and I don't think I went far\n> > enough in \"grep & rev-list doc: stop promising libpcre for\n> > --perl-regexp\") was to stop promising some specific version of PCRE.\n> \n> We don't necessarily have to document them. This is just in the general\n> rule of \"if there's config, there should be command-line to override\n> it\". Because without that, you get this exact situation where you have\n> to bolt on \"-c\" options to another part of the command line, which gets\n> awkward.\n> \n> I'm also not sure it would be strictly correct, if the sub-program runs\n> other sub-programs. Providing \"-c\" affects all child processes, whereas\n> command-line options are propagated manually. So imagine you have a\n> process tree like:\n> \n> grep\n> \\-grep\n> \\-textconv\n> \n> I.e., grep recurses to a submodule which then has to kick off a textconv\n> filter for one of the files. If you use \"-c\" to pass options to the\n> second grep, then those options will continue to have an effect inside\n> the textconv filter. Which _probably_ doesn't run git commands that\n> would care, but technically it could do anything.\n> \n> > I.e. as far as the user is concerned they just want perl-y regexes,\n> > but they most likely don't care about the 1% featureset of those\n> > regexes where the various implementations of \"perl-y regex\" actually\n> > differ, because those cases tend to be really obscure syntax.\n> \n> Yeah, that's what led me to the \"why are we even worrying about run-time\n> switching\" direction. I'd think a build-time switch would be enough for\n> people to test, and it makes all of this type of complexity go away.\n\nYeah I agree with Jeff that we should probably avoid needing to pass a\nconfig option down in addition to a command line switch to do perl\nregex's. I didn't take too hard of a look at how that would be done in\nthe grep code, but it might be slightly more involved than just changing\nthe enum name.\n\nFrom [12/12] it looks like the main purpose of this series is to use a\nmore preferment version of PCRE, if all else is equal it doesn't really\nmake much sense to have both versions to be select-able at runtime. Is\nthere any benefit of being able to do that, that I'm missing?\n\n-- \nBrandon Williams\n"},{"id":"316619","messageId":"CACBZZX6HLDmWSGiQ+cJ-p0Ak6SQHcmECaGqsfVz-Js4q7aSEwg@mail.gmail.com","threadId":"45636","inReplyTo":"20170411165127.GC36152@google.com","subject":"Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T18:31:07Z","receivedAt":"2017-04-11T18:31:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 6:51 PM, Brandon Williams wrote:\n> On 04/11, Jeff King wrote:\n>> On Tue, Apr 11, 2017 at 01:02:56PM +0200, Ęvar Arnfjörš Bjarmason wrote:\n>>\n>> > >> Yes, this is a bug. I'll need to add a git_options along with\n>> > >> submodule_options and pass -c grep.patternType=....\n>> > >\n>> > > Maybe that's an indication we should have --pcre1-regexp and\n>> > > --pcre2-regexp, so we don't have to resort to config tweaking.\n>> >\n>> > I'd rather not. To reply to both your\n>> > <20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net> & this, one\n>> > thing I was trying to do in this series (and I don't think I went far\n>> > enough in \"grep & rev-list doc: stop promising libpcre for\n>> > --perl-regexp\") was to stop promising some specific version of PCRE.\n>>\n>> We don't necessarily have to document them. This is just in the general\n>> rule of \"if there's config, there should be command-line to override\n>> it\". Because without that, you get this exact situation where you have\n>> to bolt on \"-c\" options to another part of the command line, which gets\n>> awkward.\n>>\n>> I'm also not sure it would be strictly correct, if the sub-program runs\n>> other sub-programs. Providing \"-c\" affects all child processes, whereas\n>> command-line options are propagated manually. So imagine you have a\n>> process tree like:\n>>\n>> grep\n>> \\-grep\n>> \\-textconv\n>>\n>> I.e., grep recurses to a submodule which then has to kick off a textconv\n>> filter for one of the files. If you use \"-c\" to pass options to the\n>> second grep, then those options will continue to have an effect inside\n>> the textconv filter. Which _probably_ doesn't run git commands that\n>> would care, but technically it could do anything.\n>>\n>> > I.e. as far as the user is concerned they just want perl-y regexes,\n>> > but they most likely don't care about the 1% featureset of those\n>> > regexes where the various implementations of \"perl-y regex\" actually\n>> > differ, because those cases tend to be really obscure syntax.\n>>\n>> Yeah, that's what led me to the \"why are we even worrying about run-time\n>> switching\" direction. I'd think a build-time switch would be enough for\n>> people to test, and it makes all of this type of complexity go away.\n>\n> Yeah I agree with Jeff that we should probably avoid needing to pass a\n> config option down in addition to a command line switch to do perl\n> regex's. I didn't take too hard of a look at how that would be done in\n> the grep code, but it might be slightly more involved than just changing\n> the enum name.\n>\n> From [12/12] it looks like the main purpose of this series is to use a\n> more preferment version of PCRE, if all else is equal it doesn't really\n> make much sense to have both versions to be select-able at runtime. Is\n> there any benefit of being able to do that, that I'm missing?\n\nNot really no. I don't think any git user is ever going to be using\nboth pcre1 & pcre2 at runtime.\n\nThis grew more organically out of how I started to hack the code. Due\nto how different the two APIs are it's much less messier to have a new\nset of wrapper functions than to ifdef around v1 & v2. Once I had all\nthe code & config flags it was easy to shimmy it up so I could switch\nbetween the two, and it was handy for performance testing.\n\nBut the guy developing it is hardly the main target audience for a\nfeature like this, but on the other hand it's easy to support...\n\nI'm hacking up a v2 of this series. It includes some extra goodies\nlike bugfixes, v1 JIT support, and I'll try to tack a patch at the end\nthat removes this facility to switch between the two at runtime, and\nwe can see if that looks better than not having it.\n"},{"id":"316638","messageId":"CACBZZX7vEQ5jUzX3GsD6JXe50TnRUtGmSVi7zBxwOmAQGABQ4Q@mail.gmail.com","threadId":"45636","inReplyTo":"20170411100656.5bptxdaptc4zznan@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T20:20:59Z","receivedAt":"2017-04-11T20:21:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:06 PM, Jeff King wrote:\n> On Sat, Apr 08, 2017 at 01:24:55PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Add the ability to entirely disable threading by having grep.threads=0\n>> in the config or --threads=0 on the command-line.\n>\n> In pack-objects and index-pack, --threads=0 means \"auto-detect\". It\n> seems like we should try to keep this consistent.\n>\n> Wouldn't --threads=1 be a better way to disable threading? Pack-objects\n> is smart enough to just use the non-threaded code path entirely in that\n> case (rather than wasting time spawning a single worker thread). Grep\n> should probably do the same.\n\nI'm struggling to find a use-case where threading makes sense at all.\nThe example in the initial introduction in 5b594f457a is always slower\nwith >0 for me, and since then in 0579f91dd7 it got disabled entirely\nfor non-worktree cases.\n\nBut assuming it works for someone out there, then 0 threads is clearly\nnot the same as 1. On linux.git with pcre2 grepping for [q]werty for\nexample[1]\n\n Rate P_12 P_8 P_4 P_1 P_2 P_0\nP_12 0.861/s -- -7% -22% -27% -30% -43%\nP_8 0.924/s 7% -- -16% -22% -25% -39%\nP_4 1.10/s 28% 19% -- -7% -10% -27%\nP_1 1.19/s 38% 29% 8% -- -3% -21%\nP_2 1.23/s 43% 33% 12% 4% -- -18%\nP_0 1.51/s 75% 63% 37% 27% 22% --\n\nAnd for [a]var on git.git:\n\n Rate P_12 P_8 P_4 P_2 P_1 P_0\nP_12 15.6/s -- -5% -15% -17% -21% -42%\nP_8 16.4/s 5% -- -11% -12% -17% -39%\nP_4 18.4/s 18% 13% -- -1% -7% -32%\nP_2 18.7/s 20% 14% 1% -- -6% -31%\nP_1 19.8/s 27% 21% 8% 6% -- -27%\nP_0 27.0/s 73% 65% 46% 44% 36% --\n\nTthere's a >20% performance difference between 0 and 1 threads. The\nmore threads I add the slower it gets, but if there's some case where\nwe have the inverse of that and you have e.g. 2 cores, then presumably\nyou really want 1 thread, and not 0, or 2.\n\n>> +static int thread_callback(const struct option *opt,\n>> + const char *arg, int unset)\n>> +{\n>> + int *threads = (int*)opt->value;\n>> + char *end;\n>> +\n>> + if (unset) {\n>> + *threads = GREP_NUM_THREADS_DEFAULT;\n>> + return 0;\n>> + }\n>\n> This means that \"--no-threads\" will use 8 threads. Which _kind of_ makes\n> sense in that it cancels any previous \"--threads\", but I wonder if it\n> should be the same as \"--threads=1\".\n\nDear lazyweb, how do you distinguish --no-foo from no --foo=X being\nprovided in this API?\n\n> This isn't really a change in behavior from the existing code, though\n> (OPT_INTEGER will set it to 0 in that case, too, and we'd later pick up\n> the default value).\n\n1. =~/g/git/ perl -MBenchmark=cmpthese -wE 'cmpthese(10, { map { my $t\n= $_; +(\"P_$t\" => sub { system \"$ENV{PF}git -c grep.patternType=pcre2\ngrep --threads=$t [q]werty >/dev/null\" }) } 0,1,2,4,8,12 })'\n"},{"id":"316642","messageId":"20170411203434.iiupo2oovzviqju5@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX7vEQ5jUzX3GsD6JXe50TnRUtGmSVi7zBxwOmAQGABQ4Q@mail.gmail.com","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T20:34:34Z","receivedAt":"2017-04-11T20:34:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 10:20:59PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I'm struggling to find a use-case where threading makes sense at all.\n> The example in the initial introduction in 5b594f457a is always slower\n> with >0 for me, and since then in 0579f91dd7 it got disabled entirely\n> for non-worktree cases.\n\nIt's a big win for me in worktree greps of linux.git:\n\n $ best-of-five git grep --threads=1 '[q]werty'\n Attempt 1: 0.713\n Attempt 2: 0.708\n Attempt 3: 0.689\n Attempt 4: 0.695\n Attempt 5: 0.7\n\n real\t0m0.689s\n user\t0m0.560s\n sys\t0m0.248s\n\n $ best-of-five git grep --threads=8 '[q]werty'\n Attempt 1: 0.238\n Attempt 2: 0.225\n Attempt 3: 0.222\n Attempt 4: 0.221\n Attempt 5: 0.225\n\n real\t0m0.221s\n user\t0m0.936s\n sys\t0m0.356s\n\nIn non-worktree cases most of the time goes to accessing objects, which\nhappens under a lock. So you don't get any real parallelism, just\noverhead.\n\n> But assuming it works for someone out there, then 0 threads is clearly\n> not the same as 1. On linux.git with pcre2 grepping for [q]werty for\n> example[1]\n\nRight, my suggestion was to teach \"grep\" to treat --threads=1 as \"do not\nspawn any other threads\". I.e., to make it like the \"0\" case you were\nproposing, and then leave \"0\" as \"auto-detect\". There would be no way to\nspawn a _single_ thread and feed it. But why would you want to do that?\nIt's always going to be strictly worse than not threading at all.\n\n-Peff\n"},{"id":"316646","messageId":"CACBZZX7Xi2OWqHQd7jTGBEZyqcWk59oXbPJOjuYrYAFzd5huCA@mail.gmail.com","threadId":"45636","inReplyTo":"20170411203434.iiupo2oovzviqju5@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T20:56:01Z","receivedAt":"2017-04-11T20:56:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 10:34 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 10:20:59PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I'm struggling to find a use-case where threading makes sense at all.\n>> The example in the initial introduction in 5b594f457a is always slower\n>> with >0 for me, and since then in 0579f91dd7 it got disabled entirely\n>> for non-worktree cases.\n>\n> It's a big win for me in worktree greps of linux.git:\n>\n> $ best-of-five git grep --threads=1 '[q]werty'\n> Attempt 1: 0.713\n> Attempt 2: 0.708\n> Attempt 3: 0.689\n> Attempt 4: 0.695\n> Attempt 5: 0.7\n>\n> real 0m0.689s\n> user 0m0.560s\n> sys 0m0.248s\n>\n> $ best-of-five git grep --threads=8 '[q]werty'\n> Attempt 1: 0.238\n> Attempt 2: 0.225\n> Attempt 3: 0.222\n> Attempt 4: 0.221\n> Attempt 5: 0.225\n>\n> real 0m0.221s\n> user 0m0.936s\n> sys 0m0.356s\n>\n> In non-worktree cases most of the time goes to accessing objects, which\n> happens under a lock. So you don't get any real parallelism, just\n> overhead.\n>\n>> But assuming it works for someone out there, then 0 threads is clearly\n>> not the same as 1. On linux.git with pcre2 grepping for [q]werty for\n>> example[1]\n>\n> Right, my suggestion was to teach \"grep\" to treat --threads=1 as \"do not\n> spawn any other threads\". I.e., to make it like the \"0\" case you were\n> proposing, and then leave \"0\" as \"auto-detect\". There would be no way to\n> spawn a _single_ thread and feed it. But why would you want to do that?\n> It's always going to be strictly worse than not threading at all.\n\nI understand, but given the two profiles we've posted it seems clear\nthat there's cases where if we did that, we'd be locking people out of\ntheir optimal thread configuration, which would be --thread=1 with my\npatch, but wouldn't exist with this proposed change.\n\nIf you see better timings with 8 threads than 1 on linux.git, but I\nsee strictly worse, then if you apply my patch doesn't --threads=0\nlook worse than --threads=1, which looks worse than --threads=2 etc,\nuntil you reach some number where you either run out of CPU or I/O\nthroughput.\n\nAnyway, I really don't care about this feature much, I just wanted a\nway to disable threading, but looking at the perf profiles I wonder if\ndoing your proposed change would cause a regression in some cases\nwhere someone really wanted /one/ thread.\n\nBut of course my patch breaks the long documented grep.threads=0 for\n\"give me threads that you auto detect\" to now mean \"you get none\".\n\nAlso doesn't --thread=1 right now mean \"one thread, but two workers?\".\nI haven't dug into the grep worker/thread code, but it compiles the\nthe pattern twice, so isn't both the non-thread main process & the\nsole thread it spawns on --thread=1 doing work, so in some other\nuniverse it's synonymous with --workers=2?\n\nIf so do pack-objects & index-pack also behave like that? If so this\nwhole thing is very confusing for users, because some will read 1\nthread and think \"one worker\", whereas it really means \"two workers,\none using a thread, if you want three workers spawn two threads\".\n\nBah!\n"},{"id":"316859","messageId":"20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net","threadId":"45636","inReplyTo":"CACBZZX7Xi2OWqHQd7jTGBEZyqcWk59oXbPJOjuYrYAFzd5huCA@mail.gmail.com","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-14T21:23:25Z","receivedAt":"2017-04-14T21:23:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 10:56:01PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Right, my suggestion was to teach \"grep\" to treat --threads=1 as \"do not\n> > spawn any other threads\". I.e., to make it like the \"0\" case you were\n> > proposing, and then leave \"0\" as \"auto-detect\". There would be no way to\n> > spawn a _single_ thread and feed it. But why would you want to do that?\n> > It's always going to be strictly worse than not threading at all.\n> \n> I understand, but given the two profiles we've posted it seems clear\n> that there's cases where if we did that, we'd be locking people out of\n> their optimal thread configuration, which would be --thread=1 with my\n> patch, but wouldn't exist with this proposed change.\n\nMaybe I don't understand your profiles. For a single-core machine you\nprobably want fewer threads, right? There is no such thing as \"0\"\nthreads, as you always have the original main thread in which we would\ndo the work. So the lowest you can go is \"1\" (it's a separate question\nof what --threads=0 should \"mean\"; I think we should keep it as\n\"auto-detect\" for compatibility).\n\nWe could implement the single-thread case by spawning off one worker\nthread (and effectively having 2 threads, but one is just sitting in\npthread_join()). And I think that's how it's implemented now in\ngit-grep. But we can optimize out the creation of the second thread\nentirely, and just do the work in the main thread.\n\nThat saves a little bit of thread-spawning overhead, and it also makes\ndebugging much more pleasant. For --threads=2, you'd always have to\nkick in the thread-spawning code, and you'd spawn two worker threads\n(and the main thread just sits there).\n\nIOW, I am just proposing something like this:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 65070c52f..76ce38404 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -326,7 +326,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t}\n \n #ifndef NO_PTHREADS\n-\tif (num_threads) {\n+\tif (num_threads > 1) {\n \t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n@@ -360,7 +360,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t}\n \n #ifndef NO_PTHREADS\n-\tif (num_threads) {\n+\tif (num_threads > 1) {\n \t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n\nwhere we fall back to the same code that NO_PTHREADS uses when there is\nonly a single thread requested.\n\n> Anyway, I really don't care about this feature much, I just wanted a\n> way to disable threading, but looking at the perf profiles I wonder if\n> doing your proposed change would cause a regression in some cases\n> where someone really wanted /one/ thread.\n\nI'm not sure what would regress. Right now --threads=1 only does\nwork in a single worker thread. And --threads=2 does it in 2, and so on.\nIn all cases, the original main-thread is just farming out work and\nwaiting on pthread_join() (let's call that the controller thread). So\nwhy would you ever want the \"controller plus one worker\" setup? It's\nstrictly worse than \"controller just does the work\".\n\n> But of course my patch breaks the long documented grep.threads=0 for\n> \"give me threads that you auto detect\" to now mean \"you get none\".\n\nRight, that's what I'm concerned about.\n\n> Also doesn't --thread=1 right now mean \"one thread, but two workers?\".\n> I haven't dug into the grep worker/thread code, but it compiles the\n> the pattern twice, so isn't both the non-thread main process & the\n> sole thread it spawns on --thread=1 doing work, so in some other\n> universe it's synonymous with --workers=2?\n\nI think --threads=1 right now means \"one worker thread\". I think the\nmain program calls compile_grep_patterns() to make sure they are sane,\nbefore it even considers whether and how to thread. And then each\nworker thread duplicates them and re-compiles them itself (IIRC, this is\nbecause the regex code may not be thread-safe).\n\n> If so do pack-objects & index-pack also behave like that? If so this\n> whole thing is very confusing for users, because some will read 1\n> thread and think \"one worker\", whereas it really means \"two workers,\n> one using a thread, if you want three workers spawn two threads\".\n\nNo, I think --threads is \"this many workers\". If you have more than one\nworker, you may have an extra thread farming out work to them, but that\nisn't counted (and is mostly dormant).\n\n-Peff\n"},{"id":"316874","messageId":"xmqqinm6m6p0.fsf@gitster.mtv.corp.google.com","threadId":"45636","inReplyTo":"20170411104739.xzhxggpufvwgr3fu@sigill.intra.peff.net","subject":"Re: [PATCH 00/12] PCREv2 & more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-15T08:11:07Z","receivedAt":"2017-04-15T08:11:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Apr 08, 2017 at 01:24:54PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This adds PCRE v2 support, but as I was adding that I kept noticing\n>> other related problems to fix. It's all bundled up into the same\n>> series because much of it conflicts because it modifies the same test\n>> or other code. Notes on each patch below.\n>\n> Overall, the series looks OK to me.\n>\n> I'm not sure if it is worth all the complexity to carry pcre1/pcre2 as\n> run-time options. That does make it easier to do back-to-back\n> comparisons, but it makes the code a lot more complicated. In particular\n> I'm worried about subtle cases where we pcre1 turns into pcre2 (or vice\n> versa) by use of the aliases. That shouldn't matter to a user for\n> correctness, but it would throw off the benchmarking.\n>\n> If we literally just added USE_LIBPCRE2 and built against one or the\n> other, then all the complexity would be limited to a few #ifdefs. The\n> big drawback AFAICT is that anybody doing timing tests would have to\n> recompile in between.\n\nYeah, having to dl two libs at runtime, even when you would ever use\njust one in a single run, is less than ideal. A small downside\ninflicted on everybody will add up to million times more than a\nlarger downside only suffered by developers, so I tend to agree with\nyou that we probably should simplify to choose just one (or zero) at\ncompile time.\n\n> So I dunno. I had hoped libpcre2 would be a hands-down win on timing,\n> but it sounds like there's a little back-and-forth depending on the\n> context. Is there a ticking clock on pcre1 going away? I suspect it will\n> be around for many years to come, but if they'll continue optimizing\n> pcre2 but not pcre1, then it may make sense to hitch our wagon to the\n> one that upstream is actually working on.\n"},{"id":"316877","messageId":"CACBZZX6G4LFyqxvbba72ZzwuOPakiPWTSVbFCX7DzrZ5D5Vpqg@mail.gmail.com","threadId":"45636","inReplyTo":"xmqqinm6m6p0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 00/12] PCREv2 & more","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-15T09:50:05Z","receivedAt":"2017-04-15T09:50:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Apr 15, 2017 at 10:11 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Sat, Apr 08, 2017 at 01:24:54PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> This adds PCRE v2 support, but as I was adding that I kept noticing\n>>> other related problems to fix. It's all bundled up into the same\n>>> series because much of it conflicts because it modifies the same test\n>>> or other code. Notes on each patch below.\n>>\n>> Overall, the series looks OK to me.\n>>\n>> I'm not sure if it is worth all the complexity to carry pcre1/pcre2 as\n>> run-time options. That does make it easier to do back-to-back\n>> comparisons, but it makes the code a lot more complicated. In particular\n>> I'm worried about subtle cases where we pcre1 turns into pcre2 (or vice\n>> versa) by use of the aliases. That shouldn't matter to a user for\n>> correctness, but it would throw off the benchmarking.\n>>\n>> If we literally just added USE_LIBPCRE2 and built against one or the\n>> other, then all the complexity would be limited to a few #ifdefs. The\n>> big drawback AFAICT is that anybody doing timing tests would have to\n>> recompile in between.\n>\n> Yeah, having to dl two libs at runtime, even when you would ever use\n> just one in a single run, is less than ideal. A small downside\n> inflicted on everybody will add up to million times more than a\n> larger downside only suffered by developers, so I tend to agree with\n> you that we probably should simplify to choose just one (or zero) at\n> compile time.\n\nI'll document & clarify this in v2, but I don't expect / want anyone\nwho's distributing git to link to both v1 & v2, more details in my\n.\n\nIt's just something we already have 95% of the code to support anyway,\nand doing the remaining 5% makes it easier to test & benchmark it for\nus devs without incurring any real maintenance or tech burden.\n\nBut as noted elsewhere in that message I'll include a patch to only\nadd the ability to use one PCRE version. So we can just review &\ndiscuss the tradeoffs of doing that then.\n"},{"id":"316889","messageId":"CACBZZX7S_WdrNr04XnpnBhS=Kvtp0uAirx7yjUXrveh_jJSKLg@mail.gmail.com","threadId":"45636","inReplyTo":"20170411101412.jjzdscnirh5yerbn@sigill.intra.peff.net","subject":"Re: [PATCH 03/12] Makefile & configure: reword outdated comment about PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-15T12:10:20Z","receivedAt":"2017-04-15T12:10:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:14 PM, Jeff King wrote:\n> On Sat, Apr 08, 2017 at 01:24:57PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Reword an outdated comment which suggests that only git-grep can use\n>> PCRE.\n>\n> Makes sense.\n>\n>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n>> -# able to use Perl-compatible regular expressions.\n>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n>> +# commands such as like log, grep offer runtime options to use\n>> +# Perl-compatible regular expressions instead of standard or extended\n>> +# POSIX regular expressions.\n>\n> s/such as like log, grep/such as log and grep/ ?\nThanks.\n\n>> diff --git a/configure.ac b/configure.ac\n>> [...]\n>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n>> -# able to use Perl-compatible regular expressions.\n>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n>> +# commands such as like log, grep offer runtime options to use\n>> +# Perl-compatible regular expressions instead of standard or extended\n>> +# POSIX regular expressions.\n>\n> Ditto.\n>\n>> @@ -499,8 +501,10 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO])\n>> GIT_CONF_SUBST([NO_OPENSSL])\n>>\n>> #\n>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be\n>> -# able to use Perl-compatible regular expressions.\n>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various\n>> +# commands such as like log, grep offer runtime options to use\n>> +# Perl-compatible regular expressions instead of standard or extended\n>> +# POSIX regular expressions.\n>\n> And again. It's weird that the text appears twice in configure.ac.\n> Apparently you can use --with-libpcre or USE_LIBPCRE in the environment?\n> Configure is weird.\n\nYou can't, I've added this to the commit message:\n\n Copy/pasting this so much in configure.ac is lame, these Makefile-like\n flags aren't even used by autoconf, just the corresponding\n --with[out]-* options. But copy/pasting the comments that make sense\n for the Makefile to configure.ac where they make less sence is the\n pattern everything else follows in that file. I'm not going to war\n against that as part of this change, just following the existing\n pattern.\n"},{"id":"316969","messageId":"CACBZZX76hbERU5-_BQQ-swN_wxDfphS=qM7GOo_svyxQQG+Lgg@mail.gmail.com","threadId":"45636","inReplyTo":"20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-16T22:25:37Z","receivedAt":"2017-04-16T22:26:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Apr 14, 2017 at 11:23 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 10:56:01PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > Right, my suggestion was to teach \"grep\" to treat --threads=1 as \"do not\n>> > spawn any other threads\". I.e., to make it like the \"0\" case you were\n>> > proposing, and then leave \"0\" as \"auto-detect\". There would be no way to\n>> > spawn a _single_ thread and feed it. But why would you want to do that?\n>> > It's always going to be strictly worse than not threading at all.\n>>\n>> I understand, but given the two profiles we've posted it seems clear\n>> that there's cases where if we did that, we'd be locking people out of\n>> their optimal thread configuration, which would be --thread=1 with my\n>> patch, but wouldn't exist with this proposed change.\n>\n> Maybe I don't understand your profiles. For a single-core machine you\n> probably want fewer threads, right? There is no such thing as \"0\"\n> threads, as you always have the original main thread in which we would\n> do the work. So the lowest you can go is \"1\" (it's a separate question\n> of what --threads=0 should \"mean\"; I think we should keep it as\n> \"auto-detect\" for compatibility).\n>\n> We could implement the single-thread case by spawning off one worker\n> thread (and effectively having 2 threads, but one is just sitting in\n> pthread_join()). And I think that's how it's implemented now in\n> git-grep. But we can optimize out the creation of the second thread\n> entirely, and just do the work in the main thread.\n>\n> That saves a little bit of thread-spawning overhead, and it also makes\n> debugging much more pleasant. For --threads=2, you'd always have to\n> kick in the thread-spawning code, and you'd spawn two worker threads\n> (and the main thread just sits there).\n>\n> IOW, I am just proposing something like this:\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 65070c52f..76ce38404 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -326,7 +326,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n> }\n>\n> #ifndef NO_PTHREADS\n> - if (num_threads) {\n> + if (num_threads > 1) {\n> add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);\n> strbuf_release(&pathbuf);\n> return 0;\n> @@ -360,7 +360,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n> }\n>\n> #ifndef NO_PTHREADS\n> - if (num_threads) {\n> + if (num_threads > 1) {\n> add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n> strbuf_release(&buf);\n> return 0;\n>\n> where we fall back to the same code that NO_PTHREADS uses when there is\n> only a single thread requested.\n>\n>> Anyway, I really don't care about this feature much, I just wanted a\n>> way to disable threading, but looking at the perf profiles I wonder if\n>> doing your proposed change would cause a regression in some cases\n>> where someone really wanted /one/ thread.\n>\n> I'm not sure what would regress. Right now --threads=1 only does\n> work in a single worker thread. And --threads=2 does it in 2, and so on.\n> In all cases, the original main-thread is just farming out work and\n> waiting on pthread_join() (let's call that the controller thread). So\n> why would you ever want the \"controller plus one worker\" setup? It's\n> strictly worse than \"controller just does the work\".\n>\n>> But of course my patch breaks the long documented grep.threads=0 for\n>> \"give me threads that you auto detect\" to now mean \"you get none\".\n>\n> Right, that's what I'm concerned about.\n>\n>> Also doesn't --thread=1 right now mean \"one thread, but two workers?\".\n>> I haven't dug into the grep worker/thread code, but it compiles the\n>> the pattern twice, so isn't both the non-thread main process & the\n>> sole thread it spawns on --thread=1 doing work, so in some other\n>> universe it's synonymous with --workers=2?\n>\n> I think --threads=1 right now means \"one worker thread\". I think the\n> main program calls compile_grep_patterns() to make sure they are sane,\n> before it even considers whether and how to thread. And then each\n> worker thread duplicates them and re-compiles them itself (IIRC, this is\n> because the regex code may not be thread-safe).\n>\n>> If so do pack-objects & index-pack also behave like that? If so this\n>> whole thing is very confusing for users, because some will read 1\n>> thread and think \"one worker\", whereas it really means \"two workers,\n>> one using a thread, if you want three workers spawn two threads\".\n>\n> No, I think --threads is \"this many workers\". If you have more than one\n> worker, you may have an extra thread farming out work to them, but that\n> isn't counted (and is mostly dormant).\n\nI submitted a v2 of this patch series, see \"[PATCH v2 0/8] grep\nthreading cleanup & tests\", as noted there this whole confusing v1 +\nthread is me misunderstanding how grep actually worked. Thanks for\nbeing patient with me, that patch series fixes the odd handling of\ngrep patterns which led me down the garden path of thinking that\nworkers != threads in the case of grep (which in retrospect couldn't\nhave made any sense either, my mental model of C threading was wrong).\n"}]}