threads / patch / 61176

patchgrep: improve errors for unmatched ( and )

Subject: [PATCH] grep: improve errors for unmatched ( and )

## tl;dr

5 messages between Mar 22, 2024 and Apr 9, 2024. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Ahelenia Ziemiańska· Mar 22, 2024, 08:34 UTC · lore
Imagine you want to grep for (. Easy:
  $ git grep '('
  fatal: unmatched parenthesis
uhoh. This is plainly wrong. Unless you know specifically that
(a) git grep has expression groups and that
(b) the only way to work around them is by doing -- '(' or -e '('
Similarly,
  $ git grep ')'
  fatal: incomplete pattern expression: )
is somehow worse. ")" is a complete regular expression pattern.
Of course, the error wants to say "group" here.
In this case it's also not "incomplete", it's unmatched.
But whatever.
These now return
  $ ./git grep '('
  fatal: unmatched ( for expression group
  $ ./git grep ')'
  fatal: incomplete pattern expression group: )
which hopefully are clearer in indicating that it's not the expression
that's wrong (since no pattern had been parsed at all), but rather that
it's been misconstrued as a grouping operator.
Link: https://bugs.debian.org/1051205
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to grep.c +2 −2
diff --git a/grep.c b/grep.c
index 5f23d1a..ac34bfe 100644
--- a/grep.c
+++ b/grep.c
@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 		*list = p->next;
 		x = compile_pattern_or(list);
 		if (!*list || (*list)->token != GREP_CLOSE_PAREN)
-			die("unmatched parenthesis");
+			die("unmatched ( for expression group");
 		*list = (*list)->next;
 		return x;
 	default:
@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 	if (p)
 		opt->pattern_expression = compile_pattern_expr(&p);
 	if (p)
-		die("incomplete pattern expression: %s", p->pattern);
+		die("incomplete pattern expression group: %s", p->pattern);
 
 	if (opt->no_body_match && opt->pattern_expression)
 		opt->pattern_expression = grep_not_expr(opt->pattern_expression);

-- 
2.39.2
Junio C Hamano· Mar 22, 2024, 16:41 UTC · re: Ahelenia Ziemiańska · lore

Re: [PATCH] grep: improve errors for unmatched ( and )

Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:
> Imagine you want to grep for (. Easy:
Please have a blank line before and 
>   $ git grep '('
>   fatal: unmatched parenthesis

after a displayed text like this one (this applies to a few more paragraphs in the proposed log message).

> uhoh. This is plainly wrong. Unless you know specifically that
> (a) git grep has expression groups and that
> (b) the only way to work around them is by doing -- '(' or -e '('

I do not think "--" (end of options and beginning of pathspec) marker would work for that purpose, UNLESS you are talking about a file whose name is an open parenthesis. Just keep "-e '('" in the description and drop the double-dash there.

Show 5 quoted lines
> Similarly,
>   $ git grep ')'
>   fatal: incomplete pattern expression: )
> is somehow worse. ")" is a complete regular expression pattern.
> Of course, the error wants to say "group" here.
Nice problem description so far.
> These now return
Phrase it more like "Make them return" ...
>   $ ./git grep '('
>   fatal: unmatched ( for expression group
>   $ ./git grep ')'
>   fatal: incomplete pattern expression group: )
> which hopefully are clearer in indicating that it's not the expression
> that's wrong (since no pattern had been parsed at all), but rather that
> it's been misconstrued as a grouping operator.
Nicely done.
Show 28 quoted lines
> Link: https://bugs.debian.org/1051205
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  grep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 5f23d1a..ac34bfe 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
>  		*list = p->next;
>  		x = compile_pattern_or(list);
>  		if (!*list || (*list)->token != GREP_CLOSE_PAREN)
> -			die("unmatched parenthesis");
> +			die("unmatched ( for expression group");
>  		*list = (*list)->next;
>  		return x;
>  	default:
> @@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)
>  	if (p)
>  		opt->pattern_expression = compile_pattern_expr(&p);
>  	if (p)
> -		die("incomplete pattern expression: %s", p->pattern);
> +		die("incomplete pattern expression group: %s", p->pattern);
>  
>  	if (opt->no_body_match && opt->pattern_expression)
>  		opt->pattern_expression = grep_not_expr(opt->pattern_expression);
Thanks.  The changes to these two messages look good.
Ahelenia Ziemiańska· Mar 23, 2024, 13:18 UTC · re: Junio C Hamano · lore

