[PATCH/RFC] grep: add a grep.patternType configuration setting

[PATCH/RFC] grep: add a grep.patternType configuration setting

[J Smith]

Adds the grep.patternType configuration setting which sets the default
pattern matching behavior. The values "basic", "extended", "fixed", and
"perl" can be used to set "--basic-regexp", "--extended-regexp",
"--fixed-strings", and "--perl-regexp" options by default respectively.

A value of true is equivalent to "extended" as with grep.extendedRegexp,
and a value of false leaves the pattern type as unspecified and follows
the default grep behavior.

This setting overrides the value set in grep.extendedRegexp which will
be ignored completely if grep.patternType is set.
---
 Documentation/config.txt   |  11 ++-
 Documentation/git-grep.txt |  11 ++-
 builtin/grep.c             | 106 ++++++++++++++++---------
 grep.h                     |   9 +++
 t/t7810-grep.sh            | 187 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 284 insertions(+), 40 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a95e5a4..38d56d8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1210,8 +1210,17 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable '-n' option by default.

+grep.patternType::
+	Sets the default matching behavior. This option can be set to a
+	boolean value or one of 'basic', 'extended', 'fixed', or 'perl'
+	which will enable the '--basic-regexp', '--extended-regexp',
+	'--fixed-strings' or '--perl-regexp' options accordingly. The value
+	of true is equivalent to 'extended' while false leaves the
+	settings in their default state.
+
 grep.extendedRegexp::
-	If set to true, enable '--extended-regexp' option by default.
+	If set to true, enable '--extended-regexp' option by default. This
+	option is ignored when the 'grep.patternType' option is set.

 gpg.program::
 	Use this custom program instead of "gpg" found on $PATH when
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 3bec036..f56f67f 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -42,8 +42,17 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable '-n' option by default.

+grep.patternType::
+	Sets the default matching behavior. This option can be set to a
+	boolean value or one of 'basic', 'extended', 'fixed', or 'perl'
+	which will enable the '--basic-regexp', '--extended-regexp',
+	'--fixed-strings' or '--perl-regexp' options accordingly. The value
+	of true is equivalent to 'extended' while false leaves the
+	settings in their default state.
+
 grep.extendedRegexp::
-	If set to true, enable '--extended-regexp' option by default.
+	If set to true, enable '--extended-regexp' option by default. This
+	option is ignored when the 'grep.patternType' option is set.


 OPTIONS
diff --git a/builtin/grep.c b/builtin/grep.c
index 29adb0a..1de7e76 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -260,6 +260,55 @@ static int wait_all(void)
 }
 #endif

+static int parse_pattern_type_arg(const char *opt, const char *arg)
+{
+	switch (git_config_maybe_bool(opt, arg)) {
+	case 1:
+		return GREP_PATTERN_TYPE_ERE;
+	case 0:
+		return GREP_PATTERN_TYPE_UNSPECIFIED;
+	default:
+		if (!strcmp(arg, "basic"))
+			return GREP_PATTERN_TYPE_BRE;
+		else if (!strcmp(arg, "extended"))
+			return GREP_PATTERN_TYPE_ERE;
+		else if (!strcmp(arg, "fixed"))
+			return GREP_PATTERN_TYPE_FIXED;
+		else if (!strcmp(arg, "perl"))
+			return GREP_PATTERN_TYPE_PCRE;
+		die("bad %s argument: %s", opt, arg);
+	}
+}
+
+static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
+{
+	switch (pattern_type) {
+		case GREP_PATTERN_TYPE_BRE:
+			opt->fixed = 0;
+			opt->pcre = 0;
+			opt->regflags &= ~REG_EXTENDED;
+			break;
+
+		case GREP_PATTERN_TYPE_ERE:
+			opt->fixed = 0;
+			opt->pcre = 0;
+			opt->regflags |= REG_EXTENDED;
+			break;
+
+		case GREP_PATTERN_TYPE_FIXED:
+			opt->fixed = 1;
+			opt->pcre = 0;
+			opt->regflags &= ~REG_EXTENDED;
+			break;
+
+		case GREP_PATTERN_TYPE_PCRE:
+			opt->fixed = 0;
+			opt->pcre = 1;
+			opt->regflags &= ~REG_EXTENDED;
+			break;
+	}
+}
+
 static int grep_config(const char *var, const char *value, void *cb)
 {
 	struct grep_opt *opt = cb;
@@ -269,13 +318,21 @@ static 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->regflags |= REG_EXTENDED;
-		else
-			opt->regflags &= ~REG_EXTENDED;
+		if (!opt->pattern_type_used) {
+			if (git_config_bool(var, value))
+				opt->regflags |= REG_EXTENDED;
+			else
+				opt->regflags &= ~REG_EXTENDED;
+		}
 		return 0;
 	}

+	if (!strcmp(var, "grep.patterntype")) {
+		grep_pattern_type_options(parse_pattern_type_arg(var, value), opt);
+		opt->pattern_type_used = 1;
+		return 0;
+  }
+
 	if (!strcmp(var, "grep.linenumber")) {
 		opt->linenum = git_config_bool(var, value);
 		return 0;
@@ -669,14 +726,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int i;
 	int dummy;
 	int use_index = 1;
-	enum {
-		pattern_type_unspecified = 0,
-		pattern_type_bre,
-		pattern_type_ere,
-		pattern_type_fixed,
-		pattern_type_pcre,
-	};
-	int pattern_type = pattern_type_unspecified;
+	int pattern_type = GREP_PATTERN_TYPE_UNSPECIFIED;

 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
@@ -705,16 +755,16 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_SET_INT('E', "extended-regexp", &pattern_type,
 			    "use extended POSIX regular expressions",
-			    pattern_type_ere),
+			    GREP_PATTERN_TYPE_ERE),
 		OPT_SET_INT('G', "basic-regexp", &pattern_type,
 			    "use basic POSIX regular expressions (default)",
-			    pattern_type_bre),
+			    GREP_PATTERN_TYPE_BRE),
 		OPT_SET_INT('F', "fixed-strings", &pattern_type,
 			    "interpret patterns as fixed strings",
-			    pattern_type_fixed),
+			    GREP_PATTERN_TYPE_FIXED),
 		OPT_SET_INT('P', "perl-regexp", &pattern_type,
 			    "use Perl-compatible regular expressions",
-			    pattern_type_pcre),
+			    GREP_PATTERN_TYPE_PCRE),
 		OPT_GROUP(""),
 		OPT_BOOLEAN('n', "line-number", &opt.linenum, "show line numbers"),
 		OPT_NEGBIT('h', NULL, &opt.pathname, "don't show filenames", 1),
@@ -824,28 +874,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_DASHDASH |
 			     PARSE_OPT_STOP_AT_NON_OPTION |
 			     PARSE_OPT_NO_INTERNAL_HELP);
