# [PATCH 1/5] grep: remove redundant double assignment to 0

19 messages from 2017-06-28 to 2017-06-30. Participants: Ævar Arnfjörð Bjarmason, Stefan Beller, Junio C Hamano.
Thread: https://gitlist.dev/t/46282

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 1/5] grep: remove redundant double assignment to 0
Message-ID: <20170628215809.23060-2-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-2-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/grep.c b/grep.c
index 98733db623..29439886e7 100644
--- a/grep.c
+++ b/grep.c
@@ -38,7 +38,6 @@ void init_grep_defaults(void)
 	opt->regflags = REG_NEWLINE;
 	opt->max_depth = -1;
 	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-	opt->extended_regexp_option = 0;
 	color_set(opt->color_context, "");
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 0/5] grep: remove redundant code & reflags from API
Message-ID: <20170628215809.23060-1-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-1-avarab%40gmail.com
In-Reply-To: <CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com>

```
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


```

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 3/5] grep: remove redundant "fixed" field re-assignment to 0
Message-ID: <20170628215809.23060-4-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-4-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/grep.c b/grep.c
index 6614042fdc..7cd8a6512f 100644
--- a/grep.c
+++ b/grep.c
@@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	    has_null(p->pattern, p->patternlen) ||
 	    is_fixed(p->pattern, p->patternlen))
 		p->fixed = !icase || ascii_only;
-	else
-		p->fixed = 0;
 
 	if (p->fixed) {
 		p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 2/5] grep: remove redundant grep pattern type assignment
Message-ID: <20170628215809.23060-3-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-3-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/grep.c b/grep.c
index 29439886e7..6614042fdc 100644
--- a/grep.c
+++ b/grep.c
@@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "grep.extendedregexp")) {
 		if (git_config_bool(var, value))
 			opt->extended_regexp_option = 1;
-		else
-			opt->extended_regexp_option = 0;
 		return 0;
 	}
 
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 4/5] grep: remove redundant and verbose re-assignments to 0
Message-ID: <20170628215809.23060-5-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-5-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/grep.c b/grep.c
index 7cd8a6512f..736e1e00d6 100644
--- a/grep.c
+++ b/grep.c
@@ -175,28 +175,18 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		/* fall through */
 
 	case GREP_PATTERN_TYPE_BRE:
-		opt->fixed = 0;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_ERE:
-		opt->fixed = 0;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		opt->regflags |= REG_EXTENDED;
 		break;
 
 	case GREP_PATTERN_TYPE_FIXED:
 		opt->fixed = 1;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_PCRE:
-		opt->fixed = 0;
 #ifdef USE_LIBPCRE2
-		opt->pcre1 = 0;
 		opt->pcre2 = 1;
 #else
 		/*
@@ -206,7 +196,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		 * "cannot use Perl-compatible regexes[...]".
 		 */
 		opt->pcre1 = 1;
-		opt->pcre2 = 0;
 #endif
 		break;
 	}
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-28 21:58