[PATCH v2] grep: improve errors for unmatched ( and )

Imagine you want to grep for (. Easy:
  $ git grep '('
  fatal: unmatched parenthesis

uhoh. This is plainly wrong. Unless you know specifically that (a) git grep has expression groups and that (b) the only way to work around them is by doing -- '(' or -e '('

Similarly,
  $ git grep ')'
  fatal: incomplete pattern expression: )

is somehow worse. ")" is a complete regular expression pattern. Of course, the error wants to say "group" here. In this case it's also not "incomplete", it's unmatched. But whatever.

Make them return
  $ ./git grep '('
  fatal: unmatched ( for expression group
  $ ./git grep ')'
  fatal: incomplete pattern expression group: )

which hopefully are clearer in indicating that it's not the expression that's wrong (since no pattern had been parsed at all), but rather that it's been misconstrued as a grouping operator.

Link: https://bugs.debian.org/1051205
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
On Fri, Mar 22, 2024 at 09:41:40AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:
> > uhoh. This is plainly wrong. Unless you know specifically that
> > (a) git grep has expression groups and that
> > (b) the only way to work around them is by doing -- '(' or -e '('
> I do not think "--" (end of options and beginning of pathspec)
> marker would work for that purpose, UNLESS you are talking about a
> file whose name is an open parenthesis.

False. -- turns all subsequent parameters into arguments, and if there is no -e, the first argument is the pattern, and all the subsequent ones are paths. This is normal [git] grep behaviour.

> Just keep "-e '('" in the
> description and drop the double-dash there.

Disagree. This is one of the two methodologies I've devised to work around this in the past, and it's one of the two methodologies that work.

All else applied.
 grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to grep.c +2 −2
diff --git a/grep.c b/grep.c
index 5f23d1a..ac34bfe 100644
--- a/grep.c
+++ b/grep.c
@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 		*list = p->next;
 		x = compile_pattern_or(list);
 		if (!*list || (*list)->token != GREP_CLOSE_PAREN)
-			die("unmatched parenthesis");
+			die("unmatched ( for expression group");
 		*list = (*list)->next;
 		return x;
 	default:
@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 	if (p)
 		opt->pattern_expression = compile_pattern_expr(&p);
 	if (p)
-		die("incomplete pattern expression: %s", p->pattern);
+		die("incomplete pattern expression group: %s", p->pattern);
 
 	if (opt->no_body_match && opt->pattern_expression)
 		opt->pattern_expression = grep_not_expr(opt->pattern_expression);

-- 
2.39.2
Junio C Hamano· Mar 23, 2024, 18:06 UTC · re: Ahelenia Ziemiańska · lore

Re: [PATCH v2] grep: improve errors for unmatched ( and )

Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:
Show 44 quoted lines
> Imagine you want to grep for (. Easy:
>
>   $ git grep '('
>   fatal: unmatched parenthesis
>
> uhoh. This is plainly wrong. Unless you know specifically that
> (a) git grep has expression groups and that
> (b) the only way to work around them is by doing -- '(' or -e '('
>
> Similarly,
>
>   $ git grep ')'
>   fatal: incomplete pattern expression: )
>
> is somehow worse. ")" is a complete regular expression pattern.
> Of course, the error wants to say "group" here.
> In this case it's also not "incomplete", it's unmatched.
> But whatever.
>
> Make them return
>
>   $ ./git grep '('
>   fatal: unmatched ( for expression group
>   $ ./git grep ')'
>   fatal: incomplete pattern expression group: )
>
> which hopefully are clearer in indicating that it's not the expression
> that's wrong (since no pattern had been parsed at all), but rather that
> it's been misconstrued as a grouping operator.
>
> Link: https://bugs.debian.org/1051205
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
> On Fri, Mar 22, 2024 at 09:41:40AM -0700, Junio C Hamano wrote:
>> Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> writes:
>> > uhoh. This is plainly wrong. Unless you know specifically that
>> > (a) git grep has expression groups and that
>> > (b) the only way to work around them is by doing -- '(' or -e '('
>> I do not think "--" (end of options and beginning of pathspec)
>> marker would work for that purpose, UNLESS you are talking about a
>> file whose name is an open parenthesis.
> False. -- turns all subsequent parameters into arguments, and if
> there is no -e, the first argument is the pattern, and all the
> subsequent ones are paths. This is normal [git] grep behaviour.
Ah, thanks.

