[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2 00/12] nd/icase updates
From:       Duy Nguyen <pclouds () gmail ! com>
Date:       2016-06-30 15:45:52
Message-ID: CACsJy8BU0fDVR54hMpA6qVknj+QxWR9Z-i1gRgpaJ6hp+SB2xQ () mail ! gmail ! com
[Download RAW message or body]

On Mon, Jun 27, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]