Subject: [PATCH 5/5] grep: remove regflags from the public grep_opt API
Message-ID: <20170628215809.23060-6-avarab@gmail.com>
URL: https://gitlist.dev/e/20170628215809.23060-6-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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. <CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com>
   (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  2 --
 grep.c         | 43 ++++++++++++++++++++++++++++++++++---------
 grep.h         |  1 -
 revision.c     |  2 --
 4 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f61a9d938b..b682966439 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (!opt.pattern_list)
 		die(_("no pattern given."));
-	if (!opt.fixed && opt.ignore_case)
-		opt.regflags |= REG_ICASE;
 
 	/*
 	 * We have to find "--" in a separate pass, because its presence
diff --git a/grep.c b/grep.c
index 736e1e00d6..51aaad9f03 100644
--- a/grep.c
+++ b/grep.c
@@ -35,7 +35,6 @@ void init_grep_defaults(void)
 	memset(opt, 0, sizeof(*opt));
 	opt->relative = 1;
 	opt->pathname = 1;
-	opt->regflags = REG_NEWLINE;
 	opt->max_depth = -1;
 	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
 	color_set(opt->color_context, "");
@@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->linenum = def->linenum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
-	opt->regflags = def->regflags;
 	opt->relative = def->relative;
 	opt->output = def->output;
 
@@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
+	/*
+	 * When committing to the pattern type by setting the relevant
+	 * fields in grep_opt it's generally not necessary to zero out
+	 * the fields we're not choosing, since they won't have been
+	 * set by anything. The extended_regexp_option field is the
+	 * only exception to this.
+	 *
+	 * This is because in the process of parsing grep.patternType
+	 * & grep.extendedRegexp we set opt->pattern_type_option and
+	 * opt->extended_regexp_option, respectively. We then
+	 * internally use opt->extended_regexp_option to see if we're
+	 * compiling an ERE. It must be unset if that's not actually
+	 * the case.
+	 */
+	if (pattern_type != GREP_PATTERN_TYPE_ERE &&
+	    opt->extended_regexp_option)
+		opt->extended_regexp_option = 0;
+
 	switch (pattern_type) {
 	case GREP_PATTERN_TYPE_UNSPECIFIED:
 		/* fall through */
@@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		break;
 
 	case GREP_PATTERN_TYPE_ERE:
-		opt->regflags |= REG_EXTENDED;
+		opt->extended_regexp_option = 1;
 		break;
 
 	case GREP_PATTERN_TYPE_FIXED:
@@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
 	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)
+		/*
+		 * This branch *must* happen after setting from the
+		 * opt->pattern_type_option above, we don't want
+		 * grep.extendedRegexp to override grep.patternType!
+		 */
 		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
 }
 
@@ -573,7 +594,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int err;
-	int regflags = opt->regflags;
+	int regflags = REG_NEWLINE;
 
 	basic_regex_quote_buf(&sb, p->pattern);
 	if (opt->ignore_case)
@@ -592,12 +613,12 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
-	int icase, ascii_only;
+	int ascii_only;
 	int err;
+	int regflags = REG_NEWLINE;
 
 	p->word_regexp = opt->word_regexp;
 	p->ignore_case = opt->ignore_case;
