{"thread":{"id":"46011","subject":"[PATCH v3 00/30] Easy to review grep & pre-PCRE changes","startedAt":"2017-05-20T21:42:55Z","lastAt":"2017-05-26T09:49:09Z","messageCount":58,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Brandon Williams"],"isPatch":true,"patchVersion":3,"patchTotal":30},"messages":[{"id":"320327","messageId":"20170520214233.7183-1-avarab@gmail.com","threadId":"46011","inReplyTo":null,"subject":"[PATCH v3 00/30] Easy to review grep & pre-PCRE changes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:03Z","receivedAt":"2017-05-20T21:42:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Easy to review? 29 (I mean 30) patches? Are you kidding me?!\n\nAs noted in v1 (<20170511091829.5634-1-avarab@gmail.com>;\nhttps://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)\nthese are all doc, test, refactoring etc. changes needed by the\nsubsequent \"PCRE v2, PCRE v1 JIT, log -P & fixes\" series.\n\nSince Junio hasn't been picking it I'm no longer sending updates to\nthat patch series & waiting for this one to cook first.\n\nSee <20170513231509.7834-1-avarab@gmail.com>\n(https://public-inbox.org/git/20170513231509.7834-1-avarab@gmail.com/)\nfor v2 & notes about that version. What changed this time around? See\nbelow:\n\nÆvar Arnfjörð Bjarmason (30):\n Makefile & configure: reword inaccurate comment about PCRE\n grep & rev-list doc: stop promising libpcre for --perl-regexp\n test-lib: rename the LIBPCRE prerequisite to PCRE\n\nNo changes.\n\n log: add exhaustive tests for pattern style options & config\n\nTest comment clarifications in t4202-log.sh as pointed out by Junio.\n\n log: make --regexp-ignore-case work with --perl-regexp\n\nNEW: I noticed that the `-i` in `git log --perl-regexp -i --grep=`\nnever worked as intended. I.e. the flag for ignoring the case of the\npattern wasn't picked up.\n\nFixing this was trivial (one-line change), so I've included it in this\nseries since it's needed by a new t/perf patch (see below).\n\n grep: add a test asserting that --perl-regexp dies when !PCRE\n grep: add a test for backreferences in PCRE patterns\n grep: change non-ASCII -i test to stop using --debug\n grep: add tests for --threads=N and grep.threads\n grep: amend submodule recursion test for regex engine testing\n grep: add tests for grep pattern types being passed to submodules\n\nNo changes.\n\n grep: add a test helper function for less verbose -f \\0 tests\n\nTrivial style changes in nul_match() suggested by Junio. No functional\nchanges.\n\n grep: prepare for testing binary regexes containing rx metacharacters\n\nNo changes.\n\n grep: add tests to fix blind spots with \\0 patterns\n\nContinued trivial style changes in nul_match() (the other half of the\ncode in that function is added in this commit)>\n\n perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do\n perf: emit progress output when unpacking & building\n\nNo changes.\n\n perf: add a comparison test of grep regex engines\n perf: add a comparison test of grep regex engines with -F\n perf: add a comparison test of log --grep regex engines\n\nThe log --grep test is new, and all these tests learned to take an env\nvariable to pass arbitrary extra grep/log flags, so I can e.g. test\nwith -i, -v, -w etc.\n\nSubsequent commit messages that e.g. mentioned perf tests with the\nprevious hardcoded -i test have been amended to mention the new test\nresults.\n\n grep: catch a missing enum in switch statement\n\nGrammar fix in commit message.\n\n grep: remove redundant regflags assignments\n\nThe two commits that made changes to regflags assignments have been\nsquashed.\n\n grep: factor test for \\0 in grep patterns into a function\n\nRewrote commit message to not go off on a tangent about what grep -f\n[file-with-\\0-pattern] should mean, which is not what this change is\nabout.\n\n grep: change the internal PCRE macro names to be PCRE1\n grep: change internal *pcre* variable & function names to be *pcre1*\n grep: move is_fixed() earlier to avoid forward declaration\n test-lib: add a PTHREADS prerequisite\n\nNo changes.\n\n pack-objects & index-pack: add test for --threads warning\n pack-objects: fix buggy warning about threads\n\nRewrote the tests in these two so that the first one sets up a failing\ntest which is subsequently fixed in the commit that fixes the bug, as\nsuggested by Junio.\n\nRemoved a stray `cat err` left over from debugging.\n\n grep: given --threads with NO_PTHREADS=YesPlease, warn\n grep: assert that threading is enabled when calling grep_{lock,unlock}\n\nNo changes.\n\n Documentation/git-grep.txt | 7 +-\n Documentation/rev-list-options.txt | 8 +-\n Makefile | 14 ++-\n builtin/grep.c | 23 +++-\n builtin/pack-objects.c | 4 +-\n configure.ac | 12 ++-\n grep.c | 108 ++++++++++---------\n grep.h | 10 +-\n revision.c | 1 +\n t/README | 8 +-\n t/perf/README | 19 +++-\n t/perf/p4220-log-grep-engines.sh | 44 ++++++++\n t/perf/p7820-grep-engines.sh | 47 ++++++++\n t/perf/p7821-grep-engines-fixed.sh | 32 ++++++\n t/perf/run | 13 ++-\n t/t4202-log.sh | 160 +++++++++++++++++++++++++--\n t/t5300-pack-object.sh | 36 +++++++\n t/t7008-grep-binary.sh | 135 +++++++++++++++++------\n t/t7810-grep.sh | 81 +++++++++++---\n t/t7812-grep-icase-non-ascii.sh | 29 ++---\n t/t7813-grep-icase-iso.sh | 2 +-\n t/t7814-grep-recurse-submodules.sh | 215 +++++++++++++++++++++++--------------\n t/test-lib.sh | 3 +-\n 23 files changed, 771 insertions(+), 240 deletions(-)\n create mode 100755 t/perf/p4220-log-grep-engines.sh\n create mode 100755 t/perf/p7820-grep-engines.sh\n create mode 100755 t/perf/p7821-grep-engines-fixed.sh\n\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320328","messageId":"20170520214233.7183-2-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 01/30] Makefile & configure: reword inaccurate comment about PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:04Z","receivedAt":"2017-05-20T21:42:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Reword an outdated & inaccurate comment which suggests that only\ngit-grep can use PCRE.\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\nCopy/pasting this so much in configure.ac is lame, these Makefile-like\nflags aren't even used by autoconf, just the corresponding\n--with[out]-* options. But copy/pasting the comments that make sense\nfor the Makefile to configure.ac where they make less sense is the\npattern everything else follows in that file. I'm not going to war\nagainst that as part of this change, just following the existing\npattern.\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 e35542e631..eedadb8056 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 log and 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..deeb968daa 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 log and 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 log and 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.13.0.303.g4ebf302169\n\n"},{"id":"320329","messageId":"20170520214233.7183-3-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 02/30] grep & rev-list doc: stop promising libpcre for --perl-regexp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:05Z","receivedAt":"2017-05-20T21:42:59Z","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 these types of patterns using \"a compile-time dependency\".\n\nSaying \"libpcre\" means that we're talking about libpcre.so, which is\nalways going to be v1. This change is part of an ongoing saga to add\nsupport for libpcre2, which comes with PCRE v2.\n\nIn the future we might use some completely unrelated library to\nprovide perl-compatible regular expression support. By wording the\ndocumentation differently and not promising any specific version of\nPCRE or even PCRE at all we have more wiggle room to change the\nimplementation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-grep.txt | 7 +++++--\n Documentation/rev-list-options.txt | 8 ++++++--\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 71f32f3508..5033483db4 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -161,8 +161,11 @@ 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.\n++\n+Support for these types of regular expressions is an optional\n+compile-time dependency. If Git wasn't compiled with support for them\n+providing this option will cause it to die.\n \n -F::\n --fixed-strings::\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a02f7324c0..a46f70c2b1 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -92,8 +92,12 @@ endif::git-rev-list[]\n \tpattern as a regular expression).\n \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.\n++\n+Support for these types of regular expressions is an optional\n+compile-time dependency. If Git wasn't compiled with support for them\n+providing this option will cause it to die.\n \n --remove-empty::\n \tStop when a given path disappears from the tree.\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320330","messageId":"20170520214233.7183-4-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 03/30] test-lib: rename the LIBPCRE prerequisite to PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:06Z","receivedAt":"2017-05-20T21:43:03Z","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/t7810-grep.sh | 28 ++++++++++++++--------------\n t/t7812-grep-icase-non-ascii.sh | 4 ++--\n t/t7813-grep-icase-iso.sh | 2 +-\n t/test-lib.sh | 2 +-\n 5 files changed, 20 insertions(+), 20 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/t7810-grep.sh b/t/t7810-grep.sh\nindex cee42097b0..c84c4d99f9 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@@ -1118,11 +1118,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@@ -1191,13 +1191,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@@ -1208,7 +1208,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@@ -1343,12 +1343,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 '\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 26b3edfb2e..04d857a42b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1014,7 +1014,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.13.0.303.g4ebf302169\n\n"},{"id":"320331","messageId":"20170520214233.7183-5-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 04/30] log: add exhaustive tests for pattern style options & config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:07Z","receivedAt":"2017-05-20T21:43:05Z","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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 97 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex f577990716..a8dce0ca2d 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -262,7 +262,30 @@ 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+\t# basic would need \\(s\\) to do the same\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 PCRE 'log -F -E --perl-regexp --grep= uses PCRE' '\n+\ttest_when_finished \"rm -rf num_commits\" &&\n+\tgit init num_commits &&\n+\t(\n+\t\tcd num_commits &&\n+\t\ttest_commit 1d &&\n+\t\ttest_commit 2e\n+\t) &&\n+\n+\t# In PCRE \\d in [\\d] is like saying \"0-9\", and matches the 2\n+\t# in 2e...\n+\techo 2e >expect &&\n+\tgit -C num_commits log -1 --pretty=\"tformat:%s\" -F -E --perl-regexp --grep=\"[\\d]\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# ...in POSIX basic and extended it is the same as [d],\n+\t# i.e. \"d\", which matches 1d, but does not match 2e.\n+\techo 1d >expect &&\n+\tgit -C num_commits log -1 --pretty=\"tformat:%s\" -F -E --grep=\"[\\d]\" >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -280,6 +303,79 @@ 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+\n+\t\t# The tagname is overridden here because creating a\n+\t\t# tag called \"(1|2)\" as test_commit would otherwise\n+\t\t# implicitly do would fail on e.g. MINGW.\n+\t\ttest_commit \"(1|2)\" file B 2 &&\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\t# A strcmp-like match with fixed.\n+\t\tgit -c grep.patternType=fixed log --pretty=tformat:%s \\\n+\t\t\t--grep=\"(1|2)\" >actual.fixed &&\n+\n+\t\t# POSIX basic matches (, | and ) literally.\n+\t\tgit -c grep.patternType=basic log --pretty=tformat:%s \\\n+\t\t\t--grep=\"(.|.)\" >actual.basic &&\n+\n+\t\t# POSIX extended needs to have | escaped to match it\n+\t\t# literally, whereas under basic this is the same as\n+\t\t# (|2), i.e. it would also match \"1\". This test checks\n+\t\t# for extended by asserting that it is not matching\n+\t\t# what basic would match.\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 PCRE\n+\t\tthen\n+\t\t\t# Only PCRE would match [\\d]\\| with only\n+\t\t\t# \"(1|2)\" due to [\\d]. POSIX basic would match\n+\t\t\t# both it and \"1\" since similarly to the\n+\t\t\t# extended match above it is the same as\n+\t\t\t# \\([\\d]\\|\\). POSIX extended would\n+\t\t\t# match neither.\n+\t\t\tgit -c grep.patternType=perl log --pretty=tformat:%s \\\n+\t\t\t\t--grep=\"[\\d]\\|\" >actual.perl &&\n+\t\t\ttest_cmp expect.perl 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+\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 PCRE\n+\t\tthen\n+\t\t\tgit log --pretty=tformat:%s --perl-regexp \\\n+\t\t\t\t--grep=\"[\\d]\\|\" >actual.perl.long-arg &&\n+\t\t\ttest_cmp expect.perl actual.perl.long-arg\n+\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)\n+'\n+\n cat > expect <\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 8d69113695..daa906b9b0 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1114,6 +1114,13 @@ test_expect_success PCRE '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.13.0.303.g4ebf302169\n\n"},{"id":"320333","messageId":"20170520214233.7183-6-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 05/30] log: make --regexp-ignore-case work with --perl-regexp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:08Z","receivedAt":"2017-05-20T21:43:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the --regexp-ignore-case option work with --perl-regexp. This\nnever worked, and there was no test for this. Fix the bug and add a\ntest.\n\nWhen PCRE support was added in commit 63e7e9d8b6 (\"git-grep: Learn\nPCRE\", 2011-05-09) compile_pcre_regexp() would only check\nopt->ignore_case, but when the --perl-regexp option was added in\ncommit 727b6fc3ed (\"log --grep: accept --basic-regexp and\n--perl-regexp\", 2012-10-03) the code didn't set the opt->ignore_case.\n\nChange the test suite to test for -i and --invert-regexp with\nbasic/extended/perl patterns in addition to fixed, which was the only\npatternType that was tested for before in combination with those\noptions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n revision.c | 1 +\n t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 56 insertions(+), 5 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8a8c1789c7..4883cdd2d0 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1991,6 +1991,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--extended-regexp\") || !strcmp(arg, \"-E\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n \t} else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n+\t\trevs->grep_filter.ignore_case = 1;\n \t\trevs->grep_filter.regflags |= REG_ICASE;\n \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex a8dce0ca2d..547f4c19a7 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -231,14 +231,47 @@ second\n initial\n EOF\n test_expect_success 'log --invert-grep --grep' '\n-\tgit log --pretty=\"tformat:%s\" --invert-grep --grep=th --grep=Sec >actual &&\n-\ttest_cmp expect actual\n+\t# Fixed\n+\tgit -c grep.patternType=fixed log --pretty=\"tformat:%s\" --invert-grep --grep=th --grep=Sec >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX basic\n+\tgit -c grep.patternType=basic log --pretty=\"tformat:%s\" --invert-grep --grep=t[h] --grep=S[e]c >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX extended\n+\tgit -c grep.patternType=basic log --pretty=\"tformat:%s\" --invert-grep --grep=t[h] --grep=S[e]c >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# PCRE\n+\tif test_have_prereq PCRE\n+\tthen\n+\t\tgit -c grep.patternType=perl log --pretty=\"tformat:%s\" --invert-grep --grep=t[h] --grep=S[e]c >actual &&\n+\t\ttest_cmp expect actual\n+\tfi\n '\n \n test_expect_success 'log --invert-grep --grep -i' '\n \techo initial >expect &&\n-\tgit log --pretty=\"tformat:%s\" --invert-grep -i --grep=th --grep=Sec >actual &&\n-\ttest_cmp expect actual\n+\n+\t# Fixed\n+\tgit -c grep.patternType=fixed log --pretty=\"tformat:%s\" --invert-grep -i --grep=th --grep=Sec >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX basic\n+\tgit -c grep.patternType=basic log --pretty=\"tformat:%s\" --invert-grep -i --grep=t[h] --grep=S[e]c >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX extended\n+\tgit -c grep.patternType=extended log --pretty=\"tformat:%s\" --invert-grep -i --grep=t[h] --grep=S[e]c >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# PCRE\n+\tif test_have_prereq PCRE\n+\tthen\n+\t\tgit -c grep.patternType=perl log --pretty=\"tformat:%s\" --invert-grep -i --grep=t[h] --grep=S[e]c >actual &&\n+\t\ttest_cmp expect actual\n+\tfi\n '\n \n test_expect_success 'log --grep option parsing' '\n@@ -256,8 +289,25 @@ test_expect_success 'log -i --grep' '\n \n test_expect_success 'log --grep -i' '\n \techo Second >expect &&\n+\n+\t# Fixed\n \tgit log -1 --pretty=\"tformat:%s\" --grep=sec -i >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX basic\n+\tgit -c grep.patternType=basic log -1 --pretty=\"tformat:%s\" --grep=s[e]c -i >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# POSIX extended\n+\tgit -c grep.patternType=extended log -1 --pretty=\"tformat:%s\" --grep=s[e]c -i >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# PCRE\n+\tif test_have_prereq PCRE\n+\tthen\n+\t\tgit -c grep.patternType=perl log -1 --pretty=\"tformat:%s\" --grep=s[e]c -i >actual &&\n+\t\ttest_cmp expect actual\n+\tfi\n '\n \n test_expect_success 'log -F -E --grep= uses ere' '\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320334","messageId":"20170520214233.7183-7-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 06/30] grep: add a test asserting that --perl-regexp dies when !PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:09Z","receivedAt":"2017-05-20T21:43:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a test asserting that when --perl-regexp (and -P for grep) is\ngiven to git-grep & git-log that we die with an error.\n\nIn developing the PCRE v2 series I introduced a regression where -P\nwould (through control-flow fall-through) become synonymous with basic\nPOSIX matching. I.e. 'git grep -P '[\\d]' would match \"d\" instead of\ndigits.\n\nThe entire test suite would still pass with this serious regression,\nsince everything that tested for --perl-regexp would be guarded by the\nPCRE prerequisite, fix that blind-spot by adding tests under !PCRE\nasserting that git must die when given --perl-regexp or -P.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4202-log.sh | 4 +++-\n t/t7810-grep.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 547f4c19a7..dbed3efeee 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -418,7 +418,9 @@ test_expect_success 'log with various grep.patternType configurations & command-\n \t\t\tgit log --pretty=tformat:%s --perl-regexp \\\n \t\t\t\t--grep=\"[\\d]\\|\" >actual.perl.long-arg &&\n \t\t\ttest_cmp expect.perl actual.perl.long-arg\n-\n+\t\telse\n+\t\t\ttest_must_fail git log --perl-regexp \\\n+\t\t\t\t--grep=\"[\\d]\\|\"\n \t\tfi &&\n \t\ttest_cmp expect.fixed actual.fixed.long-arg &&\n \t\ttest_cmp expect.basic actual.basic.long-arg &&\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex c84c4d99f9..8d69113695 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -281,6 +281,10 @@ do\n \t\ttest_cmp expected actual\n \t'\n \n+\ttest_expect_success !PCRE \"grep $L with grep.patterntype=perl errors without PCRE\" '\n+\t\ttest_must_fail git -c grep.patterntype=perl grep \"foo.*bar\"\n+\t'\n+\n \ttest_expect_success \"grep $L with grep.patternType=default and grep.extendedRegexp=true\" '\n \t\techo \"${HC}ab:abc\" >expected &&\n \t\tgit \\\n@@ -1058,11 +1062,19 @@ test_expect_success PCRE 'grep --perl-regexp pattern' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success !PCRE 'grep --perl-regexp pattern errors without PCRE' '\n+\ttest_must_fail git grep --perl-regexp \"foo.*bar\"\n+'\n+\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 \n+test_expect_success !PCRE 'grep -P pattern errors without PCRE' '\n+\ttest_must_fail git grep -P \"foo.*bar\"\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-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320335","messageId":"20170520214233.7183-9-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 08/30] grep: change non-ASCII -i test to stop using --debug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:11Z","receivedAt":"2017-05-20T21:43:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a non-ASCII case-insensitive test case to stop using --debug,\nand instead simply test for the expected results.\n\nThe test coverage remains the same with this change, but the test\nwon't break due to internal refactoring.\n\nThis test was added in commit 793dc676e0 (\"grep/icase: avoid kwsset\nwhen -F is specified\", 2016-06-25). It was asserting that the regex\nmust be compiled with compile_fixed_regexp(), instead test for the\nexpected results, allowing the underlying implementation to change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7812-grep-icase-non-ascii.sh | 25 +++++--------------------\n 1 file changed, 5 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\nindex 04a61cb8e0..0059a1f837 100755\n--- a/t/t7812-grep-icase-non-ascii.sh\n+++ b/t/t7812-grep-icase-non-ascii.sh\n@@ -36,29 +36,14 @@ test_expect_success GETTEXT_LOCALE,PCRE 'grep pcre utf-8 string with \"+\"' '\n '\n \n test_expect_success REGEX_LOCALE 'grep literal string, with -F' '\n-\tgit grep --debug -i -F \"TILRAUN: Halló Heimur!\" 2>&1 >/dev/null |\n-\t\t grep fixed >debug1 &&\n-\ttest_write_lines \"fixed TILRAUN: Halló Heimur!\" >expect1 &&\n-\ttest_cmp expect1 debug1 &&\n-\n-\tgit grep --debug -i -F \"TILRAUN: HALLÓ HEIMUR!\" 2>&1 >/dev/null |\n-\t\t grep fixed >debug2 &&\n-\ttest_write_lines \"fixed TILRAUN: HALLÓ HEIMUR!\" >expect2 &&\n-\ttest_cmp expect2 debug2\n+\tgit grep -i -F \"TILRAUN: Halló Heimur!\" &&\n+\tgit grep -i -F \"TILRAUN: HALLÓ HEIMUR!\"\n '\n \n test_expect_success REGEX_LOCALE 'grep string with regex, with -F' '\n-\ttest_write_lines \"^*TILR^AUN:.* \\\\Halló \\$He[]imur!\\$\" >file &&\n-\n-\tgit grep --debug -i -F \"^*TILR^AUN:.* \\\\Halló \\$He[]imur!\\$\" 2>&1 >/dev/null |\n-\t\t grep fixed >debug1 &&\n-\ttest_write_lines \"fixed \\\\^*TILR^AUN:\\\\.\\\\* \\\\\\\\Halló \\$He\\\\[]imur!\\\\\\$\" >expect1 &&\n-\ttest_cmp expect1 debug1 &&\n-\n-\tgit grep --debug -i -F \"^*TILR^AUN:.* \\\\HALLÓ \\$HE[]IMUR!\\$\" 2>&1 >/dev/null |\n-\t\t grep fixed >debug2 &&\n-\ttest_write_lines \"fixed \\\\^*TILR^AUN:\\\\.\\\\* \\\\\\\\HALLÓ \\$HE\\\\[]IMUR!\\\\\\$\" >expect2 &&\n-\ttest_cmp expect2 debug2\n+\ttest_write_lines \"TILRAUN: Halló Heimur [abc]!\" >file3 &&\n+\tgit add file3 &&\n+\tgit grep -i -F \"TILRAUN: Halló Heimur [abc]!\" file3\n '\n \n test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320336","messageId":"20170520214233.7183-13-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 12/30] grep: add a test helper function for less verbose -f \\0 tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:15Z","receivedAt":"2017-05-20T21:43:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a helper function to make the tests which check for patterns with\n\\0 in them more succinct. Right now this isn't a big win, but\nsubsequent commits will add a lot more of these tests.\n\nThe helper is based on the match() function in t3070-wildmatch.sh.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7008-grep-binary.sh | 58 +++++++++++++++++++++++++-------------------------\n 1 file changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 9c9c378119..df93d8e44c 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -4,6 +4,29 @@ test_description='git grep in binary files'\n \n . ./test-lib.sh\n \n+nul_match () {\n+\tmatches=$1\n+\tflags=$2\n+\tpattern=$3\n+\tpattern_human=$(echo \"$pattern\" | sed 's/Q//g')\n+\n+\tif test \"$matches\" = 1\n+\tthen\n+\t\ttest_expect_success \"git grep -f f $flags '$pattern_human' a\" \"\n+\t\t\tprintf '$pattern' | q_to_nul >f &&\n+\t\t\tgit grep -f f $flags a\n+\t\t\"\n+\telif test \"$matches\" = 0\n+\tthen\n+\t\ttest_expect_success \"git grep -f f $flags '$pattern_human' a\" \"\n+\t\t\tprintf '$pattern' | q_to_nul >f &&\n+\t\t\ttest_must_fail git grep -f f $flags a\n+\t\t\"\n+\telse\n+\t\ttest_expect_success \"PANIC: Test framework error. Unknown matches value $matches\" 'false'\n+\tfi\n+}\n+\n test_expect_success 'setup' \"\n \techo 'binaryQfile' | q_to_nul >a &&\n \tgit add a &&\n@@ -69,35 +92,12 @@ test_expect_failure 'git grep .fi a' '\n \tgit grep .fi a\n '\n \n-test_expect_success 'git grep -F yf a' \"\n-\tprintf 'yQf' | q_to_nul >f &&\n-\tgit grep -f f -F a\n-\"\n-\n-test_expect_success 'git grep -F yx a' \"\n-\tprintf 'yQx' | q_to_nul >f &&\n-\ttest_must_fail git grep -f f -F a\n-\"\n-\n-test_expect_success 'git grep -Fi Yf a' \"\n-\tprintf 'YQf' | q_to_nul >f &&\n-\tgit grep -f f -Fi a\n-\"\n-\n-test_expect_success 'git grep -Fi Yx a' \"\n-\tprintf 'YQx' | q_to_nul >f &&\n-\ttest_must_fail git grep -f f -Fi a\n-\"\n-\n-test_expect_success 'git grep yf a' \"\n-\tprintf 'yQf' | q_to_nul >f &&\n-\tgit grep -f f a\n-\"\n-\n-test_expect_success 'git grep yx a' \"\n-\tprintf 'yQx' | q_to_nul >f &&\n-\ttest_must_fail git grep -f f a\n-\"\n+nul_match 1 '-F' 'yQf'\n+nul_match 0 '-F' 'yQx'\n+nul_match 1 '-Fi' 'YQf'\n+nul_match 0 '-Fi' 'YQx'\n+nul_match 1 '' 'yQf'\n+nul_match 0 '' 'yQx'\n \n test_expect_success 'grep respects binary diff attribute' '\n \techo text >t &&\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320337","messageId":"20170520214233.7183-11-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 10/30] grep: amend submodule recursion test for regex engine testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:13Z","receivedAt":"2017-05-20T21:43:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the submodule recursion test to prepare it for subsequent tests\nof whether it passes along the grep.patternType to the submodule\ngreps.\n\nThis is the result of searching & replacing:\n\n foobar -> (1|2)d(3|4)\n foo -> (1|2)\n bar -> (3|4)\n\nCurrently there's no tests for whether e.g. -P or -E is correctly\npassed along, tests for that will be added in a follow-up change, but\nfirst add content to the tests which will match differently under\ndifferent regex engines.\n\nReuse the pattern established in an earlier commit of mine in this\nseries (\"log: add exhaustive tests for pattern style options &\nconfig\", 2017-04-07). The pattern \"(.|.)[\\d]\" will match this content\ndifferently under fixed/basic/extended & perl.\n\nThis test code was originally added in commit 0281e487fd (\"grep:\noptionally recurse into submodules\", 2016-12-16).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7814-grep-recurse-submodules.sh | 166 ++++++++++++++++++-------------------\n 1 file changed, 83 insertions(+), 83 deletions(-)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 5b6eb3a65e..1472855e1d 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -9,13 +9,13 @@ submodules.\n . ./test-lib.sh\n \n test_expect_success 'setup directory structure and submodule' '\n-\techo \"foobar\" >a &&\n+\techo \"(1|2)d(3|4)\" >a &&\n \tmkdir b &&\n-\techo \"bar\" >b/b &&\n+\techo \"(3|4)\" >b/b &&\n \tgit add a b &&\n \tgit commit -m \"add a and b\" &&\n \tgit init submodule &&\n-\techo \"foobar\" >submodule/a &&\n+\techo \"(1|2)d(3|4)\" >submodule/a &&\n \tgit -C submodule add a &&\n \tgit -C submodule commit -m \"add a\" &&\n \tgit submodule add ./submodule &&\n@@ -24,18 +24,18 @@ test_expect_success 'setup directory structure and submodule' '\n \n test_expect_success 'grep correctly finds patterns in a submodule' '\n \tcat >expect <<-\\EOF &&\n-\ta:foobar\n-\tb/b:bar\n-\tsubmodule/a:foobar\n+\ta:(1|2)d(3|4)\n+\tb/b:(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep and basic pathspecs' '\n \tcat >expect <<-\\EOF &&\n-\tsubmodule/a:foobar\n+\tsubmodule/a:(1|2)d(3|4)\n \tEOF\n \n \tgit grep -e. --recurse-submodules -- submodule >actual &&\n@@ -44,7 +44,7 @@ test_expect_success 'grep and basic pathspecs' '\n \n test_expect_success 'grep and nested submodules' '\n \tgit init submodule/sub &&\n-\techo \"foobar\" >submodule/sub/a &&\n+\techo \"(1|2)d(3|4)\" >submodule/sub/a &&\n \tgit -C submodule/sub add a &&\n \tgit -C submodule/sub commit -m \"add a\" &&\n \tgit -C submodule submodule add ./sub &&\n@@ -54,117 +54,117 @@ test_expect_success 'grep and nested submodules' '\n \tgit commit -m \"updated submodule\" &&\n \n \tcat >expect <<-\\EOF &&\n-\ta:foobar\n-\tb/b:bar\n-\tsubmodule/a:foobar\n-\tsubmodule/sub/a:foobar\n+\ta:(1|2)d(3|4)\n+\tb/b:(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep and multiple patterns' '\n \tcat >expect <<-\\EOF &&\n-\ta:foobar\n-\tsubmodule/a:foobar\n-\tsubmodule/sub/a:foobar\n+\ta:(1|2)d(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --and -e \"foo\" --recurse-submodules >actual &&\n+\tgit grep -e \"(3|4)\" --and -e \"(1|2)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep and multiple patterns' '\n \tcat >expect <<-\\EOF &&\n-\tb/b:bar\n+\tb/b:(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --and --not -e \"foo\" --recurse-submodules >actual &&\n+\tgit grep -e \"(3|4)\" --and --not -e \"(1|2)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'basic grep tree' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD:a:foobar\n-\tHEAD:b/b:bar\n-\tHEAD:submodule/a:foobar\n-\tHEAD:submodule/sub/a:foobar\n+\tHEAD:a:(1|2)d(3|4)\n+\tHEAD:b/b:(3|4)\n+\tHEAD:submodule/a:(1|2)d(3|4)\n+\tHEAD:submodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree HEAD^' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD^:a:foobar\n-\tHEAD^:b/b:bar\n-\tHEAD^:submodule/a:foobar\n+\tHEAD^:a:(1|2)d(3|4)\n+\tHEAD^:b/b:(3|4)\n+\tHEAD^:submodule/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD^ >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD^ >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree HEAD^^' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD^^:a:foobar\n-\tHEAD^^:b/b:bar\n+\tHEAD^^:a:(1|2)d(3|4)\n+\tHEAD^^:b/b:(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD^^ >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD^^ >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree and pathspecs' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD:submodule/a:foobar\n-\tHEAD:submodule/sub/a:foobar\n+\tHEAD:submodule/a:(1|2)d(3|4)\n+\tHEAD:submodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD -- submodule >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD -- submodule >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree and pathspecs' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD:submodule/a:foobar\n-\tHEAD:submodule/sub/a:foobar\n+\tHEAD:submodule/a:(1|2)d(3|4)\n+\tHEAD:submodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD -- \"submodule*a\" >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD -- \"submodule*a\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree and more pathspecs' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD:submodule/a:foobar\n+\tHEAD:submodule/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD -- \"submodul?/a\" >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD -- \"submodul?/a\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep tree and more pathspecs' '\n \tcat >expect <<-\\EOF &&\n-\tHEAD:submodule/sub/a:foobar\n+\tHEAD:submodule/sub/a:(1|2)d(3|4)\n \tEOF\n \n-\tgit grep -e \"bar\" --recurse-submodules HEAD -- \"submodul*/sub/a\" >actual &&\n+\tgit grep -e \"(3|4)\" --recurse-submodules HEAD -- \"submodul*/sub/a\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success !MINGW 'grep recurse submodule colon in name' '\n \tgit init parent &&\n \ttest_when_finished \"rm -rf parent\" &&\n-\techo \"foobar\" >\"parent/fi:le\" &&\n+\techo \"(1|2)d(3|4)\" >\"parent/fi:le\" &&\n \tgit -C parent add \"fi:le\" &&\n \tgit -C parent commit -m \"add fi:le\" &&\n \n \tgit init \"su:b\" &&\n \ttest_when_finished \"rm -rf su:b\" &&\n-\techo \"foobar\" >\"su:b/fi:le\" &&\n+\techo \"(1|2)d(3|4)\" >\"su:b/fi:le\" &&\n \tgit -C \"su:b\" add \"fi:le\" &&\n \tgit -C \"su:b\" commit -m \"add fi:le\" &&\n \n@@ -172,30 +172,30 @@ test_expect_success !MINGW 'grep recurse submodule colon in name' '\n \tgit -C parent commit -m \"add submodule\" &&\n \n \tcat >expect <<-\\EOF &&\n-\tfi:le:foobar\n-\tsu:b/fi:le:foobar\n+\tfi:le:(1|2)d(3|4)\n+\tsu:b/fi:le:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules >actual &&\n+\tgit -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n-\tHEAD:fi:le:foobar\n-\tHEAD:su:b/fi:le:foobar\n+\tHEAD:fi:le:(1|2)d(3|4)\n+\tHEAD:su:b/fi:le:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules HEAD >actual &&\n+\tgit -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep history with moved submoules' '\n \tgit init parent &&\n \ttest_when_finished \"rm -rf parent\" &&\n-\techo \"foobar\" >parent/file &&\n+\techo \"(1|2)d(3|4)\" >parent/file &&\n \tgit -C parent add file &&\n \tgit -C parent commit -m \"add file\" &&\n \n \tgit init sub &&\n \ttest_when_finished \"rm -rf sub\" &&\n-\techo \"foobar\" >sub/file &&\n+\techo \"(1|2)d(3|4)\" >sub/file &&\n \tgit -C sub add file &&\n \tgit -C sub commit -m \"add file\" &&\n \n@@ -203,82 +203,82 @@ test_expect_success 'grep history with moved submoules' '\n \tgit -C parent commit -m \"add submodule\" &&\n \n \tcat >expect <<-\\EOF &&\n-\tdir/sub/file:foobar\n-\tfile:foobar\n+\tdir/sub/file:(1|2)d(3|4)\n+\tfile:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules >actual &&\n+\tgit -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual &&\n \n \tgit -C parent mv dir/sub sub-moved &&\n \tgit -C parent commit -m \"moved submodule\" &&\n \n \tcat >expect <<-\\EOF &&\n-\tfile:foobar\n-\tsub-moved/file:foobar\n+\tfile:(1|2)d(3|4)\n+\tsub-moved/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules >actual &&\n+\tgit -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules >actual &&\n \ttest_cmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n-\tHEAD^:dir/sub/file:foobar\n-\tHEAD^:file:foobar\n+\tHEAD^:dir/sub/file:(1|2)d(3|4)\n+\tHEAD^:file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules HEAD^ >actual &&\n+\tgit -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep using relative path' '\n \ttest_when_finished \"rm -rf parent sub\" &&\n \tgit init sub &&\n-\techo \"foobar\" >sub/file &&\n+\techo \"(1|2)d(3|4)\" >sub/file &&\n \tgit -C sub add file &&\n \tgit -C sub commit -m \"add file\" &&\n \n \tgit init parent &&\n-\techo \"foobar\" >parent/file &&\n+\techo \"(1|2)d(3|4)\" >parent/file &&\n \tgit -C parent add file &&\n \tmkdir parent/src &&\n-\techo \"foobar\" >parent/src/file2 &&\n+\techo \"(1|2)d(3|4)\" >parent/src/file2 &&\n \tgit -C parent add src/file2 &&\n \tgit -C parent submodule add ../sub &&\n \tgit -C parent commit -m \"add files and submodule\" &&\n \n \t# From top works\n \tcat >expect <<-\\EOF &&\n-\tfile:foobar\n-\tsrc/file2:foobar\n-\tsub/file:foobar\n+\tfile:(1|2)d(3|4)\n+\tsrc/file2:(1|2)d(3|4)\n+\tsub/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\tgit -C parent grep --recurse-submodules -e \"(1|2)d(3|4)\" >actual &&\n \ttest_cmp expect actual &&\n \n \t# Relative path to top\n \tcat >expect <<-\\EOF &&\n-\t../file:foobar\n-\tfile2:foobar\n-\t../sub/file:foobar\n+\t../file:(1|2)d(3|4)\n+\tfile2:(1|2)d(3|4)\n+\t../sub/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n+\tgit -C parent/src grep --recurse-submodules -e \"(1|2)d(3|4)\" -- .. >actual &&\n \ttest_cmp expect actual &&\n \n \t# Relative path to submodule\n \tcat >expect <<-\\EOF &&\n-\t../sub/file:foobar\n+\t../sub/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n+\tgit -C parent/src grep --recurse-submodules -e \"(1|2)d(3|4)\" -- ../sub >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'grep from a subdir' '\n \ttest_when_finished \"rm -rf parent sub\" &&\n \tgit init sub &&\n-\techo \"foobar\" >sub/file &&\n+\techo \"(1|2)d(3|4)\" >sub/file &&\n \tgit -C sub add file &&\n \tgit -C sub commit -m \"add file\" &&\n \n \tgit init parent &&\n \tmkdir parent/src &&\n-\techo \"foobar\" >parent/src/file &&\n+\techo \"(1|2)d(3|4)\" >parent/src/file &&\n \tgit -C parent add src/file &&\n \tgit -C parent submodule add ../sub src/sub &&\n \tgit -C parent submodule add ../sub sub &&\n@@ -286,19 +286,19 @@ test_expect_success 'grep from a subdir' '\n \n \t# Verify grep from root works\n \tcat >expect <<-\\EOF &&\n-\tsrc/file:foobar\n-\tsrc/sub/file:foobar\n-\tsub/file:foobar\n+\tsrc/file:(1|2)d(3|4)\n+\tsrc/sub/file:(1|2)d(3|4)\n+\tsub/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\tgit -C parent grep --recurse-submodules -e \"(1|2)d(3|4)\" >actual &&\n \ttest_cmp expect actual &&\n \n \t# Verify grep from a subdir works\n \tcat >expect <<-\\EOF &&\n-\tfile:foobar\n-\tsub/file:foobar\n+\tfile:(1|2)d(3|4)\n+\tsub/file:(1|2)d(3|4)\n \tEOF\n-\tgit -C parent/src grep --recurse-submodules -e \"foobar\" >actual &&\n+\tgit -C parent/src grep --recurse-submodules -e \"(1|2)d(3|4)\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320338","messageId":"20170520214233.7183-10-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 09/30] grep: add tests for --threads=N and grep.threads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:12Z","receivedAt":"2017-05-20T21:43:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests for --threads=N being supplied on the command-line, or when\ngrep.threads=N being supplied in the configuration.\n\nWhen the threading support was made run-time configurable in commit\n89f09dd34e (\"grep: add --threads= option and grep.threads\nconfiguration\", 2015-12-15) no tests were added for it.\n\nIn developing a change to the grep code I was able to make\n'--threads=1 ` segfault, while the test suite still passed. This\nchange fixes that blind spot in the tests.\n\nIn addition to asserting that asking for N threads shouldn't segfault,\ntest that the grep output given any N is the same.\n\nThe choice to test only 1..10 as opposed to 1..8 or 1..16 or whatever\nis arbitrary. Testing 1..1024 works locally for me (but gets\nnoticeably slower as more threads are spawned). Given the structure of\nthe code there's no reason to test an arbitrary number of threads,\nonly 0, 1 and >=2 are special modes of operation.\n\nA later patch introduces a PTHREADS test prerequisite which is true\nunder NO_PTHREADS=UnfortunatelyYes, but even under NO_PTHREADS it's\nfine to test --threads=N, we'll just ignore it and not use\nthreading. So these tests also make sense under that mode to assert\nthat --threads=N without pthreads still returns expected results.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7810-grep.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex daa906b9b0..561709ef6a 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -775,6 +775,22 @@ test_expect_success 'grep -W with userdiff' '\n \ttest_cmp expected actual\n '\n \n+for threads in $(test_seq 0 10)\n+do\n+\ttest_expect_success \"grep --threads=$threads & -c grep.threads=$threads\" \"\n+\t\tgit grep --threads=$threads . >actual.$threads &&\n+\t\tif test $threads -ge 1\n+\t\tthen\n+\t\t\ttest_cmp actual.\\$(($threads - 1)) actual.$threads\n+\t\tfi &&\n+\t\tgit -c grep.threads=$threads grep . >actual.$threads &&\n+\t\tif test $threads -ge 1\n+\t\tthen\n+\t\t\ttest_cmp actual.\\$(($threads - 1)) actual.$threads\n+\t\tfi\n+\t\"\n+done\n+\n test_expect_success 'grep from a subdirectory to search wider area (1)' '\n \tmkdir -p s &&\n \t(\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320339","messageId":"20170520214233.7183-14-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 13/30] grep: prepare for testing binary regexes containing rx metacharacters","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:16Z","receivedAt":"2017-05-20T21:43:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add setup code needed for testing regexes that contain both binary\ndata and regex metacharacters.\n\nThe POSIX regcomp() function inherently can't support that, because it\ntakes a \\0-delimited char *, but other regex engines APIs like PCRE v2\ntake a pattern/length pair, and are thus able to handle \\0s in\npatterns as well as any other character.\n\nWhen kwset was imported in commit 9eceddeec6 (\"Use kwset in grep\",\n2011-08-21) this limitation was fixed, but at the expense of\nintroducing the undocumented limitation that any pattern containing \\0\nimplicitly becomes a fixed match (equivalent to -F having been\nprovided).\n\nThat's not something we'd like to keep in the future. The inability to\nmatch patterns containing \\0 is a leaky implementation detail.\n\nSo add tests as a first step towards changing that. In order to test\nthat \\0-patterns can properly match as regexes the test string needs\nto have some regex metacharacters in it.\n\nThere were other blind spots in the tests. The code around kwset\nspecially handles case-insensitive & non-ASCII data, but there were no\ntests for this.\n\nFix all of that by amending the text being matched to contain both\nregex metacharacters & non-ASCII data.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7008-grep-binary.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex df93d8e44c..20370d6e0c 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -28,7 +28,7 @@ nul_match () {\n }\n \n test_expect_success 'setup' \"\n-\techo 'binaryQfile' | q_to_nul >a &&\n+\techo 'binaryQfileQm[*]cQ*æQð' | q_to_nul >a &&\n \tgit add a &&\n \tgit commit -m.\n \"\n@@ -162,7 +162,7 @@ test_expect_success 'grep does not honor textconv' '\n '\n \n test_expect_success 'grep --textconv honors textconv' '\n-\techo \"a:binaryQfile\" >expect &&\n+\techo \"a:binaryQfileQm[*]cQ*æQð\" >expect &&\n \tgit grep --textconv Qfile >actual &&\n \ttest_cmp expect actual\n '\n@@ -172,7 +172,7 @@ test_expect_success 'grep --no-textconv does not honor textconv' '\n '\n \n test_expect_success 'grep --textconv blob honors textconv' '\n-\techo \"HEAD:a:binaryQfile\" >expect &&\n+\techo \"HEAD:a:binaryQfileQm[*]cQ*æQð\" >expect &&\n \tgit grep --textconv Qfile HEAD:a >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320340","messageId":"20170520214233.7183-12-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 11/30] grep: add tests for grep pattern types being passed to submodules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:14Z","receivedAt":"2017-05-20T21:43:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add testing for grep pattern types being correctly passed to\nsubmodules. The pattern \"(.|.)[\\d]\" matches differently under\nfixed (not at all), and then matches different lines under\nbasic/extended & perl regular expressions, so this change asserts that\nthe pattern type is passed along correctly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7814-grep-recurse-submodules.sh | 49 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 1472855e1d..3a58197f47 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -313,4 +313,53 @@ test_incompatible_with_recurse_submodules ()\n test_incompatible_with_recurse_submodules --untracked\n test_incompatible_with_recurse_submodules --no-index\n \n+test_expect_success 'grep --recurse-submodules should pass the pattern type along' '\n+\t# Fixed\n+\ttest_must_fail git grep -F --recurse-submodules -e \"(.|.)[\\d]\" &&\n+\ttest_must_fail git -c grep.patternType=fixed grep --recurse-submodules -e \"(.|.)[\\d]\" &&\n+\n+\t# Basic\n+\tgit grep -G --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)d(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit -c grep.patternType=basic grep --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Extended\n+\tgit grep -E --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules:[submodule \"submodule\"]\n+\t.gitmodules:\tpath = submodule\n+\t.gitmodules:\turl = ./submodule\n+\ta:(1|2)d(3|4)\n+\tsubmodule/.gitmodules:[submodule \"sub\"]\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit -c grep.patternType=extended grep --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c grep.extendedRegexp=true grep --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Perl\n+\tif test_have_prereq PCRE\n+\tthen\n+\t\tgit grep -P --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\ta:(1|2)d(3|4)\n+\t\tb/b:(3|4)\n+\t\tsubmodule/a:(1|2)d(3|4)\n+\t\tsubmodule/sub/a:(1|2)d(3|4)\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\tgit -c grep.patternType=perl grep --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n+\t\ttest_cmp expect actual\n+\tfi\n+'\n+\n test_done\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320341","messageId":"20170520214233.7183-15-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 14/30] grep: add tests to fix blind spots with \\0 patterns","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:17Z","receivedAt":"2017-05-20T21:43:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Address a big blind spot in the tests for patterns containing \\0. The\nis_fixed() function considers any string that contains \\0 fixed, even\nif it contains regular expression metacharacters, those patterns are\ncurrently matched with kwset.\n\nBefore this change removing that memchr(s, 0, len) check from\nis_fixed() wouldn't change the result of any of the tests, since\nregcomp() will happily match the part before the \\0.\n\nThe kwset path is dependent on whether the the -i flag is on, and\nwhether the pattern has any non-ASCII characters, but none of this was\ntested for.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7008-grep-binary.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 20370d6e0c..615e7e0162 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -22,6 +22,18 @@ nul_match () {\n \t\t\tprintf '$pattern' | q_to_nul >f &&\n \t\t\ttest_must_fail git grep -f f $flags a\n \t\t\"\n+\telif test \"$matches\" = T1\n+\tthen\n+\t\ttest_expect_failure \"git grep -f f $flags '$pattern_human' a\" \"\n+\t\t\tprintf '$pattern' | q_to_nul >f &&\n+\t\t\tgit grep -f f $flags a\n+\t\t\"\n+\telif test \"$matches\" = T0\n+\tthen\n+\t\ttest_expect_failure \"git grep -f f $flags '$pattern_human' a\" \"\n+\t\t\tprintf '$pattern' | q_to_nul >f &&\n+\t\t\ttest_must_fail git grep -f f $flags a\n+\t\t\"\n \telse\n \t\ttest_expect_success \"PANIC: Test framework error. Unknown matches value $matches\" 'false'\n \tfi\n@@ -98,6 +110,65 @@ nul_match 1 '-Fi' 'YQf'\n nul_match 0 '-Fi' 'YQx'\n nul_match 1 '' 'yQf'\n nul_match 0 '' 'yQx'\n+nul_match 1 '' 'æQð'\n+nul_match 1 '-F' 'eQm[*]c'\n+nul_match 1 '-Fi' 'EQM[*]C'\n+\n+# Regex patterns that would match but shouldn't with -F\n+nul_match 0 '-F' 'yQ[f]'\n+nul_match 0 '-F' '[y]Qf'\n+nul_match 0 '-Fi' 'YQ[F]'\n+nul_match 0 '-Fi' '[Y]QF'\n+nul_match 0 '-F' 'æQ[ð]'\n+nul_match 0 '-F' '[æ]Qð'\n+nul_match 0 '-Fi' 'ÆQ[Ð]'\n+nul_match 0 '-Fi' '[Æ]QÐ'\n+\n+# kwset is disabled on -i & non-ASCII. No way to match non-ASCII \\0\n+# patterns case-insensitively.\n+nul_match T1 '-i' 'ÆQÐ'\n+\n+# \\0 implicitly disables regexes. This is an undocumented internal\n+# limitation.\n+nul_match T1 '' 'yQ[f]'\n+nul_match T1 '' '[y]Qf'\n+nul_match T1 '-i' 'YQ[F]'\n+nul_match T1 '-i' '[Y]Qf'\n+nul_match T1 '' 'æQ[ð]'\n+nul_match T1 '' '[æ]Qð'\n+nul_match T1 '-i' 'ÆQ[Ð]'\n+\n+# ... because of \\0 implicitly disabling regexes regexes that\n+# should/shouldn't match don't do the right thing.\n+nul_match T1 '' 'eQm.*cQ'\n+nul_match T1 '-i' 'EQM.*cQ'\n+nul_match T0 '' 'eQm[*]c'\n+nul_match T0 '-i' 'EQM[*]C'\n+\n+# Due to the REG_STARTEND extension when kwset() is disabled on -i &\n+# non-ASCII the string will be matched in its entirety, but the\n+# pattern will be cut off at the first \\0.\n+nul_match 0 '-i' 'NOMATCHQð'\n+nul_match T0 '-i' '[Æ]QNOMATCH'\n+nul_match T0 '-i' '[æ]QNOMATCH'\n+# Matches, but for the wrong reasons, just stops at [æ]\n+nul_match 1 '-i' '[Æ]Qð'\n+nul_match 1 '-i' '[æ]Qð'\n+\n+# Ensure that the matcher doesn't regress to something that stops at\n+# \\0\n+nul_match 0 '-F' 'yQ[f]'\n+nul_match 0 '-Fi' 'YQ[F]'\n+nul_match 0 '' 'yQNOMATCH'\n+nul_match 0 '' 'QNOMATCH'\n+nul_match 0 '-i' 'YQNOMATCH'\n+nul_match 0 '-i' 'QNOMATCH'\n+nul_match 0 '-F' 'æQ[ð]'\n+nul_match 0 '-Fi' 'ÆQ[Ð]'\n+nul_match 0 '' 'yQNÓMATCH'\n+nul_match 0 '' 'QNÓMATCH'\n+nul_match 0 '-i' 'YQNÓMATCH'\n+nul_match 0 '-i' 'QNÓMATCH'\n \n test_expect_success 'grep respects binary diff attribute' '\n \techo text >t &&\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320342","messageId":"20170520214233.7183-16-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 15/30] perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:18Z","receivedAt":"2017-05-20T21:43:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a git GIT_PERF_MAKE_COMMAND variable to compliment the existing\nGIT_PERF_MAKE_OPTS facility. This allows specifying an arbitrary shell\ncommand to execute instead of 'make'.\n\nThis is useful e.g. in cases where the name, semantics or defaults of\na Makefile flag have changed over time. It can even be used to change\nthe contents of the tree, useful for monkeypatching ancient versions\nof git to get them to build.\n\nThis opens Pandora's box in some ways, it's now possible to\n\"jailbreak\" the perf environment and e.g. modify the source tree via\nthis arbitrary instead of just issuing a custom \"make\" command, such a\ncommand has to be re-entrant in the sense that subsequent perf runs\nwill re-use the possibly modified tree.\n\nIt would be pointless to try to mitigate or work around that caveat in\na tool purely aimed at Git developers, so this change makes no attempt\nto do so.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 3 +++\n t/perf/README | 19 +++++++++++++++++--\n t/perf/run | 11 +++++++++--\n 3 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex eedadb8056..d1587452f1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2272,6 +2272,9 @@ endif\n ifdef GIT_PERF_MAKE_OPTS\n \t@echo GIT_PERF_MAKE_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_MAKE_OPTS)))'\\' >>$@+\n endif\n+ifdef GIT_PERF_MAKE_COMMAND\n+\t@echo GIT_PERF_MAKE_COMMAND=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_MAKE_COMMAND)))'\\' >>$@+\n+endif\n ifdef GIT_INTEROP_MAKE_OPTS\n \t@echo GIT_INTEROP_MAKE_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_INTEROP_MAKE_OPTS)))'\\' >>$@+\n endif\ndiff --git a/t/perf/README b/t/perf/README\nindex 49ea4349be..b3d95042a8 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak):\n \n GIT_PERF_MAKE_OPTS\n \tOptions to use when automatically building a git tree for\n-\tperformance testing. E.g., -j6 would be useful.\n-\n+\tperformance testing. E.g., -j6 would be useful. Passed\n+\tdirectly to make as \"make $GIT_PERF_MAKE_OPTS\".\n+\n+ GIT_PERF_MAKE_COMMAND\n+\tAn arbitrary command that'll be run in place of the make\n+\tcommand, if set the GIT_PERF_MAKE_OPTS variable is\n+\tignored. Useful in cases where source tree changes might\n+\trequire issuing a different make command to different\n+\trevisions.\n+\n+\tThis can be (ab)used to monkeypatch or otherwise change the\n+\ttree about to be built. Note that the build directory can be\n+\tre-used for subsequent runs so the make command might get\n+\texecuted multiple times on the same tree, but don't count on\n+\tany of that, that's an implementation detail that might change\n+\tin the future.\n+ \n GIT_PERF_REPO\n GIT_PERF_LARGE_REPO\n \tRepositories to copy for the performance tests. The normal\ndiff --git a/t/perf/run b/t/perf/run\nindex c788d713ae..b61024a830 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -37,8 +37,15 @@ build_git_rev () {\n \t\t\tcp \"../../$config\" \"build/$rev/\"\n \t\tfi\n \tdone\n-\t(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||\n-\tdie \"failed to build revision '$mydir'\"\n+\t(\n+\t\tcd build/$rev &&\n+\t\tif test -n \"$GIT_PERF_MAKE_COMMAND\"\n+\t\tthen\n+\t\t\tsh -c \"$GIT_PERF_MAKE_COMMAND\"\n+\t\telse\n+\t\t\tmake $GIT_PERF_MAKE_OPTS\n+\t\tfi\n+\t) || die \"failed to build revision '$mydir'\"\n }\n \n run_dirs_helper () {\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320343","messageId":"20170520214233.7183-19-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 18/30] perf: add a comparison test of grep regex engines with -F","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:21Z","receivedAt":"2017-05-20T21:43:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a performance comparison test which compares both case-sensitive &\ncase-insensitive fixed-string grep, as well as non-ASCII\ncase-sensitive & case-insensitive grep.\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux ./run p7821-grep-engines-fixed.sh\n [...]\n Test this tree\n ------------------------------------------------\n 7821.1: fixed grep int 0.61(1.72+0.65)\n 7821.2: basic grep int 0.69(1.72+0.53)\n 7821.3: extended grep int 0.60(1.72+0.54)\n 7821.4: perl grep int 0.65(1.65+0.64)\n 7821.6: fixed grep uncommon 0.25(0.53+0.48)\n 7821.7: basic grep uncommon 0.26(0.57+0.46)\n 7821.8: extended grep uncommon 0.25(0.52+0.51)\n 7821.9: perl grep uncommon 0.26(0.56+0.48)\n 7821.11: fixed grep æ 0.40(1.26+0.44)\n 7821.12: basic grep æ 0.40(1.28+0.43)\n 7821.13: extended grep æ 0.39(1.28+0.44)\n 7821.14: perl grep æ 0.39(1.29+0.44)\n\nThis test needs to be run with GIT_PERF_7821_GREP_OPTS=' -i' to avoid\ngoing through the same kwset.[ch] codepath, see the \"Even when -F...\"\ncomment in grep.c:\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_7821_GREP_OPTS=' -i' ./run p7821-grep-engines-fixed.sh\n [...]\n Test this tree\n ---------------------------------------------------\n 7821.1: fixed grep -i int 1.55(1.86+0.66)\n 7821.2: basic grep -i int 0.66(1.97+0.54)\n 7821.3: extended grep -i int 0.72(1.88+0.62)\n 7821.4: perl grep -i int 0.75(1.93+0.57)\n 7821.6: fixed grep -i uncommon 0.27(0.52+0.54)\n 7821.7: basic grep -i uncommon 0.25(0.58+0.44)\n 7821.8: extended grep -i uncommon 0.26(0.62+0.43)\n 7821.9: perl grep -i uncommon 0.26(0.55+0.53)\n 7821.11: fixed grep -i æ 0.32(0.87+0.46)\n 7821.12: basic grep -i æ 0.30(0.90+0.41)\n 7821.13: extended grep -i æ 0.32(0.92+0.41)\n 7821.14: perl grep -i æ 0.29(0.71+0.53)\n\nI'm planning to make that not be the case, this performance test gives\na baseline for comparing performance before & after any such change.\n\nSee commit (\"perf: add a comparison test of grep regex engines\",\n2017-04-19) for details on the machine the above test run was executed\non.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p7821-grep-engines-fixed.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n create mode 100755 t/perf/p7821-grep-engines-fixed.sh\n\ndiff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh\nnew file mode 100755\nindex 0000000000..d935194ecf\n--- /dev/null\n+++ b/t/perf/p7821-grep-engines-fixed.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description=\"Comparison of git-grep's regex engines with -F\n+\n+Set GIT_PERF_7821_GREP_OPTS in the environment to pass options to\n+git-grep. Make sure to include a leading space,\n+e.g. GIT_PERF_7821_GREP_OPTS=' -w'. See p7820-grep-engines.sh for more\n+options to try.\n+\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+for args in 'int' 'uncommon' 'æ'\n+do\n+\tfor engine in fixed basic extended perl\n+\tdo\n+\t\ttest_perf \"$engine grep$GIT_PERF_7821_GREP_OPTS $args\" \"\n+\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $args >'out.$engine.$args' || :\n+\t\t\"\n+\tdone\n+\n+\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $args\" \"\n+\t\ttest_cmp 'out.fixed.$args' 'out.basic.$args' &&\n+\t\ttest_cmp 'out.fixed.$args' 'out.extended.$args' &&\n+\t\ttest_cmp 'out.fixed.$args' 'out.perl.$args'\n+\t\"\n+done\n+\n+test_done\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320344","messageId":"20170520214233.7183-21-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 20/30] grep: catch a missing enum in switch statement","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:23Z","receivedAt":"2017-05-20T21:44:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a die(...) to a default case for the switch statement selecting\nbetween grep pattern types under --recurse-submodules.\n\nNormally this would be caught by -Wswitch, but the grep_pattern_type\ntype is converted to int by going through parse_options(). Changing\nthe argument type passed to compile_submodule_options() won't work,\nthe value will just get coerced. The -Wswitch-default warning will\nwarn about it, but that produces a lot of noise across the codebase,\nthis potential issue would be drowned in that noise.\n\nThus catching this at runtime is the least bad option. This won't ever\ntrigger in practice, but if a new pattern type were to be added this\ncatches an otherwise silent bug during development.\n\nSee commit 0281e487fd (\"grep: optionally recurse into submodules\",\n2016-12-16) for the initial addition of this code.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ffb5b4e81..a191e2976b 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -495,6 +495,8 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \t\tbreak;\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n \t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: Added a new grep pattern type without updating switch statement\");\n \t}\n \n \tfor (pattern = opt->pattern_list; pattern != NULL;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320345","messageId":"20170520214233.7183-18-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 17/30] perf: add a comparison test of grep regex engines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:20Z","receivedAt":"2017-05-20T21:44:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a very basic performance comparison test comparing the POSIX\nbasic, extended and perl engines.\n\nIn theory the \"basic\" and \"extended\" engines should be implemented\nusing the same underlying code with a slightly different pattern\nparser, but some implementations may not do this. Jump through some\nslight hoops to test both, which is worthwhile since \"basic\" is the\ndefault.\n\nRunning this on an i7 3.4GHz Linux 4.9.0-2 Debian testing against a\ncheckout of linux.git & latest upstream PCRE, both PCRE and git\ncompiled with -O3 using gcc 7.1.1:\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux ./run p7820-grep-engines.sh\n [...]\n Test this tree\n ---------------------------------------------------------------\n 7820.1: basic grep 'how.to' 0.34(1.24+0.53)\n 7820.2: extended grep 'how.to' 0.33(1.23+0.45)\n 7820.3: perl grep 'how.to' 0.31(1.05+0.56)\n 7820.5: basic grep '^how to' 0.32(1.24+0.42)\n 7820.6: extended grep '^how to' 0.33(1.20+0.44)\n 7820.7: perl grep '^how to' 0.57(2.67+0.42)\n 7820.9: basic grep '[how] to' 0.51(2.16+0.45)\n 7820.10: extended grep '[how] to' 0.49(2.20+0.43)\n 7820.11: perl grep '[how] to' 0.56(2.60+0.43)\n 7820.13: basic grep '\\(e.t[^ ]*\\|v.ry\\) rare' 0.66(3.25+0.40)\n 7820.14: extended grep '(e.t[^ ]*|v.ry) rare' 0.65(3.19+0.46)\n 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 1.05(5.74+0.34)\n 7820.17: basic grep 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 0.34(1.28+0.47)\n 7820.18: extended grep 'm(ú|u)lt.b(æ|y)te' 0.34(1.38+0.38)\n 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.39(1.56+0.44)\n\nOptions can also be passed to git-grep via the GIT_PERF_7820_GREP_OPTS\nenvironment variable. There are various modes such as \"-v\" that have\nvery different performance profiles, but handling the combinatorial\nexplosion of testing all those options would make this script much\nmore complex and harder to maintain. Instead just add the ability to\ndo one-shot runs with arbitrary options, e.g.:\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_7820_GREP_OPTS=\" -i\" ./run p7820-grep-engines.sh\n [...]\n Test this tree\n ------------------------------------------------------------------\n 7820.1: basic grep -i 'how.to' 0.49(1.72+0.38)\n 7820.2: extended grep -i 'how.to' 0.46(1.64+0.42)\n 7820.3: perl grep -i 'how.to' 0.44(1.45+0.45)\n 7820.5: basic grep -i '^how to' 0.47(1.76+0.38)\n 7820.6: extended grep -i '^how to' 0.47(1.70+0.42)\n 7820.7: perl grep -i '^how to' 0.65(2.72+0.37)\n 7820.9: basic grep -i '[how] to' 0.86(3.64+0.42)\n 7820.10: extended grep -i '[how] to' 0.84(3.62+0.46)\n 7820.11: perl grep -i '[how] to' 0.73(3.06+0.39)\n 7820.13: basic grep -i '\\(e.t[^ ]*\\|v.ry\\) rare' 1.63(8.13+0.36)\n 7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 1.64(8.01+0.44)\n 7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.44(6.88+0.44)\n 7820.17: basic grep -i 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 0.66(2.67+0.44)\n 7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 0.66(2.67+0.43)\n 7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.59(2.31+0.37)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p7820-grep-engines.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n create mode 100755 t/perf/p7820-grep-engines.sh\n\ndiff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh\nnew file mode 100755\nindex 0000000000..a3a1b9fa28\n--- /dev/null\n+++ b/t/perf/p7820-grep-engines.sh\n@@ -0,0 +1,47 @@\n+#!/bin/sh\n+\n+test_description=\"Comparison of git-grep's regex engines\n+\n+Set GIT_PERF_7820_GREP_OPTS in the environment to pass options to\n+git-grep. Make sure to include a leading space,\n+e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:\n+\n+\t-i\n+\t-w\n+\t-v\n+\t-vi\n+\t-vw\n+\t-viw\n+\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+for pattern in \\\n+\t'how.to' \\\n+\t'^how to' \\\n+\t'[how] to' \\\n+\t'\\(e.t[^ ]*\\|v.ry\\) rare' \\\n+\t'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te'\n+do\n+\tfor engine in basic extended perl\n+\tdo\n+\t\tif test $engine != \"basic\"\n+\t\tthen\n+\t\t\t# Poor man's basic -> extended converter.\n+\t\t\tpattern=$(echo \"$pattern\" | sed 's/\\\\//g')\n+\t\tfi\n+\t\ttest_perf \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n+\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :\n+\t\t\"\n+\tdone\n+\n+\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n+\t\ttest_cmp 'out.basic' 'out.extended' &&\n+\t\ttest_cmp 'out.basic' 'out.perl'\n+\t\"\n+done\n+\n+test_done\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320346","messageId":"20170520214233.7183-20-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 19/30] perf: add a comparison test of log --grep regex engines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:22Z","receivedAt":"2017-05-20T21:44:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a very basic performance comparison test comparing the POSIX\nbasic, extended and perl engines with patterns matching log messages\nvia --grep=.\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux ./run p4220-log-grep-engines.sh\n [...]\n Test this tree\n ---------------------------------------------------------------------\n 4220.1: basic log --grep='how.to' 6.22(6.00+0.21)\n 4220.2: extended log --grep='how.to' 6.23(5.98+0.23)\n 4220.3: perl log --grep='how.to' 6.07(5.79+0.25)\n 4220.5: basic log --grep='^how to' 6.19(5.93+0.22)\n 4220.6: extended log --grep='^how to' 6.19(5.93+0.23)\n 4220.7: perl log --grep='^how to' 6.14(5.88+0.24)\n 4220.9: basic log --grep='[how] to' 6.96(6.65+0.28)\n 4220.10: extended log --grep='[how] to' 6.96(6.69+0.24)\n 4220.11: perl log --grep='[how] to' 6.95(6.58+0.33)\n 4220.13: basic log --grep='\\(e.t[^ ]*\\|v.ry\\) rare' 7.10(6.80+0.27)\n 4220.14: extended log --grep='(e.t[^ ]*|v.ry) rare' 7.07(6.80+0.26)\n 4220.15: perl log --grep='(e.t[^ ]*|v.ry) rare' 7.70(7.46+0.22)\n 4220.17: basic log --grep='m\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 6.12(5.87+0.24)\n 4220.18: extended log --grep='m(ú|u)lt.b(æ|y)te' 6.14(5.84+0.26)\n 4220.19: perl log --grep='m(ú|u)lt.b(æ|y)te' 6.16(5.93+0.20)\n\nWith -i:\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_4220_LOG_OPTS=' -i' ./run p4220-log-grep-engines.sh\n [...]\n Test this tree\n ------------------------------------------------------------------------\n 4220.1: basic log -i --grep='how.to' 6.74(6.41+0.32)\n 4220.2: extended log -i --grep='how.to' 6.78(6.55+0.22)\n 4220.3: perl log -i --grep='how.to' 6.06(5.77+0.28)\n 4220.5: basic log -i --grep='^how to' 6.80(6.57+0.22)\n 4220.6: extended log -i --grep='^how to' 6.83(6.52+0.29)\n 4220.7: perl log -i --grep='^how to' 6.16(5.94+0.20)\n 4220.9: basic log -i --grep='[how] to' 7.87(7.61+0.24)\n 4220.10: extended log -i --grep='[how] to' 7.85(7.57+0.27)\n 4220.11: perl log -i --grep='[how] to' 7.03(6.75+0.25)\n 4220.13: basic log -i --grep='\\(e.t[^ ]*\\|v.ry\\) rare' 8.68(8.41+0.25)\n 4220.14: extended log -i --grep='(e.t[^ ]*|v.ry) rare' 8.80(8.44+0.28)\n 4220.15: perl log -i --grep='(e.t[^ ]*|v.ry) rare' 7.85(7.56+0.26)\n 4220.17: basic log -i --grep='m\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 6.94(6.68+0.24)\n 4220.18: extended log -i --grep='m(ú|u)lt.b(æ|y)te' 7.04(6.76+0.24)\n 4220.19: perl log -i --grep='m(ú|u)lt.b(æ|y)te' 6.26(5.92+0.29)\n\nSee commit (\"perf: add a comparison test of grep regex engines\",\n2017-04-19) for details on the machine the above test run was executed\non.\n\nBefore commit (\"log: make --regexp-ignore-case work with\n--perl-regexp\", 2017-05-20) this test will almost definitely\nfail (depending on the repo) if passed the -i option, since it wasn't\nproperly supported under PCRE.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p4220-log-grep-engines.sh | 44 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 44 insertions(+)\n create mode 100755 t/perf/p4220-log-grep-engines.sh\n\ndiff --git a/t/perf/p4220-log-grep-engines.sh b/t/perf/p4220-log-grep-engines.sh\nnew file mode 100755\nindex 0000000000..02793ac77b\n--- /dev/null\n+++ b/t/perf/p4220-log-grep-engines.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+\n+test_description=\"Comparison of git-log's --grep regex engines\n+\n+Set GIT_PERF_4220_LOG_OPTS in the environment to pass options to\n+git-grep. Make sure to include a leading space,\n+e.g. GIT_PERF_4220_LOG_OPTS=' -i'. Some options to try:\n+\n+\t-i\n+\t--invert-grep\n+\t-i --invert-grep\n+\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+for pattern in \\\n+\t'how.to' \\\n+\t'^how to' \\\n+\t'[how] to' \\\n+\t'\\(e.t[^ ]*\\|v.ry\\) rare' \\\n+\t'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te'\n+do\n+\tfor engine in basic extended perl\n+\tdo\n+\t\tif test $engine != \"basic\"\n+\t\tthen\n+\t\t\t# Poor man's basic -> extended converter.\n+\t\t\tpattern=$(echo $pattern | sed 's/\\\\//g')\n+\t\tfi\n+\t\ttest_perf \"$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'\" \"\n+\t\t\tgit -c grep.patternType=$engine log --pretty=format:%h$GIT_PERF_4220_LOG_OPTS --grep='$pattern' >'out.$engine' || :\n+\t\t\"\n+\tdone\n+\n+\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_4220_LOG_OPTS '$pattern'\" \"\n+\t\ttest_cmp 'out.basic' 'out.extended' &&\n+\t\ttest_cmp 'out.basic' 'out.perl'\n+\t\"\n+done\n+\n+test_done\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320347","messageId":"20170520214233.7183-17-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 16/30] perf: emit progress output when unpacking & building","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:19Z","receivedAt":"2017-05-20T21:44:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the t/perf/run output so that in addition to the \"Running N\ntests\" heading currently being emitted, it also emits \"Unpacking $rev\"\nand \"Building $rev\" when setting up the build/$rev directory & when\nbuilding it, respectively.\n\nThis makes it easier to see what's going on and what revision is being\ntested as the output scrolls by.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/run | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex b61024a830..beb4acc0e4 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -24,6 +24,7 @@ run_one_dir () {\n \n unpack_git_rev () {\n \trev=$1\n+\techo \"=== Unpacking $rev in build/$rev ===\"\n \tmkdir -p build/$rev\n \t(cd \"$(git rev-parse --show-cdup)\" && git archive --format=tar $rev) |\n \t(cd build/$rev && tar x)\n@@ -37,6 +38,7 @@ build_git_rev () {\n \t\t\tcp \"../../$config\" \"build/$rev/\"\n \t\tfi\n \tdone\n+\techo \"=== Building $rev ===\"\n \t(\n \t\tcd build/$rev &&\n \t\tif test -n \"$GIT_PERF_MAKE_COMMAND\"\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320348","messageId":"20170520214233.7183-22-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 21/30] grep: remove redundant regflags assignments","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:24Z","receivedAt":"2017-05-20T21:44:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove redundant assignments to the \"regflags\" variable. This variable\nis only used set under GREP_PATTERN_TYPE_ERE, so there's no need to\nun-set it under GREP_PATTERN_TYPE_{FIXED,BRE,PCRE}.\n\nBack in 5010cb5fcc[1], we did do \"opt.regflags &= ~REG_EXTENDED\" upon\nseeing \"-G\" on the command line and flipped the bit on upon seeing\n\"-E\", but I think that was perfectly sensible and it would have been a\nbug if we didn't. They were part of the command line parsing that\ncould have seen \"-E\" on the command line earlier.\n\nWhen cca2c172 (\"git-grep: do not die upon -F/-P when\ngrep.extendedRegexp is set.\", 2011-05-09) switched the command line\nparsing to \"read into a 'tentatively this is what we saw the last'\nvariable and then finally commit just once\", we didn't touch\nopt.regflags for PCRE and FIXED, but we still had to flip regflags\nbetween BRE and ERE, because parsing of grep.extendedregexp\nconfiguration variable directly touched opt.regflags back then, which\nwas done by b22520a3 (\"grep: allow -E and -n to be turned on by\ndefault via configuration\", 2011-03-30).\n\nWhen 84befcd0 (\"grep: add a grep.patternType configuration setting\",\n2012-08-03) introduced extended_regexp_option field, we stopped\nflipping regflags while reading the configuration, and that was when\nwe should have noticed and stopped dropping REG_EXTENDED bit in the\n\"now we can commit what type to use\" helper function.\n\nThere is no reason to do this anymore, so stop doing it, more to\nreduce \"wait this is used under fixed/BRE/PCRE how?\" confusion when\nreading the code, than to to save ourselves trivial CPU cycles by\nremoving one assignment.\n\n1. \"built-in \"git grep\"\", 2006-04-30.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 47cee45067..bf6c2494fd 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -179,7 +179,6 @@ 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->pcre = 0;\n-\t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n@@ -191,13 +190,11 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\topt->fixed = 1;\n \t\topt->pcre = 0;\n-\t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \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@@ -415,10 +412,9 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint err;\n-\tint regflags;\n+\tint regflags = opt->regflags;\n \n \tbasic_regex_quote_buf(&sb, p->pattern);\n-\tregflags = opt->regflags & ~REG_EXTENDED;\n \tif (opt->ignore_case)\n \t\tregflags |= REG_ICASE;\n \terr = regcomp(&p->regexp, sb.buf, regflags);\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320349","messageId":"20170520214233.7183-23-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 22/30] grep: factor test for \\0 in grep patterns into a function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:25Z","receivedAt":"2017-05-20T21:44:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Factor the test for \\0 in grep patterns into a function. Since commit\n9eceddeec6 (\"Use kwset in grep\", 2011-08-21) any pattern containing a\n\\0 is considered fixed as regcomp() can't handle it.\n\nThis change makes later changes that make use of either has_null() or\nis_fixed() (but not both) smaller.\n\nWhile I'm at it make the comment conform to the style guide, i.e. add\nan opening \"/*\\n\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex bf6c2494fd..79eb681c6e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -321,6 +321,18 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p,\n \tdie(\"%s'%s': %s\", where, p->pattern, error);\n }\n \n+static int has_null(const char *s, size_t len)\n+{\n+\t/*\n+\t * regcomp cannot accept patterns with NULs so when using it\n+\t * we consider any pattern containing a NUL fixed.\n+\t */\n+\tif (memchr(s, 0, len))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n #ifdef USE_LIBPCRE\n static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n {\n@@ -394,12 +406,6 @@ static int is_fixed(const char *s, size_t len)\n {\n \tsize_t i;\n \n-\t/* regcomp cannot accept patterns with NULs so we\n-\t * consider any pattern containing a NUL fixed.\n-\t */\n-\tif (memchr(s, 0, len))\n-\t\treturn 1;\n-\n \tfor (i = 0; i < len; i++) {\n \t\tif (is_regex_special(s[i]))\n \t\t\treturn 0;\n@@ -451,7 +457,7 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t * simple string match using kws. p->fixed tells us if we\n \t * want to use kws.\n \t */\n-\tif (opt->fixed || is_fixed(p->pattern, p->patternlen))\n+\tif (opt->fixed || has_null(p->pattern, p->patternlen) || is_fixed(p->pattern, p->patternlen))\n \t\tp->fixed = !icase || ascii_only;\n \telse\n \t\tp->fixed = 0;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320350","messageId":"20170520214233.7183-26-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 25/30] grep: move is_fixed() earlier to avoid forward declaration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:28Z","receivedAt":"2017-05-20T21:44:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_fixed() function which are currently only used in\ncompile_regexp() earlier so it can be used in the PCRE family of\nfunctions in a later change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 07512346b1..1157529115 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -321,6 +321,18 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p,\n \tdie(\"%s'%s': %s\", where, p->pattern, error);\n }\n \n+static int is_fixed(const char *s, size_t len)\n+{\n+\tsize_t i;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (is_regex_special(s[i]))\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n static int has_null(const char *s, size_t len)\n {\n \t/*\n@@ -402,18 +414,6 @@ static void free_pcre1_regexp(struct grep_pat *p)\n }\n #endif /* !USE_LIBPCRE1 */\n \n-static int is_fixed(const char *s, size_t len)\n-{\n-\tsize_t i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tif (is_regex_special(s[i]))\n-\t\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320351","messageId":"20170520214233.7183-24-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 23/30] grep: change the internal PCRE macro names to be PCRE1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:26Z","receivedAt":"2017-05-20T21:44:11Z","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 d1587452f1..374fbc7e58 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1088,7 +1088,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@@ -2240,7 +2240,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 79eb681c6e..854f2404be 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -333,7 +333,7 @@ static int has_null(const char *s, size_t len)\n \treturn 0;\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@@ -385,7 +385,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@@ -400,7 +400,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 04d857a42b..1d0f636cbd 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1014,7 +1014,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.13.0.303.g4ebf302169\n\n"},{"id":"320352","messageId":"20170520214233.7183-25-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 24/30] grep: change internal *pcre* variable & function names to be *pcre1*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:27Z","receivedAt":"2017-05-20T21:44:13Z","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\nAn earlier change in this series (\"grep: change the internal PCRE\nmacro names to be PCRE1\", 2017-04-07) elaborates on the motivations\nbehind this change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 52 ++++++++++++++++++++++++++--------------------------\n grep.h | 8 ++++----\n 2 files changed, 30 insertions(+), 30 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 854f2404be..07512346b1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -178,23 +178,23 @@ 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\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\tbreak;\n \n \tcase GREP_PATTERN_TYPE_PCRE:\n \t\topt->fixed = 0;\n-\t\topt->pcre = 1;\n+\t\topt->pcre1 = 1;\n \t\tbreak;\n \t}\n }\n@@ -334,7 +334,7 @@ static int has_null(const char *s, size_t len)\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@@ -342,23 +342,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@@ -366,7 +366,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@@ -379,25 +379,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@@ -477,8 +477,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@@ -834,8 +834,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@@ -914,8 +914,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..38ac82b638 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@@ -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;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320353","messageId":"20170520214233.7183-27-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 26/30] test-lib: add a PTHREADS prerequisite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:29Z","receivedAt":"2017-05-20T21:44:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a PTHREADS prerequisite which is false when git is compiled with\nNO_PTHREADS=YesPlease.\n\nThere's lots of custom code that runs when threading isn't available,\nbut before this prerequisite there was no way to test it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 1 +\n t/README | 4 ++++\n t/test-lib.sh | 1 +\n 3 files changed, 6 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 374fbc7e58..a79274e5e6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2242,6 +2242,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_EXPAT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_EXPAT)))'\\' >>$@+\n \t@echo USE_LIBPCRE1=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE)))'\\' >>$@+\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n+\t@echo NO_PTHREADS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PTHREADS)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n \t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\ndiff --git a/t/README b/t/README\nindex a90cb62583..2f95860369 100644\n--- a/t/README\n+++ b/t/README\n@@ -817,6 +817,10 @@ use these, and \"test_set_prereq\" for how to define your own.\n Test is run on a filesystem which converts decomposed utf-8 (nfd)\n to precomposed utf-8 (nfc).\n \n+ - PTHREADS\n+\n+ Git wasn't compiled with NO_PTHREADS=YesPlease.\n+\n Tips for Writing Tests\n ----------------------\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1d0f636cbd..43529451f9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1013,6 +1013,7 @@ esac\n \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_PTHREADS\" && test_set_prereq PTHREADS\n test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n test -n \"$USE_LIBPCRE1\" && test_set_prereq PCRE\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320354","messageId":"20170520214233.7183-29-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 28/30] pack-objects: fix buggy warning about threads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:31Z","receivedAt":"2017-05-20T21:44:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a buggy warning about threads under NO_PTHREADS=YesPlease. Due to\nre-using the delta_search_threads variable for both the state of the\n\"pack.threads\" config & the --threads option, setting \"pack.threads\"\nbut not supplying --threads would trigger the warning for both\n\"pack.threads\" & --threads.\n\nSolve this bug by resetting the delta_search_threads variable in\ngit_pack_config(), it might then be set by --threads again and be\nsubsequently warned about, as the test I'm changing here asserts.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 4 +++-\n t/t5300-pack-object.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9b4ba8a80d..efa21a15dd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2472,8 +2472,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\t\tdie(\"invalid number of threads specified (%d)\",\n \t\t\t delta_search_threads);\n #ifdef NO_PTHREADS\n-\t\tif (delta_search_threads != 1)\n+\t\tif (delta_search_threads != 1) {\n \t\t\twarning(\"no threads support, ignoring %s\", k);\n+\t\t\tdelta_search_threads = 0;\n+\t\t}\n #endif\n \t\treturn 0;\n \t}\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 6ed23ee1d2..9c68b99251 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -447,7 +447,7 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack.\n \n \tgit -c pack.threads=2 pack-objects --stdout --all /dev/null 2>err &&\n \tgrep ^warning: err >warnings &&\n-\ttest_must_fail test_line_count = 1 warnings &&\n+\ttest_line_count = 1 warnings &&\n \tgrep -F \"no threads support, ignoring pack.threads\" err &&\n \n \tgit -c pack.threads=2 pack-objects --threads=4 --stdout --all /dev/null 2>err &&\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320355","messageId":"20170520214233.7183-31-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 30/30] grep: assert that threading is enabled when calling grep_{lock,unlock}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:33Z","receivedAt":"2017-05-20T21:44:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the grep_{lock,unlock} functions to assert that num_threads is\ntrue, instead of only locking & unlocking the pthread mutex lock when\nit is.\n\nThese functions are never called when num_threads isn't true, this\nlogic has gone through multiple iterations since the initial\nintroduction of grep threading in commit 5b594f457a (\"Threaded grep\",\n2010-01-25), but ever since then they'd only be called if num_threads\nwas true, so this check made the code confusing to read.\n\nReplace the check with an assertion, so that it's clear to the reader\nthat this code path is never taken unless we're spawning threads.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3c721b75a5..b1095362fb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -73,14 +73,14 @@ static pthread_mutex_t grep_mutex;\n \n static inline void grep_lock(void)\n {\n-\tif (num_threads)\n-\t\tpthread_mutex_lock(&grep_mutex);\n+\tassert(num_threads);\n+\tpthread_mutex_lock(&grep_mutex);\n }\n \n static inline void grep_unlock(void)\n {\n-\tif (num_threads)\n-\t\tpthread_mutex_unlock(&grep_mutex);\n+\tassert(num_threads);\n+\tpthread_mutex_unlock(&grep_mutex);\n }\n \n /* Signalled when a new work_item is added to todo. */\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320356","messageId":"20170520214233.7183-30-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 29/30] grep: given --threads with NO_PTHREADS=YesPlease, warn","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:32Z","receivedAt":"2017-05-20T21:44:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a warning about missing thread support when grep.threads or\n--threads is set to a non 0 (default) or 1 (no parallelism) value\nunder NO_PTHREADS=YesPlease.\n\nThis is for consistency with the index-pack & pack-objects commands,\nwhich also take a --threads option & are configurable via\npack.threads, and have long warned about the same under\nNO_PTHREADS=YesPlease.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 13 +++++++++++++\n t/t7810-grep.sh | 18 ++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex a191e2976b..3c721b75a5 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -289,6 +289,17 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n \t\tif (num_threads < 0)\n \t\t\tdie(_(\"invalid number of threads specified (%d) for %s\"),\n \t\t\t num_threads, var);\n+#ifdef NO_PTHREADS\n+\t\telse if (num_threads && num_threads != 1) {\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: %s is the configuration\n+\t\t\t * variable for tweaking threads, currently\n+\t\t\t * grep.threads\n+\t\t\t */\n+\t\t\twarning(_(\"no threads support, ignoring %s\"), var);\n+\t\t\tnum_threads = 0;\n+\t\t}\n+#endif\n \t}\n \n \treturn st;\n@@ -1229,6 +1240,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \telse if (num_threads < 0)\n \t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\n #else\n+\tif (num_threads)\n+\t\twarning(_(\"no threads support, ignoring --threads\"));\n \tnum_threads = 0;\n #endif\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 561709ef6a..f106387820 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -791,6 +791,24 @@ do\n \t\"\n done\n \n+test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'grep --threads=N or pack.threads=N warns when no pthreads' '\n+\tgit grep --threads=2 Hello hello_world 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads\" err &&\n+\tgit -c grep.threads=2 grep Hello hello_world 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring grep.threads\" err &&\n+\tgit -c grep.threads=2 grep --threads=4 Hello hello_world 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads\" err &&\n+\tgrep -F \"no threads support, ignoring grep.threads\" err &&\n+\tgit -c grep.threads=0 grep --threads=0 Hello hello_world 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n test_expect_success 'grep from a subdirectory to search wider area (1)' '\n \tmkdir -p s &&\n \t(\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320357","messageId":"20170520214233.7183-28-avarab@gmail.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"[PATCH v3 27/30] pack-objects & index-pack: add test for --threads warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-20T21:42:30Z","receivedAt":"2017-05-20T21:44:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a test for the warning that's emitted when --threads or\npack.threads is provided under NO_PTHREADS=YesPlease. This uses the\nnew PTHREADS prerequisite.\n\nThe assertion for C_LOCALE_OUTPUT in the latter test is currently\nredundant, since unlike index-pack the pack-objects warnings aren't\ni18n'd. However they might be changed to be i18n'd in the future, and\nthere's no harm in future-proofing the test.\n\nThere's an existing bug in the implementation of pack-objects which\nthis test currently tests for as-is. Details about the bug & the fix\nare included in a follow-up change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5300-pack-object.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 43a672c345..6ed23ee1d2 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -421,6 +421,42 @@ test_expect_success 'index-pack works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n+test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n+\ttest_must_fail git index-pack --threads=2 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads=2\" err &&\n+\n+\ttest_must_fail git -c pack.threads=2 index-pack 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring pack.threads\" err &&\n+\n+\ttest_must_fail git -c pack.threads=2 index-pack --threads=4 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads=4\" err &&\n+\tgrep -F \"no threads support, ignoring pack.threads\" err\n+'\n+\n+test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n+\tgit pack-objects --threads=2 --stdout --all /dev/null 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads\" err &&\n+\n+\tgit -c pack.threads=2 pack-objects --stdout --all /dev/null 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_must_fail test_line_count = 1 warnings &&\n+\tgrep -F \"no threads support, ignoring pack.threads\" err &&\n+\n+\tgit -c pack.threads=2 pack-objects --threads=4 --stdout --all /dev/null 2>err &&\n+\tgrep ^warning: err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\tgrep -F \"no threads support, ignoring --threads\" err &&\n+\tgrep -F \"no threads support, ignoring pack.threads\" err\n+'\n+\n #\n # WARNING!\n #\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320362","messageId":"xmqqshjzqe9o.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170520214233.7183-1-avarab@gmail.com","subject":"Re: [PATCH v3 00/30] Easy to review grep & pre-PCRE changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-20T23:50:27Z","receivedAt":"2017-05-20T23:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Easy to review? 29 (I mean 30) patches? Are you kidding me?!\n>\n> As noted in v1 (<20170511091829.5634-1-avarab@gmail.com>;\n> https://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)\n> these are all doc, test, refactoring etc. changes needed by the\n> subsequent \"PCRE v2, PCRE v1 JIT, log -P & fixes\" series.\n>\n> Since Junio hasn't been picking it I'm no longer sending updates to\n> that patch series & waiting for this one to cook first.\n\nI actually do not mind a reroll that goes together with this. The\nonly reason why I skipped the earlier one was because I looked at\nthe original one, and the discussion on the reroll of this 'easy to\nreview' part indicated that it will be rerolled, before I got to\nlook at these upper layer patches.\n\nOverall nicely done. I only had just a few observations.\n\nThanks.\n"},{"id":"320363","messageId":"xmqqlgprqe9j.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170520214233.7183-6-avarab@gmail.com","subject":"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with --perl-regexp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-20T23:50:32Z","receivedAt":"2017-05-20T23:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Make the --regexp-ignore-case option work with --perl-regexp. This\n> never worked, and there was no test for this. Fix the bug and add a\n> test.\n>\n> When PCRE support was added in commit 63e7e9d8b6 (\"git-grep: Learn\n> PCRE\", 2011-05-09) compile_pcre_regexp() would only check\n> opt->ignore_case, but when the --perl-regexp option was added in\n> commit 727b6fc3ed (\"log --grep: accept --basic-regexp and\n> --perl-regexp\", 2012-10-03) the code didn't set the opt->ignore_case.\n>\n> Change the test suite to test for -i and --invert-regexp with\n> basic/extended/perl patterns in addition to fixed, which was the only\n> patternType that was tested for before in combination with those\n> options.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> revision.c | 1 +\n> t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n> 2 files changed, 56 insertions(+), 5 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 8a8c1789c7..4883cdd2d0 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1991,6 +1991,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t} else if (!strcmp(arg, \"--extended-regexp\") || !strcmp(arg, \"-E\")) {\n> \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n> \t} else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n> +\t\trevs->grep_filter.ignore_case = 1;\n> \t\trevs->grep_filter.regflags |= REG_ICASE;\n> \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n> \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n\nLooks good.\n\nI however wonder if it is a better approach in the longer term to\ntreat the .ignore_case field just like .extended_regexp_option\nfield, i.e. not committing immediately to .regflags but commit it\nafter config and command line parsing is done, just like we make the\n\"BRE? ERE?\" decision in grep_commit_pattern_type().\n\nThanks.\n"},{"id":"320364","messageId":"xmqqefvjqe9f.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170520214233.7183-16-avarab@gmail.com","subject":"Re: [PATCH v3 15/30] perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-20T23:50:36Z","receivedAt":"2017-05-20T23:50:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/perf/README b/t/perf/README\n> index 49ea4349be..b3d95042a8 100644\n> --- a/t/perf/README\n> +++ b/t/perf/README\n> @@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak):\n> \n> GIT_PERF_MAKE_OPTS\n> \tOptions to use when automatically building a git tree for\n> -\tperformance testing. E.g., -j6 would be useful.\n> -\n> +...\n> +\tany of that, that's an implementation detail that might change\n> +\tin the future.\n> + \n\nI'll remove the trailing whitespace on this otherwise blank line\nwhile queuing (no need to resend only to fix this one).\n\nThanks.\n\n> GIT_PERF_REPO\n> GIT_PERF_LARGE_REPO\n> \tRepositories to copy for the performance tests. The normal\n> diff --git a/t/perf/run b/t/perf/run\n> index c788d713ae..b61024a830 100755\n> --- a/t/perf/run\n> +++ b/t/perf/run\n> @@ -37,8 +37,15 @@ build_git_rev () {\n> \t\t\tcp \"../../$config\" \"build/$rev/\"\n> \t\tfi\n> \tdone\n> -\t(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||\n> -\tdie \"failed to build revision '$mydir'\"\n> +\t(\n> +\t\tcd build/$rev &&\n> +\t\tif test -n \"$GIT_PERF_MAKE_COMMAND\"\n> +\t\tthen\n> +\t\t\tsh -c \"$GIT_PERF_MAKE_COMMAND\"\n> +\t\telse\n> +\t\t\tmake $GIT_PERF_MAKE_OPTS\n> +\t\tfi\n> +\t) || die \"failed to build revision '$mydir'\"\n> }\n> \n> run_dirs_helper () {\n"},{"id":"320368","messageId":"CACBZZX5mmhQzhJPbE0wwXY8T6+i7NynmdBFXr6Ym1U=6tCtPwQ@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqefvjqe9f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 15/30] perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-21T06:23:57Z","receivedAt":"2017-05-21T06:24:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 21, 2017 at 1:50 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/t/perf/README b/t/perf/README\n>> index 49ea4349be..b3d95042a8 100644\n>> --- a/t/perf/README\n>> +++ b/t/perf/README\n>> @@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak):\n>>\n>> GIT_PERF_MAKE_OPTS\n>> Options to use when automatically building a git tree for\n>> - performance testing. E.g., -j6 would be useful.\n>> -\n>> +...\n>> + any of that, that's an implementation detail that might change\n>> + in the future.\n>> +\n>\n> I'll remove the trailing whitespace on this otherwise blank line\n> while queuing (no need to resend only to fix this one).\n>\n> Thanks.\n\nThanks, forgot about diff --check on the whole series with all the\nother checks I was doing.\n\n>> GIT_PERF_REPO\n>> GIT_PERF_LARGE_REPO\n>> Repositories to copy for the performance tests. The normal\n>> diff --git a/t/perf/run b/t/perf/run\n>> index c788d713ae..b61024a830 100755\n>> --- a/t/perf/run\n>> +++ b/t/perf/run\n>> @@ -37,8 +37,15 @@ build_git_rev () {\n>> cp \"../../$config\" \"build/$rev/\"\n>> fi\n>> done\n>> - (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||\n>> - die \"failed to build revision '$mydir'\"\n>> + (\n>> + cd build/$rev &&\n>> + if test -n \"$GIT_PERF_MAKE_COMMAND\"\n>> + then\n>> + sh -c \"$GIT_PERF_MAKE_COMMAND\"\n>> + else\n>> + make $GIT_PERF_MAKE_OPTS\n>> + fi\n>> + ) || die \"failed to build revision '$mydir'\"\n>> }\n>>\n>> run_dirs_helper () {\n"},{"id":"320369","messageId":"CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqlgprqe9j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with --perl-regexp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-21T06:58:24Z","receivedAt":"2017-05-21T06:58:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 21, 2017 at 1:50 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Make the --regexp-ignore-case option work with --perl-regexp. This\n>> never worked, and there was no test for this. Fix the bug and add a\n>> test.\n>>\n>> When PCRE support was added in commit 63e7e9d8b6 (\"git-grep: Learn\n>> PCRE\", 2011-05-09) compile_pcre_regexp() would only check\n>> opt->ignore_case, but when the --perl-regexp option was added in\n>> commit 727b6fc3ed (\"log --grep: accept --basic-regexp and\n>> --perl-regexp\", 2012-10-03) the code didn't set the opt->ignore_case.\n>>\n>> Change the test suite to test for -i and --invert-regexp with\n>> basic/extended/perl patterns in addition to fixed, which was the only\n>> patternType that was tested for before in combination with those\n>> options.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> revision.c | 1 +\n>> t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n>> 2 files changed, 56 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index 8a8c1789c7..4883cdd2d0 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -1991,6 +1991,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>> } else if (!strcmp(arg, \"--extended-regexp\") || !strcmp(arg, \"-E\")) {\n>> revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n>> } else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n>> + revs->grep_filter.ignore_case = 1;\n>> revs->grep_filter.regflags |= REG_ICASE;\n>> DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n>> } else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n>\n> Looks good.\n>\n> I however wonder if it is a better approach in the longer term to\n> treat the .ignore_case field just like .extended_regexp_option\n> field, i.e. not committing immediately to .regflags but commit it\n> after config and command line parsing is done, just like we make the\n> \"BRE? ERE?\" decision in grep_commit_pattern_type().\n\nI started hacking up a patch to fix the root cause of this, i.e. the\nusers of the grep API should only set `.ignore_case = 1` and not care\nabout setting regflags, but it was more than a trivial change, so I\ndidn't include it in this series:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ffb5b4e81..be28c37265 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1151,8 +1151,6 @@ int cmd_grep(int argc, const char **argv, const\nchar *prefix)\n\n if (!opt.pattern_list)\n die(_(\"no pattern given.\"));\n- if (!opt.fixed && opt.ignore_case)\n- opt.regflags |= REG_ICASE;\n\n compile_grep_patterns(&opt);\n\ndiff --git a/grep.c b/grep.c\nindex 47cee45067..7b13ee1043 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -435,12 +435,11 @@ static void compile_fixed_regexp(struct grep_pat\n*p, struct grep_opt *opt)\n\n static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n- int icase, ascii_only;\n+ int ascii_only;\n int err;\n\n p->word_regexp = opt->word_regexp;\n p->ignore_case = opt->ignore_case;\n- icase = opt->regflags & REG_ICASE || p->ignore_case;\n ascii_only = !has_non_ascii(p->pattern);\n\n /*\n@@ -456,12 +455,12 @@ static void compile_regexp(struct grep_pat *p,\nstruct grep_opt *opt)\n * want to use kws.\n */\n if (opt->fixed || is_fixed(p->pattern, p->patternlen))\n- p->fixed = !icase || ascii_only;\n+ p->fixed = !p->ignore_case || ascii_only;\n else\n p->fixed = 0;\n\n if (p->fixed) {\n- p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n+ p->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);\n kwsincr(p->kws, p->pattern, p->patternlen);\n kwsprep(p->kws);\n return;\n@@ -480,6 +479,8 @@ static void compile_regexp(struct grep_pat *p,\nstruct grep_opt *opt)\n return;\n }\n\n+ if (p->ignore_case)\n+ opt->regflags |= REG_ICASE;\n err = regcomp(&p->regexp, p->pattern, opt->regflags);\n if (err) {\n char errbuf[1024];\ndiff --git a/revision.c b/revision.c\nindex 4883cdd2d0..30c23a1098 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1992,7 +1992,6 @@ static int handle_revision_opt(struct rev_info\n*revs, int argc, const char **arg\n revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n } else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n revs->grep_filter.ignore_case = 1;\n- revs->grep_filter.regflags |= REG_ICASE;\n DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n } else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n\nBut an even better solution is to get rid of passing the regflags\nfield in grep_opt entirely, this conflicts with some of my later\npatches:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ffb5b4e81..be28c37265 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1151,8 +1151,6 @@ int cmd_grep(int argc, const char **argv, const\nchar *prefix)\n\n if (!opt.pattern_list)\n die(_(\"no pattern given.\"));\n- if (!opt.fixed && opt.ignore_case)\n- opt.regflags |= REG_ICASE;\n\n compile_grep_patterns(&opt);\ndiff --git a/grep.c b/grep.c\nindex 47cee45067..1bde7037ba 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -34,7 +34,6 @@ void init_grep_defaults(void)\n memset(opt, 0, sizeof(*opt));\n opt->relative = 1;\n opt->pathname = 1;\n- opt->regflags = REG_NEWLINE;\n opt->max_depth = -1;\n opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n opt->extended_regexp_option = 0;\n@@ -156,7 +155,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n opt->linenum = def->linenum;\n opt->max_depth = def->max_depth;\n opt->pathname = def->pathname;\n- opt->regflags = def->regflags;\n opt->relative = def->relative;\n opt->output = def->output;\n\n@@ -179,25 +177,25 @@ static void grep_set_pattern_type_option(enum\ngrep_pattern_type pattern_type, st\n case GREP_PATTERN_TYPE_BRE:\n opt->fixed = 0;\n opt->pcre = 0;\n- opt->regflags &= ~REG_EXTENDED;\n+ opt->extended = 0;\n break;\n case GREP_PATTERN_TYPE_ERE:\n opt->fixed = 0;\n opt->pcre = 0;\n- opt->regflags |= REG_EXTENDED;\n+ opt->extended = 1;\n break;\n\n case GREP_PATTERN_TYPE_FIXED:\n opt->fixed = 1;\n opt->pcre = 0;\n- opt->regflags &= ~REG_EXTENDED;\n+ opt->extended = 0;\n break;\n\n case GREP_PATTERN_TYPE_PCRE:\n opt->fixed = 0;\n opt->pcre = 1;\n- opt->regflags &= ~REG_EXTENDED;\n+ opt->extended = 0;\n break;\n }\n }\n@@ -415,10 +413,9 @@ static void compile_fixed_regexp(struct grep_pat\n*p, struct grep_opt *opt)\n {\n struct strbuf sb = STRBUF_INIT;\n int err;\n- int regflags;\n+ int regflags = REG_NEWLINE;\n\n basic_regex_quote_buf(&sb, p->pattern);\n- regflags = opt->regflags & ~REG_EXTENDED;\n if (opt->ignore_case)\n regflags |= REG_ICASE;\n err = regcomp(&p->regexp, sb.buf, regflags);\n@@ -435,12 +432,12 @@ static void compile_fixed_regexp(struct grep_pat\n*p, struct grep_opt *opt)\n\n static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n- int icase, ascii_only;\n+ int ascii_only;\n int err;\n+ int regflags = REG_NEWLINE;\n\n p->word_regexp = opt->word_regexp;\n p->ignore_case = opt->ignore_case;\n- icase = opt->regflags & REG_ICASE || p->ignore_case;\n ascii_only = !has_non_ascii(p->pattern);\n\n /*\n@@ -456,12 +453,12 @@ static void compile_regexp(struct grep_pat *p,\nstruct grep_opt *opt)\n * want to use kws.\n */\n if (opt->fixed || is_fixed(p->pattern, p->patternlen))\n- p->fixed = !icase || ascii_only;\n+ p->fixed = !p->ignore_case || ascii_only;\n else\n p->fixed = 0;\n\n if (p->fixed) {\n- p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n+ p->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);\n kwsincr(p->kws, p->pattern, p->patternlen);\n kwsprep(p->kws);\n return;\n@@ -480,7 +477,11 @@ static void compile_regexp(struct grep_pat *p,\nstruct grep_opt *opt)\n return;\n }\n\n- err = regcomp(&p->regexp, p->pattern, opt->regflags);\n+ if (p->ignore_case)\n+ regflags |= REG_ICASE;\n+ if (opt->extended)\n+ regflags |= REG_EXTENDED;\n+ err = regcomp(&p->regexp, p->pattern, regflags);\n if (err) {\n char errbuf[1024];\n regerror(err, &p->regexp, errbuf, 1024);\ndiff --git a/grep.h b/grep.h\nindex 267534ca24..d9d603deb1 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -129,7 +129,6 @@ struct grep_opt {\n char color_match_selected[COLOR_MAXLEN];\n char color_selected[COLOR_MAXLEN];\n char color_sep[COLOR_MAXLEN];\n- int regflags;\n unsigned pre_context;\n unsigned post_context;\n unsigned last_shown;\ndiff --git a/revision.c b/revision.c\nindex 4883cdd2d0..67240d38af 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const\nchar *prefix)\n init_grep_defaults();\n grep_init(&revs->grep_filter, prefix);\n revs->grep_filter.status_only = 1;\n- revs->grep_filter.regflags = REG_NEWLINE;\n\n diff_setup(&revs->diffopt);\n if (prefix && !revs->diffopt.prefix) {\n@@ -1992,7 +1991,6 @@ static int handle_revision_opt(struct rev_info\n*revs, int argc, const char **arg\n revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n } else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n revs->grep_filter.ignore_case = 1;\n- revs->grep_filter.regflags |= REG_ICASE;\n DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n } else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n\nBut as all this code cleanup isn't needed for fixing this bug, and I'd\nreally like to get this series merged into next/master ASAP so I can\nstart submitting the grep/pcre patches that are actually interesting,\nlet's leave this orthogonal code cleanup for now.\n"},{"id":"320382","messageId":"xmqqa865rbgx.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com","subject":"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with --perl-regexp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-22T00:17:50Z","receivedAt":"2017-05-22T00:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Looks good.\n>>\n>> I however wonder if it is a better approach in the longer term to\n>> treat the .ignore_case field just like .extended_regexp_option\n>> field, i.e. not committing immediately to .regflags but commit it\n>> after config and command line parsing is done, just like we make the\n>> \"BRE? ERE?\" decision in grep_commit_pattern_type().\n>\n> I started hacking up a patch to fix the root cause of this, i.e. the\n> users of the grep API should only set `.ignore_case = 1` and not care\n> about setting regflags, but it was more than a trivial change, so I\n> didn't include it in this series:\n\nAh, sorry. Now I re-read my response, \"Looks good. I however\nwonder...\" does sound like I am requesting to do something more to\nsolve the same issue. I shouldn't have phrased it that way. It was\nmore like \"while I was staring the context lines of your patch, I\nnoticed this tangent\", nothing more.\n\n> ...\n> But an even better solution is to get rid of passing the regflags\n> field in grep_opt entirely, this conflicts with some of my later\n> patches:\n\nYes, that may be a good direction to go in longer term, but let's\nnot push it before the other bits already in flight land safely.\n\n> ...\n> But as all this code cleanup isn't needed for fixing this bug, and I'd\n> really like to get this series merged into next/master ASAP so I can\n> start submitting the grep/pcre patches that are actually interesting,\n> let's leave this orthogonal code cleanup for now.\n\nYes. Thanks.\n"},{"id":"320575","messageId":"20170523192453.14172-1-avarab@gmail.com","threadId":"46011","inReplyTo":"xmqqshjzqe9o.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 0/7] PCRE v2, PCRE v1 JIT, log -P & fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:46Z","receivedAt":"2017-05-23T19:25:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 21, 2017 at 1:50 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason   writes:\n>\n>> Easy to review? 29 (I mean 30) patches? Are you kidding me?!\n>>\n>> As noted in v1 (<20170511091829.5634-1-avarab@gmail.com>;\n>> https://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)\n>> these are all doc, test, refactoring etc. changes needed by the\n>> subsequent \"PCRE v2, PCRE v1 JIT, log -P & fixes\" series.\n>>\n>> Since Junio hasn't been picking it I'm no longer sending updates to\n>> that patch series & waiting for this one to cook first.\n>\n> I actually do not mind a reroll that goes together with this.  The\n> only reason why I skipped the earlier one was because I looked at\n> the original one, and the discussion on the reroll of this 'easy to\n> review' part indicated that it will be rerolled, before I got to\n> look at these upper layer patches.\n\nGreat, now that the base of this is migrating to next, here's the\nsecond part of this.\n\nFor v1 see <20170513234535.12749-1-avarab@gmail.com>\n(https://public-inbox.org/git/20170513234535.12749-1-avarab@gmail.com/).\n\nThe only changes to the content are better if/else branching around\nconditional macros (but no functional changes) in the PCRE v1 JIT API\npatch in response to a comment by Simon Ruderich.\n\nThe only other changes are trivial updates to the commit messages to\naccount for t/perf changes made in the series this builds on.\n\nÆvar Arnfjörð Bjarmason (7):\n grep: don't redundantly compile throwaway patterns under threading\n grep: skip pthreads overhead when using one thread\n log: add -P as a synonym for --perl-regexp\n grep: add support for the PCRE v1 JIT API\n grep: un-break building with PCRE < 8.32\n grep: un-break building with PCRE < 8.20\n grep: add support for PCRE v2\n\n Documentation/rev-list-options.txt | 1 +\n Makefile | 30 +++++--\n builtin/grep.c | 16 +++-\n configure.ac | 77 +++++++++++++---\n grep.c | 177 ++++++++++++++++++++++++++++++++++++-\n grep.h | 31 +++++++\n revision.c | 2 +-\n t/t4202-log.sh | 12 +++\n t/test-lib.sh | 2 +-\n 9 files changed, 324 insertions(+), 24 deletions(-)\n\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320576","messageId":"20170523192453.14172-4-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 3/7] log: add -P as a synonym for --perl-regexp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:49Z","receivedAt":"2017-05-23T19:25:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a short -P option as a synonym for the longer --perl-regexp, for\nconsistency with the options the corresponding grep invocations\naccept.\n\nThis was intentionally omitted in commit 727b6fc3ed (\"log --grep:\naccept --basic-regexp and --perl-regexp\", 2012-10-03) for unspecified\nfuture use.\n\nMake it consistent with \"grep\" rather than to keep it open for future\nuse, and to avoid the confusion of -P meaning different things for\ngrep & log, as is the case with the -G option.\n\nAs noted in the aforementioned commit the --basic-regexp option can't\nhave a corresponding -G argument, as the log command already uses that\nfor -G.\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 | 12 ++++++++++++\n 3 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a46f70c2b1..9c44eae55d 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\n \texpressions.\ndiff --git a/revision.c b/revision.c\nindex 4883cdd2d0..60329da1bd 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\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 dbed3efeee..2b07d1c0c2 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -404,8 +404,20 @@ 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=\"[\\d]\\|\" >actual.perl.short-arg\n+\t\telse\n+\t\t\ttest_must_fail git log -P \\\n+\t\t\t\t--grep=\"[\\d]\\|\"\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.13.0.303.g4ebf302169\n\n"},{"id":"320577","messageId":"20170523192453.14172-6-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 5/7] grep: un-break building with PCRE < 8.32","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:51Z","receivedAt":"2017-05-23T19:25:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend my change earlier in this series (\"grep: add support for the\nPCRE v1 JIT API\", 2017-04-11) to un-break the build on PCRE v1\nversions earlier than 8.32.\n\nThe JIT support was added in version 8.20 released on 2011-10-21, but\nit wasn't until 8.32 released on 2012-11-30 that the fast code path to\nuse the JIT via pcre_jit_exec() was added[1] (see also [2]).\n\nThis means that versions 8.20 through 8.31 could still use the JIT,\nbut supporting it on those versions would add to the already verbose\nmacro soup around JIT support it, and I don't expect that the use-case\nof compiling a brand new git against a 5 year old PCRE is particularly\ncommon, and if someone does that they can just get the existing\npre-JIT slow codepath.\n\nSo just take the easy way out and disable the JIT on any version older\nthan 8.32.\n\nThe reason this change isn't part of the initial change PCRE JIT\nsupport is because possibly slightly annoying someone who's bisecting\nwith an ancient PCRE is worth it to have a cleaner history showing\nwhich parts of the implementation are only used for ancient PCRE\nversions. This also makes it easier to revert this change if we ever\ndecide to stop supporting those old versions.\n\n1. http://www.pcre.org/original/changelog.txt (\"28. Introducing a\n native interface for JIT. Through this interface, the\n compiled[...]\")\n2. https://bugs.exim.org/show_bug.cgi?id=2121\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 8 ++++----\n grep.h | 5 +++++\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 49e9aed457..3c0c30f033 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -351,7 +351,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tconst char *error;\n \tint erroffset;\n \tint options = PCRE_MULTILINE;\n-#ifdef PCRE_CONFIG_JIT\n+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\n \tint canjit;\n #endif\n \n@@ -372,7 +372,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tif (!p->pcre1_extra_info && error)\n \t\tdie(\"%s\", error);\n \n-#ifdef PCRE_CONFIG_JIT\n+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\n \tpcre_config(PCRE_CONFIG_JIT, &canjit);\n \tif (canjit == 1) {\n \t\tp->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);\n@@ -392,7 +392,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\n \n-#ifdef PCRE_CONFIG_JIT\n+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\n \tif (p->pcre1_jit_on)\n \t\tret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,\n \t\t\t\t eol - line, 0, flags, ovector,\n@@ -418,7 +418,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n {\n \tpcre_free(p->pcre1_regexp);\n \n-#ifdef PCRE_CONFIG_JIT\n+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\n \tif (p->pcre1_jit_on) {\n \t\tpcre_free_study(p->pcre1_extra_info);\n \t\tpcre_jit_stack_free(p->pcre1_jit_stack);\ndiff --git a/grep.h b/grep.h\nindex 14f47189f9..73ef0ef8ec 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -3,6 +3,11 @@\n #include \"color.h\"\n #ifdef USE_LIBPCRE1\n #include \n+#ifdef PCRE_CONFIG_JIT\n+#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n+#define GIT_PCRE1_CAN_DO_MODERN_JIT\n+#endif\n+#endif\n #ifndef PCRE_STUDY_JIT_COMPILE\n #define PCRE_STUDY_JIT_COMPILE 0\n #endif\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320578","messageId":"20170523192453.14172-7-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 6/7] grep: un-break building with PCRE < 8.20","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:52Z","receivedAt":"2017-05-23T19:25:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend my change earlier in this series (\"grep: add support for the\nPCRE v1 JIT API\", 2017-04-11) to un-break the build on PCRE v1\nversions earlier than 8.20.\n\nThe 8.20 release was the first release to have JIT & pcre_jit_stack in\nthe headers, so a mock type needs to be provided for it on those\nreleases.\n\nNow git should compile with all PCRE versions that it supported before\nmy JIT change.\n\nI've tested it as far back as version 7.5 released on 2008-01-10, once\nI got down to version 7.0 it wouldn't build anymore with GCC 7.1.1,\nand I couldn't be bothered to anything older than 7.5 as I'm confident\nthat if the build breaks on those older versions it's not because of\nmy JIT change.\n\nSee the \"un-break\" change in this series (\"grep: un-break building\nwith PCRE < 8.32\", 2017-05-10) for why this isn't squashed into the\nmain PCRE JIT commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/grep.h b/grep.h\nindex 73ef0ef8ec..b7b9d487b0 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -11,6 +11,9 @@\n #ifndef PCRE_STUDY_JIT_COMPILE\n #define PCRE_STUDY_JIT_COMPILE 0\n #endif\n+#if PCRE_MAJOR <= 8 && PCRE_MINOR < 20\n+typedef int pcre_jit_stack;\n+#endif\n #else\n typedef int pcre;\n typedef int pcre_extra;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320579","messageId":"20170523192453.14172-3-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 2/7] grep: skip pthreads overhead when using one thread","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:48Z","receivedAt":"2017-05-23T19:25:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Skip the administrative overhead of using pthreads when only using one\nthread. Instead take the non-threaded path which would be taken under\nNO_PTHREADS.\n\nThe threading support was initially added in commit\n5b594f457a (\"Threaded grep\", 2010-01-25) with a hardcoded compile-time\nnumber of 8 threads. Later the number of threads was made configurable\nin commit 89f09dd34e (\"grep: add --threads= option and\ngrep.threads configuration\", 2015-12-15).\n\nThat change did not add any special handling for --threads=1. Now we\ntake a slightly faster path by skipping thread handling entirely when\n1 thread is requested.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 12e62fcbf3..bd008cb100 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1238,6 +1238,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\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);\n+\tif (num_threads == 1)\n+\t\tnum_threads = 0;\n #else\n \tif (num_threads)\n \t\twarning(_(\"no threads support, ignoring --threads\"));\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320580","messageId":"20170523192453.14172-5-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 4/7] grep: add support for the PCRE v1 JIT API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:50Z","receivedAt":"2017-05-23T19:25:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the grep PCRE v1 code to use JIT when available. When PCRE\nsupport was initially added in commit 63e7e9d8b6 (\"git-grep: Learn\nPCRE\", 2011-05-09) PCRE had no JIT support, it was integrated into\n8.20 released on 2011-10-21.\n\nEnabling JIT support usually improves performance by more than\n40%. The pattern compilation times are relatively slower, but those\nrelative numbers are tiny, and are easily made back in all but the\nmost trivial cases of grep. Detailed benchmarks & overview of\ncompilation times is at: http://sljit.sourceforge.net/pcre.html\n\nWith this change the difference in a t/perf/p7820-grep-engines.sh run\nis, with just the /perl/ tests shown:\n\n $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~ HEAD p7820-grep-engines.sh\n Test HEAD~ HEAD\n ---------------------------------------------------------------------------------------\n 7820.3: perl grep 'how.to' 0.35(1.11+0.43) 0.23(0.42+0.46) -34.3%\n 7820.7: perl grep '^how to' 0.64(2.71+0.36) 0.27(0.66+0.44) -57.8%\n 7820.11: perl grep '[how] to' 0.63(2.51+0.42) 0.33(0.98+0.39) -47.6%\n 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 1.17(5.61+0.35) 0.34(1.08+0.46) -70.9%\n 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.43(1.52+0.44) 0.30(0.88+0.42) -30.2%\n\nThe conditional support for JIT is implemented as suggested in the\npcrejit(3) man page. E.g. defining PCRE_STUDY_JIT_COMPILE to 0 if it's\nnot present.\n\nThe implementation is relatively verbose because even if\nPCRE_CONFIG_JIT is defined only a call to pcre_config() can determine\nif the JIT is available, and if so the faster pcre_jit_exec() function\nshould be called instead of pcre_exec(), and a different (but not\ncomplimentary!) function needs to be called to free pcre1_extra_info.\n\nThere's no graceful fallback if pcre_jit_stack_alloc() fails under\nPCRE_CONFIG_JIT, instead the program will simply abort. I don't think\nthis is worth handling gracefully, it'll only fail in cases where\nmalloc() doesn't work, in which case we're screwed anyway.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 34 +++++++++++++++++++++++++++++++++-\n grep.h | 6 ++++++\n 2 files changed, 39 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 1157529115..49e9aed457 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -351,6 +351,9 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tconst char *error;\n \tint erroffset;\n \tint options = PCRE_MULTILINE;\n+#ifdef PCRE_CONFIG_JIT\n+\tint canjit;\n+#endif\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern))\n@@ -365,9 +368,20 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tif (!p->pcre1_regexp)\n \t\tcompile_regexp_failed(p, error);\n \n-\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, 0, &error);\n+\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);\n \tif (!p->pcre1_extra_info && error)\n \t\tdie(\"%s\", error);\n+\n+#ifdef PCRE_CONFIG_JIT\n+\tpcre_config(PCRE_CONFIG_JIT, &canjit);\n+\tif (canjit == 1) {\n+\t\tp->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);\n+\t\tif (!p->pcre1_jit_stack)\n+\t\t\tdie(\"BUG: Couldn't allocate PCRE JIT stack\");\n+\t\tpcre_assign_jit_stack(p->pcre1_extra_info, NULL, p->pcre1_jit_stack);\n+\t\tp->pcre1_jit_on = 1;\n+\t}\n+#endif\n }\n \n static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n@@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\n \n+#ifdef PCRE_CONFIG_JIT\n+\tif (p->pcre1_jit_on)\n+\t\tret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,\n+\t\t\t\t eol - line, 0, flags, ovector,\n+\t\t\t\t ARRAY_SIZE(ovector), p->pcre1_jit_stack);\n+\telse\n+#endif\n+\t/* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n \tret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,\n \t\t\t0, flags, ovector, ARRAY_SIZE(ovector));\n+\n \tif (ret < 0 && ret != PCRE_ERROR_NOMATCH)\n \t\tdie(\"pcre_exec failed with error code %d\", ret);\n \tif (ret > 0) {\n@@ -394,7 +417,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n static void free_pcre1_regexp(struct grep_pat *p)\n {\n \tpcre_free(p->pcre1_regexp);\n+\n+#ifdef PCRE_CONFIG_JIT\n+\tif (p->pcre1_jit_on) {\n+\t\tpcre_free_study(p->pcre1_extra_info);\n+\t\tpcre_jit_stack_free(p->pcre1_jit_stack);\n+\t} else\n+#endif\n+\t/* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n \tpcre_free(p->pcre1_extra_info);\n+\n \tpcre_free((void *)p->pcre1_tables);\n }\n #else /* !USE_LIBPCRE1 */\ndiff --git a/grep.h b/grep.h\nindex 38ac82b638..14f47189f9 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -3,9 +3,13 @@\n #include \"color.h\"\n #ifdef USE_LIBPCRE1\n #include \n+#ifndef PCRE_STUDY_JIT_COMPILE\n+#define PCRE_STUDY_JIT_COMPILE 0\n+#endif\n #else\n typedef int pcre;\n typedef int pcre_extra;\n+typedef int pcre_jit_stack;\n #endif\n #include \"kwset.h\"\n #include \"thread-utils.h\"\n@@ -48,7 +52,9 @@ struct grep_pat {\n \tregex_t regexp;\n \tpcre *pcre1_regexp;\n \tpcre_extra *pcre1_extra_info;\n+\tpcre_jit_stack *pcre1_jit_stack;\n \tconst unsigned char *pcre1_tables;\n+\tint pcre1_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n \tunsigned ignore_case:1;\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320581","messageId":"20170523192453.14172-8-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 7/7] grep: add support for PCRE v2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:53Z","receivedAt":"2017-05-23T19:26:04Z","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].\n\nThe regular expression syntax is the same, but while the API is\nsimilar, pretty much every function is either renamed or takes\ndifferent arguments. Thus using it via entirely new functions makes\nsense, as opposed to trying to e.g. have one compile_pcre_pattern()\nthat would call either PCRE v1 or v2 functions.\n\nGit can now be compiled with either USE_LIBPCRE1=YesPlease or\nUSE_LIBPCRE2=YesPlease, with USE_LIBPCRE=YesPlease currently being a\nsynonym for the former. Providing both is a compile-time error.\n\nWith earlier patches to enable JIT for PCRE v1 the performance of the\nrelease versions of both libraries is almost exactly the same, with\nPCRE v2 being around 1% slower.\n\nHowever after I reported this to the pcre-dev mailing list[2] I got a\nlot of help with the API use from Zoltán Herczeg, he subsequently\noptimized some of the JIT functionality in v2 of the library.\n\nRunning the p7820-grep-engines.sh performance test against the latest\nSubversion trunk of both, with both them and git compiled as -O3, and\nthe test run against linux.git, gives the following results. Just the\n/perl/ tests shown:\n\n $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_COMMAND='grep -q LIBPCRE2 Makefile && make -j8 USE_LIBPCRE2=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre2/inst/lib || make -j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~2 HEAD~ HEAD p7820-grep-engines.sh\n [...]\n Test HEAD~2 HEAD~ HEAD\n ----------------------------------------------------------------------------------------------------------------\n 7820.3: perl grep 'how.to' 0.22(0.40+0.48) 0.22(0.31+0.58) +0.0% 0.22(0.26+0.59) +0.0%\n 7820.7: perl grep '^how to' 0.27(0.62+0.50) 0.28(0.60+0.50) +3.7% 0.22(0.25+0.60) -18.5%\n 7820.11: perl grep '[how] to' 0.33(0.92+0.47) 0.33(0.94+0.45) +0.0% 0.25(0.42+0.51) -24.2%\n 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.35(1.08+0.46) 0.35(1.12+0.41) +0.0% 0.25(0.52+0.50) -28.6%\n 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.78+0.51) 0.30(0.86+0.42) +0.0% 0.25(0.29+0.54) -16.7%\n\nSee commit (\"perf: add a comparison test of grep regex engines\",\n2017-04-19) for details on the machine the above test run was executed\non.\n\nHere HEAD~2 is git with PCRE v1 without JIT, HEAD~ is PCRE v1 with\nJIT, and HEAD is PCRE v2 (also with JIT). See previous commits of mine\nmentioning p7820-grep-engines.sh for more details on the test setup.\n\nFor ease of readability, a different run just of HEAD~ (PCRE v1 with\nJIT v.s. PCRE v2), again with just the /perl/ tests shown:\n\n Test HEAD~ HEAD\n ---------------------------------------------------------------------------------------\n 7820.3: perl grep 'how.to' 0.23(0.41+0.47) 0.23(0.26+0.59) +0.0%\n 7820.7: perl grep '^how to' 0.27(0.64+0.47) 0.23(0.28+0.56) -14.8%\n 7820.11: perl grep '[how] to' 0.34(0.95+0.44) 0.25(0.38+0.56) -26.5%\n 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.34(1.07+0.46) 0.24(0.52+0.49) -29.4%\n 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.81+0.46) 0.22(0.33+0.54) -26.7%\n\nI.e. the two are either neck-to-neck, but PCRE v2 usually pulls ahead,\nwhen it does it's around 20% faster.\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\n2. https://lists.exim.org/lurker/thread/20170419.172322.833ee099.en.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 30 +++++++++---\n configure.ac | 77 ++++++++++++++++++++++++++-----\n grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n grep.h | 17 +++++++\n t/test-lib.sh | 2 +-\n 5 files changed, 250 insertions(+), 19 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex a79274e5e6..d77ca4c1a5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -29,7 +29,12 @@ all::\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+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define\n+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE\n+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by\n+# default in future releases.\n+#\n+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in\n # /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@@ -1087,15 +1092,27 @@ ifdef NO_LIBGEN_H\n \tCOMPAT_OBJS += compat/basename.o\n endif\n \n-ifdef USE_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+USE_LIBPCRE1 ?= $(USE_LIBPCRE)\n+\n+ifneq (,$(USE_LIBPCRE1))\n+\tifdef USE_LIBPCRE2\n+$(error Only set USE_LIBPCRE1 (or its alias USE_LIBPCRE) or USE_LIBPCRE2, not both!)\n \tendif\n+\n+\tBASIC_CFLAGS += -DUSE_LIBPCRE1\n \tEXTLIBS += -lpcre\n endif\n \n+ifdef USE_LIBPCRE2\n+\tBASIC_CFLAGS += -DUSE_LIBPCRE2\n+\tEXTLIBS += -lpcre2-8\n+endif\n+\n+ifdef LIBPCREDIR\n+\tBASIC_CFLAGS += -I$(LIBPCREDIR)/include\n+\tEXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)\n+endif\n+\n ifdef HAVE_ALLOCA_H\n \tBASIC_CFLAGS += -DHAVE_ALLOCA_H\n endif\n@@ -2241,6 +2258,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_PTHREADS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PTHREADS)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\ndiff --git a/configure.ac b/configure.ac\nindex deeb968daa..11d083fbe0 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -255,21 +255,61 @@ GIT_PARSE_WITH([openssl]))\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+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define\n+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE\n+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by\n+# default in future releases.\n+#\n+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in\n # /foo/bar/include and /foo/bar/lib directories.\n #\n AC_ARG_WITH(libpcre,\n-AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes (default is NO)])\n+AS_HELP_STRING([--with-libpcre],[synonym for --with-libpcre1]),\n+ if test \"$withval\" = \"no\"; then\n+\tUSE_LIBPCRE1=\n+ elif test \"$withval\" = \"yes\"; then\n+\tUSE_LIBPCRE1=YesPlease\n+ else\n+\tUSE_LIBPCRE1=YesPlease\n+\tLIBPCREDIR=$withval\n+\tAC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])\n+ dnl USE_LIBPCRE1 can still be modified below, so don't substitute\n+ dnl it yet.\n+\tGIT_CONF_SUBST([LIBPCREDIR])\n+ fi)\n+\n+AC_ARG_WITH(libpcre1,\n+AS_HELP_STRING([--with-libpcre1],[support Perl-compatible regexes via libpcre1 (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_LIBPCRE1=\n+ elif test \"$withval\" = \"yes\"; then\n+\tUSE_LIBPCRE1=YesPlease\n+ else\n+\tUSE_LIBPCRE1=YesPlease\n+\tLIBPCREDIR=$withval\n+\tAC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])\n+ dnl USE_LIBPCRE1 can still be modified below, so don't substitute\n+ dnl it yet.\n+\tGIT_CONF_SUBST([LIBPCREDIR])\n+ fi)\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 -n \"$USE_LIBPCRE1\"; then\n+ AC_MSG_ERROR([Only supply one of --with-libpcre1 or --with-libpcre2!])\n+ fi\n+\n if test \"$withval\" = \"no\"; then\n-\tUSE_LIBPCRE=\n+\tUSE_LIBPCRE2=\n elif test \"$withval\" = \"yes\"; then\n-\tUSE_LIBPCRE=YesPlease\n+\tUSE_LIBPCRE2=YesPlease\n else\n-\tUSE_LIBPCRE=YesPlease\n+\tUSE_LIBPCRE2=YesPlease\n \tLIBPCREDIR=$withval\n \tAC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])\n- dnl USE_LIBPCRE can still be modified below, so don't substitute\n+ dnl USE_LIBPCRE2 can still be modified below, so don't substitute\n dnl it yet.\n \tGIT_CONF_SUBST([LIBPCREDIR])\n fi)\n@@ -501,13 +541,11 @@ 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. Various\n-# commands such as log and grep offer runtime options to use\n-# Perl-compatible regular expressions instead of standard or extended\n-# POSIX regular expressions.\n+# Handle the USE_LIBPCRE1 and USE_LIBPCRE2 options potentially set\n+# above.\n #\n \n-if test -n \"$USE_LIBPCRE\"; then\n+if test -n \"$USE_LIBPCRE1\"; then\n \n GIT_STASH_FLAGS($LIBPCREDIR)\n \n@@ -517,7 +555,22 @@ AC_CHECK_LIB([pcre], [pcre_version],\n \n GIT_UNSTASH_FLAGS($LIBPCREDIR)\n \n-GIT_CONF_SUBST([USE_LIBPCRE])\n+GIT_CONF_SUBST([USE_LIBPCRE1])\n+\n+fi\n+\n+\n+if test -n \"$USE_LIBPCRE2\"; then\n+\n+GIT_STASH_FLAGS($LIBPCREDIR)\n+\n+AC_CHECK_LIB([pcre2-8], [pcre2_config_8],\n+[USE_LIBPCRE2=YesPlease],\n+[USE_LIBPCRE2=])\n+\n+GIT_UNSTASH_FLAGS($LIBPCREDIR)\n+\n+GIT_CONF_SUBST([USE_LIBPCRE2])\n \n fi\n \ndiff --git a/grep.c b/grep.c\nindex 3c0c30f033..569cf9e290 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -179,22 +179,36 @@ 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\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\tbreak;\n \n \tcase GREP_PATTERN_TYPE_PCRE:\n \t\topt->fixed = 0;\n+#ifdef USE_LIBPCRE2\n+\t\topt->pcre1 = 0;\n+\t\topt->pcre2 = 1;\n+#else\n+\t\t/* It's important that pcre1 always be assigned to\n+\t\t * even when there's no USE_LIBPCRE* defined. We still\n+\t\t * call the PCRE stub function, it just dies with\n+\t\t * \"cannot use Perl-compatible regexes[...]\".\n+\t\t */\n \t\topt->pcre1 = 1;\n+\t\topt->pcre2 = 0;\n+#endif\n \t\tbreak;\n \t}\n }\n@@ -446,6 +460,126 @@ 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+\tassert(opt->pcre2);\n+\n+\tp->pcre2_compile_context = 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_compile_context = pcre2_compile_context_create(NULL);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, 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_compile_context);\n+\n+\tif (p->pcre2_pattern) {\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\t\tif (!p->pcre2_match_data)\n+\t\t\tdie(\"BUG: Couldn't allocate PCRE2 match data\");\n+\t} else {\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\tp->pcre2_jit_on = 1;\n+\t\telse\n+\t\t\tdie(\"BUG: Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n+\t\tif (!p->pcre2_jit_stack)\n+\t\t\tdie(\"BUG: Couldn't allocate PCRE2 JIT stack\");\n+\t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\n+\t\tif (!p->pcre2_jit_stack)\n+\t\t\tdie(\"BUG: Couldn't allocate PCRE2 match context\");\n+\t\tpcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);\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+\tPCRE2_UCHAR errbuf[256];\n+\n+\tif (eflags & REG_NOTBOL)\n+\t\tflags |= PCRE2_NOTBOL;\n+\n+\tif (p->pcre2_jit_on)\n+\t\tret = pcre2_jit_match(p->pcre2_pattern, (unsigned char *)line,\n+\t\t\t\t eol - line, 0, flags, p->pcre2_match_data,\n+\t\t\t\t NULL);\n+\telse\n+\t\tret = pcre2_match(p->pcre2_pattern, (unsigned char *)line,\n+\t\t\t\t eol - line, 0, flags, p->pcre2_match_data,\n+\t\t\t\t NULL);\n+\n+\tif (ret < 0 && ret != PCRE2_ERROR_NOMATCH) {\n+\t\tpcre2_get_error_message(ret, errbuf, sizeof(errbuf));\n+\t\tdie(\"%s failed with error code %d: %s\",\n+\t\t (p->pcre2_jit_on ? \"pcre2_jit_match\" : \"pcre2_match\"), ret,\n+\t\t errbuf);\n+\t}\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_compile_context_free(p->pcre2_compile_context);\n+\tpcre2_code_free(p->pcre2_pattern);\n+\tpcre2_match_data_free(p->pcre2_match_data);\n+\tpcre2_jit_stack_free(p->pcre2_jit_stack);\n+\tpcre2_match_context_free(p->pcre2_match_context);\n+}\n+#else /* !USE_LIBPCRE2 */\n+static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n+{\n+\t/* Unreachable until USE_LIBPCRE2 becomes synonymous with\n+\t * USE_LIBPCRE. See the sibling comment in\n+\t * grep_set_pattern_type_option().\n+\t */\n+\tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\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 void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -509,6 +643,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 \tif (opt->pcre1) {\n \t\tcompile_pcre1_regexp(p, opt);\n \t\treturn;\n@@ -868,6 +1007,8 @@ void free_grep_patterns(struct grep_opt *opt)\n \t\t\t\tkwsfree(p->kws);\n \t\t\telse if (p->pcre1_regexp)\n \t\t\t\tfree_pcre1_regexp(p);\n+\t\t\telse if (p->pcre2_pattern)\n+\t\t\t\tfree_pcre2_pattern(p);\n \t\t\telse\n \t\t\t\tregfree(&p->regexp);\n \t\t\tfree(p->pattern);\n@@ -948,6 +1089,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 b7b9d487b0..b40afc2e2f 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -19,6 +19,16 @@ typedef int pcre;\n typedef int pcre_extra;\n typedef int pcre_jit_stack;\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+typedef int pcre2_match_context;\n+typedef int pcre2_jit_stack;\n+#endif\n #include \"kwset.h\"\n #include \"thread-utils.h\"\n #include \"userdiff.h\"\n@@ -63,6 +73,12 @@ struct grep_pat {\n \tpcre_jit_stack *pcre1_jit_stack;\n \tconst unsigned char *pcre1_tables;\n \tint pcre1_jit_on;\n+\tpcre2_code *pcre2_pattern;\n+\tpcre2_match_data *pcre2_match_data;\n+\tpcre2_compile_context *pcre2_compile_context;\n+\tpcre2_match_context *pcre2_match_context;\n+\tpcre2_jit_stack *pcre2_jit_stack;\n+\tint pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n \tunsigned ignore_case:1;\n@@ -126,6 +142,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/t/test-lib.sh b/t/test-lib.sh\nindex 43529451f9..f5da636bea 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1015,7 +1015,7 @@ esac\n test -z \"$NO_PERL\" && test_set_prereq PERL\n test -z \"$NO_PTHREADS\" && test_set_prereq PTHREADS\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 -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320582","messageId":"20170523192453.14172-2-avarab@gmail.com","threadId":"46011","inReplyTo":"20170523192453.14172-1-avarab@gmail.com","subject":"[PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-23T19:24:47Z","receivedAt":"2017-05-23T19:26:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the pattern compilation logic under threading so that grep\ndoesn't compile a pattern it never ends up using on the non-threaded\ncode path, only to compile it again N times for N threads which will\neach use their own copy, ignoring the initially compiled pattern.\n\nThis redundant compilation dates back to the initial introduction of\nthe threaded grep in commit 5b594f457a (\"Threaded grep\",\n2010-01-25).\n\nThere was never any reason for doing this redundant work other than an\noversight in the initial commit. Jeff King suggested on-list in\n<20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net> that this\nmight be needed to check the pattern for sanity before threaded\nexecution commences.\n\nThat's not the case. The pattern is compiled under threading in\nstart_threads() before any concurrent execution has started by calling\npthread_create(), so if the pattern contains an error we still do the\nright thing. I.e. die with one error before any threaded execution has\ncommenced, instead of e.g. spewing out an error for each N threads,\nwhich could be a regression a change like this might inadvertently\nintroduce.\n\nThis change is not meant as an optimization, any performance gains\nfrom this are in the hundreds to thousands of nanoseconds at most. If\nwe wanted more performance here we could just re-use the compiled\npatterns in multiple threads (regcomp(3) is thread-safe), or partially\nre-use them and the associated structures in the case of later PCRE\nJIT changes.\n\nRather, it's just to make the code easier to reason about. It's\nconfusing to debug this under threading & non-threading when the\nthreading codepaths redundantly compile a pattern which is never used.\n\nThe reason the patterns are recompiled is as a side-effect of\nduplicating the whole grep_opt structure, which is not thread safe,\nwritable, and munged during execution. The grep_opt structure then\npoints to the grep_pat structure where pattern or patterns are stored.\n\nI looked into e.g. splitting the API into some \"do & alloc threadsafe\nstuff\", \"spawn thread\", \"do and alloc non-threadsafe stuff\", but the\nexecution time of grep_opt_dup() & pattern compilation is trivial\ncompared to actually executing the grep, so there was no point. Even\nwith the more expensive JIT changes to follow the most expensive PCRE\npatterns take something like 0.0X milliseconds to compile at most[1].\n\nThe undocumented --debug mode added in commit 17bf35a3c7 (\"grep: teach\n--debug option to dump the parse tree\", 2012-09-13) still works\nproperly with this change. It only emits debugging info during pattern\ncompilation, which is now dumped by the pattern compiled just before\nthe first thread is started.\n\n1. http://sljit.sourceforge.net/pcre.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex b1095362fb..12e62fcbf3 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -224,7 +224,8 @@ static void start_threads(struct grep_opt *opt)\n \t\tint err;\n \t\tstruct grep_opt *o = grep_opt_dup(opt);\n \t\to->output = strbuf_out;\n-\t\to->debug = 0;\n+\t\tif (i)\n+\t\t\to->debug = 0;\n \t\tcompile_grep_patterns(o);\n \t\terr = pthread_create(&threads[i], NULL, run, o);\n \n@@ -1167,8 +1168,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!opt.fixed && opt.ignore_case)\n \t\topt.regflags |= REG_ICASE;\n \n-\tcompile_grep_patterns(&opt);\n-\n \t/*\n \t * We have to find \"--\" in a separate pass, because its presence\n \t * influences how we will parse arguments that come before it.\n@@ -1245,6 +1244,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tnum_threads = 0;\n #endif\n \n+\tif (!num_threads)\n+\t\t/*\n+\t\t * The compiled patterns on the main path are only\n+\t\t * used when not using threading. Otherwise\n+\t\t * start_threads() below calls compile_grep_patterns()\n+\t\t * for each thread.\n+\t\t */\n+\t\tcompile_grep_patterns(&opt);\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tif (!(opt.name_only || opt.unmatch_name_only || opt.count)\n-- \n2.13.0.303.g4ebf302169\n\n"},{"id":"320598","messageId":"20170523211754.GI115919@google.com","threadId":"46011","inReplyTo":"20170520214233.7183-23-avarab@gmail.com","subject":"Re: [PATCH v3 22/30] grep: factor test for \\0 in grep patterns into a function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-23T21:17:54Z","receivedAt":"2017-05-23T21:18:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/20, Ævar Arnfjörð Bjarmason wrote:\n> Factor the test for \\0 in grep patterns into a function. Since commit\n> 9eceddeec6 (\"Use kwset in grep\", 2011-08-21) any pattern containing a\n> \\0 is considered fixed as regcomp() can't handle it.\n> \n> This change makes later changes that make use of either has_null() or\n> is_fixed() (but not both) smaller.\n> \n> While I'm at it make the comment conform to the style guide, i.e. add\n> an opening \"/*\\n\".\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 20 +++++++++++++-------\n> 1 file changed, 13 insertions(+), 7 deletions(-)\n> \n> diff --git a/grep.c b/grep.c\n> index bf6c2494fd..79eb681c6e 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -321,6 +321,18 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p,\n> \tdie(\"%s'%s': %s\", where, p->pattern, error);\n> }\n> \n> +static int has_null(const char *s, size_t len)\n> +{\n> +\t/*\n> +\t * regcomp cannot accept patterns with NULs so when using it\n> +\t * we consider any pattern containing a NUL fixed.\n> +\t */\n> +\tif (memchr(s, 0, len))\n> +\t\treturn 1;\n> +\n> +\treturn 0;\n> +}\n> +\n> #ifdef USE_LIBPCRE\n> static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> {\n> @@ -394,12 +406,6 @@ static int is_fixed(const char *s, size_t len)\n> {\n> \tsize_t i;\n> \n> -\t/* regcomp cannot accept patterns with NULs so we\n> -\t * consider any pattern containing a NUL fixed.\n> -\t */\n> -\tif (memchr(s, 0, len))\n> -\t\treturn 1;\n> -\n> \tfor (i = 0; i < len; i++) {\n> \t\tif (is_regex_special(s[i]))\n> \t\t\treturn 0;\n> @@ -451,7 +457,7 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n> \t * simple string match using kws. p->fixed tells us if we\n> \t * want to use kws.\n> \t */\n> -\tif (opt->fixed || is_fixed(p->pattern, p->patternlen))\n> +\tif (opt->fixed || has_null(p->pattern, p->patternlen) || is_fixed(p->pattern, p->patternlen))\n\nsmall nit: longer than 80 char\n\n> \t\tp->fixed = !icase || ascii_only;\n> \telse\n> \t\tp->fixed = 0;\n> -- \n> 2.13.0.303.g4ebf302169\n> \n\n-- \nBrandon Williams\n"},{"id":"320615","messageId":"xmqqvaoqhnlw.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170523192453.14172-2-avarab@gmail.com","subject":"Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T04:42:35Z","receivedAt":"2017-05-24T04:42:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Rather, it's just to make the code easier to reason about. It's\n> confusing to debug this under threading & non-threading when the\n> threading codepaths redundantly compile a pattern which is never used.\n>\n> The reason the patterns are recompiled is as a side-effect of\n> duplicating the whole grep_opt structure, which is not thread safe,\n> writable, and munged during execution. The grep_opt structure then\n> points to the grep_pat structure where pattern or patterns are stored.\n>\n> I looked into e.g. splitting the API into some \"do & alloc threadsafe\n> stuff\", \"spawn thread\", \"do and alloc non-threadsafe stuff\", but the\n> execution time of grep_opt_dup() & pattern compilation is trivial\n> compared to actually executing the grep, so there was no point. Even\n> with the more expensive JIT changes to follow the most expensive PCRE\n> patterns take something like 0.0X milliseconds to compile at most[1].\n\nOK.\n\n> The undocumented --debug mode added in commit 17bf35a3c7 (\"grep: teach\n> --debug option to dump the parse tree\", 2012-09-13) still works\n> properly with this change. It only emits debugging info during pattern\n> compilation, which is now dumped by the pattern compiled just before\n> the first thread is started.\n\nWhen opt is passed to run(), opt->debug is still true for the first\nworker thread. As long as opt->debug never makes difference after\ncompile_grep_patterns(opt) returns, I think the change in this patch\nsafe. I do not know if we want to rely on it, but we can explain it\naway by saying \"we'll only debug the runtime behaviour for the first\nworker only\", or something, so it is not a big deal either way.\n\nThanks.\n"},{"id":"320616","messageId":"xmqqr2zehngz.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170523192453.14172-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/7] grep: skip pthreads overhead when using one thread","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T04:45:32Z","receivedAt":"2017-05-24T04:45:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Skip the administrative overhead of using pthreads when only using one\n> thread. Instead take the non-threaded path which would be taken under\n> NO_PTHREADS.\n>\n> The threading support was initially added in commit\n> 5b594f457a (\"Threaded grep\", 2010-01-25) with a hardcoded compile-time\n> number of 8 threads. Later the number of threads was made configurable\n> in commit 89f09dd34e (\"grep: add --threads= option and\n> grep.threads configuration\", 2015-12-15).\n>\n> That change did not add any special handling for --threads=1. Now we\n> take a slightly faster path by skipping thread handling entirely when\n> 1 thread is requested.\n\nOK, this is what Peff and you were discussing in the earlier round,\nhaving the controller do the work himself, instead of sitting and\nwaiting for a sole worker to finish the work. Looks good.\n\nThanks.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/grep.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 12e62fcbf3..bd008cb100 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1238,6 +1238,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\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);\n> +\tif (num_threads == 1)\n> +\t\tnum_threads = 0;\n> #else\n> \tif (num_threads)\n> \t\twarning(_(\"no threads support, ignoring --threads\"));\n"},{"id":"320645","messageId":"xmqqlgpmhm08.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170523192453.14172-5-avarab@gmail.com","subject":"Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T05:17:11Z","receivedAt":"2017-05-24T05:17:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/grep.c b/grep.c\n> index 1157529115..49e9aed457 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -351,6 +351,9 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> \tconst char *error;\n> \tint erroffset;\n> \tint options = PCRE_MULTILINE;\n> +#ifdef PCRE_CONFIG_JIT\n> +\tint canjit;\n> +#endif\n\nIs \"canjit\" a property purely of the library (e.g. version and\ncompilation option), or of combination of that and nature of the\npattern, or something else like the memory pressure?\n\nI am wondering if it is worth doing something like this:\n\n\tstatic int canjit = -1;\n\tif (canjit < 0) {\n\t\tpcre_config(PCRE_CONFIG_JIT, &canjit);\n\t}\n\nif it depends purely on the library linked to the process.\n\n> @@ -365,9 +368,20 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> \tif (!p->pcre1_regexp)\n> \t\tcompile_regexp_failed(p, error);\n> \n> -\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, 0, &error);\n> +\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);\n> \tif (!p->pcre1_extra_info && error)\n> \t\tdie(\"%s\", error);\n> +\n> +#ifdef PCRE_CONFIG_JIT\n> +\tpcre_config(PCRE_CONFIG_JIT, &canjit);\n> +\tif (canjit == 1) {\n> +\t\tp->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);\n> +\t\tif (!p->pcre1_jit_stack)\n> +\t\t\tdie(\"BUG: Couldn't allocate PCRE JIT stack\");\n\nI agree that dying is OK, but as far as I can tell, this is not a\nBUG (there is no error a programmer can correct by a follow-up\npatch); please do not mark it as such (it is likely that we'll later\ndo a tree-wide s/die(\"BUG:/BUG(\"/ and this will interfere).\n\n> +\t\tpcre_assign_jit_stack(p->pcre1_extra_info, NULL, p->pcre1_jit_stack);\n> +\t\tp->pcre1_jit_on = 1;\n\nContrary to what I wondered about \"canjit\" above, I think it makes\ntons of sense to contain the \"is JIT in use?\" information in \"struct\ngrep_pat\" and not rely on any global state. Not that we are likely\nto want to be able to JIT some patterns while not doing others. So\nI agree with the design choice of adding pcre1_jit_on field to the\nstructure.\n\nBut then, wouldn't it make more sense to do all of the above without\nthe canjit variable at all? i.e. something like...\n\n #ifdef PCRE_CONFIG_JIT\n pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);\n if (p->pcre1_jit_on)\n ... stack thing ...\n #else\n p->pcre1_jit_on = 0;\n #endif\n\n> +#ifdef PCRE_CONFIG_JIT\n> +\tif (p->pcre1_jit_on) {\n> +\t\tpcre_free_study(p->pcre1_extra_info);\n> +\t\tpcre_jit_stack_free(p->pcre1_jit_stack);\n> +\t} else\n> +#endif\n> +\t/* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n> \tpcre_free(p->pcre1_extra_info);\n> +\n> \tpcre_free((void *)p->pcre1_tables);\n\nIt is very thoughtful to add a blank line here (and you did the same\nin another similar hunk), but I have a feeling that it is still a\nbit too subtle a hint to signal to the readers that these two\npcre_free()s fire differently, i.e. the former does not fire if jit\nis on but the latter always fires.\n\nWould this be a bit safer while being not too ugly to live, I wonder?\n\n #ifdef PCRE_CONFIG_JIT\n if (p->pcre1_jit_on) {\n pcre_free_study(p->pcre1_extra_info);\n pcre_jit_stack_free(p->pcre1_jit_stack);\n } else\n #endif\n {\n /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n pcre_free(p->pcre1_extra_info);\n }\n pcre_free((void *)p->pcre1_tables);\n\nThanks.\n"},{"id":"320649","messageId":"xmqqh90ahjzy.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170523192453.14172-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/7] grep: un-break building with PCRE < 8.32","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T06:00:33Z","receivedAt":"2017-05-24T06:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Amend my change earlier in this series (\"grep: add support for the\n> PCRE v1 JIT API\", 2017-04-11) to un-break the build on PCRE v1\n> versions earlier than 8.32.\n> ...\n> So just take the easy way out and disable the JIT on any version older\n> than 8.32.\n\nThe above were very understandable, but I had quite a hard time\nparsing first sentence of the next paragraph, especially everything\nafter \"because\". In the end I think I figured out what you wanted\nto say (so you do not have to explain it to me in a response), but I\nwish there were an easier-to-understand way to write the same thing.\n\n> The reason this change isn't part of the initial change PCRE JIT\n> support is because possibly slightly annoying someone who's bisecting\n> with an ancient PCRE is worth it to have a cleaner history showing\n> which parts of the implementation are only used for ancient PCRE\n> versions. This also makes it easier to revert this change if we ever\n> decide to stop supporting those old versions.\n>\n> 1. http://www.pcre.org/original/changelog.txt (\"28. Introducing a\n> native interface for JIT. Through this interface, the\n> compiled[...]\")\n> 2. https://bugs.exim.org/show_bug.cgi?id=2121\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 8 ++++----\n> grep.h | 5 +++++\n> 2 files changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 49e9aed457..3c0c30f033 100644\n> --- a/grep.c\n> +++ b/grep.c\n> ...\n> @@ -418,7 +418,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n> {\n> \tpcre_free(p->pcre1_regexp);\n> \n> -#ifdef PCRE_CONFIG_JIT\n> +#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\n> \tif (p->pcre1_jit_on) {\n> \t\tpcre_free_study(p->pcre1_extra_info);\n> \t\tpcre_jit_stack_free(p->pcre1_jit_stack);\n> diff --git a/grep.h b/grep.h\n> index 14f47189f9..73ef0ef8ec 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -3,6 +3,11 @@\n> #include \"color.h\"\n> #ifdef USE_LIBPCRE1\n> #include \n> +#ifdef PCRE_CONFIG_JIT\n> +#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n> +#define GIT_PCRE1_CAN_DO_MODERN_JIT\n> +#endif\n> +#endif\n> #ifndef PCRE_STUDY_JIT_COMPILE\n> #define PCRE_STUDY_JIT_COMPILE 0\n> #endif\n\nAfter reading the patch, I do not necessarily agree with your\npros-and-cons between keeping the patches as separate steps and\nsquashing them into one, though. Even if this were squashed into\n[PATCH 4/7], the logic to set GIT_PCRE1_CAN_DO_MODERN_JIT based on\nPCRE_CONFIG_JIT and PCRE's version in this patch is well isolated to\na single place, and it is easy to spot what needs to be done when we\ndecide to lose the version-based GIT_PCRE1_CAN_DO_MODERN_JIT from\nour code after making sure that nobody uses versions older than\n8.32.\n\nBy the time we will make such a decision, it is likely that we no\nlonger remember if \"#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT\" we have in\nour code at that moment in the future were all already present in\nthis patch. An update to drop support for older PCRE is likely to\nbe done by finding the above part in grep.h to remove the\nversion-dependent part, while still keeping #ifdef based on\nGIT_PCRE1_CAN_DO_MODERN_JIT in the *.c code. Being able to revert\nthis patch does not help there.\n\nWe might also do a find-and-replace of GIT_PCRE1_CAN_DO_MODERN_JIT\nto PCRE_CONFIG_JIT when we drop support for older PCRE, but I do not\nthink we would assume that it is sufficient to revert this patch\nwhen we do so. We may have added more #ifdef on the GIT_MODERN_JIT\nsymbol we now need to change to PCRE_CONFIG_JIT, so that will be\ndone by find-and-replace of the then-current code, not by reverting\nthis patch.\n\nSo in that sense, I do not think keeping them separate in practice\nhas the \"makes it easier to revert this change\" benefit.\n\nIf I were doing these two patches, I'd squash them together into\none, rename GIT_PCRE1_CAN_DO_MODERN_JIT to GIT_PCRE1_USE_JIT, and\nexplain in the log message why we turn it off for versions older\nthan 8.32 like you did in the log message for thsi patch. \n\nThe reason for the \"rename\" is because I might also be tempted to\nallow users of newer version to manually decline GIT_PCRE1_USE_JIT\nin Makefile/config.mak; i.e. we may decide not to USE something even\nif we CAN, and the #ifdef symbol you are using is about the decision\nto USE or not USE, not necessarily if the library CAN.\n\nThanks.\n"},{"id":"320650","messageId":"xmqqa862hixh.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"20170523192453.14172-8-avarab@gmail.com","subject":"Re: [PATCH v2 7/7] grep: add support for PCRE v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T06:23:38Z","receivedAt":"2017-05-24T06:23:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Add support for v2 of the PCRE API. This is a new major version of\n> PCRE that came out in early 2015[1].\n>\n> The regular expression syntax is the same, but while the API is\n> similar, pretty much every function is either renamed or takes\n> different arguments. Thus using it via entirely new functions makes\n> sense, as opposed to trying to e.g. have one compile_pcre_pattern()\n> that would call either PCRE v1 or v2 functions.\n>\n> Git can now be compiled with either USE_LIBPCRE1=YesPlease or\n> USE_LIBPCRE2=YesPlease, with USE_LIBPCRE=YesPlease currently being a\n> synonym for the former. Providing both is a compile-time error.\n>\n> With earlier patches to enable JIT for PCRE v1 the performance of the\n> release versions of both libraries is almost exactly the same, with\n> PCRE v2 being around 1% slower.\n>\n> However after I reported this to the pcre-dev mailing list[2] I got a\n> lot of help with the API use from Zoltán Herczeg, he subsequently\n> optimized some of the JIT functionality in v2 of the library.\n>\n> Running the p7820-grep-engines.sh performance test against the latest\n> Subversion trunk of both, with both them and git compiled as -O3, and\n> the test run against linux.git, gives the following results. Just the\n> /perl/ tests shown:\n>\n> $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_COMMAND='grep -q LIBPCRE2 Makefile && make -j8 USE_LIBPCRE2=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre2/inst/lib || make -j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~2 HEAD~ HEAD p7820-grep-engines.sh\n> [...]\n> Test HEAD~2 HEAD~ HEAD\n> ----------------------------------------------------------------------------------------------------------------\n> 7820.3: perl grep 'how.to' 0.22(0.40+0.48) 0.22(0.31+0.58) +0.0% 0.22(0.26+0.59) +0.0%\n> 7820.7: perl grep '^how to' 0.27(0.62+0.50) 0.28(0.60+0.50) +3.7% 0.22(0.25+0.60) -18.5%\n> 7820.11: perl grep '[how] to' 0.33(0.92+0.47) 0.33(0.94+0.45) +0.0% 0.25(0.42+0.51) -24.2%\n> 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.35(1.08+0.46) 0.35(1.12+0.41) +0.0% 0.25(0.52+0.50) -28.6%\n> 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.78+0.51) 0.30(0.86+0.42) +0.0% 0.25(0.29+0.54) -16.7%\n>\n> See commit (\"perf: add a comparison test of grep regex engines\",\n> 2017-04-19) for details on the machine the above test run was executed\n> on.\n>\n> Here HEAD~2 is git with PCRE v1 without JIT, HEAD~ is PCRE v1 with\n> JIT, and HEAD is PCRE v2 (also with JIT). See previous commits of mine\n> mentioning p7820-grep-engines.sh for more details on the test setup.\n>\n> For ease of readability, a different run just of HEAD~ (PCRE v1 with\n> JIT v.s. PCRE v2), again with just the /perl/ tests shown:\n>\n> Test HEAD~ HEAD\n> ---------------------------------------------------------------------------------------\n> 7820.3: perl grep 'how.to' 0.23(0.41+0.47) 0.23(0.26+0.59) +0.0%\n> 7820.7: perl grep '^how to' 0.27(0.64+0.47) 0.23(0.28+0.56) -14.8%\n> 7820.11: perl grep '[how] to' 0.34(0.95+0.44) 0.25(0.38+0.56) -26.5%\n> 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.34(1.07+0.46) 0.24(0.52+0.49) -29.4%\n> 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.81+0.46) 0.22(0.33+0.54) -26.7%\n>\n> I.e. the two are either neck-to-neck, but PCRE v2 usually pulls ahead,\n> when it does it's around 20% faster.\n>\n> A brief note on thread safety: As noted in pcre2api(3) & pcre2jit(3)\n> the compiled pattern can be shared between threads, but not some of\n> the JIT context, however the grep threading support does all pattern &\n> JIT compilation in separate threads, so this code doesn't need to\n> concern itself with thread safety.\n\nNicely explained.\n\n> -# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in\n> +# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define\n> +# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE\n> +# library. The USE_LIBPCRE flag will likely be changed to mean v2 by\n> +# default in future releases.\n> +#\n> +# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in\n> # /foo/bar/include and /foo/bar/lib directories.\n\nAs there is no way to use both, having a single LIBPCREDIR is not a\nhurting limitation, which makes sense.\n\n> @@ -2241,6 +2258,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\nShouldn't the line above record $(USE_LIBPCRE1) instead of the\ngeneric fallback?\n\n> +\t@echo USE_LIBPCRE2=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE2)))'\\' >>$@+\n> \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n> \t@echo NO_PTHREADS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PTHREADS)))'\\' >>$@+\n> \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n\n> diff --git a/grep.c b/grep.c\n> index 3c0c30f033..569cf9e290 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -179,22 +179,36 @@ 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\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\tbreak;\n> \n> \tcase GREP_PATTERN_TYPE_PCRE:\n> \t\topt->fixed = 0;\n> +#ifdef USE_LIBPCRE2\n> +\t\topt->pcre1 = 0;\n> +\t\topt->pcre2 = 1;\n> +#else\n> +\t\t/* It's important that pcre1 always be assigned to\n> +\t\t * even when there's no USE_LIBPCRE* defined. We still\n> +\t\t * call the PCRE stub function, it just dies with\n> +\t\t * \"cannot use Perl-compatible regexes[...]\".\n> +\t\t */\n> \t\topt->pcre1 = 1;\n\nVery well thought-out comment. Our style wants you to have\nslash-aster that opens a multi-line comment on its own line, though.\n\n> +\t\topt->pcre2 = 0;\n> +#endif\n> \t\tbreak;\n> \t}\n> }\n> @@ -446,6 +460,126 @@ 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> +...\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_compile_context);\n\nAre all die(\"BUG:...\") in this function actual bugs, or just\n\"die()\"? Just like the comment on an earlier patch, things like\nrunning out of memory that you as a Git programmer cannot fix by\ncorrecting this code are not die(\"BUG:\"), but normal runtime errors.\n\n> +\n> +\tif (p->pcre2_pattern) {\n> +\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n> +\t\tif (!p->pcre2_match_data)\n> +\t\t\tdie(\"BUG: Couldn't allocate PCRE2 match data\");\n> +\t} else {\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\tp->pcre2_jit_on = 1;\n\nI think the same \"would it be better to do this without canjit?\"\ncomment applies here.\n\n> +#else /* !USE_LIBPCRE2 */\n> +static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> +{\n> +\t/* Unreachable until USE_LIBPCRE2 becomes synonymous with\n> +\t * USE_LIBPCRE. See the sibling comment in\n> +\t * grep_set_pattern_type_option().\n> +\t */\n> +\tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\n> +}\n\nWow. If I were doing this, I wouldn't have been this cautious, but\nI have no complaints ;-).\n\n"},{"id":"320651","messageId":"xmqq60gqhi8z.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"xmqqh90ahjzy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 5/7] grep: un-break building with PCRE < 8.32","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T06:38:20Z","receivedAt":"2017-05-24T06:38:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So in that sense, I do not think keeping them separate in practice\n> has the \"makes it easier to revert this change\" benefit.\n>\n> If I were doing these two patches, I'd squash them together into\n> one, rename GIT_PCRE1_CAN_DO_MODERN_JIT to GIT_PCRE1_USE_JIT, and\n> explain in the log message why we turn it off for versions older\n> than 8.32 like you did in the log message for thsi patch. \n>\n> The reason for the \"rename\" is because I might also be tempted to\n> allow users of newer version to manually decline GIT_PCRE1_USE_JIT\n> in Makefile/config.mak; i.e. we may decide not to USE something even\n> if we CAN, and the #ifdef symbol you are using is about the decision\n> to USE or not USE, not necessarily if the library CAN.\n\nNeed to say a few things that I forgot to mention.\n\nI said that I do not see practical benefit for keeping the patches\nseparate. But at the same time, I do not see it a huge problem that\nsuch a \"main one that is partly broken\" followed by \"fix for one\nminority\" followed by \"fix for another minority\" pattern causes to\nbisection. So I'd be OK either way.\n\nEspecially, I'd be more than OK if the \"main one that is partly broken\"\nsays \"by the way, this is (deliberately) left broken for two cases,\nand if you hit this during your bisection, do not answer good or bad\nand instead reset to a few commits newer that has both fixes\" in its\nlog message. Then there is no downside in bisection.\n\n\n\n"},{"id":"320655","messageId":"CACBZZX5MPXR7Z6oSPN+UjQxZhXpe8O3XVcuC8gAGjAbofJ4YDw@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqlgpmhm08.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-24T07:37:51Z","receivedAt":"2017-05-24T07:38:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 24, 2017 at 7:17 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/grep.c b/grep.c\n>> index 1157529115..49e9aed457 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -351,6 +351,9 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n>> const char *error;\n>> int erroffset;\n>> int options = PCRE_MULTILINE;\n>> +#ifdef PCRE_CONFIG_JIT\n>> + int canjit;\n>> +#endif\n>\n> Is \"canjit\" a property purely of the library (e.g. version and\n> compilation option), or of combination of that and nature of the\n> pattern, or something else like the memory pressure?\n>\n> I am wondering if it is worth doing something like this:\n>\n> static int canjit = -1;\n> if (canjit < 0) {\n> pcre_config(PCRE_CONFIG_JIT, &canjit);\n> }\n>\n> if it depends purely on the library linked to the process.\n\nIt purely depends on how the the library, was compiled. I just wrote\nit like that because compiling the pattern is not a hot codepath (i.e.\nwe call this max 8 or so times or so, whereas exec will be called\nthousands/millions/billions of times), so trying to avoid calling this\ntrivial function seemed pointless.\n\nBut looking at this again it would be simpler to combine what you're\nsuggesting with just passing a pointer to *.pcre[12]_jit_on directly,\nskipping the canjit variables.\n\n>> @@ -365,9 +368,20 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n>> if (!p->pcre1_regexp)\n>> compile_regexp_failed(p, error);\n>>\n>> - p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 0, &error);\n>> + p->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);\n>> if (!p->pcre1_extra_info && error)\n>> die(\"%s\", error);\n>> +\n>> +#ifdef PCRE_CONFIG_JIT\n>> + pcre_config(PCRE_CONFIG_JIT, &canjit);\n>> + if (canjit == 1) {\n>> + p->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);\n>> + if (!p->pcre1_jit_stack)\n>> + die(\"BUG: Couldn't allocate PCRE JIT stack\");\n>\n> I agree that dying is OK, but as far as I can tell, this is not a\n> BUG (there is no error a programmer can correct by a follow-up\n> patch); please do not mark it as such (it is likely that we'll later\n> do a tree-wide s/die(\"BUG:/BUG(\"/ and this will interfere).\n\nMakes sense. Looks like the convention for this sort of thing is to\njust do s/BUG: //, e.g. the code in wrapper.c does that.\n\n>> + pcre_assign_jit_stack(p->pcre1_extra_info, NULL, p->pcre1_jit_stack);\n>> + p->pcre1_jit_on = 1;\n>\n> Contrary to what I wondered about \"canjit\" above, I think it makes\n> tons of sense to contain the \"is JIT in use?\" information in \"struct\n> grep_pat\" and not rely on any global state. Not that we are likely\n> to want to be able to JIT some patterns while not doing others. So\n> I agree with the design choice of adding pcre1_jit_on field to the\n> structure.\n>\n> But then, wouldn't it make more sense to do all of the above without\n> the canjit variable at all? i.e. something like...\n>\n> #ifdef PCRE_CONFIG_JIT\n> pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);\n> if (p->pcre1_jit_on)\n> ... stack thing ...\n> #else\n> p->pcre1_jit_on = 0;\n> #endif\n\n*Nod*\n\n>> +#ifdef PCRE_CONFIG_JIT\n>> + if (p->pcre1_jit_on) {\n>> + pcre_free_study(p->pcre1_extra_info);\n>> + pcre_jit_stack_free(p->pcre1_jit_stack);\n>> + } else\n>> +#endif\n>> + /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n>> pcre_free(p->pcre1_extra_info);\n>> +\n>> pcre_free((void *)p->pcre1_tables);\n>\n> It is very thoughtful to add a blank line here (and you did the same\n> in another similar hunk), but I have a feeling that it is still a\n> bit too subtle a hint to signal to the readers that these two\n> pcre_free()s fire differently, i.e. the former does not fire if jit\n> is on but the latter always fires.\n>\n> Would this be a bit safer while being not too ugly to live, I wonder?\n>\n> #ifdef PCRE_CONFIG_JIT\n> if (p->pcre1_jit_on) {\n> pcre_free_study(p->pcre1_extra_info);\n> pcre_jit_stack_free(p->pcre1_jit_stack);\n> } else\n> #endif\n> {\n> /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */\n> pcre_free(p->pcre1_extra_info);\n> }\n> pcre_free((void *)p->pcre1_tables);\n>\n\nMakes sense. I'll change it.\n"},{"id":"320702","messageId":"CACBZZX5Bp43eoWYvpnTtkwA8LyHzwZmrumxBG=-XXuqu1DMKSw@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqa862hixh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 7/7] grep: add support for PCRE v2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-25T09:49:15Z","receivedAt":"2017-05-25T09:49:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 24, 2017 at 8:23 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Add support for v2 of the PCRE API. This is a new major version of\n>> PCRE that came out in early 2015[1].\n>>\n>> The regular expression syntax is the same, but while the API is\n>> similar, pretty much every function is either renamed or takes\n>> different arguments. Thus using it via entirely new functions makes\n>> sense, as opposed to trying to e.g. have one compile_pcre_pattern()\n>> that would call either PCRE v1 or v2 functions.\n>>\n>> Git can now be compiled with either USE_LIBPCRE1=YesPlease or\n>> USE_LIBPCRE2=YesPlease, with USE_LIBPCRE=YesPlease currently being a\n>> synonym for the former. Providing both is a compile-time error.\n>>\n>> With earlier patches to enable JIT for PCRE v1 the performance of the\n>> release versions of both libraries is almost exactly the same, with\n>> PCRE v2 being around 1% slower.\n>>\n>> However after I reported this to the pcre-dev mailing list[2] I got a\n>> lot of help with the API use from Zoltán Herczeg, he subsequently\n>> optimized some of the JIT functionality in v2 of the library.\n>>\n>> Running the p7820-grep-engines.sh performance test against the latest\n>> Subversion trunk of both, with both them and git compiled as -O3, and\n>> the test run against linux.git, gives the following results. Just the\n>> /perl/ tests shown:\n>>\n>> $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_COMMAND='grep -q LIBPCRE2 Makefile && make -j8 USE_LIBPCRE2=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre2/inst/lib || make -j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~2 HEAD~ HEAD p7820-grep-engines.sh\n>> [...]\n>> Test HEAD~2 HEAD~ HEAD\n>> ----------------------------------------------------------------------------------------------------------------\n>> 7820.3: perl grep 'how.to' 0.22(0.40+0.48) 0.22(0.31+0.58) +0.0% 0.22(0.26+0.59) +0.0%\n>> 7820.7: perl grep '^how to' 0.27(0.62+0.50) 0.28(0.60+0.50) +3.7% 0.22(0.25+0.60) -18.5%\n>> 7820.11: perl grep '[how] to' 0.33(0.92+0.47) 0.33(0.94+0.45) +0.0% 0.25(0.42+0.51) -24.2%\n>> 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.35(1.08+0.46) 0.35(1.12+0.41) +0.0% 0.25(0.52+0.50) -28.6%\n>> 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.78+0.51) 0.30(0.86+0.42) +0.0% 0.25(0.29+0.54) -16.7%\n>>\n>> See commit (\"perf: add a comparison test of grep regex engines\",\n>> 2017-04-19) for details on the machine the above test run was executed\n>> on.\n>>\n>> Here HEAD~2 is git with PCRE v1 without JIT, HEAD~ is PCRE v1 with\n>> JIT, and HEAD is PCRE v2 (also with JIT). See previous commits of mine\n>> mentioning p7820-grep-engines.sh for more details on the test setup.\n>>\n>> For ease of readability, a different run just of HEAD~ (PCRE v1 with\n>> JIT v.s. PCRE v2), again with just the /perl/ tests shown:\n>>\n>> Test HEAD~ HEAD\n>> ---------------------------------------------------------------------------------------\n>> 7820.3: perl grep 'how.to' 0.23(0.41+0.47) 0.23(0.26+0.59) +0.0%\n>> 7820.7: perl grep '^how to' 0.27(0.64+0.47) 0.23(0.28+0.56) -14.8%\n>> 7820.11: perl grep '[how] to' 0.34(0.95+0.44) 0.25(0.38+0.56) -26.5%\n>> 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.34(1.07+0.46) 0.24(0.52+0.49) -29.4%\n>> 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.81+0.46) 0.22(0.33+0.54) -26.7%\n>>\n>> I.e. the two are either neck-to-neck, but PCRE v2 usually pulls ahead,\n>> when it does it's around 20% faster.\n>>\n>> A brief note on thread safety: As noted in pcre2api(3) & pcre2jit(3)\n>> the compiled pattern can be shared between threads, but not some of\n>> the JIT context, however the grep threading support does all pattern &\n>> JIT compilation in separate threads, so this code doesn't need to\n>> concern itself with thread safety.\n>\n> Nicely explained.\n>\n>> -# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in\n>> +# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define\n>> +# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE\n>> +# library. The USE_LIBPCRE flag will likely be changed to mean v2 by\n>> +# default in future releases.\n>> +#\n>> +# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in\n>> # /foo/bar/include and /foo/bar/lib directories.\n>\n> As there is no way to use both, having a single LIBPCREDIR is not a\n> hurting limitation, which makes sense.\n\nWill nevertheless add a comment to clarify this.\n\n>> @@ -2241,6 +2258,7 @@ GIT-BUILD-OPTIONS: FORCE\n>> @echo NO_CURL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_CURL)))'\\' >>$@+\n>> @echo NO_EXPAT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_EXPAT)))'\\' >>$@+\n>> @echo USE_LIBPCRE1=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE)))'\\' >>$@+\n>\n> Shouldn't the line above record $(USE_LIBPCRE1) instead of the\n> generic fallback?\n\nYes, will fix.\n\n>> + @echo USE_LIBPCRE2=\\''$(subst ','\\'',$(subst ','\\'',$(USE_LIBPCRE2)))'\\' >>$@+\n>> @echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n>> @echo NO_PTHREADS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PTHREADS)))'\\' >>$@+\n>> @echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n>\n>> diff --git a/grep.c b/grep.c\n>> index 3c0c30f033..569cf9e290 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -179,22 +179,36 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n>> case GREP_PATTERN_TYPE_BRE:\n>> opt->fixed = 0;\n>> opt->pcre1 = 0;\n>> + opt->pcre2 = 0;\n>> break;\n>>\n>> case GREP_PATTERN_TYPE_ERE:\n>> opt->fixed = 0;\n>> opt->pcre1 = 0;\n>> + opt->pcre2 = 0;\n>> opt->regflags |= REG_EXTENDED;\n>> break;\n>>\n>> case GREP_PATTERN_TYPE_FIXED:\n>> opt->fixed = 1;\n>> opt->pcre1 = 0;\n>> + opt->pcre2 = 0;\n>> break;\n>>\n>> case GREP_PATTERN_TYPE_PCRE:\n>> opt->fixed = 0;\n>> +#ifdef USE_LIBPCRE2\n>> + opt->pcre1 = 0;\n>> + opt->pcre2 = 1;\n>> +#else\n>> + /* It's important that pcre1 always be assigned to\n>> + * even when there's no USE_LIBPCRE* defined. We still\n>> + * call the PCRE stub function, it just dies with\n>> + * \"cannot use Perl-compatible regexes[...]\".\n>> + */\n>> opt->pcre1 = 1;\n>\n> Very well thought-out comment. Our style wants you to have\n> slash-aster that opens a multi-line comment on its own line, though.\nWill fix.\n>> + opt->pcre2 = 0;\n>> +#endif\n>> break;\n>> }\n>> }\n>> @@ -446,6 +460,126 @@ 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>> +...\n>> + p->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n>> + p->patternlen, options, &error, &erroffset,\n>> + p->pcre2_compile_context);\n>\n> Are all die(\"BUG:...\") in this function actual bugs, or just\n> \"die()\"? Just like the comment on an earlier patch, things like\n> running out of memory that you as a Git programmer cannot fix by\n> correcting this code are not die(\"BUG:\"), but normal runtime errors.\nWill fix these.\n>> +\n>> + if (p->pcre2_pattern) {\n>> + p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n>> + if (!p->pcre2_match_data)\n>> + die(\"BUG: Couldn't allocate PCRE2 match data\");\n>> + } else {\n>> + pcre2_get_error_message(error, errbuf, sizeof(errbuf));\n>> + compile_regexp_failed(p, (const char *)&errbuf);\n>> + }\n>> +\n>> + pcre2_config(PCRE2_CONFIG_JIT, &canjit);\n>> + if (canjit == 1) {\n>> + jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n>> + if (!jitret)\n>> + p->pcre2_jit_on = 1;\n>\n> I think the same \"would it be better to do this without canjit?\"\n> comment applies here.\nYup, changed.\n>> +#else /* !USE_LIBPCRE2 */\n>> +static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n>> +{\n>> + /* Unreachable until USE_LIBPCRE2 becomes synonymous with\n>> + * USE_LIBPCRE. See the sibling comment in\n>> + * grep_set_pattern_type_option().\n>> + */\n>> + die(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\n>> +}\n>\n> Wow. If I were doing this, I wouldn't have been this cautious, but\n> I have no complaints ;-).\n>\n"},{"id":"320704","messageId":"CACBZZX51hmq-jCkwyDt6QWfLJ+qxziUV-Bef+yVDoViLNhC4dw@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqvaoqhnlw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-25T10:33:32Z","receivedAt":"2017-05-25T10:33:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 24, 2017 at 6:42 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Rather, it's just to make the code easier to reason about. It's\n>> confusing to debug this under threading & non-threading when the\n>> threading codepaths redundantly compile a pattern which is never used.\n>>\n>> The reason the patterns are recompiled is as a side-effect of\n>> duplicating the whole grep_opt structure, which is not thread safe,\n>> writable, and munged during execution. The grep_opt structure then\n>> points to the grep_pat structure where pattern or patterns are stored.\n>>\n>> I looked into e.g. splitting the API into some \"do & alloc threadsafe\n>> stuff\", \"spawn thread\", \"do and alloc non-threadsafe stuff\", but the\n>> execution time of grep_opt_dup() & pattern compilation is trivial\n>> compared to actually executing the grep, so there was no point. Even\n>> with the more expensive JIT changes to follow the most expensive PCRE\n>> patterns take something like 0.0X milliseconds to compile at most[1].\n>\n> OK.\n>\n>> The undocumented --debug mode added in commit 17bf35a3c7 (\"grep: teach\n>> --debug option to dump the parse tree\", 2012-09-13) still works\n>> properly with this change. It only emits debugging info during pattern\n>> compilation, which is now dumped by the pattern compiled just before\n>> the first thread is started.\n>\n> When opt is passed to run(), opt->debug is still true for the first\n> worker thread. As long as opt->debug never makes difference after\n> compile_grep_patterns(opt) returns, I think the change in this patch\n> safe.\n\nRight, the --debug feature only impacts pattern compilation.\n\n> I do not know if we want to rely on it, but we can explain it\n> away by saying \"we'll only debug the runtime behaviour for the first\n> worker only\", or something, so it is not a big deal either way.\n\nI think it's a pointless distraction to start speculating in this\ncommit message what we're going to do with --debug it if it ever\nstarts emitting some debugging information at pattern execution time.\n\nAs an aside, I'd very much like to remove both --debug and the\n--and/--or/--all-match, gives some very rough edges in the UI and how\neasy it is to make that feature error or segfault, I suspect you might\nbe the only one using it.\n\nThere are pattern matching optimizations I'd like to do that are much\nmore of a pain with that feature around. It's easy to AND multiple\nregexes together into one match via -e, but when you have to deal with\nnegation and arbitrarily complex chained & parenthesized AND/OR you\nend up having to run your custom state machine on every line with\nmultiple regex matches per line.\n\nThe system grep doesn't have this feature, and people seem to do\nwithout it. The motivation for it isn't explained in commit 79d3696cfb\n(\"git-grep: boolean expression on pattern matching.\", 2006-06-30), but\nI suspect it's a hack around not being able to do \"git grep ... | git\ngrep -v ...\", which is how you'd do \"I'd like to match this, but not\nthat\" with the system grep.\n\nJust supporting that would be much easier than supporting the and/or\nmatching machinery.\n"},{"id":"320778","messageId":"xmqqlgpkcu2v.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"CACBZZX51hmq-jCkwyDt6QWfLJ+qxziUV-Bef+yVDoViLNhC4dw@mail.gmail.com","subject":"Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-26T00:58:32Z","receivedAt":"2017-05-26T00:58:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think it's a pointless distraction to start speculating in this\n> commit message what we're going to do with --debug it if it ever\n> starts emitting some debugging information at pattern execution time.\n\nOK.\n\n> As an aside, I'd very much like to remove both --debug and the\n> --and/--or/--all-match, gives some very rough edges in the UI and how\n> easy it is to make that feature error or segfault, I suspect you might\n> be the only one using it.\n\nI agree that rewriting \"grep -e A -e B\" to \"grep -e A|B\" as an\noptimization is an interesting possibility to look into, and I can\nunderstand that having to support \"--and\" and \"--not\" would\nmake such an optimization harder to implement. \"-e A --and -e B\"\nmust become \"-e A.*B|B.*A\" and as you get more terms your unified\npattern will grow combinatorial, at which point you would be better\noff matching N patterns and combining the result.\n\nEver saw a user run \"ps | grep rogue | grep -v grep\" to find a rogue\nprocess to kill? That would not work if the rogue process's command\nline has a word \"grep\". Because \"git grep\" is often run on files in\norder to find the location the patterns appear in, \"git grep -e\npattern | grep -v unwanted\" shares the same issue--the unwanted\npattern may appear in the filename, and the downstream \"grep -v\" may\nfilter out a valid hit. This is why \"--not\" exists [*1*]. I agree\nthat emulating it within the same \"concatenate patterns into one\"\noptimization you are envisioning may be hard.\n\nAttempting to optimize \"--all-match\" would share similar difficulty\nwith \"--and\", but your matching now must be done with the entire\nbuffer and not go line-by-line. It was meant to make it possible to\nsay \"find commits that avarab@ talks about both regex and log\", i.e.\n\n\t$ git log --author=avarab@ --all-match --grep=log --grep=regex\n\nThis is not something you can emulate by piping an output of grep to\nanother grep.\n\nBut none of the above means you have to give up optimizing. \n\nYou can choose not to combine them into a single pattern if certain\nconstructions are hard, and do only the easy ones. If you think\nthat harder combinations are not used very often, the result would\nbe faster for many cases while not losing useful features, which is\nwhat we want.\n\n\n[Footnote]\n\n*1* For human consumption, lack of \"--not\" may not hurt in the sense\n that there are workarounds (i.e. you can do without \"| grep -v\n unwanted\" and filter irrelevant ones by eyeballing). But it is\n essential while scripting and trying to be precise.\n"},{"id":"320809","messageId":"CACBZZX7i2RqyjXmF8wxYvs0d1nijiopVQ2EUM5rmpztEbe6tKg@mail.gmail.com","threadId":"46011","inReplyTo":"xmqqlgpkcu2v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-26T08:06:01Z","receivedAt":"2017-05-26T08:06:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, May 26, 2017 at 2:58 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I think it's a pointless distraction to start speculating in this\n>> commit message what we're going to do with --debug it if it ever\n>> starts emitting some debugging information at pattern execution time.\n>\n> OK.\n>\n>> As an aside, I'd very much like to remove both --debug and the\n>> --and/--or/--all-match, gives some very rough edges in the UI and how\n>> easy it is to make that feature error or segfault, I suspect you might\n>> be the only one using it.\n>\n> I agree that rewriting \"grep -e A -e B\" to \"grep -e A|B\" as an\n> optimization is an interesting possibility to look into, and I can\n> understand that having to support \"--and\" and \"--not\" would\n> make such an optimization harder to implement. \"-e A --and -e B\"\n> must become \"-e A.*B|B.*A\" and as you get more terms your unified\n> pattern will grow combinatorial, at which point you would be better\n> off matching N patterns and combining the result.\n>\n> Ever saw a user run \"ps | grep rogue | grep -v grep\" to find a rogue\n> process to kill? That would not work if the rogue process's command\n> line has a word \"grep\". Because \"git grep\" is often run on files in\n> order to find the location the patterns appear in, \"git grep -e\n> pattern | grep -v unwanted\" shares the same issue--the unwanted\n> pattern may appear in the filename, and the downstream \"grep -v\" may\n> filter out a valid hit. This is why \"--not\" exists [*1*]. I agree\n> that emulating it within the same \"concatenate patterns into one\"\n> optimization you are envisioning may be hard.\n>\n> Attempting to optimize \"--all-match\" would share similar difficulty\n> with \"--and\", but your matching now must be done with the entire\n> buffer and not go line-by-line. It was meant to make it possible to\n> say \"find commits that avarab@ talks about both regex and log\", i.e.\n>\n> $ git log --author=avarab@ --all-match --grep=log --grep=regex\n>\n> This is not something you can emulate by piping an output of grep to\n> another grep.\n>\n> But none of the above means you have to give up optimizing.\n>\n> You can choose not to combine them into a single pattern if certain\n> constructions are hard, and do only the easy ones. If you think\n> that harder combinations are not used very often, the result would\n> be faster for many cases while not losing useful features, which is\n> what we want.\n\nTo be clear the point of my mail was not to say \"I can't think of a\nway to support both of these things, help!\", obviously we can continue\nto maintain two codepaths. The point was to raise the idea that we\ncould simply remove the more complex & doomed to forever be slow\ncodepath.\n\nObviously there are caveats with the likes of \"grep foo | grep bar\"\nthat don't exist with \"grep -e foo --and -e bar\". I'm less interested\nin whether we can come up with cases that wouldn't be possible if this\nwere removed, than if anyone's using them in practice.\n\nI suspect that to the extent anyone uses this for common things it\ncould be emulated by --single-line --perl-regexp and e.g. 'foo.*bar'\ninstead of 'foo' --and 'bar'. I.e. we could offer to AND together your\nregexes and match them over the entire content.\n\nIf someone needed something more complex we could just show an example\nof piping e.g. \\0-delimited commit messages into an arbitrary perl\nscript you provide.\n\nAnyway, I've only looked this over a tiny bit, and I don't know\nwhether it's worth it to remove this, right now I was just interested\nin some reports of what it was used for. I.e. whether anyone uses it\nfor N-level deep mixed AND/OR branches, or whether it's really just a\nlazy way to concat regexes and get around the current limitation of\nnot being able to match across lines.\n\n> [Footnote]\n>\n> *1* For human consumption, lack of \"--not\" may not hurt in the sense\n> that there are workarounds (i.e. you can do without \"| grep -v\n> unwanted\" and filter irrelevant ones by eyeballing). But it is\n> essential while scripting and trying to be precise.\n"},{"id":"320812","messageId":"xmqqmva0aqya.fsf@gitster.mtv.corp.google.com","threadId":"46011","inReplyTo":"CACBZZX7i2RqyjXmF8wxYvs0d1nijiopVQ2EUM5rmpztEbe6tKg@mail.gmail.com","subject":"Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-26T09:49:01Z","receivedAt":"2017-05-26T09:49:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> To be clear the point of my mail was not to say \"I can't think of a\n> way to support both of these things, help!\", obviously we can continue\n> to maintain two codepaths. The point was to raise the idea that we\n> could simply remove the more complex & doomed to forever be slow\n> codepath.\n\nTo be clear, the point of my response was that these features must\nremain. As long as they are more convenient than sifting through\noutput produced by pattern matching engine that is less powerful\n(which forces the user to give wider pattern than desired, to avoid\nfalse negatives) with eyeball, having to match each pattern one by\none, instead of being able to use a combined and more efficient\nsingle pattern, is still more efficient for the end user, which is\nthe point of using a computer.\n"}]}