{"thread":{"id":"61176","subject":"[PATCH] grep: improve errors for unmatched ( and )","startedAt":"2024-03-22T08:34:46Z","lastAt":"2024-04-09T10:08:04Z","messageCount":5,"participants":["Ahelenia Ziemiańska","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491222","messageId":"petpdy3bs6wpfd3ilrg6xrjsbj5y7ql4geps6y22ozdqw7vi4k@tarta.nabijaczleweli.xyz","threadId":"61176","inReplyTo":null,"subject":"[PATCH] grep: improve errors for unmatched ( and )","fromName":"Ahelenia Ziemiańska","fromEmail":"nabijaczleweli@nabijaczleweli.xyz","sentAt":"2024-03-22T08:34:38Z","receivedAt":"2024-03-22T08:34:46Z","isPatch":true,"sender":{"key":"nabijaczleweli@nabijaczleweli.xyz","avatar":"https://avatars.githubusercontent.com/u/6709544?v=4"},"body":"Imagine you want to grep for (. Easy:\n  $ git grep '('\n  fatal: unmatched parenthesis\nuhoh. This is plainly wrong. Unless you know specifically that\n(a) git grep has expression groups and that\n(b) the only way to work around them is by doing -- '(' or -e '('\n\nSimilarly,\n  $ git grep ')'\n  fatal: incomplete pattern expression: )\nis somehow worse. \")\" is a complete regular expression pattern.\nOf course, the error wants to say \"group\" here.\nIn this case it's also not \"incomplete\", it's unmatched.\nBut whatever.\n\nThese now return\n  $ ./git grep '('\n  fatal: unmatched ( for expression group\n  $ ./git grep ')'\n  fatal: incomplete pattern expression group: )\nwhich hopefully are clearer in indicating that it's not the expression\nthat's wrong (since no pattern had been parsed at all), but rather that\nit's been misconstrued as a grouping operator.\n\nLink: https://bugs.debian.org/1051205\nSigned-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>\n---\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5f23d1a..ac34bfe 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n \t\t*list = p->next;\n \t\tx = compile_pattern_or(list);\n \t\tif (!*list || (*list)->token != GREP_CLOSE_PAREN)\n-\t\t\tdie(\"unmatched parenthesis\");\n+\t\t\tdie(\"unmatched ( for expression group\");\n \t\t*list = (*list)->next;\n \t\treturn x;\n \tdefault:\n@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n \tif (p)\n \t\topt->pattern_expression = compile_pattern_expr(&p);\n \tif (p)\n-\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n+\t\tdie(\"incomplete pattern expression group: %s\", p->pattern);\n \n \tif (opt->no_body_match && opt->pattern_expression)\n \t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n-- \n2.39.2\n"},{"id":"491250","messageId":"xmqq34si2p8b.fsf@gitster.g","threadId":"61176","inReplyTo":"petpdy3bs6wpfd3ilrg6xrjsbj5y7ql4geps6y22ozdqw7vi4k@tarta.nabijaczleweli.xyz","subject":"Re: [PATCH] grep: improve errors for unmatched ( and )","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T16:41:40Z","receivedAt":"2024-03-22T16:41:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:\n\n> Imagine you want to grep for (. Easy:\n\nPlease have a blank line before and \n\n>   $ git grep '('\n>   fatal: unmatched parenthesis\n\nafter a displayed text like this one (this applies to a few more\nparagraphs in the proposed log message).\n\n> uhoh. This is plainly wrong. Unless you know specifically that\n> (a) git grep has expression groups and that\n> (b) the only way to work around them is by doing -- '(' or -e '('\n\nI do not think \"--\" (end of options and beginning of pathspec)\nmarker would work for that purpose, UNLESS you are talking about a\nfile whose name is an open parenthesis.  Just keep \"-e '('\" in the\ndescription and drop the double-dash there.\n\n> Similarly,\n>   $ git grep ')'\n>   fatal: incomplete pattern expression: )\n> is somehow worse. \")\" is a complete regular expression pattern.\n> Of course, the error wants to say \"group\" here.\n\nNice problem description so far.\n\n> These now return\n\nPhrase it more like \"Make them return\" ...\n\n>   $ ./git grep '('\n>   fatal: unmatched ( for expression group\n>   $ ./git grep ')'\n>   fatal: incomplete pattern expression group: )\n\n> which hopefully are clearer in indicating that it's not the expression\n> that's wrong (since no pattern had been parsed at all), but rather that\n> it's been misconstrued as a grouping operator.\n\nNicely done.\n\n> Link: https://bugs.debian.org/1051205\n> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>\n> ---\n>  grep.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 5f23d1a..ac34bfe 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n>  \t\t*list = p->next;\n>  \t\tx = compile_pattern_or(list);\n>  \t\tif (!*list || (*list)->token != GREP_CLOSE_PAREN)\n> -\t\t\tdie(\"unmatched parenthesis\");\n> +\t\t\tdie(\"unmatched ( for expression group\");\n>  \t\t*list = (*list)->next;\n>  \t\treturn x;\n>  \tdefault:\n> @@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n>  \tif (p)\n>  \t\topt->pattern_expression = compile_pattern_expr(&p);\n>  \tif (p)\n> -\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n> +\t\tdie(\"incomplete pattern expression group: %s\", p->pattern);\n>  \n>  \tif (opt->no_body_match && opt->pattern_expression)\n>  \t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n\nThanks.  The changes to these two messages look good.\n"},{"id":"491297","messageId":"tkz3a5jkalcz5ajemx4b4x42pe6kv45sfmgpin4zeai3moq42o@tarta.nabijaczleweli.xyz","threadId":"61176","inReplyTo":"xmqq34si2p8b.fsf@gitster.g","subject":"[PATCH v2] grep: improve errors for unmatched ( and )","fromName":"Ahelenia Ziemiańska","fromEmail":"nabijaczleweli@nabijaczleweli.xyz","sentAt":"2024-03-23T13:18:08Z","receivedAt":"2024-03-23T13:18:16Z","isPatch":true,"sender":{"key":"nabijaczleweli@nabijaczleweli.xyz","avatar":"https://avatars.githubusercontent.com/u/6709544?v=4"},"body":"Imagine you want to grep for (. Easy:\n\n  $ git grep '('\n  fatal: unmatched parenthesis\n\nuhoh. This is plainly wrong. Unless you know specifically that\n(a) git grep has expression groups and that\n(b) the only way to work around them is by doing -- '(' or -e '('\n\nSimilarly,\n\n  $ git grep ')'\n  fatal: incomplete pattern expression: )\n\nis somehow worse. \")\" is a complete regular expression pattern.\nOf course, the error wants to say \"group\" here.\nIn this case it's also not \"incomplete\", it's unmatched.\nBut whatever.\n\nMake them return\n\n  $ ./git grep '('\n  fatal: unmatched ( for expression group\n  $ ./git grep ')'\n  fatal: incomplete pattern expression group: )\n\nwhich hopefully are clearer in indicating that it's not the expression\nthat's wrong (since no pattern had been parsed at all), but rather that\nit's been misconstrued as a grouping operator.\n\nLink: https://bugs.debian.org/1051205\nSigned-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>\n---\nOn Fri, Mar 22, 2024 at 09:41:40AM -0700, Junio C Hamano wrote:\n> Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:\n> > uhoh. This is plainly wrong. Unless you know specifically that\n> > (a) git grep has expression groups and that\n> > (b) the only way to work around them is by doing -- '(' or -e '('\n> I do not think \"--\" (end of options and beginning of pathspec)\n> marker would work for that purpose, UNLESS you are talking about a\n> file whose name is an open parenthesis.\nFalse. -- turns all subsequent parameters into arguments, and if\nthere is no -e, the first argument is the pattern, and all the\nsubsequent ones are paths. This is normal [git] grep behaviour.\n\n> Just keep \"-e '('\" in the\n> description and drop the double-dash there.\nDisagree. This is one of the two methodologies I've devised to work\naround this in the past, and it's one of the two methodologies that\nwork.\n\nAll else applied.\n\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5f23d1a..ac34bfe 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n \t\t*list = p->next;\n \t\tx = compile_pattern_or(list);\n \t\tif (!*list || (*list)->token != GREP_CLOSE_PAREN)\n-\t\t\tdie(\"unmatched parenthesis\");\n+\t\t\tdie(\"unmatched ( for expression group\");\n \t\t*list = (*list)->next;\n \t\treturn x;\n \tdefault:\n@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n \tif (p)\n \t\topt->pattern_expression = compile_pattern_expr(&p);\n \tif (p)\n-\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n+\t\tdie(\"incomplete pattern expression group: %s\", p->pattern);\n \n \tif (opt->no_body_match && opt->pattern_expression)\n \t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n-- \n2.39.2\n"},{"id":"491308","messageId":"xmqqzfuox1oi.fsf@gitster.g","threadId":"61176","inReplyTo":"tkz3a5jkalcz5ajemx4b4x42pe6kv45sfmgpin4zeai3moq42o@tarta.nabijaczleweli.xyz","subject":"Re: [PATCH v2] grep: improve errors for unmatched ( and )","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-23T18:06:53Z","receivedAt":"2024-03-23T18:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:\n\n> Imagine you want to grep for (. Easy:\n>\n>   $ git grep '('\n>   fatal: unmatched parenthesis\n>\n> uhoh. This is plainly wrong. Unless you know specifically that\n> (a) git grep has expression groups and that\n> (b) the only way to work around them is by doing -- '(' or -e '('\n>\n> Similarly,\n>\n>   $ git grep ')'\n>   fatal: incomplete pattern expression: )\n>\n> is somehow worse. \")\" is a complete regular expression pattern.\n> Of course, the error wants to say \"group\" here.\n> In this case it's also not \"incomplete\", it's unmatched.\n> But whatever.\n>\n> Make them return\n>\n>   $ ./git grep '('\n>   fatal: unmatched ( for expression group\n>   $ ./git grep ')'\n>   fatal: incomplete pattern expression group: )\n>\n> which hopefully are clearer in indicating that it's not the expression\n> that's wrong (since no pattern had been parsed at all), but rather that\n> it's been misconstrued as a grouping operator.\n>\n> Link: https://bugs.debian.org/1051205\n> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>\n> ---\n> On Fri, Mar 22, 2024 at 09:41:40AM -0700, Junio C Hamano wrote:\n>> Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:\n>> > uhoh. This is plainly wrong. Unless you know specifically that\n>> > (a) git grep has expression groups and that\n>> > (b) the only way to work around them is by doing -- '(' or -e '('\n>> I do not think \"--\" (end of options and beginning of pathspec)\n>> marker would work for that purpose, UNLESS you are talking about a\n>> file whose name is an open parenthesis.\n> False. -- turns all subsequent parameters into arguments, and if\n> there is no -e, the first argument is the pattern, and all the\n> subsequent ones are paths. This is normal [git] grep behaviour.\n\nAh, thanks.  \n\nI forgot that \"git grep\" was a oddball that allows revs come after\n\"--\" in some cases.  As long as the user understands this may not\nwork for other commands, it is OK.  \"-e\" is the only officially\nsupported way (which is why I mentioned it in the review comments I\ngave you here), so guiding users in that direction would be a better\nidea anyway, though.\n\nThanks.\n\n> diff --git a/grep.c b/grep.c\n> index 5f23d1a..ac34bfe 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n>  \t\t*list = p->next;\n>  \t\tx = compile_pattern_or(list);\n>  \t\tif (!*list || (*list)->token != GREP_CLOSE_PAREN)\n> -\t\t\tdie(\"unmatched parenthesis\");\n> +\t\t\tdie(\"unmatched ( for expression group\");\n>  \t\t*list = (*list)->next;\n>  \t\treturn x;\n>  \tdefault:\n> @@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n>  \tif (p)\n>  \t\topt->pattern_expression = compile_pattern_expr(&p);\n>  \tif (p)\n> -\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n> +\t\tdie(\"incomplete pattern expression group: %s\", p->pattern);\n>  \n>  \tif (opt->no_body_match && opt->pattern_expression)\n>  \t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n"},{"id":"492629","messageId":"gkoqujwrzxdt2rxpcbhz5zfspnajdko53wlazaymt5lbce5qch@tarta.nabijaczleweli.xyz","threadId":"61176","inReplyTo":"xmqqzfuox1oi.fsf@gitster.g","subject":"[PATCH v2] grep: improve errors for unmatched ( and )","fromName":"Ahelenia Ziemiańska","fromEmail":"nabijaczleweli@nabijaczleweli.xyz","sentAt":"2024-04-09T10:07:56Z","receivedAt":"2024-04-09T10:08:04Z","isPatch":true,"sender":{"key":"nabijaczleweli@nabijaczleweli.xyz","avatar":"https://avatars.githubusercontent.com/u/6709544?v=4"},"body":"Imagine you want to grep for (. Easy:\n\n  $ git grep '('\n  fatal: unmatched parenthesis\n\nuhoh. This is plainly wrong. Unless you know specifically that\n(a) git grep has expression groups and that\n(b) the only way to work around them is by doing -e '('\n\nSimilarly,\n\n  $ git grep ')'\n  fatal: incomplete pattern expression: )\n\nis somehow worse. \")\" is a complete regular expression pattern.\nOf course, the error wants to say \"group\" here.\nIn this case it's also not \"incomplete\", it's unmatched.\nBut whatever.\n\nMake them return\n\n  $ ./git grep '('\n  fatal: unmatched ( for expression group\n  $ ./git grep ')'\n  fatal: incomplete pattern expression group: )\n\nwhich hopefully are clearer in indicating that it's not the expression\nthat's wrong (since no pattern had been parsed at all), but rather that\nit's been misconstrued as a grouping operator.\n\nLink: https://bugs.debian.org/1051205\nSigned-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>\n---\n-- '(' no longer mentioned, otherwise no changes.\n\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5f23d1a..ac34bfe 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n \t\t*list = p->next;\n \t\tx = compile_pattern_or(list);\n \t\tif (!*list || (*list)->token != GREP_CLOSE_PAREN)\n-\t\t\tdie(\"unmatched parenthesis\");\n+\t\t\tdie(\"unmatched ( for expression group\");\n \t\t*list = (*list)->next;\n \t\treturn x;\n \tdefault:\n@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n \tif (p)\n \t\topt->pattern_expression = compile_pattern_expr(&p);\n \tif (p)\n-\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n+\t\tdie(\"incomplete pattern expression group: %s\", p->pattern);\n \n \tif (opt->no_body_match && opt->pattern_expression)\n \t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n-- \n2.39.2\n"}]}