-	switch (pattern_type) {
-	case pattern_type_fixed:
-		opt.fixed = 1;
-		opt.pcre = 0;
-		break;
-	case pattern_type_bre:
-		opt.fixed = 0;
-		opt.pcre = 0;
-		opt.regflags &= ~REG_EXTENDED;
-		break;
-	case pattern_type_ere:
-		opt.fixed = 0;
-		opt.pcre = 0;
-		opt.regflags |= REG_EXTENDED;
-		break;
-	case pattern_type_pcre:
-		opt.fixed = 0;
-		opt.pcre = 1;
-		break;
-	default:
-		break; /* nothing */
-	}
+
+	grep_pattern_type_options(pattern_type, &opt);

 	if (use_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
diff --git a/grep.h b/grep.h
index ed7de6b..9a6cdde 100644
--- a/grep.h
+++ b/grep.h
@@ -58,6 +58,14 @@ enum grep_expr_node {
 	GREP_NODE_OR
 };

+enum grep_pattern_type {
+	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
+	GREP_PATTERN_TYPE_BRE,
+	GREP_PATTERN_TYPE_ERE,
+	GREP_PATTERN_TYPE_FIXED,
+	GREP_PATTERN_TYPE_PCRE
+};
+
 struct grep_expr {
 	enum grep_expr_node node;
 	unsigned hit;
@@ -103,6 +111,7 @@ struct grep_opt {
 	int max_depth;
 	int funcname;
 	int funcbody;
+	int pattern_type_used;
 	char color_context[COLOR_MAXLEN];
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 523d041..4fa24b4 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -245,11 +245,41 @@ do
 		test_cmp expected actual
 	'

+	test_expect_success "grep $L with grep.patternType=false" '
+		echo "ab:a+bc" >expected &&
+		git -c grep.patternType=false grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "grep $L with grep.extendedRegexp=true" '
 		echo "ab:abc" >expected &&
 		git -c grep.extendedRegexp=true grep "a+b*c" ab >actual &&
 		test_cmp expected actual
 	'
+
+	test_expect_success "grep $L with grep.patternType=true" '
+		echo "ab:abc" >expected &&
+		git -c grep.patternType=true grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.patternType=false and grep.extendedRegexp=true" '
+		echo "ab:a+bc" >expected &&
+		git \
+			-c grep.patternType=false \
+			-c grep.extendedRegexp=true \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.patternType=true and grep.extendedRegexp=false" '
+		echo "ab:abc" >expected &&
+		git \
+			-c grep.patternType=true \
+			-c grep.extendedRegexp=false \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
 done

 cat >expected <<EOF
@@ -725,12 +755,43 @@ test_expect_success 'grep pattern with grep.extendedRegexp=true' '
 	test_cmp empty actual
 '

+test_expect_success 'grep pattern with grep.patternType=true' '
+	>empty &&
+	test_must_fail git -c grep.patternType=true \
+		grep "\p{Ps}.*?\p{Pe}" hello.c >actual &&
+	test_cmp empty actual
+'
+
+test_expect_success 'grep pattern with grep.patternType=basic and grep.extendedRegexp=true' '
+	>empty &&
+	test_must_fail git \
+		-c grep.patterntype=basic \
+		-c grep.extendedregexp=true \
+		grep "a?" hello.c >actual &&
+	test_cmp empty actual
+'
+
+test_expect_success 'grep pattern with grep.patternType=false and grep.extendedRegexp=true' '
+	>empty &&
+	test_must_fail git \
+		-c grep.patterntype=false \
+		-c grep.extendedregexp=true \
+		grep "a?" hello.c >actual &&
+	test_cmp empty actual
+'
+
 test_expect_success LIBPCRE 'grep -P pattern with grep.extendedRegexp=true' '
 	git -c grep.extendedregexp=true \
 		grep -P "\p{Ps}.*?\p{Pe}" hello.c >actual &&
 	test_cmp expected actual
 '

+test_expect_success LIBPCRE 'grep pattern with grep.patternType=perl' '
+	git -c grep.patternType=perl \
+		grep -P "\p{Ps}.*?\p{Pe}" hello.c >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success LIBPCRE 'grep -P -v pattern' '
 	{
 		echo "ab:a+b*c"
@@ -761,44 +822,170 @@ test_expect_success 'grep -G invalidpattern properly dies ' '
 	test_must_fail git grep -G "a["
 '

+test_expect_success 'grep invalidpattern properly dies with grep.patternType=basic' '
+	test_must_fail git -c patterntype=basic grep "a["
+'
+
 test_expect_success 'grep -E invalidpattern properly dies ' '
 	test_must_fail git grep -E "a["
 '

+test_expect_success 'grep invalidpattern properly dies with grep.patternType=extended' '
+	test_must_fail git -c patterntype=extended grep "a["
+'
+
 test_expect_success LIBPCRE 'grep -P invalidpattern properly dies ' '
 	test_must_fail git grep -P "a["
 '

+test_expect_success LIBPCRE 'grep invalidpattern properly dies with grep.patternType=perl' '
+	test_must_fail git -c patterntype=perl grep "a["
+'
+
 test_expect_success 'grep -G -E -F pattern' '
 	echo "ab:a+b*c" >expected &&
 	git grep -G -E -F "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=basic, =extended, =fixed' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=fixed \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -E -F -G pattern' '
 	echo "ab:a+bc" >expected &&
 	git grep -E -F -G "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=extended, =fixed, =basic' '
+	echo "ab:a+bc" >expected &&
+	git \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -F -G -E pattern' '
 	echo "ab:abc" >expected &&
 	git grep -F -G -E "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =extended' '
+	echo "ab:abc" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -G -F -P -E pattern' '
 	>empty &&
 	test_must_fail git grep -G -F -P -E "a\x{2b}b\x{2a}c" ab >actual &&
 	test_cmp empty actual
 '

+test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =perl, =extended' '
+	>empty &&
+	test_must_fail git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=perl \
+		-c grep.patterntype=extended \
+		grep "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp empty actual
+'
+
 test_expect_success LIBPCRE 'grep -G -F -E -P pattern' '
 	echo "ab:a+b*c" >expected &&
 	git grep -G -F -E -P "a\x{2b}b\x{2a}c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success LIBPCRE 'grep pattern with grep.patternType=fixed, =basic, =extended, =perl' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success LIBPCRE 'grep -P pattern with grep.patternType=fixed, =basic, =extended' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep -P "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -F pattern with grep.patternType=fixed, =basic, =extended, =perl' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep -F "*c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -G pattern with grep.patternType=fixed, =basic, =extended, =perl' '
+	{
+		echo "ab:a+b*c"
+		echo "ab:a+bc"
+	} >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep -G "a+b" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -E pattern with grep.patternType=fixed, =basic, =extended, =perl' '
+	{
+		echo "ab:a+b*c"
+		echo "ab:a+bc"
+		echo "ab:abc"
+	} >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep -E "a+" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep pattern with grep.patternType=extended and grep.extendedRegexp=false' '
+	cat >expected <<-EOF
+	hello.c:int main(int argc, const char **argv)
+	EOF
+	git \
+		-c grep.patterntype=extended \
+		-c grep.extendedregexp=false \
+		grep "con?st" hello.c >actual &&
+	test_cmp expected actual
+'
+
 test_config() {
 	git config "$1" "$2" &&
 	test_when_finished "git config --unset $1"
--
1.7.11.3

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[Junio C Hamano]

J Smith <dark.panda@gmail.com> writes:

As the basic structure and the direction looks good, let's start
nitpicking ;-)

> Adds the grep.patternType configuration setting which sets the default
> pattern matching behavior. The values "basic", "extended", "fixed", and
> "perl" can be used to set "--basic-regexp", "--extended-regexp",
> "--fixed-strings", and "--perl-regexp" options by default respectively.

We tend to write the commit log message in imperative mood, as if
you are giving an order to the codebase to "behave this way!".  Also
we tend to give the justification behind the change first and then
present the solution.

	There is grep.extendedRegexp configuration variable to
	enable the -E command line flag by default, but there is no
	equivalent for the -P (pcre) flag.  We could keep adding
	grep.fooRegexp variables for different regular expression
	variants, but that will be unwieldy.

	Instead, add a "grep.patternType" variable that can be set
	to "basic", "extended", "fixed" and "perl" to use
	"--basic-regexp", "--extended-regexp", "--fixed-strings",
	and "--perl-regexp" options by default respectively.

	Ignore grep.extendedRegexp when grep.patternType is set.

> A value of true is equivalent to "extended" as with grep.extendedRegexp,
> and a value of false leaves the pattern type as unspecified and follows
> the default grep behavior.

With this round, we are not updating an existing a bool variable,
but are introducing a brand new one; does it still make sense to
support the boolean values for this new variable?

> This setting overrides the value set in grep.extendedRegexp which will
> be ignored completely if grep.patternType is set.
> ---

Sign-off?

>  Documentation/config.txt   |  11 ++-
>  Documentation/git-grep.txt |  11 ++-
>  builtin/grep.c             | 106 ++++++++++++++++---------
>  grep.h                     |   9 +++
>  t/t7810-grep.sh            | 187 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 284 insertions(+), 40 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index a95e5a4..38d56d8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1210,8 +1210,17 @@ gitweb.snapshot::
>  grep.lineNumber::
>  	If set to true, enable '-n' option by default.
>
> +grep.patternType::
> +	Sets the default matching behavior. This option can be set to a
> +	boolean value or one of 'basic', 'extended', 'fixed', or 'perl'
> +	which will enable the '--basic-regexp', '--extended-regexp',
> +	'--fixed-strings' or '--perl-regexp' options accordingly. The value
> +	of true is equivalent to 'extended' while false leaves the
> +	settings in their default state.

Perhaps s/Sets the/The/ or at least s/Sets/Set/ (notice that the
description for grep.extendedRegexp says "enable foo", not "enables
foo").

The same comment as above applies to the "boolean"-ness part.

>  grep.extendedRegexp::
> -	If set to true, enable '--extended-regexp' option by default.
> +	If set to true, enable '--extended-regexp' option by default. This
> +	option is ignored when the 'grep.patternType' option is set.

We are not going to make grep.patternType a boolean, so "when ... is
set" is fine, but if we were to allow grep.patternType to be set to
"false", the description gives ambiguity to some readers who do.
Perhaps s/is set/is given/ is safer.

> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 3bec036..f56f67f 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -42,8 +42,17 @@ CONFIGURATION
>  grep.lineNumber::
>  	If set to true, enable '-n' option by default.
>
> +grep.patternType::
> ...
>  grep.extendedRegexp::
> -	If set to true, enable '--extended-regexp' option by default.
> +	If set to true, enable '--extended-regexp' option by default. This
> +	option is ignored when the 'grep.patternType' option is set.

Likewise.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 29adb0a..1de7e76 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -260,6 +260,55 @@ static int wait_all(void)
>  }
>  #endif
>
> +static int parse_pattern_type_arg(const char *opt, const char *arg)
> +{
> +	switch (git_config_maybe_bool(opt, arg)) {
> +	case 1:
> +		return GREP_PATTERN_TYPE_ERE;
> +	case 0:
> +		return GREP_PATTERN_TYPE_UNSPECIFIED;
> +	default:
> +		if (!strcmp(arg, "basic"))
> +			return GREP_PATTERN_TYPE_BRE;
> +		else if (!strcmp(arg, "extended"))
> +			return GREP_PATTERN_TYPE_ERE;
> +		else if (!strcmp(arg, "fixed"))
> +			return GREP_PATTERN_TYPE_FIXED;
> +		else if (!strcmp(arg, "perl"))
> +			return GREP_PATTERN_TYPE_PCRE;
> +		die("bad %s argument: %s", opt, arg);
> +	}

Let's not do maybe-bool, as we are not upgrading an old bool-only
variable any more.

> +static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
> +{
> +	switch (pattern_type) {
> +		case GREP_PATTERN_TYPE_BRE:
> +			opt->fixed = 0;
> ...
> +			break;
> +	}

Please de-dent these lines inside switch() one level; switch and
case align.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 523d041..4fa24b4 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -245,11 +245,41 @@ do
>  		test_cmp expected actual
>  	'
>
> +	test_expect_success "grep $L with grep.patternType=false" '
> +		echo "ab:a+bc" >expected &&
> +		git -c grep.patternType=false grep "a+b*c" ab >actual &&
> +		test_cmp expected actual
> +	'
> +
>  	test_expect_success "grep $L with grep.extendedRegexp=true" '
>  		echo "ab:abc" >expected &&
>  		git -c grep.extendedRegexp=true grep "a+b*c" ab >actual &&
>  		test_cmp expected actual
>  	'
> +
> +	test_expect_success "grep $L with grep.patternType=true" '
> +		echo "ab:abc" >expected &&
> +		git -c grep.patternType=true grep "a+b*c" ab >actual &&
> +		test_cmp expected actual
> +	'
> +
> +	test_expect_success "grep $L with grep.patternType=false and grep.extendedRegexp=true" '
> +		echo "ab:a+bc" >expected &&
> +		git \
> +			-c grep.patternType=false \
> +			-c grep.extendedRegexp=true \
> +			grep "a+b*c" ab >actual &&
> +		test_cmp expected actual
> +	'
> +
> +	test_expect_success "grep $L with grep.patternType=true and grep.extendedRegexp=false" '
> +		echo "ab:abc" >expected &&
> +		git \
> +			-c grep.patternType=true \
> +			-c grep.extendedRegexp=false \
> +			grep "a+b*c" ab >actual &&
> +		test_cmp expected actual
> +	'

It might make sense to also make sure the order in which the
configuration variables are given does not make a difference with
these tests.

> @@ -725,12 +755,43 @@ test_expect_success 'grep pattern with grep.extendedRegexp=true' '
>  	test_cmp empty actual
>  '
>
> +test_expect_success 'grep pattern with grep.patternType=true' '
> +	>empty &&
> +	test_must_fail git -c grep.patternType=true \
> +		grep "\p{Ps}.*?\p{Pe}" hello.c >actual &&
> +	test_cmp empty actual
> +'

When told to use basic-regexp, PCRE should not be used.  Good.

> +test_expect_success 'grep pattern with grep.patternType=basic and grep.extendedRegexp=true' '
> +	>empty &&
> +	test_must_fail git \
> +		-c grep.patterntype=basic \
> +		-c grep.extendedregexp=true \
> +		grep "a?" hello.c >actual &&
> +	test_cmp empty actual
> +'

When told to use basic-regexp via patternType, extendedRegexp should
not be used.  Good.

> +
> +test_expect_success 'grep pattern with grep.patternType=false and grep.extendedRegexp=true' '
> +	>empty &&
> +	test_must_fail git \
> +		-c grep.patterntype=false \
> +		-c grep.extendedregexp=true \
> +		grep "a?" hello.c >actual &&
> +	test_cmp empty actual
> +'

With the removal of "bool-or-type", this will become redundant.

>  test_expect_success LIBPCRE 'grep -P pattern with grep.extendedRegexp=true' '
>  	git -c grep.extendedregexp=true \
>  		grep -P "\p{Ps}.*?\p{Pe}" hello.c >actual &&
>  	test_cmp expected actual
>  '
>
> +test_expect_success LIBPCRE 'grep pattern with grep.patternType=perl' '
> +	git -c grep.patternType=perl \
> +		grep -P "\p{Ps}.*?\p{Pe}" hello.c >actual &&
> +	test_cmp expected actual
> +'

What does this test?  grep.patternType=perl configuration is
correctly overridden by a command line flag -P?  But you cannot tell
which one turned pcre with this test.  Drop -P from the command
line, perhaps?

You want a test that runs "git -c grep.patternType=basic grep -P" or
something, guarded with LIBPCRE prerequisite, to make sure pcre
patterns are used because command line -P trumps over configured
default, too.

> @@ -761,44 +822,170 @@ test_expect_success 'grep -G invalidpattern properly dies ' '
>  	test_must_fail git grep -G "a["
>  '
>
> +test_expect_success 'grep invalidpattern properly dies with grep.patternType=basic' '
> +	test_must_fail git -c patterntype=basic grep "a["
> +'
> +
>  test_expect_success 'grep -E invalidpattern properly dies ' '
>  	test_must_fail git grep -E "a["
>  '
>
> +test_expect_success 'grep invalidpattern properly dies with grep.patternType=extended' '
> +	test_must_fail git -c patterntype=extended grep "a["
> +'
> +
>  test_expect_success LIBPCRE 'grep -P invalidpattern properly dies ' '
>  	test_must_fail git grep -P "a["
>  '
>
> +test_expect_success LIBPCRE 'grep invalidpattern properly dies with grep.patternType=perl' '
> +	test_must_fail git -c patterntype=perl grep "a["
> +'
> +

These three may not add much value, as long as we make sure that the
configuration "-c grep.patterntype" triggers the pattern matching
backend just like command line flags do with other tests.

Besides, I do not think you are testing the right thing in them
anyway (notice the lack of "grep." prefix).

> +test_expect_success 'grep pattern with grep.patternType=basic, =extended, =fixed' '
> +	echo "ab:a+b*c" >expected &&
> +	git \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=fixed \
> +		grep "a+b*c" ab >actual &&
> +	test_cmp expected actual
> +'

What does this test?  The last one wins?

For the command line flags, people can do "alias g 'git grep -E'"
and then countermand the flags in the alias by appending a
contradicting flag when using it, e.g. "g -G", last one wins is a
defined and useful semantics, but for configuration variables that
are meant to take a single value, I do not think we give such a
strong guarantee on ordering (it may happen to work by accident,
though).

I would _not_ strongly suggest removing this test, but instead wait
until we hear from others, as they may disagree.

>  test_expect_success 'grep -E -F -G pattern' '
>  	echo "ab:a+bc" >expected &&
>  	git grep -E -F -G "a+b*c" ab >actual &&
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'grep pattern with grep.patternType=extended, =fixed, =basic' '
> +	echo "ab:a+bc" >expected &&
> +	git \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		grep "a+b*c" ab >actual &&
> +	test_cmp expected actual
> +'
> +

Likewise.

>  test_expect_success 'grep -F -G -E pattern' '
>  	echo "ab:abc" >expected &&
>  	git grep -F -G -E "a+b*c" ab >actual &&
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =extended' '
> +	echo "ab:abc" >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		grep "a+b*c" ab >actual &&
> +	test_cmp expected actual
> +'

Likewise.

>  test_expect_success 'grep -G -F -P -E pattern' '
>  	>empty &&
>  	test_must_fail git grep -G -F -P -E "a\x{2b}b\x{2a}c" ab >actual &&
>  	test_cmp empty actual
>  '
>
> +test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =perl, =extended' '
> +	>empty &&
> +	test_must_fail git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=perl \
> +		-c grep.patterntype=extended \
> +		grep "a\x{2b}b\x{2a}c" ab >actual &&
> +	test_cmp empty actual
> +'

Likewise.

>  test_expect_success LIBPCRE 'grep -G -F -E -P pattern' '
>  	echo "ab:a+b*c" >expected &&
>  	git grep -G -F -E -P "a\x{2b}b\x{2a}c" ab >actual &&
>  	test_cmp expected actual
>  '
>
> +test_expect_success LIBPCRE 'grep pattern with grep.patternType=fixed, =basic, =extended, =perl' '
> +	echo "ab:a+b*c" >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=perl \
> +		grep "a\x{2b}b\x{2a}c" ab >actual &&
> +	test_cmp expected actual
> +'

Likewise.

> +test_expect_success LIBPCRE 'grep -P pattern with grep.patternType=fixed, =basic, =extended' '
> +	echo "ab:a+b*c" >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=perl \
> +		grep -P "a\x{2b}b\x{2a}c" ab >actual &&
> +	test_cmp expected actual
> +'

As you are expecting the "last one wins" behaviour among
configuration variables, running a test with -P option would not let
you catch bugs coming from potentially screwed-up precedence between
the configuration and command line flags, would it?  At least, leave
the "-c grep.patterntype=perl" out from here to make sure what the
variable and the flag tell the command conflict with each other.  I
would also prefer to see only one "-c grep.patterntype=<foo>" used.

> +test_expect_success 'grep -F pattern with grep.patternType=fixed, =basic, =extended, =perl' '
> +	echo "ab:a+b*c" >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=perl \
> +		grep -F "*c" ab >actual &&
> +	test_cmp expected actual
> +'

Likewise.

> +test_expect_success 'grep -G pattern with grep.patternType=fixed, =basic, =extended, =perl' '
> +	{
> +		echo "ab:a+b*c"
> +		echo "ab:a+bc"
> +	} >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=perl \
> +		grep -G "a+b" ab >actual &&
> +	test_cmp expected actual
> +'

Likewise.

> +test_expect_success 'grep -E pattern with grep.patternType=fixed, =basic, =extended, =perl' '
> +	{
> +		echo "ab:a+b*c"
> +		echo "ab:a+bc"
> +		echo "ab:abc"
> +	} >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		-c grep.patterntype=basic \
> +		-c grep.patterntype=extended \
> +		-c grep.patterntype=perl \
> +		grep -E "a+" ab >actual &&
> +	test_cmp expected actual
> +'

Likewise.

> +test_expect_success 'grep pattern with grep.patternType=extended and grep.extendedRegexp=false' '
> +	cat >expected <<-EOF
> +	hello.c:int main(int argc, const char **argv)
> +	EOF
> +	git \
> +		-c grep.patterntype=extended \
> +		-c grep.extendedregexp=false \
> +		grep "con?st" hello.c >actual &&
> +	test_cmp expected actual
> +'

What does this test?  patterntype trumps extendedregexp?

That may sit better next to the earlier "patterntype says basic but
extendedregexp says true" test, if you can move this test easily
there.

Thanks.

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[Štěpán Němec]

On Wed, 01 Aug 2012 14:55:52 -0700
Junio C. Hamano wrote:

> J Smith <dark.panda@gmail.com> writes:
>
>>  grep.extendedRegexp::
>> -	If set to true, enable '--extended-regexp' option by default.
>> +	If set to true, enable '--extended-regexp' option by default. This
>> +	option is ignored when the 'grep.patternType' option is set.
>
> We are not going to make grep.patternType a boolean, so "when ... is
> set" is fine, but if we were to allow grep.patternType to be set to
> "false", the description gives ambiguity to some readers who do.
> Perhaps s/is set/is given/ is safer.

I'm not a native speaker, but to me "is given" implies command line (the
meaning is clear here, it just sounds a bit weird). If it's not just me,
"is used" or "has a value" might be better.

-- 
Štěpán

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[J Smith]

On Wed, Aug 1, 2012 at 5:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> As the basic structure and the direction looks good, let's start
> nitpicking ;-)

Sounds good.

> We tend to write the commit log message in imperative mood, as if
> you are giving an order to the codebase to "behave this way!".  Also
> we tend to give the justification behind the change first and then
> present the solution.

Sounds good to me. I'll re-word the commit messages in future
revisions of the patch.

> With this round, we are not updating an existing a bool variable,
> but are introducing a brand new one; does it still make sense to
> support the boolean values for this new variable?

Yeah, I thought about that, having noticed in your edited patch that
the boolean options were still in there for patternType. I do think it
would be useful to have a way to get back to the default settings, say
on a per-repo basis to override a global setting. I was thinking that
a "false" value could provide that, but perhaps a value of "default"
would make more sense?

> You want a test that runs "git -c grep.patternType=basic grep -P" or
> something, guarded with LIBPCRE prerequisite, to make sure pcre
> patterns are used because command line -P trumps over configured
> default, too.

Will add.

> Besides, I do not think you are testing the right thing in them
> anyway (notice the lack of "grep." prefix).

Ah geez. Yeah, that's just stupidity.

> What does this test?  The last one wins?
>
> For the command line flags, people can do "alias g 'git grep -E'"
> and then countermand the flags in the alias by appending a
> contradicting flag when using it, e.g. "g -G", last one wins is a
> defined and useful semantics, but for configuration variables that
> are meant to take a single value, I do not think we give such a
> strong guarantee on ordering (it may happen to work by accident,
> though).
>
> I would _not_ strongly suggest removing this test, but instead wait
> until we hear from others, as they may disagree.

I'll wait for others and we'll see. I'm not overly attached to them or anything.

> As you are expecting the "last one wins" behaviour among
> configuration variables, running a test with -P option would not let
> you catch bugs coming from potentially screwed-up precedence between
> the configuration and command line flags, would it?  At least, leave
> the "-c grep.patterntype=perl" out from here to make sure what the
> variable and the flag tell the command conflict with each other.  I
> would also prefer to see only one "-c grep.patterntype=<foo>" used.

Ah, yes, that was how the test was supposed to be written. That was an
oversight.

> What does this test?  patterntype trumps extendedregexp?
>
> That may sit better next to the earlier "patterntype says basic but
> extendedregexp says true" test, if you can move this test easily
> there.

Yep, I'll move it around.

Cheers

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[J Smith]

Alright, I have revised the patch and fixed up the nits that were
picked and made a quick modification. I've added a setting for
grep.patternType for "default" which can restore the default grep
pattern matching behaviour and restores the functionality back to
grep.extendedRegexp. I added this functionality for situations like
where you would have grep.patternType set to, say, "perl" in your
$HOME/.gitconfig but don't want that functionality set in a specific
repo and would rather to have it fall back to the older
grep.extendedRegexp behaviour so you can set it to "default" in the
repo's .git/config.

This change also lets us determine the final set of pattern type
options in one place rather than the current code which does two
checks -- once when we call grep_config to determine the configuration
options and then another a few lines later when we call it for the
arguments given to grep. Now we capture the values we receive from
grep.patternType and grep.extendedRegexp in the grep_opt struct as
pattern_type_option and extended_regexp_option, capture the pattern
type argument given to the command itself, and then make the final
determination for the options to be used in one place rather than the
current manner. I think it should be more obvious this way.

I'll post the latest patch shortly for review if this sounds reasonable. Cheers.

[PATCH/RFC] grep: add a grep.patternType configuration setting

[J Smith]

The grep.extendedRegexp configuration setting enables the -E flag on grep
by default but there are no equivalents for the -G, -F and -P flags.

Rather than adding an additional setting for grep.fooRegexp for current
and future pattern matching options, add a grep.patternType setting that
can accept appropriate values for modifying the default grep pattern
matching behavior. The current values are "basic", "extended", "fixed",
"perl" and "default" for setting -G, -E, -F, -P and the default behavior
respectively.

When grep.patternType is set to a value other than "default", the
grep.extendedRegexp setting is ignored. The value of "default" restores
the current default behavior, including the grep.extendedRegexp
behavior.

Signed-off-by: J Smith <dark.panda@gmail.com>
---
 Documentation/config.txt   |  10 ++-
 Documentation/git-grep.txt |  10 ++-
 builtin/grep.c             | 112 ++++++++++++++++++----------
 grep.h                     |  10 +++
 t/t7810-grep.sh            | 181 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 282 insertions(+), 41 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a95e5a4..6416cae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1210,8 +1210,16 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable '-n' option by default.

+grep.patternType::
+	Set the default matching behavior. Using a value of 'basic', 'extended',
+	'fixed', or 'perl' will enable the '--basic-regexp', '--extended-regexp',
+	'--fixed-strings', or '--perl-regexp' option accordingly, while the
+	value 'default' will return to the default matching behavior.
+
 grep.extendedRegexp::
-	If set to true, enable '--extended-regexp' option by default.
+	If set to true, enable '--extended-regexp' option by default. This
+	option is ignored when the 'grep.patternType' option is set to a value
+	other than 'default'.

 gpg.program::
 	Use this custom program instead of "gpg" found on $PATH when
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 3bec036..cfecf84 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -42,8 +42,16 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable '-n' option by default.

+grep.patternType::
+	Set the default matching behavior. Using a value of 'basic', 'extended',
+	'fixed', or 'perl' will enable the '--basic-regexp', '--extended-regexp',
+	'--fixed-strings', or '--perl-regexp' option accordingly, while the
+	value 'default' will return to the default matching behavior.
+
 grep.extendedRegexp::
-	If set to true, enable '--extended-regexp' option by default.
+	If set to true, enable '--extended-regexp' option by default. This
+	option is ignored when the 'grep.patternType' option is set to a value
+	other than 'default'.


 OPTIONS
diff --git a/builtin/grep.c b/builtin/grep.c
index 29adb0a..7ff64da 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -260,6 +260,53 @@ static int wait_all(void)
 }
 #endif

+static int parse_pattern_type_arg(const char *opt, const char *arg)
+{
+	if (!strcmp(arg, "default"))
+		return GREP_PATTERN_TYPE_UNSPECIFIED;
+	else if (!strcmp(arg, "basic"))
+		return GREP_PATTERN_TYPE_BRE;
+	else if (!strcmp(arg, "extended"))
+		return GREP_PATTERN_TYPE_ERE;
+	else if (!strcmp(arg, "fixed"))
+		return GREP_PATTERN_TYPE_FIXED;
+	else if (!strcmp(arg, "perl"))
+		return GREP_PATTERN_TYPE_PCRE;
+	die("bad %s argument: %s", opt, arg);
+}
+
+static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
+{
+	switch (pattern_type) {
+	case GREP_PATTERN_TYPE_UNSPECIFIED:
+		/* fall through */
+
+	case GREP_PATTERN_TYPE_BRE:
+		opt->fixed = 0;
+		opt->pcre = 0;
+		opt->regflags &= ~REG_EXTENDED;
+		break;
+
+	case GREP_PATTERN_TYPE_ERE:
+		opt->fixed = 0;
+		opt->pcre = 0;
+		opt->regflags |= REG_EXTENDED;
+		break;
+
+	case GREP_PATTERN_TYPE_FIXED:
+		opt->fixed = 1;
+		opt->pcre = 0;
+		opt->regflags &= ~REG_EXTENDED;
+		break;
+
+	case GREP_PATTERN_TYPE_PCRE:
+		opt->fixed = 0;
+		opt->pcre = 1;
+		opt->regflags &= ~REG_EXTENDED;
+		break;
+	}
+}
+
 static int grep_config(const char *var, const char *value, void *cb)
 {
 	struct grep_opt *opt = cb;
@@ -270,12 +317,17 @@ static int grep_config(const char *var, const char *value, void *cb)

 	if (!strcmp(var, "grep.extendedregexp")) {
 		if (git_config_bool(var, value))
-			opt->regflags |= REG_EXTENDED;
+			opt->extended_regexp_option = 1;
 		else
-			opt->regflags &= ~REG_EXTENDED;
+			opt->extended_regexp_option = 0;
 		return 0;
 	}

+	if (!strcmp(var, "grep.patterntype")) {
+		opt->pattern_type_option = parse_pattern_type_arg(var, value);
+		return 0;
+  }
+
 	if (!strcmp(var, "grep.linenumber")) {
 		opt->linenum = git_config_bool(var, value);
 		return 0;
@@ -669,14 +721,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int i;
 	int dummy;
 	int use_index = 1;
-	enum {
-		pattern_type_unspecified = 0,
-		pattern_type_bre,
-		pattern_type_ere,
-		pattern_type_fixed,
-		pattern_type_pcre,
-	};
-	int pattern_type = pattern_type_unspecified;
+	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;

 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
@@ -703,18 +748,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			"descend at most <depth> levels", PARSE_OPT_NONEG,
 			NULL, 1 },
 		OPT_GROUP(""),
-		OPT_SET_INT('E', "extended-regexp", &pattern_type,
+		OPT_SET_INT('E', "extended-regexp", &pattern_type_arg,
 			    "use extended POSIX regular expressions",
-			    pattern_type_ere),
-		OPT_SET_INT('G', "basic-regexp", &pattern_type,
+			    GREP_PATTERN_TYPE_ERE),
+		OPT_SET_INT('G', "basic-regexp", &pattern_type_arg,
 			    "use basic POSIX regular expressions (default)",
-			    pattern_type_bre),
-		OPT_SET_INT('F', "fixed-strings", &pattern_type,
+			    GREP_PATTERN_TYPE_BRE),
+		OPT_SET_INT('F', "fixed-strings", &pattern_type_arg,
 			    "interpret patterns as fixed strings",
-			    pattern_type_fixed),
-		OPT_SET_INT('P', "perl-regexp", &pattern_type,
+			    GREP_PATTERN_TYPE_FIXED),
+		OPT_SET_INT('P', "perl-regexp", &pattern_type_arg,
 			    "use Perl-compatible regular expressions",
-			    pattern_type_pcre),
+			    GREP_PATTERN_TYPE_PCRE),
 		OPT_GROUP(""),
 		OPT_BOOLEAN('n', "line-number", &opt.linenum, "show line numbers"),
 		OPT_NEGBIT('h', NULL, &opt.pathname, "don't show filenames", 1),
@@ -799,6 +844,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	opt.header_tail = &opt.header_list;
 	opt.regflags = REG_NEWLINE;
 	opt.max_depth = -1;
+	opt.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
+	opt.extended_regexp_option = 0;

 	strcpy(opt.color_context, "");
 	strcpy(opt.color_filename, "");
@@ -824,27 +871,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_DASHDASH |
 			     PARSE_OPT_STOP_AT_NON_OPTION |
 			     PARSE_OPT_NO_INTERNAL_HELP);
-	switch (pattern_type) {
-	case pattern_type_fixed:
-		opt.fixed = 1;
-		opt.pcre = 0;
-		break;
-	case pattern_type_bre:
-		opt.fixed = 0;
-		opt.pcre = 0;
-		opt.regflags &= ~REG_EXTENDED;
-		break;
-	case pattern_type_ere:
-		opt.fixed = 0;
-		opt.pcre = 0;
-		opt.regflags |= REG_EXTENDED;
-		break;
-	case pattern_type_pcre:
-		opt.fixed = 0;
-		opt.pcre = 1;
-		break;
-	default:
-		break; /* nothing */
+
+	if (pattern_type_arg > GREP_PATTERN_TYPE_UNSPECIFIED)
+		grep_pattern_type_options(pattern_type_arg, &opt);
+	else {
+		if (opt.pattern_type_option > GREP_PATTERN_TYPE_UNSPECIFIED)
+			grep_pattern_type_options(opt.pattern_type_option, &opt);
+		else if (opt.extended_regexp_option)
+			grep_pattern_type_options(GREP_PATTERN_TYPE_ERE, &opt);
 	}

 	if (use_index && !startup_info->have_repository)
diff --git a/grep.h b/grep.h
index ed7de6b..75afb7b 100644
--- a/grep.h
+++ b/grep.h
@@ -58,6 +58,14 @@ enum grep_expr_node {
 	GREP_NODE_OR
 };

+enum grep_pattern_type {
+	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
+	GREP_PATTERN_TYPE_BRE,
+	GREP_PATTERN_TYPE_ERE,
+	GREP_PATTERN_TYPE_FIXED,
+	GREP_PATTERN_TYPE_PCRE
+};
+
 struct grep_expr {
 	enum grep_expr_node node;
 	unsigned hit;
@@ -103,6 +111,8 @@ struct grep_opt {
 	int max_depth;
 	int funcname;
 	int funcbody;
+	int extended_regexp_option;
+	int pattern_type_option;
 	char color_context[COLOR_MAXLEN];
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 523d041..35d357d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -250,6 +250,84 @@ do
 		git -c grep.extendedRegexp=true grep "a+b*c" ab >actual &&
 		test_cmp expected actual
 	'
+
+	test_expect_success "grep $L with grep.patterntype=basic" '
+		echo "ab:a+bc" >expected &&
+		git -c grep.patterntype=basic grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.patterntype=extended" '
+		echo "ab:abc" >expected &&
+		git -c grep.patterntype=extended grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.patterntype=fixed" '
+		echo "ab:a+b*c" >expected &&
+		git -c grep.patterntype=fixed grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success LIBPCRE "grep $L with grep.patterntype=perl" '
+		echo "ab:a+b*c" >expected &&
+		git -c grep.patterntype=perl grep "a\x{2b}b\x{2a}c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.patternType=default and grep.extendedRegexp=true" '
+		echo "ab:abc" >expected &&
+		git \
+			-c grep.patternType=default \
+			-c grep.extendedRegexp=true \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=default" '
+		echo "ab:abc" >expected &&
+		git \
+			-c grep.extendedRegexp=true \
+			-c grep.patternType=default \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success 'grep $L with grep.patternType=extended and grep.extendedRegexp=false' '
+		echo "ab:abc" >expected &&
+		git \
+			-c grep.patternType=extended \
+			-c grep.extendedRegexp=false \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success 'grep $L with grep.patternType=basic and grep.extendedRegexp=true' '
+		echo "ab:a+bc" >expected &&
+		git \
+			-c grep.patternType=basic \
+			-c grep.extendedRegexp=true \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success 'grep $L with grep.extendedRegexp=false and grep.patternType=extended' '
+		echo "ab:abc" >expected &&
+		git \
+			-c grep.extendedRegexp=false \
+			-c grep.patternType=extended \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success 'grep $L with grep.extendedRegexp=true and grep.patternType=basic' '
+		echo "ab:a+bc" >expected &&
+		git \
+			-c grep.extendedRegexp=true \
+			-c grep.patternType=basic \
+			grep "a+b*c" ab >actual &&
+		test_cmp expected actual
+	'
 done

 cat >expected <<EOF
@@ -761,44 +839,147 @@ test_expect_success 'grep -G invalidpattern properly dies ' '
 	test_must_fail git grep -G "a["
 '

+test_expect_success 'grep invalidpattern properly dies with grep.patternType=basic' '
+	test_must_fail git -c grep.patterntype=basic grep "a["
+'
+
 test_expect_success 'grep -E invalidpattern properly dies ' '
 	test_must_fail git grep -E "a["
 '

+test_expect_success 'grep invalidpattern properly dies with grep.patternType=extended' '
+	test_must_fail git -c grep.patterntype=extended grep "a["
+'
+
 test_expect_success LIBPCRE 'grep -P invalidpattern properly dies ' '
 	test_must_fail git grep -P "a["
 '

+test_expect_success LIBPCRE 'grep invalidpattern properly dies with grep.patternType=perl' '
+	test_must_fail git -c grep.patterntype=perl grep "a["
+'
+
 test_expect_success 'grep -G -E -F pattern' '
 	echo "ab:a+b*c" >expected &&
 	git grep -G -E -F "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=basic, =extended, =fixed' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=fixed \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -E -F -G pattern' '
 	echo "ab:a+bc" >expected &&
 	git grep -E -F -G "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=extended, =fixed, =basic' '
+	echo "ab:a+bc" >expected &&
+	git \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -F -G -E pattern' '
 	echo "ab:abc" >expected &&
 	git grep -F -G -E "a+b*c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =extended' '
+	echo "ab:abc" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		grep "a+b*c" ab >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep -G -F -P -E pattern' '
 	>empty &&
 	test_must_fail git grep -G -F -P -E "a\x{2b}b\x{2a}c" ab >actual &&
 	test_cmp empty actual
 '

+test_expect_success 'grep pattern with grep.patternType=fixed, =basic, =perl, =extended' '
+	>empty &&
+	test_must_fail git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=perl \
+		-c grep.patterntype=extended \
+		grep "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp empty actual
+'
+
 test_expect_success LIBPCRE 'grep -G -F -E -P pattern' '
 	echo "ab:a+b*c" >expected &&
 	git grep -G -F -E -P "a\x{2b}b\x{2a}c" ab >actual &&
 	test_cmp expected actual
 '

+test_expect_success LIBPCRE 'grep pattern with grep.patternType=fixed, =basic, =extended, =perl' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		-c grep.patterntype=basic \
+		-c grep.patterntype=extended \
+		-c grep.patterntype=perl \
+		grep "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success LIBPCRE 'grep -P pattern with grep.patternType=fixed' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		grep -P "a\x{2b}b\x{2a}c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -F pattern with grep.patternType=basic' '
+	echo "ab:a+b*c" >expected &&
+	git \
+		-c grep.patterntype=basic \
+		grep -F "*c" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -G pattern with grep.patternType=fixed' '
+	{
+		echo "ab:a+b*c"
+		echo "ab:a+bc"
+	} >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		grep -G "a+b" ab >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'grep -E pattern with grep.patternType=fixed' '
+	{
+		echo "ab:a+b*c"
+		echo "ab:a+bc"
+		echo "ab:abc"
+	} >expected &&
+	git \
+		-c grep.patterntype=fixed \
+		grep -E "a+" ab >actual &&
+	test_cmp expected actual
+'
+
 test_config() {
 	git config "$1" "$2" &&
 	test_when_finished "git config --unset $1"
--
1.7.11.3

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[Junio C Hamano]

J Smith <dark.panda@gmail.com> writes:

> The grep.extendedRegexp configuration setting enables the -E flag on grep
> by default but there are no equivalents for the -G, -F and -P flags.
>
> Rather than adding an additional setting for grep.fooRegexp for current
> and future pattern matching options, add a grep.patternType setting that
> can accept appropriate values for modifying the default grep pattern
> matching behavior. The current values are "basic", "extended", "fixed",
> "perl" and "default" for setting -G, -E, -F, -P and the default behavior
> respectively.
>
> When grep.patternType is set to a value other than "default", the
> grep.extendedRegexp setting is ignored. The value of "default" restores
> the current default behavior, including the grep.extendedRegexp
> behavior.
>
> Signed-off-by: J Smith <dark.panda@gmail.com>
> ---

Nicely done.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 29adb0a..7ff64da 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -260,6 +260,53 @@ static int wait_all(void)
>  }
>  #endif
>
> +static int parse_pattern_type_arg(const char *opt, const char *arg)
> +{
> +	if (!strcmp(arg, "default"))
> +		return GREP_PATTERN_TYPE_UNSPECIFIED;
> +	else if (!strcmp(arg, "basic"))
> +		return GREP_PATTERN_TYPE_BRE;
> +	else if (!strcmp(arg, "extended"))
> +		return GREP_PATTERN_TYPE_ERE;
> +	else if (!strcmp(arg, "fixed"))
> +		return GREP_PATTERN_TYPE_FIXED;
> +	else if (!strcmp(arg, "perl"))
> +		return GREP_PATTERN_TYPE_PCRE;
> +	die("bad %s argument: %s", opt, arg);
> +}
> +
> +static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
> +{
> +	switch (pattern_type) {
> +	case GREP_PATTERN_TYPE_UNSPECIFIED:
> +		/* fall through */
> +
> +	case GREP_PATTERN_TYPE_BRE:
> +		opt->fixed = 0;
> +		opt->pcre = 0;
> +		opt->regflags &= ~REG_EXTENDED;
> +		break;
> +
> +	case GREP_PATTERN_TYPE_ERE:
> +		opt->fixed = 0;
> +		opt->pcre = 0;
> +		opt->regflags |= REG_EXTENDED;
> +		break;
> +
> +	case GREP_PATTERN_TYPE_FIXED:
> +		opt->fixed = 1;
> +		opt->pcre = 0;
> +		opt->regflags &= ~REG_EXTENDED;
> +		break;
> +
> +	case GREP_PATTERN_TYPE_PCRE:
> +		opt->fixed = 0;
> +		opt->pcre = 1;
> +		opt->regflags &= ~REG_EXTENDED;
> +		break;
> +	}
> +}
> +
>  static int grep_config(const char *var, const char *value, void *cb)
>  {
>  	struct grep_opt *opt = cb;
> @@ -270,12 +317,17 @@ static int grep_config(const char *var, const char *value, void *cb)
>
>  	if (!strcmp(var, "grep.extendedregexp")) {
>  		if (git_config_bool(var, value))
> -			opt->regflags |= REG_EXTENDED;
> +			opt->extended_regexp_option = 1;
>  		else
> -			opt->regflags &= ~REG_EXTENDED;
> +			opt->extended_regexp_option = 0;
>  		return 0;
>  	}
>
> +	if (!strcmp(var, "grep.patterntype")) {
> +		opt->pattern_type_option = parse_pattern_type_arg(var, value);
> +		return 0;
> +  }
> +
>  	if (!strcmp(var, "grep.linenumber")) {
>  		opt->linenum = git_config_bool(var, value);
>  		return 0;
> ...
> @@ -799,6 +844,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> +	opt.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
> +	opt.extended_regexp_option = 0;
> ...
> +
> +	if (pattern_type_arg > GREP_PATTERN_TYPE_UNSPECIFIED)
> +		grep_pattern_type_options(pattern_type_arg, &opt);
> +	else {
> +		if (opt.pattern_type_option > GREP_PATTERN_TYPE_UNSPECIFIED)
> +			grep_pattern_type_options(opt.pattern_type_option, &opt);
> +		else if (opt.extended_regexp_option)
> +			grep_pattern_type_options(GREP_PATTERN_TYPE_ERE, &opt);

Ok, so while reading the configuration and command line options, we
do not touch the real fields that affect how the comparison is done,
(namely: regflags, fixed, ...), and just "buffer" what we read so
far in extended_regexp_option and pattern_type_option, and at the
very end set the real fields to their appropriate values.

Much nicer than the drafts in earlier discussion.  One micronit is
that probably it is preferrable to do '!=' instead of '>', as we are
only trying to see if it is unspecified on the command line (hence
need to pay attention to configuration) and do not care how the
enums are ordered, but that is nothing that requires a re-roll.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 523d041..35d357d 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> ...
> +test_expect_success 'grep -G pattern with grep.patternType=fixed' '
> +	{
> +		echo "ab:a+b*c"
> +		echo "ab:a+bc"
> +	} >expected &&
> +	git \
> +		-c grep.patterntype=fixed \
> +		grep -G "a+b" ab >actual &&
> +	test_cmp expected actual
> +'

All the new tests in the script looked very well thought out.

I noticed that this particular one will still succeed when somebody
breaks your code to ignore the configuration (as -G "a+b" would give
the expected match) or give higher precedence to the configuration
(as fixed "a+b" also will give the expected match).  Not that it is
wrong to have such a test that is unlikely to catch certain kinds of
regressions in the suite, and the particular kind of breakage will
be caught by the next test (snipped) anyway.

Will queue.  Thanks.

Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

[J Smith]

On Fri, Aug 3, 2012 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Much nicer than the drafts in earlier discussion.  One micronit is
> that probably it is preferrable to do '!=' instead of '>', as we are
> only trying to see if it is unspecified on the command line (hence
> need to pay attention to configuration) and do not care how the
> enums are ordered, but that is nothing that requires a re-roll.

True enough. Old habits I guess, eh?

> All the new tests in the script looked very well thought out.
>
> I noticed that this particular one will still succeed when somebody
> breaks your code to ignore the configuration (as -G "a+b" would give
> the expected match) or give higher precedence to the configuration
> (as fixed "a+b" also will give the expected match).  Not that it is
> wrong to have such a test that is unlikely to catch certain kinds of
> regressions in the suite, and the particular kind of breakage will
> be caught by the next test (snipped) anyway.

Yeah, that test is kind of iffy, but does no harm I suppose.

> Will queue.  Thanks.

Great, thanks. Been an informative experience for a first-time git
patcher. Cheers.