-	icase	       = opt->regflags & REG_ICASE || p->ignore_case;
 	ascii_only     = !has_non_ascii(p->pattern);
 
 	/*
@@ -615,10 +636,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	if (opt->fixed ||
 	    has_null(p->pattern, p->patternlen) ||
 	    is_fixed(p->pattern, p->patternlen))
-		p->fixed = !icase || ascii_only;
+		p->fixed = !p->ignore_case || ascii_only;
 
 	if (p->fixed) {
-		p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
+		p->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);
 		kwsincr(p->kws, p->pattern, p->patternlen);
 		kwsprep(p->kws);
 		return;
@@ -642,7 +663,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 		return;
 	}
 
-	err = regcomp(&p->regexp, p->pattern, opt->regflags);
+	if (p->ignore_case)
+		regflags |= REG_ICASE;
+	if (opt->extended_regexp_option)
+		regflags |= REG_EXTENDED;
+	err = regcomp(&p->regexp, p->pattern, regflags);
 	if (err) {
 		char errbuf[1024];
 		regerror(err, &p->regexp, errbuf, 1024);
diff --git a/grep.h b/grep.h
index b8f93bfc2d..0c091e5104 100644
--- a/grep.h
+++ b/grep.h
@@ -162,7 +162,6 @@ struct grep_opt {
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
 	char color_sep[COLOR_MAXLEN];
-	int regflags;
 	unsigned pre_context;
 	unsigned post_context;
 	unsigned last_shown;
diff --git a/revision.c b/revision.c
index e181ad1b70..207103d211 100644
--- a/revision.c
+++ b/revision.c
@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
 	revs->grep_filter.status_only = 1;
-	revs->grep_filter.regflags = REG_NEWLINE;
 
 	diff_setup(&revs->diffopt);
 	if (prefix && !revs->diffopt.prefix) {
@@ -2022,7 +2021,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;
-- 
2.13.1.611.g7e3b11ae1


```

## Stefan Beller, 2017-06-29 17:03

Subject: Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment
Message-ID: <CAGZ79kb0a2EEALj3eEAMUwQSWamhGuOFOYPrWbv6uCtC7EXgAw@mail.gmail.com>
URL: https://gitlist.dev/e/CAGZ79kb0a2EEALj3eEAMUwQSWamhGuOFOYPrWbv6uCtC7EXgAw%40mail.gmail.com
In-Reply-To: <20170628215809.23060-3-avarab@gmail.com>

```
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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 <avarab@gmail.com>
> ---
>  grep.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 29439886e7..6614042fdc 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)
>         if (!strcmp(var, "grep.extendedregexp")) {
>                 if (git_config_bool(var, value))
>                         opt->extended_regexp_option = 1;
> -               else
> -                       opt->extended_regexp_option = 0;
>                 return 0;

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

```

## Stefan Beller, 2017-06-29 17:10

Subject: Re: [PATCH 3/5] grep: remove redundant "fixed" field re-assignment to 0
Message-ID: <CAGZ79kaLEA+6DDgWnqw-hg1GrWVD7CgGNAkmXx_iQmeKGSJUqQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAGZ79kaLEA%2B6DDgWnqw-hg1GrWVD7CgGNAkmXx_iQmeKGSJUqQ%40mail.gmail.com
In-Reply-To: <20170628215809.23060-4-avarab@gmail.com>

```
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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 <avarab@gmail.com>
> ---
>  grep.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 6614042fdc..7cd8a6512f 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>             has_null(p->pattern, p->patternlen) ||
>             is_fixed(p->pattern, p->patternlen))
>                 p->fixed = !icase || ascii_only;
> -       else
> -               p->fixed = 0;
>

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

```

## Stefan Beller, 2017-06-29 17:43

Subject: Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API
Message-ID: <CAGZ79kbOpMpi0Dv6=ViW45gq5E9KHgpz4GE4o8XA-KMaiR78Vw@mail.gmail.com>
URL: https://gitlist.dev/e/CAGZ79kbOpMpi0Dv6%3DViW45gq5E9KHgpz4GE4o8XA-KMaiR78Vw%40mail.gmail.com
In-Reply-To: <20170628215809.23060-6-avarab@gmail.com>

```
On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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.

This sounds as if extra careful review is needed.


>
> 1. <CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com>
>    (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/grep.c |  2 --
>  grep.c         | 43 ++++++++++++++++++++++++++++++++++---------
>  grep.h         |  1 -
>  revision.c     |  2 --
>  4 files changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index f61a9d938b..b682966439 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>
>         if (!opt.pattern_list)
>                 die(_("no pattern given."));
> -       if (!opt.fixed && opt.ignore_case)
> -               opt.regflags |= REG_ICASE;
>
>         /*
>          * We have to find "--" in a separate pass, because its presence
> diff --git a/grep.c b/grep.c
> index 736e1e00d6..51aaad9f03 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -35,7 +35,6 @@ void init_grep_defaults(void)
>         memset(opt, 0, sizeof(*opt));
>         opt->relative = 1;
>         opt->pathname = 1;
> -       opt->regflags = REG_NEWLINE;
>         opt->max_depth = -1;
>         opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
>         color_set(opt->color_context, "");
> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>         opt->linenum = def->linenum;
>         opt->max_depth = def->max_depth;
>         opt->pathname = def->pathname;
> -       opt->regflags = def->regflags;
>         opt->relative = def->relative;
>         opt->output = def->output;
>
> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>
>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>  {
> +       /*
> +        * When committing to the pattern type by setting the relevant
> +        * fields in grep_opt it's generally not necessary to zero out
> +        * the fields we're not choosing, since they won't have been
> +        * set by anything. The extended_regexp_option field is the
> +        * only exception to this.
> +        *
> +        * This is because in the process of parsing grep.patternType
> +        * & grep.extendedRegexp we set opt->pattern_type_option and
> +        * opt->extended_regexp_option, respectively. We then
> +        * internally use opt->extended_regexp_option to see if we're
> +        * compiling an ERE. It must be unset if that's not actually
> +        * the case.
> +        */
> +       if (pattern_type != GREP_PATTERN_TYPE_ERE &&
> +           opt->extended_regexp_option)
> +               opt->extended_regexp_option = 0;
> +
>         switch (pattern_type) {
>         case GREP_PATTERN_TYPE_UNSPECIFIED:
>                 /* fall through */
> @@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
>                 break;
>
>         case GREP_PATTERN_TYPE_ERE:
> -               opt->regflags |= REG_EXTENDED;
> +               opt->extended_regexp_option = 1;
>                 break;
>
>         case GREP_PATTERN_TYPE_FIXED:
> @@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
>         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)
> +               /*
> +                * This branch *must* happen after setting from the
> +                * opt->pattern_type_option above,

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

```

## Ævar Arnfjörð Bjarmason, 2017-06-29 17:50

Subject: Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment
Message-ID: <871sq2k7jp.fsf@gmail.com>
URL: https://gitlist.dev/e/871sq2k7jp.fsf%40gmail.com
In-Reply-To: <CAGZ79kb0a2EEALj3eEAMUwQSWamhGuOFOYPrWbv6uCtC7EXgAw@mail.gmail.com>

```

On Thu, Jun 29 2017, Stefan Beller jotted:

> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> 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 <avarab@gmail.com>
>> ---
>>  grep.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/grep.c b/grep.c
>> index 29439886e7..6614042fdc 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)
>>         if (!strcmp(var, "grep.extendedregexp")) {
>>                 if (git_config_bool(var, value))
>>                         opt->extended_regexp_option = 1;
>> -               else
>> -                       opt->extended_regexp_option = 0;
>>                 return 0;
>
> 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.

```

## Ævar Arnfjörð Bjarmason, 2017-06-29 18:16

Subject: Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API
Message-ID: <87zicqirrg.fsf@gmail.com>
URL: https://gitlist.dev/e/87zicqirrg.fsf%40gmail.com
In-Reply-To: <CAGZ79kbOpMpi0Dv6=ViW45gq5E9KHgpz4GE4o8XA-KMaiR78Vw@mail.gmail.com>

```

On Thu, Jun 29 2017, Stefan Beller jotted:

> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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.

Thanks.

>>
>> 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.

>>
>> 1. <CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com>
>>    (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1KO2g@mail.gmail.com/)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/grep.c |  2 --
>>  grep.c         | 43 ++++++++++++++++++++++++++++++++++---------
>>  grep.h         |  1 -
>>  revision.c     |  2 --
>>  4 files changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index f61a9d938b..b682966439 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>
>>         if (!opt.pattern_list)
>>                 die(_("no pattern given."));
>> -       if (!opt.fixed && opt.ignore_case)
>> -               opt.regflags |= REG_ICASE;
>>
>>         /*
>>          * We have to find "--" in a separate pass, because its presence
>> diff --git a/grep.c b/grep.c
>> index 736e1e00d6..51aaad9f03 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -35,7 +35,6 @@ void init_grep_defaults(void)
>>         memset(opt, 0, sizeof(*opt));
>>         opt->relative = 1;
>>         opt->pathname = 1;
>> -       opt->regflags = REG_NEWLINE;
>>         opt->max_depth = -1;
>>         opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
>>         color_set(opt->color_context, "");
>> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>>         opt->linenum = def->linenum;
>>         opt->max_depth = def->max_depth;
>>         opt->pathname = def->pathname;
>> -       opt->regflags = def->regflags;
>>         opt->relative = def->relative;
>>         opt->output = def->output;
>>
>> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>>
>>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>>  {
>> +       /*
>> +        * When committing to the pattern type by setting the relevant
>> +        * fields in grep_opt it's generally not necessary to zero out
>> +        * the fields we're not choosing, since they won't have been
>> +        * set by anything. The extended_regexp_option field is the
>> +        * only exception to this.
>> +        *
>> +        * This is because in the process of parsing grep.patternType
>> +        * & grep.extendedRegexp we set opt->pattern_type_option and
>> +        * opt->extended_regexp_option, respectively. We then
>> +        * internally use opt->extended_regexp_option to see if we're
>> +        * compiling an ERE. It must be unset if that's not actually
>> +        * the case.
>> +        */
>> +       if (pattern_type != GREP_PATTERN_TYPE_ERE &&
>> +           opt->extended_regexp_option)
>> +               opt->extended_regexp_option = 0;
>> +
>>         switch (pattern_type) {
>>         case GREP_PATTERN_TYPE_UNSPECIFIED:
>>                 /* fall through */
>> @@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
>>                 break;
>>
>>         case GREP_PATTERN_TYPE_ERE:
>> -               opt->regflags |= REG_EXTENDED;
>> +               opt->extended_regexp_option = 1;
>>                 break;
>>
>>         case GREP_PATTERN_TYPE_FIXED:
>> @@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
>>         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)
>> +               /*
>> +                * This branch *must* happen after setting from the
>> +                * opt->pattern_type_option above,
>
> 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. ;)

*Points up at "moving it around" comment above*

```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 0/6] grep: remove redundant code & reflags from API
Message-ID: <20170629222222.4694-1-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-1-avarab%40gmail.com
In-Reply-To: <20170628215809.23060-1-avarab@gmail.com>

```
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


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 1/6] grep: remove redundant double assignment to 0
Message-ID: <20170629222222.4694-2-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-2-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/grep.c b/grep.c
index 98733db623..29439886e7 100644
--- a/grep.c
+++ b/grep.c
@@ -38,7 +38,6 @@ void init_grep_defaults(void)
 	opt->regflags = REG_NEWLINE;
 	opt->max_depth = -1;
 	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-	opt->extended_regexp_option = 0;
 	color_set(opt->color_context, "");
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 2/6] grep: adjust a redundant grep pattern type assignment
Message-ID: <20170629222222.4694-3-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-3-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 29439886e7..817270d081 100644
--- a/grep.c
+++ b/grep.c
@@ -78,10 +78,7 @@ int grep_config(const char *var, const char *value, void *cb)
 		return -1;
 
 	if (!strcmp(var, "grep.extendedregexp")) {
-		if (git_config_bool(var, value))
-			opt->extended_regexp_option = 1;
-		else
-			opt->extended_regexp_option = 0;
+		opt->extended_regexp_option = git_config_bool(var, value);
 		return 0;
 	}
 
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 3/6] grep: remove redundant "fixed" field re-assignment to 0
Message-ID: <20170629222222.4694-4-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-4-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/grep.c b/grep.c
index 817270d081..86dc9b696f 100644
--- a/grep.c
+++ b/grep.c
@@ -626,8 +626,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	    has_null(p->pattern, p->patternlen) ||
 	    is_fixed(p->pattern, p->patternlen))
 		p->fixed = !icase || ascii_only;
-	else
-		p->fixed = 0;
 
 	if (p->fixed) {
 		p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 4/6] grep: remove redundant and verbose re-assignments to 0
Message-ID: <20170629222222.4694-5-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-5-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/grep.c b/grep.c
index 86dc9b696f..7fcdaa0753 100644
--- a/grep.c
+++ b/grep.c
@@ -174,28 +174,18 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		/* fall through */
 
 	case GREP_PATTERN_TYPE_BRE:
-		opt->fixed = 0;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_ERE:
-		opt->fixed = 0;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		opt->regflags |= REG_EXTENDED;
 		break;
 
 	case GREP_PATTERN_TYPE_FIXED:
 		opt->fixed = 1;
-		opt->pcre1 = 0;
-		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_PCRE:
-		opt->fixed = 0;
 #ifdef USE_LIBPCRE2
-		opt->pcre1 = 0;
 		opt->pcre2 = 1;
 #else
 		/*
@@ -205,7 +195,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		 * "cannot use Perl-compatible regexes[...]".
 		 */
 		opt->pcre1 = 1;
-		opt->pcre2 = 0;
 #endif
 		break;
 	}
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 6/6] grep: remove redundant REG_NEWLINE when compiling fixed regex
Message-ID: <20170629222222.4694-7-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-7-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 11a86548d6..2efec0e182 100644
--- a/grep.c
+++ b/grep.c
@@ -593,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int err;
-	int regflags = REG_NEWLINE;
+	int regflags = 0;
 
 	basic_regex_quote_buf(&sb, p->pattern);
 	if (opt->ignore_case)
-- 
2.13.1.611.g7e3b11ae1


```

## Ævar Arnfjörð Bjarmason, 2017-06-29 22:22

Subject: [PATCH v2 5/6] grep: remove regflags from the public grep_opt API
Message-ID: <20170629222222.4694-6-avarab@gmail.com>
URL: https://gitlist.dev/e/20170629222222.4694-6-avarab%40gmail.com
In-Reply-To: <20170629222222.4694-1-avarab@gmail.com>

```
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 <avarab@gmail.com>
---
 builtin/grep.c |  2 --
 grep.c         | 43 ++++++++++++++++++++++++++++++++++---------
 grep.h         |  1 -
 revision.c     |  2 --
 4 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f61a9d938b..b682966439 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (!opt.pattern_list)
 		die(_("no pattern given."));
