git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 00/12] nd/icase updates

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 27, 2016, 14:53 UTC
Message-ID
<xmqqeg7id6ns.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20160625052238.13615-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 6 quoted lines
> v2 fixes Junio's and Jeff's comments (both good). The sharing "!icase
> || ascii_only" is made a separate commit (6/12) because I think it
> takes some seconds to realize that the conversion is correct and it's
> technically not needed in 5/12 (and it's sort of the opposite of 1/12)
>
> Interdiff
OK.  regcomp_or_die() does make the code simpler.
Show 21 quoted lines
> 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);
 	if (opt->debug)
 		fprintf(stderr, "fixed %s\n", sb.buf);
 	strbuf_release(&sb);
 	if (err) {
 		char errbuf[1024];
 		regerror(err, &p->regexp, errbuf, sizeof(errbuf));
 		regfree(&p->regexp);
 		compile_regexp_failed(p, errbuf);
 	}
 }
@@ -425,38 +429,55 @@ 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 err;
 
 	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);
 
+	/*
+	 * Even when -F (fixed) asks us to do a non-regexp search, we
+	 * may not be able to correctly case-fold when -i
+	 * (ignore-case) is asked (in which case, we'll synthesize a
+	 * regexp to match the pattern that matches regexp special
+	 * characters literally, while ignoring case differences).  On
+	 * the other hand, even without -F, if the pattern does not
+	 * have any regexp special characters and there is no need for
+	 * case-folding search, we can internally turn it into a
+	 * simple string match using kws.  p->fixed tells us if we
+	 * want to use kws.
+	 */
 	if (opt->fixed || 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);
 		kwsincr(p->kws, p->pattern, p->patternlen);
 		kwsprep(p->kws);
 		return;
 	} else if (opt->fixed) {
+		/*
+		 * We only come here when the pattern has the regexp
+		 * special characters in it, which need to be matched
+		 * literally, while ignoring case.
+		 */
 		compile_fixed_regexp(p, opt);
 		return;
 	}
 
 	if (opt->pcre) {
 		compile_pcre_regexp(p, opt);
 		return;
 	}
 
 	err = regcomp(&p->regexp, p->pattern, opt->regflags);
 	if (err) {
 		char errbuf[1024];
 		regerror(err, &p->regexp, errbuf, 1024);
 		regfree(&p->regexp);
 		compile_regexp_failed(p, errbuf);
 	}
 }
Previous: Nguyễn Thái Ngọc DuyNext: Junio C Hamano
Message 30 of 39 in “nd/icase updates”
  1. 00/11 nd/icase updatesNguyễn Thái Ngọc Duy, Jun 23, 2016
  2. 02/11 test-regex: isolate the bug test codeNguyễn Thái Ngọc Duy, Jun 23, 2016
  3. 05/11 grep/icase: avoid kwsset when -F is specifiedNguyễn Thái Ngọc Duy, Jun 23, 2016
  4. Junio C HamanoJun 23, 2016
  5. 09/11 diffcore-pickaxe: "share" regex error handling codeNguyễn Thái Ngọc Duy, Jun 23, 2016
  6. Jeff KingJun 23, 2016
  7. 11/11 grep.c: reuse "icase" variableNguyễn Thái Ngọc Duy, Jun 23, 2016
  8. 03/11 test-regex: expose full regcomp() to the command lineNguyễn Thái Ngọc Duy, Jun 23, 2016
  9. 08/11 grep/pcre: support utf-8Nguyễn Thái Ngọc Duy, Jun 23, 2016
  10. 10/11 diffcore-pickaxe: support case insensitive match on non-asciiNguyễn Thái Ngọc Duy, Jun 23, 2016
  11. 07/11 gettext: add is_utf8_locale()Nguyễn Thái Ngọc Duy, Jun 23, 2016
  12. 01/11 grep: break down an "if" stmt in preparation for next changesNguyễn Thái Ngọc Duy, Jun 23, 2016
  13. 04/11 grep/icase: avoid kwsset on literal non-ascii stringsNguyễn Thái Ngọc Duy, Jun 23, 2016
  14. Junio C HamanoJun 23, 2016
  15. 06/11 grep/pcre: prepare locale-dependent tables for icase matchingNguyễn Thái Ngọc Duy, Jun 23, 2016
  16. Junio C HamanoJun 23, 2016
  17. 00/12 nd/icase updatesNguyễn Thái Ngọc Duy, Jun 25, 2016
  18. 03/12 test-regex: expose full regcomp() to the command lineNguyễn Thái Ngọc Duy, Jun 25, 2016
  19. 04/12 grep/icase: avoid kwsset on literal non-ascii stringsNguyễn Thái Ngọc Duy, Jun 25, 2016
  20. 07/12 grep/pcre: prepare locale-dependent tables for icase matchingNguyễn Thái Ngọc Duy, Jun 25, 2016
  21. 06/12 grep: rewrite an if/else condition to avoid duplicate expressionNguyễn Thái Ngọc Duy, Jun 25, 2016
  22. 10/12 diffcore-pickaxe: Add regcomp_or_die()Nguyễn Thái Ngọc Duy, Jun 25, 2016
  23. 09/12 grep/pcre: support utf-8Nguyễn Thái Ngọc Duy, Jun 25, 2016
  24. 05/12 grep/icase: avoid kwsset when -F is specifiedNguyễn Thái Ngọc Duy, Jun 25, 2016
  25. 12/12 grep.c: reuse "icase" variableNguyễn Thái Ngọc Duy, Jun 25, 2016
  26. 08/12 gettext: add is_utf8_locale()Nguyễn Thái Ngọc Duy, Jun 25, 2016
  27. 01/12 grep: break down an "if" stmt in preparation for next changesNguyễn Thái Ngọc Duy, Jun 25, 2016
  28. 11/12 diffcore-pickaxe: support case insensitive match on non-asciiNguyễn Thái Ngọc Duy, Jun 25, 2016
  29. 02/12 test-regex: isolate the bug test codeNguyễn Thái Ngọc Duy, Jun 25, 2016
  30. Junio C HamanoJun 27, 2016
  31. Junio C HamanoJun 27, 2016
  32. Duy NguyenJun 30, 2016
  33. Junio C HamanoJul 1, 2016
  34. Duy NguyenJul 1, 2016
  35. Junio C HamanoJul 1, 2016
  36. Junio C HamanoJul 1, 2016
  37. Junio C HamanoJul 1, 2016
  38. Junio C HamanoJul 1, 2016
  39. Junio C HamanoJul 1, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.