I forgot that "git grep" was a oddball that allows revs come after "--" in some cases. As long as the user understands this may not work for other commands, it is OK. "-e" is the only officially supported way (which is why I mentioned it in the review comments I gave you here), so guiding users in that direction would be a better idea anyway, though.

Thanks.
Show 22 quoted lines
> diff --git a/grep.c b/grep.c
> index 5f23d1a..ac34bfe 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
>  		*list = p->next;
>  		x = compile_pattern_or(list);
>  		if (!*list || (*list)->token != GREP_CLOSE_PAREN)
> -			die("unmatched parenthesis");
> +			die("unmatched ( for expression group");
>  		*list = (*list)->next;
>  		return x;
>  	default:
> @@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)
>  	if (p)
>  		opt->pattern_expression = compile_pattern_expr(&p);
>  	if (p)
> -		die("incomplete pattern expression: %s", p->pattern);
> +		die("incomplete pattern expression group: %s", p->pattern);
>  
>  	if (opt->no_body_match && opt->pattern_expression)
>  		opt->pattern_expression = grep_not_expr(opt->pattern_expression);
Ahelenia Ziemiańska· Apr 9, 2024, 10:07 UTC · re: Junio C Hamano · lore

[PATCH v2] grep: improve errors for unmatched ( and )

Imagine you want to grep for (. Easy:
  $ git grep '('
  fatal: unmatched parenthesis

uhoh. This is plainly wrong. Unless you know specifically that (a) git grep has expression groups and that (b) the only way to work around them is by doing -e '('

Similarly,
  $ git grep ')'
  fatal: incomplete pattern expression: )

is somehow worse. ")" is a complete regular expression pattern. Of course, the error wants to say "group" here. In this case it's also not "incomplete", it's unmatched. But whatever.

Make them return
  $ ./git grep '('
  fatal: unmatched ( for expression group
  $ ./git grep ')'
  fatal: incomplete pattern expression group: )

which hopefully are clearer in indicating that it's not the expression that's wrong (since no pattern had been parsed at all), but rather that it's been misconstrued as a grouping operator.

Link: https://bugs.debian.org/1051205
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
-- '(' no longer mentioned, otherwise no changes.
 grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to grep.c +2 −2
diff --git a/grep.c b/grep.c
index 5f23d1a..ac34bfe 100644
--- a/grep.c
+++ b/grep.c
@@ -621,7 +621,7 @@ static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 		*list = p->next;
 		x = compile_pattern_or(list);
 		if (!*list || (*list)->token != GREP_CLOSE_PAREN)
-			die("unmatched parenthesis");
+			die("unmatched ( for expression group");
 		*list = (*list)->next;
 		return x;
 	default:
@@ -792,7 +792,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 	if (p)
 		opt->pattern_expression = compile_pattern_expr(&p);
 	if (p)
-		die("incomplete pattern expression: %s", p->pattern);
+		die("incomplete pattern expression group: %s", p->pattern);
 
 	if (opt->no_body_match && opt->pattern_expression)
 		opt->pattern_expression = grep_not_expr(opt->pattern_expression);

-- 
2.39.2

← back to recent threads