Re: [PATCH] userdiff: remove empty subexpression from elixir regex

Re: [PATCH] userdiff: remove empty subexpression from elixir regex

[Ed Maste]

On Fri, 13 Dec 2019 at 12:45, Jeff King <peff@peff.net> wrote:
>
> And that is the right thing, since these strings are the funcname and
> word_regex patterns, respectively.
>
> So I think this is the correct fix. Many of the other regexes in this
> list use "/* -- */" to seperate the two for readability. Maybe worth
> doing here, too?

Yeah, this elixir set seems to be the only one with comments on the
individual subexpressions in the second set but the extra /* -- */
does make it a bit more clear. Patch v2 sent.

Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex

[Ed Maste]

On Fri, 13 Dec 2019 at 14:24, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 13.12.19 um 18:55 schrieb Ed Maste:
> > The regex failed to compile on FreeBSD.
> >
> > Fixes: a807200f67588f6e
>
> Having a references is this form is unusual for our codebase. (Not that
> I mind a lot, though.) I expect that Junio will commit the fix on top of
> the commit that introduced the bogus regex anyway (branch
> ln/userdiff-elixir), and then it will be easy find.

Ok, I picked this up from the Linux kernel where someone added a
Fixes: tag to one of my changes (which had the hash of the original
change as part of the commit message body).

> > Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> > ---
> > Add /* -- */ to make things more clear and be consistent with other
> > patterns.
>
> This text would be nice to have in the commit message.

Ah, I didn't think it was remarkable (it's consistent with all of the
existing entries) but the change is indeed broader than what the
commit message implies. I'm happy to send a v3 with an amended commit
message if that's desired.

[PATCH] userdiff: remove empty subexpression from elixir regex

[Ed Maste]

The regex failed to compile on FreeBSD.

Fixes: a807200f67588f6e
Signed-off-by: Ed Maste <emaste@FreeBSD.org>
---
 userdiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/userdiff.c b/userdiff.c
