{"thread":{"id":"51972","subject":"[PATCH] userdiff: Fix some corner cases in dts regex","startedAt":"2019-10-04T21:30:32Z","lastAt":"2019-10-16T20:24:25Z","messageCount":5,"participants":["Stephen Boyd","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"383443","messageId":"20191004213029.145027-1-sboyd@kernel.org","threadId":"51972","inReplyTo":null,"subject":"[PATCH] userdiff: Fix some corner cases in dts regex","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-10-04T21:30:29Z","receivedAt":"2019-10-04T21:30:32Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"While reviewing some dts diffs recently I noticed that the hunk header\nlogic was failing to find the containing node. This is because the regex\ndoesn't consider properties that may span multiple lines, i.e.\n\n\tproperty = ,\n\t\t ;\n\nand it got hung up on comments inside nodes that look like the root node\nbecause they start with '/*'. Add tests for these cases and update the\nregex to find them. Maybe detecting the root node is too complicated but\nforcing it to be a backslash with any amount of whitespace up to an open\nbracket seemed OK. I tried to detect that a comment is in-between the\ntwo parts but I wasn't happy so I just dropped it.\n\nCc: Rob Herring \nCc: Frank Rowand \nSigned-off-by: Stephen Boyd \n---\n t/t4018/dts-nodes-multiline-prop | 12 ++++++++++++\n t/t4018/dts-root | 2 +-\n t/t4018/dts-root-comment | 8 ++++++++\n userdiff.c | 3 ++-\n 4 files changed, 23 insertions(+), 2 deletions(-)\n create mode 100644 t/t4018/dts-nodes-multiline-prop\n create mode 100644 t/t4018/dts-root-comment\n\ndiff --git a/t/t4018/dts-nodes-multiline-prop b/t/t4018/dts-nodes-multiline-prop\nnew file mode 100644\nindex 000000000000..f7b655935429\n--- /dev/null\n+++ b/t/t4018/dts-nodes-multiline-prop\n@@ -0,0 +1,12 @@\n+/ {\n+\tlabel_1: node1@ff00 {\n+\t\tRIGHT@deadf00,4000 {\n+\t\t\tmultilineprop = <3>,\n+\t\t\t\t\t<4>;\n+\n+\n+\n+\t\t\tChangeMe = <0xffeedd00>;\n+\t\t};\n+\t};\n+};\ndiff --git a/t/t4018/dts-root b/t/t4018/dts-root\nindex 2ef9e6ffaa2c..4353b8220c91 100644\n--- a/t/t4018/dts-root\n+++ b/t/t4018/dts-root\n@@ -1,4 +1,4 @@\n-/RIGHT { /* Technically just supposed to be a slash */\n+/ { RIGHT /* Technically just supposed to be a slash and brace */\n \t#size-cells = <1>;\n \n \tChangeMe = <0xffeedd00>;\ndiff --git a/t/t4018/dts-root-comment b/t/t4018/dts-root-comment\nnew file mode 100644\nindex 000000000000..333a625c7007\n--- /dev/null\n+++ b/t/t4018/dts-root-comment\n@@ -0,0 +1,8 @@\n+/ { RIGHT /* Technically just supposed to be a slash and brace */\n+\t#size-cells = <1>;\n+\n+\t/* This comment should be ignored */\n+\n+\tsome-property = <40+2>;\n+\tChangeMe = <0xffeedd00>;\n+};\ndiff --git a/userdiff.c b/userdiff.c\nindex 86e3244e15dd..651b56caec56 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -25,8 +25,9 @@ IPATTERN(\"ada\",\n \t \"|=>|\\\\.\\\\.|\\\\*\\\\*|:=|/=|>=|<=|<<|>>|<>\"),\n PATTERNS(\"dts\",\n \t \"!;\\n\"\n+\t \"!.*=.*\\n\"\n \t /* lines beginning with a word optionally preceded by '&' or the root */\n-\t \"^[ \\t]*((/|&?[a-zA-Z_]).*)\",\n+\t \"^[ \\t]*((/[ \\t]*\\\\{|&?[a-zA-Z_]).*)\",\n \t /* -- */\n \t /* Property names and math operators */\n \t \"[a-zA-Z0-9,._+?#-]+\"\n-- \nSent by a computer through tubes\n\n"},{"id":"383466","messageId":"c3a054d9-2646-e440-40c5-b0aecf21e690@kdbg.org","threadId":"51972","inReplyTo":"20191004213029.145027-1-sboyd@kernel.org","subject":"Re: [PATCH] userdiff: Fix some corner cases in dts regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-10-05T14:09:11Z","receivedAt":"2019-10-05T14:09:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.10.19 um 23:30 schrieb Stephen Boyd:\n> While reviewing some dts diffs recently I noticed that the hunk header\n> logic was failing to find the containing node. This is because the regex\n> doesn't consider properties that may span multiple lines, i.e.\n> \n> \tproperty = ,\n> \t\t ;\n\nWhat if the property spans more than two lines?\n\n\tproperty = ,\n\t\t more,\n\t\t ;\n\nCan the second line \"more,\" begin with a word, or are the angle brackets\nmandatory?\n\nI understand that the continuation lines can begin with a word when the\nproperty is an expression that is distributed over a number of lines.\nSuch continuation lines could be picked up as hunk headers.\n\nBut I don't want to complicate things: The hunk header patterns do not\nhave to be perfect; it is sufficient when they are helpful in a good\nmajority of cases that occur in practice.\n\n> and it got hung up on comments inside nodes that look like the root node\n> because they start with '/*'. Add tests for these cases and update the\n> regex to find them. Maybe detecting the root node is too complicated but\n> forcing it to be a backslash with any amount of whitespace up to an open\n> bracket seemed OK. I tried to detect that a comment is in-between the\n> two parts but I wasn't happy so I just dropped it.\n> \n> Cc: Rob Herring \n> Cc: Frank Rowand \n> Signed-off-by: Stephen Boyd \n> ---\n> t/t4018/dts-nodes-multiline-prop | 12 ++++++++++++\n> t/t4018/dts-root | 2 +-\n> t/t4018/dts-root-comment | 8 ++++++++\n> userdiff.c | 3 ++-\n> 4 files changed, 23 insertions(+), 2 deletions(-)\n> create mode 100644 t/t4018/dts-nodes-multiline-prop\n> create mode 100644 t/t4018/dts-root-comment\n> \n> diff --git a/t/t4018/dts-nodes-multiline-prop b/t/t4018/dts-nodes-multiline-prop\n> new file mode 100644\n> index 000000000000..f7b655935429\n> --- /dev/null\n> +++ b/t/t4018/dts-nodes-multiline-prop\n> @@ -0,0 +1,12 @@\n> +/ {\n> +\tlabel_1: node1@ff00 {\n> +\t\tRIGHT@deadf00,4000 {\n> +\t\t\tmultilineprop = <3>,\n> +\t\t\t\t\t<4>;\n\nYou could insert more lines to demonstrate that \",\" on a line by\nitself is not picked up.\n\n> +\n> +\n> +> +\t\t\tChangeMe = <0xffeedd00>;\n\nSufficient distance to the incorrect candidates above. Good.\n\n> +\t\t};\n> +\t};\n> +};\n> diff --git a/t/t4018/dts-root b/t/t4018/dts-root\n> index 2ef9e6ffaa2c..4353b8220c91 100644\n> --- a/t/t4018/dts-root\n> +++ b/t/t4018/dts-root\n> @@ -1,4 +1,4 @@\n> -/RIGHT { /* Technically just supposed to be a slash */\n> +/ { RIGHT /* Technically just supposed to be a slash and brace */\n\nDo I understand correctly that the updated form, \"/ {\", is the common\nway to spell a root node, but \"/\" or \"/word\" are not?\n\n> \t#size-cells = <1>;\n> \n> \tChangeMe = <0xffeedd00>;\n> diff --git a/t/t4018/dts-root-comment b/t/t4018/dts-root-comment\n> new file mode 100644\n> index 000000000000..333a625c7007\n> --- /dev/null\n> +++ b/t/t4018/dts-root-comment\n> @@ -0,0 +1,8 @@\n> +/ { RIGHT /* Technically just supposed to be a slash and brace */\n\nDevil's advocate here: insert ';' or '=' in the comment, and the line\nwould not be picked up. Does that hurt in practice?\n\n> +\t#size-cells = <1>;\n> +\n> +\t/* This comment should be ignored */\n> +\n> +\tsome-property = <40+2>;\n> +\tChangeMe = <0xffeedd00>;\n> +};\n> diff --git a/userdiff.c b/userdiff.c\n> index 86e3244e15dd..651b56caec56 100644\n> --- a/userdiff.c\n> +++ b/userdiff.c\n> @@ -25,8 +25,9 @@ IPATTERN(\"ada\",\n> \t \"|=>|\\\\.\\\\.|\\\\*\\\\*|:=|/=|>=|<=|<<|>>|<>\"),\n> PATTERNS(\"dts\",\n> \t \"!;\\n\"\n> +\t \"!.*=.*\\n\"\n\nThis behaves the same way as just\n\n\t\"!=\\n\"\n\nno?\n\n> \t /* lines beginning with a word optionally preceded by '&' or the root */\n> -\t \"^[ \\t]*((/|&?[a-zA-Z_]).*)\",\n> +\t \"^[ \\t]*((/[ \\t]*\\\\{|&?[a-zA-Z_]).*)\",\n> \t /* -- */\n> \t /* Property names and math operators */\n> \t \"[a-zA-Z0-9,._+?#-]+\"\n> \n\n-- Hannes\n"},{"id":"383685","messageId":"20191008144306.B2B0820659@mail.kernel.org","threadId":"51972","inReplyTo":"c3a054d9-2646-e440-40c5-b0aecf21e690@kdbg.org","subject":"Re: [PATCH] userdiff: Fix some corner cases in dts regex","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-10-08T14:43:05Z","receivedAt":"2019-10-08T14:43:09Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Quoting Johannes Sixt (2019-10-05 07:09:11)\n> Am 04.10.19 um 23:30 schrieb Stephen Boyd:\n> > While reviewing some dts diffs recently I noticed that the hunk header\n> > logic was failing to find the containing node. This is because the regex\n> > doesn't consider properties that may span multiple lines, i.e.\n> > \n> > property = ,\n> > ;\n> \n> What if the property spans more than two lines?\n> \n> property = ,\n> more,\n> ;\n> \n> Can the second line \"more,\" begin with a word, or are the angle brackets\n> mandatory?\n\nAngle brackets aren't mandatory, but it is very odd to have a property\nwith mixed numbers and strings because parsing becomes difficult.\n\n> \n> I understand that the continuation lines can begin with a word when the\n> property is an expression that is distributed over a number of lines.\n> Such continuation lines could be picked up as hunk headers.\n> \n> But I don't want to complicate things: The hunk header patterns do not\n> have to be perfect; it is sufficient when they are helpful in a good\n> majority of cases that occur in practice.\n> \n> > and it got hung up on comments inside nodes that look like the root node\n> > because they start with '/*'. Add tests for these cases and update the\n> > regex to find them. Maybe detecting the root node is too complicated but\n> > forcing it to be a backslash with any amount of whitespace up to an open\n> > bracket seemed OK. I tried to detect that a comment is in-between the\n> > two parts but I wasn't happy so I just dropped it.\n> > \n> > Cc: Rob Herring \n> > Cc: Frank Rowand \n> > Signed-off-by: Stephen Boyd \n> > ---\n> > t/t4018/dts-nodes-multiline-prop | 12 ++++++++++++\n> > t/t4018/dts-root | 2 +-\n> > t/t4018/dts-root-comment | 8 ++++++++\n> > userdiff.c | 3 ++-\n> > 4 files changed, 23 insertions(+), 2 deletions(-)\n> > create mode 100644 t/t4018/dts-nodes-multiline-prop\n> > create mode 100644 t/t4018/dts-root-comment\n> > \n> > diff --git a/t/t4018/dts-nodes-multiline-prop b/t/t4018/dts-nodes-multiline-prop\n> > new file mode 100644\n> > index 000000000000..f7b655935429\n> > --- /dev/null\n> > +++ b/t/t4018/dts-nodes-multiline-prop\n> > @@ -0,0 +1,12 @@\n> > +/ {\n> > + label_1: node1@ff00 {\n> > + RIGHT@deadf00,4000 {\n> > + multilineprop = <3>,\n> > + <4>;\n> \n> You could insert more lines to demonstrate that \",\" on a line by\n> itself is not picked up.\n\nMaybe I should add another test?\n\n> \n> > +\n> > +\n> > +> + ChangeMe = <0xffeedd00>;\n> \n> Sufficient distance to the incorrect candidates above. Good.\n> \n> > + };\n> > + };\n> > +};\n> > diff --git a/t/t4018/dts-root b/t/t4018/dts-root\n> > index 2ef9e6ffaa2c..4353b8220c91 100644\n> > --- a/t/t4018/dts-root\n> > +++ b/t/t4018/dts-root\n> > @@ -1,4 +1,4 @@\n> > -/RIGHT { /* Technically just supposed to be a slash */\n> > +/ { RIGHT /* Technically just supposed to be a slash and brace */\n> \n> Do I understand correctly that the updated form, \"/ {\", is the common\n> way to spell a root node, but \"/\" or \"/word\" are not?\n\nCorrect. A root node is '/' and the '{' opens the node. There is the\npossibility of something like '/delete-node nodename;' or\n'/delete-property property;', where the latter exists inside some node.\nThe regex would need to avoid all of those.\n\n> \n> > #size-cells = <1>;\n> > \n> > ChangeMe = <0xffeedd00>;\n> > diff --git a/t/t4018/dts-root-comment b/t/t4018/dts-root-comment\n> > new file mode 100644\n> > index 000000000000..333a625c7007\n> > --- /dev/null\n> > +++ b/t/t4018/dts-root-comment\n> > @@ -0,0 +1,8 @@\n> > +/ { RIGHT /* Technically just supposed to be a slash and brace */\n> \n> Devil's advocate here: insert ';' or '=' in the comment, and the line\n> would not be picked up. Does that hurt in practice?\n\nI don't think it hurts in practice so I'd like to ignore it.\n\n> \n> > + #size-cells = <1>;\n> > +\n> > + /* This comment should be ignored */\n> > +\n> > + some-property = <40+2>;\n> > + ChangeMe = <0xffeedd00>;\n> > +};\n> > diff --git a/userdiff.c b/userdiff.c\n> > index 86e3244e15dd..651b56caec56 100644\n> > --- a/userdiff.c\n> > +++ b/userdiff.c\n> > @@ -25,8 +25,9 @@ IPATTERN(\"ada\",\n> > \"|=>|\\\\.\\\\.|\\\\*\\\\*|:=|/=|>=|<=|<<|>>|<>\"),\n> > PATTERNS(\"dts\",\n> > \"!;\\n\"\n> > + \"!.*=.*\\n\"\n> \n> This behaves the same way as just\n> \n> \"!=\\n\"\n> \n> no?\n> \n\nNot exactly. Properties don't always get assigned. There are boolean\nproperties that can be tested for by the presence of some string with an\nending semi-colon, like 'this-is-true;'. If we just check for not equal\nto a line with a semicolon and newline then we'll see boolean\nproperties. Should I add that as another test?\n\n"},{"id":"383967","messageId":"ab9b0dba-5c5b-5a97-07c5-ce8344cd74cd@kdbg.org","threadId":"51972","inReplyTo":"20191008144306.B2B0820659@mail.kernel.org","subject":"Re: [PATCH] userdiff: Fix some corner cases in dts regex","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-10-12T12:54:00Z","receivedAt":"2019-10-12T12:54:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.10.19 um 16:43 schrieb Stephen Boyd:\n> Quoting Johannes Sixt (2019-10-05 07:09:11)\n>> Am 04.10.19 um 23:30 schrieb Stephen Boyd:\n>>> --- /dev/null\n>>> +++ b/t/t4018/dts-nodes-multiline-prop\n>>> @@ -0,0 +1,12 @@\n>>> +/ {\n>>> + label_1: node1@ff00 {\n>>> + RIGHT@deadf00,4000 {\n>>> + multilineprop = <3>,\n>>> + <4>;\n>>\n>> You could insert more lines to demonstrate that \",\" on a line by\n>> itself is not picked up.\n> \n> Maybe I should add another test?\n\nThis is is the _multi_line test case, right? ;) Just add one or two\nlines between the <3> and the <4> that look like common real-world cases\nto show that those lines won't be picked up. I don't think that another\ntest file is required.\n\n>>> +/ { RIGHT /* Technically just supposed to be a slash and brace */\n>>\n>> Devil's advocate here: insert ';' or '=' in the comment, and the line\n>> would not be picked up. Does that hurt in practice?\n> \n> I don't think it hurts in practice so I'd like to ignore it.\n\nSure, no problem.\n\n>>> PATTERNS(\"dts\",\n>>> \"!;\\n\"\n>>> + \"!.*=.*\\n\"\n>>\n>> This behaves the same way as just\n>>\n>> \"!=\\n\"\n>>\n>> no?\n>>\n> \n> Not exactly. Properties don't always get assigned.\n\nI was just refering to the added line, not the combination of the two lines.\n\nBut while you are speaking of it:\n\n> There are boolean\n> properties that can be tested for by the presence of some string with an\n> ending semi-colon, like 'this-is-true;'. If we just check for not equal\n> to a line with a semicolon and newline then we'll see boolean\n> properties. Should I add that as another test?\n\nI agree that a test case with a Boolean property would be great.\n\n-- Hannes\n"},{"id":"384212","messageId":"20191016202423.3B5FF21835@mail.kernel.org","threadId":"51972","inReplyTo":"ab9b0dba-5c5b-5a97-07c5-ce8344cd74cd@kdbg.org","subject":"Re: [PATCH] userdiff: Fix some corner cases in dts regex","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-10-16T20:24:22Z","receivedAt":"2019-10-16T20:24:25Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Quoting Johannes Sixt (2019-10-12 05:54:00)\n> Am 08.10.19 um 16:43 schrieb Stephen Boyd:\n> > Quoting Johannes Sixt (2019-10-05 07:09:11)\n> >> Am 04.10.19 um 23:30 schrieb Stephen Boyd:\n> >>> --- /dev/null\n> >>> +++ b/t/t4018/dts-nodes-multiline-prop\n> >>> @@ -0,0 +1,12 @@\n> >>> +/ {\n> >>> + label_1: node1@ff00 {\n> >>> + RIGHT@deadf00,4000 {\n> >>> + multilineprop = <3>,\n> >>> + <4>;\n> >>\n> >> You could insert more lines to demonstrate that \",\" on a line by\n> >> itself is not picked up.\n> > \n> > Maybe I should add another test?\n> \n> This is is the _multi_line test case, right? ;) Just add one or two\n> lines between the <3> and the <4> that look like common real-world cases\n> to show that those lines won't be picked up. I don't think that another\n> test file is required.\n\nOk got it!\n\n> \n> >>> +/ { RIGHT /* Technically just supposed to be a slash and brace */\n> >>\n> >> Devil's advocate here: insert ';' or '=' in the comment, and the line\n> >> would not be picked up. Does that hurt in practice?\n> > \n> > I don't think it hurts in practice so I'd like to ignore it.\n> \n> Sure, no problem.\n> \n> >>> PATTERNS(\"dts\",\n> >>> \"!;\\n\"\n> >>> + \"!.*=.*\\n\"\n> >>\n> >> This behaves the same way as just\n> >>\n> >> \"!=\\n\"\n> >>\n> >> no?\n> >>\n> > \n> > Not exactly. Properties don't always get assigned.\n> \n> I was just refering to the added line, not the combination of the two lines.\n\nAh ok. I'll reduce the line as you suggest then. Thanks.\n\n> \n> But while you are speaking of it:\n> \n> > There are boolean\n> > properties that can be tested for by the presence of some string with an\n> > ending semi-colon, like 'this-is-true;'. If we just check for not equal\n> > to a line with a semicolon and newline then we'll see boolean\n> > properties. Should I add that as another test?\n> \n> I agree that a test case with a Boolean property would be great.\n> \n\nAlright I'll work on that and resend.\n\n"}]}