[PATCH v7 00/10] grep: simplify & delete "init" & "config" code
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-12-28 01:07:52
A re-roll that should *hopefully* address the comments Junio had in https://lore.kernel.org/git/xmqq4k6tyj8r.fsf@gitster.g/ (local); To quote those:
Junio C Hamano [off-list ref] writes: [...] By the way, the example I gave you for the previous round, and similarly the one in the message I am responding to were all written to help you realize that it is simply a broken approach if we do not keep "default" as default and instead resolve it to either "basic" or "extended" too early. The goal of these examples was *NOT* to tell you "this single thing is broken with the code in this round so let's fix it".
Yes, the v6 was broken. Sorry about that. Between re-rolling some other things and coming back to this series I managed to miss some of those subtleties.
So let me try to be a bit more explicit. Let's not do another round of "I think this is a moral equivalent of what you want, even though it is not done the way you suggested." I think we wasted a reroll or three with that attitude in changes leading to v6 already, after I gave my review to v5, and I think the v5 review essentially was a repeat of my review for v3's 3/7, so if I conveyed the point clearly enough back then, perhaps we didn't have to waste your time on v4 and v5, either. Sorry about that. So, here is what this step of the series SHOULD do:
So, at the risk of another wasted re-roll this doesn't exactly
implement what you suggested, but I think it'll finally cover all the
basis of being bug-for-bug compatible with the old implementation now.
I started this re-roll by implementing it exactly as you suggested,
along with new tests (both your fixup, and more), but as the updated
commit message in 09/10 details found that I could convert it back to
a state machine around a static variable in "grep_config()", which has
the advantage of getting rid of more "struct grep_opt" fields annd
narrowing the behavior to just that function.
More comments on the range-diff below:
Ævar Arnfjörð Bjarmason (10):
grep.h: remove unused "regex_t regexp" from grep_opt
log tests: check if grep_config() is called by "log"-like cmds
No changes.
grep tests: add missing "grep.patternType" config tests
A couple of new tests.
built-ins: trust the "prefix" from run_builtin()
grep.c: don't pass along NULL callback value
grep API: call grep_config() after grep_init()
No changes.
grep.h: make "grep_opt.pattern_type_option" use its enum
grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"
I split these two off from the previous tip commit to make the diff
for the next one smaller.
grep: simplify config parsing and option parsing
Changed, as noted above.
grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED
With that new behavior we can also get rid of
GREP_PATTERN_TYPE_UNSPECIFIED. It's still implicitly there as == 0,
but for a non-switch'd enum type treating these as "flags" makes more
sense, and makes the code in grep_config() more straightforward &
brief.
builtin/grep.c | 27 +++++-----
builtin/log.c | 13 ++++-
builtin/ls-tree.c | 2 +-
git.c | 1 +
grep.c | 126 +++++++++-------------------------------------
grep.h | 34 +++++++++----
revision.c | 4 +-
t/t4202-log.sh | 24 +++++++++
t/t7810-grep.sh | 68 +++++++++++++++++++++++++
9 files changed, 168 insertions(+), 131 deletions(-)
Range-diff against v6:
1: b62e6b6162a = 1: b62e6b6162a grep.h: remove unused "regex_t regexp" from grep_opt
2: 0edcdb50afd = 2: 0edcdb50afd log tests: check if grep_config() is called by "log"-like cmds
3: 1b724d5e2e9 ! 3: e1b4b5b77e0 grep tests: add missing "grep.patternType" config tests
@@ t/t7810-grep.sh: do
+ test_cmp expected actual
+ '
+
++ test_expect_success "grep $L with grep.extendedRegexp and grep.patternType are both last-one-wins independently (BRE)" '
++ echo "${HC}ab:abc" >expected &&
++ git \
++ -c grep.patternType=fixed \
++ -c grep.extendedRegexp=true \
++ -c grep.patternType=default \
++ grep "a+b*c" $H ab >actual &&
++ test_cmp expected actual
++ '
++
++ test_expect_success "grep $L with grep.extendedRegexp and grep.patternType are both last-one-wins independently (ERE)" '
++ echo "${HC}ab:a+bc" >expected &&
++ git \
++ -c grep.patternType=default \
++ -c grep.extendedRegexp=true \
++ -c grep.patternType=basic \
++ grep "a+b*c" $H ab >actual &&
++ test_cmp expected actual
++ '
++
+ test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
+ echo "${HC}ab:a+bc" >expected &&
+ git \
@@ t/t7810-grep.sh: do
test_expect_success "grep $L with grep.patternType=extended and grep.extendedRegexp=false" '
echo "${HC}ab:abc" >expected &&
git \
+@@ t/t7810-grep.sh: do
+ test_cmp expected actual
+ '
+
++ test_expect_success "grep $L with grep.extendedRegexp=false and grep.patternType=default" '
++ echo "${HC}ab:abc" >expected &&
++ git \
++ -c grep.extendedRegexp=false \
++ -c grep.patternType=extended \
++ grep "a+b*c" $H ab >actual &&
++ test_cmp expected actual
++ '
++
+ test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=basic" '
+ echo "${HC}ab:a+bc" >expected &&
+ git \
4: f4876552771 = 4: 6d91a765fd7 built-ins: trust the "prefix" from run_builtin()
5: 069b0339146 = 5: 844b4727ca3 grep.c: don't pass along NULL callback value
6: e38eca56959 = 6: d9cf9bf5e37 grep API: call grep_config() after grep_init()
-: ----------- > 7: 57ecc5c0d65 grep.h: make "grep_opt.pattern_type_option" use its enum
-: ----------- > 8: 7dbeafde26b grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"
7: 88dfd40bf9e ! 9: c6ca39b4554 grep: simplify config parsing and option parsing
@@ Commit message
grep_init(), which means that much of the complexity here can go
away.
- As before "grep.extendedRegexp" is a last-one-wins variable. We need
- to maintain state inside parse_pattern_type_arg() to ignore it if a
- non-"default" grep.patternType was seen, but otherwise flip between
- BRE and ERE for "grep.extendedRegexp=[false|true]".
+ As before both "grep.patternType" and "grep.extendedRegexp" are
+ last-one-wins variable, with "grep.extendedRegexp" yielding to
+ "grep.patternType", except when "grep.patternType=default".
+
+ Note that this applies as we parse the config, i.e. a sequence of:
+
+ -c grep.patternType=perl
+ -c grep.extendedRegexp=true \
+ -c grep.patternType=default
+
+ Should select ERE due to "grep.extendedRegexp=true and
+ grep.extendedRegexp=default", not BRE, even though that's the
+ "default" patternType. We can determine this as we parse the config,
+ because:
+
+ * If we see "grep.extendedRegexp" we set the internal "ero" to its
+ boolean value.
+
+ * If we see "grep.extendedRegexp" but
+ "grep.patternType=[default|<unset>]" is in effect we *don't* set
+ the internal "pattern_type_option" to update the pattern type.
+
+ * If we see "grep.patternType!=default" we can set our internal
+ "pattern_type_option" directly, it doesn't matter what the state of
+ "grep.extendedRegexp" is, but we don't forget what it was, in case
+ we see a "grep.patternType=default" again.
+
+ * If we see a "grep.patternType=default" we can set the pattern to
+ ERE or BRE depending on whether we last saw a
+ "grep.extendedRegexp=true" or
+ "grep.extendedRegexp=[false|<unset>]".
+
+ We could equally call this new adjust_pattern_type() in
+ compile_regexp(), i.e. this fixup on top of this passes all our
+ tests (with -U0 for brevity):
+
+ @@ -60,0 +61 @@ static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
+ +static int ero = -1;
+ @@ -65 +65,0 @@ int grep_config(const char *var, const char *value, void *cb)
+ - static int ero = -1;
+ @@ -72 +71,0 @@ int grep_config(const char *var, const char *value, void *cb)
+ - adjust_pattern_type(&opt->pattern_type_option, ero);
+ @@ -80 +78,0 @@ int grep_config(const char *var, const char *value, void *cb)
+ - adjust_pattern_type(&opt->pattern_type_option, ero);
+ @@ -445,0 +444,2 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
+ + if (ero != -1)
+ + adjust_pattern_type(&opt->pattern_type_option, ero);
+
+ But doing it as we stream the git_config() makes it
+ clear that we can determine the interplay between these two variables
+ as we go. We don't need to wait until we see the last value of the two
+ configuration variables.
+
+ This is true because of the rationale above, and because the
+ subsequent code in compile_regexp() treats
+ "pattern_type_option=GREP_PATTERN_TYPE_{UNSPECIFIED,BRE}"
+ equally. I.e. we'll end up with different internal
+ ""pattern_type_option" values there for:
+
+ # UNSPECIFIED
+ -c grep.patternType=default
+ # BRE
+ -c grep.extendedRegexp=false -c grep.patternType=default
+
+ But the difference won't matter, which simplifies some of this logic,
+ we never need to adjust a "grep.patternType" if we didn't see a
+ "grep.extendedRegexp" before. We can also remove the
+ "extended_regexp_option" member from "struct grep_opt" in favor of a
+ static variable in grep_config().
+
+ The command-line parsing in cmd_grep() can then completely ignore
+ "grep.extendedRegexp". Whatever effect it had before that step won't
+ matter if we see -G, -E, -P etc.
See my 07a3d411739 (grep: remove regflags from the public grep_opt
API, 2017-06-29) for addition of the two comments being removed here,
i.e. the complexity noted in that commit is now going away.
- We don't need grep_commit_pattern_type() anymore, we can instead have
- OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option"
- member in "struct grep_opt" directly.
-
- We can also do away with the indirection of the "int fixed" and "int
- pcre2" members in favor of using "pattern_type_option" directly in
- "grep.c".
-
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## builtin/grep.c ##
@@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
int fallback = 0;
## grep.c ##
-@@ grep.c: static const char *color_grep_slots[] = {
+@@ grep.c: static int parse_pattern_type_arg(const char *opt, const char *arg)
- static int parse_pattern_type_arg(const char *opt, const char *arg)
- {
-- if (!strcmp(arg, "default"))
-- return GREP_PATTERN_TYPE_UNSPECIFIED;
-- else if (!strcmp(arg, "basic"))
-+ if (!strcmp(arg, "basic"))
- return GREP_PATTERN_TYPE_BRE;
- else if (!strcmp(arg, "extended"))
- return GREP_PATTERN_TYPE_ERE;
+ define_list_config_array_extra(color_grep_slots, {"match"});
+
++static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
++{
++ if (*pto == GREP_PATTERN_TYPE_UNSPECIFIED)
++ *pto = ero ? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
++}
++
+ /*
+ * Read the configuration file once and store it in
+ * the grep_defaults template.
@@ grep.c: int grep_config(const char *var, const char *value, void *cb)
+ {
+ struct grep_opt *opt = cb;
+ const char *slot;
++ static int ero = -1;
+
+ if (userdiff_config(var, value) < 0)
return -1;
if (!strcmp(var, "grep.extendedregexp")) {
-+ if (opt->extended_regexp_option == -1)
-+ return 0;
- opt->extended_regexp_option = git_config_bool(var, value);
-+ if (opt->extended_regexp_option)
-+ opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
-+ else
-+ opt->pattern_type_option = GREP_PATTERN_TYPE_BRE;
-+ return 0;
-+ }
-+
-+ if (!strcmp(var, "grep.patterntype") &&
-+ !strcmp(value, "default")) {
-+ opt->pattern_type_option = opt->extended_regexp_option == 1
-+ ? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
+- opt->extended_regexp_option = git_config_bool(var, value);
++ ero = git_config_bool(var, value);
++ adjust_pattern_type(&opt->pattern_type_option, ero);
return 0;
}
if (!strcmp(var, "grep.patterntype")) {
-+ opt->extended_regexp_option = -1; /* ignore */
opt->pattern_type_option = parse_pattern_type_arg(var, value);
++ if (ero == -1)
++ return 0;
++ adjust_pattern_type(&opt->pattern_type_option, ero);
return 0;
}
+
@@ grep.c: void grep_init(struct grep_opt *opt, struct repository *repo)
opt->header_tail = &opt->header_list;
}
@@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
- p->fixed = opt->fixed;
+ p->fixed = opt->pattern_type_option == GREP_PATTERN_TYPE_FIXED;
-- if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2)
+- if (!opt->pcre2 &&
+ if (opt->pattern_type_option != GREP_PATTERN_TYPE_PCRE &&
-+ memchr(p->pattern, 0, p->patternlen))
+ memchr(p->pattern, 0, p->patternlen))
die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2"));
- p->is_fixed = is_fixed(p->pattern, p->patternlen);
@@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
return;
}
@@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
if (err) {
## grep.h ##
-@@ grep.h: enum grep_expr_node {
- };
-
- enum grep_pattern_type {
-- GREP_PATTERN_TYPE_UNSPECIFIED = 0,
-- GREP_PATTERN_TYPE_BRE,
-+ GREP_PATTERN_TYPE_BRE = 0,
- GREP_PATTERN_TYPE_ERE,
- GREP_PATTERN_TYPE_FIXED,
- GREP_PATTERN_TYPE_PCRE
@@ grep.h: struct grep_opt {
int unmatch_name_only;
int count;
@@ grep.h: struct grep_opt {
int pathname;
int null_following_name;
@@ grep.h: struct grep_opt {
+ int max_depth;
int funcname;
int funcbody;
- int extended_regexp_option;
-- int pattern_type_option;
-+ enum grep_pattern_type pattern_type_option;
+- int extended_regexp_option;
+ enum grep_pattern_type pattern_type_option;
int ignore_locale;
char colors[NR_GREP_COLORS][COLOR_MAXLEN];
- unsigned pre_context;
-@@ grep.h: struct grep_opt {
- .relative = 1, \
- .pathname = 1, \
- .max_depth = -1, \
-- .pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
- .colors = { \
- [GREP_COLOR_CONTEXT] = "", \
- [GREP_COLOR_FILENAME] = "", \
@@ grep.h: struct grep_opt {
int grep_config(const char *var, const char *value, void *);
-: ----------- > 10: b764c09d2b7 grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED
--
2.34.1.1250.g6a242c1e9ad