{"thread":{"id":"46282","subject":"[PATCH 1/5] grep: remove redundant double assignment to 0","startedAt":"2017-06-28T21:58:30Z","lastAt":"2017-06-30T17:20:18Z","messageCount":19,"participants":["Ævar Arnfjörð Bjarmason","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"323455","messageId":"20170628215809.23060-2-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH 1/5] grep: remove redundant double assignment to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:05Z","receivedAt":"2017-06-28T21:58:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop assigning 0 to the extended_regexp_option field right after we've\nzeroed out the entire struct with memset() just a few lines earlier.\n\nUnlike some of the code being refactored in subsequent commits, this\nwas always completely redundant. See the original code introduced in\n84befcd0a4 (\"grep: add a grep.patternType configuration setting\",\n2012-08-03).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 98733db623..29439886e7 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -38,7 +38,6 @@ void init_grep_defaults(void)\n \topt->regflags = REG_NEWLINE;\n \topt->max_depth = -1;\n \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n-\topt->extended_regexp_option = 0;\n \tcolor_set(opt->color_context, \"\");\n \tcolor_set(opt->color_filename, \"\");\n \tcolor_set(opt->color_function, \"\");\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323456","messageId":"20170628215809.23060-1-avarab@gmail.com","threadId":"46282","inReplyTo":"CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com","subject":"[PATCH 0/5] grep: remove redundant code & reflags from API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:04Z","receivedAt":"2017-06-28T21:58:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Here's a follow-up to a small tangent of discussion in my ~30 patch\ngrep cleanup series.\n\nThere are no functional changes here, just getting rid of dead code,\nand removing the POSIX `regflags` variable from the grep API used by\ngrep/log, which was the cause of the long-standing bug of \"-i\" not\nworking with PCRE when used via git-log.\n\nÆvar Arnfjörð Bjarmason (5):\n grep: remove redundant double assignment to 0\n grep: remove redundant grep pattern type assignment\n grep: remove redundant \"fixed\" field re-assignment to 0\n grep: remove redundant and verbose re-assignments to 0\n grep: remove regflags from the public grep_opt API\n\n builtin/grep.c | 2 --\n grep.c | 59 +++++++++++++++++++++++++++++++++-------------------------\n grep.h | 1 -\n revision.c | 2 --\n 4 files changed, 34 insertions(+), 30 deletions(-)\n\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323457","messageId":"20170628215809.23060-4-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH 3/5] grep: remove redundant \"fixed\" field re-assignment to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:07Z","receivedAt":"2017-06-28T21:58:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the redundant re-assignment of the fixed field to zero right\nafter the entire struct has been set to zero via memset(...).\n\nUnlike some nearby commits this pattern doesn't date back to the\npattern described in e0b9f8ae09 (\"grep: remove redundant regflags\nassignments\", 2017-05-25), instead it was apparently cargo-culted in\n9eceddeec6 (\"Use kwset in grep\", 2011-08-21).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 6614042fdc..7cd8a6512f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t has_null(p->pattern, p->patternlen) ||\n \t is_fixed(p->pattern, p->patternlen))\n \t\tp->fixed = !icase || ascii_only;\n-\telse\n-\t\tp->fixed = 0;\n \n \tif (p->fixed) {\n \t\tp->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323458","messageId":"20170628215809.23060-3-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH 2/5] grep: remove redundant grep pattern type assignment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:06Z","receivedAt":"2017-06-28T21:58:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove a redundant assignment to extended_regexp_option to make it\nzero if grep.extendedRegexp is not set. This is always called right\nafter init_grep_defaults() which memsets the entire structure to 0.\n\nThis is a logical follow-up to my commit to remove redundant regflags\nassignments[1]. This logic was originally introduced in [2], but as\nexplained in the former commit it's working around a pattern in our\ncode that no longer exists, and is now confusing as it leads the\nreader to think that this needs to be flipped back & forth.\n\n1. e0b9f8ae09 (\"grep: remove redundant regflags assignments\",\n 2017-05-25)\n2. b22520a37c (\"grep: allow -E and -n to be turned on by default via\n configuration\", 2011-03-30)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 29439886e7..6614042fdc 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"grep.extendedregexp\")) {\n \t\tif (git_config_bool(var, value))\n \t\t\topt->extended_regexp_option = 1;\n-\t\telse\n-\t\t\topt->extended_regexp_option = 0;\n \t\treturn 0;\n \t}\n \n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323459","messageId":"20170628215809.23060-5-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH 4/5] grep: remove redundant and verbose re-assignments to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:08Z","receivedAt":"2017-06-28T21:58:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to\nzero right after the entire struct has been set to zero via\nmemset(...).\n\nSee an earlier related cleanup commit e0b9f8ae09 (\"grep: remove\nredundant regflags assignments\", 2017-05-25) for an explanation of why\nthe code was structured like this to begin with.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 11 -----------\n 1 file changed, 11 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 7cd8a6512f..736e1e00d6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -175,28 +175,18 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\t/* fall through */\n \n \tcase GREP_PATTERN_TYPE_BRE:\n-\t\topt->fixed = 0;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n-\t\topt->fixed = 0;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\topt->regflags |= REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\topt->fixed = 1;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_PCRE:\n-\t\topt->fixed = 0;\n #ifdef USE_LIBPCRE2\n-\t\topt->pcre1 = 0;\n \t\topt->pcre2 = 1;\n #else\n \t\t/*\n@@ -206,7 +196,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\t * \"cannot use Perl-compatible regexes[...]\".\n \t\t */\n \t\topt->pcre1 = 1;\n-\t\topt->pcre2 = 0;\n #endif\n \t\tbreak;\n \t}\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323460","messageId":"20170628215809.23060-6-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH 5/5] grep: remove regflags from the public grep_opt API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-28T21:58:09Z","receivedAt":"2017-06-28T21:58:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor calls to the grep machinery to always pass opt.ignore_case &\nopt.extended_regexp_option instead of setting the equivalent regflags\nbits.\n\nThe bug fixed when making -i work with -P in commit 9e3cbc59d5 (\"log:\nmake --regexp-ignore-case work with --perl-regexp\", 2017-05-20) was\nreally just plastering over the code smell which this change fixes.\n\nSee my \"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with\n--perl-regexp\"[1] for the discussion leading up to this.\n\nThe reason for adding the extensive commentary here is that I\ndiscovered some subtle complexity in implementing this that really\nshould be called out explicitly to future readers.\n\nBefore this change we'd rely on the difference between\n`extended_regexp_option` and `regflags` to serve as a membrane between\nour preliminary parsing of grep.extendedRegexp and grep.patternType,\nand what we decided to do internally.\n\nNow that those two are the same thing, it's necessary to unset\n`extended_regexp_option` just before we commit in cases where both of\nthose config variables are set. See 84befcd0a4 (\"grep: add a\ngrep.patternType configuration setting\", 2012-08-03) for the code and\ndocumentation related to that.\n\nThe explanation of why the if/else branches in\ngrep_commit_pattern_type() are ordered the way they are exists in that\ncommit message, but I think it's worth calling this subtlety out\nexplicitly with a comment for future readers.\n\nUnrelated to that: I could have factored out the default REG_NEWLINE\nflag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but\nsince it's just used in two places I didn't think it was worth the\neffort.\n\nAs an aside we're really lacking test coverage regflags being\ninitiated as 0 instead of as REG_NEWLINE. Tests will fail if it's\nremoved from compile_regexp(), but not if it's removed from\ncompile_fixed_regexp(). I have not dug to see if it's actually needed\nin the latter case or if the test coverage is lacking.\n\n1. \n (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 2 --\n grep.c | 43 ++++++++++++++++++++++++++++++++++---------\n grep.h | 1 -\n revision.c | 2 --\n 4 files changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f61a9d938b..b682966439 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (!opt.pattern_list)\n \t\tdie(_(\"no pattern given.\"));\n-\tif (!opt.fixed && opt.ignore_case)\n-\t\topt.regflags |= REG_ICASE;\n \n \t/*\n \t * We have to find \"--\" in a separate pass, because its presence\ndiff --git a/grep.c b/grep.c\nindex 736e1e00d6..51aaad9f03 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -35,7 +35,6 @@ void init_grep_defaults(void)\n \tmemset(opt, 0, sizeof(*opt));\n \topt->relative = 1;\n \topt->pathname = 1;\n-\topt->regflags = REG_NEWLINE;\n \topt->max_depth = -1;\n \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tcolor_set(opt->color_context, \"\");\n@@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \topt->linenum = def->linenum;\n \topt->max_depth = def->max_depth;\n \topt->pathname = def->pathname;\n-\topt->regflags = def->regflags;\n \topt->relative = def->relative;\n \topt->output = def->output;\n \n@@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n+\t/*\n+\t * When committing to the pattern type by setting the relevant\n+\t * fields in grep_opt it's generally not necessary to zero out\n+\t * the fields we're not choosing, since they won't have been\n+\t * set by anything. The extended_regexp_option field is the\n+\t * only exception to this.\n+\t *\n+\t * This is because in the process of parsing grep.patternType\n+\t * & grep.extendedRegexp we set opt->pattern_type_option and\n+\t * opt->extended_regexp_option, respectively. We then\n+\t * internally use opt->extended_regexp_option to see if we're\n+\t * compiling an ERE. It must be unset if that's not actually\n+\t * the case.\n+\t */\n+\tif (pattern_type != GREP_PATTERN_TYPE_ERE &&\n+\t opt->extended_regexp_option)\n+\t\topt->extended_regexp_option = 0;\n+\n \tswitch (pattern_type) {\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n \t\t/* fall through */\n@@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n-\t\topt->regflags |= REG_EXTENDED;\n+\t\topt->extended_regexp_option = 1;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n@@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n \telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n \t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n \telse if (opt->extended_regexp_option)\n+\t\t/*\n+\t\t * This branch *must* happen after setting from the\n+\t\t * opt->pattern_type_option above, we don't want\n+\t\t * grep.extendedRegexp to override grep.patternType!\n+\t\t */\n \t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n }\n \n@@ -573,7 +594,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint err;\n-\tint regflags = opt->regflags;\n+\tint regflags = REG_NEWLINE;\n \n \tbasic_regex_quote_buf(&sb, p->pattern);\n \tif (opt->ignore_case)\n@@ -592,12 +613,12 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n \n static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n-\tint icase, ascii_only;\n+\tint ascii_only;\n \tint err;\n+\tint regflags = REG_NEWLINE;\n \n \tp->word_regexp = opt->word_regexp;\n \tp->ignore_case = opt->ignore_case;\n-\ticase\t = opt->regflags & REG_ICASE || p->ignore_case;\n \tascii_only = !has_non_ascii(p->pattern);\n \n \t/*\n@@ -615,10 +636,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tif (opt->fixed ||\n \t has_null(p->pattern, p->patternlen) ||\n \t is_fixed(p->pattern, p->patternlen))\n-\t\tp->fixed = !icase || ascii_only;\n+\t\tp->fixed = !p->ignore_case || ascii_only;\n \n \tif (p->fixed) {\n-\t\tp->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n+\t\tp->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);\n \t\tkwsincr(p->kws, p->pattern, p->patternlen);\n \t\tkwsprep(p->kws);\n \t\treturn;\n@@ -642,7 +663,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t\treturn;\n \t}\n \n-\terr = regcomp(&p->regexp, p->pattern, opt->regflags);\n+\tif (p->ignore_case)\n+\t\tregflags |= REG_ICASE;\n+\tif (opt->extended_regexp_option)\n+\t\tregflags |= REG_EXTENDED;\n+\terr = regcomp(&p->regexp, p->pattern, regflags);\n \tif (err) {\n \t\tchar errbuf[1024];\n \t\tregerror(err, &p->regexp, errbuf, 1024);\ndiff --git a/grep.h b/grep.h\nindex b8f93bfc2d..0c091e5104 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -162,7 +162,6 @@ struct grep_opt {\n \tchar color_match_selected[COLOR_MAXLEN];\n \tchar color_selected[COLOR_MAXLEN];\n \tchar color_sep[COLOR_MAXLEN];\n-\tint regflags;\n \tunsigned pre_context;\n \tunsigned post_context;\n \tunsigned last_shown;\ndiff --git a/revision.c b/revision.c\nindex e181ad1b70..207103d211 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, prefix);\n \trevs->grep_filter.status_only = 1;\n-\trevs->grep_filter.regflags = REG_NEWLINE;\n \n \tdiff_setup(&revs->diffopt);\n \tif (prefix && !revs->diffopt.prefix) {\n@@ -2022,7 +2021,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n \t} else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n \t\trevs->grep_filter.ignore_case = 1;\n-\t\trevs->grep_filter.regflags |= REG_ICASE;\n \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323479","messageId":"CAGZ79kb0a2EEALj3eEAMUwQSWamhGuOFOYPrWbv6uCtC7EXgAw@mail.gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-3-avarab@gmail.com","subject":"Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-29T17:03:27Z","receivedAt":"2017-06-29T17:03:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Remove a redundant assignment to extended_regexp_option to make it\n> zero if grep.extendedRegexp is not set. This is always called right\n> after init_grep_defaults() which memsets the entire structure to 0.\n>\n> This is a logical follow-up to my commit to remove redundant regflags\n> assignments[1]. This logic was originally introduced in [2], but as\n> explained in the former commit it's working around a pattern in our\n> code that no longer exists, and is now confusing as it leads the\n> reader to think that this needs to be flipped back & forth.\n>\n> 1. e0b9f8ae09 (\"grep: remove redundant regflags assignments\",\n> 2017-05-25)\n> 2. b22520a37c (\"grep: allow -E and -n to be turned on by default via\n> configuration\", 2011-03-30)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 29439886e7..6614042fdc 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)\n> if (!strcmp(var, \"grep.extendedregexp\")) {\n> if (git_config_bool(var, value))\n> opt->extended_regexp_option = 1;\n> - else\n> - opt->extended_regexp_option = 0;\n> return 0;\n\nInstead of having a condition here, have you considered to remove the\ncondition alltogether?\n\n if (!strcmp(var, \"grep.extendedregexp\")) {\n opt->extended_regexp_option = git_config_bool(var, value);\n return 0;\n }\n\nThis does not have the effect of not assigning the value in case of 0,\nbut it may be easier to reason about when reading the code.\n\nThis would also conform to the code below in that function, that parses\ngrep.linenumber or grep.fullname\n\nThanks,\nStefan\n"},{"id":"323480","messageId":"CAGZ79kaLEA+6DDgWnqw-hg1GrWVD7CgGNAkmXx_iQmeKGSJUqQ@mail.gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-4-avarab@gmail.com","subject":"Re: [PATCH 3/5] grep: remove redundant \"fixed\" field re-assignment to 0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-29T17:10:36Z","receivedAt":"2017-06-29T17:10:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Remove the redundant re-assignment of the fixed field to zero right\n> after the entire struct has been set to zero via memset(...).\n>\n> Unlike some nearby commits this pattern doesn't date back to the\n> pattern described in e0b9f8ae09 (\"grep: remove redundant regflags\n> assignments\", 2017-05-25), instead it was apparently cargo-culted in\n> 9eceddeec6 (\"Use kwset in grep\", 2011-08-21).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 6614042fdc..7cd8a6512f 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n> has_null(p->pattern, p->patternlen) ||\n> is_fixed(p->pattern, p->patternlen))\n> p->fixed = !icase || ascii_only;\n> - else\n> - p->fixed = 0;\n>\n\nI was about to propose a similar action as in 2/5,\nbut getting the condition right is not as easy:\n\n p->fixed = (opt->fixed ||\n has_null(p->pattern, p->patternlen) ||\n is_fixed(p->pattern, p->patternlen)) &&\n (!icase || ascii_only);\n\ndoes not look as convincing here.\n\nThanks for mentioning 9eceddeec6 as in that commit\nI would have been easy with just proposing to have\n\n p->fixed = opt->fixed || is_fixed(p->pattern, p->patternlen);\n\nThanks,\nStefan\n"},{"id":"323481","messageId":"CAGZ79kbOpMpi0Dv6=ViW45gq5E9KHgpz4GE4o8XA-KMaiR78Vw@mail.gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-6-avarab@gmail.com","subject":"Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-29T17:43:09Z","receivedAt":"2017-06-29T17:43:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Refactor calls to the grep machinery to always pass opt.ignore_case &\n> opt.extended_regexp_option instead of setting the equivalent regflags\n> bits.\n>\n> The bug fixed when making -i work with -P in commit 9e3cbc59d5 (\"log:\n> make --regexp-ignore-case work with --perl-regexp\", 2017-05-20) was\n> really just plastering over the code smell which this change fixes.\n>\n> See my \"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with\n> --perl-regexp\"[1] for the discussion leading up to this.\n>\n> The reason for adding the extensive commentary here is that I\n> discovered some subtle complexity in implementing this that really\n> should be called out explicitly to future readers.\n>\n> Before this change we'd rely on the difference between\n> `extended_regexp_option` and `regflags` to serve as a membrane between\n> our preliminary parsing of grep.extendedRegexp and grep.patternType,\n> and what we decided to do internally.\n>\n> Now that those two are the same thing, it's necessary to unset\n> `extended_regexp_option` just before we commit in cases where both of\n> those config variables are set. See 84befcd0a4 (\"grep: add a\n> grep.patternType configuration setting\", 2012-08-03) for the code and\n> documentation related to that.\n>\n> The explanation of why the if/else branches in\n> grep_commit_pattern_type() are ordered the way they are exists in that\n> commit message, but I think it's worth calling this subtlety out\n> explicitly with a comment for future readers.\n\nUp to here the commit message is inspiring confidence.\n\n>\n> Unrelated to that: I could have factored out the default REG_NEWLINE\n> flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but\n> since it's just used in two places I didn't think it was worth the\n> effort.\n>\n> As an aside we're really lacking test coverage regflags being\n> initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's\n> removed from compile_regexp(), but not if it's removed from\n> compile_fixed_regexp(). I have not dug to see if it's actually needed\n> in the latter case or if the test coverage is lacking.\n\nThis sounds as if extra careful review is needed.\n\n\n>\n> 1. \n> (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/grep.c | 2 --\n> grep.c | 43 ++++++++++++++++++++++++++++++++++---------\n> grep.h | 1 -\n> revision.c | 2 --\n> 4 files changed, 34 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index f61a9d938b..b682966439 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>\n> if (!opt.pattern_list)\n> die(_(\"no pattern given.\"));\n> - if (!opt.fixed && opt.ignore_case)\n> - opt.regflags |= REG_ICASE;\n>\n> /*\n> * We have to find \"--\" in a separate pass, because its presence\n> diff --git a/grep.c b/grep.c\n> index 736e1e00d6..51aaad9f03 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -35,7 +35,6 @@ void init_grep_defaults(void)\n> memset(opt, 0, sizeof(*opt));\n> opt->relative = 1;\n> opt->pathname = 1;\n> - opt->regflags = REG_NEWLINE;\n> opt->max_depth = -1;\n> opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n> color_set(opt->color_context, \"\");\n> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n> opt->linenum = def->linenum;\n> opt->max_depth = def->max_depth;\n> opt->pathname = def->pathname;\n> - opt->regflags = def->regflags;\n> opt->relative = def->relative;\n> opt->output = def->output;\n>\n> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>\n> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n> {\n> + /*\n> + * When committing to the pattern type by setting the relevant\n> + * fields in grep_opt it's generally not necessary to zero out\n> + * the fields we're not choosing, since they won't have been\n> + * set by anything. The extended_regexp_option field is the\n> + * only exception to this.\n> + *\n> + * This is because in the process of parsing grep.patternType\n> + * & grep.extendedRegexp we set opt->pattern_type_option and\n> + * opt->extended_regexp_option, respectively. We then\n> + * internally use opt->extended_regexp_option to see if we're\n> + * compiling an ERE. It must be unset if that's not actually\n> + * the case.\n> + */\n> + if (pattern_type != GREP_PATTERN_TYPE_ERE &&\n> + opt->extended_regexp_option)\n> + opt->extended_regexp_option = 0;\n> +\n> switch (pattern_type) {\n> case GREP_PATTERN_TYPE_UNSPECIFIED:\n> /* fall through */\n> @@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n> break;\n>\n> case GREP_PATTERN_TYPE_ERE:\n> - opt->regflags |= REG_EXTENDED;\n> + opt->extended_regexp_option = 1;\n> break;\n>\n> case GREP_PATTERN_TYPE_FIXED:\n> @@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n> else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n> grep_set_pattern_type_option(opt->pattern_type_option, opt);\n> else if (opt->extended_regexp_option)\n> + /*\n> + * This branch *must* happen after setting from the\n> + * opt->pattern_type_option above,\n\nI do not quite understand this. Are you saying\n\n opt->pattern_type_option takes precedence over\n opt->extended_regexp_option if the former is not _UNSPECIFIED ?\n\nAs grep_set_pattern_type_option is only called from here,\nI wondered if we can put the long comment (and the code)\nhere in this function grep_commit_pattern_type to have it less\nsubtle? I have no proposal how though.\n\nI think I grokked this patch and it makes sense, though the commit\nmessage strongly hints at asking for tests. ;)\n\nThanks,\nStefan\n"},{"id":"323482","messageId":"871sq2k7jp.fsf@gmail.com","threadId":"46282","inReplyTo":"CAGZ79kb0a2EEALj3eEAMUwQSWamhGuOFOYPrWbv6uCtC7EXgAw@mail.gmail.com","subject":"Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T17:50:02Z","receivedAt":"2017-06-29T17:50:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 29 2017, Stefan Beller jotted:\n\n> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> Remove a redundant assignment to extended_regexp_option to make it\n>> zero if grep.extendedRegexp is not set. This is always called right\n>> after init_grep_defaults() which memsets the entire structure to 0.\n>>\n>> This is a logical follow-up to my commit to remove redundant regflags\n>> assignments[1]. This logic was originally introduced in [2], but as\n>> explained in the former commit it's working around a pattern in our\n>> code that no longer exists, and is now confusing as it leads the\n>> reader to think that this needs to be flipped back & forth.\n>>\n>> 1. e0b9f8ae09 (\"grep: remove redundant regflags assignments\",\n>> 2017-05-25)\n>> 2. b22520a37c (\"grep: allow -E and -n to be turned on by default via\n>> configuration\", 2011-03-30)\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> grep.c | 2 --\n>> 1 file changed, 2 deletions(-)\n>>\n>> diff --git a/grep.c b/grep.c\n>> index 29439886e7..6614042fdc 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)\n>> if (!strcmp(var, \"grep.extendedregexp\")) {\n>> if (git_config_bool(var, value))\n>> opt->extended_regexp_option = 1;\n>> - else\n>> - opt->extended_regexp_option = 0;\n>> return 0;\n>\n> Instead of having a condition here, have you considered to remove the\n> condition alltogether?\n>\n> if (!strcmp(var, \"grep.extendedregexp\")) {\n> opt->extended_regexp_option = git_config_bool(var, value);\n> return 0;\n> }\n>\n> This does not have the effect of not assigning the value in case of 0,\n> but it may be easier to reason about when reading the code.\n>\n> This would also conform to the code below in that function, that parses\n> grep.linenumber or grep.fullname\n\nI didn't think about that. Good point. I'll do that instead in v2.\n"},{"id":"323487","messageId":"87zicqirrg.fsf@gmail.com","threadId":"46282","inReplyTo":"CAGZ79kbOpMpi0Dv6=ViW45gq5E9KHgpz4GE4o8XA-KMaiR78Vw@mail.gmail.com","subject":"Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T18:16:19Z","receivedAt":"2017-06-29T18:16:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 29 2017, Stefan Beller jotted:\n\n> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> Refactor calls to the grep machinery to always pass opt.ignore_case &\n>> opt.extended_regexp_option instead of setting the equivalent regflags\n>> bits.\n>>\n>> The bug fixed when making -i work with -P in commit 9e3cbc59d5 (\"log:\n>> make --regexp-ignore-case work with --perl-regexp\", 2017-05-20) was\n>> really just plastering over the code smell which this change fixes.\n>>\n>> See my \"Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with\n>> --perl-regexp\"[1] for the discussion leading up to this.\n>>\n>> The reason for adding the extensive commentary here is that I\n>> discovered some subtle complexity in implementing this that really\n>> should be called out explicitly to future readers.\n>>\n>> Before this change we'd rely on the difference between\n>> `extended_regexp_option` and `regflags` to serve as a membrane between\n>> our preliminary parsing of grep.extendedRegexp and grep.patternType,\n>> and what we decided to do internally.\n>>\n>> Now that those two are the same thing, it's necessary to unset\n>> `extended_regexp_option` just before we commit in cases where both of\n>> those config variables are set. See 84befcd0a4 (\"grep: add a\n>> grep.patternType configuration setting\", 2012-08-03) for the code and\n>> documentation related to that.\n>>\n>> The explanation of why the if/else branches in\n>> grep_commit_pattern_type() are ordered the way they are exists in that\n>> commit message, but I think it's worth calling this subtlety out\n>> explicitly with a comment for future readers.\n>\n> Up to here the commit message is inspiring confidence.\n\nThanks.\n\n>>\n>> Unrelated to that: I could have factored out the default REG_NEWLINE\n>> flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but\n>> since it's just used in two places I didn't think it was worth the\n>> effort.\n>>\n>> As an aside we're really lacking test coverage regflags being\n>> initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's\n>> removed from compile_regexp(), but not if it's removed from\n>> compile_fixed_regexp(). I have not dug to see if it's actually needed\n>> in the latter case or if the test coverage is lacking.\n>\n> This sounds as if extra careful review is needed.\n\nNote though (since I didn't say this explicitly) nothing about this\ncommit changes the semanics of what we pass to regcomp, I'm just noting\nthis caveat with REG_NEWLINE as an aside since I'm moving it around.\n\n>>\n>> 1. \n>> (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/grep.c | 2 --\n>> grep.c | 43 ++++++++++++++++++++++++++++++++++---------\n>> grep.h | 1 -\n>> revision.c | 2 --\n>> 4 files changed, 34 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index f61a9d938b..b682966439 100644\n>> --- a/builtin/grep.c\n>> +++ b/builtin/grep.c\n>> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>\n>> if (!opt.pattern_list)\n>> die(_(\"no pattern given.\"));\n>> - if (!opt.fixed && opt.ignore_case)\n>> - opt.regflags |= REG_ICASE;\n>>\n>> /*\n>> * We have to find \"--\" in a separate pass, because its presence\n>> diff --git a/grep.c b/grep.c\n>> index 736e1e00d6..51aaad9f03 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -35,7 +35,6 @@ void init_grep_defaults(void)\n>> memset(opt, 0, sizeof(*opt));\n>> opt->relative = 1;\n>> opt->pathname = 1;\n>> - opt->regflags = REG_NEWLINE;\n>> opt->max_depth = -1;\n>> opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n>> color_set(opt->color_context, \"\");\n>> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>> opt->linenum = def->linenum;\n>> opt->max_depth = def->max_depth;\n>> opt->pathname = def->pathname;\n>> - opt->regflags = def->regflags;\n>> opt->relative = def->relative;\n>> opt->output = def->output;\n>>\n>> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>>\n>> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n>> {\n>> + /*\n>> + * When committing to the pattern type by setting the relevant\n>> + * fields in grep_opt it's generally not necessary to zero out\n>> + * the fields we're not choosing, since they won't have been\n>> + * set by anything. The extended_regexp_option field is the\n>> + * only exception to this.\n>> + *\n>> + * This is because in the process of parsing grep.patternType\n>> + * & grep.extendedRegexp we set opt->pattern_type_option and\n>> + * opt->extended_regexp_option, respectively. We then\n>> + * internally use opt->extended_regexp_option to see if we're\n>> + * compiling an ERE. It must be unset if that's not actually\n>> + * the case.\n>> + */\n>> + if (pattern_type != GREP_PATTERN_TYPE_ERE &&\n>> + opt->extended_regexp_option)\n>> + opt->extended_regexp_option = 0;\n>> +\n>> switch (pattern_type) {\n>> case GREP_PATTERN_TYPE_UNSPECIFIED:\n>> /* fall through */\n>> @@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n>> break;\n>>\n>> case GREP_PATTERN_TYPE_ERE:\n>> - opt->regflags |= REG_EXTENDED;\n>> + opt->extended_regexp_option = 1;\n>> break;\n>>\n>> case GREP_PATTERN_TYPE_FIXED:\n>> @@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n>> else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n>> grep_set_pattern_type_option(opt->pattern_type_option, opt);\n>> else if (opt->extended_regexp_option)\n>> + /*\n>> + * This branch *must* happen after setting from the\n>> + * opt->pattern_type_option above,\n>\n> I do not quite understand this. Are you saying\n>\n> opt->pattern_type_option takes precedence over\n> opt->extended_regexp_option if the former is not _UNSPECIFIED ?\n\nI mean this \"else if\" code *must* be in that order, i.e.:\n\n\telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n\t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n\telse if (opt->extended_regexp_option)\n\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n\nNot:\n\n\telse if (opt->extended_regexp_option)\n\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n\telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n\t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n\nSince we only want to pay attention to grep.extendedRegexp it\ngrep.patternType is not set. If grep.patternType is set then the\npattern_type_option will not be GREP_PATTERN_TYPE_UNSPECIFIED (but\ne.g. GREP_PATTERN_TYPE_BRE).\n\n> As grep_set_pattern_type_option is only called from here,\n> I wondered if we can put the long comment (and the code)\n> here in this function grep_commit_pattern_type to have it less\n> subtle? I have no proposal how though.\n\nAh you mean the whole \"When committing to the pattern type by\" comment +\ncode. Yeah I think that makes sense. I'll try that for v2 and see if\nit's better.\n\n> I think I grokked this patch and it makes sense, though the commit\n> message strongly hints at asking for tests. ;)\n\n*Points up at \"moving it around\" comment above*\n"},{"id":"323501","messageId":"20170629222222.4694-1-avarab@gmail.com","threadId":"46282","inReplyTo":"20170628215809.23060-1-avarab@gmail.com","subject":"[PATCH v2 0/6] grep: remove redundant code & reflags from API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:16Z","receivedAt":"2017-06-29T22:22:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresses comments from Stefan Beller (thanks!). I looked into it and\nthe REG_NEWLINE flag was redundant in 1/2 cases, see 6/6 for the\nremoval of that.\n\nI looked into refactoring 5/6 as noted in 87zicqirrg.fsf@gmail.com,\nbut for the reasons now explained in the last paragraph of 5/6 decided\nnot to and to keep it as it was.\n\nÆvar Arnfjörð Bjarmason (6):\n grep: remove redundant double assignment to 0\n grep: adjust a redundant grep pattern type assignment\n grep: remove redundant \"fixed\" field re-assignment to 0\n grep: remove redundant and verbose re-assignments to 0\n grep: remove regflags from the public grep_opt API\n grep: remove redundant REG_NEWLINE when compiling fixed regex\n\n builtin/grep.c | 2 --\n grep.c | 62 +++++++++++++++++++++++++++++++++-------------------------\n grep.h | 1 -\n revision.c | 2 --\n 4 files changed, 35 insertions(+), 32 deletions(-)\n\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323502","messageId":"20170629222222.4694-2-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 1/6] grep: remove redundant double assignment to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:17Z","receivedAt":"2017-06-29T22:22:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop assigning 0 to the extended_regexp_option field right after we've\nzeroed out the entire struct with memset() just a few lines earlier.\n\nUnlike some of the code being refactored in subsequent commits, this\nwas always completely redundant. See the original code introduced in\n84befcd0a4 (\"grep: add a grep.patternType configuration setting\",\n2012-08-03).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 98733db623..29439886e7 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -38,7 +38,6 @@ void init_grep_defaults(void)\n \topt->regflags = REG_NEWLINE;\n \topt->max_depth = -1;\n \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n-\topt->extended_regexp_option = 0;\n \tcolor_set(opt->color_context, \"\");\n \tcolor_set(opt->color_filename, \"\");\n \tcolor_set(opt->color_function, \"\");\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323503","messageId":"20170629222222.4694-3-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 2/6] grep: adjust a redundant grep pattern type assignment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:18Z","receivedAt":"2017-06-29T22:22:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust a now-redundant assignment to extended_regexp_option to make it\nzero if grep.extendedRegexp is not set. This is always called right\nafter init_grep_defaults() which memsets the entire structure to 0, so\nthere's no need to set it again to zero.\n\nHowever the reason for the if/else pattern is a holdover from[1] where\nthis was adjusted from a bitfield assignment to a boolean. Rather than\ngetting rid of the assignment to 0 in all cases, let's just use the\nvalue returned by git_config_bool(), which is more idiomatic and in\nsync with the rest of the boolean handling in this function.\n\nThis is a logical follow-up to my commit to remove redundant regflags\nassignments[2]. This logic was originally introduced in [3], but as\nexplained in the former commit it's working around a pattern in our\ncode that no longer exists, and is now confusing as it leads the\nreader to think that this needs to be flipped back & forth.\n\n1. 84befcd0a4 (\"grep: add a grep.patternType configuration setting\",\n 2012-08-03)\n2. e0b9f8ae09 (\"grep: remove redundant regflags assignments\",\n 2017-05-25)\n3. b22520a37c (\"grep: allow -E and -n to be turned on by default via\n configuration\", 2011-03-30)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 29439886e7..817270d081 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -78,10 +78,7 @@ int grep_config(const char *var, const char *value, void *cb)\n \t\treturn -1;\n \n \tif (!strcmp(var, \"grep.extendedregexp\")) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\topt->extended_regexp_option = 1;\n-\t\telse\n-\t\t\topt->extended_regexp_option = 0;\n+\t\topt->extended_regexp_option = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323504","messageId":"20170629222222.4694-4-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 3/6] grep: remove redundant \"fixed\" field re-assignment to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:19Z","receivedAt":"2017-06-29T22:22:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the redundant re-assignment of the fixed field to zero right\nafter the entire struct has been set to zero via memset(...).\n\nUnlike some nearby commits this pattern doesn't date back to the\npattern described in e0b9f8ae09 (\"grep: remove redundant regflags\nassignments\", 2017-05-25), instead it was apparently cargo-culted in\n9eceddeec6 (\"Use kwset in grep\", 2011-08-21).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 817270d081..86dc9b696f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -626,8 +626,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t has_null(p->pattern, p->patternlen) ||\n \t is_fixed(p->pattern, p->patternlen))\n \t\tp->fixed = !icase || ascii_only;\n-\telse\n-\t\tp->fixed = 0;\n \n \tif (p->fixed) {\n \t\tp->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323505","messageId":"20170629222222.4694-5-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 4/6] grep: remove redundant and verbose re-assignments to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:20Z","receivedAt":"2017-06-29T22:22:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to\nzero right after the entire struct has been set to zero via\nmemset(...).\n\nSee an earlier related cleanup commit e0b9f8ae09 (\"grep: remove\nredundant regflags assignments\", 2017-05-25) for an explanation of why\nthe code was structured like this to begin with.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 11 -----------\n 1 file changed, 11 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 86dc9b696f..7fcdaa0753 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -174,28 +174,18 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\t/* fall through */\n \n \tcase GREP_PATTERN_TYPE_BRE:\n-\t\topt->fixed = 0;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n-\t\topt->fixed = 0;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\topt->regflags |= REG_EXTENDED;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n \t\topt->fixed = 1;\n-\t\topt->pcre1 = 0;\n-\t\topt->pcre2 = 0;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_PCRE:\n-\t\topt->fixed = 0;\n #ifdef USE_LIBPCRE2\n-\t\topt->pcre1 = 0;\n \t\topt->pcre2 = 1;\n #else\n \t\t/*\n@@ -205,7 +195,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\t * \"cannot use Perl-compatible regexes[...]\".\n \t\t */\n \t\topt->pcre1 = 1;\n-\t\topt->pcre2 = 0;\n #endif\n \t\tbreak;\n \t}\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323506","messageId":"20170629222222.4694-7-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 6/6] grep: remove redundant REG_NEWLINE when compiling fixed regex","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:22Z","receivedAt":"2017-06-29T22:22:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the redundant REG_NEWLINE regcomp() flag from the code that\ncompiles a fixed-string regular-expression.\n\nThe REG_NEWLINE causes metacharacters such as \".\" to match a newline,\nsince the basic_regex_quote_buf() function being called here escapes\nall metacharacters using REG_NEWLINE is confusing and redundant.\n\nThe use of this flag was introduced as an unintended emergent property\nof 793dc676e0 (\"grep/icase: avoid kwsset when -F is specified\",\n2016-06-25).\n\nThat change amended the existing regflags, which were initialized to\nREG_NEWLINE in init_grep_defaults() assuming a subsequent non-fixed\nregcomp().\n\nManual testing reveals that this was always redundant, since no flags\nof any use were inherited from opt->regflags even back\nthen. 793dc676e0 passes all tests with this on top:\n\n diff --git a/grep.c b/grep.c\n index 627ae3e3e8..89e84ed7fd 100644\n --- a/grep.c\n +++ b/grep.c\n @@ -407,3 +407,3 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n basic_regex_quote_buf(&sb, p->pattern);\n - regflags = opt->regflags & ~REG_EXTENDED;\n + regflags = 0;\n if (opt->ignore_case)\n\nSince this isn't used for anything and never was, remove it to reduce\nconfusion when reading this code.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 11a86548d6..2efec0e182 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -593,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint err;\n-\tint regflags = REG_NEWLINE;\n+\tint regflags = 0;\n \n \tbasic_regex_quote_buf(&sb, p->pattern);\n \tif (opt->ignore_case)\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323507","messageId":"20170629222222.4694-6-avarab@gmail.com","threadId":"46282","inReplyTo":"20170629222222.4694-1-avarab@gmail.com","subject":"[PATCH v2 5/6] grep: remove regflags from the public grep_opt API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-29T22:22:21Z","receivedAt":"2017-06-29T22:22:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor calls to the grep machinery to always pass opt.ignore_case &\nopt.extended_regexp_option instead of setting the equivalent regflags\nbits.\n\nThe bug fixed when making -i work with -P in commit 9e3cbc59d5 (\"log:\nmake --regexp-ignore-case work with --perl-regexp\", 2017-05-20) was\nreally just plastering over the code smell which this change fixes.\n\nThe reason for adding the extensive commentary here is that I\ndiscovered some subtle complexity in implementing this that really\nshould be called out explicitly to future readers.\n\nBefore this change we'd rely on the difference between\n`extended_regexp_option` and `regflags` to serve as a membrane between\nour preliminary parsing of grep.extendedRegexp and grep.patternType,\nand what we decided to do internally.\n\nNow that those two are the same thing, it's necessary to unset\n`extended_regexp_option` just before we commit in cases where both of\nthose config variables are set. See 84befcd0a4 (\"grep: add a\ngrep.patternType configuration setting\", 2012-08-03) for the code and\ndocumentation related to that.\n\nThe explanation of why the if/else branches in\ngrep_commit_pattern_type() are ordered the way they are exists in that\ncommit message, but I think it's worth calling this subtlety out\nexplicitly with a comment for future readers.\n\nEven though grep_commit_pattern_type() is the only caller of\ngrep_set_pattern_type_option() it's simpler to reset the\nextended_regexp_option flag in the latter, since 2/3 branches in the\nformer would otherwise need to reset it, this way we can do it in one\nplace.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 2 --\n grep.c | 43 ++++++++++++++++++++++++++++++++++---------\n grep.h | 1 -\n revision.c | 2 --\n 4 files changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f61a9d938b..b682966439 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (!opt.pattern_list)\n \t\tdie(_(\"no pattern given.\"));\n-\tif (!opt.fixed && opt.ignore_case)\n-\t\topt.regflags |= REG_ICASE;\n \n \t/*\n \t * We have to find \"--\" in a separate pass, because its presence\ndiff --git a/grep.c b/grep.c\nindex 7fcdaa0753..11a86548d6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -35,7 +35,6 @@ void init_grep_defaults(void)\n \tmemset(opt, 0, sizeof(*opt));\n \topt->relative = 1;\n \topt->pathname = 1;\n-\topt->regflags = REG_NEWLINE;\n \topt->max_depth = -1;\n \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tcolor_set(opt->color_context, \"\");\n@@ -153,7 +152,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \topt->linenum = def->linenum;\n \topt->max_depth = def->max_depth;\n \topt->pathname = def->pathname;\n-\topt->regflags = def->regflags;\n \topt->relative = def->relative;\n \topt->output = def->output;\n \n@@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n+\t/*\n+\t * When committing to the pattern type by setting the relevant\n+\t * fields in grep_opt it's generally not necessary to zero out\n+\t * the fields we're not choosing, since they won't have been\n+\t * set by anything. The extended_regexp_option field is the\n+\t * only exception to this.\n+\t *\n+\t * This is because in the process of parsing grep.patternType\n+\t * & grep.extendedRegexp we set opt->pattern_type_option and\n+\t * opt->extended_regexp_option, respectively. We then\n+\t * internally use opt->extended_regexp_option to see if we're\n+\t * compiling an ERE. It must be unset if that's not actually\n+\t * the case.\n+\t */\n+\tif (pattern_type != GREP_PATTERN_TYPE_ERE &&\n+\t opt->extended_regexp_option)\n+\t\topt->extended_regexp_option = 0;\n+\n \tswitch (pattern_type) {\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n \t\t/* fall through */\n@@ -177,7 +193,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_ERE:\n-\t\topt->regflags |= REG_EXTENDED;\n+\t\topt->extended_regexp_option = 1;\n \t\tbreak;\n \n \tcase GREP_PATTERN_TYPE_FIXED:\n@@ -207,6 +223,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n \telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n \t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n \telse if (opt->extended_regexp_option)\n+\t\t/*\n+\t\t * This branch *must* happen after setting from the\n+\t\t * opt->pattern_type_option above, we don't want\n+\t\t * grep.extendedRegexp to override grep.patternType!\n+\t\t */\n \t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n }\n \n@@ -572,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint err;\n-\tint regflags = opt->regflags;\n+\tint regflags = REG_NEWLINE;\n \n \tbasic_regex_quote_buf(&sb, p->pattern);\n \tif (opt->ignore_case)\n@@ -591,12 +612,12 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n \n static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n-\tint icase, ascii_only;\n+\tint ascii_only;\n \tint err;\n+\tint regflags = REG_NEWLINE;\n \n \tp->word_regexp = opt->word_regexp;\n \tp->ignore_case = opt->ignore_case;\n-\ticase\t = opt->regflags & REG_ICASE || p->ignore_case;\n \tascii_only = !has_non_ascii(p->pattern);\n \n \t/*\n@@ -614,10 +635,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tif (opt->fixed ||\n \t has_null(p->pattern, p->patternlen) ||\n \t is_fixed(p->pattern, p->patternlen))\n-\t\tp->fixed = !icase || ascii_only;\n+\t\tp->fixed = !p->ignore_case || ascii_only;\n \n \tif (p->fixed) {\n-\t\tp->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);\n+\t\tp->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);\n \t\tkwsincr(p->kws, p->pattern, p->patternlen);\n \t\tkwsprep(p->kws);\n \t\treturn;\n@@ -641,7 +662,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t\treturn;\n \t}\n \n-\terr = regcomp(&p->regexp, p->pattern, opt->regflags);\n+\tif (p->ignore_case)\n+\t\tregflags |= REG_ICASE;\n+\tif (opt->extended_regexp_option)\n+\t\tregflags |= REG_EXTENDED;\n+\terr = regcomp(&p->regexp, p->pattern, regflags);\n \tif (err) {\n \t\tchar errbuf[1024];\n \t\tregerror(err, &p->regexp, errbuf, 1024);\ndiff --git a/grep.h b/grep.h\nindex b8f93bfc2d..0c091e5104 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -162,7 +162,6 @@ struct grep_opt {\n \tchar color_match_selected[COLOR_MAXLEN];\n \tchar color_selected[COLOR_MAXLEN];\n \tchar color_sep[COLOR_MAXLEN];\n-\tint regflags;\n \tunsigned pre_context;\n \tunsigned post_context;\n \tunsigned last_shown;\ndiff --git a/revision.c b/revision.c\nindex e181ad1b70..207103d211 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, prefix);\n \trevs->grep_filter.status_only = 1;\n-\trevs->grep_filter.regflags = REG_NEWLINE;\n \n \tdiff_setup(&revs->diffopt);\n \tif (prefix && !revs->diffopt.prefix) {\n@@ -2022,7 +2021,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n \t} else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n \t\trevs->grep_filter.ignore_case = 1;\n-\t\trevs->grep_filter.regflags |= REG_ICASE;\n \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n \t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n-- \n2.13.1.611.g7e3b11ae1\n\n"},{"id":"323577","messageId":"xmqqwp7ts88k.fsf@gitster.mtv.corp.google.com","threadId":"46282","inReplyTo":"20170629222222.4694-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/6] grep: remove regflags from the public grep_opt API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-30T17:20:11Z","receivedAt":"2017-06-30T17:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n> \n> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n> {\n> +\t/*\n> +\t * When committing to the pattern type by setting the relevant\n> +\t * fields in grep_opt it's generally not necessary to zero out\n> +\t * the fields we're not choosing, since they won't have been\n> +\t * set by anything. The extended_regexp_option field is the\n> +\t * only exception to this.\n> +\t *\n> +\t * This is because in the process of parsing grep.patternType\n> +\t * & grep.extendedRegexp we set opt->pattern_type_option and\n> +\t * opt->extended_regexp_option, respectively. We then\n> +\t * internally use opt->extended_regexp_option to see if we're\n> +\t * compiling an ERE. It must be unset if that's not actually\n> +\t * the case.\n> +\t */\n> +\tif (pattern_type != GREP_PATTERN_TYPE_ERE &&\n> +\t opt->extended_regexp_option)\n> +\t\topt->extended_regexp_option = 0;\n\nGood to have the reasoning in an in-code comment like the above.\nBut after reading these two paragraphs and then before reading the\nthree line code, a more natural embodiment in the code of the\ncommentary that came to my mind was\n\n\tif (pattern_type != GREP_PATTERN_TYPE_ERE)\n\t\topt->extended_regexp_option = 0;\n\nThe end-result is the same as yours, of course, but I somehow found\nit match the reasoning better.\n\nNow, I wonder if this can further be tweaked to\n\n\topt->extended_regexp_option = (pattern_type == GREP_PATTERN_TYPE_ERE);\n\nwhich might lead us in a direction to really unify the two related\nfields extended_regexp_option and pattern_type_option.\n\nEven if that were a good longer term direction to go in, it is\noutside the scope of this step, of course. I am merely bringing it\nup as an conversation item ;-).\n\n"}]}