On Mon, Jun 27, 2016 at 4:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> diff --git a/grep.c b/grep.c
>> index cb058a5..92587a8 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -432,15 +432,8 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>> icase = opt->regflags & REG_ICASE || p->ignore_case;
>> ascii_only = !has_non_ascii(p->pattern);
>>
>> + if (opt->fixed || is_fixed(p->pattern, p->patternlen))
>> p->fixed = !icase || ascii_only;
>> else
>> p->fixed = 0;
>>
>> @@ -449,6 +442,9 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>> kwsincr(p->kws, p->pattern, p->patternlen);
>> kwsprep(p->kws);
>> return;
>> + } else if (opt->fixed) {
>> + compile_fixed_regexp(p, opt);
>> + return;
>> }
>
> This if/elseif/else cascade made a lot simpler and while the
> discussion is fresh in my brain it makes sense, but it may deserve a
> bit of commenting.
>
> And while attempting to do so, I found one possible issue there.
>
> Can't p->ignore_case be true even when opt->regflags does not have
> REG_ICASE? The user never asked us to do a regexp match in such a
> case, and the logical place to compensate for that case would be
> inside compile_fixed_regexp(), where we use regexp engine behind
> user's back for our convenience, I would think.
>
> In the current code, compile_fixed_regexp() is only called when we
> want ICASE, but hardcoding that assumption to it unnecessarily robs
> flexibility (and the function name does not tell us it is only for
> icase in the first place), so I taught it to do the REG_ICASE thing
> only when opt->ignore_case is set.
>
> How does this look?
>
>
> grep.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 92587a8..3a3a9f4 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -407,17 +407,21 @@ static int is_fixed(const char *s, size_t len)
> static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
> {
> struct strbuf sb = STRBUF_INIT;
> int err;
> + int regflags;
>
> basic_regex_quote_buf(&sb, p->pattern);
> - err = regcomp(&p->regexp, sb.buf, opt->regflags & ~REG_EXTENDED);
> + regflags = opt->regflags & ~REG_EXTENDED;
> + if (opt->ignore_case)
> + regflags |= REG_ICASE;
> + err = regcomp(&p->regexp, sb.buf, regflags);
Makes sense. But then if opt->ignore_case is false and regflags
happens to have REG_ICASE set, should we clear it as well?
The rest looks good (after your comment fixup). I see you already have
all the changes in your SQUASH??? commit. Do you want me to resend or
you will just squash this in locally?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html