{"thread":{"id":"42875","subject":"git-prompt.sh incompatible with non-basic global grep.patternType","startedAt":"2016-07-18T22:56:15Z","lastAt":"2016-07-25T16:16:45Z","messageCount":14,"participants":["Richard Soderberg","Johannes Schindelin","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291696","messageId":"CAEvc1UQvXKtQCXvCmt-774A84--bkK-sb94BtFeqDDr0Gsf7qw@mail.gmail.com","threadId":"42875","inReplyTo":null,"subject":"git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Richard Soderberg","fromEmail":"rsoderberg@gmail.com","sentAt":"2016-07-18T22:56:09Z","receivedAt":"2016-07-18T22:56:15Z","isPatch":false,"sender":{"key":"rsoderberg@gmail.com","avatar":null},"body":"Hi, I wanted to report something interesting that I found while\ntracing a severe slowdown in git-prompt.sh.\n\nhttps://github.com/git/git/commit/6d158cba282f22fa1548af1188f78042fed30aed#diff-f37c4f4a898819f0ca4b5ff69e81d4d9R141\n\nWay back in this commit, someone added a useful chunk of code that\nworks perfectly with svn+ssh:// URLs under basic regexes:\n\n+ local svn_upstream=($(git log --first-parent -1 \\\n+ --grep=\"^git-svn-id: \\(${svn_url_pattern:2}\\)\" 2>/dev/null))\n\nHowever, if I switch over to Perl regexes (or Extended):\n\ngit config --global grep.patternType perl\n\nThen the command runs for one wall clock second and shows incorrect\nresults on my repository. I eventually traced this to an issue with\nthe regular expression provided, assuming the svn repository url is\n\"svn+ssh://...\":\n\ngit log ... --grep=\"^git-svn-id: \\(svn+ssh://...\\)\" 2>/dev/null\n\nThe + sign isn't escaped in git-prompt.sh, which under non-basic\nregexes causes the match to fail entirely.\n\n - R.\n\nps. git log --basic-regexp does not fix the issue, as for unknown\nreasons (I'll start another thread) the command-line option doesn't\noverride grep.patternType.\n"},{"id":"291714","messageId":"alpine.DEB.2.20.1607191323010.3472@virtualbox","threadId":"42875","inReplyTo":"CAEvc1UQvXKtQCXvCmt-774A84--bkK-sb94BtFeqDDr0Gsf7qw@mail.gmail.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-19T11:24:19Z","receivedAt":"2016-07-19T11:24:29Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Richard,\n\nOn Mon, 18 Jul 2016, Richard Soderberg wrote:\n\n> Hi, I wanted to report something interesting that I found while tracing\n> a severe slowdown in git-prompt.sh.\n> \n> https://github.com/git/git/commit/6d158cba282f22fa1548af1188f78042fed30aed#diff-f37c4f4a898819f0ca4b5ff69e81d4d9R141\n> \n> Way back in this commit, someone added a useful chunk of code that works\n> perfectly with svn+ssh:// URLs under basic regexes:\n> \n> + local svn_upstream=($(git log --first-parent -1 \\ +\n> --grep=\"^git-svn-id: \\(${svn_url_pattern:2}\\)\" 2>/dev/null))\n> \n> However, if I switch over to Perl regexes (or Extended):\n> \n> git config --global grep.patternType perl\n> \n> Then the command runs for one wall clock second and shows incorrect\n> results on my repository. I eventually traced this to an issue with the\n> regular expression provided, assuming the svn repository url is\n> \"svn+ssh://...\":\n> \n> git log ... --grep=\"^git-svn-id: \\(svn+ssh://...\\)\" 2>/dev/null\n> \n> The + sign isn't escaped in git-prompt.sh, which under non-basic regexes\n> causes the match to fail entirely.\n> \n> - R.\n> \n> ps. git log --basic-regexp does not fix the issue, as for unknown\n> reasons (I'll start another thread) the command-line option doesn't\n> override grep.patternType.\n\nMaybe this helps?\n\n-- snipsnap --\ndiff --git a/contrib/completion/git-prompt.sh\nb/contrib/completion/git-prompt.sh\nindex 97eacd7..74be907 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -143,7 +143,7 @@ __git_ps1_show_upstream ()\n \t\t# get the upstream from the \"git-svn-id: ...\" in a commit\n \t\t# message\n \t\t# (git-svn uses essentially the same procedure internally)\n \t\tlocal -a svn_upstream\n-\t\tsvn_upstream=($(git log --first-parent -1 \\\n+\t\tsvn_upstream=($(git -c grep.patternType=default log --first-parent -1 \\\n \t\t\t\t\t--grep=\"^git-svn-id: \\(${svn_url_pattern#??}\\)\" 2>/dev/null))\n \t\tif [[ 0 -ne ${#svn_upstream[@]} ]]; then\n \t\t\tsvn_upstream=${svn_upstream[${#svn_upstream[@]} - 2]}\n\n"},{"id":"291719","messageId":"CAEvc1UT_gmkmOjhs8PMH+vBRE1gku5yYRPhPfOL_B_vyeh23_g@mail.gmail.com","threadId":"42875","inReplyTo":"alpine.DEB.2.20.1607191323010.3472@virtualbox","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Richard Soderberg","fromEmail":"rsoderberg@gmail.com","sentAt":"2016-07-19T14:14:50Z","receivedAt":"2016-07-19T14:14:56Z","isPatch":false,"sender":{"key":"rsoderberg@gmail.com","avatar":null},"body":"Aha! Yes, this works precisely as intended: the prompt works\ncorrectly, and quickly, with this change in place.\n\n - R.\n\nOn Tue, Jul 19, 2016 at 4:24 AM, Johannes Schindelin\n wrote:\n> Hi Richard,\n>\n> On Mon, 18 Jul 2016, Richard Soderberg wrote:\n>\n>> Hi, I wanted to report something interesting that I found while tracing\n>> a severe slowdown in git-prompt.sh.\n>>\n>> https://github.com/git/git/commit/6d158cba282f22fa1548af1188f78042fed30aed#diff-f37c4f4a898819f0ca4b5ff69e81d4d9R141\n>>\n>> Way back in this commit, someone added a useful chunk of code that works\n>> perfectly with svn+ssh:// URLs under basic regexes:\n>>\n>> + local svn_upstream=($(git log --first-parent -1 \\ +\n>> --grep=\"^git-svn-id: \\(${svn_url_pattern:2}\\)\" 2>/dev/null))\n>>\n>> However, if I switch over to Perl regexes (or Extended):\n>>\n>> git config --global grep.patternType perl\n>>\n>> Then the command runs for one wall clock second and shows incorrect\n>> results on my repository. I eventually traced this to an issue with the\n>> regular expression provided, assuming the svn repository url is\n>> \"svn+ssh://...\":\n>>\n>> git log ... --grep=\"^git-svn-id: \\(svn+ssh://...\\)\" 2>/dev/null\n>>\n>> The + sign isn't escaped in git-prompt.sh, which under non-basic regexes\n>> causes the match to fail entirely.\n>>\n>> - R.\n>>\n>> ps. git log --basic-regexp does not fix the issue, as for unknown\n>> reasons (I'll start another thread) the command-line option doesn't\n>> override grep.patternType.\n>\n> Maybe this helps?\n>\n> -- snipsnap --\n> diff --git a/contrib/completion/git-prompt.sh\n> b/contrib/completion/git-prompt.sh\n> index 97eacd7..74be907 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -143,7 +143,7 @@ __git_ps1_show_upstream ()\n> # get the upstream from the \"git-svn-id: ...\" in a commit\n> # message\n> # (git-svn uses essentially the same procedure internally)\n> local -a svn_upstream\n> - svn_upstream=($(git log --first-parent -1 \\\n> + svn_upstream=($(git -c grep.patternType=default log --first-parent -1 \\\n> --grep=\"^git-svn-id: \\(${svn_url_pattern#??}\\)\" 2>/dev/null))\n> if [[ 0 -ne ${#svn_upstream[@]} ]]; then\n> svn_upstream=${svn_upstream[${#svn_upstream[@]} - 2]}\n>\n"},{"id":"291721","messageId":"alpine.DEB.2.20.1607191634350.3472@virtualbox","threadId":"42875","inReplyTo":"CAEvc1UT_gmkmOjhs8PMH+vBRE1gku5yYRPhPfOL_B_vyeh23_g@mail.gmail.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-19T14:35:26Z","receivedAt":"2016-07-19T14:35:34Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Richard,\n\nOn Tue, 19 Jul 2016, Richard Soderberg wrote:\n\n> Aha! Yes, this works precisely as intended: the prompt works\n> correctly, and quickly, with this change in place.\n\nOkay, next step for you: read\nhttp://github.com/git-for-windows/git/blob/master/Documentation/SubmittingPatches\nand submit this patch with an informative commit message.\n\nCiao,\nJohannes\n"},{"id":"291808","messageId":"20160720134211.GA19359@sigill.intra.peff.net","threadId":"42875","inReplyTo":"CAEvc1UQvXKtQCXvCmt-774A84--bkK-sb94BtFeqDDr0Gsf7qw@mail.gmail.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-20T13:42:12Z","receivedAt":"2016-07-20T13:42:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2016 at 03:56:09PM -0700, Richard Soderberg wrote:\n\n> ps. git log --basic-regexp does not fix the issue, as for unknown\n> reasons (I'll start another thread) the command-line option doesn't\n> override grep.patternType.\n\nDscho gave a fix for your immediate issue, but this \"ps\" definitely\nseems like a bug to me. Command-line options should always take\nprecedence over config.\n\n-Peff\n"},{"id":"291853","messageId":"xmqqwpkgcbp9.fsf@gitster.mtv.corp.google.com","threadId":"42875","inReplyTo":"20160720134211.GA19359@sigill.intra.peff.net","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-20T20:10:42Z","receivedAt":"2016-07-20T20:10:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 18, 2016 at 03:56:09PM -0700, Richard Soderberg wrote:\n>\n>> ps. git log --basic-regexp does not fix the issue, as for unknown\n>> reasons (I'll start another thread) the command-line option doesn't\n>> override grep.patternType.\n>\n> Dscho gave a fix for your immediate issue, but this \"ps\" definitely\n> seems like a bug to me. Command-line options should always take\n> precedence over config.\n\nThis may fix it. I think the root cause is that logic to smear\n\"pattern type\" into various broken-down fields in grep_opt for the\nshort-hands like --basic-regexp option needs to leave \"I am setting\nthis short-hand\" mark to allow the grep_commit_pattern_type() that\nis done as the final step of the set-up sequence before we call\ncompile_grep_patterns() can take notice. The calls currently made\nto grep_set_pattern_type_option() when we parse \"--basic-regexp\" and\nfriends forgets to override the \"source of truth\" field and only\nupdates the broken-down fields.\n\nAn alternative may be to update places that parse \"--basic-regexp\"\nand friends to just write to .pattern_type_option without calling\ngrep_set_pattern_type_option(); that might be a cleaner, but I am\nnot feeling well today so I won't be able to do a deeper analysis\nright now.\n\n grep.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/grep.c b/grep.c\nindex 394c856..908ed3d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -203,6 +203,7 @@ void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct gr\n \t\topt->regflags &= ~REG_EXTENDED;\n \t\tbreak;\n \t}\n+\topt->pattern_type_option = pattern_type;\n }\n \n static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,\n\n"},{"id":"291858","messageId":"20160720205207.GA578@sigill.intra.peff.net","threadId":"42875","inReplyTo":"xmqqwpkgcbp9.fsf@gitster.mtv.corp.google.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-20T20:52:07Z","receivedAt":"2016-07-20T20:52:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 20, 2016 at 01:10:42PM -0700, Junio C Hamano wrote:\n\n> This may fix it. I think the root cause is that logic to smear\n> \"pattern type\" into various broken-down fields in grep_opt for the\n> short-hands like --basic-regexp option needs to leave \"I am setting\n> this short-hand\" mark to allow the grep_commit_pattern_type() that\n> is done as the final step of the set-up sequence before we call\n> compile_grep_patterns() can take notice. The calls currently made\n> to grep_set_pattern_type_option() when we parse \"--basic-regexp\" and\n> friends forgets to override the \"source of truth\" field and only\n> updates the broken-down fields.\n> \n> An alternative may be to update places that parse \"--basic-regexp\"\n> and friends to just write to .pattern_type_option without calling\n> grep_set_pattern_type_option(); that might be a cleaner, but I am\n> not feeling well today so I won't be able to do a deeper analysis\n> right now.\n\nI gave a very cursory look when I wrote the other email, and your\nalternative solution is what looked like the most elegant fix to me.\n\nI suspect this bug has been there quite a while, so no rush. :)\n\n-Peff\n"},{"id":"291992","messageId":"xmqqr3ala37o.fsf@gitster.mtv.corp.google.com","threadId":"42875","inReplyTo":"20160720205207.GA578@sigill.intra.peff.net","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T19:21:31Z","receivedAt":"2016-07-22T19:21:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I gave a very cursory look when I wrote the other email, and your\n> alternative solution is what looked like the most elegant fix to me.\n>\n> I suspect this bug has been there quite a while, so no rush. :)\n\nHere is what I am going to queue.\n\nOne thing that I noticed is that there is this strange field\nin grep_opt called .extended_regexp_option; it only is set from the\nboolean configuration grep.extendedregexp and worse yet it takes\nprecedence over the command line option or grep.patternType, e.g.\nt7810-grep expects crazy things like these:\n\n * \"git -c grep.extendedregexp=true -c grep.patterntype=basic grep\"\n wants to be like \"git grep -E\"\n\n * \"git -c grep.extendedregexp=false -c grep.patterntype=extended grep\"\n wants to be like \"git grep -G\"\n\nThis comes from b22520a3 (grep: allow -E and -n to be turned on by\ndefault via configuration, 2011-03-30) back when we didn't have a\nmore generic grep.patternType configuration mechanism in v1.7.5\ndays, and it probably need to be deprecated to maintain our sanity.\nI.e. when we see the configuration used, first we warn the user and\nset grep.patternType to extended instead, and then eventually error\nout in a backward-compatibility breaking release of Git we will make\nin some future date, together with things like other compatibility\nbreaking topics like ex/deprecate-empty-pathspec-as-match-all.\n\nBut that is a separate topic after this fix goes in anyway.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Fri, 22 Jul 2016 11:43:14 -0700\nSubject: [PATCH] grep: further simplify setting the pattern type\n\nWhen c5c31d33 (grep: move pattern-type bits support to top-level\ngrep.[ch], 2012-10-03) introduced grep_commit_pattern_type() helper\nfunction, the intention was to allow the users of grep API to having\nto fiddle only with .pattern_type_option (which can be set to \"fixed\",\n\"basic\", \"extended\", and \"pcre\"), and then immediately before compiling\nthe pattern strings for use, call grep_commit_pattern_type() to have\nit prepare various bits in the grep_opt structure (like .fixed,\n.regflags, etc.).\n\nHowever, grep_set_pattern_type_option() helper function the grep API\ninternally uses were left as an external function by mistake. This\nfunction shouldn't have been made callable by the users of the API.\n\nLater when the grep API was used in revision graversal machinery,\nthe caller then mistakenly started calling the function around\n34a4ae55 (log --grep: use the same helper to set -E/-F options as\n\"git grep\", 2012-10-03), instead of setting the .pattern_type_option\nfield and letting the grep_commit_pattern_type() to take care of the\ndetails.\n\nThis caused an unnecessary bug that made a configured\ngrep.patternType take precedence over the command line options\n(e.g. --basic-regexp, --fixed-strings) in \"git log\" family of\ncommands.\n\nSigned-off-by: Junio C Hamano \n---\n grep.c | 22 +++++++++++-----------\n grep.h | 1 -\n revision.c | 8 ++++----\n t/t4202-log.sh | 14 ++++++++++++++\n 4 files changed, 29 insertions(+), 16 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex b58c7c6..0bc5cc1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -161,17 +161,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \tstrcpy(opt->color_sep, def->color_sep);\n }\n \n-void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n-{\n-\tif (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED)\n-\t\tgrep_set_pattern_type_option(pattern_type, opt);\n-\telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n-\t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n-\telse if (opt->extended_regexp_option)\n-\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n-}\n-\n-void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n+static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \tswitch (pattern_type) {\n \tcase GREP_PATTERN_TYPE_UNSPECIFIED:\n@@ -203,6 +193,16 @@ void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct gr\n \t}\n }\n \n+void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n+{\n+\tif (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED)\n+\t\tgrep_set_pattern_type_option(pattern_type, opt);\n+\telse if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)\n+\t\tgrep_set_pattern_type_option(opt->pattern_type_option, opt);\n+\telse if (opt->extended_regexp_option)\n+\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n+}\n+\n static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,\n \t\t\t\t\tconst char *origin, int no,\n \t\t\t\t\tenum grep_pat_token t,\ndiff --git a/grep.h b/grep.h\nindex 95f197a..9e9ec5f 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -144,7 +144,6 @@ struct grep_opt {\n extern void init_grep_defaults(void);\n extern int grep_config(const char *var, const char *value, void *);\n extern void grep_init(struct grep_opt *, const char *prefix);\n-void grep_set_pattern_type_option(enum grep_pattern_type, struct grep_opt *opt);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n 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);\ndiff --git a/revision.c b/revision.c\nindex 871812d..c46dca4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1964,16 +1964,16 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--grep-debug\")) {\n \t\trevs->grep_filter.debug = 1;\n \t} else if (!strcmp(arg, \"--basic-regexp\")) {\n-\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_BRE, &revs->grep_filter);\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_BRE;\n \t} else if (!strcmp(arg, \"--extended-regexp\") || !strcmp(arg, \"-E\")) {\n-\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &revs->grep_filter);\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;\n \t} else if (!strcmp(arg, \"--regexp-ignore-case\") || !strcmp(arg, \"-i\")) {\n \t\trevs->grep_filter.regflags |= REG_ICASE;\n \t\tDIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);\n \t} else if (!strcmp(arg, \"--fixed-strings\") || !strcmp(arg, \"-F\")) {\n-\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_FIXED, &revs->grep_filter);\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;\n \t} else if (!strcmp(arg, \"--perl-regexp\")) {\n-\t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, &revs->grep_filter);\n+\t\trevs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;\n \t} else if (!strcmp(arg, \"--all-match\")) {\n \t\trevs->grep_filter.all_match = 1;\n \t} else if (!strcmp(arg, \"--invert-grep\")) {\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 1b2e981..f136a0f 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -232,6 +232,20 @@ test_expect_success 'log -F -E --grep= uses ere' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log with grep.patternType configuration' '\n+\t>expect &&\n+\tgit -c grep.patterntype=fixed \\\n+\tlog -1 --pretty=tformat:%s --grep=s.c.nd >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log with grep.patternType configuration and command line' '\n+\techo second >expect &&\n+\tgit -c grep.patterntype=fixed \\\n+\tlog -1 --pretty=tformat:%s --basic-regexp --grep=s.c.nd >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat > expect < One thing that I noticed is that there is this strange field\n> in grep_opt called .extended_regexp_option; it only is set from the\n> boolean configuration grep.extendedregexp and worse yet it takes\n> precedence over the command line option or grep.patternType, e.g.\n> t7810-grep expects crazy things like these:\n> \n> * \"git -c grep.extendedregexp=true -c grep.patterntype=basic grep\"\n> wants to be like \"git grep -E\"\n> \n> * \"git -c grep.extendedregexp=false -c grep.patterntype=extended grep\"\n> wants to be like \"git grep -G\"\n> \n> This comes from b22520a3 (grep: allow -E and -n to be turned on by\n> default via configuration, 2011-03-30) back when we didn't have a\n> more generic grep.patternType configuration mechanism in v1.7.5\n> days, and it probably need to be deprecated to maintain our sanity.\n> I.e. when we see the configuration used, first we warn the user and\n> set grep.patternType to extended instead, and then eventually error\n> out in a backward-compatibility breaking release of Git we will make\n> in some future date, together with things like other compatibility\n> breaking topics like ex/deprecate-empty-pathspec-as-match-all.\n> \n> But that is a separate topic after this fix goes in anyway.\n\nI am not even sure we need to deprecate it. Once it becomes merely a\nhistorical synonym for \"grep.patternType=extended\" we can live with it\nindefinitely (and I do not think we need a deprecation period to go\nthere; the existing behavior is simply buggy).\n\nNot that I mind eventually removing it, if you want to go through the\nsteps.\n\n> -- >8 --\n> From: Junio C Hamano \n> Date: Fri, 22 Jul 2016 11:43:14 -0700\n> Subject: [PATCH] grep: further simplify setting the pattern type\n> [...]\n\nThanks. This matches the cursory analysis I had done earlier, and the\npatch looks exactly as I had expected.\n\n-Peff\n"},{"id":"292001","messageId":"xmqqa8h9a1uj.fsf@gitster.mtv.corp.google.com","threadId":"42875","inReplyTo":"20160722192811.GA18079@sigill.intra.peff.net","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T19:51:00Z","receivedAt":"2016-07-22T19:51:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> This comes from b22520a3 (grep: allow -E and -n to be turned on by\n>> default via configuration, 2011-03-30) back when we didn't have a\n>> more generic grep.patternType configuration mechanism in v1.7.5\n>> days, and it probably need to be deprecated to maintain our sanity.\n>> ...\n> I am not even sure we need to deprecate it. Once it becomes merely a\n> historical synonym for \"grep.patternType=extended\" we can live with it\n> indefinitely (and I do not think we need a deprecation period to go\n> there; the existing behavior is simply buggy).\n\nI grossed over an important detail.\n\nPretending as if grep.patternType=extended were given when we see\ngrep.extendedregexp=true and grep.patternType=basic is given when\ngrep.extendedregexp=false changes the behaviour in a way that can be\nseen as the violation of (crazy) expectations t7810 makes.\n\nAny user who depends on that crazy expectation will be broken by\nsuch a change, even if we do not deprecate and remove the\nconfiguration variable.\n\n grep.c | 4 ++--\n grep.h | 1 -\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 394c856..7b1d423 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -73,9 +73,9 @@ int grep_config(const char *var, const char *value, void *cb)\n \n \tif (!strcmp(var, \"grep.extendedregexp\")) {\n \t\tif (git_config_bool(var, value))\n-\t\t\topt->extended_regexp_option = 1;\n+\t\t\topt->pattern_type_option = GREP_PATTERN_ERE;\n \t\telse\n-\t\t\topt->extended_regexp_option = 0;\n+\t\t\topt->pattern_type_option = GREP_PATTERN_BRE;\n \t\treturn 0;\n \t}\n \ndiff --git a/grep.h b/grep.h\nindex cee4357..fc36c2a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -119,7 +119,6 @@ struct grep_opt {\n \tint max_depth;\n \tint funcname;\n \tint funcbody;\n-\tint extended_regexp_option;\n \tint pattern_type_option;\n \tchar color_context[COLOR_MAXLEN];\n \tchar color_filename[COLOR_MAXLEN];\n"},{"id":"292012","messageId":"20160722200545.GA18286@sigill.intra.peff.net","threadId":"42875","inReplyTo":"xmqqa8h9a1uj.fsf@gitster.mtv.corp.google.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-22T20:05:45Z","receivedAt":"2016-07-22T20:05:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 22, 2016 at 12:51:00PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> This comes from b22520a3 (grep: allow -E and -n to be turned on by\n> >> default via configuration, 2011-03-30) back when we didn't have a\n> >> more generic grep.patternType configuration mechanism in v1.7.5\n> >> days, and it probably need to be deprecated to maintain our sanity.\n> >> ...\n> > I am not even sure we need to deprecate it. Once it becomes merely a\n> > historical synonym for \"grep.patternType=extended\" we can live with it\n> > indefinitely (and I do not think we need a deprecation period to go\n> > there; the existing behavior is simply buggy).\n> \n> I grossed over an important detail.\n> \n> Pretending as if grep.patternType=extended were given when we see\n> grep.extendedregexp=true and grep.patternType=basic is given when\n> grep.extendedregexp=false changes the behaviour in a way that can be\n> seen as the violation of (crazy) expectations t7810 makes.\n> \n> Any user who depends on that crazy expectation will be broken by\n> such a change, even if we do not deprecate and remove the\n> configuration variable.\n\nAh. Reading 84befcd (grep: add a grep.patternType configuration setting,\n2012-08-03) explains the rules, although I agree they are crazy (mostly\nbecause \"basic\" is different \"fixed\").\n\nSo I think there are two crazy things going on:\n\n 1. grep.extendedregexp takes precedence over the command-line (or at\n least \"--basic\" on the command line).\n\n 2. The weird rules in 84befcd, where patternType=fixed has higher\n precedence than extendedRegexp, but patternType=basic does not.\n\nIt seems like (1) is a bug, and one we should not have to worry about\ncompatibility while fixing. It stems from not telling the difference\nbetween \"the user asked for nothing, so we defaulted to basic\" and \"the\nuser explicitly asked for basic\".\n\nI think (2) _is_ kind of crazy, and stems from similar confusion. I\ndunno. Part of me wants to say \"I find it highly unlikely that anybody\never actually relied on this, and therefore it is OK to bring it closer\nto sanity without a deprecation period\". But I admit that is just a gut\nfeeling.\n\nBut even that is a lesser breakage than taking away grep.extendedRegexp\nentirely. Taking it away breaks anybody who uses it; tweaking (2) only\nbreaks people who set both config variables. But why would anyone do\nthat?\n\n-Peff\n"},{"id":"292016","messageId":"xmqqwpkd8lb9.fsf@gitster.mtv.corp.google.com","threadId":"42875","inReplyTo":"20160722200545.GA18286@sigill.intra.peff.net","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T20:33:30Z","receivedAt":"2016-07-22T20:33:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But even that is a lesser breakage than taking away grep.extendedRegexp\n> entirely. Taking it away breaks anybody who uses it; tweaking (2) only\n> breaks people who set both config variables. But why would anyone do\n> that?\n\nOK. Now we have an evidence that we have thought things through\nthat we can point at when people later complain, I'd feel much safer\nto apply that \"This will break t7810\" patch, together with changes\nto t7810 to drop these crazy expectations ;-)\n\n"},{"id":"292024","messageId":"CAPig+cSw2t36n3ir61BhcMbSVMYoTVrYwbyGVSPcLoGnwR4w2w@mail.gmail.com","threadId":"42875","inReplyTo":"xmqqr3ala37o.fsf@gitster.mtv.corp.google.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-07-22T23:28:22Z","receivedAt":"2016-07-22T23:28:27Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 22, 2016 at 3:21 PM, Junio C Hamano wrote:\n> Subject: [PATCH] grep: further simplify setting the pattern type\n>\n> When c5c31d33 (grep: move pattern-type bits support to top-level\n> grep.[ch], 2012-10-03) introduced grep_commit_pattern_type() helper\n> function, the intention was to allow the users of grep API to having\n> to fiddle only with .pattern_type_option (which can be set to \"fixed\",\n> \"basic\", \"extended\", and \"pcre\"), and then immediately before compiling\n> the pattern strings for use, call grep_commit_pattern_type() to have\n> it prepare various bits in the grep_opt structure (like .fixed,\n> .regflags, etc.).\n>\n> However, grep_set_pattern_type_option() helper function the grep API\n> internally uses were left as an external function by mistake. This\n> function shouldn't have been made callable by the users of the API.\n>\n> Later when the grep API was used in revision graversal machinery,\n\ns/graversal/traversal/\n\n> the caller then mistakenly started calling the function around\n> 34a4ae55 (log --grep: use the same helper to set -E/-F options as\n> \"git grep\", 2012-10-03), instead of setting the .pattern_type_option\n> field and letting the grep_commit_pattern_type() to take care of the\n> details.\n>\n> This caused an unnecessary bug that made a configured\n> grep.patternType take precedence over the command line options\n> (e.g. --basic-regexp, --fixed-strings) in \"git log\" family of\n> commands.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"292100","messageId":"xmqqshux7kwr.fsf@gitster.mtv.corp.google.com","threadId":"42875","inReplyTo":"CAPig+cSw2t36n3ir61BhcMbSVMYoTVrYwbyGVSPcLoGnwR4w2w@mail.gmail.com","subject":"Re: git-prompt.sh incompatible with non-basic global grep.patternType","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-25T16:16:36Z","receivedAt":"2016-07-25T16:16:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Later when the grep API was used in revision graversal machinery,\n>\n> s/graversal/traversal/\n\nThanks.\n"}]}