# Re: [PATCH] userdiff: remove empty subexpression from elixir regex 10 messages from 2019-12-13 to 2019-12-13. Participants: Jeff King, Ed Maste, Johannes Sixt, Junio C Hamano, Achim Gratz. Thread: https://gitlist.dev/t/52450 ## Ed Maste, 2019-12-13 17:39 Subject: [PATCH] userdiff: remove empty subexpression from elixir regex Message-ID: <20191213173902.71541-1-emaste@FreeBSD.org> URL: https://gitlist.dev/e/20191213173902.71541-1-emaste%40FreeBSD.org ``` The regex failed to compile on FreeBSD. Fixes: a807200f67588f6e Signed-off-by: Ed Maste --- 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 ``` ## Jeff King, 2019-12-13 17:45 Subject: Re: [PATCH] userdiff: remove empty subexpression from elixir regex Message-ID: <20191213174542.GB117158@coredump.intra.peff.net> URL: https://gitlist.dev/e/20191213174542.GB117158%40coredump.intra.peff.net In-Reply-To: <20191213173902.71541-1-emaste@FreeBSD.org> ``` 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 ``` ## Ed Maste, 2019-12-13 17:55 Subject: [PATCH v2] userdiff: remove empty subexpression from elixir regex Message-ID: <20191213175535.87725-1-emaste@FreeBSD.org> URL: https://gitlist.dev/e/20191213175535.87725-1-emaste%40FreeBSD.org In-Reply-To: <20191213173902.71541-1-emaste@FreeBSD.org> ``` The regex failed to compile on FreeBSD. Fixes: a807200f67588f6e Signed-off-by: Ed Maste --- 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 ``` ## Ed Maste, 2019-12-13 14:11 Subject: Re: [PATCH] userdiff: remove empty subexpression from elixir regex Message-ID: URL: https://gitlist.dev/e/CAPyFy2DfhVwEFen2G4oOdQS2uo_L%3DV5gyrpPWUB0uRxNSnWcuQ%40mail.gmail.com In-Reply-To: <20191213174542.GB117158@coredump.intra.peff.net> ``` On Fri, 13 Dec 2019 at 12:45, Jeff King 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. ``` ## Jeff King, 2019-12-13 18:18 Subject: Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex Message-ID: <20191213181830.GA122626@coredump.intra.peff.net> URL: https://gitlist.dev/e/20191213181830.GA122626%40coredump.intra.peff.net In-Reply-To: <20191213175535.87725-1-emaste@FreeBSD.org> ``` 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 > --- > Add /* -- */ to make things more clear and be consistent with other > patterns. Thanks, this looks good to me. -Peff ``` ## Johannes Sixt, 2019-12-13 19:24 Subject: Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex Message-ID: <0c9d891e-382f-03d1-bcbd-d652f1d58f4d@kdbg.org> URL: https://gitlist.dev/e/0c9d891e-382f-03d1-bcbd-d652f1d58f4d%40kdbg.org In-Reply-To: <20191213175535.87725-1-emaste@FreeBSD.org> ``` 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 > --- > 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 Thanks! -- Hannes ``` ## Ed Maste, 2019-12-13 15:58 Subject: Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex Message-ID: URL: https://gitlist.dev/e/CAPyFy2B_P7qJ%2Bocg8rzNWEZWo2uKzaZsfYRvvhwUbAXv2AB6pg%40mail.gmail.com In-Reply-To: <0c9d891e-382f-03d1-bcbd-d652f1d58f4d@kdbg.org> ``` On Fri, 13 Dec 2019 at 14:24, Johannes Sixt 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 > > --- > > 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. ``` ## Junio C Hamano, 2019-12-13 20:23 Subject: Re: [PATCH v2] userdiff: remove empty subexpression from elixir regex Message-ID: URL: https://gitlist.dev/e/xmqqzhfwht40.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Ed Maste 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 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 Reviewed-by: Jeff King Helped-by: Johannes Sixt Signed-off-by: Junio C Hamano --- 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 ``` ## Achim Gratz, 2019-12-13 20:59 Subject: Numbers with specific base (was: [PATCH] userdiff: remove empty subexpression from elixir regex) Message-ID: <87tv64ymam.fsf@Rainer.invalid> URL: https://gitlist.dev/e/87tv64ymam.fsf%40Rainer.invalid In-Reply-To: <20191213173902.71541-1-emaste@FreeBSD.org> ``` 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 ``` ## Junio C Hamano, 2019-12-13 22:00 Subject: Re: Numbers with specific base Message-ID: URL: https://gitlist.dev/e/xmqqimmjhon9.fsf%40gitster-ct.c.googlers.com In-Reply-To: <87tv64ymam.fsf@Rainer.invalid> ``` Achim Gratz 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. ```