-	if (!opt.fixed && opt.ignore_case)
-		opt.regflags |= REG_ICASE;
 
 	/*
 	 * We have to find "--" in a separate pass, because its presence
diff --git a/grep.c b/grep.c
index 7fcdaa0753..11a86548d6 100644
--- a/grep.c
+++ b/grep.c
@@ -35,7 +35,6 @@ void init_grep_defaults(void)
 	memset(opt, 0, sizeof(*opt));
 	opt->relative = 1;
 	opt->pathname = 1;
-	opt->regflags = REG_NEWLINE;
 	opt->max_depth = -1;
 	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
 	color_set(opt->color_context, "");
@@ -153,7 +152,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->linenum = def->linenum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
-	opt->regflags = def->regflags;
 	opt->relative = def->relative;
 	opt->output = def->output;
 
@@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
+	/*
+	 * When committing to the pattern type by setting the relevant
+	 * fields in grep_opt it's generally not necessary to zero out
+	 * the fields we're not choosing, since they won't have been
+	 * set by anything. The extended_regexp_option field is the
+	 * only exception to this.
+	 *
+	 * This is because in the process of parsing grep.patternType
+	 * & grep.extendedRegexp we set opt->pattern_type_option and
+	 * opt->extended_regexp_option, respectively. We then
+	 * internally use opt->extended_regexp_option to see if we're
+	 * compiling an ERE. It must be unset if that's not actually
+	 * the case.
+	 */
+	if (pattern_type != GREP_PATTERN_TYPE_ERE &&
+	    opt->extended_regexp_option)
+		opt->extended_regexp_option = 0;
+
 	switch (pattern_type) {
 	case GREP_PATTERN_TYPE_UNSPECIFIED:
 		/* fall through */
@@ -177,7 +193,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 		break;
 
 	case GREP_PATTERN_TYPE_ERE:
-		opt->regflags |= REG_EXTENDED;
+		opt->extended_regexp_option = 1;
 		break;
 
 	case GREP_PATTERN_TYPE_FIXED:
@@ -207,6 +223,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
 	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)
