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

Re: git-prompt.sh incompatible with non-basic global grep.patternType

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 22, 2016, 19:21 UTC
Message-ID
<xmqqr3ala37o.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20160720205207.GA578@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I gave a very cursory look when I wrote the other email, and your
> alternative solution is what looked like the most elegant fix to me.
>
> I suspect this bug has been there quite a while, so no rush. :)
Here is what I am going to queue.

One thing that I noticed is that there is this strange field in grep_opt called .extended_regexp_option; it only is set from the boolean configuration grep.extendedregexp and worse yet it takes precedence over the command line option or grep.patternType, e.g. t7810-grep expects crazy things like these:

 * "git -c grep.extendedregexp=true -c grep.patterntype=basic grep"
   wants to be like "git grep -E"
 * "git -c grep.extendedregexp=false -c grep.patterntype=extended grep"
   wants to be like "git grep -G"

This comes from b22520a3 (grep: allow -E and -n to be turned on by default via configuration, 2011-03-30) back when we didn't have a more generic grep.patternType configuration mechanism in v1.7.5 days, and it probably need to be deprecated to maintain our sanity. I.e. when we see the configuration used, first we warn the user and set grep.patternType to extended instead, and then eventually error out in a backward-compatibility breaking release of Git we will make in some future date, together with things like other compatibility breaking topics like ex/deprecate-empty-pathspec-as-match-all.

But that is a separate topic after this fix goes in anyway.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 22 Jul 2016 11:43:14 -0700
Subject: [PATCH] grep: further simplify setting the pattern type

When c5c31d33 (grep: move pattern-type bits support to top-level grep.[ch], 2012-10-03) introduced grep_commit_pattern_type() helper function, the intention was to allow the users of grep API to having to fiddle only with .pattern_type_option (which can be set to "fixed", "basic", "extended", and "pcre"), and then immediately before compiling the pattern strings for use, call grep_commit_pattern_type() to have it prepare various bits in the grep_opt structure (like .fixed, .regflags, etc.).

However, grep_set_pattern_type_option() helper function the grep API internally uses were left as an external function by mistake. This function shouldn't have been made callable by the users of the API.

Later when the grep API was used in revision graversal machinery, the caller then mistakenly started calling the function around 34a4ae55 (log --grep: use the same helper to set -E/-F options as "git grep", 2012-10-03), instead of setting the .pattern_type_option field and letting the grep_commit_pattern_type() to take care of the details.

This caused an unnecessary bug that made a configured grep.patternType take precedence over the command line options (e.g. --basic-regexp, --fixed-strings) in "git log" family of commands.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 grep.c         | 22 +++++++++++-----------
 grep.h         |  1 -
 revision.c     |  8 ++++----
 t/t4202-log.sh | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/grep.c b/grep.c
index b58c7c6..0bc5cc1 100644
--- a/grep.c
+++ b/grep.c
@@ -161,17 +161,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	strcpy(opt->color_sep, def->color_sep);
 }
 
-void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt)
-{
-	if (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED)
-		grep_set_pattern_type_option(pattern_type, opt);
-	else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
-		grep_set_pattern_type_option(opt->pattern_type_option, opt);
-	else if (opt->extended_regexp_option)
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
-}
-
-void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
+static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	switch (pattern_type) {
 	case GREP_PATTERN_TYPE_UNSPECIFIED:
@@ -203,6 +193,16 @@ void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct gr
 	}
 }
 
+void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt)
+{
+	if (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED)
+		grep_set_pattern_type_option(pattern_type, opt);
+	else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
+		grep_set_pattern_type_option(opt->pattern_type_option, opt);
+	else if (opt->extended_regexp_option)
+		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
+}
+
 static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,
 					const char *origin, int no,
 					enum grep_pat_token t,
diff --git a/grep.h b/grep.h
index 95f197a..9e9ec5f 100644
--- a/grep.h
+++ b/grep.h
@@ -144,7 +144,6 @@ struct grep_opt {
 extern void init_grep_defaults(void);
 extern int grep_config(const char *var, const char *value, void *);
 extern void grep_init(struct grep_opt *, const char *prefix);
-void grep_set_pattern_type_option(enum grep_pattern_type, struct grep_opt *opt);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 extern void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
diff --git a/revision.c b/revision.c
index 871812d..c46dca4 100644
--- a/revision.c
+++ b/revision.c
@@ -1964,16 +1964,16 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--grep-debug")) {
 		revs->grep_filter.debug = 1;
 	} else if (!strcmp(arg, "--basic-regexp")) {
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_BRE, &revs->grep_filter);
+		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_BRE;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &revs->grep_filter);
+		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
 		revs->grep_filter.regflags |= REG_ICASE;
 		DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_FIXED, &revs->grep_filter);
+		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;
 	} else if (!strcmp(arg, "--perl-regexp")) {
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, &revs->grep_filter);
+		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;
 	} else if (!strcmp(arg, "--all-match")) {
 		revs->grep_filter.all_match = 1;
 	} else if (!strcmp(arg, "--invert-grep")) {
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..f136a0f 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -232,6 +232,20 @@ test_expect_success 'log -F -E --grep=<ere> uses ere' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log with grep.patternType configuration' '
+	>expect &&
+	git -c grep.patterntype=fixed \
+	log -1 --pretty=tformat:%s --grep=s.c.nd >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log with grep.patternType configuration and command line' '
+	echo second >expect &&
+	git -c grep.patterntype=fixed \
+	log -1 --pretty=tformat:%s --basic-regexp --grep=s.c.nd >actual &&
+	test_cmp expect actual
+'
+
 cat > expect <<EOF
 * Second
 * sixth

-- 
2.9.2-587-gac3bda4
Previous: Jeff KingNext: Jeff King
Message 8 of 14 in “git-prompt.sh incompatible with non-basic global grep.patternType”
  1. Richard SoderbergJul 18, 2016
  2. Johannes SchindelinJul 19, 2016
  3. Richard SoderbergJul 19, 2016
  4. Johannes SchindelinJul 19, 2016
  5. Jeff KingJul 20, 2016
  6. Junio C HamanoJul 20, 2016
  7. Jeff KingJul 20, 2016
  8. Junio C HamanoJul 22, 2016
  9. Jeff KingJul 22, 2016
  10. Junio C HamanoJul 22, 2016
  11. Jeff KingJul 22, 2016
  12. Junio C HamanoJul 22, 2016
  13. Eric SunshineJul 22, 2016
  14. Junio C HamanoJul 25, 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.