{"thread":{"id":"56249","subject":"[PATCH v4] userdiff: improve java hunk header regex","startedAt":"2021-08-10T19:09:51Z","lastAt":"2021-08-11T20:32:29Z","messageCount":9,"participants":["Tassilo Horn","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"432404","messageId":"20210810190937.305765-1-tsdh@gnu.org","threadId":"56249","inReplyTo":null,"subject":"[PATCH v4] userdiff: improve java hunk header regex","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2021-08-10T19:09:37Z","receivedAt":"2021-08-10T19:09:51Z","isPatch":true,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Currently, the git diff hunk headers show the wrong method signature if the\nmethod has a qualified return type, an array return type, or a generic return\ntype because the regex doesn't allow dots (.), [], or < and > in the return\ntype.  Also, type parameter declarations couldn't be matched.\n\nAdd several t4018 tests asserting the right hunk headers for increasingly\ncomplex method signatures:\n\n  public String[] myMethod(String[] RIGHT)\n  public List<String> myMethod(String[] RIGHT)\n  public <T> List<T> myMethod(T[] RIGHT)\n  public <AType, B> Map<AType, B> myMethod(String[] RIGHT)\n  public <AType, B> java.util.Map<AType, Map<B, B[]>> myMethod(String[] RIGHT)\n  public List<? extends Comparable> myMethod(String[] RIGHT)\n  public <T extends Serializable & Comparable<T>> List<T> myMethod(String[] RIGHT)\n\nSigned-off-by: Tassilo Horn <tsdh@gnu.org>\n---\n t/t4018/java-constructor             |  6 ++++++\n t/t4018/java-enum-constant           |  6 ++++++\n t/t4018/java-nested-field            |  6 ++++++\n t/t4018/java-return-array            |  6 ++++++\n t/t4018/java-return-generic          |  6 ++++++\n t/t4018/java-return-generic-bounded  |  6 ++++++\n t/t4018/java-return-generic-wildcart |  6 ++++++\n t/t4018/java-return-generic2         |  6 ++++++\n t/t4018/java-return-generic3         |  6 ++++++\n t/t4018/java-return-generic4         |  6 ++++++\n userdiff.c                           | 23 ++++++++++++++++++++++-\n 11 files changed, 82 insertions(+), 1 deletion(-)\n create mode 100644 t/t4018/java-constructor\n create mode 100644 t/t4018/java-enum-constant\n create mode 100644 t/t4018/java-nested-field\n create mode 100644 t/t4018/java-return-array\n create mode 100644 t/t4018/java-return-generic\n create mode 100644 t/t4018/java-return-generic-bounded\n create mode 100644 t/t4018/java-return-generic-wildcart\n create mode 100644 t/t4018/java-return-generic2\n create mode 100644 t/t4018/java-return-generic3\n create mode 100644 t/t4018/java-return-generic4\n\ndiff --git a/t/t4018/java-constructor b/t/t4018/java-constructor\nnew file mode 100644\nindex 0000000000..9daf7c5430\n--- /dev/null\n+++ b/t/t4018/java-constructor\n@@ -0,0 +1,6 @@\n+public class MyClass {\n+    MyClass(String RIGHT) {\n+        // Whatever\n+        // ChangeMe\n+    }\n+}\ndiff --git a/t/t4018/java-enum-constant b/t/t4018/java-enum-constant\nnew file mode 100644\nindex 0000000000..a1931c8379\n--- /dev/null\n+++ b/t/t4018/java-enum-constant\n@@ -0,0 +1,6 @@\n+private enum RIGHT {\n+    ONE,\n+    TWO,\n+    THREE,\n+    ChangeMe\n+}\ndiff --git a/t/t4018/java-nested-field b/t/t4018/java-nested-field\nnew file mode 100644\nindex 0000000000..d92d3ec688\n--- /dev/null\n+++ b/t/t4018/java-nested-field\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    private static class RIGHT {\n+        // change an inner class field\n+        String inner = \"ChangeMe\";\n+    }\n+}\ndiff --git a/t/t4018/java-return-array b/t/t4018/java-return-array\nnew file mode 100644\nindex 0000000000..747638b9a8\n--- /dev/null\n+++ b/t/t4018/java-return-array\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public String[] myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return new; // ChangeMe\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic b/t/t4018/java-return-generic\nnew file mode 100644\nindex 0000000000..161dd8338f\n--- /dev/null\n+++ b/t/t4018/java-return-generic\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public List<String> myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return Arrays.asList(\"ChangeMe\");\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic-bounded b/t/t4018/java-return-generic-bounded\nnew file mode 100644\nindex 0000000000..440115a788\n--- /dev/null\n+++ b/t/t4018/java-return-generic-bounded\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public <T extends Serializable & Comparable<T>> List<T> myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return (List<T>) Arrays.asList(\"ChangeMe\");\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic-wildcart b/t/t4018/java-return-generic-wildcart\nnew file mode 100644\nindex 0000000000..2d682e1e2b\n--- /dev/null\n+++ b/t/t4018/java-return-generic-wildcart\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public List<? extends Comparable> myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return Arrays.asList(\"ChangeMe\");\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic2 b/t/t4018/java-return-generic2\nnew file mode 100644\nindex 0000000000..7109c27456\n--- /dev/null\n+++ b/t/t4018/java-return-generic2\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public <T> List<T> myMethod(T[] RIGHT) {\n+        // Whatever...\n+        return (List<T>) Arrays.asList(\"ChangeMe\");\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic3 b/t/t4018/java-return-generic3\nnew file mode 100644\nindex 0000000000..849f116f50\n--- /dev/null\n+++ b/t/t4018/java-return-generic3\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public <AType, B> Map<AType, B> myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return new java.util.HashMap<>(); // ChangeMe\n+    }\n+}\ndiff --git a/t/t4018/java-return-generic4 b/t/t4018/java-return-generic4\nnew file mode 100644\nindex 0000000000..1b22c8c037\n--- /dev/null\n+++ b/t/t4018/java-return-generic4\n@@ -0,0 +1,6 @@\n+class MyExample {\n+    public <AType, B> java.util.Map<AType, Map<B, B[]>> myMethod(String[] RIGHT) {\n+        // Whatever...\n+        return new java.util.HashMap<>(); // ChangeMe\n+    }\n+}\ndiff --git a/userdiff.c b/userdiff.c\nindex 3c3bbe38b0..9bd751b7d2 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -142,7 +142,28 @@ PATTERNS(\"html\",\n \t \"[^<>= \\t]+\"),\n PATTERNS(\"java\",\n \t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n-\t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\",\n+         \"^[ \\t]*(\"\n+         /* Class, enum, and interface declarations: */\n+         /*   optional modifiers: public */\n+         \"(([a-z]+[ \\t]+)*\"\n+         /*   the kind of declaration */\n+         \"(class|enum|interface)[ \\t]+\"\n+         /*   the name */\n+         \"[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n+         /* Method & constructor signatures: */\n+         /*   optional modifiers: public static */\n+         \"|(([a-z]+[ \\t]+)*\"\n+         /*   type params and return types for methods but not constructors */\n+         \"(\"\n+         /*     optional type parameters: <A, B extends Comparable<B>> */\n+         \"(<[A-Za-z0-9_,.&<> \\t]+>[ \\t]+)?\"\n+         /*     return type: java.util.Map<A, B[]> or List<?> */\n+         \"([A-Za-z_]([A-Za-z_0-9<>,.?]|\\\\[[ \\t]*\\\\])*[ \\t]+)+\"\n+         /*   end of type params and return type */\n+         \")?\"\n+         /*   the method name followed by the parameter list: myMethod(...) */\n+         \"[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n+         \")$\",\n \t /* -- */\n \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-- \n2.32.0\n\n"},{"id":"432417","messageId":"d3484278-8413-0d10-e6cd-59a7ff04564b@kdbg.org","threadId":"56249","inReplyTo":"20210810190937.305765-1-tsdh@gnu.org","subject":"Re: [PATCH v4] userdiff: improve java hunk header regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-08-10T20:57:47Z","receivedAt":"2021-08-10T20:57:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.08.21 um 21:09 schrieb Tassilo Horn:\n> Currently, the git diff hunk headers show the wrong method signature if the\n> method has a qualified return type, an array return type, or a generic return\n> type because the regex doesn't allow dots (.), [], or < and > in the return\n> type.  Also, type parameter declarations couldn't be matched.\n> \n> Add several t4018 tests asserting the right hunk headers for increasingly\n> complex method signatures:\n> \n>   public String[] myMethod(String[] RIGHT)\n>   public List<String> myMethod(String[] RIGHT)\n>   public <T> List<T> myMethod(T[] RIGHT)\n>   public <AType, B> Map<AType, B> myMethod(String[] RIGHT)\n>   public <AType, B> java.util.Map<AType, Map<B, B[]>> myMethod(String[] RIGHT)\n>   public List<? extends Comparable> myMethod(String[] RIGHT)\n>   public <T extends Serializable & Comparable<T>> List<T> myMethod(String[] RIGHT)\n> \n> Signed-off-by: Tassilo Horn <tsdh@gnu.org>\n> ---\n>  t/t4018/java-constructor             |  6 ++++++\n>  t/t4018/java-enum-constant           |  6 ++++++\n>  t/t4018/java-nested-field            |  6 ++++++\n>  t/t4018/java-return-array            |  6 ++++++\n>  t/t4018/java-return-generic          |  6 ++++++\n>  t/t4018/java-return-generic-bounded  |  6 ++++++\n>  t/t4018/java-return-generic-wildcart |  6 ++++++\n>  t/t4018/java-return-generic2         |  6 ++++++\n>  t/t4018/java-return-generic3         |  6 ++++++\n>  t/t4018/java-return-generic4         |  6 ++++++\n>  userdiff.c                           | 23 ++++++++++++++++++++++-\n>  11 files changed, 82 insertions(+), 1 deletion(-)\n>  create mode 100644 t/t4018/java-constructor\n>  create mode 100644 t/t4018/java-enum-constant\n>  create mode 100644 t/t4018/java-nested-field\n>  create mode 100644 t/t4018/java-return-array\n>  create mode 100644 t/t4018/java-return-generic\n>  create mode 100644 t/t4018/java-return-generic-bounded\n>  create mode 100644 t/t4018/java-return-generic-wildcart\n>  create mode 100644 t/t4018/java-return-generic2\n>  create mode 100644 t/t4018/java-return-generic3\n>  create mode 100644 t/t4018/java-return-generic4\n> \n\nThese new tests are very much appreciated. You do not have to go wild\nwith that many return type tests; IMO, the simple one and the most\ncomplicated one should do it. (And btw, s/cart/card/)\n\n> diff --git a/t/t4018/java-return-array b/t/t4018/java-return-array\n> new file mode 100644\n> index 0000000000..747638b9a8\n> --- /dev/null\n> +++ b/t/t4018/java-return-array\n> @@ -0,0 +1,6 @@\n> +class MyExample {\n> +    public String[] myMethod(String[] RIGHT) {\n> +        // Whatever...\n> +        return new; // ChangeMe\n> +    }\n> +}\n> diff --git a/userdiff.c b/userdiff.c\n> index 3c3bbe38b0..9bd751b7d2 100644\n> --- a/userdiff.c\n> +++ b/userdiff.c\n> @@ -142,7 +142,28 @@ PATTERNS(\"html\",\n>  \t \"[^<>= \\t]+\"),\n>  PATTERNS(\"java\",\n>  \t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n> -\t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\",\n> +         \"^[ \\t]*(\"\n> +         /* Class, enum, and interface declarations: */\n> +         /*   optional modifiers: public */\n> +         \"(([a-z]+[ \\t]+)*\"\n> +         /*   the kind of declaration */\n> +         \"(class|enum|interface)[ \\t]+\"\n> +         /*   the name */\n> +         \"[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n> +         /* Method & constructor signatures: */\n> +         /*   optional modifiers: public static */\n> +         \"|(([a-z]+[ \\t]+)*\"\n> +         /*   type params and return types for methods but not constructors */\n> +         \"(\"\n> +         /*     optional type parameters: <A, B extends Comparable<B>> */\n> +         \"(<[A-Za-z0-9_,.&<> \\t]+>[ \\t]+)?\"\n> +         /*     return type: java.util.Map<A, B[]> or List<?> */\n> +         \"([A-Za-z_]([A-Za-z_0-9<>,.?]|\\\\[[ \\t]*\\\\])*[ \\t]+)+\"\n> +         /*   end of type params and return type */\n> +         \")?\"\n> +         /*   the method name followed by the parameter list: myMethod(...) */\n> +         \"[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n> +         \")$\",\n\nI don't see the point in this complicated regex. Please recall that it\nwill be applied only to syntactically correct Java text. Therefore, you\ndo not have to implement all syntactical corner cases, just be\nsufficiently permissive.\n\nWhat is wrong with\n\n\t\"^[ \\t]*(([A-Za-z_][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[\n\\t]*\\\\([^;]*)$\",\n\ni.e. take every \"token\" until an identifier followed by an opening\nparenthesis is found. Can types in Java contain parentheses? That would\nmake my suggested simplified regex too permissive, but otherwise it\nwould do its job, I would think.\n\n-- Hannes\n"},{"id":"432421","messageId":"xmqq35rhc5la.fsf_-_@gitster.g","threadId":"56249","inReplyTo":"d3484278-8413-0d10-e6cd-59a7ff04564b@kdbg.org","subject":"Re* [PATCH v4] userdiff: improve java hunk header regex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-10T22:12:01Z","receivedAt":"2021-08-10T22:12:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> I don't see the point in this complicated regex. Please recall that it\n> will be applied only to syntactically correct Java text. Therefore, you\n> do not have to implement all syntactical corner cases, just be\n> sufficiently permissive.\n\nGood suggestion.  We may want to mention the above principle as a\ncomment near the top of the patterns array.\n\n> What is wrong with\n>\n> \t\"^[ \\t]*(([A-Za-z_][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[\n> \\t]*\\\\([^;]*)$\",\n>\n> i.e. take every \"token\" until an identifier followed by an opening\n> parenthesis is found. Can types in Java contain parentheses? That would\n> make my suggested simplified regex too permissive, but otherwise it\n> would do its job, I would think.\n\nThanks.\n\n---- >8 -------- >8 -------- >8 -------- >8 -------- >8 --------\nSubject: userdiff: comment on the builtin patterns\n\nRemind developers that they do not need to go overboard to implement\npatterns to prepare for invalid constructs.  They only have to be\nsufficiently permissive, assuming that the payload is syntactically\ncorrect.\n\nText stolen mostly from Johannes Sixt.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n userdiff.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git c/userdiff.c w/userdiff.c\nindex d9b2ba752f..1a6d27fda6 100644\n--- c/userdiff.c\n+++ w/userdiff.c\n@@ -13,6 +13,16 @@ static int drivers_alloc;\n #define IPATTERN(name, pattern, word_regex)\t\t\t\\\n \t{ name, NULL, -1, { pattern, REG_EXTENDED | REG_ICASE }, \\\n \t  word_regex \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\" }\n+\n+/*\n+ * Built-in drivers for various languages, sorted by their names\n+ * (except that the \"default\" is left at the end).\n+ *\n+ * When writing or updating patterns, assume that the contents these\n+ * patterns are applied to are syntactically correct.  You do not have\n+ * to implement all syntactical corner cases---the patterns have to be\n+ * sufficiently permissive.\n+ */\n static struct userdiff_driver builtin_drivers[] = {\n IPATTERN(\"ada\",\n \t \"!^(.*[ \\t])?(is[ \\t]+new|renames|is[ \\t]+separate)([ \\t].*)?$\\n\"\n"},{"id":"432448","messageId":"87zgtoh6bm.fsf@gnu.org","threadId":"56249","inReplyTo":"d3484278-8413-0d10-e6cd-59a7ff04564b@kdbg.org","subject":"Re: [PATCH v4] userdiff: improve java hunk header regex","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2021-08-11T05:22:06Z","receivedAt":"2021-08-11T05:57:30Z","isPatch":true,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\nHi Hannes & Junio,\n\n> These new tests are very much appreciated. You do not have to go wild\n> with that many return type tests; IMO, the simple one and the most\n> complicated one should do it. (And btw, s/cart/card/)\n\nWell, they appeared naturally as a result during development and made it\neasier to spot errors when you know up to which level of complexity it\nstill worked.  Is there a stronger reason to remove tests which might\nnot be needed, e.g., runtime cost on some CI machines?\n\n>> -\t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\",\n>> +         \"^[ \\t]*(\"\n>> +         /* Class, enum, and interface declarations: */\n>> +         /*   optional modifiers: public */\n>> +         \"(([a-z]+[ \\t]+)*\"\n>> +         /*   the kind of declaration */\n>> +         \"(class|enum|interface)[ \\t]+\"\n>> +         /*   the name */\n>> +         \"[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n>> +         /* Method & constructor signatures: */\n>> +         /*   optional modifiers: public static */\n>> +         \"|(([a-z]+[ \\t]+)*\"\n>> +         /*   type params and return types for methods but not constructors */\n>> +         \"(\"\n>> +         /*     optional type parameters: <A, B extends Comparable<B>> */\n>> +         \"(<[A-Za-z0-9_,.&<> \\t]+>[ \\t]+)?\"\n>> +         /*     return type: java.util.Map<A, B[]> or List<?> */\n>> +         \"([A-Za-z_]([A-Za-z_0-9<>,.?]|\\\\[[ \\t]*\\\\])*[ \\t]+)+\"\n>> +         /*   end of type params and return type */\n>> +         \")?\"\n>> +         /*   the method name followed by the parameter list: myMethod(...) */\n>> +         \"[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n>> +         \")$\",\n>\n> I don't see the point in this complicated regex. Please recall that it\n> will be applied only to syntactically correct Java text. Therefore,\n> you do not have to implement all syntactical corner cases, just be\n> sufficiently permissive.\n\nI actually find it easier to understand if it is broken up into more\nconcrete alternatives and parts which are commented instaed of one\nopaque \"permissively match everything in one alternative\" regex.  It\nshows the intent of what you want to match.  But YMMV and since Junio\nagrees with you, I'm fine with that approach.\n\n> What is wrong with\n>\n> \t\"^[ \\t]*(([A-Za-z_][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[\n> \\t]*\\\\([^;]*)$\",\n\nThat doesn't work for\n\n  <T> List<T> foo()\n\nor\n\n  <T extends Foo & Bar> T foo()\n\nso at least it needs to include &<> in the first group, too.\n\nAlso, it doesn't match class/enum/interface declarations anymore, so\n\n  class Foo {\n    String x = \"ChangeMe\";\n  }\n\nwill have an empty hunk header.\n\nAnother thing I've noticed (with my suggested patch) is that I should\nnot try to match constructor signatures.  I think that's impossible\nbecause they are indistinguishable from method calls, e.g., in\n\n  public class MyClass {\n      MyClass(String RIGHT) {\n          someMethodCall();\n          someOtherMethod(17)\n              .doThat();\n          // Whatever\n          // ChangeMe\n      }\n  }\n\nthere is no regex way to prefer MyClass(String RIGHT) over\nsomeOtherMethod().\n\nSo all in all, I'd propose this version in the next patch version:\n\n--8<---------------cut here---------------start------------->8---\nPATTERNS(\"java\",\n\t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n         \"^[ \\t]*(\"\n         /* Class, enum, and interface declarations */\n         \"(([a-z]+[ \\t]+)*(class|enum|interface)[ \\t]+[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n         /* Method definitions; note that constructor signatures are not */\n         /* matched because they are indistinguishable from method calls. */\n         \"|(([A-Za-z_<>&][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n         \")$\",\n\t /* -- */\n\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n\t \"|[-+*/<>%&^|=!]=\"\n\t \"|--|\\\\+\\\\+|<<=?|>>>?=?|&&|\\\\|\\\\|\"),\n--8<---------------cut here---------------end--------------->8---\n\nThat works for all my test cases (which I have also altered to include\nthe method calls from above before the ChangeMe) except for\njava-constructor where it shows\n\n  public class MyClass {\n\ninstead of\n\n      MyClass(String RIGHT) {\n\nin the hunk header which is expected as explained earlier and in the\ncomment.\n\nDoes that seem like a good middle ground?\n\nBye,\nTassilo\n"},{"id":"432452","messageId":"157a0c35-1c82-9a2e-3bcd-ae6059ec71bd@kdbg.org","threadId":"56249","inReplyTo":"xmqq35rhc5la.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v4] userdiff: improve java hunk header regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-08-11T07:14:13Z","receivedAt":"2021-08-11T07:14:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.08.21 um 00:12 schrieb Junio C Hamano:\n> Subject: userdiff: comment on the builtin patterns\n> \n> Remind developers that they do not need to go overboard to implement\n> patterns to prepare for invalid constructs.  They only have to be\n> sufficiently permissive, assuming that the payload is syntactically\n> correct.\n> \n> Text stolen mostly from Johannes Sixt.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  userdiff.c | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n> \n> diff --git c/userdiff.c w/userdiff.c\n> index d9b2ba752f..1a6d27fda6 100644\n> --- c/userdiff.c\n> +++ w/userdiff.c\n> @@ -13,6 +13,16 @@ static int drivers_alloc;\n>  #define IPATTERN(name, pattern, word_regex)\t\t\t\\\n>  \t{ name, NULL, -1, { pattern, REG_EXTENDED | REG_ICASE }, \\\n>  \t  word_regex \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\" }\n> +\n> +/*\n> + * Built-in drivers for various languages, sorted by their names\n> + * (except that the \"default\" is left at the end).\n> + *\n> + * When writing or updating patterns, assume that the contents these\n> + * patterns are applied to are syntactically correct.  You do not have\n> + * to implement all syntactical corner cases---the patterns have to be\n> + * sufficiently permissive.\n> + */\n\nIMO, as written, the comment falls short of suggesting that patterns can\nbe simple. How about appending \"and can be simple\"?\n\n>  static struct userdiff_driver builtin_drivers[] = {\n>  IPATTERN(\"ada\",\n>  \t \"!^(.*[ \\t])?(is[ \\t]+new|renames|is[ \\t]+separate)([ \\t].*)?$\\n\"\n> \n\n"},{"id":"432455","messageId":"95ebb2cf-2e6e-912e-7d80-3947a8e3d9e4@kdbg.org","threadId":"56249","inReplyTo":"87zgtoh6bm.fsf@gnu.org","subject":"Re: [PATCH v4] userdiff: improve java hunk header regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-08-11T07:34:22Z","receivedAt":"2021-08-11T07:34:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.08.21 um 07:22 schrieb Tassilo Horn:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>> These new tests are very much appreciated. You do not have to go wild\n>> with that many return type tests; IMO, the simple one and the most\n>> complicated one should do it. (And btw, s/cart/card/)\n> \n> Well, they appeared naturally as a result during development and made it\n> easier to spot errors when you know up to which level of complexity it\n> still worked.  Is there a stronger reason to remove tests which might\n> not be needed, e.g., runtime cost on some CI machines?\n\nI totally understand how the test cases evolved. Having many of them is\nnot a big deal. It's just the disproportion of tests of this new feature\nvs. the existing tests that your patch creates, in particular, when\nearlier of the new tests are subsumed by later new tests.\n\n> Another thing I've noticed (with my suggested patch) is that I should\n> not try to match constructor signatures.  I think that's impossible\n> because they are indistinguishable from method calls, e.g., in\n> \n>   public class MyClass {\n>       MyClass(String RIGHT) {\n>           someMethodCall();\n>           someOtherMethod(17)\n>               .doThat();\n>           // Whatever\n>           // ChangeMe\n>       }\n>   }\n> \n> there is no regex way to prefer MyClass(String RIGHT) over\n> someOtherMethod().\n\nGood find.\n\n> So all in all, I'd propose this version in the next patch version:\n> \n> --8<---------------cut here---------------start------------->8---\n> PATTERNS(\"java\",\n> \t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n>          \"^[ \\t]*(\"\n>          /* Class, enum, and interface declarations */\n>          \"(([a-z]+[ \\t]+)*(class|enum|interface)[ \\t]+[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n>          /* Method definitions; note that constructor signatures are not */\n>          /* matched because they are indistinguishable from method calls. */\n>          \"|(([A-Za-z_<>&][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n>          \")$\",\n> \t /* -- */\n> \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n> \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n> \t \"|[-+*/<>%&^|=!]=\"\n> \t \"|--|\\\\+\\\\+|<<=?|>>>?=?|&&|\\\\|\\\\|\"),\n> --8<---------------cut here---------------end--------------->8---\n\nThat looks fine.\n\nOne suggestion, though. You do not have to have all positive patterns\n(\"class, enum, interface\" and \"method definitions\") in a single pattern\nseparated by \"|\". You can place them on different \"lines\" (note the \"\\n\"\nat the end of the first pattern):\n\n\t/* Class, enum, and interface declarations */\n\t\"^[ \\t]*(...(class|enum|interface)...)$\\n\"\n\t/*\n\t * Method definitions; note that constructor signatures are not\n\t * matched because they are indistinguishable from method calls.\n\t */\n\t\"^[ \\t]*(...[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*))$\",\n\nI don't think there is a technical difference, but I find this form\neasier to understand because fewer open parentheses have to be tracked.\n\n-- Hannes\n"},{"id":"432464","messageId":"87wnosh0gz.fsf@gnu.org","threadId":"56249","inReplyTo":"95ebb2cf-2e6e-912e-7d80-3947a8e3d9e4@kdbg.org","subject":"Re: [PATCH v4] userdiff: improve java hunk header regex","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2021-08-11T07:39:02Z","receivedAt":"2021-08-11T08:07:58Z","isPatch":true,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\nHi Hannes,\n\n>>> These new tests are very much appreciated. You do not have to go\n>>> wild with that many return type tests; IMO, the simple one and the\n>>> most complicated one should do it. (And btw, s/cart/card/)\n>> \n>> Well, they appeared naturally as a result during development and made\n>> it easier to spot errors when you know up to which level of\n>> complexity it still worked.  Is there a stronger reason to remove\n>> tests which might not be needed, e.g., runtime cost on some CI\n>> machines?\n>\n> I totally understand how the test cases evolved. Having many of them\n> is not a big deal. It's just the disproportion of tests of this new\n> feature vs. the existing tests that your patch creates, in particular,\n> when earlier of the new tests are subsumed by later new tests.\n\nSure thing, I'll see if I can remove some tests.\n\n>> Another thing I've noticed (with my suggested patch) is that I should\n>> not try to match constructor signatures.  I think that's impossible\n>> because they are indistinguishable from method calls, e.g., in\n>> \n>>   public class MyClass {\n>>       MyClass(String RIGHT) {\n>>           someMethodCall();\n>>           someOtherMethod(17)\n>>               .doThat();\n>>           // Whatever\n>>           // ChangeMe\n>>       }\n>>   }\n>> \n>> there is no regex way to prefer MyClass(String RIGHT) over\n>> someOtherMethod().\n>\n> Good find.\n\nThe longer you play with it, the more you find out.\n\n>> So all in all, I'd propose this version in the next patch version:\n>> \n>> --8<---------------cut here---------------start------------->8---\n>> PATTERNS(\"java\",\n>> \t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n>>          \"^[ \\t]*(\"\n>>          /* Class, enum, and interface declarations */\n>>          \"(([a-z]+[ \\t]+)*(class|enum|interface)[ \\t]+[A-Za-z][A-Za-z0-9_$]*[ \\t]+.*)\"\n>>          /* Method definitions; note that constructor signatures are not */\n>>          /* matched because they are indistinguishable from method calls. */\n>>          \"|(([A-Za-z_<>&][][?&<>.,A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)\"\n>>          \")$\",\n>> \t /* -- */\n>> \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n>> \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n>> \t \"|[-+*/<>%&^|=!]=\"\n>> \t \"|--|\\\\+\\\\+|<<=?|>>>?=?|&&|\\\\|\\\\|\"),\n>> --8<---------------cut here---------------end--------------->8---\n>\n> That looks fine.\n>\n> One suggestion, though. You do not have to have all positive patterns\n> (\"class, enum, interface\" and \"method definitions\") in a single\n> pattern separated by \"|\". You can place them on different \"lines\"\n> (note the \"\\n\" at the end of the first pattern):\n>\n> \t/* Class, enum, and interface declarations */\n> \t\"^[ \\t]*(...(class|enum|interface)...)$\\n\"\n> \t/*\n> \t * Method definitions; note that constructor signatures are not\n> \t * matched because they are indistinguishable from method calls.\n> \t */\n> \t\"^[ \\t]*(...[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*))$\",\n>\n> I don't think there is a technical difference, but I find this form\n> easier to understand because fewer open parentheses have to be\n> tracked.\n\nYes, indeed.  Because of that reason I've put the first ( and the last )\non separate lines but your approach is even better.\n\nPatch version v5 will come anytime soon.\n\nThanks!\nTassilo\n"},{"id":"432490","messageId":"xmqqv94c9ddm.fsf@gitster.g","threadId":"56249","inReplyTo":"157a0c35-1c82-9a2e-3bcd-ae6059ec71bd@kdbg.org","subject":"Re: Re* [PATCH v4] userdiff: improve java hunk header regex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-11T16:04:21Z","receivedAt":"2021-08-11T16:04:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> + * When writing or updating patterns, assume that the contents these\n>> + * patterns are applied to are syntactically correct.  You do not have\n>> + * to implement all syntactical corner cases---the patterns have to be\n>> + * sufficiently permissive.\n>> + */\n>\n> IMO, as written, the comment falls short of suggesting that patterns can\n> be simple. How about appending \"and can be simple\"?\n\n    The patterns can be simple without implementing all syntactical\n    corner cases, as long as they are sufficiently permissive.\n\nperhaps?\n\nThanks.\n\n\n"},{"id":"432516","messageId":"adb3a9cd-f5f6-5746-eb52-d12f6b88a995@kdbg.org","threadId":"56249","inReplyTo":"xmqqv94c9ddm.fsf@gitster.g","subject":"Re: Re* [PATCH v4] userdiff: improve java hunk header regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-08-11T20:32:24Z","receivedAt":"2021-08-11T20:32:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.08.21 um 18:04 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>>> + * When writing or updating patterns, assume that the contents these\n>>> + * patterns are applied to are syntactically correct.  You do not have\n>>> + * to implement all syntactical corner cases---the patterns have to be\n>>> + * sufficiently permissive.\n>>> + */\n>>\n>> IMO, as written, the comment falls short of suggesting that patterns can\n>> be simple. How about appending \"and can be simple\"?\n> \n>     The patterns can be simple without implementing all syntactical\n>     corner cases, as long as they are sufficiently permissive.\n> \n> perhaps?\n\nPerfect! Thank you.\n\n-- Hannes\n"}]}