+		/*
+		 * This branch *must* happen after setting from the
+		 * opt->pattern_type_option above, we don't want
+		 * grep.extendedRegexp to override grep.patternType!
+		 */
 		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
 }
 
@@ -572,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int err;
-	int regflags = opt->regflags;
+	int regflags = REG_NEWLINE;
 
 	basic_regex_quote_buf(&sb, p->pattern);
 	if (opt->ignore_case)
@@ -591,12 +612,12 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
-	int icase, ascii_only;
+	int ascii_only;
 	int err;
+	int regflags = REG_NEWLINE;
 
 	p->word_regexp = opt->word_regexp;
 	p->ignore_case = opt->ignore_case;
-	icase	       = opt->regflags & REG_ICASE || p->ignore_case;
 	ascii_only     = !has_non_ascii(p->pattern);
 
 	/*
@@ -614,10 +635,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	if (opt->fixed ||
 	    has_null(p->pattern, p->patternlen) ||
 	    is_fixed(p->pattern, p->patternlen))
-		p->fixed = !icase || ascii_only;
+		p->fixed = !p->ignore_case || ascii_only;
 
 	if (p->fixed) {
-		p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
+		p->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);
 		kwsincr(p->kws, p->pattern, p->patternlen);
 		kwsprep(p->kws);
 		return;
@@ -641,7 +662,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 		return;
 	}
 
-	err = regcomp(&p->regexp, p->pattern, opt->regflags);
+	if (p->ignore_case)
+		regflags |= REG_ICASE;
+	if (opt->extended_regexp_option)
+		regflags |= REG_EXTENDED;
+	err = regcomp(&p->regexp, p->pattern, regflags);
 	if (err) {
 		char errbuf[1024];
 		regerror(err, &p->regexp, errbuf, 1024);
diff --git a/grep.h b/grep.h
index b8f93bfc2d..0c091e5104 100644
--- a/grep.h
+++ b/grep.h
@@ -162,7 +162,6 @@ struct grep_opt {
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
 	char color_sep[COLOR_MAXLEN];
-	int regflags;
 	unsigned pre_context;
 	unsigned post_context;
 	unsigned last_shown;
diff --git a/revision.c b/revision.c
index e181ad1b70..207103d211 100644
--- a/revision.c
+++ b/revision.c
@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
 	revs->grep_filter.status_only = 1;
-	revs->grep_filter.regflags = REG_NEWLINE;
 
 	diff_setup(&revs->diffopt);
 	if (prefix && !revs->diffopt.prefix) {
@@ -2022,7 +2021,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;
-- 
2.13.1.611.g7e3b11ae1


```

## Junio C Hamano, 2017-06-30 17:20

Subject: Re: [PATCH v2 5/6] grep: remove regflags from the public grep_opt API
Message-ID: <xmqqwp7ts88k.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqwp7ts88k.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20170629222222.4694-6-avarab@gmail.com>

```
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>  
>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>  {
> +	/*
> +	 * When committing to the pattern type by setting the relevant
> +	 * fields in grep_opt it's generally not necessary to zero out
> +	 * the fields we're not choosing, since they won't have been
> +	 * set by anything. The extended_regexp_option field is the
> +	 * only exception to this.
> +	 *
> +	 * This is because in the process of parsing grep.patternType
> +	 * & grep.extendedRegexp we set opt->pattern_type_option and
> +	 * opt->extended_regexp_option, respectively. We then
> +	 * internally use opt->extended_regexp_option to see if we're
> +	 * compiling an ERE. It must be unset if that's not actually
> +	 * the case.
> +	 */
> +	if (pattern_type != GREP_PATTERN_TYPE_ERE &&
> +	    opt->extended_regexp_option)
> +		opt->extended_regexp_option = 0;

Good to have the reasoning in an in-code comment like the above.
But after reading these two paragraphs and then before reading the
three line code, a more natural embodiment in the code of the
commentary that came to my mind was

	if (pattern_type != GREP_PATTERN_TYPE_ERE)
		opt->extended_regexp_option = 0;

The end-result is the same as yours, of course, but I somehow found
it match the reasoning better.

Now, I wonder if this can further be tweaked to

	opt->extended_regexp_option = (pattern_type == GREP_PATTERN_TYPE_ERE);

which might lead us in a direction to really unify the two related
fields extended_regexp_option and pattern_type_option.

Even if that were a good longer term direction to go in, it is
outside the scope of this step, of course.  I am merely bringing it
up as an conversation item ;-).


```