Easy to review? 29 (I mean 30) patches? Are you kidding me?!
As noted in v1 ([off-list ref];
https://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)
these are all doc, test, refactoring etc. changes needed by the
subsequent "PCRE v2, PCRE v1 JIT, log -P & fixes" series.
Since Junio hasn't been picking it I'm no longer sending updates to
that patch series & waiting for this one to cook first.
See [off-list ref]
(https://public-inbox.org/git/20170513231509.7834-1-avarab@gmail.com/)
for v2 & notes about that version. What changed this time around? See
below:
Ævar Arnfjörð Bjarmason (30):
Makefile & configure: reword inaccurate comment about PCRE
grep & rev-list doc: stop promising libpcre for --perl-regexp
test-lib: rename the LIBPCRE prerequisite to PCRE
No changes.
log: add exhaustive tests for pattern style options & config
Test comment clarifications in t4202-log.sh as pointed out by Junio.
log: make --regexp-ignore-case work with --perl-regexp
NEW: I noticed that the `-i` in `git log --perl-regexp -i --grep=<rx>`
never worked as intended. I.e. the flag for ignoring the case of the
pattern wasn't picked up.
Fixing this was trivial (one-line change), so I've included it in this
series since it's needed by a new t/perf patch (see below).
grep: add a test asserting that --perl-regexp dies when !PCRE
grep: add a test for backreferences in PCRE patterns
grep: change non-ASCII -i test to stop using --debug
grep: add tests for --threads=N and grep.threads
grep: amend submodule recursion test for regex engine testing
grep: add tests for grep pattern types being passed to submodules
No changes.
grep: add a test helper function for less verbose -f \0 tests
Trivial style changes in nul_match() suggested by Junio. No functional
changes.
grep: prepare for testing binary regexes containing rx metacharacters
No changes.
grep: add tests to fix blind spots with \0 patterns
Continued trivial style changes in nul_match() (the other half of the
code in that function is added in this commit)>
perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do
perf: emit progress output when unpacking & building
No changes.
perf: add a comparison test of grep regex engines
perf: add a comparison test of grep regex engines with -F
perf: add a comparison test of log --grep regex engines
The log --grep test is new, and all these tests learned to take an env
variable to pass arbitrary extra grep/log flags, so I can e.g. test
with -i, -v, -w etc.
Subsequent commit messages that e.g. mentioned perf tests with the
previous hardcoded -i test have been amended to mention the new test
results.
grep: catch a missing enum in switch statement
Grammar fix in commit message.
grep: remove redundant regflags assignments
The two commits that made changes to regflags assignments have been
squashed.
grep: factor test for \0 in grep patterns into a function
Rewrote commit message to not go off on a tangent about what grep -f
[file-with-\0-pattern] should mean, which is not what this change is
about.
grep: change the internal PCRE macro names to be PCRE1
grep: change internal *pcre* variable & function names to be *pcre1*
grep: move is_fixed() earlier to avoid forward declaration
test-lib: add a PTHREADS prerequisite
No changes.
pack-objects & index-pack: add test for --threads warning
pack-objects: fix buggy warning about threads
Rewrote the tests in these two so that the first one sets up a failing
test which is subsequently fixed in the commit that fixes the bug, as
suggested by Junio.
Removed a stray `cat err` left over from debugging.
grep: given --threads with NO_PTHREADS=YesPlease, warn
grep: assert that threading is enabled when calling grep_{lock,unlock}
No changes.
Documentation/git-grep.txt | 7 +-
Documentation/rev-list-options.txt | 8 +-
Makefile | 14 ++-
builtin/grep.c | 23 +++-
builtin/pack-objects.c | 4 +-
configure.ac | 12 ++-
grep.c | 108 ++++++++++---------
grep.h | 10 +-
revision.c | 1 +
t/README | 8 +-
t/perf/README | 19 +++-
t/perf/p4220-log-grep-engines.sh | 44 ++++++++
t/perf/p7820-grep-engines.sh | 47 ++++++++
t/perf/p7821-grep-engines-fixed.sh | 32 ++++++
t/perf/run | 13 ++-
t/t4202-log.sh | 160 +++++++++++++++++++++++++--
t/t5300-pack-object.sh | 36 +++++++
t/t7008-grep-binary.sh | 135 +++++++++++++++++------
t/t7810-grep.sh | 81 +++++++++++---
t/t7812-grep-icase-non-ascii.sh | 29 ++---
t/t7813-grep-icase-iso.sh | 2 +-
t/t7814-grep-recurse-submodules.sh | 215 +++++++++++++++++++++++--------------
t/test-lib.sh | 3 +-
23 files changed, 771 insertions(+), 240 deletions(-)
create mode 100755 t/perf/p4220-log-grep-engines.sh
create mode 100755 t/perf/p7820-grep-engines.sh
create mode 100755 t/perf/p7821-grep-engines-fixed.sh
--
2.13.0.303.g4ebf302169
Reword an outdated & inaccurate comment which suggests that only
git-grep can use PCRE.
This comment was added back when PCRE support was initially added in
commit 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09), and was true
at the time.
It hasn't been telling the full truth since git-log learned to use
PCRE with --grep in commit 727b6fc3ed ("log --grep: accept
--basic-regexp and --perl-regexp", 2012-10-03), and more importantly
is likely to get more inaccurate over time as more use is made of PCRE
in other areas.
Reword it to be more future-proof, and to more clearly explain that
this enables user-initiated runtime behavior.
Copy/pasting this so much in configure.ac is lame, these Makefile-like
flags aren't even used by autoconf, just the corresponding
--with[out]-* options. But copy/pasting the comments that make sense
for the Makefile to configure.ac where they make less sense is the
pattern everything else follows in that file. I'm not going to war
against that as part of this change, just following the existing
pattern.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 6 ++++--
configure.ac | 12 ++++++++----
2 files changed, 12 insertions(+), 6 deletions(-)
@@ -24,8 +24,10 @@ all::# Define NO_OPENSSL environment variable if you do not have OpenSSL.# This also implies BLK_SHA1.#-# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be-# able to use Perl-compatible regular expressions.+# Define USE_LIBPCRE if you have and want to use libpcre. Various+# commands such as log and grep offer runtime options to use+# Perl-compatible regular expressions instead of standard or extended+# POSIX regular expressions.## Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in# /foo/bar/include and /foo/bar/lib directories.
@@ -250,8 +250,10 @@ AS_HELP_STRING([--with-openssl],[use OpenSSL library (default is YES)]) AS_HELP_STRING([], [ARG can be prefix for openssl library and headers]), GIT_PARSE_WITH([openssl]))-# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be-# able to use Perl-compatible regular expressions.+# Define USE_LIBPCRE if you have and want to use libpcre. Various+# commands such as log and grep offer runtime options to use+# Perl-compatible regular expressions instead of standard or extended+# POSIX regular expressions. # # Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in # /foo/bar/include and /foo/bar/lib directories.
@@ -499,8 +501,10 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO]) GIT_CONF_SUBST([NO_OPENSSL]) #-# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be-# able to use Perl-compatible regular expressions.+# Define USE_LIBPCRE if you have and want to use libpcre. Various+# commands such as log and grep offer runtime options to use+# Perl-compatible regular expressions instead of standard or extended+# POSIX regular expressions. # if test -n "$USE_LIBPCRE"; then
Stop promising in our grep & rev-list options documentation that we're
always going to be using libpcre when given the --perl-regexp option.
Instead talk about using "Perl-compatible regular expressions" and
using these types of patterns using "a compile-time dependency".
Saying "libpcre" means that we're talking about libpcre.so, which is
always going to be v1. This change is part of an ongoing saga to add
support for libpcre2, which comes with PCRE v2.
In the future we might use some completely unrelated library to
provide perl-compatible regular expression support. By wording the
documentation differently and not promising any specific version of
PCRE or even PCRE at all we have more wiggle room to change the
implementation.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-grep.txt | 7 +++++--
Documentation/rev-list-options.txt | 8 ++++++--
2 files changed, 11 insertions(+), 4 deletions(-)
@@ -161,8 +161,11 @@ OPTIONS -P:: --perl-regexp::- Use Perl-compatible regexp for patterns. Requires libpcre to be- compiled in.+ Use Perl-compatible regular expressions for patterns.+++Support for these types of regular expressions is an optional+compile-time dependency. If Git wasn't compiled with support for them+providing this option will cause it to die. -F:: --fixed-strings::
@@ -92,8 +92,12 @@ endif::git-rev-list[] pattern as a regular expression). --perl-regexp::- Consider the limiting patterns to be Perl-compatible regular expressions.- Requires libpcre to be compiled in.+ Consider the limiting patterns to be Perl-compatible regular+ expressions.+++Support for these types of regular expressions is an optional+compile-time dependency. If Git wasn't compiled with support for them+providing this option will cause it to die. --remove-empty:: Stop when a given path disappears from the tree.
Rename the LIBPCRE prerequisite to PCRE. This is for preparation for
libpcre2 support, where having just "LIBPCRE" would be confusing as it
implies v1 of the library.
None of these tests are incompatible between versions 1 & 2 of
libpcre, it's less confusing to give them a more general name to make
it clear that they work on both library versions.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/README | 4 ++--
t/t7810-grep.sh | 28 ++++++++++++++--------------
t/t7812-grep-icase-non-ascii.sh | 4 ++--
t/t7813-grep-icase-iso.sh | 2 +-
t/test-lib.sh | 2 +-
5 files changed, 20 insertions(+), 20 deletions(-)
@@ -803,9 +803,9 @@ use these, and "test_set_prereq" for how to define your own. Test is not run by root user, and an attempt to write to an unwritable file is expected to fail correctly.- - LIBPCRE+ - PCRE- Git was compiled with USE_LIBPCRE=YesPlease. Wrap any tests+ Git was compiled with support for PCRE. Wrap any tests that use git-grep --perl-regexp or git-grep -P in these. - CASE_INSENSITIVE_FS
@@ -1118,11 +1118,11 @@ test_expect_success 'grep invalidpattern properly dies with grep.patternType=exttest_must_failgit-cgrep.patterntype=extendedgrep"a["'-test_expect_successLIBPCRE'grep -P invalidpattern properly dies ''+test_expect_successPCRE'grep -P invalidpattern properly dies ''test_must_failgitgrep-P"a["'-test_expect_successLIBPCRE'grep invalidpattern properly dies with grep.patternType=perl''+test_expect_successPCRE'grep invalidpattern properly dies with grep.patternType=perl''test_must_failgit-cgrep.patterntype=perlgrep"a["'
@@ -1014,7 +1014,7 @@ esac(COLUMNS=1&&test$COLUMNS=1)&&test_set_prereqCOLUMNS_CAN_BE_1test-z"$NO_PERL"&&test_set_prereqPERLtest-z"$NO_PYTHON"&&test_set_prereqPYTHON-test-n"$USE_LIBPCRE"&&test_set_prereqLIBPCRE+test-n"$USE_LIBPCRE"&&test_set_prereqPCREtest-z"$NO_GETTEXT"&&test_set_prereqGETTEXT# Can we rely on git's output in the C locale?
Add exhaustive tests for how the different grep.patternType options &
the corresponding command-line options affect git-log.
Before this change it was possible to patch revision.c so that the
--basic-regexp option was synonymous with --extended-regexp, and
--perl-regexp wasn't recognized at all, and still have 100% of the
test suite pass.
This was because the first test being modified here, added in commit
34a4ae55b2 ("log --grep: use the same helper to set -E/-F options as
"git grep"", 2012-10-03), didn't actually check whether we'd enabled
extended regular expressions as distinct from re-toggling non-fixed
string support.
Fix that by changing the pattern to a pattern that'll only match if
--extended-regexp option is provided, but won't match under the
default --basic-regexp option.
Other potential regressions were possible since there were no tests
for the rest of the combinations of grep.patternType configuration
toggles & corresponding git-log command-line options. Add exhaustive
tests for those.
The patterns being passed to fixed/basic/extended/PCRE are carefully
crafted to return the wrong thing if the grep engine were to pick any
other matching method than the one it's told to use.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t4202-log.sh | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 1 deletion(-)
@@ -262,7 +262,30 @@ test_expect_success 'log --grep -i' ' test_expect_success'log -F -E --grep=<ere> uses ere''echosecond>expect&&-gitlog-1--pretty="tformat:%s"-F-E--grep=s.c.nd>actual&&+# basic would need \(s\) to do the same+gitlog-1--pretty="tformat:%s"-F-E--grep="(s).c.nd">actual&&+test_cmpexpectactual+'++test_expect_successPCRE'log -F -E --perl-regexp --grep=<pcre> uses PCRE''+test_when_finished"rm -rf num_commits"&&+gitinitnum_commits&&+(+cdnum_commits&&+test_commit1d&&+test_commit2e+)&&++# In PCRE \d in [\d] is like saying "0-9", and matches the 2+# in 2e...+echo2e>expect&&+git-Cnum_commitslog-1--pretty="tformat:%s"-F-E--perl-regexp--grep="[\d]">actual&&+test_cmpexpectactual&&++# ...in POSIX basic and extended it is the same as [d],+# i.e. "d", which matches 1d, but does not match 2e.+echo1d>expect&&+git-Cnum_commitslog-1--pretty="tformat:%s"-F-E--grep="[\d]">actual&&test_cmpexpectactual'
@@ -280,6 +303,79 @@ test_expect_success 'log with grep.patternType configuration and command line' 'test_cmpexpectactual'+test_expect_success'log with various grep.patternType configurations & command-lines''+gitinitpattern-type&&+(+cdpattern-type&&+test_commit1fileA&&++# The tagname is overridden here because creating a+# tag called "(1|2)" as test_commit would otherwise+# implicitly do would fail on e.g. MINGW.+test_commit"(1|2)"fileB2&&++echo"(1|2)">expect.fixed&&+cpexpect.fixedexpect.basic&&+cpexpect.fixedexpect.extended&&+cpexpect.fixedexpect.perl&&++# A strcmp-like match with fixed.+git-cgrep.patternType=fixedlog--pretty=tformat:%s\+--grep="(1|2)">actual.fixed&&++# POSIX basic matches (, | and ) literally.+git-cgrep.patternType=basiclog--pretty=tformat:%s\+--grep="(.|.)">actual.basic&&++# POSIX extended needs to have | escaped to match it+# literally, whereas under basic this is the same as+# (|2), i.e. it would also match "1". This test checks+# for extended by asserting that it is not matching+# what basic would match.+git-cgrep.patternType=extendedlog--pretty=tformat:%s\+--grep="\|2">actual.extended&&+iftest_have_prereqPCRE+then+# Only PCRE would match [\d]\| with only+# "(1|2)" due to [\d]. POSIX basic would match+# both it and "1" since similarly to the+# extended match above it is the same as+# \([\d]\|\). POSIX extended would+# match neither.+git-cgrep.patternType=perllog--pretty=tformat:%s\+--grep="[\d]\|">actual.perl&&+test_cmpexpect.perlactual.perl+fi&&+test_cmpexpect.fixedactual.fixed&&+test_cmpexpect.basicactual.basic&&+test_cmpexpect.extendedactual.extended&&++gitlog--pretty=tformat:%s-F\+--grep="(1|2)">actual.fixed.short-arg&&+gitlog--pretty=tformat:%s-E\+--grep="\|2">actual.extended.short-arg&&+test_cmpexpect.fixedactual.fixed.short-arg&&+test_cmpexpect.extendedactual.extended.short-arg&&++gitlog--pretty=tformat:%s--fixed-strings\+--grep="(1|2)">actual.fixed.long-arg&&+gitlog--pretty=tformat:%s--basic-regexp\+--grep="(.|.)">actual.basic.long-arg&&+gitlog--pretty=tformat:%s--extended-regexp\+--grep="\|2">actual.extended.long-arg&&+iftest_have_prereqPCRE+then+gitlog--pretty=tformat:%s--perl-regexp\+--grep="[\d]\|">actual.perl.long-arg&&+test_cmpexpect.perlactual.perl.long-arg++fi&&+test_cmpexpect.fixedactual.fixed.long-arg&&+test_cmpexpect.basicactual.basic.long-arg&&+test_cmpexpect.extendedactual.extended.long-arg+)+'+ cat>expect<<EOF *Second *sixth
Add a test for backreferences such as (.)\1 in PCRE patterns. This
test ensures that the PCRE_NO_AUTO_CAPTURE option isn't turned
on. Before this change turning it on would break these sort of
patterns, but wouldn't break any tests.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7810-grep.sh | 7 +++++++
1 file changed, 7 insertions(+)
@@ -1114,6 +1114,13 @@ test_expect_success PCRE 'grep -P -w pattern' 'test_cmpexpectedactual'+test_expect_successPCRE'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE flag is not set)''+gitgrep-P-h"(?P<one>.)(?P=one)"hello_world>actual&&+test_cmphello_worldactual&&+gitgrep-P-h"(.)\1"hello_world>actual&&+test_cmphello_worldactual+'+ test_expect_success'grep -G invalidpattern properly dies ''test_must_failgitgrep-G"a["'
Make the --regexp-ignore-case option work with --perl-regexp. This
never worked, and there was no test for this. Fix the bug and add a
test.
When PCRE support was added in commit 63e7e9d8b6 ("git-grep: Learn
PCRE", 2011-05-09) compile_pcre_regexp() would only check
opt->ignore_case, but when the --perl-regexp option was added in
commit 727b6fc3ed ("log --grep: accept --basic-regexp and
--perl-regexp", 2012-10-03) the code didn't set the opt->ignore_case.
Change the test suite to test for -i and --invert-regexp with
basic/extended/perl patterns in addition to fixed, which was the only
patternType that was tested for before in combination with those
options.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
revision.c | 1 +
t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 56 insertions(+), 5 deletions(-)
Add a test asserting that when --perl-regexp (and -P for grep) is
given to git-grep & git-log that we die with an error.
In developing the PCRE v2 series I introduced a regression where -P
would (through control-flow fall-through) become synonymous with basic
POSIX matching. I.e. 'git grep -P '[\d]' would match "d" instead of
digits.
The entire test suite would still pass with this serious regression,
since everything that tested for --perl-regexp would be guarded by the
PCRE prerequisite, fix that blind-spot by adding tests under !PCRE
asserting that git must die when given --perl-regexp or -P.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t4202-log.sh | 4 +++-
t/t7810-grep.sh | 12 ++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
@@ -281,6 +281,10 @@ dotest_cmpexpectedactual'+test_expect_success!PCRE"grep $L with grep.patterntype=perl errors without PCRE"'+test_must_failgit-cgrep.patterntype=perlgrep"foo.*bar"+'+test_expect_success"grep $L with grep.patternType=default and grep.extendedRegexp=true"'echo"${HC}ab:abc">expected&&git\
@@ -1058,11 +1062,19 @@ test_expect_success PCRE 'grep --perl-regexp pattern' 'test_cmpexpectedactual'+test_expect_success!PCRE'grep --perl-regexp pattern errors without PCRE''+test_must_failgitgrep--perl-regexp"foo.*bar"+'+ test_expect_successPCRE'grep -P pattern''gitgrep-P"\p{Ps}.*?\p{Pe}"hello.c>actual&&test_cmpexpectedactual'+test_expect_success!PCRE'grep -P pattern errors without PCRE''+test_must_failgitgrep-P"foo.*bar"+'+ test_expect_success'grep pattern with grep.extendedRegexp=true''>empty&&test_must_failgit-cgrep.extendedregexp=true\
Change a non-ASCII case-insensitive test case to stop using --debug,
and instead simply test for the expected results.
The test coverage remains the same with this change, but the test
won't break due to internal refactoring.
This test was added in commit 793dc676e0 ("grep/icase: avoid kwsset
when -F is specified", 2016-06-25). It was asserting that the regex
must be compiled with compile_fixed_regexp(), instead test for the
expected results, allowing the underlying implementation to change.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7812-grep-icase-non-ascii.sh | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 deletions(-)
Add a helper function to make the tests which check for patterns with
\0 in them more succinct. Right now this isn't a big win, but
subsequent commits will add a lot more of these tests.
The helper is based on the match() function in t3070-wildmatch.sh.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7008-grep-binary.sh | 58 +++++++++++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 29 deletions(-)
Amend the submodule recursion test to prepare it for subsequent tests
of whether it passes along the grep.patternType to the submodule
greps.
This is the result of searching & replacing:
foobar -> (1|2)d(3|4)
foo -> (1|2)
bar -> (3|4)
Currently there's no tests for whether e.g. -P or -E is correctly
passed along, tests for that will be added in a follow-up change, but
first add content to the tests which will match differently under
different regex engines.
Reuse the pattern established in an earlier commit of mine in this
series ("log: add exhaustive tests for pattern style options &
config", 2017-04-07). The pattern "(.|.)[\d]" will match this content
differently under fixed/basic/extended & perl.
This test code was originally added in commit 0281e487fd ("grep:
optionally recurse into submodules", 2016-12-16).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7814-grep-recurse-submodules.sh | 166 ++++++++++++++++++-------------------
1 file changed, 83 insertions(+), 83 deletions(-)
@@ -9,13 +9,13 @@ submodules. ../test-lib.sh test_expect_success'setup directory structure and submodule''-echo"foobar">a&&+echo"(1|2)d(3|4)">a&&mkdirb&&-echo"bar">b/b&&+echo"(3|4)">b/b&&gitaddab&&gitcommit-m"add a and b"&&gitinitsubmodule&&-echo"foobar">submodule/a&&+echo"(1|2)d(3|4)">submodule/a&&git-Csubmoduleadda&&git-Csubmodulecommit-m"add a"&&gitsubmoduleadd./submodule&&
@@ -24,18 +24,18 @@ test_expect_success 'setup directory structure and submodule' ' test_expect_success'grep correctly finds patterns in a submodule''cat>expect<<-\EOF&&-a:foobar-b/b:bar-submodule/a:foobar+a:(1|2)d(3|4)+b/b:(3|4)+submodule/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodules>actual&&+gitgrep-e"(3|4)"--recurse-submodules>actual&&test_cmpexpectactual' test_expect_success'grep and basic pathspecs''cat>expect<<-\EOF&&-submodule/a:foobar+submodule/a:(1|2)d(3|4)EOFgitgrep-e.--recurse-submodules--submodule>actual&&
@@ -44,7 +44,7 @@ test_expect_success 'grep and basic pathspecs' ' test_expect_success'grep and nested submodules''gitinitsubmodule/sub&&-echo"foobar">submodule/sub/a&&+echo"(1|2)d(3|4)">submodule/sub/a&&git-Csubmodule/subadda&&git-Csubmodule/subcommit-m"add a"&&git-Csubmodulesubmoduleadd./sub&&
@@ -54,117 +54,117 @@ test_expect_success 'grep and nested submodules' 'gitcommit-m"updated submodule"&&cat>expect<<-\EOF&&-a:foobar-b/b:bar-submodule/a:foobar-submodule/sub/a:foobar+a:(1|2)d(3|4)+b/b:(3|4)+submodule/a:(1|2)d(3|4)+submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodules>actual&&+gitgrep-e"(3|4)"--recurse-submodules>actual&&test_cmpexpectactual' test_expect_success'grep and multiple patterns''cat>expect<<-\EOF&&-a:foobar-submodule/a:foobar-submodule/sub/a:foobar+a:(1|2)d(3|4)+submodule/a:(1|2)d(3|4)+submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--and-e"foo"--recurse-submodules>actual&&+gitgrep-e"(3|4)"--and-e"(1|2)"--recurse-submodules>actual&&test_cmpexpectactual' test_expect_success'grep and multiple patterns''cat>expect<<-\EOF&&-b/b:bar+b/b:(3|4)EOF-gitgrep-e"bar"--and--not-e"foo"--recurse-submodules>actual&&+gitgrep-e"(3|4)"--and--not-e"(1|2)"--recurse-submodules>actual&&test_cmpexpectactual' test_expect_success'basic grep tree''cat>expect<<-\EOF&&-HEAD:a:foobar-HEAD:b/b:bar-HEAD:submodule/a:foobar-HEAD:submodule/sub/a:foobar+HEAD:a:(1|2)d(3|4)+HEAD:b/b:(3|4)+HEAD:submodule/a:(1|2)d(3|4)+HEAD:submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD>actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD>actual&&test_cmpexpectactual' test_expect_success'grep tree HEAD^''cat>expect<<-\EOF&&-HEAD^:a:foobar-HEAD^:b/b:bar-HEAD^:submodule/a:foobar+HEAD^:a:(1|2)d(3|4)+HEAD^:b/b:(3|4)+HEAD^:submodule/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD^>actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD^>actual&&test_cmpexpectactual' test_expect_success'grep tree HEAD^^''cat>expect<<-\EOF&&-HEAD^^:a:foobar-HEAD^^:b/b:bar+HEAD^^:a:(1|2)d(3|4)+HEAD^^:b/b:(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD^^>actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD^^>actual&&test_cmpexpectactual' test_expect_success'grep tree and pathspecs''cat>expect<<-\EOF&&-HEAD:submodule/a:foobar-HEAD:submodule/sub/a:foobar+HEAD:submodule/a:(1|2)d(3|4)+HEAD:submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD--submodule>actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD--submodule>actual&&test_cmpexpectactual' test_expect_success'grep tree and pathspecs''cat>expect<<-\EOF&&-HEAD:submodule/a:foobar-HEAD:submodule/sub/a:foobar+HEAD:submodule/a:(1|2)d(3|4)+HEAD:submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD--"submodule*a">actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD--"submodule*a">actual&&test_cmpexpectactual' test_expect_success'grep tree and more pathspecs''cat>expect<<-\EOF&&-HEAD:submodule/a:foobar+HEAD:submodule/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD--"submodul?/a">actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD--"submodul?/a">actual&&test_cmpexpectactual' test_expect_success'grep tree and more pathspecs''cat>expect<<-\EOF&&-HEAD:submodule/sub/a:foobar+HEAD:submodule/sub/a:(1|2)d(3|4)EOF-gitgrep-e"bar"--recurse-submodulesHEAD--"submodul*/sub/a">actual&&+gitgrep-e"(3|4)"--recurse-submodulesHEAD--"submodul*/sub/a">actual&&test_cmpexpectactual' test_expect_success!MINGW'grep recurse submodule colon in name''gitinitparent&&test_when_finished"rm -rf parent"&&-echo"foobar">"parent/fi:le"&&+echo"(1|2)d(3|4)">"parent/fi:le"&&git-Cparentadd"fi:le"&&git-Cparentcommit-m"add fi:le"&&gitinit"su:b"&&test_when_finished"rm -rf su:b"&&-echo"foobar">"su:b/fi:le"&&+echo"(1|2)d(3|4)">"su:b/fi:le"&&git-C"su:b"add"fi:le"&&git-C"su:b"commit-m"add fi:le"&&
@@ -172,30 +172,30 @@ test_expect_success !MINGW 'grep recurse submodule colon in name' 'git-Cparentcommit-m"add submodule"&&cat>expect<<-\EOF&&-fi:le:foobar-su:b/fi:le:foobar+fi:le:(1|2)d(3|4)+su:b/fi:le:(1|2)d(3|4)EOF-git-Cparentgrep-e"foobar"--recurse-submodules>actual&&+git-Cparentgrep-e"(1|2)d(3|4)"--recurse-submodules>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-HEAD:fi:le:foobar-HEAD:su:b/fi:le:foobar+HEAD:fi:le:(1|2)d(3|4)+HEAD:su:b/fi:le:(1|2)d(3|4)EOF-git-Cparentgrep-e"foobar"--recurse-submodulesHEAD>actual&&+git-Cparentgrep-e"(1|2)d(3|4)"--recurse-submodulesHEAD>actual&&test_cmpexpectactual' test_expect_success'grep history with moved submoules''gitinitparent&&test_when_finished"rm -rf parent"&&-echo"foobar">parent/file&&+echo"(1|2)d(3|4)">parent/file&&git-Cparentaddfile&&git-Cparentcommit-m"add file"&&gitinitsub&&test_when_finished"rm -rf sub"&&-echo"foobar">sub/file&&+echo"(1|2)d(3|4)">sub/file&&git-Csubaddfile&&git-Csubcommit-m"add file"&&
@@ -203,82 +203,82 @@ test_expect_success 'grep history with moved submoules' 'git-Cparentcommit-m"add submodule"&&cat>expect<<-\EOF&&-dir/sub/file:foobar-file:foobar+dir/sub/file:(1|2)d(3|4)+file:(1|2)d(3|4)EOF-git-Cparentgrep-e"foobar"--recurse-submodules>actual&&+git-Cparentgrep-e"(1|2)d(3|4)"--recurse-submodules>actual&&test_cmpexpectactual&&git-Cparentmvdir/subsub-moved&&git-Cparentcommit-m"moved submodule"&&cat>expect<<-\EOF&&-file:foobar-sub-moved/file:foobar+file:(1|2)d(3|4)+sub-moved/file:(1|2)d(3|4)EOF-git-Cparentgrep-e"foobar"--recurse-submodules>actual&&+git-Cparentgrep-e"(1|2)d(3|4)"--recurse-submodules>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-HEAD^:dir/sub/file:foobar-HEAD^:file:foobar+HEAD^:dir/sub/file:(1|2)d(3|4)+HEAD^:file:(1|2)d(3|4)EOF-git-Cparentgrep-e"foobar"--recurse-submodulesHEAD^>actual&&+git-Cparentgrep-e"(1|2)d(3|4)"--recurse-submodulesHEAD^>actual&&test_cmpexpectactual' test_expect_success'grep using relative path''test_when_finished"rm -rf parent sub"&&gitinitsub&&-echo"foobar">sub/file&&+echo"(1|2)d(3|4)">sub/file&&git-Csubaddfile&&git-Csubcommit-m"add file"&&gitinitparent&&-echo"foobar">parent/file&&+echo"(1|2)d(3|4)">parent/file&&git-Cparentaddfile&&mkdirparent/src&&-echo"foobar">parent/src/file2&&+echo"(1|2)d(3|4)">parent/src/file2&&git-Cparentaddsrc/file2&&git-Cparentsubmoduleadd../sub&&git-Cparentcommit-m"add files and submodule"&&# From top workscat>expect<<-\EOF&&-file:foobar-src/file2:foobar-sub/file:foobar+file:(1|2)d(3|4)+src/file2:(1|2)d(3|4)+sub/file:(1|2)d(3|4)EOF-git-Cparentgrep--recurse-submodules-e"foobar">actual&&+git-Cparentgrep--recurse-submodules-e"(1|2)d(3|4)">actual&&test_cmpexpectactual&&# Relative path to topcat>expect<<-\EOF&&-../file:foobar-file2:foobar-../sub/file:foobar+../file:(1|2)d(3|4)+file2:(1|2)d(3|4)+../sub/file:(1|2)d(3|4)EOF-git-Cparent/srcgrep--recurse-submodules-e"foobar"--..>actual&&+git-Cparent/srcgrep--recurse-submodules-e"(1|2)d(3|4)"--..>actual&&test_cmpexpectactual&&# Relative path to submodulecat>expect<<-\EOF&&-../sub/file:foobar+../sub/file:(1|2)d(3|4)EOF-git-Cparent/srcgrep--recurse-submodules-e"foobar"--../sub>actual&&+git-Cparent/srcgrep--recurse-submodules-e"(1|2)d(3|4)"--../sub>actual&&test_cmpexpectactual' test_expect_success'grep from a subdir''test_when_finished"rm -rf parent sub"&&gitinitsub&&-echo"foobar">sub/file&&+echo"(1|2)d(3|4)">sub/file&&git-Csubaddfile&&git-Csubcommit-m"add file"&&gitinitparent&&mkdirparent/src&&-echo"foobar">parent/src/file&&+echo"(1|2)d(3|4)">parent/src/file&&git-Cparentaddsrc/file&&git-Cparentsubmoduleadd../subsrc/sub&&git-Cparentsubmoduleadd../subsub&&
@@ -286,19 +286,19 @@ test_expect_success 'grep from a subdir' '# Verify grep from root workscat>expect<<-\EOF&&-src/file:foobar-src/sub/file:foobar-sub/file:foobar+src/file:(1|2)d(3|4)+src/sub/file:(1|2)d(3|4)+sub/file:(1|2)d(3|4)EOF-git-Cparentgrep--recurse-submodules-e"foobar">actual&&+git-Cparentgrep--recurse-submodules-e"(1|2)d(3|4)">actual&&test_cmpexpectactual&&# Verify grep from a subdir workscat>expect<<-\EOF&&-file:foobar-sub/file:foobar+file:(1|2)d(3|4)+sub/file:(1|2)d(3|4)EOF-git-Cparent/srcgrep--recurse-submodules-e"foobar">actual&&+git-Cparent/srcgrep--recurse-submodules-e"(1|2)d(3|4)">actual&&test_cmpexpectactual'
Add tests for --threads=N being supplied on the command-line, or when
grep.threads=N being supplied in the configuration.
When the threading support was made run-time configurable in commit
89f09dd34e ("grep: add --threads=<num> option and grep.threads
configuration", 2015-12-15) no tests were added for it.
In developing a change to the grep code I was able to make
'--threads=1 <pat>` segfault, while the test suite still passed. This
change fixes that blind spot in the tests.
In addition to asserting that asking for N threads shouldn't segfault,
test that the grep output given any N is the same.
The choice to test only 1..10 as opposed to 1..8 or 1..16 or whatever
is arbitrary. Testing 1..1024 works locally for me (but gets
noticeably slower as more threads are spawned). Given the structure of
the code there's no reason to test an arbitrary number of threads,
only 0, 1 and >=2 are special modes of operation.
A later patch introduces a PTHREADS test prerequisite which is true
under NO_PTHREADS=UnfortunatelyYes, but even under NO_PTHREADS it's
fine to test --threads=N, we'll just ignore it and not use
threading. So these tests also make sense under that mode to assert
that --threads=N without pthreads still returns expected results.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7810-grep.sh | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
@@ -775,6 +775,22 @@ test_expect_success 'grep -W with userdiff' 'test_cmpexpectedactual'+forthreadsin$(test_seq010)+do+test_expect_success"grep --threads=$threads & -c grep.threads=$threads""+gitgrep--threads=$threads.>actual.$threads&&+iftest$threads-ge1+then+test_cmpactual.\$(($threads-1))actual.$threads+fi&&+git-cgrep.threads=$threadsgrep.>actual.$threads&&+iftest$threads-ge1+then+test_cmpactual.\$(($threads-1))actual.$threads+fi+"+done+ test_expect_success'grep from a subdirectory to search wider area (1)''mkdir-ps&&(
Add setup code needed for testing regexes that contain both binary
data and regex metacharacters.
The POSIX regcomp() function inherently can't support that, because it
takes a \0-delimited char *, but other regex engines APIs like PCRE v2
take a pattern/length pair, and are thus able to handle \0s in
patterns as well as any other character.
When kwset was imported in commit 9eceddeec6 ("Use kwset in grep",
2011-08-21) this limitation was fixed, but at the expense of
introducing the undocumented limitation that any pattern containing \0
implicitly becomes a fixed match (equivalent to -F having been
provided).
That's not something we'd like to keep in the future. The inability to
match patterns containing \0 is a leaky implementation detail.
So add tests as a first step towards changing that. In order to test
that \0-patterns can properly match as regexes the test string needs
to have some regex metacharacters in it.
There were other blind spots in the tests. The code around kwset
specially handles case-insensitive & non-ASCII data, but there were no
tests for this.
Fix all of that by amending the text being matched to contain both
regex metacharacters & non-ASCII data.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7008-grep-binary.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Add testing for grep pattern types being correctly passed to
submodules. The pattern "(.|.)[\d]" matches differently under
fixed (not at all), and then matches different lines under
basic/extended & perl regular expressions, so this change asserts that
the pattern type is passed along correctly.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7814-grep-recurse-submodules.sh | 49 ++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
Address a big blind spot in the tests for patterns containing \0. The
is_fixed() function considers any string that contains \0 fixed, even
if it contains regular expression metacharacters, those patterns are
currently matched with kwset.
Before this change removing that memchr(s, 0, len) check from
is_fixed() wouldn't change the result of any of the tests, since
regcomp() will happily match the part before the \0.
The kwset path is dependent on whether the the -i flag is on, and
whether the pattern has any non-ASCII characters, but none of this was
tested for.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7008-grep-binary.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
@@ -22,6 +22,18 @@ nul_match () {printf'$pattern'|q_to_nul>f&&test_must_failgitgrep-ff$flagsa"+eliftest"$matches"=T1+then+test_expect_failure"git grep -f f $flags '$pattern_human' a""+printf'$pattern'|q_to_nul>f&&+gitgrep-ff$flagsa+"+eliftest"$matches"=T0+then+test_expect_failure"git grep -f f $flags '$pattern_human' a""+printf'$pattern'|q_to_nul>f&&+test_must_failgitgrep-ff$flagsa+"elsetest_expect_success"PANIC: Test framework error. Unknown matches value $matches"'false'fi
@@ -98,6 +110,65 @@ nul_match 1 '-Fi' 'YQf' nul_match0'-Fi''YQx' nul_match1'''yQf' nul_match0'''yQx'+nul_match1'''æQð'+nul_match1'-F''eQm[*]c'+nul_match1'-Fi''EQM[*]C'++# Regex patterns that would match but shouldn't with -F+nul_match0'-F''yQ[f]'+nul_match0'-F''[y]Qf'+nul_match0'-Fi''YQ[F]'+nul_match0'-Fi''[Y]QF'+nul_match0'-F''æQ[ð]'+nul_match0'-F''[æ]Qð'+nul_match0'-Fi''ÆQ[Ð]'+nul_match0'-Fi''[Æ]QÐ'++# kwset is disabled on -i & non-ASCII. No way to match non-ASCII \0+# patterns case-insensitively.+nul_matchT1'-i''ÆQÐ'++# \0 implicitly disables regexes. This is an undocumented internal+# limitation.+nul_matchT1'''yQ[f]'+nul_matchT1'''[y]Qf'+nul_matchT1'-i''YQ[F]'+nul_matchT1'-i''[Y]Qf'+nul_matchT1'''æQ[ð]'+nul_matchT1'''[æ]Qð'+nul_matchT1'-i''ÆQ[Ð]'++# ... because of \0 implicitly disabling regexes regexes that+# should/shouldn't match don't do the right thing.+nul_matchT1'''eQm.*cQ'+nul_matchT1'-i''EQM.*cQ'+nul_matchT0'''eQm[*]c'+nul_matchT0'-i''EQM[*]C'++# Due to the REG_STARTEND extension when kwset() is disabled on -i &+# non-ASCII the string will be matched in its entirety, but the+# pattern will be cut off at the first \0.+nul_match0'-i''NOMATCHQð'+nul_matchT0'-i''[Æ]QNOMATCH'+nul_matchT0'-i''[æ]QNOMATCH'+# Matches, but for the wrong reasons, just stops at [æ]+nul_match1'-i''[Æ]Qð'+nul_match1'-i''[æ]Qð'++# Ensure that the matcher doesn't regress to something that stops at+# \0+nul_match0'-F''yQ[f]'+nul_match0'-Fi''YQ[F]'+nul_match0'''yQNOMATCH'+nul_match0'''QNOMATCH'+nul_match0'-i''YQNOMATCH'+nul_match0'-i''QNOMATCH'+nul_match0'-F''æQ[ð]'+nul_match0'-Fi''ÆQ[Ð]'+nul_match0'''yQNÓMATCH'+nul_match0'''QNÓMATCH'+nul_match0'-i''YQNÓMATCH'+nul_match0'-i''QNÓMATCH' test_expect_success'grep respects binary diff attribute''echotext>t&&
Add a git GIT_PERF_MAKE_COMMAND variable to compliment the existing
GIT_PERF_MAKE_OPTS facility. This allows specifying an arbitrary shell
command to execute instead of 'make'.
This is useful e.g. in cases where the name, semantics or defaults of
a Makefile flag have changed over time. It can even be used to change
the contents of the tree, useful for monkeypatching ancient versions
of git to get them to build.
This opens Pandora's box in some ways, it's now possible to
"jailbreak" the perf environment and e.g. modify the source tree via
this arbitrary instead of just issuing a custom "make" command, such a
command has to be re-entrant in the sense that subsequent perf runs
will re-use the possibly modified tree.
It would be pointless to try to mitigate or work around that caveat in
a tool purely aimed at Git developers, so this change makes no attempt
to do so.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 3 +++
t/perf/README | 19 +++++++++++++++++--
t/perf/run | 11 +++++++++--
3 files changed, 29 insertions(+), 4 deletions(-)
@@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak): GIT_PERF_MAKE_OPTS Options to use when automatically building a git tree for- performance testing. E.g., -j6 would be useful.-+ performance testing. E.g., -j6 would be useful. Passed+ directly to make as "make $GIT_PERF_MAKE_OPTS".++ GIT_PERF_MAKE_COMMAND+ An arbitrary command that'll be run in place of the make+ command, if set the GIT_PERF_MAKE_OPTS variable is+ ignored. Useful in cases where source tree changes might+ require issuing a different make command to different+ revisions.++ This can be (ab)used to monkeypatch or otherwise change the+ tree about to be built. Note that the build directory can be+ re-used for subsequent runs so the make command might get+ executed multiple times on the same tree, but don't count on+ any of that, that's an implementation detail that might change+ in the future.+ GIT_PERF_REPO GIT_PERF_LARGE_REPO Repositories to copy for the performance tests. The normal
@@ -37,8 +37,15 @@ build_git_rev () { cp "../../$config" "build/$rev/" fi done- (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||- die "failed to build revision '$mydir'"+ (+ cd build/$rev &&+ if test -n "$GIT_PERF_MAKE_COMMAND"+ then+ sh -c "$GIT_PERF_MAKE_COMMAND"+ else+ make $GIT_PERF_MAKE_OPTS+ fi+ ) || die "failed to build revision '$mydir'" } run_dirs_helper () {
Add a performance comparison test which compares both case-sensitive &
case-insensitive fixed-string grep, as well as non-ASCII
case-sensitive & case-insensitive grep.
$ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux ./run p7821-grep-engines-fixed.sh
[...]
Test this tree
------------------------------------------------
7821.1: fixed grep int 0.61(1.72+0.65)
7821.2: basic grep int 0.69(1.72+0.53)
7821.3: extended grep int 0.60(1.72+0.54)
7821.4: perl grep int 0.65(1.65+0.64)
7821.6: fixed grep uncommon 0.25(0.53+0.48)
7821.7: basic grep uncommon 0.26(0.57+0.46)
7821.8: extended grep uncommon 0.25(0.52+0.51)
7821.9: perl grep uncommon 0.26(0.56+0.48)
7821.11: fixed grep æ 0.40(1.26+0.44)
7821.12: basic grep æ 0.40(1.28+0.43)
7821.13: extended grep æ 0.39(1.28+0.44)
7821.14: perl grep æ 0.39(1.29+0.44)
This test needs to be run with GIT_PERF_7821_GREP_OPTS=' -i' to avoid
going through the same kwset.[ch] codepath, see the "Even when -F..."
comment in grep.c:
$ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_7821_GREP_OPTS=' -i' ./run p7821-grep-engines-fixed.sh
[...]
Test this tree
---------------------------------------------------
7821.1: fixed grep -i int 1.55(1.86+0.66)
7821.2: basic grep -i int 0.66(1.97+0.54)
7821.3: extended grep -i int 0.72(1.88+0.62)
7821.4: perl grep -i int 0.75(1.93+0.57)
7821.6: fixed grep -i uncommon 0.27(0.52+0.54)
7821.7: basic grep -i uncommon 0.25(0.58+0.44)
7821.8: extended grep -i uncommon 0.26(0.62+0.43)
7821.9: perl grep -i uncommon 0.26(0.55+0.53)
7821.11: fixed grep -i æ 0.32(0.87+0.46)
7821.12: basic grep -i æ 0.30(0.90+0.41)
7821.13: extended grep -i æ 0.32(0.92+0.41)
7821.14: perl grep -i æ 0.29(0.71+0.53)
I'm planning to make that not be the case, this performance test gives
a baseline for comparing performance before & after any such change.
See commit ("perf: add a comparison test of grep regex engines",
2017-04-19) for details on the machine the above test run was executed
on.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/perf/p7821-grep-engines-fixed.sh | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100755 t/perf/p7821-grep-engines-fixed.sh
@@ -0,0 +1,32 @@+#!/bin/sh++test_description="Comparison of git-grep's regex engines with -F++SetGIT_PERF_7821_GREP_OPTSintheenvironmenttopassoptionsto+git-grep.Makesuretoincludealeadingspace,+e.g.GIT_PERF_7821_GREP_OPTS=' -w'.Seep7820-grep-engines.shformore+optionstotry.+"++../perf-lib.sh++test_perf_large_repo+test_checkout_worktree++forargsin'int''uncommon''æ'+do+forengineinfixedbasicextendedperl+do+test_perf"$engine grep$GIT_PERF_7821_GREP_OPTS$args""+git-cgrep.patternType=$enginegrep$GIT_PERF_7821_GREP_OPTS$args>'out.$engine.$args'||:+"+done++test_expect_success"assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS$args""+test_cmp'out.fixed.$args''out.basic.$args'&&+test_cmp'out.fixed.$args''out.extended.$args'&&+test_cmp'out.fixed.$args''out.perl.$args'+"+done++test_done
Add a die(...) to a default case for the switch statement selecting
between grep pattern types under --recurse-submodules.
Normally this would be caught by -Wswitch, but the grep_pattern_type
type is converted to int by going through parse_options(). Changing
the argument type passed to compile_submodule_options() won't work,
the value will just get coerced. The -Wswitch-default warning will
warn about it, but that produces a lot of noise across the codebase,
this potential issue would be drowned in that noise.
Thus catching this at runtime is the least bad option. This won't ever
trigger in practice, but if a new pattern type were to be added this
catches an otherwise silent bug during development.
See commit 0281e487fd ("grep: optionally recurse into submodules",
2016-12-16) for the initial addition of this code.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 2 ++
1 file changed, 2 insertions(+)
@@ -495,6 +495,8 @@ static void compile_submodule_options(const struct grep_opt *opt,break;caseGREP_PATTERN_TYPE_UNSPECIFIED:break;+default:+die("BUG: Added a new grep pattern type without updating switch statement");}for(pattern=opt->pattern_list;pattern!=NULL;
Add a very basic performance comparison test comparing the POSIX
basic, extended and perl engines.
In theory the "basic" and "extended" engines should be implemented
using the same underlying code with a slightly different pattern
parser, but some implementations may not do this. Jump through some
slight hoops to test both, which is worthwhile since "basic" is the
default.
Running this on an i7 3.4GHz Linux 4.9.0-2 Debian testing against a
checkout of linux.git & latest upstream PCRE, both PCRE and git
compiled with -O3 using gcc 7.1.1:
$ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux ./run p7820-grep-engines.sh
[...]
Test this tree
---------------------------------------------------------------
7820.1: basic grep 'how.to' 0.34(1.24+0.53)
7820.2: extended grep 'how.to' 0.33(1.23+0.45)
7820.3: perl grep 'how.to' 0.31(1.05+0.56)
7820.5: basic grep '^how to' 0.32(1.24+0.42)
7820.6: extended grep '^how to' 0.33(1.20+0.44)
7820.7: perl grep '^how to' 0.57(2.67+0.42)
7820.9: basic grep '[how] to' 0.51(2.16+0.45)
7820.10: extended grep '[how] to' 0.49(2.20+0.43)
7820.11: perl grep '[how] to' 0.56(2.60+0.43)
7820.13: basic grep '\(e.t[^ ]*\|v.ry\) rare' 0.66(3.25+0.40)
7820.14: extended grep '(e.t[^ ]*|v.ry) rare' 0.65(3.19+0.46)
7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 1.05(5.74+0.34)
7820.17: basic grep 'm\(ú\|u\)lt.b\(æ\|y\)te' 0.34(1.28+0.47)
7820.18: extended grep 'm(ú|u)lt.b(æ|y)te' 0.34(1.38+0.38)
7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.39(1.56+0.44)
Options can also be passed to git-grep via the GIT_PERF_7820_GREP_OPTS
environment variable. There are various modes such as "-v" that have
very different performance profiles, but handling the combinatorial
explosion of testing all those options would make this script much
more complex and harder to maintain. Instead just add the ability to
do one-shot runs with arbitrary options, e.g.:
$ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_7820_GREP_OPTS=" -i" ./run p7820-grep-engines.sh
[...]
Test this tree
------------------------------------------------------------------
7820.1: basic grep -i 'how.to' 0.49(1.72+0.38)
7820.2: extended grep -i 'how.to' 0.46(1.64+0.42)
7820.3: perl grep -i 'how.to' 0.44(1.45+0.45)
7820.5: basic grep -i '^how to' 0.47(1.76+0.38)
7820.6: extended grep -i '^how to' 0.47(1.70+0.42)
7820.7: perl grep -i '^how to' 0.65(2.72+0.37)
7820.9: basic grep -i '[how] to' 0.86(3.64+0.42)
7820.10: extended grep -i '[how] to' 0.84(3.62+0.46)
7820.11: perl grep -i '[how] to' 0.73(3.06+0.39)
7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare' 1.63(8.13+0.36)
7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 1.64(8.01+0.44)
7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.44(6.88+0.44)
7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te' 0.66(2.67+0.44)
7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 0.66(2.67+0.43)
7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.59(2.31+0.37)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/perf/p7820-grep-engines.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100755 t/perf/p7820-grep-engines.sh
@@ -0,0 +1,47 @@+#!/bin/sh++test_description="Comparison of git-grep's regex engines++SetGIT_PERF_7820_GREP_OPTSintheenvironmenttopassoptionsto+git-grep.Makesuretoincludealeadingspace,+e.g.GIT_PERF_7820_GREP_OPTS=' -i'.Someoptionstotry:++-i+-w+-v+-vi+-vw+-viw+"++../perf-lib.sh++test_perf_large_repo+test_checkout_worktree++forpatternin\+'how.to'\+'^how to'\+'[how] to'\+'\(e.t[^ ]*\|v.ry\) rare'\+'m\(ú\|u\)lt.b\(æ\|y\)te'+do+forengineinbasicextendedperl+do+iftest$engine!="basic"+then+# Poor man's basic -> extended converter.+pattern=$(echo"$pattern"|sed's/\\//g')+fi+test_perf"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'""+git-cgrep.patternType=$enginegrep$GIT_PERF_7820_GREP_OPTS--'$pattern'>'out.$engine'||:+"+done++test_expect_success"assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'""+test_cmp'out.basic''out.extended'&&+test_cmp'out.basic''out.perl'+"+done++test_done
@@ -0,0 +1,44 @@+#!/bin/sh++test_description="Comparison of git-log's --grep regex engines++SetGIT_PERF_4220_LOG_OPTSintheenvironmenttopassoptionsto+git-grep.Makesuretoincludealeadingspace,+e.g.GIT_PERF_4220_LOG_OPTS=' -i'.Someoptionstotry:++-i+--invert-grep+-i--invert-grep+"++../perf-lib.sh++test_perf_large_repo+test_checkout_worktree++forpatternin\+'how.to'\+'^how to'\+'[how] to'\+'\(e.t[^ ]*\|v.ry\) rare'\+'m\(ú\|u\)lt.b\(æ\|y\)te'+do+forengineinbasicextendedperl+do+iftest$engine!="basic"+then+# Poor man's basic -> extended converter.+pattern=$(echo$pattern|sed's/\\//g')+fi+test_perf"$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'""+git-cgrep.patternType=$enginelog--pretty=format:%h$GIT_PERF_4220_LOG_OPTS--grep='$pattern'>'out.$engine'||:+"+done++test_expect_success"assert that all engines found the same for$GIT_PERF_4220_LOG_OPTS '$pattern'""+test_cmp'out.basic''out.extended'&&+test_cmp'out.basic''out.perl'+"+done++test_done
Amend the t/perf/run output so that in addition to the "Running N
tests" heading currently being emitted, it also emits "Unpacking $rev"
and "Building $rev" when setting up the build/$rev directory & when
building it, respectively.
This makes it easier to see what's going on and what revision is being
tested as the output scrolls by.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/perf/run | 2 ++
1 file changed, 2 insertions(+)
@@ -37,6 +38,7 @@ build_git_rev () { cp "../../$config" "build/$rev/" fi done+ echo "=== Building $rev ===" ( cd build/$rev && if test -n "$GIT_PERF_MAKE_COMMAND"
Remove redundant assignments to the "regflags" variable. This variable
is only used set under GREP_PATTERN_TYPE_ERE, so there's no need to
un-set it under GREP_PATTERN_TYPE_{FIXED,BRE,PCRE}.
Back in 5010cb5fcc[1], we did do "opt.regflags &= ~REG_EXTENDED" upon
seeing "-G" on the command line and flipped the bit on upon seeing
"-E", but I think that was perfectly sensible and it would have been a
bug if we didn't. They were part of the command line parsing that
could have seen "-E" on the command line earlier.
When cca2c172 ("git-grep: do not die upon -F/-P when
grep.extendedRegexp is set.", 2011-05-09) switched the command line
parsing to "read into a 'tentatively this is what we saw the last'
variable and then finally commit just once", we didn't touch
opt.regflags for PCRE and FIXED, but we still had to flip regflags
between BRE and ERE, because parsing of grep.extendedregexp
configuration variable directly touched opt.regflags back then, which
was done by b22520a3 ("grep: allow -E and -n to be turned on by
default via configuration", 2011-03-30).
When 84befcd0 ("grep: add a grep.patternType configuration setting",
2012-08-03) introduced extended_regexp_option field, we stopped
flipping regflags while reading the configuration, and that was when
we should have noticed and stopped dropping REG_EXTENDED bit in the
"now we can commit what type to use" helper function.
There is no reason to do this anymore, so stop doing it, more to
reduce "wait this is used under fixed/BRE/PCRE how?" confusion when
reading the code, than to to save ourselves trivial CPU cycles by
removing one assignment.
1. "built-in "git grep"", 2006-04-30.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
Factor the test for \0 in grep patterns into a function. Since commit
9eceddeec6 ("Use kwset in grep", 2011-08-21) any pattern containing a
\0 is considered fixed as regcomp() can't handle it.
This change makes later changes that make use of either has_null() or
is_fixed() (but not both) smaller.
While I'm at it make the comment conform to the style guide, i.e. add
an opening "/*\n".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
Move the is_fixed() function which are currently only used in
compile_regexp() earlier so it can be used in the PCRE family of
functions in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Change the internal USE_LIBPCRE define, & build options flag to use a
naming convention ending in PCRE1, without changing the long-standing
USE_LIBPCRE Makefile flag which enables this code.
This is for preparation for libpcre2 support where having things like
USE_LIBPCRE and USE_LIBPCRE2 in any more places than we absolutely
need to for backwards compatibility with old Makefile arguments would
be confusing.
In some ways it would be better to change everything that now uses
USE_LIBPCRE to use USE_LIBPCRE1, and to make specifying
USE_LIBPCRE (or --with-pcre) an error. This would impose a one-time
burden on packagers of git to s/USE_LIBPCRE/USE_LIBPCRE1/ in their
build scripts.
However I'd like to leave the door open to making
USE_LIBPCRE=YesPlease eventually mean USE_LIBPCRE2=YesPlease,
i.e. once PCRE v2 is ubiquitous enough that it makes sense to make it
the default.
This code and the USE_LIBPCRE Makefile argument was added in commit
63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09). At the time there was
no indication that the PCRE project would release an entirely new &
incompatible API around 3 years later.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 4 ++--
grep.c | 6 +++---
grep.h | 2 +-
t/test-lib.sh | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
@@ -1014,7 +1014,7 @@ esac(COLUMNS=1&&test$COLUMNS=1)&&test_set_prereqCOLUMNS_CAN_BE_1test-z"$NO_PERL"&&test_set_prereqPERLtest-z"$NO_PYTHON"&&test_set_prereqPYTHON-test-n"$USE_LIBPCRE"&&test_set_prereqPCRE+test-n"$USE_LIBPCRE1"&&test_set_prereqPCREtest-z"$NO_GETTEXT"&&test_set_prereqGETTEXT# Can we rely on git's output in the C locale?
Change the internal PCRE variable & function names to have a "1"
suffix. This is for preparation for libpcre2 support, where having
non-versioned names would be confusing.
An earlier change in this series ("grep: change the internal PCRE
macro names to be PCRE1", 2017-04-07) elaborates on the motivations
behind this change.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 52 ++++++++++++++++++++++++++--------------------------
grep.h | 8 ++++----
2 files changed, 30 insertions(+), 30 deletions(-)
Add a PTHREADS prerequisite which is false when git is compiled with
NO_PTHREADS=YesPlease.
There's lots of custom code that runs when threading isn't available,
but before this prerequisite there was no way to test it.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
t/README | 4 ++++
t/test-lib.sh | 1 +
3 files changed, 6 insertions(+)
@@ -817,6 +817,10 @@ use these, and "test_set_prereq" for how to define your own. Test is run on a filesystem which converts decomposed utf-8 (nfd) to precomposed utf-8 (nfc).+ - PTHREADS++ Git wasn't compiled with NO_PTHREADS=YesPlease.+ Tips for Writing Tests ----------------------
Fix a buggy warning about threads under NO_PTHREADS=YesPlease. Due to
re-using the delta_search_threads variable for both the state of the
"pack.threads" config & the --threads option, setting "pack.threads"
but not supplying --threads would trigger the warning for both
"pack.threads" & --threads.
Solve this bug by resetting the delta_search_threads variable in
git_pack_config(), it might then be set by --threads again and be
subsequently warned about, as the test I'm changing here asserts.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/pack-objects.c | 4 +++-
t/t5300-pack-object.sh | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
Change the grep_{lock,unlock} functions to assert that num_threads is
true, instead of only locking & unlocking the pthread mutex lock when
it is.
These functions are never called when num_threads isn't true, this
logic has gone through multiple iterations since the initial
introduction of grep threading in commit 5b594f457a ("Threaded grep",
2010-01-25), but ever since then they'd only be called if num_threads
was true, so this check made the code confusing to read.
Replace the check with an assertion, so that it's clear to the reader
that this code path is never taken unless we're spawning threads.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -73,14 +73,14 @@ static pthread_mutex_t grep_mutex;staticinlinevoidgrep_lock(void){-if(num_threads)-pthread_mutex_lock(&grep_mutex);+assert(num_threads);+pthread_mutex_lock(&grep_mutex);}staticinlinevoidgrep_unlock(void){-if(num_threads)-pthread_mutex_unlock(&grep_mutex);+assert(num_threads);+pthread_mutex_unlock(&grep_mutex);}/* Signalled when a new work_item is added to todo. */
Add a warning about missing thread support when grep.threads or
--threads is set to a non 0 (default) or 1 (no parallelism) value
under NO_PTHREADS=YesPlease.
This is for consistency with the index-pack & pack-objects commands,
which also take a --threads option & are configurable via
pack.threads, and have long warned about the same under
NO_PTHREADS=YesPlease.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 13 +++++++++++++
t/t7810-grep.sh | 18 ++++++++++++++++++
2 files changed, 31 insertions(+)
Add a test for the warning that's emitted when --threads or
pack.threads is provided under NO_PTHREADS=YesPlease. This uses the
new PTHREADS prerequisite.
The assertion for C_LOCALE_OUTPUT in the latter test is currently
redundant, since unlike index-pack the pack-objects warnings aren't
i18n'd. However they might be changed to be i18n'd in the future, and
there's no harm in future-proofing the test.
There's an existing bug in the implementation of pack-objects which
this test currently tests for as-is. Details about the bug & the fix
are included in a follow-up change.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t5300-pack-object.sh | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
From: Junio C Hamano <hidden> Date: 2017-05-20 23:50:34
Ævar Arnfjörð Bjarmason [off-list ref] writes:
Easy to review? 29 (I mean 30) patches? Are you kidding me?!
As noted in v1 ([off-list ref];
https://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)
these are all doc, test, refactoring etc. changes needed by the
subsequent "PCRE v2, PCRE v1 JIT, log -P & fixes" series.
Since Junio hasn't been picking it I'm no longer sending updates to
that patch series & waiting for this one to cook first.
I actually do not mind a reroll that goes together with this. The
only reason why I skipped the earlier one was because I looked at
the original one, and the discussion on the reroll of this 'easy to
review' part indicated that it will be rerolled, before I got to
look at these upper layer patches.
Overall nicely done. I only had just a few observations.
Thanks.
From: Junio C Hamano <hidden> Date: 2017-05-20 23:50:39
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
Make the --regexp-ignore-case option work with --perl-regexp. This
never worked, and there was no test for this. Fix the bug and add a
test.
When PCRE support was added in commit 63e7e9d8b6 ("git-grep: Learn
PCRE", 2011-05-09) compile_pcre_regexp() would only check
opt->ignore_case, but when the --perl-regexp option was added in
commit 727b6fc3ed ("log --grep: accept --basic-regexp and
--perl-regexp", 2012-10-03) the code didn't set the opt->ignore_case.
Change the test suite to test for -i and --invert-regexp with
basic/extended/perl patterns in addition to fixed, which was the only
patternType that was tested for before in combination with those
options.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
revision.c | 1 +
t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 56 insertions(+), 5 deletions(-)
@@ -1991,6 +1991,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg}elseif(!strcmp(arg,"--extended-regexp")||!strcmp(arg,"-E")){revs->grep_filter.pattern_type_option=GREP_PATTERN_TYPE_ERE;}elseif(!strcmp(arg,"--regexp-ignore-case")||!strcmp(arg,"-i")){+revs->grep_filter.ignore_case=1;revs->grep_filter.regflags|=REG_ICASE;DIFF_OPT_SET(&revs->diffopt,PICKAXE_IGNORE_CASE);}elseif(!strcmp(arg,"--fixed-strings")||!strcmp(arg,"-F")){
Looks good.
I however wonder if it is a better approach in the longer term to
treat the .ignore_case field just like .extended_regexp_option
field, i.e. not committing immediately to .regflags but commit it
after config and command line parsing is done, just like we make the
"BRE? ERE?" decision in grep_commit_pattern_type().
Thanks.
@@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak): GIT_PERF_MAKE_OPTS Options to use when automatically building a git tree for- performance testing. E.g., -j6 would be useful.-+...+ any of that, that's an implementation detail that might change+ in the future.+
I'll remove the trailing whitespace on this otherwise blank line
while queuing (no need to resend only to fix this one).
Thanks.
quoted hunk
GIT_PERF_REPO
GIT_PERF_LARGE_REPO
Repositories to copy for the performance tests. The normal
@@ -37,8 +37,15 @@ build_git_rev () { cp "../../$config" "build/$rev/" fi done- (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||- die "failed to build revision '$mydir'"+ (+ cd build/$rev &&+ if test -n "$GIT_PERF_MAKE_COMMAND"+ then+ sh -c "$GIT_PERF_MAKE_COMMAND"+ else+ make $GIT_PERF_MAKE_OPTS+ fi+ ) || die "failed to build revision '$mydir'" } run_dirs_helper () {
@@ -60,8 +60,23 @@ You can set the following variables (also in your config.mak): GIT_PERF_MAKE_OPTS Options to use when automatically building a git tree for- performance testing. E.g., -j6 would be useful.-+...+ any of that, that's an implementation detail that might change+ in the future.+
I'll remove the trailing whitespace on this otherwise blank line
while queuing (no need to resend only to fix this one).
Thanks.
Thanks, forgot about diff --check on the whole series with all the
other checks I was doing.
quoted
GIT_PERF_REPO
GIT_PERF_LARGE_REPO
Repositories to copy for the performance tests. The normal
@@ -37,8 +37,15 @@ build_git_rev () { cp "../../$config" "build/$rev/" fi done- (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||- die "failed to build revision '$mydir'"+ (+ cd build/$rev &&+ if test -n "$GIT_PERF_MAKE_COMMAND"+ then+ sh -c "$GIT_PERF_MAKE_COMMAND"+ else+ make $GIT_PERF_MAKE_OPTS+ fi+ ) || die "failed to build revision '$mydir'" } run_dirs_helper () {
On Sun, May 21, 2017 at 1:50 AM, Junio C Hamano [off-list ref] wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
Make the --regexp-ignore-case option work with --perl-regexp. This
never worked, and there was no test for this. Fix the bug and add a
test.
When PCRE support was added in commit 63e7e9d8b6 ("git-grep: Learn
PCRE", 2011-05-09) compile_pcre_regexp() would only check
opt->ignore_case, but when the --perl-regexp option was added in
commit 727b6fc3ed ("log --grep: accept --basic-regexp and
--perl-regexp", 2012-10-03) the code didn't set the opt->ignore_case.
Change the test suite to test for -i and --invert-regexp with
basic/extended/perl patterns in addition to fixed, which was the only
patternType that was tested for before in combination with those
options.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
revision.c | 1 +
t/t4202-log.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 56 insertions(+), 5 deletions(-)
@@ -1991,6 +1991,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg}elseif(!strcmp(arg,"--extended-regexp")||!strcmp(arg,"-E")){revs->grep_filter.pattern_type_option=GREP_PATTERN_TYPE_ERE;}elseif(!strcmp(arg,"--regexp-ignore-case")||!strcmp(arg,"-i")){+revs->grep_filter.ignore_case=1;revs->grep_filter.regflags|=REG_ICASE;DIFF_OPT_SET(&revs->diffopt,PICKAXE_IGNORE_CASE);}elseif(!strcmp(arg,"--fixed-strings")||!strcmp(arg,"-F")){
Looks good.
I however wonder if it is a better approach in the longer term to
treat the .ignore_case field just like .extended_regexp_option
field, i.e. not committing immediately to .regflags but commit it
after config and command line parsing is done, just like we make the
"BRE? ERE?" decision in grep_commit_pattern_type().
I started hacking up a patch to fix the root cause of this, i.e. the
users of the grep API should only set `.ignore_case = 1` and not care
about setting regflags, but it was more than a trivial change, so I
didn't include it in this series:
@@ -1992,7 +1992,6 @@ static int handle_revision_opt(struct rev_info
*revs, int argc, const char **arg
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;
} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
revs->grep_filter.ignore_case = 1;
- revs->grep_filter.regflags |= REG_ICASE;
DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;
But an even better solution is to get rid of passing the regflags
field in grep_opt entirely, this conflicts with some of my later
patches:
@@ -1992,7 +1991,6 @@ static int handle_revision_opt(struct rev_info
*revs, int argc, const char **arg
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;
} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
revs->grep_filter.ignore_case = 1;
- revs->grep_filter.regflags |= REG_ICASE;
DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;
But as all this code cleanup isn't needed for fixing this bug, and I'd
really like to get this series merged into next/master ASAP so I can
start submitting the grep/pcre patches that are actually interesting,
let's leave this orthogonal code cleanup for now.
On Sun, May 21, 2017 at 1:50 AM, Junio C Hamano [off-list ref] wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
Easy to review? 29 (I mean 30) patches? Are you kidding me?!
As noted in v1 ([off-list ref];
https://public-inbox.org/git/20170511091829.5634-1-avarab@gmail.com/)
these are all doc, test, refactoring etc. changes needed by the
subsequent "PCRE v2, PCRE v1 JIT, log -P & fixes" series.
Since Junio hasn't been picking it I'm no longer sending updates to
that patch series & waiting for this one to cook first.
I actually do not mind a reroll that goes together with this. The
only reason why I skipped the earlier one was because I looked at
the original one, and the discussion on the reroll of this 'easy to
review' part indicated that it will be rerolled, before I got to
look at these upper layer patches.
Great, now that the base of this is migrating to next, here's the
second part of this.
For v1 see [off-list ref]
(https://public-inbox.org/git/20170513234535.12749-1-avarab@gmail.com/).
The only changes to the content are better if/else branching around
conditional macros (but no functional changes) in the PCRE v1 JIT API
patch in response to a comment by Simon Ruderich.
The only other changes are trivial updates to the commit messages to
account for t/perf changes made in the series this builds on.
Ævar Arnfjörð Bjarmason (7):
grep: don't redundantly compile throwaway patterns under threading
grep: skip pthreads overhead when using one thread
log: add -P as a synonym for --perl-regexp
grep: add support for the PCRE v1 JIT API
grep: un-break building with PCRE < 8.32
grep: un-break building with PCRE < 8.20
grep: add support for PCRE v2
Documentation/rev-list-options.txt | 1 +
Makefile | 30 +++++--
builtin/grep.c | 16 +++-
configure.ac | 77 +++++++++++++---
grep.c | 177 ++++++++++++++++++++++++++++++++++++-
grep.h | 31 +++++++
revision.c | 2 +-
t/t4202-log.sh | 12 +++
t/test-lib.sh | 2 +-
9 files changed, 324 insertions(+), 24 deletions(-)
--
2.13.0.303.g4ebf302169
Add a short -P option as a synonym for the longer --perl-regexp, for
consistency with the options the corresponding grep invocations
accept.
This was intentionally omitted in commit 727b6fc3ed ("log --grep:
accept --basic-regexp and --perl-regexp", 2012-10-03) for unspecified
future use.
Make it consistent with "grep" rather than to keep it open for future
use, and to avoid the confusion of -P meaning different things for
grep & log, as is the case with the -G option.
As noted in the aforementioned commit the --basic-regexp option can't
have a corresponding -G argument, as the log command already uses that
for -G<regex>.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/rev-list-options.txt | 1 +
revision.c | 2 +-
t/t4202-log.sh | 12 ++++++++++++
3 files changed, 14 insertions(+), 1 deletion(-)
@@ -91,6 +91,7 @@ endif::git-rev-list[] Consider the limiting patterns to be fixed strings (don't interpret pattern as a regular expression).+-P:: --perl-regexp:: Consider the limiting patterns to be Perl-compatible regular expressions.
Amend my change earlier in this series ("grep: add support for the
PCRE v1 JIT API", 2017-04-11) to un-break the build on PCRE v1
versions earlier than 8.32.
The JIT support was added in version 8.20 released on 2011-10-21, but
it wasn't until 8.32 released on 2012-11-30 that the fast code path to
use the JIT via pcre_jit_exec() was added[1] (see also [2]).
This means that versions 8.20 through 8.31 could still use the JIT,
but supporting it on those versions would add to the already verbose
macro soup around JIT support it, and I don't expect that the use-case
of compiling a brand new git against a 5 year old PCRE is particularly
common, and if someone does that they can just get the existing
pre-JIT slow codepath.
So just take the easy way out and disable the JIT on any version older
than 8.32.
The reason this change isn't part of the initial change PCRE JIT
support is because possibly slightly annoying someone who's bisecting
with an ancient PCRE is worth it to have a cleaner history showing
which parts of the implementation are only used for ancient PCRE
versions. This also makes it easier to revert this change if we ever
decide to stop supporting those old versions.
1. http://www.pcre.org/original/changelog.txt ("28. Introducing a
native interface for JIT. Through this interface, the
compiled[...]")
2. https://bugs.exim.org/show_bug.cgi?id=2121
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 8 ++++----
grep.h | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
Amend my change earlier in this series ("grep: add support for the
PCRE v1 JIT API", 2017-04-11) to un-break the build on PCRE v1
versions earlier than 8.20.
The 8.20 release was the first release to have JIT & pcre_jit_stack in
the headers, so a mock type needs to be provided for it on those
releases.
Now git should compile with all PCRE versions that it supported before
my JIT change.
I've tested it as far back as version 7.5 released on 2008-01-10, once
I got down to version 7.0 it wouldn't build anymore with GCC 7.1.1,
and I couldn't be bothered to anything older than 7.5 as I'm confident
that if the build breaks on those older versions it's not because of
my JIT change.
See the "un-break" change in this series ("grep: un-break building
with PCRE < 8.32", 2017-05-10) for why this isn't squashed into the
main PCRE JIT commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.h | 3 +++
1 file changed, 3 insertions(+)
Skip the administrative overhead of using pthreads when only using one
thread. Instead take the non-threaded path which would be taken under
NO_PTHREADS.
The threading support was initially added in commit
5b594f457a ("Threaded grep", 2010-01-25) with a hardcoded compile-time
number of 8 threads. Later the number of threads was made configurable
in commit 89f09dd34e ("grep: add --threads=<num> option and
grep.threads configuration", 2015-12-15).
That change did not add any special handling for --threads=1. Now we
take a slightly faster path by skipping thread handling entirely when
1 thread is requested.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 2 ++
1 file changed, 2 insertions(+)
Change the grep PCRE v1 code to use JIT when available. When PCRE
support was initially added in commit 63e7e9d8b6 ("git-grep: Learn
PCRE", 2011-05-09) PCRE had no JIT support, it was integrated into
8.20 released on 2011-10-21.
Enabling JIT support usually improves performance by more than
40%. The pattern compilation times are relatively slower, but those
relative numbers are tiny, and are easily made back in all but the
most trivial cases of grep. Detailed benchmarks & overview of
compilation times is at: http://sljit.sourceforge.net/pcre.html
With this change the difference in a t/perf/p7820-grep-engines.sh run
is, with just the /perl/ tests shown:
$ 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
Test HEAD~ HEAD
---------------------------------------------------------------------------------------
7820.3: perl grep 'how.to' 0.35(1.11+0.43) 0.23(0.42+0.46) -34.3%
7820.7: perl grep '^how to' 0.64(2.71+0.36) 0.27(0.66+0.44) -57.8%
7820.11: perl grep '[how] to' 0.63(2.51+0.42) 0.33(0.98+0.39) -47.6%
7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 1.17(5.61+0.35) 0.34(1.08+0.46) -70.9%
7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.43(1.52+0.44) 0.30(0.88+0.42) -30.2%
The conditional support for JIT is implemented as suggested in the
pcrejit(3) man page. E.g. defining PCRE_STUDY_JIT_COMPILE to 0 if it's
not present.
The implementation is relatively verbose because even if
PCRE_CONFIG_JIT is defined only a call to pcre_config() can determine
if the JIT is available, and if so the faster pcre_jit_exec() function
should be called instead of pcre_exec(), and a different (but not
complimentary!) function needs to be called to free pcre1_extra_info.
There's no graceful fallback if pcre_jit_stack_alloc() fails under
PCRE_CONFIG_JIT, instead the program will simply abort. I don't think
this is worth handling gracefully, it'll only fail in cases where
malloc() doesn't work, in which case we're screwed anyway.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 34 +++++++++++++++++++++++++++++++++-
grep.h | 6 ++++++
2 files changed, 39 insertions(+), 1 deletion(-)
Add support for v2 of the PCRE API. This is a new major version of
PCRE that came out in early 2015[1].
The regular expression syntax is the same, but while the API is
similar, pretty much every function is either renamed or takes
different arguments. Thus using it via entirely new functions makes
sense, as opposed to trying to e.g. have one compile_pcre_pattern()
that would call either PCRE v1 or v2 functions.
Git can now be compiled with either USE_LIBPCRE1=YesPlease or
USE_LIBPCRE2=YesPlease, with USE_LIBPCRE=YesPlease currently being a
synonym for the former. Providing both is a compile-time error.
With earlier patches to enable JIT for PCRE v1 the performance of the
release versions of both libraries is almost exactly the same, with
PCRE v2 being around 1% slower.
However after I reported this to the pcre-dev mailing list[2] I got a
lot of help with the API use from Zoltán Herczeg, he subsequently
optimized some of the JIT functionality in v2 of the library.
Running the p7820-grep-engines.sh performance test against the latest
Subversion trunk of both, with both them and git compiled as -O3, and
the test run against linux.git, gives the following results. Just the
/perl/ tests shown:
$ 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
[...]
Test HEAD~2 HEAD~ HEAD
----------------------------------------------------------------------------------------------------------------
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%
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%
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%
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%
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%
See commit ("perf: add a comparison test of grep regex engines",
2017-04-19) for details on the machine the above test run was executed
on.
Here HEAD~2 is git with PCRE v1 without JIT, HEAD~ is PCRE v1 with
JIT, and HEAD is PCRE v2 (also with JIT). See previous commits of mine
mentioning p7820-grep-engines.sh for more details on the test setup.
For ease of readability, a different run just of HEAD~ (PCRE v1 with
JIT v.s. PCRE v2), again with just the /perl/ tests shown:
Test HEAD~ HEAD
---------------------------------------------------------------------------------------
7820.3: perl grep 'how.to' 0.23(0.41+0.47) 0.23(0.26+0.59) +0.0%
7820.7: perl grep '^how to' 0.27(0.64+0.47) 0.23(0.28+0.56) -14.8%
7820.11: perl grep '[how] to' 0.34(0.95+0.44) 0.25(0.38+0.56) -26.5%
7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.34(1.07+0.46) 0.24(0.52+0.49) -29.4%
7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.30(0.81+0.46) 0.22(0.33+0.54) -26.7%
I.e. the two are either neck-to-neck, but PCRE v2 usually pulls ahead,
when it does it's around 20% faster.
A brief note on thread safety: As noted in pcre2api(3) & pcre2jit(3)
the compiled pattern can be shared between threads, but not some of
the JIT context, however the grep threading support does all pattern &
JIT compilation in separate threads, so this code doesn't need to
concern itself with thread safety.
See commit 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) for the
initial addition of PCRE v1. This change follows some of the same
patterns it did (and which were discussed on list at the time),
e.g. mocking up types with typedef instead of ifdef-ing them out when
USE_LIBPCRE2 isn't defined. This adds some trivial memory use to the
program, but makes the code look nicer.
1. https://lists.exim.org/lurker/message/20150105.162835.0666407a.en.html
2. https://lists.exim.org/lurker/thread/20170419.172322.833ee099.en.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 30 +++++++++---
configure.ac | 77 ++++++++++++++++++++++++++-----
grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
grep.h | 17 +++++++
t/test-lib.sh | 2 +-
5 files changed, 250 insertions(+), 19 deletions(-)
@@ -29,7 +29,12 @@ all::# Perl-compatible regular expressions instead of standard or extended# POSIX regular expressions.#-# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by+# default in future releases.+#+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in# /foo/bar/include and /foo/bar/lib directories.## Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
@@ -255,21 +255,61 @@ GIT_PARSE_WITH([openssl])) # Perl-compatible regular expressions instead of standard or extended # POSIX regular expressions. #-# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by+# default in future releases.+#+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in # /foo/bar/include and /foo/bar/lib directories. # AC_ARG_WITH(libpcre,-AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes (default is NO)])+AS_HELP_STRING([--with-libpcre],[synonym for --with-libpcre1]),+ if test "$withval" = "no"; then+ USE_LIBPCRE1=+ elif test "$withval" = "yes"; then+ USE_LIBPCRE1=YesPlease+ else+ USE_LIBPCRE1=YesPlease+ LIBPCREDIR=$withval+ AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])+ dnl USE_LIBPCRE1 can still be modified below, so don't substitute+ dnl it yet.+ GIT_CONF_SUBST([LIBPCREDIR])+ fi)++AC_ARG_WITH(libpcre1,+AS_HELP_STRING([--with-libpcre1],[support Perl-compatible regexes via libpcre1 (default is NO)])+AS_HELP_STRING([], [ARG can be also prefix for libpcre library and headers]),+ if test "$withval" = "no"; then+ USE_LIBPCRE1=+ elif test "$withval" = "yes"; then+ USE_LIBPCRE1=YesPlease+ else+ USE_LIBPCRE1=YesPlease+ LIBPCREDIR=$withval+ AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])+ dnl USE_LIBPCRE1 can still be modified below, so don't substitute+ dnl it yet.+ GIT_CONF_SUBST([LIBPCREDIR])+ fi)++AC_ARG_WITH(libpcre2,+AS_HELP_STRING([--with-libpcre2],[support Perl-compatible regexes via libpcre2 (default is NO)]) AS_HELP_STRING([], [ARG can be also prefix for libpcre library and headers]),+ if test -n "$USE_LIBPCRE1"; then+ AC_MSG_ERROR([Only supply one of --with-libpcre1 or --with-libpcre2!])+ fi+ if test "$withval" = "no"; then- USE_LIBPCRE=+ USE_LIBPCRE2= elif test "$withval" = "yes"; then- USE_LIBPCRE=YesPlease+ USE_LIBPCRE2=YesPlease else- USE_LIBPCRE=YesPlease+ USE_LIBPCRE2=YesPlease LIBPCREDIR=$withval AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])- dnl USE_LIBPCRE can still be modified below, so don't substitute+ dnl USE_LIBPCRE2 can still be modified below, so don't substitute dnl it yet. GIT_CONF_SUBST([LIBPCREDIR]) fi)
@@ -501,13 +541,11 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO]) GIT_CONF_SUBST([NO_OPENSSL]) #-# Define USE_LIBPCRE if you have and want to use libpcre. Various-# commands such as log and grep offer runtime options to use-# Perl-compatible regular expressions instead of standard or extended-# POSIX regular expressions.+# Handle the USE_LIBPCRE1 and USE_LIBPCRE2 options potentially set+# above. #-if test -n "$USE_LIBPCRE"; then+if test -n "$USE_LIBPCRE1"; then GIT_STASH_FLAGS($LIBPCREDIR)
@@ -517,7 +555,22 @@ AC_CHECK_LIB([pcre], [pcre_version], GIT_UNSTASH_FLAGS($LIBPCREDIR)-GIT_CONF_SUBST([USE_LIBPCRE])+GIT_CONF_SUBST([USE_LIBPCRE1])++fi+++if test -n "$USE_LIBPCRE2"; then++GIT_STASH_FLAGS($LIBPCREDIR)++AC_CHECK_LIB([pcre2-8], [pcre2_config_8],+[USE_LIBPCRE2=YesPlease],+[USE_LIBPCRE2=])++GIT_UNSTASH_FLAGS($LIBPCREDIR)++GIT_CONF_SUBST([USE_LIBPCRE2]) fi
@@ -1015,7 +1015,7 @@ esactest-z"$NO_PERL"&&test_set_prereqPERLtest-z"$NO_PTHREADS"&&test_set_prereqPTHREADStest-z"$NO_PYTHON"&&test_set_prereqPYTHON-test-n"$USE_LIBPCRE1"&&test_set_prereqPCRE+test-n"$USE_LIBPCRE1$USE_LIBPCRE2"&&test_set_prereqPCREtest-z"$NO_GETTEXT"&&test_set_prereqGETTEXT# Can we rely on git's output in the C locale?
Change the pattern compilation logic under threading so that grep
doesn't compile a pattern it never ends up using on the non-threaded
code path, only to compile it again N times for N threads which will
each use their own copy, ignoring the initially compiled pattern.
This redundant compilation dates back to the initial introduction of
the threaded grep in commit 5b594f457a ("Threaded grep",
2010-01-25).
There was never any reason for doing this redundant work other than an
oversight in the initial commit. Jeff King suggested on-list in
[off-list ref] that this
might be needed to check the pattern for sanity before threaded
execution commences.
That's not the case. The pattern is compiled under threading in
start_threads() before any concurrent execution has started by calling
pthread_create(), so if the pattern contains an error we still do the
right thing. I.e. die with one error before any threaded execution has
commenced, instead of e.g. spewing out an error for each N threads,
which could be a regression a change like this might inadvertently
introduce.
This change is not meant as an optimization, any performance gains
from this are in the hundreds to thousands of nanoseconds at most. If
we wanted more performance here we could just re-use the compiled
patterns in multiple threads (regcomp(3) is thread-safe), or partially
re-use them and the associated structures in the case of later PCRE
JIT changes.
Rather, it's just to make the code easier to reason about. It's
confusing to debug this under threading & non-threading when the
threading codepaths redundantly compile a pattern which is never used.
The reason the patterns are recompiled is as a side-effect of
duplicating the whole grep_opt structure, which is not thread safe,
writable, and munged during execution. The grep_opt structure then
points to the grep_pat structure where pattern or patterns are stored.
I looked into e.g. splitting the API into some "do & alloc threadsafe
stuff", "spawn thread", "do and alloc non-threadsafe stuff", but the
execution time of grep_opt_dup() & pattern compilation is trivial
compared to actually executing the grep, so there was no point. Even
with the more expensive JIT changes to follow the most expensive PCRE
patterns take something like 0.0X milliseconds to compile at most[1].
The undocumented --debug mode added in commit 17bf35a3c7 ("grep: teach
--debug option to dump the parse tree", 2012-09-13) still works
properly with this change. It only emits debugging info during pattern
compilation, which is now dumped by the pattern compiled just before
the first thread is started.
1. http://sljit.sourceforge.net/pcre.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
From: Brandon Williams <hidden> Date: 2017-05-23 21:18:16
On 05/20, Ævar Arnfjörð Bjarmason wrote:
quoted hunk
Factor the test for \0 in grep patterns into a function. Since commit
9eceddeec6 ("Use kwset in grep", 2011-08-21) any pattern containing a
\0 is considered fixed as regcomp() can't handle it.
This change makes later changes that make use of either has_null() or
is_fixed() (but not both) smaller.
While I'm at it make the comment conform to the style guide, i.e. add
an opening "/*\n".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
Stop assigning 0 to the extended_regexp_option field right after we've
zeroed out the entire struct with memset() just a few lines earlier.
Unlike some of the code being refactored in subsequent commits, this
was always completely redundant. See the original code introduced in
84befcd0a4 ("grep: add a grep.patternType configuration setting",
2012-08-03).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 1 -
1 file changed, 1 deletion(-)
Here's a follow-up to a small tangent of discussion in my ~30 patch
grep cleanup series.
There are no functional changes here, just getting rid of dead code,
and removing the POSIX `regflags` variable from the grep API used by
grep/log, which was the cause of the long-standing bug of "-i" not
working with PCRE when used via git-log.
Ævar Arnfjörð Bjarmason (5):
grep: remove redundant double assignment to 0
grep: remove redundant grep pattern type assignment
grep: remove redundant "fixed" field re-assignment to 0
grep: remove redundant and verbose re-assignments to 0
grep: remove regflags from the public grep_opt API
builtin/grep.c | 2 --
grep.c | 59 +++++++++++++++++++++++++++++++++-------------------------
grep.h | 1 -
revision.c | 2 --
4 files changed, 34 insertions(+), 30 deletions(-)
--
2.13.1.611.g7e3b11ae1
Remove the redundant re-assignment of the fixed field to zero right
after the entire struct has been set to zero via memset(...).
Unlike some nearby commits this pattern doesn't date back to the
pattern described in e0b9f8ae09 ("grep: remove redundant regflags
assignments", 2017-05-25), instead it was apparently cargo-culted in
9eceddeec6 ("Use kwset in grep", 2011-08-21).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
Remove a redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0.
This is a logical follow-up to my commit to remove redundant regflags
assignments[1]. This logic was originally introduced in [2], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.
1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
2017-05-25)
2. b22520a37c ("grep: allow -E and -n to be turned on by default via
configuration", 2011-03-30)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to
zero right after the entire struct has been set to zero via
memset(...).
See an earlier related cleanup commit e0b9f8ae09 ("grep: remove
redundant regflags assignments", 2017-05-25) for an explanation of why
the code was structured like this to begin with.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 11 -----------
1 file changed, 11 deletions(-)
Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.
The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.
See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
--perl-regexp"[1] for the discussion leading up to this.
The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.
Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.
Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.
The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.
Unrelated to that: I could have factored out the default REG_NEWLINE
flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
since it's just used in two places I didn't think it was worth the
effort.
As an aside we're really lacking test coverage regflags being
initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
removed from compile_regexp(), but not if it's removed from
compile_fixed_regexp(). I have not dug to see if it's actually needed
in the latter case or if the test coverage is lacking.
1. [off-list ref]
(https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 2 --
grep.c | 43 ++++++++++++++++++++++++++++++++++---------
grep.h | 1 -
revision.c | 2 --
4 files changed, 34 insertions(+), 14 deletions(-)
From: Stefan Beller <hidden> Date: 2017-06-29 17:03:33
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted hunk
Remove a redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0.
This is a logical follow-up to my commit to remove redundant regflags
assignments[1]. This logic was originally introduced in [2], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.
1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
2017-05-25)
2. b22520a37c ("grep: allow -E and -n to be turned on by default via
configuration", 2011-03-30)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
Instead of having a condition here, have you considered to remove the
condition alltogether?
if (!strcmp(var, "grep.extendedregexp")) {
opt->extended_regexp_option = git_config_bool(var, value);
return 0;
}
This does not have the effect of not assigning the value in case of 0,
but it may be easier to reason about when reading the code.
This would also conform to the code below in that function, that parses
grep.linenumber or grep.fullname
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2017-06-29 17:10:42
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted hunk
Remove the redundant re-assignment of the fixed field to zero right
after the entire struct has been set to zero via memset(...).
Unlike some nearby commits this pattern doesn't date back to the
pattern described in e0b9f8ae09 ("grep: remove redundant regflags
assignments", 2017-05-25), instead it was apparently cargo-culted in
9eceddeec6 ("Use kwset in grep", 2011-08-21).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
I was about to propose a similar action as in 2/5,
but getting the condition right is not as easy:
p->fixed = (opt->fixed ||
has_null(p->pattern, p->patternlen) ||
is_fixed(p->pattern, p->patternlen)) &&
(!icase || ascii_only);
does not look as convincing here.
Thanks for mentioning 9eceddeec6 as in that commit
I would have been easy with just proposing to have
p->fixed = opt->fixed || is_fixed(p->pattern, p->patternlen);
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2017-06-29 17:43:15
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.
The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.
See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
--perl-regexp"[1] for the discussion leading up to this.
The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.
Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.
Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.
The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.
Up to here the commit message is inspiring confidence.
Unrelated to that: I could have factored out the default REG_NEWLINE
flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
since it's just used in two places I didn't think it was worth the
effort.
As an aside we're really lacking test coverage regflags being
initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
removed from compile_regexp(), but not if it's removed from
compile_fixed_regexp(). I have not dug to see if it's actually needed
in the latter case or if the test coverage is lacking.
I do not quite understand this. Are you saying
opt->pattern_type_option takes precedence over
opt->extended_regexp_option if the former is not _UNSPECIFIED ?
As grep_set_pattern_type_option is only called from here,
I wondered if we can put the long comment (and the code)
here in this function grep_commit_pattern_type to have it less
subtle? I have no proposal how though.
I think I grokked this patch and it makes sense, though the commit
message strongly hints at asking for tests. ;)
Thanks,
Stefan
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
Remove a redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0.
This is a logical follow-up to my commit to remove redundant regflags
assignments[1]. This logic was originally introduced in [2], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.
1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
2017-05-25)
2. b22520a37c ("grep: allow -E and -n to be turned on by default via
configuration", 2011-03-30)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
Instead of having a condition here, have you considered to remove the
condition alltogether?
if (!strcmp(var, "grep.extendedregexp")) {
opt->extended_regexp_option = git_config_bool(var, value);
return 0;
}
This does not have the effect of not assigning the value in case of 0,
but it may be easier to reason about when reading the code.
This would also conform to the code below in that function, that parses
grep.linenumber or grep.fullname
I didn't think about that. Good point. I'll do that instead in v2.
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.
The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.
See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
--perl-regexp"[1] for the discussion leading up to this.
The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.
Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.
Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.
The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.
Up to here the commit message is inspiring confidence.
Thanks.
quoted
Unrelated to that: I could have factored out the default REG_NEWLINE
flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
since it's just used in two places I didn't think it was worth the
effort.
As an aside we're really lacking test coverage regflags being
initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
removed from compile_regexp(), but not if it's removed from
compile_fixed_regexp(). I have not dug to see if it's actually needed
in the latter case or if the test coverage is lacking.
This sounds as if extra careful review is needed.
Note though (since I didn't say this explicitly) nothing about this
commit changes the semanics of what we pass to regcomp, I'm just noting
this caveat with REG_NEWLINE as an aside since I'm moving it around.
I do not quite understand this. Are you saying
opt->pattern_type_option takes precedence over
opt->extended_regexp_option if the former is not _UNSPECIFIED ?
I mean this "else if" code *must* be in that order, i.e.:
else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
grep_set_pattern_type_option(opt->pattern_type_option, opt);
else if (opt->extended_regexp_option)
grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
Not:
else if (opt->extended_regexp_option)
grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
grep_set_pattern_type_option(opt->pattern_type_option, opt);
Since we only want to pay attention to grep.extendedRegexp it
grep.patternType is not set. If grep.patternType is set then the
pattern_type_option will not be GREP_PATTERN_TYPE_UNSPECIFIED (but
e.g. GREP_PATTERN_TYPE_BRE).
As grep_set_pattern_type_option is only called from here,
I wondered if we can put the long comment (and the code)
here in this function grep_commit_pattern_type to have it less
subtle? I have no proposal how though.
Ah you mean the whole "When committing to the pattern type by" comment +
code. Yeah I think that makes sense. I'll try that for v2 and see if
it's better.
I think I grokked this patch and it makes sense, though the commit
message strongly hints at asking for tests. ;)
Addresses comments from Stefan Beller (thanks!). I looked into it and
the REG_NEWLINE flag was redundant in 1/2 cases, see 6/6 for the
removal of that.
I looked into refactoring 5/6 as noted in 87zicqirrg.fsf@gmail.com,
but for the reasons now explained in the last paragraph of 5/6 decided
not to and to keep it as it was.
Ævar Arnfjörð Bjarmason (6):
grep: remove redundant double assignment to 0
grep: adjust a redundant grep pattern type assignment
grep: remove redundant "fixed" field re-assignment to 0
grep: remove redundant and verbose re-assignments to 0
grep: remove regflags from the public grep_opt API
grep: remove redundant REG_NEWLINE when compiling fixed regex
builtin/grep.c | 2 --
grep.c | 62 +++++++++++++++++++++++++++++++++-------------------------
grep.h | 1 -
revision.c | 2 --
4 files changed, 35 insertions(+), 32 deletions(-)
--
2.13.1.611.g7e3b11ae1
Stop assigning 0 to the extended_regexp_option field right after we've
zeroed out the entire struct with memset() just a few lines earlier.
Unlike some of the code being refactored in subsequent commits, this
was always completely redundant. See the original code introduced in
84befcd0a4 ("grep: add a grep.patternType configuration setting",
2012-08-03).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 1 -
1 file changed, 1 deletion(-)
Adjust a now-redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0, so
there's no need to set it again to zero.
However the reason for the if/else pattern is a holdover from[1] where
this was adjusted from a bitfield assignment to a boolean. Rather than
getting rid of the assignment to 0 in all cases, let's just use the
value returned by git_config_bool(), which is more idiomatic and in
sync with the rest of the boolean handling in this function.
This is a logical follow-up to my commit to remove redundant regflags
assignments[2]. This logic was originally introduced in [3], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.
1. 84befcd0a4 ("grep: add a grep.patternType configuration setting",
2012-08-03)
2. e0b9f8ae09 ("grep: remove redundant regflags assignments",
2017-05-25)
3. b22520a37c ("grep: allow -E and -n to be turned on by default via
configuration", 2011-03-30)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Remove the redundant re-assignment of the fixed field to zero right
after the entire struct has been set to zero via memset(...).
Unlike some nearby commits this pattern doesn't date back to the
pattern described in e0b9f8ae09 ("grep: remove redundant regflags
assignments", 2017-05-25), instead it was apparently cargo-culted in
9eceddeec6 ("Use kwset in grep", 2011-08-21).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 --
1 file changed, 2 deletions(-)
Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to
zero right after the entire struct has been set to zero via
memset(...).
See an earlier related cleanup commit e0b9f8ae09 ("grep: remove
redundant regflags assignments", 2017-05-25) for an explanation of why
the code was structured like this to begin with.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 11 -----------
1 file changed, 11 deletions(-)
Remove the redundant REG_NEWLINE regcomp() flag from the code that
compiles a fixed-string regular-expression.
The REG_NEWLINE causes metacharacters such as "." to match a newline,
since the basic_regex_quote_buf() function being called here escapes
all metacharacters using REG_NEWLINE is confusing and redundant.
The use of this flag was introduced as an unintended emergent property
of 793dc676e0 ("grep/icase: avoid kwsset when -F is specified",
2016-06-25).
That change amended the existing regflags, which were initialized to
REG_NEWLINE in init_grep_defaults() assuming a subsequent non-fixed
regcomp().
Manual testing reveals that this was always redundant, since no flags
of any use were inherited from opt->regflags even back
then. 793dc676e0 passes all tests with this on top:
diff --git a/grep.c b/grep.c
index 627ae3e3e8..89e84ed7fd 100644
--- a/grep.c
+++ b/grep.c
@@ -407,3 +407,3 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
basic_regex_quote_buf(&sb, p->pattern);
- regflags = opt->regflags & ~REG_EXTENDED;
+ regflags = 0;
if (opt->ignore_case)
Since this isn't used for anything and never was, remove it to reduce
confusion when reading this code.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
grep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.
The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.
The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.
Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.
Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.
The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.
Even though grep_commit_pattern_type() is the only caller of
grep_set_pattern_type_option() it's simpler to reset the
extended_regexp_option flag in the latter, since 2/3 branches in the
former would otherwise need to reset it, this way we can do it in one
place.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/grep.c | 2 --
grep.c | 43 ++++++++++++++++++++++++++++++++++---------
grep.h | 1 -
revision.c | 2 --
4 files changed, 34 insertions(+), 14 deletions(-)