Re: [PATCH v3 6/6] git-grep: Bail out when -P is used with -F or -E

[Michal Kiedrowicz <michal.kiedrowicz@gmail.com>]

On 09.05.2011 18:48:36 -0700 Junio C Hamano <gitster@pobox.com> wrote:

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > This also makes it bail out when grep.extendedRegexp is enabled.
> 
> That is a no-starter.  "git grep -P foo" from the command line should
> just ignore the configured default.  It is not entirely your fault,
> as I think you inherited the bug from the existing code that lets
> grep.extendedRegexp interact with the "--fixed" option from the
> command line.
> 
> > But `git grep -G -P pattern` and `git grep -E -G -P pattern` still
> > work because -G and -E set opts.regflags during parse_options() and
> > there is no way to detect `-G` or `-E -G`.
> 
> How about following the usual pattern of letting the last one win?
> 
> Perhaps like this?  This is not even compile tested, but should apply
> cleanly on top of, and can be squashed into, your 6/6.  You of course
> would need to rewrite the commit log message and documentation, if you
> said only one of these can be used.

Sounds good, thanks.

> 
> We would need some tests for "grep -P", no?  

What about those in patch 5/6?

> Please throw in the
> "last one wins" and "command line defeats configuration" when you add
> one.
> 
> Also I deliberately said "--ignore-case and -P are not compatible
> (yet)"; shouldn't you be able to do ignore case fairly easily, I
> wonder?  Isn't it just the matter of wrapping each one with "(?i:"
> and ")" pair, or anything more involved necessary?

If you look at patch 3/6 you will see:

+
+	if (opt->ignore_case)
+		options |= PCRE_CASELESS;
+
+	p->pcre_regexp = pcre_compile(p->pattern, options, &error,
&erroffset,
+			NULL);

and also

+test_expect_success LIBPCRE 'grep -P -i pattern' '

in patch 5/6 :). Or perhaps it doesn't work for you?

> 
>  builtin/grep.c |   58
> +++++++++++++++++++++++++++++++++++++++++++------------ 1 files
> changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8e422b3..37f2331 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -753,6 +753,15 @@ 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;
> +
>  	struct option options[] = {
>  		OPT_BOOLEAN(0, "cached", &cached,
>  			"search in index instead of in the work
> tree"), @@ -774,15 +783,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_BIT('E', "extended-regexp", &opt.regflags,
> -			"use extended POSIX regular expressions",
> REG_EXTENDED),
> -		OPT_NEGBIT('G', "basic-regexp", &opt.regflags,
> -			"use basic POSIX regular expressions
> (default)",
> -			REG_EXTENDED),
> -		OPT_BOOLEAN('F', "fixed-strings", &opt.fixed,
> -			"interpret patterns as fixed strings"),
> -		OPT_BOOLEAN('P', "perl-regexp", &opt.pcre,
> -				"use Perl-compatible regular
> expressions"),
> +		OPT_SET_INT('E', "extended-regexp", &pattern_type,
> +			    "use extended POSIX regular expressions",
> +			    pattern_type_ere),
> +		OPT_SET_INT('G', "basic-regexp", &pattern_type,
> +			    "use basic POSIX regular expressions
> (default)",
> +			    pattern_type_bre),
> +		OPT_SET_INT('F', "fixed-strings", &pattern_type,
> +			    "interpret patterns as fixed strings",
> +			    pattern_type_fixed),
> +		OPT_SET_INT('P', "perl-regexp", &pattern_type,
> +			    "use Perl-compatible regular
> expressions",
> +			    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), @@ -888,6 +900,28 @@ 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 (use_index && !startup_info->have_repository)
>  		/* die the same way as if we did it at the beginning
> */ @@ -925,12 +959,10 @@ int cmd_grep(int argc, const char **argv,
> const char *prefix) 
>  	if (!opt.pattern_list)
>  		die(_("no pattern given."));
> -	if (opt.regflags != REG_NEWLINE && opt.pcre)
> -		die(_("cannot mix --extended-regexp and
> --perl-regexp")); if (!opt.fixed && opt.ignore_case)
>  		opt.regflags |= REG_ICASE;
> -	if ((opt.regflags != REG_NEWLINE || opt.pcre) && opt.fixed)
> -		die(_("cannot mix --fixed-strings and regexp"));
> +	if (opt.pcre && opt.ignore_case)
> +		die(_("--ignore-case and -P are not compatible
> (yet)")); 
>  #ifndef NO_PTHREADS
>  	if (online_cpus() == 1 || !grep_threads_ok(&opt))
>