index 324916f20f..165d7e8653 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -35,7 +35,7 @@ PATTERNS("dts",
 PATTERNS("elixir",
 	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
 	 /* Atoms, names, and module attributes */
-	 "|[@:]?[a-zA-Z0-9@_?!]+"
+	 "[@:]?[a-zA-Z0-9@_?!]+"
 	 /* Numbers with specific base */
 	 "|[-+]?0[xob][0-9a-fA-F]+"
 	 /* Numbers */
-- 
2.24.0

Re: [PATCH] userdiff: remove empty subexpression from elixir regex

[Jeff King]

On Fri, Dec 13, 2019 at 05:39:02PM +0000, Ed Maste wrote:

> diff --git a/userdiff.c b/userdiff.c
> index 324916f20f..165d7e8653 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -35,7 +35,7 @@ PATTERNS("dts",
>  PATTERNS("elixir",
>  	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
>  	 /* Atoms, names, and module attributes */
> -	 "|[@:]?[a-zA-Z0-9@_?!]+"
> +	 "[@:]?[a-zA-Z0-9@_?!]+"
>  	 /* Numbers with specific base */
>  	 "|[-+]?0[xob][0-9a-fA-F]+"
>  	 /* Numbers */

It took me a minute to see why this was different than the similar
"Numbers" line below. The issue is the comma at the end of the previous
line; this is starting a new string, whereas the "Numbers" line is
pasting to the existing string.

And that is the right thing, since these strings are the funcname and
word_regex patterns, respectively.

So I think this is the correct fix. Many of the other regexes in this
list use "/* -- */" to seperate the two for readability. Maybe worth
doing here, too?

-Peff

[PATCH v2] userdiff: remove empty subexpression from elixir regex

[Ed Maste]

The regex failed to compile on FreeBSD.

Fixes: a807200f67588f6e
Signed-off-by: Ed Maste <emaste@FreeBSD.org>
---
Add /* -- */ to make things more clear and be consistent with other
patterns.

 userdiff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/userdiff.c b/userdiff.c
index 324916f20f..efbe05e5a5 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -34,8 +34,9 @@ PATTERNS("dts",
 	 "|[-+*/%&^|!~]|>>|<<|&&|\\|\\|"),
 PATTERNS("elixir",
 	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
+	 /* -- */
 	 /* Atoms, names, and module attributes */
-	 "|[@:]?[a-zA-Z0-9@_?!]+"
+	 "[@:]?[a-zA-Z0-9@_?!]+"
 	 /* Numbers with specific base */
 	 "|[-+]?0[xob][0-9a-fA-F]+"
 	 /* Numbers */
-- 
2.24.0

Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex

[Jeff King]

On Fri, Dec 13, 2019 at 05:55:35PM +0000, Ed Maste wrote:

> The regex failed to compile on FreeBSD.
> 
> Fixes: a807200f67588f6e
> Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> ---
> Add /* -- */ to make things more clear and be consistent with other
> patterns.

Thanks, this looks good to me.

-Peff

Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex

[Johannes Sixt]

Am 13.12.19 um 18:55 schrieb Ed Maste:
> The regex failed to compile on FreeBSD.
> 
> Fixes: a807200f67588f6e

Having a references is this form is unusual for our codebase. (Not that
I mind a lot, though.) I expect that Junio will commit the fix on top of
the commit that introduced the bogus regex anyway (branch
ln/userdiff-elixir), and then it will be easy find.

> Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> ---
> Add /* -- */ to make things more clear and be consistent with other
> patterns.

This text would be nice to have in the commit message.

> 
>  userdiff.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/userdiff.c b/userdiff.c
> index 324916f20f..efbe05e5a5 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -34,8 +34,9 @@ PATTERNS("dts",
>  	 "|[-+*/%&^|!~]|>>|<<|&&|\\|\\|"),
>  PATTERNS("elixir",
>  	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
> +	 /* -- */
>  	 /* Atoms, names, and module attributes */
> -	 "|[@:]?[a-zA-Z0-9@_?!]+"
> +	 "[@:]?[a-zA-Z0-9@_?!]+"
>  	 /* Numbers with specific base */
>  	 "|[-+]?0[xob][0-9a-fA-F]+"
>  	 /* Numbers */
> 

Good catch!

Tested-by: Johannes Sixt <j6t@kdbg.org>

Thanks!

-- Hannes

Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex

[Junio C Hamano]

Ed Maste <emaste@freebsd.org> writes:

>> > Add /* -- */ to make things more clear and be consistent with other
>> > patterns.
>>
>> This text would be nice to have in the commit message.
>
> Ah, I didn't think it was remarkable (it's consistent with all of the
> existing entries) but the change is indeed broader than what the
> commit message implies. I'm happy to send a v3 with an amended commit
> message if that's desired.

Let's save one round-trip, then.  Here is what I will queue on the
'pu' branch.

Thanks, all.

-- >8 --
From: Ed Maste <emaste@FreeBSD.org>
Date: Fri, 13 Dec 2019 17:55:35 +0000
Subject: [PATCH] userdiff: remove empty subexpression from elixir regex

The regex failed to compile on FreeBSD.

Also add /* -- */ mark to separate the two regex entries given to
the PATTERNS() macro, to make it consistent with patterns for other
content types.

Signed-off-by: Ed Maste <emaste@FreeBSD.org>
Reviewed-by: Jeff King <peff@peff.net>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 userdiff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/userdiff.c b/userdiff.c
index 577053c10a..0eb34bcd76 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -34,8 +34,9 @@ PATTERNS("dts",
 	 "|[-+*/%&^|!~]|>>|<<|&&|\\|\\|"),
 PATTERNS("elixir",
 	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
+	 /* -- */
 	 /* Atoms, names, and module attributes */
-	 "|[@:]?[a-zA-Z0-9@_?!]+"
+	 "[@:]?[a-zA-Z0-9@_?!]+"
 	 /* Numbers with specific base */
 	 "|[-+]?0[xob][0-9a-fA-F]+"
 	 /* Numbers */
-- 
2.24.1-664-g198078bb5a

Numbers with specific base (was: [PATCH] userdiff: remove empty subexpression from elixir regex)

[Achim Gratz]

Nothing to do with the patch from Ed, but the regex following his
correction matches a lot of things that decidedly are not "Numbers with
specific bases" as it claims to do in the comment.

Ed Maste writes:
>  PATTERNS("elixir",
>  	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
>  	 /* Atoms, names, and module attributes */
> -	 "|[@:]?[a-zA-Z0-9@_?!]+"
> +	 "[@:]?[a-zA-Z0-9@_?!]+"
>  	 /* Numbers with specific base */
>  	 "|[-+]?0[xob][0-9a-fA-F]+"

Here, things like "+0bad" would match as a base 2 number, which doesn't
seem right.  If it's intended to match that broadly, I'd have expected a
comment to that effect.  Maybe something like

"|[-+]?0b[01]+|[-+]?0o[0-7]+|[-+]?0x[0-9a-fA-F]+"

or (if the resulting group is not a problem someplace else)

"|[-+]?0(b[01]+|o[0-7]+|x[0-9a-fA-F]+)"

to more specifically match only what the comment says?



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: Numbers with specific base

[Junio C Hamano]

Achim Gratz <Stromeko@nexgo.de> writes:

> Nothing to do with the patch from Ed, but the regex following his
> correction matches a lot of things that decidedly are not "Numbers with
> specific bases" as it claims to do in the comment.
>
> Ed Maste writes:
>>  PATTERNS("elixir",
>>  	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
>>  	 /* Atoms, names, and module attributes */
>> -	 "|[@:]?[a-zA-Z0-9@_?!]+"
>> +	 "[@:]?[a-zA-Z0-9@_?!]+"
>>  	 /* Numbers with specific base */
>>  	 "|[-+]?0[xob][0-9a-fA-F]+"
>
> Here, things like "+0bad" would match as a base 2 number, which doesn't
> seem right.  If it's intended to match that broadly, I'd have expected a
> comment to that effect.

No need for such a comment, as it is implicit that we assume the
user writes reasonable text that our patterns try to match.